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
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 0000000..9de5220
--- /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<String, List<CommentInfo>> 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<String, List<ReviewInput.CommentInput>>();
+ input.comments.put(comment.path, Lists.newArrayList(comment));
+ revision(r).review(input);
+ Map<String, List<CommentInfo>> 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<String, List<CommentInfo>> 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<String, List<CommentInfo>> 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<String, List<CommentInfo>> 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<Map<String, List<CommentInfo>>>() {}.getType();
+ return newGson().fromJson(r.getReader(), mapType);
+ }
+
+ private Map<String, List<CommentInfo>> 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<Map<String, List<CommentInfo>>>() {}.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 73cc83a..4a673cb 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 @@
// 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 80213f6..2ed37cb 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 @@
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 f917613..6aca1b5 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 @@
*/
@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<PatchLineComment> 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<PatchLineComment> byChange(ReviewDb db,
+ ChangeNotes notes) throws OrmException {
+ if (!migration.readComments()) {
+ return db.patchComments().byChange(notes.getChangeId()).toList();
+ }
+ notes.load();
+
+ List<PatchLineComment> comments = Lists.newArrayList();
+ comments.addAll(notes.getBaseComments().values());
+ comments.addAll(notes.getPatchSetComments().values());
+
+ Iterable<String> 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<PatchLineComment> byPatchSet(ReviewDb db,
+ ChangeNotes notes, PatchSet.Id psId) throws OrmException {
+ if (!migration.readComments()) {
+ return db.patchComments().byPatchSet(psId).toList();
+ }
+ List<PatchLineComment> comments = Lists.newArrayList();
+ comments.addAll(publishedByPatchSet(db, notes, psId));
+
+ Iterable<String> 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<PatchLineComment> 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<PatchLineComment> commentsOnFile = new ArrayList<PatchLineComment>();
+ List<PatchLineComment> 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<PatchLineComment> 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<PatchLineComment> commentsOnPs = new ArrayList<PatchLineComment>();
- commentsOnPs.addAll(notes.getPatchSetComments().get(psId));
- commentsOnPs.addAll(notes.getBaseComments().get(psId));
- return commentsOnPs;
+ List<PatchLineComment> comments = new ArrayList<PatchLineComment>();
+ 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<PatchLineComment> draftByPatchSetAuthor(ReviewDb db,
+ PatchSet.Id psId, Account.Id author, ChangeNotes notes)
+ throws OrmException {
+ if (!migration.readComments()) {
+ return db.patchComments().draftByPatchSetAuthor(psId, author).toList();
+ }
+
+ List<PatchLineComment> 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<PatchLineComment> 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<PatchLineComment> 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<PatchLineComment> draftByChangeAuthor(ReviewDb db,
+ ChangeNotes notes, Account.Id author)
+ throws OrmException {
+ if (!migration.readComments()) {
+ return db.patchComments().byChange(notes.getChangeId()).toList();
+ }
+ List<PatchLineComment> comments = Lists.newArrayList();
+ comments.addAll(notes.getDraftBaseComments(author).values());
+ comments.addAll(notes.getDraftPsComments(author).values());
+ return comments;
+ }
+
+ public List<PatchLineComment> draftByAuthor(ReviewDb db,
+ Account.Id author) throws OrmException {
+ if (!migration.readComments()) {
+ return db.patchComments().draftByAuthor(author).toList();
+ }
+
+ Set<String> refNames =
+ getRefNamesAllUsers(RefNames.REFS_DRAFT_COMMENTS);
+
+ List<PatchLineComment> 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<PatchLineComment> comments) throws OrmException {
+ for (PatchLineComment c : comments) {
+ update.insertComment(c);
+ }
+ db.patchComments().insert(comments);
+ }
+
+ public void upsertComments(ReviewDb db, ChangeUpdate update,
Iterable<PatchLineComment> comments) throws OrmException {
for (PatchLineComment c : comments) {
update.upsertComment(c);
@@ -86,7 +239,23 @@
db.patchComments().upsert(comments);
}
- private static Collection<PatchLineComment> addCommentsInFile(
+ public void updateComments(ReviewDb db, ChangeUpdate update,
+ Iterable<PatchLineComment> comments) throws OrmException {
+ for (PatchLineComment c : comments) {
+ update.updateComment(c);
+ }
+ db.patchComments().update(comments);
+ }
+
+ public void deleteComments(ReviewDb db, ChangeUpdate update,
+ Iterable<PatchLineComment> comments) throws OrmException {
+ for (PatchLineComment c : comments) {
+ update.deleteComment(c);
+ }
+ db.patchComments().delete(comments);
+ }
+
+ private static Collection<PatchLineComment> addCommentsOnFile(
Collection<PatchLineComment> commentsOnFile,
Collection<PatchLineComment> allComments,
String file) {
@@ -98,4 +267,49 @@
}
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<String> 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<String> getDraftRefs(final Change.Id changeId)
+ throws OrmException {
+ Set<String> refNames = getRefNamesAllUsers(RefNames.REFS_DRAFT_COMMENTS);
+ final String suffix = "-" + changeId.get();
+ return Iterables.filter(refNames, new Predicate<String>() {
+ @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 586c294..fb80984 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.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 @@
private final Provider<WebLinks> webLinks;
private final EnumSet<ListChangesOption> options;
private final ChangeMessagesUtil cmUtil;
+ private final PatchLineCommentsUtil plcUtil;
private AccountInfo.Loader accountLoader;
@@ -147,7 +149,8 @@
DynamicMap<RestView<ChangeResource>> changeViews,
Revisions revisions,
Provider<WebLinks> webLinks,
- ChangeMessagesUtil cmUtil) {
+ ChangeMessagesUtil cmUtil,
+ PatchLineCommentsUtil plcUtil) {
this.db = db;
this.labelNormalizer = ln;
this.userProvider = user;
@@ -163,6 +166,7 @@
this.revisions = revisions;
this.webLinks = webLinks;
this.cmUtil = cmUtil;
+ this.plcUtil = plcUtil;
options = EnumSet.noneOf(ListChangesOption.class);
}
@@ -821,9 +825,8 @@
&& 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 1d2fa406..b025e65 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.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<RevisionResource, Input> {
private final Provider<ReviewDb> db;
+ private final ChangeUpdate.Factory updateFactory;
+ private final PatchLineCommentsUtil plcUtil;
+ private final PatchListCache patchListCache;
@Inject
- CreateDraft(Provider<ReviewDb> db) {
+ CreateDraft(Provider<ReviewDb> db,
+ ChangeUpdate.Factory updateFactory,
+ PatchLineCommentsUtil plcUtil,
+ PatchListCache patchListCache) {
this.db = db;
+ this.updateFactory = updateFactory;
+ this.plcUtil = plcUtil;
+ this.patchListCache = patchListCache;
}
@Override
public Response<CommentInfo> 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 @@
? 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 46ae834..f7fb300 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 @@
}
private final Provider<ReviewDb> db;
+ private final PatchLineCommentsUtil plcUtil;
+ private final ChangeUpdate.Factory updateFactory;
+ private final PatchListCache patchListCache;
@Inject
- DeleteDraft(Provider<ReviewDb> db) {
+ DeleteDraft(Provider<ReviewDb> db,
+ PatchLineCommentsUtil plcUtil,
+ ChangeUpdate.Factory updateFactory,
+ PatchListCache patchListCache) {
this.db = db;
+ this.plcUtil = plcUtil;
+ this.updateFactory = updateFactory;
+ this.patchListCache = patchListCache;
}
@Override
public Response<CommentInfo> 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 322faea..b0ed7d0 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.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 @@
private final Provider<CurrentUser> user;
private final ListDrafts list;
private final Provider<ReviewDb> dbProvider;
+ private final PatchLineCommentsUtil plcUtil;
@Inject
Drafts(DynamicMap<RestView<DraftResource>> views,
Provider<CurrentUser> user,
ListDrafts list,
- Provider<ReviewDb> dbProvider) {
+ Provider<ReviewDb> dbProvider,
+ PatchLineCommentsUtil plcUtil) {
this.views = views;
this.user = user;
this.list = list;
this.dbProvider = dbProvider;
+ this.plcUtil = plcUtil;
}
@Override
@@ -62,10 +66,8 @@
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 f4d7b49..146ded8 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 @@
@Singleton
class ListComments extends ListDrafts {
- private final PatchLineCommentsUtil plcUtil;
-
@Inject
ListComments(Provider<ReviewDb> 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 bd3aa04..1a26898 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.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 @@
@Singleton
class ListDrafts implements RestReadView<RevisionResource> {
protected final Provider<ReviewDb> db;
+ protected final PatchLineCommentsUtil plcUtil;
private final AccountInfo.Loader.Factory accountLoaderFactory;
@Inject
- ListDrafts(Provider<ReviewDb> db, AccountInfo.Loader.Factory alf) {
+ ListDrafts(Provider<ReviewDb> db, AccountInfo.Loader.Factory alf,
+ PatchLineCommentsUtil plcUtil) {
this.db = db;
this.accountLoaderFactory = alf;
+ this.plcUtil = plcUtil;
}
protected Iterable<PatchLineComment> 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 9a2a81e..bf8474f 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.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.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.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 @@
List<PatchLineComment> del = Lists.newArrayList();
List<PatchLineComment> ups = Lists.newArrayList();
- PatchList patchList = null;
- try {
- patchList = patchListCache.get(rsrc.getChange(), rsrc.getPatchSet());
- } catch (PatchListNotAvailableException e) {
- throw new OrmException("could not load PatchList for this patchset", e);
- }
- RevId patchSetCommit = new RevId(ObjectId.toString(patchList.getNewId()));
- RevId baseCommit = new RevId(ObjectId.toString(patchList.getOldId()));
-
for (Map.Entry<String, List<CommentInput>> ent : in.entrySet()) {
String path = ent.getKey();
for (CommentInput c : ent.getValue()) {
@@ -374,7 +362,8 @@
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 @@
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 @@
private Map<String, PatchLineComment> scanDraftComments(
RevisionResource rsrc) throws OrmException {
Map<String, PatchLineComment> 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 c1fb304..a3b0614 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.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 @@
private final Provider<ReviewDb> db;
private final DeleteDraft delete;
+ private final PatchLineCommentsUtil plcUtil;
+ private final ChangeUpdate.Factory updateFactory;
+ private final PatchListCache patchListCache;
@Inject
- PutDraft(Provider<ReviewDb> db, DeleteDraft delete) {
+ PutDraft(Provider<ReviewDb> 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<CommentInfo> 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 @@
&& !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 @@
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 2beb49f..5f2ffcb 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.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 @@
private final NotifyHandling notify;
private List<PatchLineComment> 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<PatchLineComment> plc)
@@ -232,17 +237,19 @@
private void appendQuotedParent(StringBuilder out, PatchLineComment child) {
if (child.getParentUuid() != null) {
- PatchLineComment parent;
+ Optional<PatchLineComment> 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 7510412..ca32c14 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 @@
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 @@
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 @@
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 @@
Table<PatchSet.Id, String, PatchLineComment> 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 @@
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 24d4b46..770315a 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 @@
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 @@
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 @@
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 @@
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 36f685d..679e272 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 @@
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 @@
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 @@
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 b4337cf..decae6d 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 @@
private void loadDrafts(final Map<Patch.Key, Patch> 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 8ce6fa3..37d96ea 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 @@
new InvalidProvider<ReviewDb>(), //
new InvalidProvider<ChangeQueryRewriter>(), //
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<ChangeData, BasicChangeRewrites> mydef =
new QueryRewriter.Definition<ChangeData, BasicChangeRewrites>(
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 ed64015..5eb9655 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.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 @@
*/
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 @@
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 @@
ChangeNotes.Factory notesFactory,
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil cmUtil,
+ PatchLineCommentsUtil plcUtil,
PatchListCache patchListCache,
NotesMigration notesMigration,
@Assisted ReviewDb db,
@@ -205,6 +208,7 @@
this.notesFactory = notesFactory;
this.approvalsUtil = approvalsUtil;
this.cmUtil = cmUtil;
+ this.plcUtil = plcUtil;
this.patchListCache = patchListCache;
this.notesMigration = notesMigration;
legacyId = id;
@@ -218,6 +222,7 @@
ChangeNotes.Factory notesFactory,
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil cmUtil,
+ PatchLineCommentsUtil plcUtil,
PatchListCache patchListCache,
NotesMigration notesMigration,
@Assisted ReviewDb db,
@@ -229,6 +234,7 @@
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 @@
ChangeNotes.Factory notesFactory,
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil cmUtil,
+ PatchLineCommentsUtil plcUtil,
PatchListCache patchListCache,
NotesMigration notesMigration,
@Assisted ReviewDb db,
@@ -254,6 +261,7 @@
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 Collection<PatchLineComment> 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 ac7b9ae..fabe61c 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.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 @@
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 @@
CapabilityControl.Factory capabilityControlFactory,
ChangeControl.GenericFactory changeControlGenericFactory,
ChangeData.Factory changeDataFactory,
+ PatchLineCommentsUtil plcUtil,
AccountResolver accountResolver,
GroupBackend groupBackend,
AllProjectsName allProjectsName,
@@ -187,6 +190,7 @@
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 53d2bbd..ebb7389 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 @@
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 @@
@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<ChangeData> read() throws OrmException {
Set<Change.Id> 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 da687be..9e4d2b2 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.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.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.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 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 @@
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<RestView<CommentResource>> views =
+ final DynamicMap<RestView<CommentResource>> commentViews =
createMock(DynamicMap.class);
- final TypeLiteral<DynamicMap<RestView<CommentResource>>> viewsType =
+ final TypeLiteral<DynamicMap<RestView<CommentResource>>> commentViewsType =
new TypeLiteral<DynamicMap<RestView<CommentResource>>>() {};
+ @SuppressWarnings("unchecked")
+ final DynamicMap<RestView<DraftResource>> draftViews =
+ createMock(DynamicMap.class);
+ final TypeLiteral<DynamicMap<RestView<DraftResource>>> draftViewsType =
+ new TypeLiteral<DynamicMap<RestView<DraftResource>>>() {};
+
final AccountInfo.Loader.Factory alf =
createMock(AccountInfo.Loader.Factory.class);
final ReviewDb db = createMock(ReviewDb.class);
@@ -139,10 +149,23 @@
@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.<ReviewDb>of(db));
bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(config);
@@ -162,28 +185,16 @@
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 @@
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 @@
"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<PatchLineComment> commentsByOwner = Lists.newArrayList();
commentsByOwner.add(plc1);
commentsByOwner.add(plc3);
List<PatchLineComment> commentsByReviewer = Lists.newArrayList();
commentsByReviewer.add(plc2);
+ List<PatchLineComment> 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 @@
assertGetComment(injector, revRes1, null, "BadComment");
}
+ @Test
+ public void testListDrafts() throws Exception {
+ // test ListDrafts for patch set 1
+ assertListDrafts(injector, revRes1,
+ Collections.<String, ArrayList<PatchLineComment>> emptyMap());
+
+ // test ListDrafts for patch set 2
+ assertListDrafts(injector, revRes2, ImmutableMap.of(
+ "FileOne.txt", Lists.newArrayList(plc4, plc5)));
+ }
+
private static IAnswer<ResultSet<PatchLineComment>> results(
final PatchLineComment... comments) {
return new IAnswer<ResultSet<PatchLineComment>>() {
@@ -305,7 +353,7 @@
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 @@
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<String, ArrayList<PatchLineComment>> expected) throws Exception {
+ Drafts drafts = inj.getInstance(Drafts.class);
+ RestReadView<RevisionResource> listView =
+ (RestReadView<RevisionResource>) drafts.list();
+ @SuppressWarnings("unchecked")
+ Map<String, List<CommentInfo>> actual =
+ (Map<String, List<CommentInfo>>) listView.apply(res);
+ assertNotNull(actual);
+ assertEquals(expected.size(), actual.size());
+ assertEquals(expected.keySet(), actual.keySet());
+ for (Map.Entry<String, ArrayList<PatchLineComment>> entry : expected.entrySet()) {
+ List<PatchLineComment> expectedComments = entry.getValue();
+ List<CommentInfo> 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 @@
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 @@
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 50e5764..7090f0d 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 @@
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 81d2503..a089ea6 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 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 @@
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 @@
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 @@
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 @@
+ "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 @@
+ "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 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 @@
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 @@
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 @@
+ "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 @@
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 @@
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 @@
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 @@
@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 @@
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 @@
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);
