From af3e16f61d334c49a820e12c27393cb0674247e7 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 22 Jun 2020 09:56:41 +0900 Subject: [PATCH 1/5] Update buildifier version to 3.2.1 Change-Id: Ib60c7f3570d36b22b45dd8107a19e50e3a626114 --- Documentation/dev-contributing.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/dev-contributing.txt b/Documentation/dev-contributing.txt index ed2561ab30..b2b95469c4 100644 --- a/Documentation/dev-contributing.txt +++ b/Documentation/dev-contributing.txt @@ -171,7 +171,7 @@ To format Java source code, Gerrit uses the link:https://github.com/google/google-java-format[`google-java-format`] tool (version 1.7), and to format Bazel BUILD, WORKSPACE and .bzl files the link:https://github.com/bazelbuild/buildtools/tree/master/buildifier[`buildifier`] -tool (version 3.0.0). Unused dependencies are found and removed using the +tool (version 3.2.1). Unused dependencies are found and removed using the link:https://github.com/bazelbuild/buildtools/tree/master/unused_deps[`unused_deps`] build tool, a sibling of `buildifier`. From 50739f48b8d07ff5e2df4ce68543384aad93f433 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 22 Jun 2020 13:04:09 +0900 Subject: [PATCH 2/5] MergeOp: Update comment reference to IntegrationException IntegrationException was replaced by IntegrationConflictException in change Ifdc9d122b. Change-Id: I6fcfd0b1e914c0dfa9726923abf4531c76421a34 --- java/com/google/gerrit/server/submit/MergeOp.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 9018d9db2c..f96b0c5518 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -635,13 +635,13 @@ public class MergeOp implements AutoCloseable { throw e; } - // BatchUpdate may have inadvertently wrapped an IntegrationException + // BatchUpdate may have inadvertently wrapped an IntegrationConflictException // thrown by some legacy SubmitStrategyOp code that intended the error // message to be user-visible. Copy the message from the wrapped // exception. // // If you happen across one of these, the correct fix is to convert the - // inner IntegrationException to a ResourceConflictException. + // inner IntegrationConflictException to a ResourceConflictException. if (e.getCause() instanceof IntegrationConflictException) { throw (IntegrationConflictException) e.getCause(); } From 357df96320b92efa1fa079ac495fc0244863c188 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 22 Jun 2020 13:09:55 +0900 Subject: [PATCH 3/5] Patch: Restore the forCode method This method was removed in change I1e11f3aa5, but it's used in a plugin at CollabNet. Add it back, with the @UsedAt annotation. Change-Id: Ic52ff40921a1106ed1d202067826f2622946038e Signed-off-by: David Pursehouse --- java/com/google/gerrit/entities/Patch.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/java/com/google/gerrit/entities/Patch.java b/java/com/google/gerrit/entities/Patch.java index 50f86fea4d..e47d1979a3 100644 --- a/java/com/google/gerrit/entities/Patch.java +++ b/java/com/google/gerrit/entities/Patch.java @@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.auto.value.AutoValue; import com.google.common.base.Splitter; import com.google.common.primitives.Ints; +import com.google.gerrit.common.UsedAt; import java.util.List; /** @@ -101,6 +102,16 @@ public final class Patch { public char getCode() { return code; } + + @UsedAt(UsedAt.Project.COLLABNET) + public static ChangeType forCode(char c) { + for (ChangeType s : ChangeType.values()) { + if (s.code == c) { + return s; + } + } + return null; + } } /** Type of formatting for this patch. */ From dadc193ebad3828bf757f6ecadb34b00c062e916 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 26 Nov 2019 15:36:06 +0100 Subject: [PATCH 4/5] Handle duplicate label values on project load and push of config updates If a project had a label definition where a value was defined twice getting the project info via REST failed [1]. With this change we now generate a validation error on load of the project.config file, but let the loading succeed (the duplicate value is filtered out). Hence getting the project info of such a project no longer fails. The new validation error also ensures that it is no longer possible to push updates of project.config files that introduce duplicate label values. [1] java.lang.IllegalStateException: Duplicate key 1 (attempted merging values Looks Good and Looks Good) at java.util.stream.Collectors.duplicateKeyException(Collectors.java:131) at java.util.stream.Collectors.lambda$uniqKeysMapAccumulator$1(Collectors.java:178) at java.util.stream.ReduceOps$3ReducingSink.accept(ReduceOps.java:169) at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1383) at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481) at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471) at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708) at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499) at com.google.gerrit.server.project.ProjectJson.format(ProjectJson.java:52) at com.google.gerrit.server.restapi.project.GetProject.apply(GetProject.java:37) at com.google.gerrit.server.restapi.project.GetProject.apply(GetProject.java:25) at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestReadViewWithRetry$3(RestApiServlet.java:723) at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78) at com.github.rholder.retry.Retryer.call(Retryer.java:160) at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:417) at com.google.gerrit.server.update.RetryHelper.executeWithAttemptAndTimeoutCount(RetryHelper.java:368) at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:271) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:820) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestReadViewWithRetry(RestApiServlet.java:718) at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:501) at javax.servlet.http.HttpServlet.service(HttpServlet.java:717) ... Bug: Issue 12952 Signed-off-by: Edwin Kempin Change-Id: I90e5439a00ae613b1b46e04cf7152508c1a0fecc --- .../gerrit/server/project/ProjectConfig.java | 11 ++- .../acceptance/api/project/ProjectIT.java | 74 +++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java index a7133bbe26..09279fabb5 100644 --- a/java/com/google/gerrit/server/project/ProjectConfig.java +++ b/java/com/google/gerrit/server/project/ProjectConfig.java @@ -911,9 +911,18 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. lowerNames.put(lower, name); List values = new ArrayList<>(); + Set allValues = new HashSet<>(); for (String value : rc.getStringList(LABEL, name, KEY_VALUE)) { try { - values.add(parseLabelValue(value)); + LabelValue labelValue = parseLabelValue(value); + if (allValues.add(labelValue.getValue())) { + values.add(labelValue); + } else { + error( + new ValidationError( + PROJECT_CONFIG, + String.format("Duplicate %s \"%s\" for label \"%s\"", KEY_VALUE, value, name))); + } } catch (IllegalArgumentException notValue) { error( new ValidationError( diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java index 2b6cd08e79..766f8537b1 100644 --- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java +++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java @@ -19,6 +19,7 @@ import static com.google.gerrit.server.project.ProjectState.INHERITED_FROM_GLOBA import static com.google.gerrit.server.project.ProjectState.INHERITED_FROM_PARENT; import static com.google.gerrit.server.project.ProjectState.OVERRIDDEN_BY_GLOBAL; import static com.google.gerrit.server.project.ProjectState.OVERRIDDEN_BY_PARENT; +import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.toSet; import com.google.common.collect.ImmutableList; @@ -56,12 +57,21 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.config.ProjectConfigEntry; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.project.CommentLinkInfoImpl; +import com.google.gerrit.server.project.ProjectConfig; import com.google.inject.AbstractModule; import com.google.inject.Inject; import com.google.inject.Module; import java.util.HashMap; import java.util.Map; +import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.ObjectLoader; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevObject; +import org.eclipse.jgit.revwalk.RevTree; +import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.PushResult; import org.eclipse.jgit.transport.RemoteRefUpdate.Status; import org.junit.After; @@ -657,6 +667,70 @@ public class ProjectIT extends AbstractDaemonTest { assertCommentLinks(getConfig(), expected); } + @Test + public void cannotPushLabelDefinitionWithDuplicateValues() throws Exception { + Config cfg = readAllProjectsConfig(); + cfg.setStringList( + "label", + "Code-Review", + "value", + ImmutableList.of("+1 LGTM", "1 LGTM", "0 No Value", "-1 Looks Bad")); + + TestRepository repo = cloneProject(allProjects); + GitUtil.fetch(repo, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG); + repo.reset(RefNames.REFS_CONFIG); + PushOneCommit.Result r = + pushFactory + .create(admin.newIdent(), repo, "Subject", "project.config", cfg.toText()) + .to(RefNames.REFS_CONFIG); + r.assertErrorStatus("invalid project configuration"); + r.assertMessage("project.config: duplicate value \"1 lgtm\" for label \"code-review\""); + } + + @Test + public void getProjectThatHasLabelDefinitionWithDuplicateValues() throws Exception { + // Update the definition of the Code-Review label so that it has the value "+1 LGTM" twice. + // This update bypasses all validation checks so that the duplicate label value doesn't get + // rejected. + Config cfg = readAllProjectsConfig(); + cfg.setStringList( + "label", + "Code-Review", + "value", + ImmutableList.of("+1 LGTM", "1 LGTM", "0 No Value", "-1 Looks Bad")); + + try (TestRepository repo = + new TestRepository<>(repoManager.openRepository(allProjects))) { + repo.update( + RefNames.REFS_CONFIG, + repo.commit() + .message("Set label with duplicate value") + .parent(getHead(repo.getRepository(), RefNames.REFS_CONFIG)) + .add(ProjectConfig.PROJECT_CONFIG, cfg.toText())); + } + + // Verify that project info can be retrieved and that the label value "+1 LGTM" appears only + // once. + ProjectInfo projectInfo = gApi.projects().name(allProjects.get()).get(); + assertThat(projectInfo.labels.keySet()).containsExactly("Code-Review"); + assertThat(projectInfo.labels.get("Code-Review").values) + .containsExactly("+1", "LGTM", " 0", "No Value", "-1", "Looks Bad"); + } + + private Config readAllProjectsConfig() throws Exception { + try (TestRepository repo = + new TestRepository<>(repoManager.openRepository(allProjects))) { + RevWalk rw = repo.getRevWalk(); + RevTree tree = rw.parseTree(repo.getRepository().resolve("HEAD")); + RevObject obj = rw.parseAny(repo.get(tree, "project.config")); + ObjectLoader loader = rw.getObjectReader().open(obj); + String text = new String(loader.getCachedBytes(), UTF_8); + Config cfg = new Config(); + cfg.fromText(text); + return cfg; + } + } + private CommentLinkInfo commentLinkInfo(String name, String match, String link) { return new CommentLinkInfoImpl(name, match, link, null /*html*/, null /*enabled*/); } From 812512a6a068b6b885148b69c24ae317f6308839 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Mon, 22 Jun 2020 08:12:12 +0200 Subject: [PATCH 5/5] maven_jar: Remove unused unsign attribute Last usage of unsign artifacts machinery was removed in I3be17767ed4, but the unsign attribute was missed to be removed in maven_jar rule. Change-Id: Ib925ddf4c65aa73e9cbeec02cf4c57c296a7a722 --- tools/bzl/maven_jar.bzl | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/bzl/maven_jar.bzl b/tools/bzl/maven_jar.bzl index c8ba7caf37..177e80b0b3 100644 --- a/tools/bzl/maven_jar.bzl +++ b/tools/bzl/maven_jar.bzl @@ -170,7 +170,6 @@ maven_jar = repository_rule( "repository": attr.string(default = MAVEN_CENTRAL), "sha1": attr.string(), "src_sha1": attr.string(), - "unsign": attr.bool(default = False), "exports": attr.string_list(), "deps": attr.string_list(), "_download_script": attr.label(default = Label("//tools:download_file.py")),