Add PatchLineCommentsUtil to assist with notedb migration

Like ApprovalsUtil and ChangeMessagesUtil, we added an abstraction
layer to assist in the migration to notedb. So far, we have only
migrated published comments over to the notedb (drafts still are only
stored in the reviewdb). This utility class helps to read published
comments from either the notedb or the ReviewDb depending on the state
of the NotesMigration instance.

Additionally, in this change, I modified all callers of
PatchLineCommentAccess that query for only published comments to
instead use the corresponding methods in the PatchLineCommentsUtil.

Finally, the test class, CommentsTest, had to be modified to add the
ability to test reading published comments from the notedb, so I
created a separate Config in that class to test the notedb path.

Change-Id: I3a5637dda5d97df335c40d5cda22165ecf1d8232

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

@@ -0,0 +1,100 @@
+// Copyright (C) 2014 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+
+package com.google.gerrit.server;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchLineComment;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * Utility functions to manipulate PatchLineComments.
+ * <p>
+ * These methods either query for and update PatchLineComments in the NoteDb or
+ * ReviewDb, depending on the state of the NotesMigration.
+ */
+@Singleton
+public class PatchLineCommentsUtil {
+  private final NotesMigration migration;
+
+  @VisibleForTesting
+  @Inject
+  public PatchLineCommentsUtil(NotesMigration migration) {
+    this.migration = migration;
+  }
+
+  public List<PatchLineComment> publishedByChangeFile(ReviewDb db,
+      ChangeNotes notes, Change.Id changeId, String file) throws OrmException {
+    if (!migration.readPublishedComments()) {
+      return db.patchComments().publishedByChangeFile(changeId, file).toList();
+    }
+    notes.load();
+    List<PatchLineComment> commentsOnFile = new ArrayList<PatchLineComment>();
+
+    // We must iterate through all comments to find the ones on this file.
+    addCommentsInFile(commentsOnFile, notes.getBaseComments().values(), file);
+    addCommentsInFile(commentsOnFile, notes.getPatchSetComments().values(),
+        file);
+
+    Collections.sort(commentsOnFile, ChangeNotes.PatchLineCommentComparator);
+    return commentsOnFile;
+  }
+
+  public List<PatchLineComment> publishedByPatchSet(ReviewDb db,
+      ChangeNotes notes, PatchSet.Id psId) throws OrmException {
+    if (!migration.readPublishedComments()) {
+      return db.patchComments().publishedByPatchSet(psId).toList();
+    }
+    notes.load();
+    List<PatchLineComment> commentsOnPs = new ArrayList<PatchLineComment>();
+    commentsOnPs.addAll(notes.getPatchSetComments().get(psId));
+    commentsOnPs.addAll(notes.getBaseComments().get(psId));
+    return commentsOnPs;
+  }
+
+  private static Collection<PatchLineComment> addCommentsInFile(
+      Collection<PatchLineComment> commentsOnFile,
+      Collection<PatchLineComment> allComments,
+      String file) {
+    for (PatchLineComment c : allComments) {
+      String currentFilename = c.getKey().getParentKey().getFileName();
+      if (currentFilename.equals(file)) {
+        commentsOnFile.add(c);
+      }
+    }
+    return commentsOnFile;
+  }
+
+  public void addPublishedComments(ReviewDb db, ChangeUpdate update,
+      Iterable<PatchLineComment> comments) throws OrmException {
+    for (PatchLineComment c : comments) {
+      update.putComment(c);
+    }
+    db.patchComments().upsert(comments);
+  }
+}

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

@@ -21,6 +21,8 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.PatchLineCommentsUtil;
+import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -31,14 +33,16 @@ class Comments implements ChildCollection<RevisionResource, CommentResource> {
   private final DynamicMap<RestView<CommentResource>> views;
   private final ListComments list;
   private final Provider<ReviewDb> dbProvider;
+  private final PatchLineCommentsUtil plcUtil;
 
   @Inject
   Comments(DynamicMap<RestView<CommentResource>> views,
-      ListComments list,
-      Provider<ReviewDb> dbProvider) {
+      ListComments list, Provider<ReviewDb> dbProvider,
+      PatchLineCommentsUtil plcUtil) {
     this.views = views;
     this.list = list;
     this.dbProvider = dbProvider;
+    this.plcUtil = plcUtil;
   }
 
   @Override
@@ -55,8 +59,10 @@ class Comments implements ChildCollection<RevisionResource, CommentResource> {
   public CommentResource parse(RevisionResource rev, IdString id)
       throws ResourceNotFoundException, OrmException {
     String uuid = id.get();
-    for (PatchLineComment c : dbProvider.get().patchComments()
-        .publishedByPatchSet(rev.getPatchSet().getId())) {
+    ChangeNotes notes = rev.getNotes();
+
+    for (PatchLineComment c : plcUtil.publishedByPatchSet(dbProvider.get(),
+        notes, rev.getPatchSet().getId())) {
       if (uuid.equals(c.getKey().get())) {
         return new CommentResource(rev, c);
       }

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

@@ -16,7 +16,9 @@ package com.google.gerrit.server.change;
 
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.account.AccountInfo;
+import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -24,9 +26,13 @@ import com.google.inject.Singleton;
 
 @Singleton
 class ListComments extends ListDrafts {
+  private final PatchLineCommentsUtil plcUtil;
+
   @Inject
-  ListComments(Provider<ReviewDb> db, AccountInfo.Loader.Factory alf) {
+  ListComments(Provider<ReviewDb> db, AccountInfo.Loader.Factory alf,
+      PatchLineCommentsUtil plcUtil) {
     super(db, alf);
+    this.plcUtil = plcUtil;
   }
 
   @Override
@@ -36,7 +42,7 @@ class ListComments extends ListDrafts {
 
   protected Iterable<PatchLineComment> listComments(RevisionResource rsrc)
       throws OrmException {
-    return db.get().patchComments()
-        .publishedByPatchSet(rsrc.getPatchSet().getId());
+    ChangeNotes notes = rsrc.getNotes();
+    return plcUtil.publishedByPatchSet(db.get(), notes, rsrc.getPatchSet().getId());
   }
 }

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

@@ -44,14 +44,19 @@ import com.google.gerrit.reviewdb.client.CommentRange;
 import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.account.AccountsCollection;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.patch.PatchList;
+import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.util.LabelVote;
@@ -60,6 +65,7 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
+import org.eclipse.jgit.lib.ObjectId;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -84,6 +90,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
   private final ChangeUpdate.Factory updateFactory;
   private final ApprovalsUtil approvalsUtil;
   private final ChangeMessagesUtil cmUtil;
+  private final PatchLineCommentsUtil plcUtil;
+  private final PatchListCache patchListCache;
   private final ChangeIndexer indexer;
   private final AccountsCollection accounts;
   private final EmailReviewComments.Factory email;
@@ -103,6 +111,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       ChangeUpdate.Factory updateFactory,
       ApprovalsUtil approvalsUtil,
       ChangeMessagesUtil cmUtil,
+      PatchLineCommentsUtil plcUtil,
+      PatchListCache patchListCache,
       ChangeIndexer indexer,
       AccountsCollection accounts,
       EmailReviewComments.Factory email,
@@ -111,6 +121,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     this.changes = changes;
     this.changeDataFactory = changeDataFactory;
     this.updateFactory = updateFactory;
+    this.plcUtil = plcUtil;
+    this.patchListCache = patchListCache;
     this.approvalsUtil = approvalsUtil;
     this.cmUtil = cmUtil;
     this.indexer = indexer;
@@ -146,7 +158,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       timestamp = change.getLastUpdatedOn();
 
       update = updateFactory.create(revision.getControl(), timestamp);
-      dirty |= insertComments(revision, input.comments, input.drafts);
+      update.setPatchSetId(revision.getPatchSet().getId());
+      dirty |= insertComments(revision, update, input.comments, input.drafts);
       dirty |= updateLabels(revision, update, input.labels);
       dirty |= insertMessage(revision, input.message, update);
       if (dirty) {
@@ -319,8 +332,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
   }
 
   private boolean insertComments(RevisionResource rsrc,
-      Map<String, List<CommentInput>> in, DraftHandling draftsHandling)
-      throws OrmException {
+      ChangeUpdate update, Map<String, List<CommentInput>> in, DraftHandling draftsHandling)
+      throws OrmException, IOException {
     if (in == null) {
       in = Collections.emptyMap();
     }
@@ -333,6 +346,15 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     List<PatchLineComment> del = Lists.newArrayList();
     List<PatchLineComment> ups = Lists.newArrayList();
 
+    PatchList patchList = null;
+    try {
+      patchList = patchListCache.get(rsrc.getChange(), rsrc.getPatchSet());
+    } catch (PatchListNotAvailableException e) {
+      throw new OrmException("could not load PatchList for this patchset", e);
+    }
+    RevId patchSetCommit = new RevId(ObjectId.toString(patchList.getNewId()));
+    RevId baseCommit = new RevId(ObjectId.toString(patchList.getOldId()));;
+
     for (Map.Entry<String, List<CommentInput>> ent : in.entrySet()) {
       String path = ent.getKey();
       for (CommentInput c : ent.getValue()) {
@@ -352,6 +374,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         e.setStatus(PatchLineComment.Status.PUBLISHED);
         e.setWrittenOn(timestamp);
         e.setSide(c.side == Side.PARENT ? (short) 0 : (short) 1);
+        e.setRevId(c.side == Side.PARENT ? baseCommit : patchSetCommit);
         e.setMessage(c.message);
         if (c.range != null) {
           e.setRange(new CommentRange(
@@ -375,12 +398,13 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
         for (PatchLineComment e : drafts.values()) {
           e.setStatus(PatchLineComment.Status.PUBLISHED);
           e.setWrittenOn(timestamp);
+          e.setRevId(e.getSide() == (short) 0 ? baseCommit : patchSetCommit);
           ups.add(e);
         }
         break;
     }
     db.get().patchComments().delete(del);
-    db.get().patchComments().upsert(ups);
+    plcUtil.addPublishedComments(db.get(), update, ups);
     comments.addAll(ups);
     return !del.isEmpty() || !ups.isEmpty();
   }

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

@@ -36,12 +36,14 @@ public class NotesMigration {
     cfg.setBoolean("notedb", null, "write", true);
     cfg.setBoolean("notedb", "patchSetApprovals", "read", true);
     cfg.setBoolean("notedb", "changeMessages", "read", true);
+    cfg.setBoolean("notedb", "publishedComments", "read", true);
     return new NotesMigration(cfg);
   }
 
   private final boolean write;
   private final boolean readPatchSetApprovals;
   private final boolean readChangeMessages;
+  private final boolean readPublishedComments;
 
   @Inject
   NotesMigration(@GerritServerConfig Config cfg) {
@@ -50,6 +52,8 @@ public class NotesMigration {
         cfg.getBoolean("notedb", "patchSetApprovals", "read", false);
     readChangeMessages =
         cfg.getBoolean("notedb", "changeMessages", "read", false);
+    readPublishedComments =
+        cfg.getBoolean("notedb", "publishedComments", "read", false);
   }
 
   public boolean write() {
@@ -63,4 +67,8 @@ public class NotesMigration {
   public boolean readChangeMessages() {
     return readChangeMessages;
   }
+
+  public boolean readPublishedComments() {
+    return readPublishedComments;
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java

@@ -29,9 +29,11 @@ import com.google.gerrit.reviewdb.client.Patch.ChangeType;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.account.AccountInfoCacheFactory;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.LargeObjectException;
+import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
@@ -71,6 +73,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
   private final PatchListCache patchListCache;
   private final ReviewDb db;
   private final AccountInfoCacheFactory.Factory aicFactory;
+  private final PatchLineCommentsUtil plcUtil;
 
   private final String fileName;
   @Nullable
@@ -95,6 +98,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
       Provider<PatchScriptBuilder> builderFactory,
       final PatchListCache patchListCache, final ReviewDb db,
       final AccountInfoCacheFactory.Factory aicFactory,
+      PatchLineCommentsUtil plcUtil,
       @Assisted ChangeControl control,
       @Assisted final String fileName,
       @Assisted("patchSetA") @Nullable final PatchSet.Id patchSetA,
@@ -106,6 +110,7 @@ public class PatchScriptFactory implements Callable<PatchScript> {
     this.db = db;
     this.control = control;
     this.aicFactory = aicFactory;
+    this.plcUtil = plcUtil;
 
     this.fileName = fileName;
     this.psa = patchSetA;
@@ -316,7 +321,8 @@ public class PatchScriptFactory implements Callable<PatchScript> {
 
   private void loadPublished(final Map<Patch.Key, Patch> byKey,
       final AccountInfoCacheFactory aic, final String file) throws OrmException {
-    for (PatchLineComment c : db.patchComments().publishedByChangeFile(changeId, file)) {
+    ChangeNotes notes = control.getNotes();
+    for (PatchLineComment c : plcUtil.publishedByChangeFile(db, notes, changeId, file)) {
       if (comments.include(c)) {
         aic.want(c.getAuthor());
       }

gerrit-server/src/main/java/com/google/gerrit/server/util/TimeUtil.java

@@ -28,6 +28,11 @@ public class TimeUtil {
     return new Timestamp(nowMs());
   }
 
+  public static Timestamp roundTimestampToSecond(Timestamp t) {
+    long milliseconds = (t.getTime()/1000) * 1000;
+    return new Timestamp(milliseconds);
+  }
+
   private TimeUtil() {
   }
 }