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 <ekempin@google.com>
Change-Id: I90e5439a00ae613b1b46e04cf7152508c1a0fecc
This commit is contained in:
Edwin Kempin
committed by
David Pursehouse
David Pursehouse
parent
ab950cfaef
commit
dadc193eba
@@ -911,9 +911,18 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
|
||||
lowerNames.put(lower, name);
|
||||
|
||||
List<LabelValue> values = new ArrayList<>();
|
||||
Set<Short> 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(
|
||||
|
||||
@@ -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<InMemoryRepository> 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<Repository> 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<Repository> 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*/);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user