From d710c0cfd8bc18038fc7e8b1ec73f3736d0de7bf Mon Sep 17 00:00:00 2001 From: Yacob Yonas Date: Wed, 2 Jul 2014 16:31:27 -0700 Subject: [PATCH] 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 --- .../changedetail/PatchSetDetailFactory.java | 8 +- .../reviewdb/client/PatchLineComment.java | 15 ++ .../gerrit/server/PatchLineCommentsUtil.java | 100 ++++++++++ .../google/gerrit/server/change/Comments.java | 14 +- .../gerrit/server/change/ListComments.java | 12 +- .../gerrit/server/change/PostReview.java | 32 +++- .../gerrit/server/notedb/NotesMigration.java | 8 + .../server/patch/PatchScriptFactory.java | 8 +- .../google/gerrit/server/util/TimeUtil.java | 5 + .../gerrit/server/change/CommentsTest.java | 175 +++++++++++++++--- 10 files changed, 341 insertions(+), 36 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java index c4a8bb62bd..73cc83a92e 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java @@ -34,10 +34,12 @@ import com.google.gerrit.reviewdb.client.Project; 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.change.ChangesCollection; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.change.Revisions; import com.google.gerrit.server.extensions.webui.UiActions; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.patch.PatchListKey; @@ -78,6 +80,7 @@ class PatchSetDetailFactory extends Handler { private final ChangeControl.Factory changeControlFactory; private final ChangesCollection changes; private final Revisions revisions; + private final PatchLineCommentsUtil plcUtil; private Project.NameKey projectKey; private final PatchSet.Id psIdBase; @@ -96,6 +99,7 @@ class PatchSetDetailFactory extends Handler { final ChangeControl.Factory changeControlFactory, final ChangesCollection changes, final Revisions revisions, + final PatchLineCommentsUtil plcUtil, @Assisted("psIdBase") @Nullable final PatchSet.Id psIdBase, @Assisted("psIdNew") final PatchSet.Id psIdNew, @Assisted @Nullable final AccountDiffPreference diffPrefs) { @@ -105,6 +109,7 @@ class PatchSetDetailFactory extends Handler { this.changeControlFactory = changeControlFactory; this.changes = changes; this.revisions = revisions; + this.plcUtil = plcUtil; this.psIdBase = psIdBase; this.psIdNew = psIdNew; @@ -143,7 +148,8 @@ class PatchSetDetailFactory extends Handler { byKey.put(p.getKey(), p); } - for (final PatchLineComment c : db.patchComments().publishedByPatchSet(psIdNew)) { + ChangeNotes notes = control.getNotes(); + for (PatchLineComment c : plcUtil.publishedByPatchSet(db, notes, psIdNew)) { final Patch p = byKey.get(c.getKey().getParentKey()); if (p != null) { p.setCommentCount(p.getCommentCount() + 1); 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 a739abefb1..08c3f52d00 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 @@ -54,6 +54,21 @@ public final class PatchLineComment { protected void set(String newValue) { uuid = newValue; } + + @Override + public String toString() { + StringBuilder builder = new StringBuilder(); + builder.append("PatchLineComment.Key{"); + builder.append("Change.Id=") + .append(getParentKey().getParentKey().getParentKey().get()).append(','); + builder.append("PatchSet.Id=") + .append(getParentKey().getParentKey().get()).append(','); + builder.append("filename=") + .append(getParentKey().getFileName()).append(','); + builder.append("uuid=").append(get()); + builder.append("}"); + return builder.toString(); + } } public static final char STATUS_DRAFT = 'd'; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java new file mode 100644 index 0000000000..49185465c2 --- /dev/null +++ b/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. + *

+ * 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 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 commentsOnFile = new ArrayList(); + + // 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 publishedByPatchSet(ReviewDb db, + ChangeNotes notes, PatchSet.Id psId) throws OrmException { + if (!migration.readPublishedComments()) { + return db.patchComments().publishedByPatchSet(psId).toList(); + } + notes.load(); + List commentsOnPs = new ArrayList(); + commentsOnPs.addAll(notes.getPatchSetComments().get(psId)); + commentsOnPs.addAll(notes.getBaseComments().get(psId)); + return commentsOnPs; + } + + private static Collection addCommentsInFile( + Collection commentsOnFile, + Collection 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 comments) throws OrmException { + for (PatchLineComment c : comments) { + update.putComment(c); + } + db.patchComments().upsert(comments); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Comments.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Comments.java index 30a373d3ab..c987ce8892 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Comments.java +++ b/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 { private final DynamicMap> views; private final ListComments list; private final Provider dbProvider; + private final PatchLineCommentsUtil plcUtil; @Inject Comments(DynamicMap> views, - ListComments list, - Provider dbProvider) { + ListComments list, Provider dbProvider, + PatchLineCommentsUtil plcUtil) { this.views = views; this.list = list; this.dbProvider = dbProvider; + this.plcUtil = plcUtil; } @Override @@ -55,8 +59,10 @@ class Comments implements ChildCollection { 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); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListComments.java index f276c05a09..f4d7b496e7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListComments.java +++ b/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 db, AccountInfo.Loader.Factory alf) { + ListComments(Provider db, AccountInfo.Loader.Factory alf, + PatchLineCommentsUtil plcUtil) { super(db, alf); + this.plcUtil = plcUtil; } @Override @@ -36,7 +42,7 @@ class ListComments extends ListDrafts { protected Iterable 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()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 288172e627..276404839d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/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 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 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 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 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 } private boolean insertComments(RevisionResource rsrc, - Map> in, DraftHandling draftsHandling) - throws OrmException { + ChangeUpdate update, Map> in, DraftHandling draftsHandling) + throws OrmException, IOException { if (in == null) { in = Collections.emptyMap(); } @@ -333,6 +346,15 @@ public class PostReview implements RestModifyView List del = Lists.newArrayList(); List 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> ent : in.entrySet()) { String path = ent.getKey(); for (CommentInput c : ent.getValue()) { @@ -352,6 +374,7 @@ public class PostReview implements RestModifyView 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 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(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java index db72def0c1..36f685d70f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java +++ b/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; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java index 4be9e7d151..b4337cf45c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchScriptFactory.java +++ b/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 { 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 { Provider 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 { 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 { private void loadPublished(final Map 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()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/TimeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/TimeUtil.java index 4c4b96e3d6..6bc261fe76 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/util/TimeUtil.java +++ b/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() { } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java index 95f637a80d..a30fa92d35 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.change; +import static com.google.inject.Scopes.SINGLETON; import static org.easymock.EasyMock.createMock; import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.expectLastCall; @@ -36,37 +37,86 @@ import com.google.gerrit.reviewdb.client.Change; 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.Project; import com.google.gerrit.reviewdb.client.PatchLineComment.Status; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.PatchLineCommentAccess; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.PatchLineCommentsUtil; +import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountInfo; +import com.google.gerrit.server.account.CapabilityControl; +import com.google.gerrit.server.account.GroupBackend; +import com.google.gerrit.server.config.AnonymousCowardName; +import com.google.gerrit.server.config.AnonymousCowardNameProvider; +import com.google.gerrit.server.config.CanonicalWebUrl; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.GitModule; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.group.SystemGroupBackend; +import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.util.TimeUtil; +import com.google.gerrit.testutil.TestChanges; +import com.google.gerrit.testutil.ConfigSuite; +import com.google.gerrit.testutil.FakeAccountCache; +import com.google.gerrit.testutil.InMemoryRepositoryManager; import com.google.gwtorm.server.ListResultSet; +import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; import com.google.inject.AbstractModule; import com.google.inject.Guice; import com.google.inject.Injector; import com.google.inject.TypeLiteral; +import com.google.inject.util.Providers; import org.easymock.IAnswer; +import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; +import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.PersonIdent; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.TimeZone; -public class CommentsTest { +@RunWith(ConfigSuite.class) +public class CommentsTest { + private static final TimeZone TZ = + TimeZone.getTimeZone("America/Los_Angeles"); + + @ConfigSuite.Parameter + public Config config; + + @ConfigSuite.Config + public static @GerritServerConfig Config noteDbEnabled() { + @GerritServerConfig Config cfg = new Config(); + cfg.setBoolean("notedb", null, "write", true); + cfg.setBoolean("notedb", "publishedComments", "read", true); + return cfg; + } private Injector injector; + private Project.NameKey project; + private InMemoryRepositoryManager repoManager; + private PatchLineCommentsUtil plcUtil; private RevisionResource revRes1; private RevisionResource revRes2; private PatchLineComment plc1; private PatchLineComment plc2; private PatchLineComment plc3; + private IdentifiedUser changeOwner; @Before public void setUp() throws Exception { @@ -78,59 +128,137 @@ public class CommentsTest { final AccountInfo.Loader.Factory alf = createMock(AccountInfo.Loader.Factory.class); final ReviewDb db = createMock(ReviewDb.class); + final FakeAccountCache accountCache = new FakeAccountCache(); + final PersonIdent serverIdent = new PersonIdent( + "Gerrit Server", "noreply@gerrit.com", TimeUtil.nowTs(), TZ); + project = new Project.NameKey("test-project"); + repoManager = new InMemoryRepositoryManager(); + + @SuppressWarnings("unused") + InMemoryRepository repo = repoManager.createRepository(project); AbstractModule mod = new AbstractModule() { @Override protected void configure() { bind(viewsType).toInstance(views); bind(AccountInfo.Loader.Factory.class).toInstance(alf); - bind(ReviewDb.class).toInstance(db); - }}; + bind(ReviewDb.class).toProvider(Providers.of(db)); + bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(config); + bind(ProjectCache.class).toProvider(Providers. of(null)); + install(new GitModule()); + bind(GitRepositoryManager.class).toInstance(repoManager); + bind(CapabilityControl.Factory.class) + .toProvider(Providers. of(null)); + bind(String.class).annotatedWith(AnonymousCowardName.class) + .toProvider(AnonymousCowardNameProvider.class); + bind(String.class).annotatedWith(CanonicalWebUrl.class) + .toInstance("http://localhost:8080/"); + bind(GroupBackend.class).to(SystemGroupBackend.class).in(SINGLETON); + bind(AccountCache.class).toInstance(accountCache); + bind(GitReferenceUpdated.class) + .toInstance(GitReferenceUpdated.DISABLED); + bind(PersonIdent.class).annotatedWith(GerritPersonIdent.class) + .toInstance(serverIdent); + } + }; + + injector = Guice.createInjector(mod); + + NotesMigration migration = injector.getInstance(NotesMigration.class); + plcUtil = new PatchLineCommentsUtil(migration); + + Account co = new Account(new Account.Id(1), TimeUtil.nowTs()); + co.setFullName("Change Owner"); + co.setPreferredEmail("change@owner.com"); + accountCache.put(co); + Account.Id ownerId = co.getId(); + + Account ou = new Account(new Account.Id(2), TimeUtil.nowTs()); + ou.setFullName("Other Account"); + ou.setPreferredEmail("other@account.com"); + accountCache.put(ou); + Account.Id otherUserId = ou.getId(); + + IdentifiedUser.GenericFactory userFactory = + injector.getInstance(IdentifiedUser.GenericFactory.class); + changeOwner = userFactory.create(ownerId); + IdentifiedUser otherUser = userFactory.create(otherUserId); - Account.Id account1 = new Account.Id(1); - Account.Id account2 = new Account.Id(2); AccountInfo.Loader accountLoader = createMock(AccountInfo.Loader.class); accountLoader.fill(); expectLastCall().anyTimes(); - expect(accountLoader.get(account1)) - .andReturn(new AccountInfo(account1)).anyTimes(); - expect(accountLoader.get(account2)) - .andReturn(new AccountInfo(account2)).anyTimes(); + expect(accountLoader.get(ownerId)) + .andReturn(new AccountInfo(ownerId)).anyTimes(); + expect(accountLoader.get(otherUserId)) + .andReturn(new AccountInfo(otherUserId)).anyTimes(); expect(alf.create(true)).andReturn(accountLoader).anyTimes(); replay(accountLoader, alf); - revRes1 = createMock(RevisionResource.class); - revRes2 = createMock(RevisionResource.class); - PatchLineCommentAccess plca = createMock(PatchLineCommentAccess.class); expect(db.patchComments()).andReturn(plca).anyTimes(); - Change.Id changeId = new Change.Id(123); - PatchSet.Id psId1 = new PatchSet.Id(changeId, 1); + Change change = newChange(); + PatchSet.Id psId1 = new PatchSet.Id(change.getId(), 1); PatchSet ps1 = new PatchSet(psId1); - expect(revRes1.getPatchSet()).andReturn(ps1).anyTimes(); - PatchSet.Id psId2 = new PatchSet.Id(changeId, 2); + PatchSet.Id psId2 = new PatchSet.Id(change.getId(), 2); PatchSet ps2 = new PatchSet(psId2); - expect(revRes2.getPatchSet()).andReturn(ps2); long timeBase = TimeUtil.nowMs(); plc1 = newPatchLineComment(psId1, "Comment1", null, - "FileOne.txt", Side.REVISION, 1, account1, timeBase, + "FileOne.txt", Side.REVISION, 3, ownerId, timeBase, "First Comment", new CommentRange(1, 2, 3, 4)); + plc1.setRevId(new RevId("ABCDABCDABCDABCDABCDABCDABCDABCDABCDABCD")); plc2 = newPatchLineComment(psId1, "Comment2", "Comment1", - "FileOne.txt", Side.REVISION, 1, account2, timeBase + 1000, + "FileOne.txt", Side.REVISION, 3, otherUserId, timeBase + 1000, "Reply to First Comment", new CommentRange(1, 2, 3, 4)); + plc2.setRevId(new RevId("ABCDABCDABCDABCDABCDABCDABCDABCDABCDABCD")); plc3 = newPatchLineComment(psId1, "Comment3", "Comment1", - "FileOne.txt", Side.PARENT, 1, account1, timeBase + 2000, + "FileOne.txt", Side.PARENT, 3, ownerId, timeBase + 2000, "First Parent Comment", new CommentRange(1, 2, 3, 4)); + plc3.setRevId(new RevId("CDEFCDEFCDEFCDEFCDEFCDEFCDEFCDEFCDEFCDEF")); + + List commentsByOwner = Lists.newArrayList(); + commentsByOwner.add(plc1); + commentsByOwner.add(plc3); + List commentsByReviewer = Lists.newArrayList(); + commentsByReviewer.add(plc2); + + plca.upsert(commentsByOwner); + expectLastCall().anyTimes(); + plca.upsert(commentsByReviewer); + expectLastCall().anyTimes(); expect(plca.publishedByPatchSet(psId1)) .andAnswer(results(plc1, plc2, plc3)).anyTimes(); expect(plca.publishedByPatchSet(psId2)) .andAnswer(results()).anyTimes(); + replay(db, plca); - replay(db, revRes1, revRes2, plca); - injector = Guice.createInjector(mod); + ChangeUpdate update = newUpdate(change, changeOwner); + update.setPatchSetId(psId1); + plcUtil.addPublishedComments(db, update, commentsByOwner); + update.commit(); + + update = newUpdate(change, otherUser); + update.setPatchSetId(psId1); + plcUtil.addPublishedComments(db, update, commentsByReviewer); + update.commit(); + + ChangeControl ctl = stubChangeControl(change); + revRes1 = new RevisionResource(new ChangeResource(ctl), ps1); + revRes2 = new RevisionResource(new ChangeResource(ctl), ps2); + } + + private ChangeControl stubChangeControl(Change c) throws OrmException { + return TestChanges.stubChangeControl(repoManager, c, changeOwner); + } + + private Change newChange() { + return TestChanges.newChange(project, changeOwner); + } + + private ChangeUpdate newUpdate(Change c, final IdentifiedUser user) throws Exception { + return TestChanges.newUpdate(injector, repoManager, c, user); } @Test @@ -211,7 +339,8 @@ public class CommentsTest { assertEquals(plc.getLine(), (int) ci.line); assertEquals(plc.getSide() == 0 ? Side.PARENT : Side.REVISION, Objects.firstNonNull(ci.side, Side.REVISION)); - assertEquals(plc.getWrittenOn(), ci.updated); + assertEquals(TimeUtil.roundTimestampToSecond(plc.getWrittenOn()), + TimeUtil.roundTimestampToSecond(ci.updated)); assertEquals(plc.getRange(), ci.range); }