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.
