From 5020ff2fd06e3ada4ca163b4f5b24f13cdd9928b Mon Sep 17 00:00:00 2001 From: Yacob Yonas Date: Fri, 25 Jul 2014 09:45:07 -0700 Subject: [PATCH] Add draft comments to PatchLineCommentsUtil I added the ability to read and write published comments from either the notedb or the ReviewDb depending on the state of the NotesMigration instance. Additionally, I modified all callers of PatchLineCommentAccess that query for or edit comments to use the corresponding methods in PatchLineCommentsUtil. I added a new test to CommentsTest to test the reading and writing of draft comments from both the notedb and the ReviewDb with these new PatchLineCommentsUtil methods. Finally, I added a full integration test for inline comments in CommentsIT.java. Change-Id: Iac4ea144497fe68d28c3e886b91c7698741d6615 --- .../acceptance/server/change/CommentsIT.java | 198 ++++++++++++++ .../changedetail/PatchSetDetailFactory.java | 3 +- .../com/google/gerrit/server/ChangeUtil.java | 2 +- .../gerrit/server/PatchLineCommentsUtil.java | 250 ++++++++++++++++-- .../gerrit/server/change/ChangeJson.java | 11 +- .../gerrit/server/change/CreateDraft.java | 29 +- .../gerrit/server/change/DeleteDraft.java | 27 +- .../google/gerrit/server/change/Drafts.java | 12 +- .../gerrit/server/change/ListComments.java | 5 +- .../gerrit/server/change/ListDrafts.java | 12 +- .../gerrit/server/change/PostReview.java | 29 +- .../google/gerrit/server/change/PutDraft.java | 36 ++- .../gerrit/server/mail/CommentSender.java | 25 +- .../server/notedb/ChangeDraftUpdate.java | 43 ++- .../gerrit/server/notedb/ChangeUpdate.java | 30 ++- .../gerrit/server/notedb/NotesMigration.java | 12 +- .../server/patch/PatchScriptFactory.java | 3 +- .../query/change/BasicChangeRewrites.java | 3 +- .../server/query/change/ChangeData.java | 12 +- .../query/change/ChangeQueryBuilder.java | 4 + .../query/change/HasDraftByPredicate.java | 17 +- .../gerrit/server/change/CommentsTest.java | 145 +++++++--- .../gerrit/server/index/FakeQueryBuilder.java | 4 +- .../gerrit/server/notedb/ChangeNotesTest.java | 41 +-- 24 files changed, 785 insertions(+), 168 deletions(-) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java new file mode 100644 index 0000000000..9de5220fdf --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/CommentsIT.java @@ -0,0 +1,198 @@ +// 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.acceptance.server.change; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.RestResponse; +import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.common.Comment; +import com.google.gerrit.extensions.common.CommentInfo; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.testutil.ConfigSuite; +import com.google.gson.reflect.TypeToken; + +import org.apache.http.HttpStatus; +import org.eclipse.jgit.api.errors.GitAPIException; +import org.eclipse.jgit.lib.Config; +import org.junit.Test; + +import java.io.IOException; +import java.lang.reflect.Type; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class CommentsIT extends AbstractDaemonTest { + @ConfigSuite.Config + public static Config noteDbEnabled() { + Config cfg = new Config(); + cfg.setBoolean("notedb", null, "write", true); + cfg.setBoolean("notedb", "comments", "read", true); + return cfg; + } + + @Test + public void createDraft() throws GitAPIException, IOException { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + ReviewInput.CommentInput comment = newCommentInfo( + "file1", Comment.Side.REVISION, 1, "comment 1"); + addDraft(changeId, revId, comment); + Map> result = getDraftComments(changeId, revId); + assertEquals(1, result.size()); + CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path)); + assertCommentInfo(comment, actual); + } + + @Test + public void postComment() throws RestApiException, Exception { + String file = "file"; + String contents = "contents"; + PushOneCommit push = pushFactory.create(db, admin.getIdent(), + "first subject", file, contents); + PushOneCommit.Result r = push.to(git, "refs/for/master"); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + ReviewInput input = new ReviewInput(); + ReviewInput.CommentInput comment = newCommentInfo( + file, Comment.Side.REVISION, 1, "comment 1"); + input.comments = new HashMap>(); + input.comments.put(comment.path, Lists.newArrayList(comment)); + revision(r).review(input); + Map> result = getPublishedComments(changeId, revId); + assertTrue(!result.isEmpty()); + CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path)); + assertCommentInfo(comment, actual); + } + + @Test + public void putDraft() throws GitAPIException, IOException { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + ReviewInput.CommentInput comment = newCommentInfo( + "file1", Comment.Side.REVISION, 1, "comment 1"); + addDraft(changeId, revId, comment); + Map> result = getDraftComments(changeId, revId); + CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path)); + assertCommentInfo(comment, actual); + String uuid = actual.id; + comment.message = "updated comment 1"; + updateDraft(changeId, revId, comment, uuid); + result = getDraftComments(changeId, revId); + actual = Iterables.getOnlyElement(result.get(comment.path)); + assertCommentInfo(comment, actual); + } + + @Test + public void getDraft() throws GitAPIException, IOException { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + ReviewInput.CommentInput comment = newCommentInfo( + "file1", Comment.Side.REVISION, 1, "comment 1"); + CommentInfo returned = addDraft(changeId, revId, comment); + CommentInfo actual = getDraftComment(changeId, revId, returned.id); + assertCommentInfo(comment, actual); + } + + @Test + public void deleteDraft() throws IOException, GitAPIException { + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + String revId = r.getCommit().getName(); + ReviewInput.CommentInput comment = newCommentInfo( + "file1", Comment.Side.REVISION, 1, "comment 1"); + CommentInfo returned = addDraft(changeId, revId, comment); + deleteDraft(changeId, revId, returned.id); + Map> drafts = getDraftComments(changeId, revId); + assertTrue(drafts.isEmpty()); + } + + private CommentInfo addDraft(String changeId, String revId, + ReviewInput.CommentInput c) throws IOException { + RestResponse r = userSession.put( + "/changes/" + changeId + "/revisions/" + revId + "/drafts", c); + assertEquals(HttpStatus.SC_CREATED, r.getStatusCode()); + return newGson().fromJson(r.getReader(), CommentInfo.class); + } + + private void updateDraft(String changeId, String revId, + ReviewInput.CommentInput c, String uuid) throws IOException { + RestResponse r = userSession.put( + "/changes/" + changeId + "/revisions/" + revId + "/drafts/" + uuid, c); + assertEquals(HttpStatus.SC_OK, r.getStatusCode()); + } + + private void deleteDraft(String changeId, String revId, String uuid) + throws IOException { + RestResponse r = userSession.delete( + "/changes/" + changeId + "/revisions/" + revId + "/drafts/" + uuid); + assertEquals(HttpStatus.SC_NO_CONTENT, r.getStatusCode()); + } + + private Map> getPublishedComments(String changeId, + String revId) throws IOException { + RestResponse r = userSession.get( + "/changes/" + changeId + "/revisions/" + revId + "/comments/"); + assertEquals(HttpStatus.SC_OK, r.getStatusCode()); + Type mapType = new TypeToken>>() {}.getType(); + return newGson().fromJson(r.getReader(), mapType); + } + + private Map> getDraftComments(String changeId, + String revId) throws IOException { + RestResponse r = userSession.get( + "/changes/" + changeId + "/revisions/" + revId + "/drafts/"); + assertEquals(HttpStatus.SC_OK, r.getStatusCode()); + Type mapType = new TypeToken>>() {}.getType(); + return newGson().fromJson(r.getReader(), mapType); + } + + private CommentInfo getDraftComment(String changeId, String revId, + String uuid) throws IOException { + RestResponse r = userSession.get( + "/changes/" + changeId + "/revisions/" + revId + "/drafts/" + uuid); + assertEquals(HttpStatus.SC_OK, r.getStatusCode()); + return newGson().fromJson(r.getReader(), CommentInfo.class); + } + + private static void assertCommentInfo(ReviewInput.CommentInput expected, + CommentInfo actual) { + assertEquals(expected.line, actual.line); + assertEquals(expected.message, actual.message); + assertEquals(expected.inReplyTo, actual.inReplyTo); + if (actual.side == null) { + assertEquals(expected.side, Comment.Side.REVISION); + } + } + + private ReviewInput.CommentInput newCommentInfo(String path, + Comment.Side side, int line, String message) { + ReviewInput.CommentInput input = new ReviewInput.CommentInput(); + input.path = path; + input.side = side; + input.line = line; + input.message = message; + return input; + } +} 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 73cc83a92e..4a673cb651 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 @@ -170,7 +170,8 @@ class PatchSetDetailFactory extends Handler { // quickly locate where they have pending drafts, and review them. // final Account.Id me = ((IdentifiedUser) user).getAccountId(); - for (final PatchLineComment c : db.patchComments().draftByPatchSetAuthor(psIdNew, me)) { + for (PatchLineComment c + : plcUtil.draftByPatchSetAuthor(db, psIdNew, me, notes)) { final Patch p = byKey.get(c.getKey().getParentKey()); if (p != null) { p.setDraftCount(p.getDraftCount() + 1); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 80213f6bcd..2ed37cb46f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -508,8 +508,8 @@ public class ChangeUtil { ReviewDb db = this.db.get(); db.accountPatchReviews().delete(db.accountPatchReviews().byPatchSet(patchSetId)); db.changeMessages().delete(db.changeMessages().byPatchSet(patchSetId)); - db.patchComments().delete(db.patchComments().byPatchSet(patchSetId)); // No need to delete from notedb; draft patch sets will be filtered out. + db.patchComments().delete(db.patchComments().byPatchSet(patchSetId)); db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(patchSetId)); db.patchSetAncestors().delete(db.patchSetAncestors().byPatchSet(patchSetId)); 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 index f917613595..6aca1b5f0d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PatchLineCommentsUtil.java @@ -12,25 +12,44 @@ // 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.common.base.Optional; +import com.google.common.base.Predicate; +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchLineComment; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.config.AllUsersNameProvider; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.notedb.DraftCommentNotes; import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gerrit.server.patch.PatchList; +import com.google.gerrit.server.patch.PatchListCache; +import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.RefDatabase; +import org.eclipse.jgit.lib.Repository; + +import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Set; /** * Utility functions to manipulate PatchLineComments. @@ -40,45 +59,179 @@ import java.util.List; */ @Singleton public class PatchLineCommentsUtil { + private final GitRepositoryManager repoManager; + private final AllUsersName allUsers; + private final DraftCommentNotes.Factory draftFactory; private final NotesMigration migration; @VisibleForTesting @Inject - public PatchLineCommentsUtil(NotesMigration migration) { + public PatchLineCommentsUtil(GitRepositoryManager repoManager, + AllUsersNameProvider allUsersProvider, + DraftCommentNotes.Factory draftFactory, + NotesMigration migration) { + this.repoManager = repoManager; + this.allUsers = allUsersProvider.get(); + this.draftFactory = draftFactory; this.migration = migration; } + public Optional get(ReviewDb db, ChangeNotes notes, + PatchLineComment.Key key) throws OrmException { + if (!migration.readComments()) { + return Optional.fromNullable(db.patchComments().get(key)); + } + for (PatchLineComment c : byChange(db, notes)) { + if (key.equals(c.getKey())) { + return Optional.of(c); + } + } + return Optional.absent(); + } + + public List byChange(ReviewDb db, + ChangeNotes notes) throws OrmException { + if (!migration.readComments()) { + return db.patchComments().byChange(notes.getChangeId()).toList(); + } + notes.load(); + + List comments = Lists.newArrayList(); + comments.addAll(notes.getBaseComments().values()); + comments.addAll(notes.getPatchSetComments().values()); + + Iterable filtered = getDraftRefs(notes.getChangeId()); + for (String refName : filtered) { + Account.Id account = Account.Id.fromRefPart(refName); + if (account != null) { + comments.addAll(draftByChangeAuthor(db, notes, account)); + } + } + return comments; + } + + public List byPatchSet(ReviewDb db, + ChangeNotes notes, PatchSet.Id psId) throws OrmException { + if (!migration.readComments()) { + return db.patchComments().byPatchSet(psId).toList(); + } + List comments = Lists.newArrayList(); + comments.addAll(publishedByPatchSet(db, notes, psId)); + + Iterable filtered = getDraftRefs(notes.getChangeId()); + for (String refName : filtered) { + Account.Id account = Account.Id.fromRefPart(refName); + if (account != null) { + comments.addAll(draftByPatchSetAuthor(db, psId, account, notes)); + } + } + return comments; + } + public List publishedByChangeFile(ReviewDb db, ChangeNotes notes, Change.Id changeId, String file) throws OrmException { - if (!migration.readPublishedComments()) { + if (!migration.readComments()) { return db.patchComments().publishedByChangeFile(changeId, file).toList(); } notes.load(); - List commentsOnFile = new ArrayList(); + List comments = Lists.newArrayList(); - // We must iterate through all comments to find the ones on this file. - addCommentsInFile(commentsOnFile, notes.getBaseComments().values(), file); - addCommentsInFile(commentsOnFile, notes.getPatchSetComments().values(), + addCommentsOnFile(comments, notes.getBaseComments().values(), file); + addCommentsOnFile(comments, notes.getPatchSetComments().values(), file); - - Collections.sort(commentsOnFile, ChangeNotes.PatchLineCommentComparator); - return commentsOnFile; + Collections.sort(comments, ChangeNotes.PatchLineCommentComparator); + return comments; } public List publishedByPatchSet(ReviewDb db, ChangeNotes notes, PatchSet.Id psId) throws OrmException { - if (!migration.readPublishedComments()) { + if (!migration.readComments()) { 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; + List comments = new ArrayList(); + comments.addAll(notes.getPatchSetComments().get(psId)); + comments.addAll(notes.getBaseComments().get(psId)); + return comments; } - // TODO(yyonas): Delete drafts if they already existed. - public void addPublishedComments(ReviewDb db, ChangeUpdate update, + public List draftByPatchSetAuthor(ReviewDb db, + PatchSet.Id psId, Account.Id author, ChangeNotes notes) + throws OrmException { + if (!migration.readComments()) { + return db.patchComments().draftByPatchSetAuthor(psId, author).toList(); + } + + List comments = Lists.newArrayList(); + + comments.addAll(notes.getDraftBaseComments(author).row(psId).values()); + comments.addAll(notes.getDraftPsComments(author).row(psId).values()); + Collections.sort(comments, ChangeNotes.PatchLineCommentComparator); + return comments; + } + + public List draftByChangeFileAuthor(ReviewDb db, + ChangeNotes notes, String file, Account.Id author) + throws OrmException { + if (!migration.readComments()) { + return db.patchComments() + .draftByChangeFileAuthor(notes.getChangeId(), file, author) + .toList(); + } + List comments = Lists.newArrayList(); + addCommentsOnFile(comments, notes.getDraftBaseComments(author).values(), + file); + addCommentsOnFile(comments, notes.getDraftPsComments(author).values(), + file); + Collections.sort(comments, ChangeNotes.PatchLineCommentComparator); + return comments; + } + + public List draftByChangeAuthor(ReviewDb db, + ChangeNotes notes, Account.Id author) + throws OrmException { + if (!migration.readComments()) { + return db.patchComments().byChange(notes.getChangeId()).toList(); + } + List comments = Lists.newArrayList(); + comments.addAll(notes.getDraftBaseComments(author).values()); + comments.addAll(notes.getDraftPsComments(author).values()); + return comments; + } + + public List draftByAuthor(ReviewDb db, + Account.Id author) throws OrmException { + if (!migration.readComments()) { + return db.patchComments().draftByAuthor(author).toList(); + } + + Set refNames = + getRefNamesAllUsers(RefNames.REFS_DRAFT_COMMENTS); + + List comments = Lists.newArrayList(); + for (String refName : refNames) { + Account.Id id = Account.Id.fromRefPart(refName); + if (!author.equals(id)) { + continue; + } + Change.Id changeId = Change.Id.parse(refName); + DraftCommentNotes draftNotes = + draftFactory.create(changeId, author).load(); + comments.addAll(draftNotes.getDraftBaseComments().values()); + comments.addAll(draftNotes.getDraftPsComments().values()); + } + return comments; + } + + public void insertComments(ReviewDb db, ChangeUpdate update, + Iterable comments) throws OrmException { + for (PatchLineComment c : comments) { + update.insertComment(c); + } + db.patchComments().insert(comments); + } + + public void upsertComments(ReviewDb db, ChangeUpdate update, Iterable comments) throws OrmException { for (PatchLineComment c : comments) { update.upsertComment(c); @@ -86,7 +239,23 @@ public class PatchLineCommentsUtil { db.patchComments().upsert(comments); } - private static Collection addCommentsInFile( + public void updateComments(ReviewDb db, ChangeUpdate update, + Iterable comments) throws OrmException { + for (PatchLineComment c : comments) { + update.updateComment(c); + } + db.patchComments().update(comments); + } + + public void deleteComments(ReviewDb db, ChangeUpdate update, + Iterable comments) throws OrmException { + for (PatchLineComment c : comments) { + update.deleteComment(c); + } + db.patchComments().delete(comments); + } + + private static Collection addCommentsOnFile( Collection commentsOnFile, Collection allComments, String file) { @@ -98,4 +267,49 @@ public class PatchLineCommentsUtil { } return commentsOnFile; } + + public static void setCommentRevId(PatchLineComment c, + PatchListCache cache, Change change, PatchSet ps) throws OrmException { + if (c.getRevId() != null) { + return; + } + PatchList patchList; + try { + patchList = cache.get(change, ps); + } catch (PatchListNotAvailableException e) { + throw new OrmException(e); + } + c.setRevId((c.getSide() == (short) 0) + ? new RevId(ObjectId.toString(patchList.getOldId())) + : new RevId(ObjectId.toString(patchList.getNewId()))); + } + + private Set getRefNamesAllUsers(String prefix) throws OrmException { + Repository repo; + try { + repo = repoManager.openRepository(allUsers); + } catch (IOException e) { + throw new OrmException(e); + } + try { + RefDatabase refDb = repo.getRefDatabase(); + return refDb.getRefs(prefix).keySet(); + } catch (IOException e) { + throw new OrmException(e); + } finally { + repo.close(); + } + } + + private Iterable getDraftRefs(final Change.Id changeId) + throws OrmException { + Set refNames = getRefNamesAllUsers(RefNames.REFS_DRAFT_COMMENTS); + final String suffix = "-" + changeId.get(); + return Iterables.filter(refNames, new Predicate() { + @Override + public boolean apply(String input) { + return input.endsWith(suffix); + } + }); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 586c294909..fb80984a3b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -77,6 +77,7 @@ import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gerrit.server.WebLinks; import com.google.gerrit.server.account.AccountInfo; import com.google.gerrit.server.extensions.webui.UiActions; @@ -127,6 +128,7 @@ public class ChangeJson { private final Provider webLinks; private final EnumSet options; private final ChangeMessagesUtil cmUtil; + private final PatchLineCommentsUtil plcUtil; private AccountInfo.Loader accountLoader; @@ -147,7 +149,8 @@ public class ChangeJson { DynamicMap> changeViews, Revisions revisions, Provider webLinks, - ChangeMessagesUtil cmUtil) { + ChangeMessagesUtil cmUtil, + PatchLineCommentsUtil plcUtil) { this.db = db; this.labelNormalizer = ln; this.userProvider = user; @@ -163,6 +166,7 @@ public class ChangeJson { this.revisions = revisions; this.webLinks = webLinks; this.cmUtil = cmUtil; + this.plcUtil = plcUtil; options = EnumSet.noneOf(ListChangesOption.class); } @@ -821,9 +825,8 @@ public class ChangeJson { && userProvider.get().isIdentifiedUser()) { IdentifiedUser user = (IdentifiedUser)userProvider.get(); out.hasDraftComments = - db.get().patchComments() - .draftByPatchSetAuthor(in.getId(), user.getAccountId()) - .iterator().hasNext() + plcUtil.draftByPatchSetAuthor(db.get(), in.getId(), + user.getAccountId(), ctl.getNotes()).iterator().hasNext() ? true : null; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraft.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraft.java index 1d2fa40669..b025e65471 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraft.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraft.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.change; +import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId; + import com.google.common.base.Strings; import com.google.gerrit.common.changes.Side; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -24,27 +26,41 @@ import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gerrit.server.change.PutDraft.Input; +import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.util.TimeUtil; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; +import java.sql.Timestamp; import java.util.Collections; @Singleton class CreateDraft implements RestModifyView { private final Provider db; + private final ChangeUpdate.Factory updateFactory; + private final PatchLineCommentsUtil plcUtil; + private final PatchListCache patchListCache; @Inject - CreateDraft(Provider db) { + CreateDraft(Provider db, + ChangeUpdate.Factory updateFactory, + PatchLineCommentsUtil plcUtil, + PatchListCache patchListCache) { this.db = db; + this.updateFactory = updateFactory; + this.plcUtil = plcUtil; + this.patchListCache = patchListCache; } @Override public Response apply(RevisionResource rsrc, Input in) - throws BadRequestException, OrmException { + throws BadRequestException, OrmException, IOException { if (Strings.isNullOrEmpty(in.path)) { throw new BadRequestException("path must be non-empty"); } else if (in.message == null || in.message.trim().isEmpty()) { @@ -59,15 +75,20 @@ class CreateDraft implements RestModifyView { ? in.line : in.range != null ? in.range.getEndLine() : 0; + Timestamp now = TimeUtil.nowTs(); + ChangeUpdate update = updateFactory.create(rsrc.getControl(), now); + PatchLineComment c = new PatchLineComment( new PatchLineComment.Key( new Patch.Key(rsrc.getPatchSet().getId(), in.path), ChangeUtil.messageUUID(db.get())), - line, rsrc.getAccountId(), Url.decode(in.inReplyTo), TimeUtil.nowTs()); + line, rsrc.getAccountId(), Url.decode(in.inReplyTo), now); c.setSide(in.side == Side.PARENT ? (short) 0 : (short) 1); c.setMessage(in.message.trim()); c.setRange(in.range); - db.get().patchComments().insert(Collections.singleton(c)); + setCommentRevId(c, patchListCache, rsrc.getChange(), rsrc.getPatchSet()); + plcUtil.insertComments(db.get(), update, Collections.singleton(c)); + update.commit(); return Response.created(new CommentInfo(c, null)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraft.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraft.java index 46ae8341d3..f7fb3004c2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraft.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraft.java @@ -14,15 +14,22 @@ package com.google.gerrit.server.change; +import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId; + import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; +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.change.DeleteDraft.Input; +import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.patch.PatchListCache; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; import java.util.Collections; @Singleton @@ -31,16 +38,30 @@ class DeleteDraft implements RestModifyView { } private final Provider db; + private final PatchLineCommentsUtil plcUtil; + private final ChangeUpdate.Factory updateFactory; + private final PatchListCache patchListCache; @Inject - DeleteDraft(Provider db) { + DeleteDraft(Provider db, + PatchLineCommentsUtil plcUtil, + ChangeUpdate.Factory updateFactory, + PatchListCache patchListCache) { this.db = db; + this.plcUtil = plcUtil; + this.updateFactory = updateFactory; + this.patchListCache = patchListCache; } @Override public Response apply(DraftResource rsrc, Input input) - throws OrmException { - db.get().patchComments().delete(Collections.singleton(rsrc.getComment())); + throws OrmException, IOException { + ChangeUpdate update = updateFactory.create(rsrc.getControl()); + + PatchLineComment c = rsrc.getComment(); + setCommentRevId(c, patchListCache, rsrc.getChange(), rsrc.getPatchSet()); + plcUtil.deleteComments(db.get(), update, Collections.singleton(c)); + update.commit(); return Response.none(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Drafts.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Drafts.java index 322faea778..b0ed7d0439 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Drafts.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Drafts.java @@ -23,6 +23,7 @@ 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.CurrentUser; +import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -34,16 +35,19 @@ class Drafts implements ChildCollection { private final Provider user; private final ListDrafts list; private final Provider dbProvider; + private final PatchLineCommentsUtil plcUtil; @Inject Drafts(DynamicMap> views, Provider user, ListDrafts list, - Provider dbProvider) { + Provider dbProvider, + PatchLineCommentsUtil plcUtil) { this.views = views; this.user = user; this.list = list; this.dbProvider = dbProvider; + this.plcUtil = plcUtil; } @Override @@ -62,10 +66,8 @@ class Drafts implements ChildCollection { throws ResourceNotFoundException, OrmException, AuthException { checkIdentifiedUser(); String uuid = id.get(); - for (PatchLineComment c : dbProvider.get().patchComments() - .draftByPatchSetAuthor( - rev.getPatchSet().getId(), - rev.getAccountId())) { + for (PatchLineComment c : plcUtil.draftByPatchSetAuthor(dbProvider.get(), + rev.getPatchSet().getId(), rev.getAccountId(), rev.getNotes())) { if (uuid.equals(c.getKey().get())) { return new DraftResource(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 f4d7b496e7..146ded8ceb 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 @@ -26,13 +26,10 @@ import com.google.inject.Singleton; @Singleton class ListComments extends ListDrafts { - private final PatchLineCommentsUtil plcUtil; - @Inject ListComments(Provider db, AccountInfo.Loader.Factory alf, PatchLineCommentsUtil plcUtil) { - super(db, alf); - this.plcUtil = plcUtil; + super(db, alf, plcUtil); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListDrafts.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListDrafts.java index bd3aa0447f..1a26898081 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListDrafts.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListDrafts.java @@ -22,6 +22,7 @@ import com.google.gerrit.common.changes.Side; import com.google.gerrit.extensions.restapi.RestReadView; 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.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -36,20 +37,21 @@ import java.util.Map; @Singleton class ListDrafts implements RestReadView { protected final Provider db; + protected final PatchLineCommentsUtil plcUtil; private final AccountInfo.Loader.Factory accountLoaderFactory; @Inject - ListDrafts(Provider db, AccountInfo.Loader.Factory alf) { + ListDrafts(Provider db, AccountInfo.Loader.Factory alf, + PatchLineCommentsUtil plcUtil) { this.db = db; this.accountLoaderFactory = alf; + this.plcUtil = plcUtil; } protected Iterable listComments(RevisionResource rsrc) throws OrmException { - return db.get().patchComments() - .draftByPatchSetAuthor( - rsrc.getPatchSet().getId(), - rsrc.getAccountId()); + return plcUtil.draftByPatchSetAuthor(db.get(), rsrc.getPatchSet().getId(), + rsrc.getAccountId(), rsrc.getNotes()); } protected boolean includeAuthorInfo() { 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 9a2a81e190..bf8474ff7c 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 @@ -15,6 +15,7 @@ package com.google.gerrit.server.change; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId; import com.google.common.base.Objects; import com.google.common.base.Strings; @@ -44,7 +45,6 @@ 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; @@ -54,9 +54,7 @@ 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; @@ -65,7 +63,6 @@ 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; @@ -346,15 +343,6 @@ 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()) { @@ -374,7 +362,8 @@ 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); + setCommentRevId(e, patchListCache, rsrc.getChange(), + rsrc.getPatchSet()); e.setMessage(c.message); if (c.range != null) { e.setRange(new CommentRange( @@ -398,13 +387,14 @@ 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); + setCommentRevId(e, patchListCache, rsrc.getChange(), + rsrc.getPatchSet()); ups.add(e); } break; } - db.get().patchComments().delete(del); - plcUtil.addPublishedComments(db.get(), update, ups); + plcUtil.deleteComments(db.get(), update, del); + plcUtil.upsertComments(db.get(), update, ups); comments.addAll(ups); return !del.isEmpty() || !ups.isEmpty(); } @@ -412,9 +402,8 @@ public class PostReview implements RestModifyView private Map scanDraftComments( RevisionResource rsrc) throws OrmException { Map drafts = Maps.newHashMap(); - for (PatchLineComment c : db.get().patchComments().draftByPatchSetAuthor( - rsrc.getPatchSet().getId(), - rsrc.getAccountId())) { + for (PatchLineComment c : plcUtil.draftByPatchSetAuthor(db.get(), + rsrc.getPatchSet().getId(), rsrc.getAccountId(), rsrc.getNotes())) { drafts.put(c.getKey().get(), c); } return drafts; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraft.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraft.java index c1fb30430a..a3b0614eab 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraft.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutDraft.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.change; +import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId; + import com.google.gerrit.common.changes.Side; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.DefaultInput; @@ -24,13 +26,17 @@ 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.server.ReviewDb; +import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gerrit.server.change.PutDraft.Input; +import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.util.TimeUtil; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.io.IOException; import java.sql.Timestamp; import java.util.Collections; @@ -51,17 +57,28 @@ class PutDraft implements RestModifyView { private final Provider db; private final DeleteDraft delete; + private final PatchLineCommentsUtil plcUtil; + private final ChangeUpdate.Factory updateFactory; + private final PatchListCache patchListCache; @Inject - PutDraft(Provider db, DeleteDraft delete) { + PutDraft(Provider db, + DeleteDraft delete, + PatchLineCommentsUtil plcUtil, + ChangeUpdate.Factory updateFactory, + PatchListCache patchListCache) { this.db = db; this.delete = delete; + this.plcUtil = plcUtil; + this.updateFactory = updateFactory; + this.patchListCache = patchListCache; } @Override public Response apply(DraftResource rsrc, Input in) throws - BadRequestException, OrmException { + BadRequestException, OrmException, IOException { PatchLineComment c = rsrc.getComment(); + ChangeUpdate update = updateFactory.create(rsrc.getControl()); if (in == null || in.message == null || in.message.trim().isEmpty()) { return delete.apply(rsrc, null); } else if (in.id != null && !rsrc.getId().equals(in.id)) { @@ -76,7 +93,8 @@ class PutDraft implements RestModifyView { && !in.path.equals(c.getKey().getParentKey().getFileName())) { // Updating the path alters the primary key, which isn't possible. // Delete then recreate the comment instead of an update. - db.get().patchComments().delete(Collections.singleton(c)); + + plcUtil.deleteComments(db.get(), update, Collections.singleton(c)); c = new PatchLineComment( new PatchLineComment.Key( new Patch.Key(rsrc.getPatchSet().getId(), in.path), @@ -84,10 +102,18 @@ class PutDraft implements RestModifyView { c.getLine(), rsrc.getAuthorId(), c.getParentUuid(), TimeUtil.nowTs()); - db.get().patchComments().insert(Collections.singleton(update(c, in))); + setCommentRevId(c, patchListCache, rsrc.getChange(), rsrc.getPatchSet()); + plcUtil.insertComments(db.get(), update, + Collections.singleton(update(c, in))); } else { - db.get().patchComments().update(Collections.singleton(update(c, in))); + if (c.getRevId() == null) { + setCommentRevId(c, patchListCache, rsrc.getChange(), + rsrc.getPatchSet()); + } + plcUtil.updateComments(db.get(), update, + Collections.singleton(update(c, in))); } + update.commit(); return Response.ok(new CommentInfo(c, null)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java index 2beb49fbd7..5f2ffcb02d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/CommentSender.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.mail; +import com.google.common.base.Optional; import com.google.common.base.Strings; import com.google.common.collect.Ordering; import com.google.gerrit.common.errors.EmailException; @@ -24,6 +25,7 @@ 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.PatchSet; +import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gerrit.server.patch.PatchFile; import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListNotAvailableException; @@ -53,13 +55,16 @@ public class CommentSender extends ReplyToChangeSender { private final NotifyHandling notify; private List inlineComments = Collections.emptyList(); + private final PatchLineCommentsUtil plcUtil; @Inject public CommentSender(EmailArguments ea, @Assisted NotifyHandling notify, - @Assisted Change c) { + @Assisted Change c, + PatchLineCommentsUtil plcUtil) { super(ea, c, "comment"); this.notify = notify; + this.plcUtil = plcUtil; } public void setPatchLineComments(final List plc) @@ -232,17 +237,19 @@ public class CommentSender extends ReplyToChangeSender { private void appendQuotedParent(StringBuilder out, PatchLineComment child) { if (child.getParentUuid() != null) { - PatchLineComment parent; + Optional parent; + PatchLineComment.Key key = new PatchLineComment.Key( + child.getKey().getParentKey(), + child.getParentUuid()); try { - parent = args.db.get().patchComments().get( - new PatchLineComment.Key( - child.getKey().getParentKey(), - child.getParentUuid())); + parent = plcUtil.get(args.db.get(), changeData.notes(), key); } catch (OrmException e) { - parent = null; + log.warn("Could not find the parent of this comment: " + + child.toString()); + parent = Optional.absent(); } - if (parent != null) { - String msg = parent.getMessage().trim(); + if (parent.isPresent()) { + String msg = parent.get().getMessage().trim(); if (msg.length() > 75) { msg = msg.substring(0, 75); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java index 75104126b5..ca32c14349 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java @@ -105,9 +105,11 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { verifyComment(c); checkArgument(c.getStatus() == Status.DRAFT, "Cannot insert a published comment into a ChangeDraftUpdate"); - checkArgument(!changeNotes.containsComment(c), - "A comment already exists with the same key," - + " so the following comment cannot be inserted: %s", c); + if (migration.readComments()) { + checkArgument(!changeNotes.containsComment(c), + "A comment already exists with the same key," + + " so the following comment cannot be inserted: %s", c); + } upsertComments.add(c); } @@ -122,16 +124,32 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { verifyComment(c); checkArgument(c.getStatus() == Status.DRAFT, "Cannot update a published comment into a ChangeDraftUpdate"); - checkArgument(draftNotes.containsComment(c), - "Cannot update this comment because it didn't exist previously"); + // Here, we check to see if this comment existed previously as a draft. + // However, this could cause a race condition if there is a delete and an + // update operation happening concurrently (or two deletes) and they both + // believe that the comment exists. If a delete happens first, then + // the update will fail. However, this is an acceptable risk since the + // caller wanted the comment deleted anyways, so the end result will be the + // same either way. + if (migration.readComments()) { + checkArgument(draftNotes.containsComment(c), + "Cannot update this comment because it didn't exist previously"); + } upsertComments.add(c); } public void deleteComment(PatchLineComment c) { verifyComment(c); - checkArgument(draftNotes.containsComment(c), "Cannot delete this comment" - + " because it didn't previously exist as a draft"); - deleteComments.add(c); + // See the comment above about potential race condition. + if (migration.readComments()) { + checkArgument(draftNotes.containsComment(c), "Cannot delete this comment" + + " because it didn't previously exist as a draft"); + } + if (migration.write()) { + if (draftNotes.containsComment(c)) { + deleteComments.add(c); + } + } } /** @@ -151,7 +169,9 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { checkArgument(getCommentPsId(comment).equals(psId), "Comment on %s does not match configured patch set %s", getCommentPsId(comment), psId); - checkArgument(comment.getRevId() != null); + if (migration.write()) { + checkArgument(comment.getRevId() != null); + } checkArgument(comment.getAuthor().equals(accountId), "The author for the following comment does not match the author of" + " this ChangeDraftUpdate (%s): %s", accountId, comment); @@ -174,6 +194,8 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { Table psDrafts = draftNotes.getDraftPsComments(); + boolean draftsEmpty = baseDrafts.isEmpty() && psDrafts.isEmpty(); + // There is no need to rewrite the note for one of the sides of the patch // set if all of the modifications were made to the comments of one side, // so we set these flags to potentially save that extra work. @@ -219,7 +241,8 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate { updateNoteMap(revisionSideChanged, noteMap, newPsDrafts, psRevId); - removedAllComments.set(baseDrafts.isEmpty() && psDrafts.isEmpty()); + removedAllComments.set( + baseDrafts.isEmpty() && psDrafts.isEmpty() && !draftsEmpty); return noteMap.writeTree(inserter); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 24d4b46747..770315a232 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -239,9 +239,11 @@ public class ChangeUpdate extends AbstractChangeUpdate { if (notes == null) { notes = getChangeNotes().load(); } - checkArgument(!notes.containsComment(c), - "A comment already exists with the same key as the following comment," - + " so we cannot insert this comment: %s", c); + if (migration.readComments()) { + checkArgument(!notes.containsComment(c), + "A comment already exists with the same key as the following comment," + + " so we cannot insert this comment: %s", c); + } if (c.getSide() == 0) { commentsForBase.add(c); } else { @@ -254,8 +256,19 @@ public class ChangeUpdate extends AbstractChangeUpdate { draftUpdate.insertComment(c); } - private void upsertPublishedComment(PatchLineComment c) { + private void upsertPublishedComment(PatchLineComment c) throws OrmException { verifyComment(c); + if (notes == null) { + notes = getChangeNotes().load(); + } + // This could allow callers to update a published comment if migration.write + // is on and migration.readComments is off because we will not be able to + // verify that the comment didn't already exist as a published comment + // since we don't have a ReviewDb. + if (migration.readComments()) { + checkArgument(!notes.containsCommentPublished(c), + "Cannot update a comment that has already been published and saved"); + } if (c.getSide() == 0) { commentsForBase.add(c); } else { @@ -273,8 +286,12 @@ public class ChangeUpdate extends AbstractChangeUpdate { if (notes == null) { notes = getChangeNotes().load(); } - checkArgument(!notes.containsCommentPublished(c), - "Cannot update a comment that has already been published and saved"); + // See comment above in upsertPublishedComment() about potential risk with + // this check. + if (migration.readComments()) { + checkArgument(!notes.containsCommentPublished(c), + "Cannot update a comment that has already been published and saved"); + } if (c.getSide() == 0) { commentsForBase.add(c); } else { @@ -298,7 +315,6 @@ public class ChangeUpdate extends AbstractChangeUpdate { draftUpdate.deleteCommentIfPresent(c); } - private void createDraftUpdateIfNull(PatchLineComment c) throws OrmException { if (draftUpdate == null) { draftUpdate = draftUpdateFactory.create(ctl, when); 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 36f685d70f..679e27287e 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,14 +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); + cfg.setBoolean("notedb", "comments", "read", true); return new NotesMigration(cfg); } private final boolean write; private final boolean readPatchSetApprovals; private final boolean readChangeMessages; - private final boolean readPublishedComments; + private final boolean readComments; @Inject NotesMigration(@GerritServerConfig Config cfg) { @@ -52,8 +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); + readComments = + cfg.getBoolean("notedb", "comments", "read", false); } public boolean write() { @@ -68,7 +68,7 @@ public class NotesMigration { return readChangeMessages; } - public boolean readPublishedComments() { - return readPublishedComments; + public boolean readComments() { + return readComments; } } 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 b4337cf45c..decae6dc03 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 @@ -338,7 +338,8 @@ public class PatchScriptFactory implements Callable { private void loadDrafts(final Map byKey, final AccountInfoCacheFactory aic, final Account.Id me, final String file) throws OrmException { - for (PatchLineComment c : db.patchComments().draftByChangeFileAuthor(changeId, file, me)) { + for (PatchLineComment c : + plcUtil.draftByChangeFileAuthor(db, control.getNotes(), file, me)) { if (comments.include(c)) { aic.want(me); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java index 8ce6fa3772..37d96ea3b9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java @@ -30,7 +30,8 @@ public class BasicChangeRewrites extends QueryRewriter { new InvalidProvider(), // new InvalidProvider(), // null, null, null, null, null, null, null, // - null, null, null, null, null, null, null, null, null, null), null); + null, null, null, null, null, null, null, null, null, null, null), + null); private static final QueryRewriter.Definition mydef = new QueryRewriter.Definition( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index ed640154b8..5eb96550d4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -35,6 +35,7 @@ import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; @@ -154,7 +155,7 @@ public class ChangeData { */ static ChangeData createForTest(Change.Id id, int currentPatchSetId) { ChangeData cd = new ChangeData(null, null, null, null, null, - null, null, null, null, id); + null, null, null, null, null, id); cd.currentPatchSet = new PatchSet(new PatchSet.Id(id, currentPatchSetId)); return cd; } @@ -166,6 +167,7 @@ public class ChangeData { private final ChangeNotes.Factory notesFactory; private final ApprovalsUtil approvalsUtil; private final ChangeMessagesUtil cmUtil; + private final PatchLineCommentsUtil plcUtil; private final PatchListCache patchListCache; private final NotesMigration notesMigration; private final Change.Id legacyId; @@ -194,6 +196,7 @@ public class ChangeData { ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, ChangeMessagesUtil cmUtil, + PatchLineCommentsUtil plcUtil, PatchListCache patchListCache, NotesMigration notesMigration, @Assisted ReviewDb db, @@ -205,6 +208,7 @@ public class ChangeData { this.notesFactory = notesFactory; this.approvalsUtil = approvalsUtil; this.cmUtil = cmUtil; + this.plcUtil = plcUtil; this.patchListCache = patchListCache; this.notesMigration = notesMigration; legacyId = id; @@ -218,6 +222,7 @@ public class ChangeData { ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, ChangeMessagesUtil cmUtil, + PatchLineCommentsUtil plcUtil, PatchListCache patchListCache, NotesMigration notesMigration, @Assisted ReviewDb db, @@ -229,6 +234,7 @@ public class ChangeData { this.notesFactory = notesFactory; this.approvalsUtil = approvalsUtil; this.cmUtil = cmUtil; + this.plcUtil = plcUtil; this.patchListCache = patchListCache; this.notesMigration = notesMigration; legacyId = c.getId(); @@ -243,6 +249,7 @@ public class ChangeData { ChangeNotes.Factory notesFactory, ApprovalsUtil approvalsUtil, ChangeMessagesUtil cmUtil, + PatchLineCommentsUtil plcUtil, PatchListCache patchListCache, NotesMigration notesMigration, @Assisted ReviewDb db, @@ -254,6 +261,7 @@ public class ChangeData { this.notesFactory = notesFactory; this.approvalsUtil = approvalsUtil; this.cmUtil = cmUtil; + this.plcUtil = plcUtil; this.patchListCache = patchListCache; this.notesMigration = notesMigration; legacyId = c.getChange().getId(); @@ -522,7 +530,7 @@ public class ChangeData { public Collection comments() throws OrmException { if (comments == null) { - comments = db.patchComments().byChange(legacyId).toList(); + comments = plcUtil.byChange(db, notes()); } return comments; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index ac7b9aed85..fabe61cd9e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -25,6 +25,7 @@ import com.google.gerrit.reviewdb.client.Change; 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.AccountResolver; import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupBackend; @@ -146,6 +147,7 @@ public class ChangeQueryBuilder extends QueryBuilder { final CapabilityControl.Factory capabilityControlFactory; final ChangeControl.GenericFactory changeControlGenericFactory; final ChangeData.Factory changeDataFactory; + final PatchLineCommentsUtil plcUtil; final AccountResolver accountResolver; final GroupBackend groupBackend; final AllProjectsName allProjectsName; @@ -168,6 +170,7 @@ public class ChangeQueryBuilder extends QueryBuilder { CapabilityControl.Factory capabilityControlFactory, ChangeControl.GenericFactory changeControlGenericFactory, ChangeData.Factory changeDataFactory, + PatchLineCommentsUtil plcUtil, AccountResolver accountResolver, GroupBackend groupBackend, AllProjectsName allProjectsName, @@ -187,6 +190,7 @@ public class ChangeQueryBuilder extends QueryBuilder { this.capabilityControlFactory = capabilityControlFactory; this.changeControlGenericFactory = changeControlGenericFactory; this.changeDataFactory = changeDataFactory; + this.plcUtil = plcUtil; this.accountResolver = accountResolver; this.groupBackend = groupBackend; this.allProjectsName = allProjectsName; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/HasDraftByPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/HasDraftByPredicate.java index 53d2bbda2c..ebb7389bc7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/HasDraftByPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/HasDraftByPredicate.java @@ -33,7 +33,8 @@ class HasDraftByPredicate extends OperatorPredicate implements private final Arguments args; private final Account.Id accountId; - HasDraftByPredicate(Arguments args, Account.Id accountId) { + HasDraftByPredicate(Arguments args, + Account.Id accountId) { super(ChangeQueryBuilder.FIELD_DRAFTBY, accountId.toString()); this.args = args; this.accountId = accountId; @@ -41,20 +42,16 @@ class HasDraftByPredicate extends OperatorPredicate implements @Override public boolean match(final ChangeData object) throws OrmException { - for (PatchLineComment c : object.comments()) { - if (c.getStatus() == PatchLineComment.Status.DRAFT - && c.getAuthor().equals(accountId)) { - return true; - } - } - return false; + return !args.plcUtil + .draftByChangeAuthor(args.db.get(), object.notes(), accountId) + .isEmpty(); } @Override public ResultSet read() throws OrmException { Set ids = new HashSet<>(); - for (PatchLineComment sc : args.db.get().patchComments() - .draftByAuthor(accountId)) { + for (PatchLineComment sc : + args.plcUtil.draftByAuthor(args.db.get(), accountId)) { ids.add(sc.getKey().getParentKey().getParentKey().getParentKey()); } 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 da687be97b..9e4d2b2097 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 @@ -43,6 +43,7 @@ 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.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchLineCommentsUtil; @@ -60,7 +61,6 @@ 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; @@ -74,6 +74,8 @@ 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.Provides; +import com.google.inject.Singleton; import com.google.inject.TypeLiteral; import com.google.inject.util.Providers; @@ -104,7 +106,7 @@ public class CommentsTest { public static @GerritServerConfig Config noteDbEnabled() { @GerritServerConfig Config cfg = new Config(); cfg.setBoolean("notedb", null, "write", true); - cfg.setBoolean("notedb", "publishedComments", "read", true); + cfg.setBoolean("notedb", "comments", "read", true); return cfg; } @@ -118,15 +120,23 @@ public class CommentsTest { private PatchLineComment plc1; private PatchLineComment plc2; private PatchLineComment plc3; + private PatchLineComment plc4; + private PatchLineComment plc5; private IdentifiedUser changeOwner; @Before public void setUp() throws Exception { @SuppressWarnings("unchecked") - final DynamicMap> views = + final DynamicMap> commentViews = createMock(DynamicMap.class); - final TypeLiteral>> viewsType = + final TypeLiteral>> commentViewsType = new TypeLiteral>>() {}; + @SuppressWarnings("unchecked") + final DynamicMap> draftViews = + createMock(DynamicMap.class); + final TypeLiteral>> draftViewsType = + new TypeLiteral>>() {}; + final AccountInfo.Loader.Factory alf = createMock(AccountInfo.Loader.Factory.class); final ReviewDb db = createMock(ReviewDb.class); @@ -139,10 +149,23 @@ public class CommentsTest { @SuppressWarnings("unused") InMemoryRepository repo = repoManager.createRepository(project); + Account co = new Account(new Account.Id(1), TimeUtil.nowTs()); + co.setFullName("Change Owner"); + co.setPreferredEmail("change@owner.com"); + accountCache.put(co); + final 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(); + AbstractModule mod = new AbstractModule() { @Override protected void configure() { - bind(viewsType).toInstance(views); + bind(commentViewsType).toInstance(commentViews); + bind(draftViewsType).toInstance(draftViews); bind(AccountInfo.Loader.Factory.class).toInstance(alf); bind(ReviewDb.class).toProvider(Providers.of(db)); bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(config); @@ -162,28 +185,16 @@ public class CommentsTest { bind(PersonIdent.class).annotatedWith(GerritPersonIdent.class) .toInstance(serverIdent); } + + @Provides + @Singleton + CurrentUser getCurrentUser(IdentifiedUser.GenericFactory userFactory) { + return userFactory.create(ownerId); + } }; injector = Guice.createInjector(mod); - NotesMigration migration = injector.getInstance(NotesMigration.class); - allUsers = injector.getInstance(AllUsersNameProvider.class); - repoManager.createRepository(allUsers.get()); - - 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); @@ -199,6 +210,10 @@ public class CommentsTest { expect(alf.create(true)).andReturn(accountLoader).anyTimes(); replay(accountLoader, alf); + allUsers = injector.getInstance(AllUsersNameProvider.class); + repoManager.createRepository(allUsers.get()); + plcUtil = injector.getInstance(PatchLineCommentsUtil.class); + PatchLineCommentAccess plca = createMock(PatchLineCommentAccess.class); expect(db.patchComments()).andReturn(plca).anyTimes(); @@ -221,32 +236,54 @@ public class CommentsTest { "FileOne.txt", Side.PARENT, 3, ownerId, timeBase + 2000, "First Parent Comment", new CommentRange(1, 2, 3, 4)); plc3.setRevId(new RevId("CDEFCDEFCDEFCDEFCDEFCDEFCDEFCDEFCDEFCDEF")); + plc4 = newPatchLineComment(psId2, "Comment4", null, "FileOne.txt", + Side.REVISION, 3, ownerId, timeBase + 3000, "Second Comment", + new CommentRange(1, 2, 3, 4), Status.DRAFT); + plc4.setRevId(new RevId("BCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDE")); + plc5 = newPatchLineComment(psId2, "Comment5", null, "FileOne.txt", + Side.REVISION, 5, ownerId, timeBase + 4000, "Third Comment", + new CommentRange(3, 4, 5, 6), Status.DRAFT); + plc5.setRevId(new RevId("BCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDEBCDE")); List commentsByOwner = Lists.newArrayList(); commentsByOwner.add(plc1); commentsByOwner.add(plc3); List commentsByReviewer = Lists.newArrayList(); commentsByReviewer.add(plc2); + List drafts = Lists.newArrayList(); + drafts.add(plc4); + drafts.add(plc5); plca.upsert(commentsByOwner); expectLastCall().anyTimes(); plca.upsert(commentsByReviewer); expectLastCall().anyTimes(); + plca.upsert(drafts); + expectLastCall().anyTimes(); expect(plca.publishedByPatchSet(psId1)) .andAnswer(results(plc1, plc2, plc3)).anyTimes(); expect(plca.publishedByPatchSet(psId2)) .andAnswer(results()).anyTimes(); + expect(plca.draftByPatchSetAuthor(psId1, ownerId)) + .andAnswer(results()).anyTimes(); + expect(plca.draftByPatchSetAuthor(psId2, ownerId)) + .andAnswer(results(plc4, plc5)).anyTimes(); replay(db, plca); ChangeUpdate update = newUpdate(change, changeOwner); update.setPatchSetId(psId1); - plcUtil.addPublishedComments(db, update, commentsByOwner); + plcUtil.upsertComments(db, update, commentsByOwner); update.commit(); update = newUpdate(change, otherUser); update.setPatchSetId(psId1); - plcUtil.addPublishedComments(db, update, commentsByReviewer); + plcUtil.upsertComments(db, update, commentsByReviewer); + update.commit(); + + update = newUpdate(change, changeOwner); + update.setPatchSetId(psId2); + plcUtil.upsertComments(db, update, drafts); update.commit(); ChangeControl ctl = stubChangeControl(change); @@ -286,6 +323,17 @@ public class CommentsTest { assertGetComment(injector, revRes1, null, "BadComment"); } + @Test + public void testListDrafts() throws Exception { + // test ListDrafts for patch set 1 + assertListDrafts(injector, revRes1, + Collections.> emptyMap()); + + // test ListDrafts for patch set 2 + assertListDrafts(injector, revRes2, ImmutableMap.of( + "FileOne.txt", Lists.newArrayList(plc4, plc5))); + } + private static IAnswer> results( final PatchLineComment... comments) { return new IAnswer>() { @@ -305,7 +353,7 @@ public class CommentsTest { fail("Expected no comment"); } CommentInfo actual = getComment.apply(commentRes); - assertComment(expected, actual); + assertComment(expected, actual, true); } catch (ResourceNotFoundException e) { if (expected != null) { fail("Expected to find comment"); @@ -330,17 +378,42 @@ public class CommentsTest { assertNotNull(actualComments); assertEquals(expectedComments.size(), actualComments.size()); for (int i = 0; i < expectedComments.size(); i++) { - assertComment(expectedComments.get(i), actualComments.get(i)); + assertComment(expectedComments.get(i), actualComments.get(i), true); } } } - private static void assertComment(PatchLineComment plc, CommentInfo ci) { + private static void assertListDrafts(Injector inj, RevisionResource res, + Map> expected) throws Exception { + Drafts drafts = inj.getInstance(Drafts.class); + RestReadView listView = + (RestReadView) drafts.list(); + @SuppressWarnings("unchecked") + Map> actual = + (Map>) listView.apply(res); + assertNotNull(actual); + assertEquals(expected.size(), actual.size()); + assertEquals(expected.keySet(), actual.keySet()); + for (Map.Entry> entry : expected.entrySet()) { + List expectedComments = entry.getValue(); + List actualComments = actual.get(entry.getKey()); + assertNotNull(actualComments); + assertEquals(expectedComments.size(), actualComments.size()); + for (int i = 0; i < expectedComments.size(); i++) { + assertComment(expectedComments.get(i), actualComments.get(i), false); + } + } + } + + private static void assertComment(PatchLineComment plc, CommentInfo ci, + boolean isPublished) { assertEquals(plc.getKey().get(), ci.id); assertEquals(plc.getParentUuid(), ci.inReplyTo); assertEquals(plc.getMessage(), ci.message); - assertNotNull(ci.author); - assertEquals(plc.getAuthor(), ci.author._id); + if (isPublished) { + assertNotNull(ci.author); + assertEquals(plc.getAuthor(), ci.author._id); + } assertEquals(plc.getLine(), (int) ci.line); assertEquals(plc.getSide() == 0 ? Side.PARENT : Side.REVISION, Objects.firstNonNull(ci.side, Side.REVISION)); @@ -351,7 +424,8 @@ public class CommentsTest { private static PatchLineComment newPatchLineComment(PatchSet.Id psId, String uuid, String inReplyToUuid, String filename, Side side, int line, - Account.Id authorId, long millis, String message, CommentRange range) { + Account.Id authorId, long millis, String message, CommentRange range, + PatchLineComment.Status status) { Patch.Key p = new Patch.Key(psId, filename); PatchLineComment.Key id = new PatchLineComment.Key(p, uuid); PatchLineComment plc = @@ -359,8 +433,15 @@ public class CommentsTest { plc.setMessage(message); plc.setRange(range); plc.setSide(side == Side.PARENT ? (short) 0 : (short) 1); - plc.setStatus(Status.PUBLISHED); + plc.setStatus(status); plc.setWrittenOn(new Timestamp(millis)); return plc; } + + private static PatchLineComment newPatchLineComment(PatchSet.Id psId, + String uuid, String inReplyToUuid, String filename, Side side, int line, + Account.Id authorId, long millis, String message, CommentRange range) { + return newPatchLineComment(psId, uuid, inReplyToUuid, filename, side, line, + authorId, millis, message, range, Status.PUBLISHED); + } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java index 50e57642aa..7090f0df54 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java @@ -26,8 +26,8 @@ public class FakeQueryBuilder extends ChangeQueryBuilder { new FakeQueryBuilder.Definition<>( FakeQueryBuilder.class), new ChangeQueryBuilder.Arguments(null, null, null, null, null, null, - null, null, null, null, null, null, null, null, indexes, null, null, - null, null), + null, null, null, null, null, null, null, null, null, indexes, null, + null, null, null), null); } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java index 81d2503c2a..a089ea6d02 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java @@ -895,7 +895,9 @@ public class ChangeNotesTest { public void patchLineCommentNotesFormatSide1() throws Exception { Change c = newChange(); ChangeUpdate update = newUpdate(c, otherUser); - String uuid = "uuid"; + String uuid1 = "uuid1"; + String uuid2 = "uuid2"; + String uuid3 = "uuid3"; String message1 = "comment 1"; String message2 = "comment 2"; String message3 = "comment 3"; @@ -906,7 +908,7 @@ public class ChangeNotesTest { PatchSet.Id psId = c.currentPatchSetId(); PatchLineComment comment1 = newPublishedPatchLineComment(psId, "file1", - uuid, range1, range1.getEndLine(), otherUser, null, time1, message1, + uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1, (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); update.upsertComment(comment1); @@ -915,7 +917,7 @@ public class ChangeNotesTest { update = newUpdate(c, otherUser); CommentRange range2 = new CommentRange(2, 1, 3, 1); PatchLineComment comment2 = newPublishedPatchLineComment(psId, "file1", - uuid, range2, range2.getEndLine(), otherUser, null, time2, message2, + uuid2, range2, range2.getEndLine(), otherUser, null, time2, message2, (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); update.upsertComment(comment2); @@ -924,7 +926,7 @@ public class ChangeNotesTest { update = newUpdate(c, otherUser); CommentRange range3 = new CommentRange(3, 1, 4, 1); PatchLineComment comment3 = newPublishedPatchLineComment(psId, "file2", - uuid, range3, range3.getEndLine(), otherUser, null, time3, message3, + uuid3, range3, range3.getEndLine(), otherUser, null, time3, message3, (short) 1, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); update.upsertComment(comment3); @@ -948,14 +950,14 @@ public class ChangeNotesTest { + "1:1-2:1\n" + CommentsInNotesUtil.formatTime(serverIdent, time1) + "\n" + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid\n" + + "UUID: uuid1\n" + "Bytes: 9\n" + "comment 1\n" + "\n" + "2:1-3:1\n" + CommentsInNotesUtil.formatTime(serverIdent, time2) + "\n" + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid\n" + + "UUID: uuid2\n" + "Bytes: 9\n" + "comment 2\n" + "\n" @@ -964,7 +966,7 @@ public class ChangeNotesTest { + "3:1-4:1\n" + CommentsInNotesUtil.formatTime(serverIdent, time3) + "\n" + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid\n" + + "UUID: uuid3\n" + "Bytes: 9\n" + "comment 3\n" + "\n", @@ -975,7 +977,8 @@ public class ChangeNotesTest { public void patchLineCommentNotesFormatSide0() throws Exception { Change c = newChange(); ChangeUpdate update = newUpdate(c, otherUser); - String uuid = "uuid"; + String uuid1 = "uuid1"; + String uuid2 = "uuid2"; String message1 = "comment 1"; String message2 = "comment 2"; CommentRange range1 = new CommentRange(1, 1, 2, 1); @@ -984,7 +987,7 @@ public class ChangeNotesTest { PatchSet.Id psId = c.currentPatchSetId(); PatchLineComment comment1 = newPublishedPatchLineComment(psId, "file1", - uuid, range1, range1.getEndLine(), otherUser, null, time1, message1, + uuid1, range1, range1.getEndLine(), otherUser, null, time1, message1, (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); update.upsertComment(comment1); @@ -993,7 +996,7 @@ public class ChangeNotesTest { update = newUpdate(c, otherUser); CommentRange range2 = new CommentRange(2, 1, 3, 1); PatchLineComment comment2 = newPublishedPatchLineComment(psId, "file1", - uuid, range2, range2.getEndLine(), otherUser, null, time2, message2, + uuid2, range2, range2.getEndLine(), otherUser, null, time2, message2, (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); update.upsertComment(comment2); @@ -1017,14 +1020,14 @@ public class ChangeNotesTest { + "1:1-2:1\n" + CommentsInNotesUtil.formatTime(serverIdent, time1) + "\n" + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid\n" + + "UUID: uuid1\n" + "Bytes: 9\n" + "comment 1\n" + "\n" + "2:1-3:1\n" + CommentsInNotesUtil.formatTime(serverIdent, time2) + "\n" + "Author: Other Account <2@gerrit>\n" - + "UUID: uuid\n" + + "UUID: uuid2\n" + "Bytes: 9\n" + "comment 2\n" + "\n", @@ -1037,7 +1040,8 @@ public class ChangeNotesTest { throws Exception { Change c = newChange(); ChangeUpdate update = newUpdate(c, otherUser); - String uuid = "uuid"; + String uuid1 = "uuid1"; + String uuid2 = "uuid2"; String messageForBase = "comment for base"; String messageForPS = "comment for ps"; CommentRange range = new CommentRange(1, 1, 2, 1); @@ -1045,7 +1049,7 @@ public class ChangeNotesTest { PatchSet.Id psId = c.currentPatchSetId(); PatchLineComment commentForBase = - newPublishedPatchLineComment(psId, "filename", uuid, + newPublishedPatchLineComment(psId, "filename", uuid1, range, range.getEndLine(), otherUser, null, now, messageForBase, (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); @@ -1054,7 +1058,7 @@ public class ChangeNotesTest { update = newUpdate(c, otherUser); PatchLineComment commentForPS = - newPublishedPatchLineComment(psId, "filename", uuid, + newPublishedPatchLineComment(psId, "filename", uuid2, range, range.getEndLine(), otherUser, null, now, messageForPS, (short) 1, "abcd4567abcd4567abcd4567abcd4567abcd4567"); update.setPatchSetId(psId); @@ -1078,7 +1082,8 @@ public class ChangeNotesTest { @Test public void patchLineCommentMultipleOnePatchsetOneFile() throws Exception { Change c = newChange(); - String uuid = "uuid"; + String uuid1 = "uuid1"; + String uuid2 = "uuid2"; CommentRange range = new CommentRange(1, 1, 2, 1); PatchSet.Id psId = c.currentPatchSetId(); String filename = "filename"; @@ -1088,7 +1093,7 @@ public class ChangeNotesTest { Timestamp timeForComment1 = TimeUtil.nowTs(); Timestamp timeForComment2 = TimeUtil.nowTs(); PatchLineComment comment1 = newPublishedPatchLineComment(psId, filename, - uuid, range, range.getEndLine(), otherUser, null, timeForComment1, + uuid1, range, range.getEndLine(), otherUser, null, timeForComment1, "comment 1", side, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); update.upsertComment(comment1); @@ -1096,7 +1101,7 @@ public class ChangeNotesTest { update = newUpdate(c, otherUser); PatchLineComment comment2 = newPublishedPatchLineComment(psId, filename, - uuid, range, range.getEndLine(), otherUser, null, timeForComment2, + uuid2, range, range.getEndLine(), otherUser, null, timeForComment2, "comment 2", side, "abcd1234abcd1234abcd1234abcd1234abcd1234"); update.setPatchSetId(psId); update.upsertComment(comment2);