opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-3.0' into stable-3.1

Browse Source 
* 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
This commit is contained in:
David Pursehouse
2019-11-01 08:46:48 +09:00
parent b7b1472a5a 9790275c12
commit 31f45164e9
8 changed files with 29 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
java/com/google/gerrit/elasticsearch/builders/XContentBuilder.java
View File

@@ -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 {

7
java/com/google/gerrit/gpg/server/DeleteGpgKey.java
View File

@@ -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();

5
java/com/google/gerrit/gpg/server/PostGpgKeys.java
View File

@@ -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;

2
java/com/google/gerrit/pgm/init/BaseInit.java
View File

@@ -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);

6
java/com/google/gerrit/server/restapi/project/DeleteRef.java
View File

@@ -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()));
}
}
}

2
javatests/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java
View File

@@ -41,7 +41,7 @@ public class ElasticReindexIT extends AbstractReindexTests {
}
@Override
public void configureIndex(Injector injector) throws Exception {
public void configureIndex(Injector injector) {
createAllIndexes(injector);
}

13
javatests/com/google/gerrit/acceptance/ssh/AbstractIndexTests.java
View File

@@ -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 {

1
tools/bzl/genrule2.bzl
View File

@@ -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,