Parse draft notes from staged results if auto-rebuilding fails

It's possible for ChangeNotes to think the change is up to date with
respect to the NoteDbChangeState, but drafts for one or more users
might be out of date, so DraftCommentNotes may actually be responsible
for rebuilding the change. Do the same thing as in I2797d4fb but for
drafts.

I attempted to write a test for this in I2797d4fb, but missed the fact
that setting the NoteDbChangeState to an invalid value would cause
rebuilding to happen in the ChangeNotes rather than the
DraftCommentNotes, so we weren't exercising that code.

Fix the bug and add another test method. The first method was actually
testing something useful, that ChangeNotes was properly passing the
rebuildResult field to its child DraftCommentNotes. It just wasn't
testing DraftCommentNotes#rebuildAndOpen.

Change-Id: I1eadba8e9ccd18c39a18535d9d737c4178dc1626

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

@@ -22,6 +22,7 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
 import com.google.gerrit.acceptance.PushOneCommit;
@@ -431,7 +432,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
   }
 
   @Test
-  public void rebuildReturnsCorrectDraftResultEvenIfSavingToNoteDbFailed()
+  public void rebuildReturnsDraftResultWhenRebuildingInChangeNotesFails()
       throws Exception {
     setNotesMigration(true, true);
 
@@ -451,7 +452,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId())))
         .isEqualTo(oldMetaId);
 
-    // Force the next rebuild attempt to fail.
+    // Force the next rebuild attempt to fail (in ChangeNotes).
     rebuilderWrapper.failNextUpdate();
     setNotesMigration(true, true);
     ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id);
@@ -473,6 +474,62 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
         .isNotEqualTo(oldMetaId);
   }
 
+  @Test
+  public void rebuildReturnsDraftResultWhenRebuildingInDraftCommentNotesFails()
+      throws Exception {
+    setNotesMigration(true, true);
+
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getPatchSetId().getParentKey();
+    putDraft(user, id, 1, "comment by user");
+    assertChangeUpToDate(true, id);
+
+    ObjectId oldMetaId =
+        getMetaRef(allUsers, refsDraftComments(id, user.getId()));
+
+    // Add a draft behind NoteDb's back.
+    setNotesMigration(false, false);
+    putDraft(user, id, 1, "second comment by user");
+
+    ReviewDb db = unwrapDb();
+    Change c = db.changes().get(id);
+    // Leave change meta ID alone so DraftCommentNotes does the rebuild.
+    NoteDbChangeState bogusState = new NoteDbChangeState(
+        id, NoteDbChangeState.parse(c).getChangeMetaId(),
+        ImmutableMap.<Account.Id, ObjectId>of(
+            user.getId(),
+            ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")));
+    c.setNoteDbState(bogusState.toString());
+    db.changes().update(Collections.singleton(c));
+
+    assertDraftsUpToDate(false, id, user);
+    assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId())))
+        .isEqualTo(oldMetaId);
+
+    // Force the next rebuild attempt to fail (in DraftCommentNotes).
+    rebuilderWrapper.failNextUpdate();
+    setNotesMigration(true, true);
+    ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id);
+    notes.getDraftComments(user.getId());
+    assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId())))
+        .isEqualTo(oldMetaId);
+
+    // Not up to date, but the actual returned state matches anyway.
+    assertChangeUpToDate(true, id);
+    assertDraftsUpToDate(false, id, user);
+    ChangeBundle actual = ChangeBundle.fromNotes(plcUtil, notes);
+    ChangeBundle expected = ChangeBundle.fromReviewDb(unwrapDb(), id);
+    assertThat(actual.differencesFrom(expected)).isEmpty();
+
+    // Another rebuild attempt succeeds
+    notesFactory.create(dbProvider.get(), project, id)
+        .getDraftComments(user.getId());
+    assertChangeUpToDate(true, id);
+    assertDraftsUpToDate(true, id, user);
+    assertThat(getMetaRef(allUsers, refsDraftComments(id, user.getId())))
+        .isNotEqualTo(oldMetaId);
+  }
+
   @Test
   public void rebuildAutomaticallyWhenDraftsOutOfDate() throws Exception {
     setNotesMigration(true, true);

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

@@ -15,17 +15,20 @@
 package com.google.gerrit.server.notedb;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.Multimap;
+import com.google.gerrit.metrics.Timer1;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.git.RepoRefCache;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager.StagedResult;
 import com.google.gerrit.server.project.NoSuchChangeException;
@@ -181,22 +184,42 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
   }
 
   private LoadHandle rebuildAndOpen(Repository repo) throws IOException {
-    try {
-      NoteDbUpdateManager.Result r =
-          args.rebuilder.get().rebuild(args.db.get(), getChangeId());
-      if (r == null) {
+    try (Timer1.Context timer =
+        args.metrics.autoRebuildLatency.start(CHANGES)) {
+      Change.Id cid = getChangeId();
+      ReviewDb db = args.db.get();
+      ChangeRebuilder rebuilder = args.rebuilder.get();
+      NoteDbUpdateManager manager = rebuilder.stage(db, cid);
+      if (manager == null) {
         return super.openHandle(repo); // May be null in tests.
       }
-      ObjectId draftsId = r.newState().getDraftIds().get(author);
+      NoteDbUpdateManager.Result r = manager.stageAndApplyDelta(change);
+      try {
+        rebuilder.execute(db, cid, manager);
         repo.scanForRepoChanges();
-      return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), draftsId);
+      } catch (OrmException | IOException e) {
+        // See ChangeNotes#rebuildAndOpen.
+        args.metrics.autoRebuildFailureCount.increment(CHANGES);
+        checkNotNull(r.staged());
+        return LoadHandle.create(
+            ChangeNotesCommit.newStagedRevWalk(
+                repo, r.staged().allUsersObjects()),
+            draftsId(r));
+      }
+      return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), draftsId(r));
     } catch (NoSuchChangeException e) {
       return super.openHandle(repo);
-    } catch (OrmException | ConfigInvalidException e) {
+    } catch (OrmException e) {
       throw new IOException(e);
     }
   }
 
+  private ObjectId draftsId(NoteDbUpdateManager.Result r) {
+    checkNotNull(r);
+    checkNotNull(r.newState());
+    return r.newState().getDraftIds().get(author);
+  }
+
   @VisibleForTesting
   NoteMap getNoteMap() {
     return revisionNoteMap != null ? revisionNoteMap.noteMap : null;

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

@@ -209,7 +209,8 @@ public class NoteDbChangeState {
     return changeId;
   }
 
-  ObjectId getChangeMetaId() {
+  @VisibleForTesting
+  public ObjectId getChangeMetaId() {
     return changeMetaId;
   }