From 692b4ec251af8e6b0b605c28f4b24378acf8c9c3 Mon Sep 17 00:00:00 2001 From: Bill Wendling Date: Mon, 19 Oct 2015 15:40:57 -0700 Subject: [PATCH] Add option to omit duplicate comments. Change-Id: I6afc938ad72afbcfd5cb653b88f7e65431d2018c --- Documentation/rest-api-changes.txt | 18 +++--- .../acceptance/server/change/CommentsIT.java | 32 ++++++++++- .../extensions/api/changes/ReviewInput.java | 6 ++ .../google/gerrit/extensions/client/Side.java | 15 ++++- .../reviewdb/client/PatchLineComment.java | 2 +- .../gerrit/server/change/PostReview.java | 57 ++++++++++++++++++- 6 files changed, 114 insertions(+), 16 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 7e9655678b..55764b1fa2 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -4482,23 +4482,23 @@ revision. [options="header",cols="1,^1,5"] |============================ -|Field Name ||Description -|`message` |optional| +|Field Name ||Description +|`message` |optional| The message to be added as review comment. -|`labels` |optional| +|`labels` |optional| The votes that should be added to the revision as a map that maps the label names to the voting values. -|`comments` |optional| +|`comments` |optional| The comments that should be added as a map that maps a file path to a list of link:#comment-input[CommentInput] entities. -|`strict_labels`|`true` if not set| +|`strict_labels` |`true` if not set| Whether all labels are required to be within the user's permitted ranges based on access controls. + If `true`, attempting to use a label not granted to the user will fail the entire modify operation early. + If `false`, the operation will execute anyway, but the proposed labels will be modified to be the "best" value allowed by the access controls. -|`drafts` |optional| +|`drafts` |optional| Draft handling that defines how draft comments are handled that are already in the database but that were not also described in this input. + @@ -4506,12 +4506,14 @@ Allowed values are `DELETE`, `PUBLISH`, `PUBLISH_ALL_REVISIONS` and `KEEP`. All values except `PUBLISH_ALL_REVISIONS` operate only on drafts for a single revision. + If not set, the default is `DELETE`. -|`notify` |optional| +|`notify` |optional| Notify handling that defines to whom email notifications should be sent after the review is stored. + Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. + If not set, the default is `ALL`. -|`on_behalf_of`|optional| +|`omit_duplicate_comments`|optional| +If `true`, comments with the same content at the same place will be omitted. +|`on_behalf_of` |optional| link:rest-api-accounts.html#account-id[\{account-id\}] the review should be posted on behalf of. To use this option the caller must have been granted `labelAs-NAME` permission for all keys of labels. 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 index 5592755dc6..b784f05b61 100644 --- 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 @@ -188,12 +188,35 @@ public class CommentsIT extends AbstractDaemonTest { } } + @Test + public void addDuplicateComments() throws Exception { + PushOneCommit.Result r1 = createChange(); + String changeId = r1.getChangeId(); + String revId = r1.getCommit().getName(); + addComment(r1, "nit: trailing whitespace"); + addComment(r1, "nit: trailing whitespace"); + Map> result = getPublishedComments(changeId, revId); + assertThat(result.get(FILE_NAME)).hasSize(2); + addComment(r1, "nit: trailing whitespace", true); + result = getPublishedComments(changeId, revId); + assertThat(result.get(FILE_NAME)).hasSize(2); + + PushOneCommit.Result r2 = pushFactory.create( + db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "content") + .to("refs/for/master"); + changeId = r2.getChangeId(); + revId = r2.getCommit().getName(); + addComment(r2, "nit: trailing whitespace", true); + result = getPublishedComments(changeId, revId); + assertThat(result.get(FILE_NAME)).hasSize(1); + } + @Test public void listChangeDrafts() throws Exception { PushOneCommit.Result r1 = createChange(); PushOneCommit.Result r2 = pushFactory.create( - db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new cntent", + db, admin.getIdent(), testRepo, SUBJECT, FILE_NAME, "new content", r1.getChangeId()) .to("refs/for/master"); @@ -363,9 +386,13 @@ public class CommentsIT extends AbstractDaemonTest { + "-- \n"); } - private void addComment(PushOneCommit.Result r, String message) throws Exception { + addComment(r, message, false); + } + + private void addComment(PushOneCommit.Result r, String message, + boolean omitDuplicateComments) throws Exception { CommentInput c = new CommentInput(); c.line = 1; c.message = message; @@ -373,6 +400,7 @@ public class CommentsIT extends AbstractDaemonTest { ReviewInput in = new ReviewInput(); in.comments = ImmutableMap.> of( FILE_NAME, ImmutableList.of(c)); + in.omitDuplicateComments = omitDuplicateComments; gApi.changes() .id(r.getChangeId()) .revision(r.getCommit().name()) diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java index 873c560fb9..e6ce6b2ee4 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java @@ -48,6 +48,12 @@ public class ReviewInput { /** Who to send email notifications to after review is stored. */ public NotifyHandling notify = NotifyHandling.ALL; + /** + * If true check to make sure that the comments being posted aren't already + * present. + */ + public boolean omitDuplicateComments; + /** * Account ID, name, email address or username of another user. The review * will be posted/updated on behalf of this named user instead of the diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Side.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Side.java index 5d5af75d11..3485b8bb81 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Side.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Side.java @@ -15,5 +15,16 @@ package com.google.gerrit.extensions.client; public enum Side { - PARENT, REVISION -} \ No newline at end of file + PARENT, + REVISION; + + public static Side fromShort(short s) { + switch (s) { + case 0: + return PARENT; + case 1: + return REVISION; + } + return null; + } +} diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java index acf8b453f8..3ecd539ca9 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchLineComment.java @@ -52,7 +52,7 @@ public final class PatchLineComment { } @Override - protected void set(String newValue) { + public void set(String newValue) { uuid = newValue; } 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 7a24dbcfd5..68ea6882a0 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 @@ -17,12 +17,17 @@ package com.google.gerrit.server.change; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId; +import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.auto.value.AutoValue; import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; +import com.google.common.hash.HashCode; +import com.google.common.hash.Hashing; import com.google.gerrit.common.ChangeHooks; +import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelTypes; @@ -75,9 +80,11 @@ import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; @Singleton @@ -299,6 +306,32 @@ public class PostReview implements RestModifyView } } + /** + * Used to compare PatchLineComments with CommentInput comments. + */ + @AutoValue + abstract static class CommentSetEntry { + private static CommentSetEntry create(Patch.Key key, + Integer line, Side side, HashCode message, CommentRange range) { + return new AutoValue_PostReview_CommentSetEntry(key, line, side, message, + range); + } + + public static CommentSetEntry create(PatchLineComment comment) { + return create(comment.getKey().getParentKey(), + comment.getLine(), + Side.fromShort(comment.getSide()), + Hashing.sha1().hashString(comment.getMessage(), UTF_8), + comment.getRange()); + } + + abstract Patch.Key key(); + @Nullable abstract Integer line(); + abstract Side side(); + abstract HashCode message(); + @Nullable abstract CommentRange range(); + } + private class Op extends BatchUpdate.Op { private final PatchSet.Id psId; private final ReviewInput in; @@ -374,6 +407,10 @@ public class PostReview implements RestModifyView List del = Lists.newArrayList(); List ups = Lists.newArrayList(); + Set existingIds = in.omitDuplicateComments + ? readExistingComments(ctx) + : Collections.emptySet(); + for (Map.Entry> ent : map.entrySet()) { String path = ent.getKey(); for (CommentInput c : ent.getValue()) { @@ -381,9 +418,7 @@ public class PostReview implements RestModifyView PatchLineComment e = drafts.remove(Url.decode(c.id)); if (e == null) { e = new PatchLineComment( - new PatchLineComment.Key( - new Patch.Key(psId, path), - ChangeUtil.messageUUID(ctx.getDb())), + new PatchLineComment.Key(new Patch.Key(psId, path), null), c.line != null ? c.line : 0, user.getAccountId(), parent, ctx.getWhen()); @@ -403,6 +438,12 @@ public class PostReview implements RestModifyView c.range.endCharacter)); e.setLine(c.range.endLine); } + if (existingIds.contains(CommentSetEntry.create(e))) { + continue; + } + if (e.getKey().get() == null) { + e.getKey().set(ChangeUtil.messageUUID(ctx.getDb())); + } ups.add(e); } } @@ -430,6 +471,16 @@ public class PostReview implements RestModifyView return !del.isEmpty() || !ups.isEmpty(); } + private Set readExistingComments(ChangeContext ctx) + throws OrmException { + Set r = new HashSet<>(); + for (PatchLineComment c : plcUtil.publishedByChange(ctx.getDb(), + ctx.getChangeNotes())) { + r.add(CommentSetEntry.create(c)); + } + return r; + } + private Map changeDrafts(ChangeContext ctx) throws OrmException { Map drafts = Maps.newHashMap();