Merge "ChangeData: Filter out zombie draft refs"

gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java

@@ -461,6 +461,18 @@ public class CommentsUtil {
     }
   }
 
+  /**
+   * Get NoteDb draft refs for a change.
+   * <p>
+   * Works if NoteDb is not enabled, but the results are not meaningful.
+   * <p>
+   * This is just a simple ref scan, so the results may potentially include refs
+   * for zombie draft comments. A zombie draft is one which has been published
+   * but the write to delete the draft ref from All-Users failed.
+   *
+   * @param changeId change ID.
+   * @return raw refs from All-Users repo.
+   */
   public Collection<Ref> getDraftRefs(Change.Id changeId)
       throws OrmException {
     try (Repository repo = repoManager.openRepository(allUsers)) {

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

@@ -29,7 +29,6 @@ import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
-import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.common.collect.Multimaps;
 import com.google.common.collect.Ordering;
@@ -355,6 +354,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   // ChangeNotesCache from handlers.
   private ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
   private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
+  private ImmutableSet<Comment.Key> commentKeys;
 
   @VisibleForTesting
   public ChangeNotes(Args args, Change change) {
@@ -458,26 +458,32 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return state.publishedComments();
   }
 
+  public ImmutableSet<Comment.Key> getCommentKeys() {
+    if (commentKeys == null) {
+      ImmutableSet.Builder<Comment.Key> b = ImmutableSet.builder();
+      for (Comment c : getComments().values()) {
+        b.add(new Comment.Key(c.key));
+      }
+      commentKeys = b.build();
+    }
+    return commentKeys;
+  }
+
   public ImmutableListMultimap<RevId, Comment> getDraftComments(
       Account.Id author) throws OrmException {
-    loadDraftComments(author);
+    return getDraftComments(author, null);
-    final Multimap<RevId, Comment> published =
+  }
-        state.publishedComments();
+
-    // Filter out any draft comments that also exist in the published map, in
+  public ImmutableListMultimap<RevId, Comment> getDraftComments(
-    // case the update to All-Users to delete them during the publish operation
+      Account.Id author, @Nullable Ref ref) throws OrmException {
-    // failed.
+    loadDraftComments(author, ref);
-    Multimap<RevId, Comment> filtered = Multimaps.filterEntries(
+    // Filter out any zombie draft comments. These are drafts that are also in
-        draftCommentNotes.getComments(),
+    // the published map, and arise when the update to All-Users to delete them
-        (Map.Entry<RevId, Comment> e) -> {
+    // during the publish operation failed.
-            for (Comment c : published.get(e.getKey())) {
-              if (c.key.equals(e.getValue().key)) {
-                return false;
-              }
-            }
-            return true;
-        });
     return ImmutableListMultimap.copyOf(
-        filtered);
+        Multimaps.filterEntries(
+            draftCommentNotes.getComments(),
+            e -> !getCommentKeys().contains(e.getValue().key)));
   }
 
   public ImmutableListMultimap<RevId, RobotComment> getRobotComments()
@@ -492,12 +498,13 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
    * comments have been loaded or if the caller would like the drafts for
    * another author.
    */
-  private void loadDraftComments(Account.Id author)
+  private void loadDraftComments(Account.Id author, @Nullable Ref ref)
       throws OrmException {
-    if (draftCommentNotes == null ||
+    if (draftCommentNotes == null
-        !author.equals(draftCommentNotes.getAuthor())) {
+        || !author.equals(draftCommentNotes.getAuthor())
+        || ref != null) {
       draftCommentNotes = new DraftCommentNotes(
-          args, change, author, autoRebuild, rebuildResult);
+          args, change, author, autoRebuild, rebuildResult, ref);
       draftCommentNotes.load();
     }
   }
@@ -522,7 +529,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     if (containsCommentPublished(c)) {
       return true;
     }
-    loadDraftComments(c.author.getId());
+    loadDraftComments(c.author.getId(), null);
     return draftCommentNotes.containsComment(c);
   }
 

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

@@ -14,20 +14,22 @@
 
 package com.google.gerrit.server.notedb;
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
 import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.MultimapBuilder;
+import com.google.gerrit.common.Nullable;
 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.Comment;
 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;
@@ -42,6 +44,7 @@ import com.google.inject.assistedinject.AssistedInject;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.notes.NoteMap;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -69,6 +72,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
   private final Change change;
   private final Account.Id author;
   private final NoteDbUpdateManager.Result rebuildResult;
+  private final Ref ref;
 
   private ImmutableListMultimap<RevId, Comment> comments;
   private RevisionNoteMap<ChangeRevisionNote> revisionNoteMap;
@@ -78,7 +82,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
       Args args,
       @Assisted Change change,
       @Assisted Account.Id author) {
-    this(args, change, author, true, null);
+    this(args, change, author, true, null, null);
   }
 
   @AssistedInject
@@ -92,6 +96,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
     this.change = null;
     this.author = author;
     this.rebuildResult = null;
+    this.ref = null;
   }
 
   DraftCommentNotes(
@@ -99,11 +104,19 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
       Change change,
       Account.Id author,
       boolean autoRebuild,
-      NoteDbUpdateManager.Result rebuildResult) {
+      @Nullable NoteDbUpdateManager.Result rebuildResult,
+      @Nullable Ref ref) {
     super(args, change.getId(), PrimaryStorage.of(change), autoRebuild);
     this.change = change;
     this.author = author;
     this.rebuildResult = rebuildResult;
+    this.ref = ref;
+    if (ref != null) {
+      checkArgument(
+          ref.getName().equals(getRefName()),
+          "draft ref not for change %s and account %s: %s",
+          getChangeId(), author, ref.getName());
+    }
   }
 
   RevisionNoteMap<ChangeRevisionNote> getRevisionNoteMap() {
@@ -129,7 +142,15 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
 
   @Override
   protected String getRefName() {
-    return RefNames.refsDraftComments(getChangeId(), author);
+    return refsDraftComments(getChangeId(), author);
+  }
+
+  @Override
+  protected ObjectId readRef(Repository repo) throws IOException {
+    if (ref != null) {
+      return ref.getObjectId();
+    }
+    return super.readRef(repo);
   }
 
   @Override

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

@@ -83,7 +83,9 @@ public class NoteDbChangeState {
 
   @AutoValue
   public abstract static class Delta {
-    static Delta create(Change.Id changeId, Optional<ObjectId> newChangeMetaId,
+    @VisibleForTesting
+    public static Delta create(Change.Id changeId,
+        Optional<ObjectId> newChangeMetaId,
         Map<Account.Id, ObjectId> newDraftIds) {
       if (newDraftIds == null) {
         newDraftIds = ImmutableMap.of();

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -1147,7 +1147,14 @@ public class ChangeData {
       if (notesMigration.readChanges()) {
         for (Ref ref : commentsUtil.getDraftRefs(notes.getChangeId())) {
           Account.Id account = Account.Id.fromRefSuffix(ref.getName());
-          if (account != null) {
+          if (account != null
+              // Double-check that any drafts exist for this user after
+              // filtering out zombies. If some but not all drafts in the ref
+              // were zombies, the returned Ref still includes those zombies;
+              // this is suboptimal, but is ok for the purposes of
+              // draftsByUser(), and easier than trying to rebuild the change at
+              // this point.
+              && !notes().getDraftComments(account, ref).isEmpty()) {
             draftsByUser.put(account, ref);
           }
         }

gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java

@@ -39,6 +39,7 @@ import com.google.gerrit.extensions.api.changes.DraftInput;
 import com.google.gerrit.extensions.api.changes.HashtagsInput;
 import com.google.gerrit.extensions.api.changes.NotifyHandling;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
 import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
 import com.google.gerrit.extensions.api.changes.StarsInput;
 import com.google.gerrit.extensions.api.groups.GroupInput;
@@ -66,6 +67,7 @@ import com.google.gerrit.server.account.AuthRequest;
 import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.ChangeTriplet;
 import com.google.gerrit.server.change.PatchSetInserter;
+import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.edit.ChangeEditModifier;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.validators.CommitValidators;
@@ -78,6 +80,7 @@ import com.google.gerrit.server.index.change.IndexedChangeQuery;
 import com.google.gerrit.server.index.change.StalenessChecker;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.NoteDbChangeState;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.schema.SchemaCreator;
 import com.google.gerrit.server.util.RequestContext;
@@ -97,6 +100,7 @@ import com.google.inject.util.Providers;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.util.SystemReader;
@@ -114,6 +118,7 @@ import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 
 @Ignore
@@ -126,6 +131,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
   }
 
   @Inject protected AccountManager accountManager;
+  @Inject protected AllUsersName allUsersName;
   @Inject protected BatchUpdate.Factory updateFactory;
   @Inject protected ChangeInserter.Factory changeFactory;
   @Inject protected ChangeQueryBuilder queryBuilder;
@@ -1306,6 +1312,59 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     assertQuery("draftby:" + user2);
   }
 
+  @Test
+  public void byDraftByExcludesZombieDrafts() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+
+    Project.NameKey project = new Project.NameKey("repo");
+    TestRepository<Repo> repo = createProject(project.get());
+    Change change = insert(repo, newChange(repo));
+    Change.Id id = change.getId();
+
+    DraftInput in = new DraftInput();
+    in.line = 1;
+    in.message = "nit: trailing whitespace";
+    in.path = Patch.COMMIT_MSG;
+    gApi.changes().id(id.get()).current().createDraft(in);
+
+    assertQuery("draftby:" + userId, change);
+    assertQuery("commentby:" + userId);
+
+    TestRepository<Repo> allUsers =
+        new TestRepository<>(repoManager.openRepository(allUsersName));
+
+    Ref draftsRef = allUsers.getRepository().exactRef(
+        RefNames.refsDraftComments(id, userId));
+    assertThat(draftsRef).isNotNull();
+
+    ReviewInput rin = ReviewInput.dislike();
+    rin.drafts = DraftHandling.PUBLISH_ALL_REVISIONS;
+    gApi.changes().id(id.get()).current().review(rin);
+
+    assertQuery("draftby:" + userId);
+    assertQuery("commentby:" + userId, change);
+    assertThat(allUsers.getRepository().exactRef(draftsRef.getName())).isNull();
+
+    // Re-add drafts ref and ensure it gets filtered out during indexing.
+    allUsers.update(draftsRef.getName(), draftsRef.getObjectId());
+    assertThat(allUsers.getRepository().exactRef(draftsRef.getName()))
+        .isNotNull();
+
+    if (PrimaryStorage.of(change) == PrimaryStorage.REVIEW_DB) {
+      // Record draft ref in noteDbState as well.
+      ReviewDb db = ReviewDbUtil.unwrapDb(this.db);
+      change = db.changes().get(id);
+      NoteDbChangeState.applyDelta(change,
+          NoteDbChangeState.Delta.create(
+              id, Optional.empty(),
+              ImmutableMap.of(userId, draftsRef.getObjectId())));
+      db.changes().update(Collections.singleton(change));
+    }
+
+    indexer.index(db, project, id);
+    assertQuery("draftby:" + userId);
+  }
+
   @Test
   public void byStarredBy() throws Exception {
     TestRepository<Repo> repo = createProject("repo");