From 74c8f429f41034dd575959b7144bf69089ed6369 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 14 Dec 2018 15:07:19 -0800 Subject: [PATCH] Remove ReviewDbUtil#intKeyOrdering() None of the callers are subject to the null safety issue described in the Javadoc. Although it's true that some callers require more code to use the equivalent stream API, it's still plenty readable. On balance, it doesn't justify having this gwtorm-specific helper. Change-Id: Ic5b9dc07566ec9bf20da9a0ac10b9cf95d78386b --- .../gerrit/reviewdb/server/ReviewDbUtil.java | 24 ------------------- .../server/change/ConsistencyChecker.java | 16 ++++++++----- .../server/notedb/ChangeNotesParser.java | 4 ++-- .../server/submit/SubmitStrategyOp.java | 5 ++-- .../gerrit/server/notedb/ChangeNotesTest.java | 14 +++++++---- 5 files changed, 23 insertions(+), 40 deletions(-) diff --git a/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java b/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java index 1f0f12f02b..840945361b 100644 --- a/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java +++ b/java/com/google/gerrit/reviewdb/server/ReviewDbUtil.java @@ -16,10 +16,8 @@ package com.google.gerrit.reviewdb.server; import static com.google.common.base.Preconditions.checkState; -import com.google.common.collect.Ordering; import com.google.common.collect.Sets; import com.google.gwtorm.client.Column; -import com.google.gwtorm.client.IntKey; import java.lang.reflect.Field; import java.util.Arrays; import java.util.Set; @@ -27,28 +25,6 @@ import java.util.TreeSet; /** Static utilities for ReviewDb types. */ public class ReviewDbUtil { - private static final Ordering> INT_KEY_ORDERING = - Ordering.natural().nullsFirst().>onResultOf(IntKey::get).nullsFirst(); - - /** - * Null-safe ordering over arbitrary subclass of {@code IntKey}. - * - *

In some cases, {@code Comparator.comparing(Change.Id::get)} may be shorter and cleaner. - * However, this method may be preferable in some cases: - * - *

- */ - @SuppressWarnings("unchecked") - public static > Ordering intKeyOrdering() { - return (Ordering) INT_KEY_ORDERING; - } - public static ReviewDb unwrapDb(ReviewDb db) { if (db instanceof DisallowedReviewDb) { return unwrapDb(((DisallowedReviewDb) db).unsafeGetDelegate()); diff --git a/java/com/google/gerrit/server/change/ConsistencyChecker.java b/java/com/google/gerrit/server/change/ConsistencyChecker.java index c819316300..7a553e3a53 100644 --- a/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -15,9 +15,10 @@ package com.google.gerrit.server.change; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES; -import static com.google.gerrit.reviewdb.server.ReviewDbUtil.intKeyOrdering; import static com.google.gerrit.server.ChangeUtil.PS_ID_ORDER; +import static java.util.Comparator.comparing; import static java.util.Objects.requireNonNull; import com.google.auto.value.AutoValue; @@ -37,7 +38,6 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; -import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; @@ -67,6 +67,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.TreeSet; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; @@ -457,7 +458,11 @@ public class ConsistencyChecker { problem( String.format( "Multiple patch sets for expected merged commit %s: %s", - commit.name(), intKeyOrdering().sortedCopy(thisCommitPsIds))); + commit.name(), + thisCommitPsIds + .stream() + .sorted(comparing(PatchSet.Id::get)) + .collect(toImmutableList()))); break; } } catch (IOException e) { @@ -695,7 +700,7 @@ public class ConsistencyChecker { if (!toDelete.contains(ctx.getChange().currentPatchSetId())) { return false; } - Set all = new HashSet<>(); + TreeSet all = new TreeSet<>(comparing(PatchSet.Id::get)); // Doesn't make any assumptions about the order in which deletes happen // and whether they are seen by this op; we are already given the full set // of patch sets that will eventually be deleted in this update. @@ -707,8 +712,7 @@ public class ConsistencyChecker { if (all.isEmpty()) { throw new NoPatchSetsWouldRemainException(); } - PatchSet.Id latest = ReviewDbUtil.intKeyOrdering().max(all); - ctx.getChange().setCurrentPatchSet(patchSetInfoFactory.get(ctx.getNotes(), latest)); + ctx.getChange().setCurrentPatchSet(patchSetInfoFactory.get(ctx.getNotes(), all.last())); return true; } } diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 890febb25e..e53a37f073 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -37,6 +37,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC; import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_WORK_IN_PROGRESS; import static com.google.gerrit.server.notedb.ChangeNoteUtil.parseCommitMessageRange; import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES; +import static java.util.Comparator.comparing; import static java.util.stream.Collectors.joining; import com.google.auto.value.AutoValue; @@ -67,7 +68,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.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; @@ -1004,7 +1004,7 @@ class ChangeNotesParser { } private void updatePatchSetStates() { - Set missing = new TreeSet<>(ReviewDbUtil.intKeyOrdering()); + Set missing = new TreeSet<>(comparing(PatchSet.Id::get)); for (Iterator it = patchSets.values().iterator(); it.hasNext(); ) { PatchSet ps = it.next(); if (ps.getRevision().equals(PARTIAL_PATCH_SET)) { diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java index 5d49f648ba..2d8e52beb7 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.submit; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; +import static java.util.Comparator.comparing; import static java.util.Objects.requireNonNull; import com.google.common.base.Function; @@ -32,7 +33,6 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; -import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.IdentifiedUser; @@ -182,8 +182,7 @@ abstract class SubmitStrategyOp implements BatchUpdateOp { continue; // Bogus ref, can't be merged into tip so we don't care. } } - commits.sort( - ReviewDbUtil.intKeyOrdering().reverse().onResultOf(CodeReviewCommit::getPatchsetId)); + commits.sort(comparing((CodeReviewCommit c) -> c.getPatchsetId().get()).reversed()); CodeReviewCommit result = MergeUtil.findAnyMergedInto(rw, commits, tip); if (result == null) { return null; diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java index 21443dca3e..8892e84b22 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.notedb; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef; import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments; @@ -21,6 +22,7 @@ import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC; import static com.google.gerrit.server.notedb.ReviewerStateInternal.REMOVED; import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Comparator.comparing; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; import static org.junit.Assert.fail; @@ -44,7 +46,6 @@ import com.google.gerrit.reviewdb.client.PatchLineComment.Status; 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.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.ReviewerSet; @@ -405,10 +406,13 @@ public class ChangeNotesTest extends AbstractChangeNotesTest { update.commit(); ChangeNotes notes = newNotes(c); - List approvals = - ReviewDbUtil.intKeyOrdering() - .onResultOf(PatchSetApproval::getAccountId) - .sortedCopy(notes.getApprovals().get(c.currentPatchSetId())); + ImmutableList approvals = + notes + .getApprovals() + .get(c.currentPatchSetId()) + .stream() + .sorted(comparing(a -> a.getAccountId().get())) + .collect(toImmutableList()); assertThat(approvals).hasSize(2); assertThat(approvals.get(0).getAccountId()).isEqualTo(changeOwner.getAccountId());