From 085209e825ed93339f7e96fcd29eb884741f483a Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 20 Nov 2017 15:18:15 -0500 Subject: [PATCH 1/5] ProjectConfig: Trim LabelValues when emitting label config LabelValue#format for a 0 value previously prefaced the result with a space, which then got written as a value in project.config like so: [label "My-Label"] value = -1 Nope value = 0 Meh value = +1 Yup This arguably had an aesthetic benefit in project.config by aligning the numbers and descriptions between the various values. However, it had no effect on the parsed value, since JGit would trim the leading whitespace from the config value. This particular serialization was actually a bug in JGit: it was unable to round-trip values starting with a single space. That bug was fixed in https://git.eclipse.org/r/111920, which causes the side effect in Gerrit of surrounding this config value with quotes: [label "My-Label"] value = -1 Nope value = " 0 Meh" value = +1 Yup This still has no effect on parsing within ProjectConfig, specifically because the splitter in parseLabelValue uses omitEmptyStrings(). But the aesthetics are frankly now worse, so trim the value instead. It might be worth rethinking whether LabelValue should even prepend this space in the first place, but that is a more invasive change, e.g. it touches GWT UI code, so we don't attempt it here. Change-Id: Idca2a30495243d7b58f557f61016a3210d21a084 --- Documentation/config-labels.txt | 6 +-- .../google/gerrit/pgm/init/InitLabels.java | 2 +- .../gerrit/server/git/ProjectConfig.java | 2 +- .../gerrit/server/git/ProjectConfigTest.java | 54 ++++++++++++++++++- 4 files changed, 57 insertions(+), 7 deletions(-) diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt index 532b8c42a8..227651a083 100644 --- a/Documentation/config-labels.txt +++ b/Documentation/config-labels.txt @@ -95,7 +95,7 @@ adding the following to `project.config` in `All-Projects`: [label "Verified"] function = MaxWithBlock value = -1 Fails - value = 0 No score + value = 0 No score value = +1 Verified copyAllScoresIfNoCodeChange = true ---- @@ -379,7 +379,7 @@ but has different names/labels: [label "Copyright-Check"] function = MaxWithBlock value = -1 Do not have copyright - value = 0 No score + value = 0 No score value = +1 Copyright clear ---- @@ -401,7 +401,7 @@ user permissions. Assume the configuration below. value = -3 Ohh, hell no! value = -2 Hmm, I'm not a fan value = -1 I'm not sure I like this - value = 0 No score + value = 0 No score value = +1 I like, but need another to like it as well value = +2 Hmm, this is pretty nice value = +3 Ohh, hell yes! diff --git a/java/com/google/gerrit/pgm/init/InitLabels.java b/java/com/google/gerrit/pgm/init/InitLabels.java index c7309f8524..3d1ec7b9c2 100644 --- a/java/com/google/gerrit/pgm/init/InitLabels.java +++ b/java/com/google/gerrit/pgm/init/InitLabels.java @@ -61,7 +61,7 @@ public class InitLabels implements InitStep { KEY_LABEL, LABEL_VERIFIED, KEY_VALUE, - Arrays.asList(new String[] {"-1 Fails", " 0 No score", "+1 Verified"})); + Arrays.asList(new String[] {"-1 Fails", "0 No score", "+1 Verified"})); cfg.setBoolean(KEY_LABEL, LABEL_VERIFIED, KEY_COPY_ALL_SCORES_IF_NO_CODE_CHANGE, true); allProjectsConfig.save("Configure 'Verified' label"); } diff --git a/java/com/google/gerrit/server/git/ProjectConfig.java b/java/com/google/gerrit/server/git/ProjectConfig.java index ed5695cb7e..3086e81f25 100644 --- a/java/com/google/gerrit/server/git/ProjectConfig.java +++ b/java/com/google/gerrit/server/git/ProjectConfig.java @@ -1421,7 +1421,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. rc, LABEL, name, KEY_CAN_OVERRIDE, label.canOverride(), LabelType.DEF_CAN_OVERRIDE); List values = Lists.newArrayListWithCapacity(label.getValues().size()); for (LabelValue value : label.getValues()) { - values.add(value.format()); + values.add(value.format().trim()); } rc.setStringList(LABEL, name, KEY_VALUE, values); } diff --git a/javatests/com/google/gerrit/server/git/ProjectConfigTest.java b/javatests/com/google/gerrit/server/git/ProjectConfigTest.java index ed7a0053fe..9f25797b98 100644 --- a/javatests/com/google/gerrit/server/git/ProjectConfigTest.java +++ b/javatests/com/google/gerrit/server/git/ProjectConfigTest.java @@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.config.PluginConfig; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.project.testing.Util; import com.google.gwtorm.client.KeyUtil; import com.google.gwtorm.server.StandardKeyEncoder; import java.io.IOException; @@ -156,6 +157,30 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { "" // + "[label \"CustomLabel\"]\n" // + " value = -1 Negative\n" // + // No leading space before 0. + + " value = 0 No Score\n" // + + " value = 1 Positive\n")) // + )); + + ProjectConfig cfg = read(rev); + Map labels = cfg.getLabelSections(); + Short dv = labels.entrySet().iterator().next().getValue().getDefaultValue(); + assertThat((int) dv).isEqualTo(0); + } + + @Test + public void readConfigLabelOldStyleWithLeadingSpace() throws Exception { + RevCommit rev = + util.commit( + util.tree( // + util.file("groups", util.blob(group(developers))), // + util.file( + "project.config", + util.blob( + "" // + + "[label \"CustomLabel\"]\n" // + + " value = -1 Negative\n" // + // Leading space before 0. + " value = 0 No Score\n" // + " value = 1 Positive\n")) // )); @@ -178,7 +203,7 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { "" // + "[label \"CustomLabel\"]\n" // + " value = -1 Negative\n" // - + " value = 0 No Score\n" // + + " value = 0 No Score\n" // + " value = 1 Positive\n" // + " defaultValue = -1\n")) // )); @@ -201,7 +226,7 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { "" // + "[label \"CustomLabel\"]\n" // + " value = -1 Negative\n" // - + " value = 0 No Score\n" // + + " value = 0 No Score\n" // + " value = 1 Positive\n" // + " defaultValue = -2\n")) // )); @@ -301,6 +326,31 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { + "\tdefaultValue = 0\n"); // label gets this value when it is created } + @Test + public void editConfigLabelValues() throws Exception { + RevCommit rev = util.commit(util.tree()); + update(rev); + + ProjectConfig cfg = read(rev); + cfg.getLabelSections() + .put( + "My-Label", + Util.category( + "My-Label", + Util.value(-1, "Negative"), + Util.value(0, "No score"), + Util.value(1, "Positive"))); + rev = commit(cfg); + assertThat(text(rev, "project.config")) + .isEqualTo( + "[label \"My-Label\"]\n" + + "\tfunction = MaxWithBlock\n" + + "\tdefaultValue = 0\n" + + "\tvalue = -1 Negative\n" + + "\tvalue = 0 No score\n" + + "\tvalue = +1 Positive\n"); + } + @Test public void editConfigMissingGroupTableEntry() throws Exception { RevCommit rev = From 9eb5c4eed99f88d4620d3590cf3d3d3af9d969c6 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 20 Nov 2017 15:14:45 -0500 Subject: [PATCH 2/5] Update JGit to 4.9.0.201710071750-r.65-g8b3ab4343 Includes a fix to round-tripping Config values with leading/trailing whitespace. Change-Id: I43bd6cbf37ea057b1c7eae13f48654497f36b4b8 --- lib/jgit/jgit.bzl | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/jgit/jgit.bzl b/lib/jgit/jgit.bzl index 6e655c3820..5d29b68019 100644 --- a/lib/jgit/jgit.bzl +++ b/lib/jgit/jgit.bzl @@ -1,6 +1,6 @@ load("//tools/bzl:maven_jar.bzl", "GERRIT", "MAVEN_LOCAL", "MAVEN_CENTRAL", "maven_jar") -_JGIT_VERS = "4.9.0.201710071750-r.30-g651e17bac" +_JGIT_VERS = "4.9.0.201710071750-r.65-g8b3ab4343" _DOC_VERS = "4.9.0.201710071750-r" # Set to _JGIT_VERS unless using a snapshot @@ -26,28 +26,28 @@ def jgit_maven_repos(): name = "jgit_lib", artifact = "org.eclipse.jgit:org.eclipse.jgit:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "5f0ab69e6dd369c9efcf2fc23f7ae923113d6d3d", - src_sha1 = "8dcfff9612815fc1b2bb0e8d032c9b99a6ed89e4", + sha1 = "5e9a6147583d2071da3a4eb55b1da5955793da45", + src_sha1 = "61fee1a923b718b81b1e0f763865a222a78ed3d5", unsign = True, ) maven_jar( name = "jgit_servlet", artifact = "org.eclipse.jgit:org.eclipse.jgit.http.server:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "58629a74278c502f7fb6926b9a4b483da46072b5", + sha1 = "f769fd0052de1eb5d78760683ca8d5cdb2c28e8a", unsign = True, ) maven_jar( name = "jgit_archive", artifact = "org.eclipse.jgit:org.eclipse.jgit.archive:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "a31a98bac30977bbad867aa89a0338d3260eb46a", + sha1 = "ea97248007428a3e5a552eb93ae9b71fc041fcb9", ) maven_jar( name = "jgit_junit", artifact = "org.eclipse.jgit:org.eclipse.jgit.junit:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "358989980e6faf77c3af955f5eea268020a6407d", + sha1 = "b9ddbf0819f6411ecb616eabcabdbd3ea4ead9a3", unsign = True, ) From bda5a3bffae23dcc2b123c07de87b64fe23a2f0d Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 20 Nov 2017 14:08:34 -0500 Subject: [PATCH 3/5] Tests for group names with extra whitespace Until today JGit was not able to round-trip group names with a single leading whitespace. Now that it can, add tests in both GroupRebuilderTest and GroupsIT. Change-Id: I3dfa9388e07417c3753e12cbc8051332e74db15a --- .../gerrit/acceptance/api/group/GroupsIT.java | 11 +++++++++++ .../gerrit/server/group/db/GroupRebuilderTest.java | 14 +++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index a1fcaaa4ec..fab7378c25 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -1187,6 +1187,17 @@ public class GroupsIT extends AbstractDaemonTest { assertStaleGroupAndReindex(groupUuid); } + @Test + public void groupNamesWithLeadingAndTrailingWhitespace() throws Exception { + for (String leading : ImmutableList.of("", " ", " ")) { + for (String trailing : ImmutableList.of("", " ", " ")) { + String name = leading + name("group") + trailing; + GroupInfo g = gApi.groups().create(name).get(); + assertThat(g.name).isEqualTo(name); + } + } + } + private void assertStaleGroupAndReindex(AccountGroup.UUID groupUuid) throws IOException { // Evict group from cache to be sure that we use the index state for staleness checks. This has // to happen directly on the groupsByUUID cache because GroupsCacheImpl triggers a reindex for diff --git a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java index b142887f00..baedaaaeee 100644 --- a/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java +++ b/javatests/com/google/gerrit/server/group/db/GroupRebuilderTest.java @@ -403,6 +403,18 @@ public class GroupRebuilderTest extends AbstractGroupTest { assertThat(GroupTestUtil.readNameToUuidMap(repo)).containsExactly("a", "a-1", "b", "b-2"); } + @Test + public void groupNamesWithLeadingAndTrailingWhitespace() throws Exception { + for (String leading : ImmutableList.of("", " ", " ")) { + for (String trailing : ImmutableList.of("", " ", " ")) { + AccountGroup g = newGroup(leading + "a" + trailing); + GroupBundle b = builder().group(g).build(); + rebuilder.rebuild(repo, b, null); + assertThat(reload(g)).isEqualTo(b); + } + } + } + private GroupBundle reload(AccountGroup g) throws Exception { return bundleFactory.fromNoteDb(repo, g.getGroupUUID()); } @@ -412,7 +424,7 @@ public class GroupRebuilderTest extends AbstractGroupTest { return new AccountGroup( new AccountGroup.NameKey(name), new AccountGroup.Id(id), - new AccountGroup.UUID(name + "-" + id), + new AccountGroup.UUID(name.trim() + "-" + id), TimeUtil.nowTs()); } From 8fc836bd1f1b22f6a4fd1da7be8078d93e65d744 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 21 Nov 2017 08:03:31 -0500 Subject: [PATCH 4/5] Clean up ProjectConfigTest * Use an InMemoryRepository instead of a local disk repo. * Inherit GerritBaseTests to avoid calling KeyUtil. * Rename the TestRepository instance to tr for consistency with other tests. * Use the simpler chained commit builder from TestRepository rather than constructing blobs and trees manually. * Remove trailing empty comments, which are not currently used in Gerrit style. Change-Id: Ibf346b7b0563825c5f41a6ad04be5a3396cbb881 --- .../gerrit/server/git/ProjectConfigTest.java | 395 +++++++----------- 1 file changed, 161 insertions(+), 234 deletions(-) diff --git a/javatests/com/google/gerrit/server/git/ProjectConfigTest.java b/javatests/com/google/gerrit/server/git/ProjectConfigTest.java index 9f25797b98..627fc27988 100644 --- a/javatests/com/google/gerrit/server/git/ProjectConfigTest.java +++ b/javatests/com/google/gerrit/server/git/ProjectConfigTest.java @@ -30,8 +30,7 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.config.PluginConfig; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.project.testing.Util; -import com.google.gwtorm.client.KeyUtil; -import com.google.gwtorm.server.StandardKeyEncoder; +import com.google.gerrit.testing.GerritBaseTests; import java.io.IOException; import java.util.Arrays; import java.util.Collections; @@ -39,7 +38,8 @@ import java.util.Map; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; -import org.eclipse.jgit.junit.LocalDiskRepositoryTestCase; +import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription; +import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; @@ -49,26 +49,25 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.util.RawParseUtils; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Test; -public class ProjectConfigTest extends LocalDiskRepositoryTestCase { +public class ProjectConfigTest extends GerritBaseTests { private static final String LABEL_SCORES_CONFIG = " copyMinScore = " + !LabelType.DEF_COPY_MIN_SCORE - + "\n" // + + "\n" + " copyMaxScore = " + !LabelType.DEF_COPY_MAX_SCORE - + "\n" // + + "\n" + " copyAllScoresOnMergeFirstParentUpdate = " + !LabelType.DEF_COPY_ALL_SCORES_ON_MERGE_FIRST_PARENT_UPDATE - + "\n" // + + "\n" + " copyAllScoresOnTrivialRebase = " + !LabelType.DEF_COPY_ALL_SCORES_ON_TRIVIAL_REBASE - + "\n" // + + "\n" + " copyAllScoresIfNoCodeChange = " + !LabelType.DEF_COPY_ALL_SCORES_IF_NO_CODE_CHANGE - + "\n" // + + "\n" + " copyAllScoresIfNoChange = " + !LabelType.DEF_COPY_ALL_SCORES_IF_NO_CHANGE + "\n"; @@ -78,46 +77,36 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { private final GroupReference staff = new GroupReference(new AccountGroup.UUID("Y"), "Staff"); private Repository db; - private TestRepository util; + private TestRepository tr; - @BeforeClass - public static void setUpOnce() { - KeyUtil.setEncoderImpl(new StandardKeyEncoder()); - } - - @Override @Before public void setUp() throws Exception { - super.setUp(); - db = createBareRepository(); - util = new TestRepository<>(db); + db = new InMemoryRepository(new DfsRepositoryDescription("repo")); + tr = new TestRepository<>(db); } @Test public void readConfig() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[access \"refs/heads/*\"]\n" // - + " exclusiveGroupPermissions = read submit create\n" // - + " submit = group Developers\n" // - + " push = group Developers\n" // - + " read = group Developers\n" // - + "[accounts]\n" // - + " sameGroupVisibility = deny group Developers\n" // - + " sameGroupVisibility = block group Staff\n" // - + "[contributor-agreement \"Individual\"]\n" // - + " description = A simple description\n" // - + " accepted = group Developers\n" // - + " accepted = group Staff\n" // - + " autoVerify = group Developers\n" // - + " agreementUrl = http://www.example.com/agree\n")) // - )); + tr.commit() + .add("groups", group(developers)) + .add( + "project.config", + "[access \"refs/heads/*\"]\n" + + " exclusiveGroupPermissions = read submit create\n" + + " submit = group Developers\n" + + " push = group Developers\n" + + " read = group Developers\n" + + "[accounts]\n" + + " sameGroupVisibility = deny group Developers\n" + + " sameGroupVisibility = block group Staff\n" + + "[contributor-agreement \"Individual\"]\n" + + " description = A simple description\n" + + " accepted = group Developers\n" + + " accepted = group Staff\n" + + " autoVerify = group Developers\n" + + " agreementUrl = http://www.example.com/agree\n") + .create(); ProjectConfig cfg = read(rev); assertThat(cfg.getAccountsSection().getSameGroupVisibility()).hasSize(2); @@ -148,19 +137,16 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void readConfigLabelDefaultValue() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[label \"CustomLabel\"]\n" // - + " value = -1 Negative\n" // - // No leading space before 0. - + " value = 0 No Score\n" // - + " value = 1 Positive\n")) // - )); + tr.commit() + .add("groups", group(developers)) + .add( + "project.config", + "[label \"CustomLabel\"]\n" + + " value = -1 Negative\n" + // No leading space before 0. + + " value = 0 No Score\n" + + " value = 1 Positive\n") + .create(); ProjectConfig cfg = read(rev); Map labels = cfg.getLabelSections(); @@ -171,19 +157,16 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void readConfigLabelOldStyleWithLeadingSpace() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[label \"CustomLabel\"]\n" // - + " value = -1 Negative\n" // - // Leading space before 0. - + " value = 0 No Score\n" // - + " value = 1 Positive\n")) // - )); + tr.commit() + .add("groups", group(developers)) + .add( + "project.config", + "[label \"CustomLabel\"]\n" + + " value = -1 Negative\n" + // Leading space before 0. + + " value = 0 No Score\n" + + " value = 1 Positive\n") + .create(); ProjectConfig cfg = read(rev); Map labels = cfg.getLabelSections(); @@ -194,19 +177,16 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void readConfigLabelDefaultValueInRange() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[label \"CustomLabel\"]\n" // - + " value = -1 Negative\n" // - + " value = 0 No Score\n" // - + " value = 1 Positive\n" // - + " defaultValue = -1\n")) // - )); + tr.commit() + .add("groups", group(developers)) + .add( + "project.config", + "[label \"CustomLabel\"]\n" + + " value = -1 Negative\n" + + " value = 0 No Score\n" + + " value = 1 Positive\n" + + " defaultValue = -1\n") + .create(); ProjectConfig cfg = read(rev); Map labels = cfg.getLabelSections(); @@ -217,19 +197,16 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void readConfigLabelDefaultValueNotInRange() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[label \"CustomLabel\"]\n" // - + " value = -1 Negative\n" // - + " value = 0 No Score\n" // - + " value = 1 Positive\n" // - + " defaultValue = -2\n")) // - )); + tr.commit() + .add("groups", group(developers)) + .add( + "project.config", + "[label \"CustomLabel\"]\n" + + " value = -1 Negative\n" + + " value = 0 No Score\n" + + " value = 1 Positive\n" + + " defaultValue = -2\n") + .create(); ProjectConfig cfg = read(rev); assertThat(cfg.getValidationErrors()).hasSize(1); @@ -240,16 +217,10 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void readConfigLabelScores() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[label \"CustomLabel\"]\n" // - + LABEL_SCORES_CONFIG)) // - )); + tr.commit() + .add("groups", group(developers)) + .add("project.config", "[label \"CustomLabel\"]\n" + LABEL_SCORES_CONFIG) + .create(); ProjectConfig cfg = read(rev); Map labels = cfg.getLabelSections(); @@ -269,29 +240,26 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void editConfig() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[access \"refs/heads/*\"]\n" // - + " exclusiveGroupPermissions = read submit\n" // - + " submit = group Developers\n" // - + " upload = group Developers\n" // - + " read = group Developers\n" // - + "[accounts]\n" // - + " sameGroupVisibility = deny group Developers\n" // - + " sameGroupVisibility = block group Staff\n" // - + "[contributor-agreement \"Individual\"]\n" // - + " description = A simple description\n" // - + " accepted = group Developers\n" // - + " autoVerify = group Developers\n" // - + " agreementUrl = http://www.example.com/agree\n" // - + "[label \"CustomLabel\"]\n" // - + LABEL_SCORES_CONFIG)) // - )); + tr.commit() + .add("groups", group(developers)) + .add( + "project.config", + "[access \"refs/heads/*\"]\n" + + " exclusiveGroupPermissions = read submit\n" + + " submit = group Developers\n" + + " upload = group Developers\n" + + " read = group Developers\n" + + "[accounts]\n" + + " sameGroupVisibility = deny group Developers\n" + + " sameGroupVisibility = block group Staff\n" + + "[contributor-agreement \"Individual\"]\n" + + " description = A simple description\n" + + " accepted = group Developers\n" + + " autoVerify = group Developers\n" + + " agreementUrl = http://www.example.com/agree\n" + + "[label \"CustomLabel\"]\n" + + LABEL_SCORES_CONFIG) + .create(); update(rev); ProjectConfig cfg = read(rev); @@ -307,20 +275,19 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { rev = commit(cfg); assertThat(text(rev, "project.config")) .isEqualTo( - "" // - + "[access \"refs/heads/*\"]\n" // - + " exclusiveGroupPermissions = read submit\n" // - + " submit = group Developers\n" // - + "\tsubmit = group Staff\n" // - + " upload = group Developers\n" // - + " read = group Developers\n" // - + "[accounts]\n" // - + " sameGroupVisibility = group Staff\n" // - + "[contributor-agreement \"Individual\"]\n" // - + " description = A new description\n" // - + " accepted = group Staff\n" // + "[access \"refs/heads/*\"]\n" + + " exclusiveGroupPermissions = read submit\n" + + " submit = group Developers\n" + + "\tsubmit = group Staff\n" + + " upload = group Developers\n" + + " read = group Developers\n" + + "[accounts]\n" + + " sameGroupVisibility = group Staff\n" + + "[contributor-agreement \"Individual\"]\n" + + " description = A new description\n" + + " accepted = group Staff\n" + " agreementUrl = http://www.example.com/agree\n" - + "[label \"CustomLabel\"]\n" // + + "[label \"CustomLabel\"]\n" + LABEL_SCORES_CONFIG + "\tfunction = MaxWithBlock\n" // label gets this function when it is created + "\tdefaultValue = 0\n"); // label gets this value when it is created @@ -328,7 +295,7 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void editConfigLabelValues() throws Exception { - RevCommit rev = util.commit(util.tree()); + RevCommit rev = tr.commit().create(); update(rev); ProjectConfig cfg = read(rev); @@ -354,19 +321,16 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void editConfigMissingGroupTableEntry() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[access \"refs/heads/*\"]\n" // - + " exclusiveGroupPermissions = read submit\n" // - + " submit = group People Who Can Submit\n" // - + " upload = group Developers\n" // - + " read = group Developers\n")) // - )); + tr.commit() + .add("groups", group(developers)) + .add( + "project.config", + "[access \"refs/heads/*\"]\n" + + " exclusiveGroupPermissions = read submit\n" + + " submit = group People Who Can Submit\n" + + " upload = group Developers\n" + + " read = group Developers\n") + .create(); update(rev); ProjectConfig cfg = read(rev); @@ -376,29 +340,25 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { rev = commit(cfg); assertThat(text(rev, "project.config")) .isEqualTo( - "" // - + "[access \"refs/heads/*\"]\n" // - + " exclusiveGroupPermissions = read submit\n" // - + " submit = group People Who Can Submit\n" // - + "\tsubmit = group Staff\n" // - + " upload = group Developers\n" // + "[access \"refs/heads/*\"]\n" + + " exclusiveGroupPermissions = read submit\n" + + " submit = group People Who Can Submit\n" + + "\tsubmit = group Staff\n" + + " upload = group Developers\n" + " read = group Developers\n"); } @Test public void readExistingPluginConfig() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file( - "project.config", - util.blob( - "" // - + "[plugin \"somePlugin\"]\n" // - + " key1 = value1\n" // - + " key2 = value2a\n" - + " key2 = value2b\n")) // - )); + tr.commit() + .add( + "project.config", + "[plugin \"somePlugin\"]\n" + + " key1 = value1\n" + + " key2 = value2a\n" + + " key2 = value2b\n") + .create(); update(rev); ProjectConfig cfg = read(rev); @@ -419,17 +379,14 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void editPluginConfig() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file( - "project.config", - util.blob( - "" // - + "[plugin \"somePlugin\"]\n" // - + " key1 = value1\n" // - + " key2 = value2a\n" // - + " key2 = value2b\n")) // - )); + tr.commit() + .add( + "project.config", + "[plugin \"somePlugin\"]\n" + + " key1 = value1\n" + + " key2 = value2a\n" + + " key2 = value2b\n") + .create(); update(rev); ProjectConfig cfg = read(rev); @@ -439,28 +396,19 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { rev = commit(cfg); assertThat(text(rev, "project.config")) .isEqualTo( - "" // - + "[plugin \"somePlugin\"]\n" // - + "\tkey1 = updatedValue1\n" // - + "\tkey2 = updatedValue2a\n" // + "[plugin \"somePlugin\"]\n" + + "\tkey1 = updatedValue1\n" + + "\tkey2 = updatedValue2a\n" + "\tkey2 = updatedValue2b\n"); } @Test public void readPluginConfigGroupReference() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[plugin \"somePlugin\"]\n" // - + "key1 = " - + developers.toConfigValue() - + "\n")) // - )); + tr.commit() + .add("groups", group(developers)) + .add("project.config", "[plugin \"somePlugin\"]\nkey1 = " + developers.toConfigValue()) + .create(); update(rev); ProjectConfig cfg = read(rev); @@ -472,18 +420,10 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void readPluginConfigGroupReferenceNotInGroupsFile() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[plugin \"somePlugin\"]\n" // - + "key1 = " - + staff.toConfigValue() - + "\n")) // - )); + tr.commit() + .add("groups", group(developers)) + .add("project.config", "[plugin \"somePlugin\"]\nkey1 = " + staff.toConfigValue()) + .create(); update(rev); ProjectConfig cfg = read(rev); @@ -496,18 +436,10 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { @Test public void editPluginConfigGroupReference() throws Exception { RevCommit rev = - util.commit( - util.tree( // - util.file("groups", util.blob(group(developers))), // - util.file( - "project.config", - util.blob( - "" // - + "[plugin \"somePlugin\"]\n" // - + "key1 = " - + developers.toConfigValue() - + "\n")) // - )); + tr.commit() + .add("groups", group(developers)) + .add("project.config", "[plugin \"somePlugin\"]\nkey1 = " + developers.toConfigValue()) + .create(); update(rev); ProjectConfig cfg = read(rev); @@ -518,16 +450,11 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { pluginCfg.setGroupReference("key1", staff); rev = commit(cfg); assertThat(text(rev, "project.config")) - .isEqualTo( - "" // - + "[plugin \"somePlugin\"]\n" // - + "\tkey1 = " - + staff.toConfigValue() - + "\n"); + .isEqualTo("[plugin \"somePlugin\"]\n\tkey1 = " + staff.toConfigValue() + "\n"); assertThat(text(rev, "groups")) .isEqualTo( - "# UUID\tGroup Name\n" // - + "#\n" // + "# UUID\tGroup Name\n" + + "#\n" + staff.getUUID().get() + " \t" + staff.getName() @@ -544,13 +471,13 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { throws IOException, MissingObjectException, IncorrectObjectTypeException { try (MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, cfg.getProject().getNameKey(), db)) { - util.tick(5); - util.setAuthorAndCommitter(md.getCommitBuilder()); + tr.tick(5); + tr.setAuthorAndCommitter(md.getCommitBuilder()); md.setMessage("Edit\n"); cfg.commit(md); Ref ref = db.exactRef(RefNames.REFS_CONFIG); - return util.getRevWalk().parseCommit(ref.getObjectId()); + return tr.getRevWalk().parseCommit(ref.getObjectId()); } } @@ -565,7 +492,7 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { } private String text(RevCommit rev, String path) throws Exception { - RevObject blob = util.get(rev.getTree(), path); + RevObject blob = tr.get(rev.getTree(), path); byte[] data = db.open(blob).getCachedBytes(Integer.MAX_VALUE); return RawParseUtils.decode(data); } From d93962eaf28ed0e6eeabf1bc2d73261abda82efa Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 21 Nov 2017 09:20:59 -0500 Subject: [PATCH 5/5] GroupBundle: Warn if input entity lists are not unique We found a corner case in the audit log where distinct entities might compare unique; see the code for details. Fixing such corruption in ReviewDb, if it's even possible, is considerable work, which we don't need to attempt if we don't actually run into issues in practice. In order to find out if there are issues in practice, log and continue. Change-Id: I580ed457cb402fda1a70dc2bd715127547fef21b --- .../gerrit/server/group/db/GroupBundle.java | 70 +++++++++++++++---- 1 file changed, 58 insertions(+), 12 deletions(-) diff --git a/java/com/google/gerrit/server/group/db/GroupBundle.java b/java/com/google/gerrit/server/group/db/GroupBundle.java index 642a6bdbaf..e0dc035fff 100644 --- a/java/com/google/gerrit/server/group/db/GroupBundle.java +++ b/java/com/google/gerrit/server/group/db/GroupBundle.java @@ -31,8 +31,11 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; +import java.util.Collection; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Repository; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A bundle of all entities rooted at a single {@link AccountGroup} entity. @@ -42,6 +45,8 @@ import org.eclipse.jgit.lib.Repository; */ @AutoValue public abstract class GroupBundle { + private static final Logger log = LoggerFactory.getLogger(GroupBundle.class); + static { // Initialization-time checks that the column set hasn't changed since the // last time this file was updated. @@ -63,6 +68,22 @@ public abstract class GroupBundle { checkColumns(AccountGroupMemberAudit.class, 1, 2, 3, 4); } + public enum Source { + REVIEW_DB("ReviewDb"), + NOTE_DB("NoteDb"); + + private final String name; + + private Source(String name) { + this.name = name; + } + + @Override + public String toString() { + return name; + } + } + @Singleton public static class Factory { private final AuditLogReader auditLogReader; @@ -78,11 +99,12 @@ public abstract class GroupBundle { throw new OrmException("Group " + id + " not found"); } return create( + Source.REVIEW_DB, group, - db.accountGroupMembers().byGroup(id), - db.accountGroupMembersAudit().byGroup(id), - db.accountGroupById().byGroup(id), - db.accountGroupByIdAud().byGroup(id)); + db.accountGroupMembers().byGroup(id).toList(), + db.accountGroupMembersAudit().byGroup(id).toList(), + db.accountGroupById().byGroup(id).toList(), + db.accountGroupByIdAud().byGroup(id).toList()); } public GroupBundle fromNoteDb(Repository repo, AccountGroup.UUID uuid) @@ -102,6 +124,7 @@ public abstract class GroupBundle { accountGroup.setVisibleToAll(internalGroup.isVisibleToAll()); return create( + Source.NOTE_DB, accountGroup, internalGroup .getMembers() @@ -123,20 +146,43 @@ public abstract class GroupBundle { } public static GroupBundle create( + Source source, AccountGroup group, - Iterable members, - Iterable memberAudit, - Iterable byId, - Iterable byIdAudit) { + Collection members, + Collection memberAudit, + Collection byId, + Collection byIdAudit) { + AccountGroup.UUID uuid = group.getGroupUUID(); return new AutoValue_GroupBundle.Builder() .group(group) - .members(members) - .memberAudit(memberAudit) - .byId(byId) - .byIdAudit(byIdAudit) + .members(logIfNotUnique(source, uuid, members, AccountGroupMember.class)) + .memberAudit(logIfNotUnique(source, uuid, memberAudit, AccountGroupMemberAudit.class)) + .byId(logIfNotUnique(source, uuid, byId, AccountGroupById.class)) + .byIdAudit(logIfNotUnique(source, uuid, byIdAudit, AccountGroupByIdAud.class)) .build(); } + private static ImmutableSet logIfNotUnique( + Source source, AccountGroup.UUID uuid, Collection collection, Class clazz) { + ImmutableSet set = ImmutableSet.copyOf(collection); + if (set.size() != collection.size()) { + // One way this can happen is that distinct audit entities can compare equal, because + // AccountGroup{MemberAudit,ByIdAud}.Key does not include the addedOn timestamp in its + // members() list. However, this particular issue only applies to pure adds, since removedOn + // *is* included in equality. As a result, if this happens, it means the audit log is already + // corrupt, and it's not clear if we can programmatically repair it. For migrating to NoteDb, + // we'll try our best to recreate it, but no guarantees it will match the real sequence of + // attempted operations, which is in any case lost in the mists of time. + log.warn( + "group {} in {} has duplicate {} entities: {}", + uuid, + source, + clazz.getSimpleName(), + collection); + } + return set; + } + static Builder builder() { return new AutoValue_GroupBundle.Builder().members().memberAudit().byId().byIdAudit(); }