Copy approvals to the latest PS by matching labels with submit records
Background: we store submit records in NoteDb when the change is merged.
Submit records optionally contain a list of labels and the account ID
that applied the vote, but they do not contain the vote value.
Problem we are solving: approvals from prior patchsets are no longer
copied on demand but approvals are solely read from NoteDb. For old
changes copied approvals are not stored in NoteDb because they were not
backfilled. For merged changes, this means that approvals for labels
that were required for submit and that were satisfied by copied votes
are no longer shown, so we need to backfill them.
In this change, we update reading back approvals from NoteDb for closed
changes: If we find a submit record with an approved label from some
user and we don't find a corresponding approval for it at the latest
patchset, we search for the most recent non-latest patchset that
contains an approval for this label from the user and copy it to the
latest patchset.
Change-Id: I58ea31a5fec033f28cdb9e51f96ea47676816d76
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
index c554ca5..b8b8a2c 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
@@ -61,7 +61,7 @@
.weigher(Weigher.class)
.maximumWeight(10 << 20)
.diskLimit(-1)
- .version(2)
+ .version(3)
.keySerializer(Key.Serializer.INSTANCE)
.valueSerializer(ChangeNotesState.Serializer.INSTANCE);
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 5cf3a64..a3a2b4b 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -40,11 +40,13 @@
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_WORK_IN_PROGRESS;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.parseCommitMessageRange;
import static java.util.Comparator.comparing;
+import static java.util.Comparator.comparingInt;
import static java.util.stream.Collectors.joining;
import com.google.common.base.Enums;
import com.google.common.base.Splitter;
import com.google.common.collect.HashBasedTable;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableTable;
import com.google.common.collect.ListMultimap;
@@ -70,6 +72,7 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRecord;
+import com.google.gerrit.entities.SubmitRecord.Label.Status;
import com.google.gerrit.entities.SubmitRequirementResult;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.server.AssigneeStatusUpdate;
@@ -83,6 +86,7 @@
import java.sql.Timestamp;
import java.time.Instant;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
@@ -95,6 +99,7 @@
import java.util.Set;
import java.util.TreeSet;
import java.util.function.Function;
+import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.lib.ObjectId;
@@ -312,10 +317,81 @@
}
result.put(a.key().patchSetId(), a.build());
}
+ if (status != null && status.isClosed() && !isAnyApprovalCopied(result)) {
+ // If the change is closed, check if there are "submit records" with approvals that do not
+ // exist on the latest patch-set and copy them to the latest patch-set.
+ // We do not invoke this logic if any approval is copied. This is because prior to change
+ // https://gerrit-review.googlesource.com/c/gerrit/+/318135 we used to copy approvals
+ // dynamically (e.g. when requesting the change page). After that change, we started
+ // persisting copied votes in NoteDb, so we don't need to do this back-filling.
+ // Prior to that change (318135), we could've had changes with dynamically copied approvals
+ // that were merged in NoteDb but these approvals do not exist on the latest patch-set, so
+ // we need to back-fill these approvals.
+ PatchSet.Id latestPs = buildCurrentPatchSetId();
+ backFillMissingCopiedApprovalsFromSubmitRecords(result, latestPs).stream()
+ .forEach(a -> result.put(latestPs, a));
+ }
result.keySet().forEach(k -> result.get(k).sort(ChangeNotes.PSA_BY_TIME));
return result;
}
+ /**
+ * Returns patch-set approvals that do not exist on the latest patch-set but for which a submit
+ * record exists in NoteDb when the change was merged.
+ */
+ private List<PatchSetApproval> backFillMissingCopiedApprovalsFromSubmitRecords(
+ ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals, @Nullable PatchSet.Id latestPs) {
+ List<PatchSetApproval> copiedApprovals = new ArrayList<>();
+ if (latestPs == null) {
+ return copiedApprovals;
+ }
+ List<PatchSetApproval> approvalsOnLatestPs = allApprovals.get(latestPs);
+ ListMultimap<Account.Id, PatchSetApproval> approvalsByUser = getApprovalsByUser(allApprovals);
+ List<SubmitRecord.Label> submitRecordLabels =
+ submitRecords.stream()
+ .filter(r -> r.labels != null)
+ .flatMap(r -> r.labels.stream())
+ .filter(label -> Status.OK.equals(label.status) || Status.MAY.equals(label.status))
+ .collect(Collectors.toList());
+ for (SubmitRecord.Label recordLabel : submitRecordLabels) {
+ String labelName = recordLabel.label;
+ Account.Id appliedBy = recordLabel.appliedBy;
+ if (appliedBy == null || labelName == null) {
+ continue;
+ }
+ boolean existsAtLatestPs =
+ approvalsOnLatestPs.stream()
+ .anyMatch(a -> a.accountId().equals(appliedBy) && a.label().equals(labelName));
+ if (existsAtLatestPs) {
+ continue;
+ }
+ // Search for an approval for this label on the max previous patch-set and copy the approval.
+ Collection<PatchSetApproval> userApprovals =
+ approvalsByUser.get(appliedBy).stream()
+ .filter(approval -> approval.label().equals(labelName))
+ .collect(Collectors.toList());
+ if (userApprovals.isEmpty()) {
+ continue;
+ }
+ PatchSetApproval lastApproved =
+ Collections.max(userApprovals, comparingInt(a -> a.patchSetId().get()));
+ copiedApprovals.add(lastApproved.copyWithPatchSet(latestPs));
+ }
+ return copiedApprovals;
+ }
+
+ private boolean isAnyApprovalCopied(ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals) {
+ return allApprovals.values().stream().anyMatch(approval -> approval.copied());
+ }
+
+ private ListMultimap<Account.Id, PatchSetApproval> getApprovalsByUser(
+ ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals) {
+ return allApprovals.values().stream()
+ .collect(
+ ImmutableListMultimap.toImmutableListMultimap(
+ PatchSetApproval::accountId, Function.identity()));
+ }
+
private List<ReviewerStatusUpdate> buildReviewerUpdates() {
List<ReviewerStatusUpdate> result = new ArrayList<>();
HashMap<Account.Id, ReviewerStateInternal> lastState = new HashMap<>();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
index 07a5a9c..ff8a2d0 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
@@ -34,7 +34,10 @@
import com.google.common.cache.Cache;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
+import com.google.common.collect.MoreCollectors;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.ExtensionRegistry;
+import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
@@ -44,12 +47,15 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.RawInputUtil;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelFunction;
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.client.ChangeKind;
@@ -58,13 +64,18 @@
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.server.change.ChangeKindCacheImpl;
import com.google.gerrit.server.project.testing.TestLabels;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.rules.SubmitRule;
import com.google.inject.Inject;
import com.google.inject.name.Named;
+import java.util.Arrays;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
+import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Before;
import org.junit.Test;
@@ -75,6 +86,7 @@
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ChangeOperations changeOperations;
@Inject private ChangeKindCreator changeKindCreator;
+ @Inject private ExtensionRegistry extensionRegistry;
@Inject
@Named("change_kind")
@@ -260,6 +272,113 @@
}
@Test
+ public void sticky_copiedToLatestPatchSetFromSubmitRecords() throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig().updateLabelType(LabelId.VERIFIED, b -> b.setFunction(LabelFunction.NO_BLOCK));
+ u.save();
+ }
+
+ // This test is covering the backfilling logic for changes which have been submitted, based on
+ // copied approvals, before Gerrit persisted copied votes as Copied-Label footers in NoteDb. It
+ // verifies that for such changes copied approvals are returned from the API even if the copied
+ // votes were not persisted as Copied-Label footers.
+ //
+ // In other words, this test verifies that given a change that was approved by a copied vote and
+ // then submitted and for which the copied approval is not persisted as a Copied-Label footer in
+ // NoteDb the copied approval is backfilled from the corresponding Submitted-With footer that
+ // got written to NoteDb on submit.
+ //
+ // Creating such a change would be possible by running the old Gerrit code from before Gerrit
+ // persisted copied labels as Copied-Label footers. However since this old Gerrit code is no
+ // longer available, the test needs to apply a trick to create a change in this state. It
+ // configures a fake submit rule, that pretends that an approval for a non-sticky label from an
+ // old patch set is still present on the current patch set and allows to submit the change.
+ // Since the label is non-sticky no Copied-Label footer is written for it. On submit the fake
+ // submit rule results in a Submitted-With footer that records the label as approved (although
+ // the label is actually not present on the current patch set). This is exactly the change state
+ // that we would have had by running the old code if submit was based on a copied label. As
+ // result of the backfilling logic we expect that this "copied" label (the label that is
+ // mentioned in the Submitted-With footer) is returned from the API.
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(new TestSubmitRule(user.id()))) {
+ // We want to add a vote on PS1, then not copy it to PS2, but include it in submit records
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Vote on patch-set 1
+ vote(admin, changeId, 2, 1);
+ vote(user, changeId, 1, -1);
+
+ // Upload patch-set 2. Change user's "Verified" vote on PS2.
+ changeOperations
+ .change(Change.id(r.getChange().getId().get()))
+ .newPatchset()
+ .file("new_file")
+ .content("content")
+ .commitMessage("Upload PS2")
+ .create();
+ vote(admin, changeId, 2, 1);
+ vote(user, changeId, 1, 1);
+
+ // Upload patch-set 3
+ changeOperations
+ .change(Change.id(r.getChange().getId().get()))
+ .newPatchset()
+ .file("another_file")
+ .content("content")
+ .commitMessage("Upload PS3")
+ .create();
+ vote(admin, changeId, 2, 1);
+
+ List<PatchSetApproval> patchSetApprovals =
+ notesFactory.create(project, r.getChange().getId()).getApprovalsWithCopied().values()
+ .stream()
+ .sorted(comparing(a -> a.patchSetId().get()))
+ .collect(toImmutableList());
+
+ // There's no verified approval on PS#3.
+ assertThat(
+ patchSetApprovals.stream()
+ .filter(
+ a ->
+ a.accountId().equals(user.id())
+ && a.label().equals(TestLabels.verified().getName())
+ && a.patchSetId().get() == 3)
+ .collect(Collectors.toList()))
+ .isEmpty();
+
+ // Submit the change. The TestSubmitRule will store a "submit record" containing a label
+ // voted by user, but the latest patch-set does not have an approval for this user, hence
+ // it will be copied if we request approvals after the change is merged.
+ requestScopeOperations.setApiUser(admin.id());
+ gApi.changes().id(changeId).current().submit();
+
+ patchSetApprovals =
+ notesFactory.create(project, r.getChange().getId()).getApprovalsWithCopied().values()
+ .stream()
+ .sorted(comparing(a -> a.patchSetId().get()))
+ .collect(toImmutableList());
+
+ // Get the copied approval for user on PS3 for the "Verified" label.
+ PatchSetApproval verifiedApproval =
+ patchSetApprovals.stream()
+ .filter(
+ a ->
+ a.accountId().equals(user.id())
+ && a.label().equals(TestLabels.verified().getName())
+ && a.patchSetId().get() == 3)
+ .collect(MoreCollectors.onlyElement());
+
+ assertCopied(
+ verifiedApproval,
+ /* psId= */ 3,
+ TestLabels.verified().getName(),
+ (short) 1,
+ /* copied= */ true);
+ }
+ }
+
+ @Test
public void stickyOnCopyValues() throws Exception {
TestAccount user2 = accountCreator.user2();
@@ -1381,4 +1500,29 @@
assertThat(approval.value()).isEqualTo(value);
assertThat(approval.copied()).isEqualTo(copied);
}
+
+ /**
+ * Test submit rule that always return a passing record with a "Verified" label applied by {@link
+ * TestSubmitRule#userAccountId}.
+ */
+ private static class TestSubmitRule implements SubmitRule {
+ Account.Id userAccountId;
+
+ TestSubmitRule(Account.Id userAccountId) {
+ this.userAccountId = userAccountId;
+ }
+
+ @Override
+ public Optional<SubmitRecord> evaluate(ChangeData changeData) {
+ SubmitRecord record = new SubmitRecord();
+ record.ruleName = "testSubmitRule";
+ record.status = SubmitRecord.Status.OK;
+ SubmitRecord.Label label = new SubmitRecord.Label();
+ label.label = "Verified";
+ label.status = SubmitRecord.Label.Status.OK;
+ label.appliedBy = userAccountId;
+ record.labels = Arrays.asList(label);
+ return Optional.of(record);
+ }
+ }
}
