ChangeUpdate: fix updating change meta ref when the update is no/op There was a bug in RemoveAttentionSetOp in the sense that it always returned true signalling to callers to perform an update regardless of whether the attention set will get updated or not. At the time we build the change meta ref commit, we append all updates to the commit message. The method ChangeUpdate#isEmpty checks if there are no updates and bails out at [1]. Sometimes the variable plannedAttentionSetUpdates contained some updates, but this variable is processed in [2] while writing the commit message and in some cases no updates are written after all. This triggered the creation of a new commit in the change meta ref which is redundant, hence fixing in this change and adapting the test in PostReviewIT to ensure the change meta ref is not updated in this case. We fix this by factoring out plannedAttentionSetUpdates from #isEmpty and explicitly checking that [2] did not include any attention set updates. We do that because #isEmpty is also called from [3] in AbstractChangeUpdate#apply (at the beginning of the processing) and it bails out quickly if plannedAttentionSetUpdates is null. [1] https://gerrit.googlesource.com/gerrit/+/23678de2cdc56b676fcd80ea5d76b10af8c77304/java/com/google/gerrit/server/notedb/ChangeUpdate.java#1085 [2] https://gerrit.googlesource.com/gerrit/+/23678de2cdc56b676fcd80ea5d76b10af8c77304/java/com/google/gerrit/server/notedb/ChangeUpdate.java#984 [3] https://gerrit.googlesource.com/gerrit/+/23678de2cdc56b676fcd80ea5d76b10af8c77304/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java#220 Release-Notes: skip Google-Bug-Id: b/221124220 Change-Id: I4086c614ff670e3bc43f772c3d004c65477db4eb (cherry picked from commit ecf033e5ef52eab3b0a8d4d6aef9d2799a467020)
diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 45fd83d..8f352cb 100644 --- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -820,7 +820,10 @@ } } - updateAttentionSet(msg); + boolean hasAttentionSeUpdates = updateAttentionSet(msg); + if (isEmptyWithoutAttentionSet() && !hasAttentionSeUpdates) { + return NO_OP_UPDATE; + } CommitBuilder cb = new CommitBuilder(); cb.setMessage(msg.toString()); @@ -954,8 +957,11 @@ * <p>Changing the behaviour of this method might affect the way a ChangeUpdate is considered to * be an "Attention Set Change Only". Make sure the {@link #isAttentionSetChangeOnly} logic is * amended as well if needed. + * + * @return True if one or more attention set updates are appended to the {@code msg}, and false + * otherwise. */ - private void updateAttentionSet(StringBuilder msg) { + private boolean updateAttentionSet(StringBuilder msg) { if (plannedAttentionSetUpdates == null) { plannedAttentionSetUpdates = new HashMap<>(); } @@ -981,6 +987,8 @@ removeInactiveUsersFromAttentionSet(currentReviewers); + boolean hasUpdates = false; + for (AttentionSetUpdate attentionSetUpdate : plannedAttentionSetUpdates.values()) { if (attentionSetUpdate.operation() == AttentionSetUpdate.Operation.ADD && currentUsersInAttentionSet.contains(attentionSetUpdate.account())) { @@ -1015,7 +1023,9 @@ } addFooter(msg, FOOTER_ATTENTION, noteUtil.attentionSetUpdateToJson(attentionSetUpdate)); + hasUpdates = true; } + return hasUpdates; } private void removeInactiveUsersFromAttentionSet(Set<Account.Id> currentReviewers) { @@ -1083,6 +1093,10 @@ @Override public boolean isEmpty() { + return isEmptyWithoutAttentionSet() && plannedAttentionSetUpdates == null; + } + + private boolean isEmptyWithoutAttentionSet() { return commitSubject == null && approvals.isEmpty() && copiedApprovals.isEmpty() @@ -1095,7 +1109,6 @@ && status == null && submissionId == null && submitRecords == null - && plannedAttentionSetUpdates == null && assignee == null && hashtags == null && topic == null
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java index f8991b4..dcd8f77f 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -39,6 +39,7 @@ import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Account; +import com.google.gerrit.entities.AttentionSetUpdate; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.LabelId; import com.google.gerrit.entities.PatchSet; @@ -83,6 +84,8 @@ import java.util.Map; import java.util.Optional; import java.util.regex.Pattern; +import java.util.stream.Collectors; +import org.eclipse.jgit.lib.ObjectId; import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; @@ -633,8 +636,15 @@ assertThat(r.getChange().approvals().values()).hasSize(1); // Post without changing the vote. + ChangeNotes notes = notesFactory.create(project, r.getChange().getId()); + ObjectId metaId = notes.getMetaId(); + assertAttentionSet(notes.getAttentionSet(), user.id()); input = new ReviewInput().label(LabelId.CODE_REVIEW, 2); gApi.changes().id(r.getChangeId()).current().review(input); + notes = notesFactory.create(project, r.getChange().getId()); + // Change meta ID did not change since the update is No/Op. Attention set is same. + assertThat(notes.getMetaId()).isEqualTo(metaId); + assertAttentionSet(notes.getAttentionSet(), user.id()); // Second vote replaced the original vote, so still only one vote. assertThat(r.getChange().approvals().values()).hasSize(1); @@ -1086,4 +1096,10 @@ .collect(toImmutableSet()); } } + + private static void assertAttentionSet( + ImmutableSet<AttentionSetUpdate> attentionSet, Account.Id... accounts) { + assertThat(attentionSet.stream().map(AttentionSetUpdate::account).collect(Collectors.toList())) + .containsExactly(accounts); + } }
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index ab01bf3..55c7921 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1996,12 +1996,13 @@ TestTimeUtil.setClock(new Timestamp(startMs + 2 * thirtyHoursInMs)); submit(change2); TestTimeUtil.setClock(new Timestamp(startMs + 3 * thirtyHoursInMs)); - // Put another approval on the change, just to update it. + // Put another approval on the change, just to update it. This does not record an update in + // NoteDb since this is a no/op. approve(change1); approve(change3); assertThat(TimeUtil.nowMs()).isEqualTo(startMs + 3 * thirtyHoursInMs); - assertThat(lastUpdatedMsApi(change3)).isEqualTo(startMs + 3 * thirtyHoursInMs); + assertThat(lastUpdatedMsApi(change3)).isEqualTo(startMs + thirtyHoursInMs); assertThat(lastUpdatedMsApi(change2)).isEqualTo(startMs + 2 * thirtyHoursInMs); assertThat(lastUpdatedMsApi(change1)).isEqualTo(startMs + 3 * thirtyHoursInMs); @@ -2020,7 +2021,7 @@ assertQuery("mergedbefore:2009-10-03", change3); // Changes are sorted by lastUpdatedOn first, then by mergedOn. // Even though Change2 was merged after Change3, Change3 is returned first. - assertQuery("mergedbefore:2009-10-04", change3, change2); + assertQuery("mergedbefore:2009-10-04", change2, change3); // Same test as above, but using filter code path. assertQuery(makeIndexedPredicateFilterQuery("mergedbefore:2009-10-01")); @@ -2033,7 +2034,7 @@ makeIndexedPredicateFilterQuery("mergedbefore:\"2009-10-02 03:02:00 -0000\""), change3); assertQuery(makeIndexedPredicateFilterQuery("mergedbefore:\"2009-10-02 03:02:00\""), change3); assertQuery(makeIndexedPredicateFilterQuery("mergedbefore:2009-10-03"), change3); - assertQuery(makeIndexedPredicateFilterQuery("mergedbefore:2009-10-04"), change3, change2); + assertQuery(makeIndexedPredicateFilterQuery("mergedbefore:2009-10-04"), change2, change3); } @Test @@ -2058,13 +2059,14 @@ submit(change2); TestTimeUtil.setClock(new Timestamp(startMs + 3 * thirtyHoursInMs)); - // Put another approval on the change, just to update it. + // Put another approval on the change, just to update it. This does not record an update + // in NoteDb since this is a no/op. approve(change1); approve(change3); assertThat(TimeUtil.nowMs()).isEqualTo(startMs + 3 * thirtyHoursInMs); - assertThat(lastUpdatedMsApi(change3)).isEqualTo(startMs + 3 * thirtyHoursInMs); + assertThat(lastUpdatedMsApi(change3)).isEqualTo(startMs + thirtyHoursInMs); assertThat(lastUpdatedMsApi(change2)).isEqualTo(startMs + 2 * thirtyHoursInMs); assertThat(lastUpdatedMsApi(change1)).isEqualTo(startMs + 3 * thirtyHoursInMs); @@ -2072,36 +2074,36 @@ // 1. Change1 was not submitted and should be never returned. // 2. Change2 was merged on 2009-10-02 03:00:00 -0000 // 3. Change3 was merged on 2009-10-03 09:00:00.0 -0000 - assertQuery("mergedafter:2009-10-01", change3, change2); + assertQuery("mergedafter:2009-10-01", change2, change3); // Changes are sorted by lastUpdatedOn first, then by mergedOn. - // Even though Change2 was merged after Change3, Change3 is returned first. - assertQuery("mergedafter:\"2009-10-01 22:59:00 -0400\"", change3, change2); - assertQuery("mergedafter:\"2009-10-02 02:59:00 -0000\"", change3, change2); + // Change 2 (which was updated last) is returned before change 3. + assertQuery("mergedafter:\"2009-10-01 22:59:00 -0400\"", change2, change3); + assertQuery("mergedafter:\"2009-10-02 02:59:00 -0000\"", change2, change3); assertQuery("mergedafter:\"2009-10-01 23:02:00 -0400\"", change2); assertQuery("mergedafter:\"2009-10-02 03:02:00 -0000\"", change2); // Changes included on the date submitted. - assertQuery("mergedafter:2009-10-02", change3, change2); + assertQuery("mergedafter:2009-10-02", change2, change3); assertQuery("mergedafter:2009-10-03", change2); // Same test as above, but using filter code path. - assertQuery(makeIndexedPredicateFilterQuery("mergedafter:2009-10-01"), change3, change2); + assertQuery(makeIndexedPredicateFilterQuery("mergedafter:2009-10-01"), change2, change3); // Changes are sorted by lastUpdatedOn first, then by mergedOn. // Even though Change2 was merged after Change3, Change3 is returned first. assertQuery( makeIndexedPredicateFilterQuery("mergedafter:\"2009-10-01 22:59:00 -0400\""), - change3, - change2); + change2, + change3); assertQuery( makeIndexedPredicateFilterQuery("mergedafter:\"2009-10-02 02:59:00 -0000\""), - change3, - change2); + change2, + change3); assertQuery( makeIndexedPredicateFilterQuery("mergedafter:\"2009-10-01 23:02:00 -0400\""), change2); assertQuery( makeIndexedPredicateFilterQuery("mergedafter:\"2009-10-02 03:02:00 -0000\""), change2); // Changes included on the date submitted. - assertQuery(makeIndexedPredicateFilterQuery("mergedafter:2009-10-02"), change3, change2); + assertQuery(makeIndexedPredicateFilterQuery("mergedafter:2009-10-02"), change2, change3); assertQuery(makeIndexedPredicateFilterQuery("mergedafter:2009-10-03"), change2); } @@ -2124,14 +2126,15 @@ submit(change2); submit(change3); TestTimeUtil.setClock(new Timestamp(startMs + 2 * thirtyHoursInMs)); - // Approve post submit just to update lastUpdatedOn + // Approve post submit just to update lastUpdatedOn. This does not record an update in NoteDb + // since this is a No/op. approve(change3); approve(change2); submit(change1); // All Changes were last updated at the same time. - assertThat(lastUpdatedMsApi(change3)).isEqualTo(startMs + 2 * thirtyHoursInMs); - assertThat(lastUpdatedMsApi(change2)).isEqualTo(startMs + 2 * thirtyHoursInMs); + assertThat(lastUpdatedMsApi(change3)).isEqualTo(startMs + thirtyHoursInMs); + assertThat(lastUpdatedMsApi(change2)).isEqualTo(startMs + thirtyHoursInMs); assertThat(lastUpdatedMsApi(change1)).isEqualTo(startMs + 2 * thirtyHoursInMs); // Changes are sorted by lastUpdatedOn first, then by mergedOn, then by Id in reverse order.