Merge changes Ibe956014,I4222b894,Iac4ea144,I803717e8,I39e97dcd, ... * changes: Support updating multiple VersionedMetaDatas in a BatchRefUpdate Refactor byChange() into two methods: drafts and published Add draft comments to PatchLineCommentsUtil Fix bug for comments with no range Resolve issue with naming of drafts ref Add method to parse an AccountId out of a RefName Improve PatchSet.Id.fromRef performance and avoid double-parsing
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-reviewdb/BUCK b/gerrit-reviewdb/BUCK index faf80a8..9b1991b 100644 --- a/gerrit-reviewdb/BUCK +++ b/gerrit-reviewdb/BUCK
@@ -1,4 +1,5 @@ SRC = 'src/main/java/com/google/gerrit/reviewdb/' +TESTS = 'src/test/java/com/google/gerrit/reviewdb/' gwt_module( name = 'client', @@ -22,3 +23,15 @@ ], visibility = ['PUBLIC'], ) + +java_test( + name = 'client_tests', + srcs = glob([TESTS + 'client/**/*.java']), + deps = [ + ':client', + '//lib:gwtorm', + '//lib:junit', + ], + source_under_test = [':client'], + visibility = ['//tools/eclipse:classpath'], +)
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java index e131f7a..3d156f9 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java
@@ -104,6 +104,55 @@ r.fromString(str); return r; } + + /** + * Parse an Account.Id out of a part of a ref-name. + * + * @param name a ref name with the following syntax: {@code "34/1234..."}. + * We assume that the caller has trimmed any prefix. + */ + public static Id fromRefPart(String name) { + if (name == null) { + return null; + } + + String[] parts = name.split("/"); + int n = parts.length; + if (n < 2) { + return null; + } + + // Last 2 digits. + int le; + for (le = 0; le < parts[0].length(); le++) { + if (!Character.isDigit(parts[0].charAt(le))) { + return null; + } + } + if (le != 2) { + return null; + } + + // Full ID. + int ie; + for (ie = 0; ie < parts[1].length(); ie++) { + if (!Character.isDigit(parts[1].charAt(ie))) { + if (ie == 0) { + return null; + } else { + break; + } + } + } + + int shard = Integer.parseInt(parts[0]); + int id = Integer.parseInt(parts[1].substring(0, ie)); + + if (id % 100 != shard) { + return null; + } + return new Account.Id(id); + } } @Column(id = 1)
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java index 613978a..ed4d999 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSet.java
@@ -24,28 +24,8 @@ /** A single revision of a {@link Change}. */ public final class PatchSet { /** Is the reference name a change reference? */ - public static boolean isRef(final String name) { - if (name == null || !name.startsWith(REFS_CHANGES)) { - return false; - } - boolean accepted = false; - int numsFound = 0; - for (int i = name.length() - 1; i >= REFS_CHANGES.length() - 1; i--) { - char c = name.charAt(i); - if (c >= '0' && c <= '9') { - accepted = (c != '0'); - } else if (c == '/') { - if (accepted) { - if (++numsFound == 2) { - return true; - } - accepted = false; - } - } else { - return false; - } - } - return false; + public static boolean isRef(String name) { + return Id.fromRef(name) != null; } public static class Id extends IntKey<Change.Id> { @@ -106,16 +86,63 @@ /** Parse a PatchSet.Id from a {@link PatchSet#getRefName()} result. */ public static Id fromRef(String name) { - if (!name.startsWith(REFS_CHANGES)) { - throw new IllegalArgumentException("Not a PatchSet.Id: " + name); + if (name == null || !name.startsWith(REFS_CHANGES)) { + return null; } - final String[] parts = name.substring(REFS_CHANGES.length()).split("/"); - final int n = parts.length; - if (n < 2) { - throw new IllegalArgumentException("Not a PatchSet.Id: " + name); + + // Last 2 digits. + int ls = REFS_CHANGES.length(); + int le; + for (le = ls; le < name.length() && name.charAt(le) != '/'; le++) { + if (name.charAt(le) < '0' || name.charAt(le) > '9') { + return null; + } } - final int changeId = Integer.parseInt(parts[n - 2]); - final int patchSetId = Integer.parseInt(parts[n - 1]); + if (le - ls != 2) { + return null; + } + + // Change ID. + int cs = le + 1; + if (cs >= name.length() || name.charAt(cs) == '0') { + return null; + } + int ce; + for (ce = cs; ce < name.length() && name.charAt(ce) != '/'; ce++) { + if (name.charAt(ce) < '0' || name.charAt(ce) > '9') { + return null; + } + } + switch (ce - cs) { + case 0: + return null; + case 1: + if (name.charAt(ls) != '0' + || name.charAt(ls + 1) != name.charAt(cs)) { + return null; + } + break; + default: + if (name.charAt(ls) != name.charAt(ce - 2) + || name.charAt(ls + 1) != name.charAt(ce - 1)) { + return null; + } + break; + } + + // Patch set ID. + int ps = ce + 1; + if (ps >= name.length() || name.charAt(ps) == '0') { + return null; + } + for (int i = ps; i < name.length(); i++) { + if (name.charAt(i) < '0' || name.charAt(i) > '9') { + return null; + } + } + + int changeId = Integer.parseInt(name.substring(cs, ce)); + int patchSetId = Integer.parseInt(name.substring(ps)); return new PatchSet.Id(new Change.Id(changeId), patchSetId); } }
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java index 06e1803..390295f 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
@@ -31,6 +31,8 @@ /** Configurations of project-specific dashboards (canned search queries). */ public static final String REFS_DASHBOARDS = "refs/meta/dashboards/"; + public static final String REFS_DRAFT_COMMENTS = "refs/draft-comments/"; + /** * Prefix applied to merge commit base nodes. * <p> @@ -43,8 +45,6 @@ */ public static final String REFS_CACHE_AUTOMERGE = "refs/cache-automerge/"; - public static final String REFS_DRAFT_PREFIX = "comments-"; - public static String refsUsers(Account.Id accountId) { StringBuilder r = new StringBuilder(); r.append(REFS_USER); @@ -62,9 +62,15 @@ public static String refsDraftComments(Account.Id accountId, Change.Id changeId) { StringBuilder r = new StringBuilder(); - r.append(refsUsers(accountId)); + r.append(REFS_DRAFT_COMMENTS); + int n = accountId.get() % 100; + if (n < 10) { + r.append('0'); + } + r.append(n); r.append('/'); - r.append(REFS_DRAFT_PREFIX); + r.append(accountId.get()); + r.append('-'); r.append(changeId.get()); return r.toString(); }
diff --git a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/AccountTest.java b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/AccountTest.java new file mode 100644 index 0000000..3ffdb92 --- /dev/null +++ b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/AccountTest.java
@@ -0,0 +1,55 @@ +// 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.reviewdb.client; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import com.google.gerrit.reviewdb.client.Account; + +import org.junit.Test; + +public class AccountTest { + @Test + public void parseRefNameParts() { + assertRefPart(1, "01/1"); + assertRefPart(1, "01/1-drafts"); + assertRefPart(1, "01/1-drafts/2"); + + assertNotRefPart(null); + assertNotRefPart(""); + + // This method assumes that the common prefix "refs/users/" will be removed. + assertNotRefPart("refs/users/01/1"); + + // Invalid characters. + assertNotRefPart("01a/1"); + assertNotRefPart("01/a1"); + + // Mismatched shard. + assertNotRefPart("01/23"); + + // Shard too short. + assertNotRefPart("1/1"); + } + + private static void assertRefPart(int accountId, String refName) { + assertEquals(new Account.Id(accountId), Account.Id.fromRefPart(refName)); + } + + private static void assertNotRefPart(String refName) { + assertNull(Account.Id.fromRefPart(refName)); + } +}
diff --git a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/PatchSetTest.java b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/PatchSetTest.java new file mode 100644 index 0000000..525fb18 --- /dev/null +++ b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/PatchSetTest.java
@@ -0,0 +1,69 @@ +// 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.reviewdb.client; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +public class PatchSetTest { + @Test + public void parseRefNames() { + assertRef(1, 1, "refs/changes/01/1/1"); + assertRef(1234, 56, "refs/changes/34/1234/56"); + + // Not even close. + assertNotRef(null); + assertNotRef(""); + assertNotRef("01/1/1"); + assertNotRef("HEAD"); + assertNotRef("refs/tags/v1"); + + // Invalid characters. + assertNotRef("refs/changes/0x/1/1"); + assertNotRef("refs/changes/01/x/1"); + assertNotRef("refs/changes/01/1/x"); + + // Truncations. + assertNotRef("refs/changes/"); + assertNotRef("refs/changes/1"); + assertNotRef("refs/changes/01"); + assertNotRef("refs/changes/01/"); + assertNotRef("refs/changes/01/1/"); + assertNotRef("refs/changes/01/1/1/"); + assertNotRef("refs/changes/01//1/1"); + + // Leading zeroes. + assertNotRef("refs/changes/01/01/1"); + assertNotRef("refs/changes/01/1/01"); + + // Mismatched last 2 digits. + assertNotRef("refs/changes/35/1234/56"); + } + + private static void assertRef(int changeId, int psId, String refName) { + assertTrue(PatchSet.isRef(refName)); + assertEquals(new PatchSet.Id(new Change.Id(changeId), psId), + PatchSet.Id.fromRef(refName)); + } + + private static void assertNotRef(String refName) { + assertFalse(PatchSet.isRef(refName)); + assertNull(PatchSet.Id.fromRef(refName)); + } +}
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..d12d3cc 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,46 @@ // 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.PatchLineComment.Status; +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.gwtorm.server.ResultSet; 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 +61,208 @@ */ @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 : publishedByChange(db, notes)) { + if (key.equals(c.getKey())) { + return Optional.of(c); + } + } + for (PatchLineComment c : draftByChange(db, notes)) { + if (key.equals(c.getKey())) { + return Optional.of(c); + } + } + return Optional.absent(); + } + + public List<PatchLineComment> publishedByChange(ReviewDb db, + ChangeNotes notes) throws OrmException { + if (!migration.readComments()) { + return byCommentStatus(db.patchComments().byChange(notes.getChangeId()), + Status.PUBLISHED); + } + + notes.load(); + List<PatchLineComment> comments = Lists.newArrayList(); + comments.addAll(notes.getBaseComments().values()); + comments.addAll(notes.getPatchSetComments().values()); + return comments; + } + + public List<PatchLineComment> draftByChange(ReviewDb db, + ChangeNotes notes) throws OrmException { + if (!migration.readComments()) { + return byCommentStatus(db.patchComments().byChange(notes.getChangeId()), + Status.DRAFT); + } + + List<PatchLineComment> comments = Lists.newArrayList(); + 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; + } + + private static List<PatchLineComment> byCommentStatus( + ResultSet<PatchLineComment> comments, + final PatchLineComment.Status status) { + return Lists.newArrayList( + Iterables.filter(comments, new Predicate<PatchLineComment>() { + @Override + public boolean apply(PatchLineComment input) { + return (input.getStatus() == status); + } + }) + ); + } + + 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 +270,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 +298,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/git/MetaDataUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MetaDataUpdate.java index b48028e..b65cc85 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MetaDataUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MetaDataUpdate.java
@@ -14,6 +14,7 @@ package com.google.gerrit.server.git; +import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; @@ -21,8 +22,10 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; +import com.google.inject.assistedinject.AssistedInject; import org.eclipse.jgit.errors.RepositoryNotFoundException; +import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.RefUpdate; @@ -59,7 +62,24 @@ public MetaDataUpdate create(Project.NameKey name, IdentifiedUser user) throws RepositoryNotFoundException, IOException { - MetaDataUpdate md = factory.create(name, mgr.openRepository(name)); + return create(name, user, null); + } + + /** + * Create an update using an existing batch ref update. + * <p> + * This allows batching together updates to multiple metadata refs. For making + * multiple commits to a single metadata ref, see + * {@link VersionedMetaData#openUpdate(MetaDataUpdate)}. + * + * @param name project name. + * @param user user for the update. + * @param batch batch update to use; the caller is responsible for committing + * the update. + */ + public MetaDataUpdate create(Project.NameKey name, IdentifiedUser user, + BatchRefUpdate batch) throws RepositoryNotFoundException, IOException { + MetaDataUpdate md = factory.create(name, mgr.openRepository(name), batch); md.getCommitBuilder().setAuthor(createPersonIdent(user)); md.getCommitBuilder().setCommitter(serverIdent); return md; @@ -86,7 +106,13 @@ public MetaDataUpdate create(Project.NameKey name) throws RepositoryNotFoundException, IOException { - MetaDataUpdate md = factory.create(name, mgr.openRepository(name)); + return create(name, null); + } + + /** @see User#create(Project.NameKey, IdentifiedUser, BatchRefUpdate) */ + public MetaDataUpdate create(Project.NameKey name, BatchRefUpdate batch) + throws RepositoryNotFoundException, IOException { + MetaDataUpdate md = factory.create(name, mgr.openRepository(name), batch); md.getCommitBuilder().setAuthor(serverIdent); md.getCommitBuilder().setCommitter(serverIdent); return md; @@ -95,24 +121,32 @@ interface InternalFactory { MetaDataUpdate create(@Assisted Project.NameKey projectName, - @Assisted Repository db); + @Assisted Repository db, @Assisted @Nullable BatchRefUpdate batch); } private final GitReferenceUpdated gitRefUpdated; private final Project.NameKey projectName; private final Repository db; + private final BatchRefUpdate batch; private final CommitBuilder commit; private boolean allowEmpty; - @Inject + @AssistedInject public MetaDataUpdate(GitReferenceUpdated gitRefUpdated, - @Assisted Project.NameKey projectName, @Assisted Repository db) { + @Assisted Project.NameKey projectName, @Assisted Repository db, + @Assisted @Nullable BatchRefUpdate batch) { this.gitRefUpdated = gitRefUpdated; this.projectName = projectName; this.db = db; + this.batch = batch; this.commit = new CommitBuilder(); } + public MetaDataUpdate(GitReferenceUpdated gitRefUpdated, + Project.NameKey projectName, Repository db) { + this(gitRefUpdated, projectName, db, null); + } + /** Set the commit message used when committing the update. */ public void setMessage(String message) { getCommitBuilder().setMessage(message); @@ -128,6 +162,11 @@ this.allowEmpty = allowEmpty; } + /** @return batch in which to run the update, or {@code null} for no batch. */ + BatchRefUpdate getBatch() { + return batch; + } + /** Close the cached Repository handle. */ public void close() { getRepository().close();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index b12750d..0a91f2e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -2131,8 +2131,11 @@ allRefs.size() / estRefsPerChange, estRefsPerChange); for (Ref ref : allRefs.values()) { - if (ref.getObjectId() != null && PatchSet.isRef(ref.getName())) { - refsByChange.put(Change.Id.fromRef(ref.getName()), ref); + if (ref.getObjectId() != null) { + PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName()); + if (psId != null) { + refsByChange.put(psId.getParentKey(), ref); + } } } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java index 4c5ebc6..a58d9b6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VersionedMetaData.java
@@ -26,6 +26,7 @@ import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.lib.AnyObjectId; +import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; @@ -40,6 +41,7 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevTree; import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.ReceiveCommand; import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.util.RawParseUtils; @@ -181,6 +183,21 @@ void close(); } + /** + * Open a batch of updates to the same metadata ref. + * <p> + * This allows making multiple commits to a single metadata ref, at the end of + * which is a single ref update. For batching together updates to multiple + * refs (each consisting of one or more commits against their respective + * refs), create the {@link MetaDataUpdate} with a {@link BatchRefUpdate}. + * <p> + * A ref update produced by this {@link BatchMetaDataUpdate} is only committed + * if there is no associated {@link BatchRefUpdate}. As a result, the + * configured ref updated event is not fired if there is an associated batch. + * + * @param update helper info about the update. + * @throws IOException if the update failed. + */ public BatchMetaDataUpdate openUpdate(final MetaDataUpdate update) throws IOException { final Repository db = update.getRepository(); @@ -302,6 +319,15 @@ private RevCommit updateRef(AnyObjectId oldId, AnyObjectId newId, String refName) throws IOException { + BatchRefUpdate bru = update.getBatch(); + if (bru != null) { + bru.addCommand(new ReceiveCommand( + oldId.toObjectId(), newId.toObjectId(), refName)); + inserter.flush(); + revision = rw.parseCommit(newId); + return revision; + } + RefUpdate ru = db.updateRef(refName); ru.setExpectedOldObjectId(oldId); ru.setNewObjectId(src);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java index 3b1a8fb..30db2b9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
@@ -82,12 +82,13 @@ final List<Ref> deferredTags = new ArrayList<>(); for (Ref ref : refs.values()) { + PatchSet.Id psId; if (ref.getName().startsWith(RefNames.REFS_CACHE_AUTOMERGE)) { continue; - } else if (PatchSet.isRef(ref.getName())) { + } else if ((psId = PatchSet.Id.fromRef(ref.getName())) != null) { // Reference to a patch set is visible if the change is visible. // - if (showChanges && visibleChanges.contains(Change.Id.fromRef(ref.getName()))) { + if (showChanges && visibleChanges.contains(psId.getParentKey())) { result.put(ref.getName(), ref); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java index 41dfba5..68e7857 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
@@ -408,7 +408,7 @@ public Iterable<String> get(ChangeData input, FillArgs args) throws OrmException { Set<String> r = Sets.newHashSet(); - for (PatchLineComment c : input.comments()) { + for (PatchLineComment c : input.publishedComments()) { r.add(c.getMessage()); } for (ChangeMessage m : input.messages()) {
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/AbstractChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java index e1641b8..f114af0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
@@ -29,6 +29,7 @@ import com.google.gerrit.server.project.ChangeControl; import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; @@ -105,10 +106,15 @@ } public BatchMetaDataUpdate openUpdate() throws IOException { + return openUpdateInBatch(null); + } + + public BatchMetaDataUpdate openUpdateInBatch(BatchRefUpdate bru) + throws IOException { if (migration.write()) { load(); MetaDataUpdate md = - updateFactory.create(getProjectName(), getUser()); + updateFactory.create(getProjectName(), getUser(), bru); md.setAllowEmpty(true); return super.openUpdate(md); }
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/CommentsInNotesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java index 99f9542..c2e5dfc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java
@@ -239,6 +239,7 @@ if (note[ptr.value] == '\n') { range.setEndLine(startLine); + ptr.value += 1; return range; } else if (note[ptr.value] == ':') { range.setStartLine(startLine);
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..0310824 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; @@ -179,7 +181,7 @@ private ListMultimap<PatchSet.Id, PatchSetApproval> allApprovals; private List<PatchSetApproval> currentApprovals; private Map<Integer, List<String>> files = new HashMap<>(); - private Collection<PatchLineComment> comments; + private Collection<PatchLineComment> publishedComments; private CurrentUser visibleTo; private ChangeControl changeControl; private List<ChangeMessage> messages; @@ -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(); @@ -519,12 +527,12 @@ return approvalsUtil.getReviewers(notes(), approvals().values()); } - public Collection<PatchLineComment> comments() + public Collection<PatchLineComment> publishedComments() throws OrmException { - if (comments == null) { - comments = db.patchComments().byChange(legacyId).toList(); + if (publishedComments == null) { + publishedComments = plcUtil.publishedByChange(db, notes()); } - return comments; + return publishedComments; } public List<ChangeMessage> messages()
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/main/java/com/google/gerrit/server/query/change/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java index 8889f53..f6a6108 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java
@@ -366,7 +366,7 @@ eventFactory.addComments(c, d.messages()); if (includePatchSets) { for (PatchSetAttribute attribute : c.patchSets) { - eventFactory.addPatchSetComments(attribute, d.comments()); + eventFactory.addPatchSetComments(attribute, d.publishedComments()); } } }
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..a8880a1 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
@@ -21,6 +21,7 @@ import static org.easymock.EasyMock.replay; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import com.google.common.base.Objects; @@ -43,6 +44,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 +62,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 +75,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 +107,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 +121,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 +150,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 +186,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 +211,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(); @@ -208,7 +224,7 @@ PatchSet.Id psId2 = new PatchSet.Id(change.getId(), 2); PatchSet ps2 = new PatchSet(psId2); - long timeBase = TimeUtil.nowMs(); + long timeBase = TimeUtil.roundToSecond(TimeUtil.nowTs()).getTime(); plc1 = newPatchLineComment(psId1, "Comment1", null, "FileOne.txt", Side.REVISION, 3, ownerId, timeBase, "First Comment", new CommentRange(1, 2, 3, 4)); @@ -221,32 +237,56 @@ "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(); + expect(plca.byChange(change.getId())) + .andAnswer(results(plc1, plc2, plc3, 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 +326,37 @@ 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))); + } + + @Test + public void testPatchLineCommentsUtilByCommentStatus() throws OrmException { + List<PatchLineComment> publishedActual = plcUtil.publishedByChange( + injector.getInstance(ReviewDb.class), revRes2.getNotes()); + List<PatchLineComment> draftActual = plcUtil.draftByChange( + injector.getInstance(ReviewDb.class), revRes2.getNotes()); + List<PatchLineComment> publishedExpected = + Lists.newArrayList(plc1, plc2, plc3); + List<PatchLineComment> draftExpected = + Lists.newArrayList(plc4, plc5); + assertEquals(publishedExpected.size(), publishedActual.size()); + assertEquals(draftExpected.size(), draftActual.size()); + for (PatchLineComment c : draftExpected) { + assertTrue(draftActual.contains(c)); + } + for (PatchLineComment c : publishedExpected) { + assertTrue(publishedActual.contains(c)); + } + } + private static IAnswer<ResultSet<PatchLineComment>> results( final PatchLineComment... comments) { return new IAnswer<ResultSet<PatchLineComment>>() { @@ -305,7 +376,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 +401,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 +447,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 +456,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 6db2557..72e3838 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
@@ -62,13 +62,12 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.VersionedMetaData.BatchMetaDataUpdate; import com.google.gerrit.server.group.SystemGroupBackend; -import com.google.gerrit.server.notedb.CommentsInNotesUtil; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.util.TimeUtil; -import com.google.gerrit.testutil.TestChanges; import com.google.gerrit.testutil.FakeAccountCache; import com.google.gerrit.testutil.FakeRealm; import com.google.gerrit.testutil.InMemoryRepositoryManager; +import com.google.gerrit.testutil.TestChanges; import com.google.gwtorm.client.KeyUtil; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.StandardKeyEncoder; @@ -77,13 +76,16 @@ import com.google.inject.util.Providers; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; +import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.notes.Note; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.ReceiveCommand; import org.joda.time.DateTime; import org.joda.time.DateTimeUtils; import org.joda.time.DateTimeUtils.MillisProvider; @@ -695,6 +697,58 @@ } @Test + public void multipleUpdatesAcrossRefs() throws Exception { + Change c1 = newChange(); + ChangeUpdate update1 = newUpdate(c1, changeOwner); + update1.putApproval("Verified", (short) 1); + + Change c2 = newChange(); + ChangeUpdate update2 = newUpdate(c2, otherUser); + update2.putApproval("Code-Review", (short) 2); + + BatchMetaDataUpdate batch1 = null; + BatchMetaDataUpdate batch2 = null; + + BatchRefUpdate bru = repo.getRefDatabase().newBatchUpdate(); + try { + batch1 = update1.openUpdateInBatch(bru); + batch1.write(update1, new CommitBuilder()); + batch1.commit(); + assertNull(repo.getRef(update1.getRefName())); + + batch2 = update2.openUpdateInBatch(bru); + batch2.write(update2, new CommitBuilder()); + batch2.commit(); + assertNull(repo.getRef(update2.getRefName())); + } finally { + if (batch1 != null) { + batch1.close(); + } + if (batch2 != null) { + batch2.close(); + } + } + + List<ReceiveCommand> cmds = bru.getCommands(); + assertEquals(2, cmds.size()); + assertEquals(update1.getRefName(), cmds.get(0).getRefName()); + assertEquals(update2.getRefName(), cmds.get(1).getRefName()); + + RevWalk rw = new RevWalk(repo); + try { + bru.execute(rw, NullProgressMonitor.INSTANCE); + } finally { + rw.release(); + } + + assertEquals(ReceiveCommand.Result.OK, cmds.get(0).getResult()); + assertEquals(ReceiveCommand.Result.OK, cmds.get(1).getResult()); + + assertNotNull(repo.getRef(update1.getRefName())); + assertNotNull(repo.getRef(update2.getRefName())); + } + + @Test public void changeMessageOnePatchSet() throws Exception { Change c = newChange(); ChangeUpdate update = newUpdate(c, changeOwner); @@ -895,7 +949,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 +962,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 +971,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 +980,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 +1004,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 +1020,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 +1031,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 +1041,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 +1050,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 +1074,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 +1094,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 +1103,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 +1112,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 +1136,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 +1147,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 +1155,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); @@ -1370,6 +1429,34 @@ assertTrue(notes.getDraftPsComments(otherUserId).values().isEmpty()); } + @Test + public void patchLineCommentNoRange() throws Exception { + Change c = newChange(); + ChangeUpdate update = newUpdate(c, otherUser); + String uuid = "uuid"; + String messageForBase = "comment for base"; + Timestamp now = TimeUtil.nowTs(); + PatchSet.Id psId = c.currentPatchSetId(); + + PatchLineComment commentForBase = + newPublishedPatchLineComment(psId, "filename", uuid, + null, 1, otherUser, null, now, messageForBase, + (short) 0, "abcd1234abcd1234abcd1234abcd1234abcd1234"); + update.setPatchSetId(psId); + update.upsertComment(commentForBase); + update.commit(); + + ChangeNotes notes = newNotes(c); + Multimap<PatchSet.Id, PatchLineComment> commentsForBase = + notes.getBaseComments(); + Multimap<PatchSet.Id, PatchLineComment> commentsForPs = + notes.getPatchSetComments(); + + assertTrue(commentsForPs.isEmpty()); + assertEquals(commentForBase, + Iterables.getOnlyElement(commentsForBase.get(psId))); + } + private Change newChange() { return TestChanges.newChange(project, changeOwner); }
diff --git a/tools/eclipse/BUCK b/tools/eclipse/BUCK index 4e76b029..2cc66a0 100644 --- a/tools/eclipse/BUCK +++ b/tools/eclipse/BUCK
@@ -11,6 +11,7 @@ '//gerrit-main:main_lib', '//gerrit-patch-jgit:jgit_patch_tests', '//gerrit-plugin-gwtui:gwtui-api-lib', + '//gerrit-reviewdb:client_tests', '//gerrit-server:server', '//gerrit-server:server_tests', '//lib/asciidoctor:asciidoc_lib',