From dadc193ebad3828bf757f6ecadb34b00c062e916 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 26 Nov 2019 15:36:06 +0100 Subject: [PATCH] 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*/); }