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); }