Merge branch 'stable-3.0' into stable-3.1

* stable-3.0:
  Post/Delete GPG keys: Do not return 409 Conflict if an error occurred
  DeleteRef: Do not return 409 Conflict if an error occurred
  DeleteRef: Do not log an error if user attempted to delete the current branch
  Init: Increase severity log level to error on exceptions
  Bazel: Fix genrule2 to clean up temporary folder content
  AbstractIndexTests: Inline constant assertChangeQuery string parameter
  AbstractIndexTests: Align assertChangeQuery method access with its use
  AbstractIndexTests: Provide default configureIndex method implementation
  XContentBuilder: Use JsonFactory.builder() and new feature enums

Change-Id: Id3499febf123f2f3aa872d589b5df478d1d66b63

8 files changed, 29 insertions, 26 deletions

diff --git a/java/com/google/gerrit/elasticsearch/builders/XContentBuilder.java b/java/com/google/gerrit/elasticsearch/builders/XContentBuilder.java
index 06427f1079..061a37321a 100644
--- a/java/com/google/gerrit/elasticsearch/builders/XContentBuilder.java
+++ b/java/com/google/gerrit/elasticsearch/builders/XContentBuilder.java
@@ -19,7 +19,8 @@ import static java.time.format.DateTimeFormatter.ISO_INSTANT;
 import com.fasterxml.jackson.core.JsonEncoding;
 import com.fasterxml.jackson.core.JsonFactory;
 import com.fasterxml.jackson.core.JsonGenerator;
-import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.json.JsonReadFeature;
+import com.fasterxml.jackson.core.json.JsonWriteFeature;
 import com.google.common.base.Charsets;
 import java.io.ByteArrayOutputStream;
 import java.io.Closeable;
@@ -38,14 +39,14 @@ public final class XContentBuilder implements Closeable {
    * Inspired from org.elasticsearch.common.xcontent.json.JsonXContent static block.
    */
   public XContentBuilder() throws IOException {
-    JsonFactory jsonFactory = new JsonFactory();
-    jsonFactory.configure(JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES, true);
-    jsonFactory.configure(JsonGenerator.Feature.QUOTE_FIELD_NAMES, true);
-    jsonFactory.configure(JsonParser.Feature.ALLOW_COMMENTS, true);
-    jsonFactory.configure(
-        JsonFactory.Feature.FAIL_ON_SYMBOL_HASH_OVERFLOW,
-        false); // this trips on many mappings now...
-    this.generator = jsonFactory.createGenerator(bos, JsonEncoding.UTF8);
+    this.generator =
+        JsonFactory.builder()
+            .configure(JsonReadFeature.ALLOW_UNQUOTED_FIELD_NAMES, true)
+            .configure(JsonWriteFeature.QUOTE_FIELD_NAMES, true)
+            .configure(JsonReadFeature.ALLOW_JAVA_COMMENTS, true)
+            .configure(JsonFactory.Feature.FAIL_ON_SYMBOL_HASH_OVERFLOW, false)
+            .build()
+            .createGenerator(bos, JsonEncoding.UTF8);
   }
 
   public XContentBuilder startObject(String name) throws IOException {
diff --git a/java/com/google/gerrit/gpg/server/DeleteGpgKey.java b/java/com/google/gerrit/gpg/server/DeleteGpgKey.java
index 24bfd4f854..6233605e15 100644
--- a/java/com/google/gerrit/gpg/server/DeleteGpgKey.java
+++ b/java/com/google/gerrit/gpg/server/DeleteGpgKey.java
@@ -21,8 +21,8 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.flogger.FluentLogger;
 import com.google.common.io.BaseEncoding;
 import com.google.gerrit.exceptions.EmailException;
+import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.common.Input;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
@@ -108,9 +108,10 @@ public class DeleteGpgKey implements RestModifyView<GpgKey, Input> {
                 rsrc.getUser().getAccount().preferredEmail());
           }
           break;
+        case LOCK_FAILURE:
+          // should not happen since this case is already handled by PublicKeyStore#save
         case FORCED:
         case IO_FAILURE:
-        case LOCK_FAILURE:
         case NEW:
         case NOT_ATTEMPTED:
         case REJECTED:
@@ -119,7 +120,7 @@ public class DeleteGpgKey implements RestModifyView<GpgKey, Input> {
         case REJECTED_MISSING_OBJECT:
         case REJECTED_OTHER_REASON:
         default:
-          throw new ResourceConflictException("Failed to delete public key: " + saveResult);
+          throw new StorageException(String.format("Failed to delete public key: %s", saveResult));
       }
     }
     return Response.none();
diff --git a/java/com/google/gerrit/gpg/server/PostGpgKeys.java b/java/com/google/gerrit/gpg/server/PostGpgKeys.java
index ac5209e26e..f4a165571a 100644
--- a/java/com/google/gerrit/gpg/server/PostGpgKeys.java
+++ b/java/com/google/gerrit/gpg/server/PostGpgKeys.java
@@ -274,8 +274,9 @@ public class PostGpgKeys implements RestModifyView<AccountResource, GpgKeysInput
           break;
         case NO_CHANGE:
           break;
-        case IO_FAILURE:
         case LOCK_FAILURE:
+          // should not happen since this case is already handled by PublicKeyStore#save
+        case IO_FAILURE:
         case NOT_ATTEMPTED:
         case REJECTED:
         case REJECTED_CURRENT_BRANCH:
@@ -283,7 +284,7 @@ public class PostGpgKeys implements RestModifyView<AccountResource, GpgKeysInput
         case REJECTED_MISSING_OBJECT:
         case REJECTED_OTHER_REASON:
         default:
-          throw new ResourceConflictException("Failed to save public keys: " + saveResult);
+          throw new StorageException(String.format("Failed to save public keys: %s", saveResult));
       }
     }
     return null;
diff --git a/java/com/google/gerrit/pgm/init/BaseInit.java b/java/com/google/gerrit/pgm/init/BaseInit.java
index 008969e674..7fbf2d7a5d 100644
--- a/java/com/google/gerrit/pgm/init/BaseInit.java
+++ b/java/com/google/gerrit/pgm/init/BaseInit.java
@@ -123,7 +123,7 @@ public class BaseInit extends SiteProgram {
         } catch (StorageException e) {
           String msg = "Couldn't upgrade schema. Expected if slave and read-only database";
           System.err.println(msg);
-          logger.atWarning().withCause(e).log(msg);
+          logger.atSevere().withCause(e).log(msg);
         }
 
         init.initializer.postRun(sysInjector);
diff --git a/java/com/google/gerrit/server/restapi/project/DeleteRef.java b/java/com/google/gerrit/server/restapi/project/DeleteRef.java
index 805a4d7c92..1979d611ed 100644
--- a/java/com/google/gerrit/server/restapi/project/DeleteRef.java
+++ b/java/com/google/gerrit/server/restapi/project/DeleteRef.java
@@ -27,6 +27,7 @@ import com.google.common.flogger.FluentLogger;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Project;
+import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.server.IdentifiedUser;
@@ -156,7 +157,7 @@ public class DeleteRef {
           break;
 
         case REJECTED_CURRENT_BRANCH:
-          logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
+          logger.atFine().log("Cannot delete current branch %s: %s", ref, result.name());
           throw new ResourceConflictException("cannot delete current branch");
 
         case IO_FAILURE:
@@ -167,8 +168,7 @@ public class DeleteRef {
         case REJECTED_MISSING_OBJECT:
         case REJECTED_OTHER_REASON:
         default:
-          logger.atSevere().log("Cannot delete %s: %s", ref, result.name());
-          throw new ResourceConflictException("cannot delete: " + result.name());
+          throw new StorageException(String.format("Cannot delete %s: %s", ref, result.name()));
       }
     }
   }
diff --git a/javatests/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java b/javatests/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java
index c0f27bd208..dcdca860ff 100644
--- a/javatests/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java
+++ b/javatests/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java
@@ -41,7 +41,7 @@ public class ElasticReindexIT extends AbstractReindexTests {
   }
 
   @Override
-  public void configureIndex(Injector injector) throws Exception {
+  public void configureIndex(Injector injector) {
     createAllIndexes(injector);
   }
 
diff --git a/javatests/com/google/gerrit/acceptance/ssh/AbstractIndexTests.java b/javatests/com/google/gerrit/acceptance/ssh/AbstractIndexTests.java
index c841559416..4fe0df48f6 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/AbstractIndexTests.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/AbstractIndexTests.java
@@ -57,7 +57,7 @@ public abstract class AbstractIndexTests extends AbstractDaemonTest {
       disableChangeIndexWrites();
       amendChange(changeId, "second test", "test2.txt", "test2");
 
-      assertChangeQuery("message:second", change.getChange(), false);
+      assertChangeQuery(change.getChange(), false);
       enableChangeIndexWrites();
 
       changeIndexedCounter.clear();
@@ -67,7 +67,7 @@ public abstract class AbstractIndexTests extends AbstractDaemonTest {
 
       changeIndexedCounter.assertReindexOf(changeInfo, 1);
 
-      assertChangeQuery("message:second", change.getChange(), true);
+      assertChangeQuery(change.getChange(), true);
     }
   }
 
@@ -86,7 +86,7 @@ public abstract class AbstractIndexTests extends AbstractDaemonTest {
       disableChangeIndexWrites();
       amendChange(changeId, "second test", "test2.txt", "test2");
 
-      assertChangeQuery("message:second", change.getChange(), false);
+      assertChangeQuery(change.getChange(), false);
       enableChangeIndexWrites();
 
       changeIndexedCounter.clear();
@@ -103,13 +103,12 @@ public abstract class AbstractIndexTests extends AbstractDaemonTest {
 
       changeIndexedCounter.assertReindexOf(changeInfo, 1);
 
-      assertChangeQuery("message:second", change.getChange(), true);
+      assertChangeQuery(change.getChange(), true);
     }
   }
 
-  protected void assertChangeQuery(String q, ChangeData change, boolean assertTrue)
-      throws Exception {
-    List<Integer> ids = query(q).stream().map(c -> c._number).collect(toList());
+  private void assertChangeQuery(ChangeData change, boolean assertTrue) throws Exception {
+    List<Integer> ids = query("message:second").stream().map(c -> c._number).collect(toList());
     if (assertTrue) {
       assertThat(ids).contains(change.getId().get());
     } else {
diff --git a/tools/bzl/genrule2.bzl b/tools/bzl/genrule2.bzl
index 3113022104..d0b0969438 100644
--- a/tools/bzl/genrule2.bzl
+++ b/tools/bzl/genrule2.bzl
@@ -21,6 +21,7 @@ def genrule2(cmd, **kwargs):
         "ROOT=$$PWD",
         "TMP=$$(mktemp -d || mktemp -d -t bazel-tmp)",
         "(" + cmd + ")",
+        "rm -rf $$TMP",
     ])
     native.genrule(
         cmd = cmd,