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

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java

@@ -17,11 +17,14 @@ package com.google.gerrit.acceptance.server.notedb;
 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.Before;
 import org.junit.Test;
 
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.concurrent.TimeUnit;
 
 public class ChangeRebuilderIT extends AbstractDaemonTest {
@@ -94,6 +98,40 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     checker.rebuildAndCheckChanges(id);
   }
 
+  @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();
@@ -298,6 +336,35 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     }
   }
 
+  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) {

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java

@@ -144,6 +144,25 @@ public final class PatchLineComment {
     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;
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraftComment.java

@@ -119,7 +119,8 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
         // 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 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
         // 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),

gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java

@@ -186,6 +186,8 @@ public abstract class AbstractChangeUpdate {
     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 @@ public abstract class AbstractChangeUpdate {
    *     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;
   }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java

@@ -16,7 +16,6 @@ package com.google.gerrit.server.notedb;
 
 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.notes.NoteMap;
 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 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
 
   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 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
     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 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
     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 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
         + " 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 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
     }
 
     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 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
       }
     }
 
+    // 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 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
       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 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
       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 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
     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

gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java

@@ -60,18 +60,19 @@ class RevisionNote {
     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 @@ class RevisionNote {
         ? PatchLineComment.Status.DRAFT
         : PatchLineComment.Status.PUBLISHED;
     comments = ImmutableList.copyOf(
-        noteUtil.parseNote(bytes, p, changeId, status));
+        noteUtil.parseNote(raw, p, changeId, status));
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java

@@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkArgument;
 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 @@ class RevisionNoteBuilder {
     }
   }
 
+  final byte[] baseRaw;
   final List<PatchLineComment> baseComments;
   final Map<PatchLineComment.Key, PatchLineComment> put;
   final Set<PatchLineComment.Key> delete;
@@ -65,10 +65,12 @@ class RevisionNoteBuilder {
 
   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 @@ class RevisionNoteBuilder {
 
     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);
       }

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;
 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 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
   @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 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     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());
   }
 
@@ -1624,6 +1625,62 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(noteMap.contains(objId2)).isFalse();
   }
 
+  @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();
@@ -1784,8 +1841,7 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     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);