NoteDb: Avoid no-op commits when touching published comments
The implementation of ChangeUpdate eagerly creates a ChangeDraftUpdate
any time a published comment was added, for the purpose of deleting
the corresponding draft if present. NoteDbUpdateManager would happily
add and execute this ChangeDraftUpdate if it exists, even though it
was really a no-op.
This was not triggering ChangeDraftUpdate#isEmpty(), because there was
actually a key in the set of comments to delete. In fact isEmpty()
cannot actually know whether the update is empty, because it doesn't
have access to the original RevisionNotes to know whether the delete
set refers to any comments that actually exist.
Tweak the AbstractChangeUpdate interface to allow applyImpl to specify
that the update was really a no-op. Use a special sentinel return
value for this purpose; it's ugly, but restricted to the
tightly-coupled AbstractChangeUpdate hierarchy. Determine whether a
change is a no-op in ChangeDraftUpdate by checking the literal byte
array output in each of the notes.
Add regression tests to ChangeNotesTest for this case. Also add some
tests to ChangeRebuilderIT that touch this codepath, although
NoteDbChecker wouldn't actually flag the no-op commits.
Fix some bugs exposed in the ChangDraftUpdate pipeline by the new
codepaths and tests, mostly introduced by acda8b37. Use the correct
cached RevisionNoteMap object. Don't mutate comments from within
PutDraftComment, as those instances originally came from and would be
shared with the underlying ChangeNotes; make defensive copies
instead. Don't double-serialize the old version of an updated note
along with the updated version.
Change-Id: I50a42a4e8edc3390a02dc7e658ce6beef966e24e
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
index 1df074e..1fb6e11 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
@@ -17,11 +17,14 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.extensions.api.changes.DraftInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
@@ -48,6 +51,7 @@
import org.junit.Test;
import java.util.Collections;
+import java.util.HashMap;
import java.util.concurrent.TimeUnit;
public class ChangeRebuilderIT extends AbstractDaemonTest {
@@ -95,6 +99,40 @@
}
@Test
+ public void publishedComment() throws Exception {
+ PushOneCommit.Result r = createChange();
+ Change.Id id = r.getPatchSetId().getParentKey();
+ putComment(user, id, 1, "comment");
+ checker.rebuildAndCheckChanges(id);
+ }
+
+ @Test
+ public void draftComment() throws Exception {
+ PushOneCommit.Result r = createChange();
+ Change.Id id = r.getPatchSetId().getParentKey();
+ putDraft(user, id, 1, "comment");
+ checker.rebuildAndCheckChanges(id);
+ }
+
+ @Test
+ public void draftAndPublishedComment() throws Exception {
+ PushOneCommit.Result r = createChange();
+ Change.Id id = r.getPatchSetId().getParentKey();
+ putDraft(user, id, 1, "draft comment");
+ putComment(user, id, 1, "published comment");
+ checker.rebuildAndCheckChanges(id);
+ }
+
+ @Test
+ public void publishDraftComment() throws Exception {
+ PushOneCommit.Result r = createChange();
+ Change.Id id = r.getPatchSetId().getParentKey();
+ putDraft(user, id, 1, "draft comment");
+ publishDrafts(user, id);
+ checker.rebuildAndCheckChanges(id);
+ }
+
+ @Test
public void noWriteToNewRef() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getPatchSetId().getParentKey();
@@ -298,6 +336,35 @@
}
}
+ private void putComment(TestAccount account, Change.Id id, int line, String msg)
+ throws Exception {
+ CommentInput in = new CommentInput();
+ in.line = line;
+ in.message = msg;
+ ReviewInput rin = new ReviewInput();
+ rin.comments = new HashMap<>();
+ rin.comments.put(PushOneCommit.FILE_NAME, ImmutableList.of(in));
+ rin.drafts = ReviewInput.DraftHandling.KEEP;
+ AcceptanceTestRequestScope.Context old = setApiUser(account);
+ try {
+ gApi.changes().id(id.get()).current().review(rin);
+ } finally {
+ atrScope.set(old);
+ }
+ }
+
+ private void publishDrafts(TestAccount account, Change.Id id)
+ throws Exception {
+ ReviewInput rin = new ReviewInput();
+ rin.drafts = ReviewInput.DraftHandling.PUBLISH_ALL_REVISIONS;
+ AcceptanceTestRequestScope.Context old = setApiUser(account);
+ try {
+ gApi.changes().id(id.get()).current().review(rin);
+ } finally {
+ atrScope.set(old);
+ }
+ }
+
private ReviewDb unwrapDb() {
ReviewDb db = dbProvider.get();
if (db instanceof DisabledChangesReviewDbWrapper) {
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java
index c04b729..a4c5da5 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java
@@ -144,6 +144,25 @@
setWrittenOn(when);
}
+ public PatchLineComment(PatchLineComment o) {
+ key = o.key;
+ lineNbr = o.lineNbr;
+ author = o.author;
+ writtenOn = o.writtenOn;
+ status = o.status;
+ side = o.side;
+ message = o.message;
+ parentUuid = o.parentUuid;
+ revId = o.revId;
+ if (o.range != null) {
+ range = new CommentRange(
+ o.range.getStartLine(),
+ o.range.getStartCharacter(),
+ o.range.getEndLine(),
+ o.range.getEndCharacter());
+ }
+ }
+
public PatchLineComment.Key getKey() {
return key;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java
index 8a1f290..647bad7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java
@@ -119,7 +119,8 @@
// because the input might be missing required fields. Just give up.
throw new ResourceNotFoundException("comment not found: " + key);
}
- comment = maybeComment.get();
+ PatchLineComment origComment = maybeComment.get();
+ comment = new PatchLineComment(origComment);
PatchSet.Id psId = comment.getKey().getParentKey().getParentKey();
ChangeUpdate update = ctx.getUpdate(psId);
@@ -134,7 +135,7 @@
// Delete then recreate the comment instead of an update.
plcUtil.deleteComments(
- ctx.getDb(), update, Collections.singleton(comment));
+ ctx.getDb(), update, Collections.singleton(origComment));
comment = new PatchLineComment(
new PatchLineComment.Key(
new Patch.Key(psId, in.path),
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
index a55888d..b55b416 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
@@ -186,6 +186,8 @@
if (cb == null) {
result = z;
return z; // Impl intends to delete the ref.
+ } else if (cb == NO_OP_UPDATE) {
+ return null; // Impl is a no-op.
}
cb.setAuthor(authorIdent);
cb.setCommitter(new PersonIdent(serverIdent, when));
@@ -214,13 +216,17 @@
* the meta ref should be deleted as a result of this update. The parent,
* author, and committer fields in the return value are always
* overwritten. The tree ID may be unset by this method, which indicates
- * to the caller that it should be copied from the parent commit.
+ * to the caller that it should be copied from the parent commit. To
+ * indicate that this update is a no-op (but this could not be determined
+ * by {@link #isEmpty()}), return the sentinel {@link #NO_OP_UPDATE}.
* @throws OrmException if a Gerrit-level error occurred.
* @throws IOException if a lower-level error occurred.
*/
protected abstract CommitBuilder applyImpl(RevWalk rw, ObjectInserter ins,
ObjectId curr) throws OrmException, IOException;
+ protected static final CommitBuilder NO_OP_UPDATE = new CommitBuilder();
+
ObjectId getResult() {
return result;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
index 2644528..4d0de7e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
@@ -16,7 +16,6 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkNotNull;
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import com.google.auto.value.AutoValue;
@@ -42,9 +41,11 @@
import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Date;
-import java.util.HashMap;
import java.util.HashSet;
+import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -75,8 +76,7 @@
private final AllUsersName draftsProject;
- // TODO: can go back to a list?
- private Map<Key, PatchLineComment> put;
+ private List<PatchLineComment> put;
private Set<Key> delete;
@AssistedInject
@@ -93,7 +93,7 @@
super(migration, noteUtil, serverIdent, anonymousCowardName, notes,
accountId, authorIdent, when);
this.draftsProject = allUsers;
- this.put = new HashMap<>();
+ this.put = new ArrayList<>();
this.delete = new HashSet<>();
}
@@ -101,7 +101,7 @@
verifyComment(c);
checkArgument(c.getStatus() == PatchLineComment.Status.DRAFT,
"Cannot insert a published comment into a ChangeDraftUpdate");
- put.put(key(c), c);
+ put.add(c);
}
public void deleteComment(PatchLineComment c) {
@@ -119,15 +119,15 @@
+ " this ChangeDraftUpdate (%s): %s", accountId, comment);
}
- /** @return the tree id for the updated tree */
- private ObjectId storeCommentsInNotes(RevWalk rw, ObjectInserter ins,
- ObjectId curr) throws ConfigInvalidException, OrmException, IOException {
+ private CommitBuilder storeCommentsInNotes(RevWalk rw, ObjectInserter ins,
+ ObjectId curr, CommitBuilder cb)
+ throws ConfigInvalidException, OrmException, IOException {
RevisionNoteMap rnm = getRevisionNoteMap(rw, curr);
Set<RevId> updatedRevs =
Sets.newHashSetWithExpectedSize(rnm.revisionNotes.size());
RevisionNoteBuilder.Cache cache = new RevisionNoteBuilder.Cache(rnm);
- for (PatchLineComment c : put.values()) {
+ for (PatchLineComment c : put) {
if (!delete.contains(key(c))) {
cache.get(c.getRevId()).putComment(c);
}
@@ -137,11 +137,15 @@
}
Map<RevId, RevisionNoteBuilder> builders = cache.getBuilders();
+ boolean touchedAnyRevs = false;
boolean hasComments = false;
for (Map.Entry<RevId, RevisionNoteBuilder> e : builders.entrySet()) {
updatedRevs.add(e.getKey());
ObjectId id = ObjectId.fromString(e.getKey().get());
byte[] data = e.getValue().build(noteUtil);
+ if (!Arrays.equals(data, e.getValue().baseRaw)) {
+ touchedAnyRevs = true;
+ }
if (data.length == 0) {
rnm.noteMap.remove(id);
} else {
@@ -151,6 +155,13 @@
}
}
+ // If we didn't touch any notes, tell the caller this was a no-op update. We
+ // couldn't have done this in isEmpty() below because we hadn't read the old
+ // data yet.
+ if (!touchedAnyRevs) {
+ return NO_OP_UPDATE;
+ }
+
// If we touched every revision and there are no comments left, tell the
// caller to delete the entire ref.
boolean touchedAllRevs = updatedRevs.equals(rnm.revisionNotes.keySet());
@@ -158,7 +169,8 @@
return null;
}
- return rnm.noteMap.writeTree(ins);
+ cb.setTreeId(rnm.noteMap.writeTree(ins));
+ return cb;
}
private RevisionNoteMap getRevisionNoteMap(RevWalk rw, ObjectId curr)
@@ -172,8 +184,9 @@
if (draftNotes != null) {
ObjectId idFromNotes =
firstNonNull(draftNotes.getRevision(), ObjectId.zeroId());
- if (idFromNotes.equals(curr)) {
- return checkNotNull(getNotes().revisionNoteMap);
+ RevisionNoteMap rnm = draftNotes.getRevisionNoteMap();
+ if (idFromNotes.equals(curr) && rnm != null) {
+ return rnm;
}
}
}
@@ -195,15 +208,10 @@
CommitBuilder cb = new CommitBuilder();
cb.setMessage("Update draft comments");
try {
- ObjectId treeId = storeCommentsInNotes(rw, ins, curr);
- if (treeId == null) {
- return null; // Delete ref.
- }
- cb.setTreeId(checkNotNull(treeId));
+ return storeCommentsInNotes(rw, ins, curr, cb);
} catch (ConfigInvalidException e) {
throw new OrmException(e);
}
- return cb;
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java
index e6d7107..974538c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java
@@ -60,18 +60,19 @@
return new String(bytes, start, p.value);
}
+ final byte[] raw;
final ImmutableList<PatchLineComment> comments;
final String pushCert;
RevisionNote(ChangeNoteUtil noteUtil, Change.Id changeId,
ObjectReader reader, ObjectId noteId, boolean draftsOnly)
throws ConfigInvalidException, IOException {
- byte[] bytes = reader.open(noteId, OBJ_BLOB).getCachedBytes(MAX_NOTE_SZ);
+ raw = reader.open(noteId, OBJ_BLOB).getCachedBytes(MAX_NOTE_SZ);
MutableInteger p = new MutableInteger();
- trimLeadingEmptyLines(bytes, p);
+ trimLeadingEmptyLines(raw, p);
if (!draftsOnly) {
- pushCert = parsePushCert(changeId, bytes, p);
- trimLeadingEmptyLines(bytes, p);
+ pushCert = parsePushCert(changeId, raw, p);
+ trimLeadingEmptyLines(raw, p);
} else {
pushCert = null;
}
@@ -79,6 +80,6 @@
? PatchLineComment.Status.DRAFT
: PatchLineComment.Status.PUBLISHED;
comments = ImmutableList.copyOf(
- noteUtil.parseNote(bytes, p, changeId, status));
+ noteUtil.parseNote(raw, p, changeId, status));
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java
index 5179146..4345e8b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java
@@ -18,7 +18,6 @@
import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER;
import static java.nio.charset.StandardCharsets.UTF_8;
-import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.RevId;
@@ -57,6 +56,7 @@
}
}
+ final byte[] baseRaw;
final List<PatchLineComment> baseComments;
final Map<PatchLineComment.Key, PatchLineComment> put;
final Set<PatchLineComment.Key> delete;
@@ -65,10 +65,12 @@
RevisionNoteBuilder(RevisionNote base) {
if (base != null) {
+ baseRaw = base.raw;
baseComments = base.comments;
put = Maps.newHashMapWithExpectedSize(base.comments.size());
pushCert = base.pushCert;
} else {
+ baseRaw = new byte[0];
baseComments = Collections.emptyList();
put = new HashMap<>();
pushCert = null;
@@ -101,7 +103,12 @@
List<PatchLineComment> all =
new ArrayList<>(baseComments.size() + put.size());
- for (PatchLineComment c : Iterables.concat(baseComments, put.values())) {
+ for (PatchLineComment c : baseComments) {
+ if (!delete.contains(c.getKey()) && !put.containsKey(c.getKey())) {
+ all.add(c);
+ }
+ }
+ for (PatchLineComment c : put.values()) {
if (!delete.contains(c.getKey())) {
all.add(c);
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
index d2dd48b..3a9e08a 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -15,6 +15,8 @@
package com.google.gerrit.server.notedb;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
+import static com.google.gerrit.server.notedb.ChangeNoteUtil.changeRefName;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static com.google.gerrit.testutil.TestChanges.incrementPatchSet;
@@ -43,7 +45,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.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -413,7 +414,7 @@
@Test
public void emptyChangeUpdate() throws Exception {
Change c = newChange();
- Ref initial = repo.exactRef(ChangeNoteUtil.changeRefName(c.getId()));
+ Ref initial = repo.exactRef(changeRefName(c.getId()));
assertThat(initial).isNotNull();
// Empty update doesn't create a new commit.
@@ -421,7 +422,7 @@
update.commit();
assertThat(update.getResult()).isNull();
- Ref updated = repo.exactRef(ChangeNoteUtil.changeRefName(c.getId()));
+ Ref updated = repo.exactRef(changeRefName(c.getId()));
assertThat(updated.getObjectId()).isEqualTo(initial.getObjectId());
}
@@ -1625,6 +1626,62 @@
}
@Test
+ public void addingPublishedCommentDoesNotCreateNoOpCommitOnEmptyDraftRef()
+ throws Exception {
+ Change c = newChange();
+ String uuid = "uuid";
+ String rev = "abcd4567abcd4567abcd4567abcd4567abcd4567";
+ CommentRange range = new CommentRange(1, 1, 2, 1);
+ PatchSet.Id ps1 = c.currentPatchSetId();
+ String filename = "filename1";
+ short side = (short) 1;
+
+ ChangeUpdate update = newUpdate(c, otherUser);
+ Timestamp now = TimeUtil.nowTs();
+ PatchLineComment comment = newComment(ps1, filename, uuid, range,
+ range.getEndLine(), otherUser, null, now, "comment on ps1", side,
+ rev, Status.PUBLISHED);
+ update.putComment(comment);
+ update.commit();
+
+ assertThat(repo.exactRef(changeRefName(c.getId()))).isNotNull();
+ String draftRef = refsDraftComments(otherUser.getAccountId(), c.getId());
+ assertThat(exactRefAllUsers(draftRef)).isNull();
+ }
+
+ @Test
+ public void addingPublishedCommentDoesNotCreateNoOpCommitOnNonEmptyDraftRef()
+ throws Exception {
+ Change c = newChange();
+ String rev = "abcd4567abcd4567abcd4567abcd4567abcd4567";
+ CommentRange range = new CommentRange(1, 1, 2, 1);
+ PatchSet.Id ps1 = c.currentPatchSetId();
+ String filename = "filename1";
+ short side = (short) 1;
+
+ ChangeUpdate update = newUpdate(c, otherUser);
+ Timestamp now = TimeUtil.nowTs();
+ PatchLineComment draft = newComment(ps1, filename, "uuid1", range,
+ range.getEndLine(), otherUser, null, now, "draft comment on ps1", side,
+ rev, Status.DRAFT);
+ update.putComment(draft);
+ update.commit();
+
+ String draftRef = refsDraftComments(otherUser.getAccountId(), c.getId());
+ ObjectId old = exactRefAllUsers(draftRef);
+ assertThat(old).isNotNull();
+
+ update = newUpdate(c, otherUser);
+ PatchLineComment pub = newComment(ps1, filename, "uuid2", range,
+ range.getEndLine(), otherUser, null, now, "comment on ps1", side,
+ rev, Status.PUBLISHED);
+ update.putComment(pub);
+ update.commit();
+
+ assertThat(exactRefAllUsers(draftRef)).isEqualTo(old);
+ }
+
+ @Test
public void fileComment() throws Exception {
Change c = newChange();
ChangeUpdate update = newUpdate(c, otherUser);
@@ -1784,8 +1841,7 @@
update.putComment(comment2);
update.commit();
-
- String refName = RefNames.refsDraftComments(otherUserId, c.getId());
+ String refName = refsDraftComments(otherUserId, c.getId());
ObjectId oldDraftId = exactRefAllUsers(refName);
update = newUpdate(c, otherUser);
