opendev/gerrit
Code Issues Proposed changes

Java-8ify ReviewDbUtil

Browse Source 
We can use method references within ReviewDbUtil to cut down on some
code. However, going through usages didn't show a uniform benefit to
using native Java 8 Comparator constructs. The naive replacement:

  comparing(Change.Id::get)

is shorter and more readable when it's correct, but it's not always
correct in the presence of nulls.

Ordering is also probably not going away from Guava, as it provides
some handy methods that are not (yet?) present on Comparator. For
example, creating a sorted copy of a Collection with Comparator is
arguably readable, but is not exactly short:

  myList.stream().sorted(comparing(Change.Id::get)).collect(toList())

Compared with the Ordering alernative:

  ReviewDbUtil.intKeyOrdering().sortedCopy(myList)

This benefit is greater when dealing with Iterable, which does not
have a readily available stream() method.

Leave some guidance in the intKeyOrdering Javadoc with advice.

Change-Id: Ib645abf060302cfc1c6d83f691e94f1dbf7c6db5
This commit is contained in:
Dave Borowitz
2016-09-20 09:01:21 -04:00
parent e45363dcbd
commit e128d6f413
9 changed files with 53 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java
View File

@@ -14,28 +14,37 @@
package com.google.gerrit.reviewdb.server;
import com.google.common.base.Function;
import com.google.common.collect.Ordering;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gwtorm.client.IntKey;
/** Static utilities for ReviewDb types. */
public class ReviewDbUtil {
public static final Function<IntKey<?>, Integer> INT_KEY_FUNCTION =
IntKey::get;
private static final Ordering<? extends IntKey<?>> INT_KEY_ORDERING =
Ordering.natural().nullsFirst().onResultOf(INT_KEY_FUNCTION).nullsFirst();
Ordering.natural()
.nullsFirst()
.<IntKey<?>>onResultOf(IntKey::get)
.nullsFirst();
/**
* Null-safe ordering over arbitrary subclass of {@code IntKey}.
* <p>
* In some cases, {@code Comparator.comparing(Change.Id::get)} may be shorter
* and cleaner. However, this method may be preferable in some cases:
* <ul>
* <li>This ordering is null-safe over both input and the result of {@link
* IntKey#get()}; {@code comparing} is only a good idea if all inputs are
* obviously non-null.</li>
* <li>{@code intKeyOrdering().sortedCopy(iterable)} is shorter than the
* stream equivalent.</li>
* <li>Creating derived comparators may be more readable with {@link Ordering}
* method chaining rather than static {@code Comparator} methods.
* </ul>
*/
@SuppressWarnings("unchecked")
public static <K extends IntKey<?>> Ordering<K> intKeyOrdering() {
return (Ordering<K>) INT_KEY_ORDERING;
}
public static Function<Change, Change.Id> changeIdFunction() {
return Change::getId;
}
public static ReviewDb unwrapDb(ReviewDb db) {
if (db instanceof DisabledChangesReviewDbWrapper) {
return ((DisabledChangesReviewDbWrapper) db).unsafeGetDelegate();

3
gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.git;
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 java.util.Comparator.comparing;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.NANOSECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
@@ -226,7 +227,7 @@ public class BatchUpdate implements AutoCloseable {
this.dbWrapper = dbWrapper;
this.threadLocalRepo = repo;
this.threadLocalRevWalk = rw;
updates = new TreeMap<>(ReviewDbUtil.intKeyOrdering());
updates = new TreeMap<>(comparing(PatchSet.Id::get));
}
@Override

8
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
View File

@@ -182,13 +182,7 @@ abstract class SubmitStrategyOp extends BatchUpdate.Op {
}
}
Collections.sort(commits, ReviewDbUtil.intKeyOrdering().reverse()
.onResultOf(
new Function<CodeReviewCommit, PatchSet.Id>() {
@Override
public PatchSet.Id apply(CodeReviewCommit in) {
return in.getPatchsetId();
}
}));
.onResultOf(c -> c.getPatchsetId()));
CodeReviewCommit result = MergeUtil.findAnyMergedInto(rw, commits, tip);
if (result == null) {
return null;

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

@@ -34,7 +34,6 @@ import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.index.FieldDef;
@@ -694,8 +693,7 @@ public class ChangeField {
@Override
public Iterable<Integer> get(ChangeData input, FillArgs args)
throws OrmException {
return Iterables.transform(input.stars().keySet(),
ReviewDbUtil.INT_KEY_FUNCTION);
return Iterables.transform(input.stars().keySet(), Account.Id::get);
}
};

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

@@ -29,6 +29,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WI
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TAG;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC;
import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
import static java.util.Comparator.comparing;
import com.google.auto.value.AutoValue;
import com.google.common.base.Enums;
@@ -60,7 +61,6 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
@@ -165,7 +165,7 @@ class ChangeNotesParser {
allChangeMessages = new ArrayList<>();
changeMessagesByPatchSet = LinkedListMultimap.create();
comments = ArrayListMultimap.create();
patchSets = Maps.newTreeMap(ReviewDbUtil.intKeyOrdering());
patchSets = Maps.newTreeMap(comparing(PatchSet.Id::get));
deletedPatchSets = new HashSet<>();
patchSetStates = new HashMap<>();
}

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

@@ -15,6 +15,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkNotNull;
import static java.util.Comparator.comparing;
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
@@ -33,7 +34,6 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
@@ -115,7 +115,7 @@ public abstract class ChangeNotesState {
status),
assignee,
ImmutableSet.copyOf(hashtags),
ImmutableSortedMap.copyOf(patchSets, ReviewDbUtil.intKeyOrdering()),
ImmutableSortedMap.copyOf(patchSets, comparing(PatchSet.Id::get)),
ImmutableListMultimap.copyOf(approvals),
reviewers,
ImmutableList.copyOf(allPastReviewers),

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

@@ -33,6 +33,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMISSION_I
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_SUBMITTED_WITH;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TAG;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC;
import static java.util.Comparator.comparing;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import com.google.common.annotations.VisibleForTesting;
@@ -49,7 +50,6 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.config.AnonymousCowardName;
@@ -57,6 +57,7 @@ import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.util.RequestId;
import com.google.gwtorm.client.IntKey;
import com.google.gwtorm.server.OrmException;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
@@ -175,7 +176,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
private static Table<String, Account.Id, Optional<Short>> approvals(
Comparator<String> nameComparator) {
return TreeBasedTable.create(nameComparator, ReviewDbUtil.intKeyOrdering());
return TreeBasedTable.create(nameComparator, comparing(IntKey::get));
}
@AssistedInject

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

@@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
import static java.util.Comparator.comparing;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
@@ -30,7 +31,6 @@ import com.google.common.collect.Maps;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.git.RefCache;
import org.eclipse.jgit.lib.ObjectId;
@@ -163,7 +163,7 @@ public class NoteDbChangeState {
public static String toString(ObjectId changeMetaId,
Map<Account.Id, ObjectId> draftIds) {
List<Account.Id> accountIds = Lists.newArrayList(draftIds.keySet());
Collections.sort(accountIds, ReviewDbUtil.intKeyOrdering());
Collections.sort(accountIds, comparing(Account.Id::get));
StringBuilder sb = new StringBuilder(changeMetaId.name());
for (Account.Id id : accountIds) {
sb.append(',')

41
gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbChecker.java
View File

@@ -15,9 +15,10 @@
package com.google.gerrit.testutil;
import static com.google.common.truth.Truth.assertThat;
import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.toList;
import com.google.common.base.Joiner;
import com.google.common.collect.Iterables;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
@@ -29,6 +30,9 @@ import com.google.gerrit.server.notedb.ChangeBundle;
import com.google.gerrit.server.notedb.ChangeBundleReader;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder;
import com.google.gwtorm.client.IntKey;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.OrmRuntimeException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -41,6 +45,7 @@ import org.slf4j.LoggerFactory;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Stream;
@Singleton
public class NoteDbChecker {
@@ -73,16 +78,14 @@ public class NoteDbChecker {
public void rebuildAndCheckAllChanges() throws Exception {
rebuildAndCheckChanges(
Iterables.transform(
getUnwrappedDb().changes().all(),
ReviewDbUtil.changeIdFunction()));
getUnwrappedDb().changes().all().toList().stream().map(Change::getId));
}
public void rebuildAndCheckChanges(Change.Id... changeIds) throws Exception {
rebuildAndCheckChanges(Arrays.asList(changeIds));
rebuildAndCheckChanges(Arrays.stream(changeIds));
}
public void rebuildAndCheckChanges(Iterable<Change.Id> changeIds)
private void rebuildAndCheckChanges(Stream<Change.Id> changeIds)
throws Exception {
ReviewDb db = getUnwrappedDb();
@@ -111,11 +114,7 @@ public class NoteDbChecker {
}
public void checkChanges(Change.Id... changeIds) throws Exception {
checkChanges(Arrays.asList(changeIds));
}
public void checkChanges(Iterable<Change.Id> changeIds) throws Exception {
checkActual(readExpected(changeIds), new ArrayList<String>());
checkActual(readExpected(Arrays.stream(changeIds)), new ArrayList<>());
}
public void assertNoChangeRef(Project.NameKey project, Change.Id changeId)
@@ -125,24 +124,26 @@ public class NoteDbChecker {
}
}
private List<ChangeBundle> readExpected(Iterable<Change.Id> changeIds)
private List<ChangeBundle> readExpected(Stream<Change.Id> changeIds)
throws Exception {
ReviewDb db = getUnwrappedDb();
boolean old = notesMigration.readChanges();
try {
notesMigration.setReadChanges(false);
List<Change.Id> sortedIds =
ReviewDbUtil.intKeyOrdering().sortedCopy(changeIds);
List<ChangeBundle> expected = new ArrayList<>(sortedIds.size());
for (Change.Id id : sortedIds) {
expected.add(bundleReader.fromReviewDb(db, id));
}
return expected;
return changeIds.sorted(comparing(IntKey::get))
.map(this::readBundleUnchecked).collect(toList());
} finally {
notesMigration.setReadChanges(old);
}
}
private ChangeBundle readBundleUnchecked(Change.Id id) {
try {
return bundleReader.fromReviewDb(getUnwrappedDb(), id);
} catch (OrmException e) {
throw new OrmRuntimeException(e);
}
}
private void checkActual(List<ChangeBundle> allExpected, List<String> msgs)
throws Exception {
ReviewDb db = getUnwrappedDb();