opendev/gerrit
Code Issues Proposed changes

Prefer subtypes of Multimap

Browse Source 
Guava team recommends using the subinterfaces of Multimap, for the
same reasons they recommend using Set and List rather than Collection:
it documents expectations about ordering, uniqueness, and behavior of
equals. Do this across the board in Gerrit.

Mostly this is straightforward and I tried to exactly match existing
behavior where possible. However, there were a few wrinkles, where
different callers passed different subtypes to the same method.

The main one is arguments to ParameterParser#parse and
splitQueryString, where some callers used SetMultimaps (perhaps
semi-intentionally, or perhaps misunderstanding the nature of
HashMultimap). For the purposes of parameter parsing, a ListMultimap
makes more sense, because it preserves argument order and repetition.

Another instance is a couple places in ReceiveCommits and downstream
where there were SetMultimap<?, Ref>. Since Refs do not implement
equals, this is effectively the same thing as a ListMultimap, and
changing the interface no longer misleads readers into thinking there
might be some deduplication happening.

Finally, this change includes a breaking API change to the return
type of ExternalIncludedIn#getIncludedIn.

Change-Id: I5f1d15e27a32e534a6aaefe204e7a31815f4c8d7
This commit is contained in:
Dave Borowitz
2017-01-13 16:26:45 -05:00
committed by David Pursehouse
parent e5c5953205
commit 484da493b3
76 changed files with 367 additions and 355 deletions
Download Patch File Download Diff File Expand all files Collapse all files

40
gerrit-server/src/test/java/com/google/gerrit/server/git/GroupCollectorTest.java
View File

@@ -17,9 +17,8 @@ package com.google.gerrit.server.git;
import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multimap;
import com.google.common.collect.SortedSetMultimap;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -47,7 +46,7 @@ public class GroupCollectorTest {
RevCommit branchTip = tr.commit().create();
RevCommit a = tr.commit().parent(branchTip).create();
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(a, branchTip),
patchSets(),
groups());
@@ -62,7 +61,7 @@ public class GroupCollectorTest {
RevCommit a = tr.commit().parent(branchTip).create();
RevCommit b = tr.commit().parent(a).create();
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(b, branchTip),
patchSets(),
groups());
@@ -79,7 +78,7 @@ public class GroupCollectorTest {
RevCommit b = tr.commit().parent(a).create();
String group = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(b, branchTip),
patchSets().put(a, psId(1, 1)),
groups().put(psId(1, 1), group));
@@ -95,7 +94,7 @@ public class GroupCollectorTest {
RevCommit a = tr.commit().parent(branchTip).create();
RevCommit b = tr.commit().parent(a).create();
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(b, branchTip),
patchSets().put(a, psId(1, 1)),
groups());
@@ -111,7 +110,7 @@ public class GroupCollectorTest {
RevCommit b = tr.commit().parent(branchTip).create();
RevCommit m = tr.commit().parent(a).parent(b).create();
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(m, branchTip),
patchSets(),
groups());
@@ -129,7 +128,7 @@ public class GroupCollectorTest {
RevCommit m = tr.commit().parent(a).parent(b).create();
String group = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(m, branchTip),
patchSets().put(b, psId(1, 1)),
groups().put(psId(1, 1), group));
@@ -150,7 +149,7 @@ public class GroupCollectorTest {
String group1 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
String group2 = "1234567812345678123456781234567812345678";
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(m, branchTip),
patchSets()
.put(a, psId(1, 1))
@@ -176,7 +175,7 @@ public class GroupCollectorTest {
String group1 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
String group2a = "1234567812345678123456781234567812345678";
String group2b = "ef123456ef123456ef123456ef123456ef123456";
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(m, branchTip),
patchSets()
.put(a, psId(1, 1))
@@ -202,7 +201,7 @@ public class GroupCollectorTest {
RevCommit m = tr.commit().parent(branchTip).parent(a).create();
String group = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(m, branchTip),
patchSets().put(a, psId(1, 1)),
groups().put(psId(1, 1), group));
@@ -218,7 +217,7 @@ public class GroupCollectorTest {
RevCommit a = tr.commit().parent(branchTip).create();
RevCommit m = tr.commit().parent(branchTip).parent(a).create();
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(m, branchTip),
patchSets(),
groups());
@@ -237,7 +236,7 @@ public class GroupCollectorTest {
RevCommit m1 = tr.commit().parent(b).parent(c).create();
RevCommit m2 = tr.commit().parent(a).parent(m1).create();
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(m2, branchTip),
patchSets(),
groups());
@@ -259,7 +258,7 @@ public class GroupCollectorTest {
assertThat(m.getParentCount()).isEqualTo(2);
assertThat(m.getParent(0)).isEqualTo(m.getParent(1));
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(m, branchTip),
patchSets(),
groups());
@@ -279,7 +278,7 @@ public class GroupCollectorTest {
String group1 = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
String group2 = "1234567812345678123456781234567812345678";
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
newWalk(m, branchTip),
patchSets()
.put(a, psId(1, 1))
@@ -307,7 +306,7 @@ public class GroupCollectorTest {
rw.markStart(rw.parseCommit(d));
// Schema upgrade case: all commits are existing patch sets, but none have
// groups assigned yet.
Multimap<ObjectId, String> groups = collectGroups(
SortedSetMultimap<ObjectId, String> groups = collectGroups(
rw,
patchSets()
.put(branchTip, psId(1, 1))
@@ -339,9 +338,9 @@ public class GroupCollectorTest {
return rw;
}
private static Multimap<ObjectId, String> collectGroups(
private static SortedSetMultimap<ObjectId, String> collectGroups(
RevWalk rw,
ImmutableMultimap.Builder<ObjectId, PatchSet.Id> patchSetsBySha,
ImmutableListMultimap.Builder<ObjectId, PatchSet.Id> patchSetsBySha,
ImmutableListMultimap.Builder<PatchSet.Id, String> groupLookup)
throws Exception {
GroupCollector gc =
@@ -355,8 +354,9 @@ public class GroupCollectorTest {
// Helper methods for constructing various map arguments, to avoid lots of
// type specifications.
private static ImmutableMultimap.Builder<ObjectId, PatchSet.Id> patchSets() {
return ImmutableMultimap.builder();
private static ImmutableListMultimap.Builder<ObjectId, PatchSet.Id>
patchSets() {
return ImmutableListMultimap.builder();
}
private static ImmutableListMultimap.Builder<PatchSet.Id, String> groups() {

26
gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java
View File

@@ -21,7 +21,7 @@ import static com.google.gerrit.testutil.TestChanges.newChange;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.toList;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.ListMultimap;
import com.google.gerrit.reviewdb.client.Account;
@@ -194,7 +194,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name()),
P2, RefState.create(ref2, id2.name())),
ImmutableMultimap.of()))
ImmutableListMultimap.of()))
.isFalse();
// Wrong ref value.
@@ -204,7 +204,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
ImmutableSetMultimap.of(
P1, RefState.create(ref1, SHA1),
P2, RefState.create(ref2, id2.name())),
ImmutableMultimap.of()))
ImmutableListMultimap.of()))
.isTrue();
// Swapped repos.
@@ -214,7 +214,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id2.name()),
P2, RefState.create(ref2, id1.name())),
ImmutableMultimap.of()))
ImmutableListMultimap.of()))
.isTrue();
// Two refs in same repo, not stale.
@@ -227,7 +227,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name()),
P1, RefState.create(ref3, id3.name())),
ImmutableMultimap.of()))
ImmutableListMultimap.of()))
.isFalse();
// Ignore ref not mentioned.
@@ -236,7 +236,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
repoManager, C,
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name())),
ImmutableMultimap.of()))
ImmutableListMultimap.of()))
.isFalse();
// One ref wrong.
@@ -246,7 +246,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name()),
P1, RefState.create(ref3, SHA1)),
ImmutableMultimap.of()))
ImmutableListMultimap.of()))
.isTrue();
}
@@ -261,7 +261,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
repoManager, C,
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name())),
ImmutableMultimap.of(
ImmutableListMultimap.of(
P1, RefStatePattern.create("refs/heads/*"))))
.isFalse();
@@ -273,7 +273,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
repoManager, C,
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name())),
ImmutableMultimap.of(
ImmutableListMultimap.of(
P1, RefStatePattern.create("refs/heads/*"))))
.isTrue();
assertThat(
@@ -282,7 +282,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name()),
P1, RefState.create(ref2, id2.name())),
ImmutableMultimap.of(
ImmutableListMultimap.of(
P1, RefStatePattern.create("refs/heads/*"))))
.isFalse();
}
@@ -299,7 +299,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
repoManager, C,
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name())),
ImmutableMultimap.of(
ImmutableListMultimap.of(
P1, RefStatePattern.create("refs/*/foo"))))
.isFalse();
@@ -311,7 +311,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
repoManager, C,
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name())),
ImmutableMultimap.of(
ImmutableListMultimap.of(
P1, RefStatePattern.create("refs/*/foo"))))
.isTrue();
assertThat(
@@ -320,7 +320,7 @@ public class StalenessCheckerTest extends GerritBaseTests {
ImmutableSetMultimap.of(
P1, RefState.create(ref1, id1.name()),
P1, RefState.create(ref3, id3.name())),
ImmutableMultimap.of(
ImmutableListMultimap.of(
P1, RefStatePattern.create("refs/*/foo"))))
.isFalse();
}

45
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -26,7 +26,6 @@ import static org.junit.Assert.fail;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableTable;
import com.google.common.collect.Iterables;
@@ -350,7 +349,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
notes = newNotes(c);
assertThat(notes.getApprovals()).containsExactlyEntriesIn(
ImmutableMultimap.of(
ImmutableListMultimap.of(
psa.getPatchSetId(),
new PatchSetApproval(psa.getKey(), (short) 0, update.getWhen())));
}
@@ -375,7 +374,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
notes = newNotes(c);
assertThat(notes.getApprovals()).containsExactlyEntriesIn(
ImmutableMultimap.of(
ImmutableListMultimap.of(
psa.getPatchSetId(),
new PatchSetApproval(psa.getKey(), (short) 0, update.getWhen())));
@@ -1452,7 +1451,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeNotes notes = newNotes(c);
assertThat(notes.getComments())
.isEqualTo(ImmutableMultimap.of(revId, comment));
.isEqualTo(ImmutableListMultimap.of(revId, comment));
}
@Test
@@ -1472,7 +1471,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeNotes notes = newNotes(c);
assertThat(notes.getComments())
.isEqualTo(ImmutableMultimap.of(revId, comment));
.isEqualTo(ImmutableListMultimap.of(revId, comment));
}
@Test
@@ -1492,7 +1491,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeNotes notes = newNotes(c);
assertThat(notes.getComments())
.isEqualTo(ImmutableMultimap.of(revId, comment));
.isEqualTo(ImmutableListMultimap.of(revId, comment));
}
@Test
@@ -1512,7 +1511,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeNotes notes = newNotes(c);
assertThat(notes.getComments())
.isEqualTo(ImmutableMultimap.of(revId, comment));
.isEqualTo(ImmutableListMultimap.of(revId, comment));
}
@Test
@@ -1822,7 +1821,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
}
}
assertThat(notes.getComments()).isEqualTo(
ImmutableMultimap.of(
ImmutableListMultimap.of(
revId, comment1,
revId, comment2,
revId, comment3));
@@ -1879,7 +1878,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
}
}
assertThat(notes.getComments())
.isEqualTo(ImmutableMultimap.of(revId, comment));
.isEqualTo(ImmutableListMultimap.of(revId, comment));
}
@Test
@@ -1934,7 +1933,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
}
}
assertThat(notes.getComments())
.isEqualTo(ImmutableMultimap.of(new RevId(comment.revId), comment));
.isEqualTo(ImmutableListMultimap.of(new RevId(comment.revId), comment));
}
@Test
@@ -1969,7 +1968,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.commit();
assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
ImmutableMultimap.of(
ImmutableListMultimap.of(
new RevId(rev1), commentForBase,
new RevId(rev2), commentForPS));
}
@@ -2004,7 +2003,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.commit();
assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
ImmutableMultimap.of(
ImmutableListMultimap.of(
new RevId(rev), comment1,
new RevId(rev), comment2)).inOrder();
}
@@ -2039,7 +2038,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.commit();
assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
ImmutableMultimap.of(
ImmutableListMultimap.of(
new RevId(rev), comment1,
new RevId(rev), comment2)).inOrder();
}
@@ -2077,7 +2076,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.commit();
assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
ImmutableMultimap.of(
ImmutableListMultimap.of(
new RevId(rev1), comment1,
new RevId(rev2), comment2));
}
@@ -2103,7 +2102,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeNotes notes = newNotes(c);
assertThat(notes.getDraftComments(otherUserId)).containsExactlyEntriesIn(
ImmutableMultimap.of(new RevId(rev), comment1));
ImmutableListMultimap.of(new RevId(rev), comment1));
assertThat(notes.getComments()).isEmpty();
update = newUpdate(c, otherUser);
@@ -2114,7 +2113,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
notes = newNotes(c);
assertThat(notes.getDraftComments(otherUserId)).isEmpty();
assertThat(notes.getComments()).containsExactlyEntriesIn(
ImmutableMultimap.of(new RevId(rev), comment1));
ImmutableListMultimap.of(new RevId(rev), comment1));
}
@Test
@@ -2146,7 +2145,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeNotes notes = newNotes(c);
assertThat(notes.getDraftComments(otherUserId)).containsExactlyEntriesIn(
ImmutableMultimap.of(
ImmutableListMultimap.of(
new RevId(rev), comment1,
new RevId(rev), comment2)).inOrder();
assertThat(notes.getComments()).isEmpty();
@@ -2159,9 +2158,9 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
notes = newNotes(c);
assertThat(notes.getDraftComments(otherUserId)).containsExactlyEntriesIn(
ImmutableMultimap.of(new RevId(rev), comment2));
ImmutableListMultimap.of(new RevId(rev), comment2));
assertThat(notes.getComments()).containsExactlyEntriesIn(
ImmutableMultimap.of(new RevId(rev), comment1));
ImmutableListMultimap.of(new RevId(rev), comment1));
}
@Test
@@ -2194,7 +2193,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
ChangeNotes notes = newNotes(c);
assertThat(notes.getDraftComments(otherUserId)).containsExactlyEntriesIn(
ImmutableMultimap.of(
ImmutableListMultimap.of(
new RevId(rev1), baseComment,
new RevId(rev2), psComment));
assertThat(notes.getComments()).isEmpty();
@@ -2210,7 +2209,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
notes = newNotes(c);
assertThat(notes.getDraftComments(otherUserId)).isEmpty();
assertThat(notes.getComments()).containsExactlyEntriesIn(
ImmutableMultimap.of(
ImmutableListMultimap.of(
new RevId(rev1), baseComment,
new RevId(rev2), psComment));
}
@@ -2373,7 +2372,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.commit();
assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
ImmutableMultimap.of(new RevId(rev), comment));
ImmutableListMultimap.of(new RevId(rev), comment));
}
@Test
@@ -2393,7 +2392,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.commit();
assertThat(newNotes(c).getComments()).containsExactlyEntriesIn(
ImmutableMultimap.of(new RevId(rev), comment));
ImmutableListMultimap.of(new RevId(rev), comment));
}
@Test