opendev/gerrit
Code Issues Proposed changes

PatchSet: Always use non-null Lists for groups

Browse Source 
Although groups may be "missing" due to patch sets being created prior
to the schema migration that added that field, the fact that these
are represented in the database as null is an implementation detail.
In practice we do not distinguish between empty groups and null
groups, so potentially returning both of them from the API is
confusing. Note that we definitely don't distinguish between these in
the secondary index.

Require that groups always be a non-null, and check for an empty group
list to determine whether groups are "missing". This also means it's
natural to use a List<String> for groups, which are naturally an
ordered list. The one place where this doesn't hold is when coming out
of GroupCollector, which for internal reasons needs to use a Set for
deduplication. Have the few callers of that class copy the result to a
list.

Change-Id: I0b276e16b3ac02fd882ff1cb23d5dfd1fc7bcd20
This commit is contained in:
Dave Borowitz
2016-01-28 15:20:49 -05:00
committed by Edwin Kempin
parent b3d7f7328e
commit a714938965
17 changed files with 65 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
View File

@@ -572,7 +572,7 @@ public class GetRelatedIT extends AbstractDaemonTest {
}
// Pretend PS1,1 was pushed before the groups field was added.
setGroups(psId1_1, null);
clearGroups(psId1_1);
indexer.index(changeDataFactory.create(db, psId1_1.getParentKey()));
// PS1,1 has no groups, so disappeared from related changes.
@@ -629,15 +629,15 @@ public class GetRelatedIT extends AbstractDaemonTest {
return result;
}
private void setGroups(final PatchSet.Id psId,
final Iterable<String> groups) throws Exception {
private void clearGroups(final PatchSet.Id psId) throws Exception {
try (BatchUpdate bu = updateFactory.create(
db, project, user(user), TimeUtil.nowTs())) {
bu.addOp(psId.getParentKey(), new BatchUpdate.Op() {
@Override
public boolean updateChange(ChangeContext ctx) throws OrmException {
PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
psUtil.setGroups(ctx.getDb(), ctx.getUpdate(psId), ps, groups);
psUtil.setGroups(ctx.getDb(), ctx.getUpdate(psId), ps,
ImmutableList.<String> of());
return true;
}
});

15
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java
View File

@@ -19,6 +19,7 @@ import com.google.gwtorm.client.IntKey;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
/** A single revision of a {@link Change}. */
@@ -38,9 +39,9 @@ public final class PatchSet {
return isChangeRef(name);
}
public static String joinGroups(Iterable<String> groups) {
static String joinGroups(List<String> groups) {
if (groups == null) {
return null;
throw new IllegalArgumentException("groups may not be null");
}
StringBuilder sb = new StringBuilder();
boolean first = true;
@@ -57,7 +58,7 @@ public final class PatchSet {
public static List<String> splitGroups(String joinedGroups) {
if (joinedGroups == null) {
return null;
throw new IllegalArgumentException("groups may not be null");
}
List<String> groups = new ArrayList<>();
int i = 0;
@@ -241,10 +242,16 @@ public final class PatchSet {
}
public List<String> getGroups() {
if (groups == null) {
return Collections.emptyList();
}
return splitGroups(groups);
}
public void setGroups(Iterable<String> groups) {
public void setGroups(List<String> groups) {
if (groups == null) {
groups = Collections.emptyList();
}
this.groups = joinGroups(groups);
}

2
gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/PatchSetTest.java
View File

@@ -65,7 +65,6 @@ public class PatchSetTest {
@Test
public void testSplitGroups() {
assertThat(splitGroups(null)).isNull();
assertThat(splitGroups("")).containsExactly("");
assertThat(splitGroups("abcd")).containsExactly("abcd");
assertThat(splitGroups("ab,cd")).containsExactly("ab", "cd").inOrder();
@@ -75,7 +74,6 @@ public class PatchSetTest {
@Test
public void testJoinGroups() {
assertThat(joinGroups(null)).isNull();
assertThat(joinGroups(ImmutableList.of(""))).isEqualTo("");
assertThat(joinGroups(ImmutableList.of("abcd"))).isEqualTo("abcd");
assertThat(joinGroups(ImmutableList.of("ab", "cd"))).isEqualTo("ab,cd");

8
gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gerrit.server.notedb.PatchSetState.DRAFT;
import static com.google.gerrit.server.notedb.PatchSetState.PUBLISHED;
@@ -37,6 +38,7 @@ import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.Collections;
import java.util.List;
/** Utilities for manipulating patch sets. */
@Singleton
@@ -69,8 +71,9 @@ public class PatchSetUtil {
public PatchSet insert(ReviewDb db, RevWalk rw, ChangeUpdate update,
PatchSet.Id psId, ObjectId commit, boolean draft,
Iterable<String> groups, String pushCertificate)
List<String> groups, String pushCertificate)
throws OrmException, IOException {
checkNotNull(groups, "groups may not be null");
ensurePatchSetMatches(psId, update);
PatchSet ps = new PatchSet(psId);
@@ -83,6 +86,7 @@ public class PatchSetUtil {
db.patchSets().insert(Collections.singleton(ps));
update.setCommit(rw, commit, pushCertificate);
update.setGroups(groups);
if (draft) {
update.setPatchSetState(DRAFT);
}
@@ -121,7 +125,7 @@ public class PatchSetUtil {
}
public void setGroups(ReviewDb db, ChangeUpdate update, PatchSet ps,
Iterable<String> groups) throws OrmException {
List<String> groups) throws OrmException {
ps.setGroups(groups);
update.setGroups(groups);
db.patchSets().update(Collections.singleton(ps));

9
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@@ -100,7 +100,7 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
private Change.Status status;
private String topic;
private String message;
private Iterable<String> groups;
private List<String> groups = Collections.emptyList();
private CommitValidators.Policy validatePolicy =
CommitValidators.Policy.GERRIT;
private NotifyHandling notify = NotifyHandling.ALL;
@@ -238,7 +238,8 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
return this;
}
public ChangeInserter setGroups(Iterable<String> groups) {
public ChangeInserter setGroups(List<String> groups) {
checkNotNull(groups, "groups may not be empty");
checkState(patchSet == null,
"setGroups(Iterable<String>) only valid before creating change");
this.groups = groups;
@@ -319,8 +320,8 @@ public class ChangeInserter extends BatchUpdate.InsertChangeOp {
update.setTopic(change.getTopic());
boolean draft = status == Change.Status.DRAFT;
Iterable<String> newGroups = groups;
if (newGroups == null) {
List<String> newGroups = groups;
if (newGroups.isEmpty()) {
newGroups = GroupCollector.getDefaultGroups(commit);
}
patchSet = psUtil.insert(ctx.getDb(), ctx.getRevWalk(), update, psId,

3
gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
View File

@@ -68,6 +68,7 @@ import org.eclipse.jgit.util.ChangeIdUtil;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.sql.Timestamp;
import java.util.Collections;
import java.util.List;
import java.util.TimeZone;
@@ -182,7 +183,7 @@ public class CreateChange implements
"Branch %s does not exist.", refName));
}
parentCommit = destRef.getObjectId();
groups = null;
groups = Collections.emptyList();
}
RevCommit mergeTip = rw.parseCommit(parentCommit);

5
gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java
View File

@@ -117,10 +117,7 @@ public class GetRelated implements RestReadView<RevisionResource> {
private Set<String> getAllGroups(ChangeNotes notes) throws OrmException {
Set<String> result = new HashSet<>();
for (PatchSet ps : psUtil.byChange(db.get(), notes)) {
List<String> groups = ps.getGroups();
if (groups != null) {
result.addAll(groups);
}
result.addAll(ps.getGroups());
}
return result;
}

11
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -61,6 +61,8 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.Collections;
import java.util.List;
public class PatchSetInserter extends BatchUpdate.Op {
private static final Logger log =
@@ -93,7 +95,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
private CommitValidators.Policy validatePolicy =
CommitValidators.Policy.GERRIT;
private boolean draft;
private Iterable<String> groups;
private List<String> groups = Collections.emptyList();
private boolean runHooks = true;
private boolean sendMail = true;
private boolean allowClosed;
@@ -158,7 +160,8 @@ public class PatchSetInserter extends BatchUpdate.Op {
return this;
}
public PatchSetInserter setGroups(Iterable<String> groups) {
public PatchSetInserter setGroups(List<String> groups) {
checkNotNull(groups, "groups may not be null");
this.groups = groups;
return this;
}
@@ -217,8 +220,8 @@ public class PatchSetInserter extends BatchUpdate.Op {
change.getId(), change.getStatus().name().toLowerCase()));
}
Iterable<String> newGroups = groups;
if (newGroups == null) {
List<String> newGroups = groups;
if (newGroups.isEmpty()) {
PatchSet prevPs = psUtil.current(ctx.getDb(), ctx.getNotes());
newGroups = prevPs != null ? prevPs.getGroups() : null;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
View File

@@ -313,7 +313,7 @@ public class EventFactory {
private void addNeededBy(RevWalk rw, ChangeAttribute ca, Change change,
PatchSet currentPs) throws OrmException, IOException {
if (currentPs.getGroups() == null || currentPs.getGroups().isEmpty()) {
if (currentPs.getGroups().isEmpty()) {
return;
}
String rev = currentPs.getRevision().get();

5
gerrit-server/src/main/java/com/google/gerrit/server/git/GroupCollector.java
View File

@@ -30,6 +30,7 @@ import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.Multimaps;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.common.collect.SortedSetMultimap;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -241,9 +242,9 @@ public class GroupCollector {
}
}
public SetMultimap<ObjectId, String> getGroups() throws OrmException {
public SortedSetMultimap<ObjectId, String> getGroups() throws OrmException {
done = true;
SetMultimap<ObjectId, String> result = MultimapBuilder
SortedSetMultimap<ObjectId, String> result = MultimapBuilder
.hashKeys(groups.keySet().size())
.treeSetValues()
.build();

25
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -40,15 +40,16 @@ import com.google.common.collect.BiMap;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.HashBiMap;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.Ordering;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.common.collect.SortedSetMultimap;
import com.google.common.util.concurrent.CheckedFuture;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
@@ -1690,16 +1691,16 @@ public class ReceiveCommits {
}
try {
Multimap<ObjectId, String> groups = groupCollector.getGroups();
SortedSetMultimap<ObjectId, String> groups = groupCollector.getGroups();
for (CreateRequest create : newChanges) {
batch.addCommand(create.cmd);
create.groups = groups.get(create.commitId);
create.groups = ImmutableList.copyOf(groups.get(create.commitId));
}
for (ReplaceRequest replace : replaceByChange.values()) {
replace.groups = groups.get(replace.newCommitId);
replace.groups = ImmutableList.copyOf(groups.get(replace.newCommitId));
}
for (UpdateGroupsRequest update : updateGroups) {
update.groups = Sets.newHashSet(groups.get(update.commit));
update.groups = ImmutableList.copyOf((groups.get(update.commit)));
}
} catch (OrmException e) {
log.error("Error collecting groups for changes", e);
@@ -1744,7 +1745,7 @@ public class ReceiveCommits {
final ChangeInserter ins;
Change.Id changeId;
Change change;
Collection<String> groups;
List<String> groups = ImmutableList.of();
CreateRequest(RevCommit c, String refName)
throws OrmException {
@@ -1974,7 +1975,7 @@ public class ReceiveCommits {
String mergedIntoRef;
boolean skip;
private PatchSet.Id priorPatchSet;
Collection<String> groups;
List<String> groups = ImmutableList.of();
ReplaceRequest(final Change.Id toChange, final RevCommit newCommit,
final ReceiveCommand cmd, final boolean checkMergedInto) {
@@ -2276,10 +2277,12 @@ public class ReceiveCommits {
return null;
}
Iterable<String> newGroups = groups;
if (newGroups == null) {
List<String> newGroups = groups;
if (newGroups.isEmpty()) {
PatchSet prevPs = psUtil.current(db, update.getChangeNotes());
newGroups = prevPs != null ? prevPs.getGroups() : null;
newGroups = prevPs != null
? prevPs.getGroups()
: ImmutableList.<String> of();
}
boolean draft = magicBranch != null && magicBranch.draft;
@@ -2429,7 +2432,7 @@ public class ReceiveCommits {
private class UpdateGroupsRequest {
private final PatchSet.Id psId;
private final RevCommit commit;
Set<String> groups;
List<String> groups = ImmutableList.of();
UpdateGroupsRequest(Ref ref, RevCommit commit) {
this.psId = checkNotNull(PatchSet.Id.fromRef(ref.getName()));

4
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.git.strategy;
import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
@@ -151,7 +152,8 @@ public class CherryPick extends SubmitStrategy {
PatchSet prevPs = args.psUtil.current(ctx.getDb(), ctx.getNotes());
PatchSet newPs = args.psUtil.insert(ctx.getDb(), ctx.getRevWalk(),
ctx.getUpdate(psId), psId, newCommit, false,
prevPs != null ? prevPs.getGroups() : null, null);
prevPs != null ? prevPs.getGroups() : ImmutableList.<String> of(),
null);
ctx.getChange().setCurrentPatchSet(patchSetInfo);
ctx.saveChange();

5
gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
View File

@@ -599,10 +599,7 @@ public class ChangeField {
throws OrmException {
Set<String> r = Sets.newHashSetWithExpectedSize(1);
for (PatchSet ps : input.patchSets()) {
List<String> groups = ps.getGroups();
if (groups != null) {
r.addAll(groups);
}
r.addAll(ps.getGroups());
}
return r;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@@ -348,7 +348,7 @@ class ChangeNotesParser implements AutoCloseable {
ps = new PatchSet(psId);
ps.setRevision(PARTIAL_PATCH_SET);
patchSets.put(psId, ps);
} else if (ps.getGroups() != null) {
} else if (!ps.getGroups().isEmpty()) {
return;
}
ps.setGroups(PatchSet.splitGroups(groupsStr));

4
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_BRANCH;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_CHANGE_ID;
@@ -411,7 +412,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
this.psState = psState;
}
public void setGroups(Iterable<String> groups) {
public void setGroups(List<String> groups) {
checkNotNull(groups, "groups may not be null");
this.groups = groups;
}

3
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_108.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.schema;
import com.google.common.base.Joiner;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Multimap;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
@@ -138,7 +139,7 @@ public class Schema_108 extends SchemaVersion {
for (PatchSet.Id psId : patchSetsBySha.get(e.getKey())) {
PatchSet ps = patchSets.get(psId);
if (ps != null) {
ps.setGroups(e.getValue());
ps.setGroups(ImmutableList.copyOf(e.getValue()));
}
}
}

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

@@ -689,7 +689,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
PatchSet.Id psId1 = c.currentPatchSetId();
ChangeNotes notes = newNotes(c);
assertThat(notes.getPatchSets().get(psId1).getGroups()).isNull();
assertThat(notes.getPatchSets().get(psId1).getGroups()).isEmpty();
// ps1
ChangeUpdate update = newUpdate(c, changeOwner);