Move fix-suggestions from RobotComment to Comment
With this change, we allow storing fix suggestions with all comment
types, not just robot comments. We have deprecated robot comments
a while ago. The frontend checks framework allows checkers to provide
fix suggestions on demand with check results. This has obsoleted the
backend integration. In this change, we mark robot comments
as deprecated in Java code.
Gerrit already allowed humans to provide fix suggestions by using
markdown codeblocks in their comments. This has some limitations,
mainly the fact that you can only provide a single suggestion per
comment and that suggestion has to be a 1:1 replacement for the range
that the comment is tied to. This vastly limits the complexity of the
edit that can be represented. A real-world example is that a user
could not suggest to call a new util class, as that would mean importing
that class (in Java).
The robot comment storage for fixes is more elaborate and has good test
coverage as well as excellent input validation. RobotComment and
HumanComment inherit from the supertype comment. Hence, we uplift the
fixSuggestions from RobotComment to Comment. Consequently, they can now
be used with draft comments, human comments and robot comments.
The main diff of this change come from added test code. We decided to
copy some of the robot comments test code instead of trying to generalize
it to run on all types of comments beacause otherwise, we'd have to untangle
that generalization when we delete the robot comments code.
This change adds a new field (fix suggestions) to comments. Comments are
serialized as JSON in NoteDb and we ignore unknown fields. So this change
should be safe in terms of staged rollout failures. However, to be extra
safe, we are creating a follow up change that gates writing the new field
by an experiment parameter. The changes will be submitted together.
Forward-Compatible: checked
Release-Notes: Allow fix-suggestions with human comments
Change-Id: I0248d7842f3d742742b983c7b353585ea7dc7056
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 56cbf2f..e816cb4 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2480,7 +2480,7 @@
----
[[list-change-robot-comments]]
-=== List Change Robot Comments
+=== List Change Robot Comments (deprecated)
--
'GET /changes/link:#change-id[\{change-id\}]/robotcomments'
--
@@ -5437,7 +5437,7 @@
----
[[list-robot-comments]]
-=== List Robot Comments
+=== List Robot Comments (deprecated)
--
'GET /changes/link:#change-id[\{change-id\}]/revisions/link:#revision-id[\{revision-id\}]/robotcomments/'
--
@@ -5494,7 +5494,7 @@
----
[[get-robot-comment]]
-=== Get Robot Comment
+=== Get Robot Comment (deprecated)
--
'GET /changes/link:#change-id[\{change-id\}]/revisions/link:#revision-id[\{revision-id\}]/robotcomments/link:#comment-id[\{comment-id\}]'
--
@@ -7526,7 +7526,8 @@
Mime type of the file where the comment is written. Available only if the
"enable-context" parameter (see link:#list-change-comments[List Change Comments])
is set.
-
+|`fix_suggestions`|optional|Suggested fixes for this comment as a list of
+<<fix-suggestion-info,FixSuggestionInfo>> entities.
|===========================
[[comment-input]]
@@ -7574,6 +7575,8 @@
Whether or not the comment must be addressed by the user. This value will
default to false if the comment is an orphan, or the value of the `in_reply_to`
comment if it is supplied.
+|`fix_suggestions`|optional|Suggested fixes for this comment as a list of
+<<fix-suggestion-info,FixSuggestionInfo>> entities.
|===========================
[[comment-range]]
@@ -8615,7 +8618,7 @@
|`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.
-|`robot_comments` |optional|
+|`robot_comments` |optional, deprecated|
The robot comments that should be added as a map that maps a file path
to a list of link:#robot-comment-input[RobotCommentInput] entities.
|`drafts` |optional|
@@ -8865,7 +8868,7 @@
|===========================
[[robot-comment-info]]
-=== RobotCommentInfo
+=== RobotCommentInfo (deprecated)
The `RobotCommentInfo` entity contains information about a robot inline
comment.
@@ -8881,12 +8884,10 @@
|`url` |optional|URL to more information.
|`properties` |optional|Robot specific properties as map that maps arbitrary
keys to values.
-|`fix_suggestions`|optional|Suggested fixes for this robot comment as a list of
-<<fix-suggestion-info,FixSuggestionInfo>> entities.
|===========================
[[robot-comment-input]]
-=== RobotCommentInput
+=== RobotCommentInput (deprecated)
The `RobotCommentInput` entity contains information for creating an inline
robot comment.
@@ -8916,8 +8917,6 @@
|`url` |optional|URL to more information.
|`properties` |optional|Robot specific properties as map that maps arbitrary
keys to values.
-|`fix_suggestions`|optional|Suggested fixes for this robot comment as a list of
-<<fix-suggestion-info,FixSuggestionInfo>> entities.
|===========================
[[rule-input]]
diff --git a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
index 3bd355b..1fd780f 100644
--- a/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
+++ b/java/com/google/gerrit/acceptance/testsuite/change/PerPatchsetOperationsImpl.java
@@ -181,7 +181,8 @@
side,
message,
unresolved,
- parentUuid);
+ parentUuid,
+ null);
// For draft comments, only the tag set on the HumanComment (and not on the ChangeUpdate)
// matters.
commentCreation.tag().ifPresent(tag -> newComment.tag = tag);
diff --git a/java/com/google/gerrit/entities/Comment.java b/java/com/google/gerrit/entities/Comment.java
index baac18f..35a60eb 100644
--- a/java/com/google/gerrit/entities/Comment.java
+++ b/java/com/google/gerrit/entities/Comment.java
@@ -20,6 +20,7 @@
import java.sql.Timestamp;
import java.time.Instant;
import java.util.Comparator;
+import java.util.List;
import java.util.Objects;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
@@ -227,6 +228,8 @@
public Range range;
public String tag;
+ @Nullable public List<FixSuggestion> fixSuggestions;
+
/**
* Hex commit SHA1 of the commit of the patchset to which this comment applies. Other classes call
* this "commitId", but this class uses the old ReviewDb term "revId", and this field name is
@@ -300,7 +303,14 @@
+ (key != null ? nullableLength(key.filename, key.uuid) : 0);
}
- public abstract int getApproximateSize();
+ public int getApproximateSize() {
+ int approximateSize = getCommentFieldApproximateSize();
+ approximateSize +=
+ fixSuggestions != null
+ ? fixSuggestions.stream().mapToInt(FixSuggestion::getApproximateSize).sum()
+ : 0;
+ return approximateSize;
+ }
static int nullableLength(String... strings) {
int length = 0;
@@ -327,7 +337,8 @@
&& Objects.equals(range, c.range)
&& Objects.equals(tag, c.tag)
&& Objects.equals(revId, c.revId)
- && Objects.equals(serverId, c.serverId);
+ && Objects.equals(serverId, c.serverId)
+ && Objects.equals(fixSuggestions, c.fixSuggestions);
}
@Override
@@ -344,7 +355,8 @@
range,
tag,
revId,
- serverId);
+ serverId,
+ fixSuggestions);
}
@Override
@@ -364,6 +376,7 @@
.add("parentUuid", Objects.toString(parentUuid, ""))
.add("range", Objects.toString(range, ""))
.add("revId", Objects.toString(revId, ""))
- .add("tag", Objects.toString(tag, ""));
+ .add("tag", Objects.toString(tag, ""))
+ .add("fixSuggestions", Objects.toString(fixSuggestions, ""));
}
}
diff --git a/java/com/google/gerrit/entities/FixReplacement.java b/java/com/google/gerrit/entities/FixReplacement.java
index fbbf746..aa15ffc 100644
--- a/java/com/google/gerrit/entities/FixReplacement.java
+++ b/java/com/google/gerrit/entities/FixReplacement.java
@@ -14,6 +14,8 @@
package com.google.gerrit.entities;
+import java.util.Objects;
+
public final class FixReplacement {
public final String path;
public final Comment.Range range;
@@ -43,4 +45,20 @@
int getApproximateSize() {
return path.length() + replacement.length();
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof FixReplacement)) {
+ return false;
+ }
+ FixReplacement f = (FixReplacement) o;
+ return Objects.equals(path, f.path)
+ && Objects.equals(range, f.range)
+ && Objects.equals(replacement, f.replacement);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(path, range, replacement);
+ }
}
diff --git a/java/com/google/gerrit/entities/FixSuggestion.java b/java/com/google/gerrit/entities/FixSuggestion.java
index 892e324..737c23e 100644
--- a/java/com/google/gerrit/entities/FixSuggestion.java
+++ b/java/com/google/gerrit/entities/FixSuggestion.java
@@ -15,6 +15,7 @@
package com.google.gerrit.entities;
import java.util.List;
+import java.util.Objects;
public final class FixSuggestion {
public final String fixId;
@@ -47,4 +48,20 @@
+ description.length()
+ replacements.stream().mapToInt(FixReplacement::getApproximateSize).sum();
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof FixSuggestion)) {
+ return false;
+ }
+ FixSuggestion fs = (FixSuggestion) o;
+ return Objects.equals(fixId, fs.fixId)
+ && Objects.equals(description, fs.description)
+ && Objects.equals(replacements, fs.replacements);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(fixId, description, replacements);
+ }
}
diff --git a/java/com/google/gerrit/entities/HumanComment.java b/java/com/google/gerrit/entities/HumanComment.java
index d287fa0..1e48f11 100644
--- a/java/com/google/gerrit/entities/HumanComment.java
+++ b/java/com/google/gerrit/entities/HumanComment.java
@@ -47,11 +47,6 @@
}
@Override
- public int getApproximateSize() {
- return super.getCommentFieldApproximateSize();
- }
-
- @Override
public String toString() {
return toStringHelper().add("unresolved", unresolved).toString();
}
diff --git a/java/com/google/gerrit/entities/RobotComment.java b/java/com/google/gerrit/entities/RobotComment.java
index 1d46d3b..a4288ca 100644
--- a/java/com/google/gerrit/entities/RobotComment.java
+++ b/java/com/google/gerrit/entities/RobotComment.java
@@ -15,16 +15,15 @@
package com.google.gerrit.entities;
import java.time.Instant;
-import java.util.List;
import java.util.Map;
import java.util.Objects;
+@Deprecated
public final class RobotComment extends Comment {
public String robotId;
public String robotRunId;
public String url;
public Map<String, String> properties;
- public List<FixSuggestion> fixSuggestions;
public RobotComment(
Key key,
@@ -64,7 +63,6 @@
.add("robotRunId", robotRunId)
.add("url", url)
.add("properties", Objects.toString(properties, ""))
- .add("fixSuggestions", Objects.toString(fixSuggestions, ""))
.toString();
}
@@ -78,12 +76,11 @@
&& Objects.equals(robotId, c.robotId)
&& Objects.equals(robotRunId, c.robotRunId)
&& Objects.equals(url, c.url)
- && Objects.equals(properties, c.properties)
- && Objects.equals(fixSuggestions, c.fixSuggestions);
+ && Objects.equals(properties, c.properties);
}
@Override
public int hashCode() {
- return Objects.hash(super.hashCode(), robotId, robotRunId, url, properties, fixSuggestions);
+ return Objects.hash(super.hashCode(), robotId, robotRunId, url, properties);
}
}
diff --git a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
index 98807cb..2584448 100644
--- a/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
+++ b/java/com/google/gerrit/extensions/api/changes/ReviewInput.java
@@ -22,7 +22,6 @@
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.ReviewerState;
-import com.google.gerrit.extensions.common.FixSuggestionInfo;
import com.google.gerrit.extensions.restapi.DefaultInput;
import java.util.ArrayList;
import java.util.Collections;
@@ -39,7 +38,7 @@
public Map<String, Short> labels;
public Map<String, List<CommentInput>> comments;
- public Map<String, List<RobotCommentInput>> robotComments;
+ @Deprecated public Map<String, List<RobotCommentInput>> robotComments;
/**
* How to process draft comments already in the database that were not also described in this
@@ -115,12 +114,12 @@
public Boolean unresolved;
}
+ @Deprecated
public static class RobotCommentInput extends Comment {
public String robotId;
public String robotRunId;
public String url;
public Map<String, String> properties;
- public List<FixSuggestionInfo> fixSuggestions;
}
@CanIgnoreReturnValue
diff --git a/java/com/google/gerrit/extensions/client/Comment.java b/java/com/google/gerrit/extensions/client/Comment.java
index b8843d3..8a68236 100644
--- a/java/com/google/gerrit/extensions/client/Comment.java
+++ b/java/com/google/gerrit/extensions/client/Comment.java
@@ -14,9 +14,11 @@
package com.google.gerrit.extensions.client;
+import com.google.gerrit.extensions.common.FixSuggestionInfo;
import java.sql.Timestamp;
import java.time.Instant;
import java.util.Comparator;
+import java.util.List;
import java.util.Objects;
public abstract class Comment {
@@ -49,6 +51,8 @@
*/
public String commitId;
+ public List<FixSuggestionInfo> fixSuggestions;
+
// TODO(issue-15508): Migrate timestamp fields in *Info/*Input classes from type Timestamp to
// Instant
@SuppressWarnings("JdkObsolete")
@@ -151,13 +155,15 @@
&& Objects.equals(inReplyTo, c.inReplyTo)
&& Objects.equals(updated, c.updated)
&& Objects.equals(message, c.message)
- && Objects.equals(commitId, c.commitId);
+ && Objects.equals(commitId, c.commitId)
+ && Objects.equals(fixSuggestions, c.fixSuggestions);
}
return false;
}
@Override
public int hashCode() {
- return Objects.hash(patchSet, id, path, side, parent, line, range, inReplyTo, updated, message);
+ return Objects.hash(
+ patchSet, id, path, side, parent, line, range, inReplyTo, updated, message, fixSuggestions);
}
}
diff --git a/java/com/google/gerrit/extensions/common/CommentInfo.java b/java/com/google/gerrit/extensions/common/CommentInfo.java
index 35587a0..aee2552 100644
--- a/java/com/google/gerrit/extensions/common/CommentInfo.java
+++ b/java/com/google/gerrit/extensions/common/CommentInfo.java
@@ -39,13 +39,14 @@
CommentInfo ci = (CommentInfo) o;
return Objects.equals(author, ci.author)
&& Objects.equals(tag, ci.tag)
- && Objects.equals(unresolved, ci.unresolved);
+ && Objects.equals(unresolved, ci.unresolved)
+ && Objects.equals(fixSuggestions, ci.fixSuggestions);
}
return false;
}
@Override
public int hashCode() {
- return Objects.hash(super.hashCode(), author, tag, unresolved);
+ return Objects.hash(super.hashCode(), author, tag, unresolved, fixSuggestions);
}
}
diff --git a/java/com/google/gerrit/extensions/common/FixReplacementInfo.java b/java/com/google/gerrit/extensions/common/FixReplacementInfo.java
index 9e5890e..6df4fb9 100644
--- a/java/com/google/gerrit/extensions/common/FixReplacementInfo.java
+++ b/java/com/google/gerrit/extensions/common/FixReplacementInfo.java
@@ -15,9 +15,26 @@
package com.google.gerrit.extensions.common;
import com.google.gerrit.extensions.client.Comment;
+import java.util.Objects;
public class FixReplacementInfo {
public String path;
public Comment.Range range;
public String replacement;
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof FixReplacementInfo)) {
+ return false;
+ }
+ FixReplacementInfo fs = (FixReplacementInfo) o;
+ return Objects.equals(path, fs.path)
+ && Objects.equals(range, fs.range)
+ && Objects.equals(replacement, fs.replacement);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(path, range, replacement);
+ }
}
diff --git a/java/com/google/gerrit/extensions/common/FixSuggestionInfo.java b/java/com/google/gerrit/extensions/common/FixSuggestionInfo.java
index 7ba7fcc..50df366 100644
--- a/java/com/google/gerrit/extensions/common/FixSuggestionInfo.java
+++ b/java/com/google/gerrit/extensions/common/FixSuggestionInfo.java
@@ -15,9 +15,26 @@
package com.google.gerrit.extensions.common;
import java.util.List;
+import java.util.Objects;
public class FixSuggestionInfo {
public String fixId;
public String description;
public List<FixReplacementInfo> replacements;
+
+ @Override
+ public boolean equals(Object o) {
+ if (!(o instanceof FixSuggestionInfo)) {
+ return false;
+ }
+ FixSuggestionInfo fs = (FixSuggestionInfo) o;
+ return Objects.equals(fixId, fs.fixId)
+ && Objects.equals(description, fs.description)
+ && Objects.equals(replacements, fs.replacements);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(fixId, description, replacements);
+ }
}
diff --git a/java/com/google/gerrit/extensions/common/RobotCommentInfo.java b/java/com/google/gerrit/extensions/common/RobotCommentInfo.java
index 8d8731f..780cab7 100644
--- a/java/com/google/gerrit/extensions/common/RobotCommentInfo.java
+++ b/java/com/google/gerrit/extensions/common/RobotCommentInfo.java
@@ -14,13 +14,12 @@
package com.google.gerrit.extensions.common;
-import java.util.List;
import java.util.Map;
+@Deprecated
public class RobotCommentInfo extends CommentInfo {
public String robotId;
public String robotRunId;
public String url;
public Map<String, String> properties;
- public List<FixSuggestionInfo> fixSuggestions;
}
diff --git a/java/com/google/gerrit/extensions/common/testing/CommentInfoSubject.java b/java/com/google/gerrit/extensions/common/testing/CommentInfoSubject.java
index c34e439..049d6e4 100644
--- a/java/com/google/gerrit/extensions/common/testing/CommentInfoSubject.java
+++ b/java/com/google/gerrit/extensions/common/testing/CommentInfoSubject.java
@@ -17,6 +17,7 @@
import static com.google.common.truth.Truth.assertAbout;
import static com.google.gerrit.extensions.common.testing.AccountInfoSubject.accounts;
import static com.google.gerrit.extensions.common.testing.RangeSubject.ranges;
+import static com.google.gerrit.truth.ListSubject.elements;
import com.google.common.truth.BooleanSubject;
import com.google.common.truth.ComparableSubject;
@@ -26,6 +27,7 @@
import com.google.common.truth.Subject;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.FixSuggestionInfo;
import com.google.gerrit.truth.ListSubject;
import java.sql.Timestamp;
import java.util.List;
@@ -52,6 +54,12 @@
this.commentInfo = commentInfo;
}
+ public ListSubject<FixSuggestionInfoSubject, FixSuggestionInfo> fixSuggestions() {
+ return check("fixSuggestions")
+ .about(elements())
+ .thatCustom(commentInfo.fixSuggestions, FixSuggestionInfoSubject.fixSuggestions());
+ }
+
public StringSubject uuid() {
return check("id").that(commentInfo().id);
}
@@ -116,4 +124,8 @@
isNotNull();
return commentInfo;
}
+
+ public FixSuggestionInfoSubject onlyFixSuggestion() {
+ return fixSuggestions().onlyElement();
+ }
}
diff --git a/java/com/google/gerrit/server/CommentsUtil.java b/java/com/google/gerrit/server/CommentsUtil.java
index e290002..30b8747 100644
--- a/java/com/google/gerrit/server/CommentsUtil.java
+++ b/java/com/google/gerrit/server/CommentsUtil.java
@@ -18,14 +18,18 @@
import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.toCollection;
+import static java.util.stream.Collectors.toList;
import com.google.common.collect.ComparisonChain;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.collect.Ordering;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.Comment;
+import com.google.gerrit.entities.FixReplacement;
+import com.google.gerrit.entities.FixSuggestion;
import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
@@ -33,6 +37,8 @@
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.FixReplacementInfo;
+import com.google.gerrit.extensions.common.FixSuggestionInfo;
import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -130,7 +136,8 @@
short side,
String message,
@Nullable Boolean unresolved,
- @Nullable String parentUuid) {
+ @Nullable String parentUuid,
+ @Nullable List<FixSuggestion> fixSuggestions) {
if (unresolved == null) {
if (parentUuid == null) {
// Default to false if comment is not descended from another.
@@ -155,6 +162,7 @@
serverId,
unresolved);
c.parentUuid = parentUuid;
+ c.fixSuggestions = fixSuggestions;
currentUser.updateRealAccountId(c::setRealAuthor);
return c;
}
@@ -398,4 +406,35 @@
comments.sort(COMMENT_ORDER);
return comments;
}
+
+ @Nullable
+ public static ImmutableList<FixSuggestion> createFixSuggestionsFromInput(
+ List<FixSuggestionInfo> fixSuggestionInfos) {
+ if (fixSuggestionInfos == null) {
+ return null;
+ }
+
+ ImmutableList.Builder<FixSuggestion> fixSuggestions =
+ ImmutableList.builderWithExpectedSize(fixSuggestionInfos.size());
+ for (FixSuggestionInfo fixSuggestionInfo : fixSuggestionInfos) {
+ fixSuggestions.add(createFixSuggestionFromInput(fixSuggestionInfo));
+ }
+ return fixSuggestions.build();
+ }
+
+ public static FixSuggestion createFixSuggestionFromInput(FixSuggestionInfo fixSuggestionInfo) {
+ List<FixReplacement> fixReplacements = toFixReplacements(fixSuggestionInfo.replacements);
+ String fixId = ChangeUtil.messageUuid();
+ return new FixSuggestion(fixId, fixSuggestionInfo.description, fixReplacements);
+ }
+
+ public static List<FixReplacement> toFixReplacements(
+ List<FixReplacementInfo> fixReplacementInfos) {
+ return fixReplacementInfos.stream().map(CommentsUtil::toFixReplacement).collect(toList());
+ }
+
+ public static FixReplacement toFixReplacement(FixReplacementInfo fixReplacementInfo) {
+ Comment.Range range = new Comment.Range(fixReplacementInfo.range);
+ return new FixReplacement(fixReplacementInfo.path, range, fixReplacementInfo.replacement);
+ }
}
diff --git a/java/com/google/gerrit/server/change/CommentsValidator.java b/java/com/google/gerrit/server/change/CommentsValidator.java
new file mode 100644
index 0000000..0cdd9b6
--- /dev/null
+++ b/java/com/google/gerrit/server/change/CommentsValidator.java
@@ -0,0 +1,251 @@
+// Copyright (C) 2014 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
+import static java.util.stream.Collectors.groupingBy;
+import static java.util.stream.Collectors.toList;
+
+import com.google.gerrit.entities.Patch;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.extensions.client.Comment.Range;
+import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
+import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.FixReplacementInfo;
+import com.google.gerrit.extensions.common.FixSuggestionInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.server.CommentsUtil;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.patch.DiffSummary;
+import com.google.gerrit.server.patch.DiffSummaryKey;
+import com.google.gerrit.server.patch.PatchListCache;
+import com.google.gerrit.server.patch.PatchListKey;
+import com.google.gerrit.server.patch.PatchListNotAvailableException;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.eclipse.jgit.lib.ObjectId;
+
+@Singleton
+public class CommentsValidator {
+
+ private final CommentsUtil commentsUtil;
+ private final PatchListCache patchListCache;
+
+ @Inject
+ CommentsValidator(CommentsUtil commentsUtil, PatchListCache patchListCache) {
+ this.commentsUtil = commentsUtil;
+ this.patchListCache = patchListCache;
+ }
+
+ public static void ensureFixSuggestionsAreAddable(
+ List<FixSuggestionInfo> fixSuggestionInfos, String commentPath) throws BadRequestException {
+ if (fixSuggestionInfos == null) {
+ return;
+ }
+
+ for (FixSuggestionInfo fixSuggestionInfo : fixSuggestionInfos) {
+ ensureDescriptionIsSet(commentPath, fixSuggestionInfo.description);
+ ensureFixReplacementsAreAddable(commentPath, fixSuggestionInfo.replacements);
+ }
+ }
+
+ public <T extends com.google.gerrit.extensions.client.Comment> void checkComments(
+ RevisionResource revision, Map<String, List<T>> commentsPerPath)
+ throws BadRequestException, PatchListNotAvailableException {
+ Set<String> revisionFilePaths = getAffectedFilePaths(revision);
+ for (Map.Entry<String, List<T>> entry : commentsPerPath.entrySet()) {
+ String path = entry.getKey();
+ PatchSet.Id patchSetId = revision.getPatchSet().id();
+ ensurePathRefersToAvailableOrMagicFile(path, revisionFilePaths, patchSetId);
+
+ List<T> comments = entry.getValue();
+ for (T comment : comments) {
+ ensureLineIsNonNegative(comment.line, path);
+ ensureCommentNotOnMagicFilesOfAutoMerge(path, comment);
+ ensureRangeIsValid(path, comment.range);
+ ensureValidPatchsetLevelComment(path, comment);
+ ensureValidInReplyTo(revision.getNotes(), comment.inReplyTo);
+ ensureFixSuggestionsAreAddable(comment.fixSuggestions, path);
+ }
+ }
+ }
+
+ private void ensureValidInReplyTo(ChangeNotes changeNotes, String inReplyTo)
+ throws BadRequestException {
+ if (inReplyTo != null
+ && !commentsUtil.getPublishedHumanComment(changeNotes, inReplyTo).isPresent()
+ && !commentsUtil.getRobotComment(changeNotes, inReplyTo).isPresent()) {
+ throw new BadRequestException(
+ String.format("Invalid inReplyTo, comment %s not found", inReplyTo));
+ }
+ }
+
+ private Set<String> getAffectedFilePaths(RevisionResource revision)
+ throws PatchListNotAvailableException {
+ ObjectId newId = revision.getPatchSet().commitId();
+ DiffSummaryKey key =
+ DiffSummaryKey.fromPatchListKey(
+ PatchListKey.againstDefaultBase(newId, Whitespace.IGNORE_NONE));
+ DiffSummary ds = patchListCache.getDiffSummary(key, revision.getProject());
+ return new HashSet<>(ds.getPaths());
+ }
+
+ private static void ensurePathRefersToAvailableOrMagicFile(
+ String path, Set<String> availableFilePaths, PatchSet.Id patchSetId)
+ throws BadRequestException {
+ if (!availableFilePaths.contains(path) && !Patch.isMagic(path)) {
+ throw new BadRequestException(
+ String.format("file %s not found in revision %s", path, patchSetId));
+ }
+ }
+
+ private static void ensureLineIsNonNegative(Integer line, String path)
+ throws BadRequestException {
+ if (line != null && line < 0) {
+ throw new BadRequestException(
+ String.format("negative line number %d not allowed on %s", line, path));
+ }
+ }
+
+ private static <T extends com.google.gerrit.extensions.client.Comment>
+ void ensureCommentNotOnMagicFilesOfAutoMerge(String path, T comment)
+ throws BadRequestException {
+ if (Patch.isMagic(path) && comment.side == Side.PARENT && comment.parent == null) {
+ throw new BadRequestException(String.format("cannot comment on %s on auto-merge", path));
+ }
+ }
+
+ private static <T extends com.google.gerrit.extensions.client.Comment>
+ void ensureValidPatchsetLevelComment(String path, T comment) throws BadRequestException {
+ if (path.equals(PATCHSET_LEVEL)
+ && (comment.side != null || comment.range != null || comment.line != null)) {
+ throw new BadRequestException("Patchset-level comments can't have side, range, or line");
+ }
+ }
+
+ private static void ensureFixReplacementsAreAddable(
+ String commentPath, List<FixReplacementInfo> fixReplacementInfos) throws BadRequestException {
+ ensureReplacementsArePresent(commentPath, fixReplacementInfos);
+
+ for (FixReplacementInfo fixReplacementInfo : fixReplacementInfos) {
+ ensureReplacementPathIsSetAndNotPatchsetLevel(commentPath, fixReplacementInfo.path);
+ ensureRangeIsSet(commentPath, fixReplacementInfo.range);
+ ensureRangeIsValid(commentPath, fixReplacementInfo.range);
+ ensureReplacementStringIsSet(commentPath, fixReplacementInfo.replacement);
+ }
+
+ Map<String, List<FixReplacementInfo>> replacementsPerFilePath =
+ fixReplacementInfos.stream().collect(groupingBy(fixReplacement -> fixReplacement.path));
+ for (List<FixReplacementInfo> sameFileReplacements : replacementsPerFilePath.values()) {
+ ensureRangesDoNotOverlap(commentPath, sameFileReplacements);
+ }
+ }
+
+ private static void ensureReplacementsArePresent(
+ String commentPath, List<FixReplacementInfo> fixReplacementInfos) throws BadRequestException {
+ if (fixReplacementInfos == null || fixReplacementInfos.isEmpty()) {
+ throw new BadRequestException(
+ String.format(
+ "At least one replacement is "
+ + "required for the suggested fix of the comment on %s",
+ commentPath));
+ }
+ }
+
+ private static void ensureReplacementPathIsSetAndNotPatchsetLevel(
+ String commentPath, String replacementPath) throws BadRequestException {
+ if (replacementPath == null) {
+ throw new BadRequestException(
+ String.format(
+ "A file path must be given for the replacement of the comment on %s", commentPath));
+ }
+ if (replacementPath.equals(PATCHSET_LEVEL)) {
+ throw new BadRequestException(
+ String.format(
+ "A file path must not be %s for the replacement of the comment on %s",
+ PATCHSET_LEVEL, commentPath));
+ }
+ }
+
+ private static void ensureRangeIsSet(String commentPath, Range range) throws BadRequestException {
+ if (range == null) {
+ throw new BadRequestException(
+ String.format(
+ "A range must be given for the replacement of the comment on %s", commentPath));
+ }
+ }
+
+ private static void ensureRangeIsValid(String commentPath, Range range)
+ throws BadRequestException {
+ if (range == null) {
+ return;
+ }
+ if (!range.isValid()) {
+ throw new BadRequestException(
+ String.format(
+ "Range (%s:%s - %s:%s) is not valid for the comment on %s",
+ range.startLine,
+ range.startCharacter,
+ range.endLine,
+ range.endCharacter,
+ commentPath));
+ }
+ }
+
+ private static void ensureReplacementStringIsSet(String commentPath, String replacement)
+ throws BadRequestException {
+ if (replacement == null) {
+ throw new BadRequestException(
+ String.format(
+ "A content for replacement "
+ + "must be indicated for the replacement of the comment on %s",
+ commentPath));
+ }
+ }
+
+ private static void ensureRangesDoNotOverlap(
+ String commentPath, List<FixReplacementInfo> fixReplacementInfos) throws BadRequestException {
+ List<Range> sortedRanges =
+ fixReplacementInfos.stream()
+ .map(fixReplacementInfo -> fixReplacementInfo.range)
+ .sorted()
+ .collect(toList());
+
+ int previousEndLine = 0;
+ int previousOffset = -1;
+ for (Range range : sortedRanges) {
+ if (range.startLine < previousEndLine
+ || (range.startLine == previousEndLine && range.startCharacter < previousOffset)) {
+ throw new BadRequestException(
+ String.format("Replacements overlap for the comment on %s", commentPath));
+ }
+ previousEndLine = range.endLine;
+ previousOffset = range.endCharacter;
+ }
+ }
+
+ private static void ensureDescriptionIsSet(String commentPath, String description)
+ throws BadRequestException {
+ if (description == null) {
+ throw new BadRequestException(
+ String.format(
+ "A description is required for the suggested fix of the comment on %s", commentPath));
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java b/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java
index 58b0cb1..f31f148 100644
--- a/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java
+++ b/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java
@@ -46,7 +46,7 @@
private boolean exceedsSizeLimit(CommentForValidation comment) {
switch (comment.getSource()) {
case HUMAN:
- return comment.getApproximateSize() > commentSizeLimit;
+ return commentSizeLimit > 0 && comment.getApproximateSize() > commentSizeLimit;
case ROBOT:
return robotCommentSizeLimit > 0 && comment.getApproximateSize() > robotCommentSizeLimit;
}
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index 76be2f4..0648a18 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -451,6 +451,7 @@
(short) side.ordinal(),
mailComment.getMessage(),
false,
+ null,
null);
comment.tag = tag;
diff --git a/java/com/google/gerrit/server/restapi/change/CommentJson.java b/java/com/google/gerrit/server/restapi/change/CommentJson.java
index 2d0c739..272afc9 100644
--- a/java/com/google/gerrit/server/restapi/change/CommentJson.java
+++ b/java/com/google/gerrit/server/restapi/change/CommentJson.java
@@ -227,6 +227,7 @@
r.author = loader.get(c.author.getId());
}
r.commitId = c.getCommitId().getName();
+ r.fixSuggestions = toFixSuggestionInfos(c.fixSuggestions);
}
protected Range toRange(Comment.Range commentRange) {
@@ -240,32 +241,6 @@
}
return range;
}
- }
-
- public class HumanCommentFormatter extends BaseCommentFormatter<HumanComment, CommentInfo> {
- @Override
- protected CommentInfo toInfo(HumanComment c, AccountLoader loader) {
- CommentInfo ci = new CommentInfo();
- fillCommentInfo(c, ci, loader);
- ci.unresolved = c.unresolved;
- return ci;
- }
-
- private HumanCommentFormatter() {}
- }
-
- class RobotCommentFormatter extends BaseCommentFormatter<RobotComment, RobotCommentInfo> {
- @Override
- protected RobotCommentInfo toInfo(RobotComment c, AccountLoader loader) {
- RobotCommentInfo rci = new RobotCommentInfo();
- rci.robotId = c.robotId;
- rci.robotRunId = c.robotRunId;
- rci.url = c.url;
- rci.properties = c.properties;
- rci.fixSuggestions = toFixSuggestionInfos(c.fixSuggestions);
- fillCommentInfo(c, rci, loader);
- return rci;
- }
@Nullable
private List<FixSuggestionInfo> toFixSuggestionInfos(
@@ -293,6 +268,31 @@
fixReplacementInfo.replacement = fixReplacement.replacement;
return fixReplacementInfo;
}
+ }
+
+ public class HumanCommentFormatter extends BaseCommentFormatter<HumanComment, CommentInfo> {
+ @Override
+ protected CommentInfo toInfo(HumanComment c, AccountLoader loader) {
+ CommentInfo ci = new CommentInfo();
+ fillCommentInfo(c, ci, loader);
+ ci.unresolved = c.unresolved;
+ return ci;
+ }
+
+ private HumanCommentFormatter() {}
+ }
+
+ class RobotCommentFormatter extends BaseCommentFormatter<RobotComment, RobotCommentInfo> {
+ @Override
+ protected RobotCommentInfo toInfo(RobotComment c, AccountLoader loader) {
+ RobotCommentInfo rci = new RobotCommentInfo();
+ rci.robotId = c.robotId;
+ rci.robotRunId = c.robotRunId;
+ rci.url = c.url;
+ rci.properties = c.properties;
+ fillCommentInfo(c, rci, loader);
+ return rci;
+ }
private RobotCommentFormatter() {}
}
diff --git a/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java b/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java
index 8849c82..c386b29 100644
--- a/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java
+++ b/java/com/google/gerrit/server/restapi/change/CreateDraftComment.java
@@ -41,6 +41,7 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.PublishCommentUtil;
+import com.google.gerrit.server.change.CommentsValidator;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -134,6 +135,8 @@
rsrc.getPatchSet(),
commentsUtil);
+ CommentsValidator.ensureFixSuggestionsAreAddable(in.fixSuggestions, in.path);
+
CommentValidationContext ctx =
CommentValidationContext.create(
rsrc.getChange().getChangeId(),
@@ -182,7 +185,8 @@
draftInput.side(),
draftInput.message.trim(),
draftInput.unresolved,
- parentUuid);
+ parentUuid,
+ CommentsUtil.createFixSuggestionsFromInput(draftInput.fixSuggestions));
comment.setLineNbrAndRange(draftInput.line, draftInput.range);
comment.tag = draftInput.tag;
diff --git a/java/com/google/gerrit/server/restapi/change/Fixes.java b/java/com/google/gerrit/server/restapi/change/Fixes.java
index 38240e3..080c1e7 100644
--- a/java/com/google/gerrit/server/restapi/change/Fixes.java
+++ b/java/com/google/gerrit/server/restapi/change/Fixes.java
@@ -14,8 +14,8 @@
package com.google.gerrit.server.restapi.change;
+import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.FixSuggestion;
-import com.google.gerrit.entities.RobotComment;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.ChildCollection;
import com.google.gerrit.extensions.restapi.IdString;
@@ -27,6 +27,7 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
@@ -53,10 +54,13 @@
String fixId = id.get();
ChangeNotes changeNotes = revisionResource.getNotes();
- List<RobotComment> robotComments =
- commentsUtil.robotCommentsByPatchSet(changeNotes, revisionResource.getPatchSet().id());
- for (RobotComment robotComment : robotComments) {
- for (FixSuggestion fixSuggestion : robotComment.fixSuggestions) {
+ List<Comment> allComments = new ArrayList<>();
+ allComments.addAll(
+ commentsUtil.publishedByPatchSet(changeNotes, revisionResource.getPatchSet().id()));
+ allComments.addAll(
+ commentsUtil.robotCommentsByPatchSet(changeNotes, revisionResource.getPatchSet().id()));
+ for (Comment comment : allComments) {
+ for (FixSuggestion fixSuggestion : comment.fixSuggestions) {
if (Objects.equals(fixId, fixSuggestion.fixId)) {
return new FixResource(revisionResource, fixSuggestion.replacements);
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 32474a4..50cd2bc 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -16,12 +16,10 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
-import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
import static com.google.gerrit.server.permissions.AbstractLabelPermission.ForUser.ON_BEHALF_OF;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.CHANGE_MODIFICATION;
import static java.nio.charset.StandardCharsets.UTF_8;
-import static java.util.stream.Collectors.groupingBy;
import static java.util.stream.Collectors.toList;
import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
@@ -42,7 +40,6 @@
import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelTypes;
-import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -52,12 +49,8 @@
import com.google.gerrit.extensions.api.changes.ReviewResult;
import com.google.gerrit.extensions.api.changes.ReviewerInput;
import com.google.gerrit.extensions.api.changes.ReviewerResult;
-import com.google.gerrit.extensions.client.Comment.Range;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.client.Side;
-import com.google.gerrit.extensions.common.FixReplacementInfo;
-import com.google.gerrit.extensions.common.FixSuggestionInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -70,7 +63,6 @@
import com.google.gerrit.metrics.Field;
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.ChangeMessagesUtil;
-import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.DraftCommentsReader;
import com.google.gerrit.server.IdentifiedUser;
@@ -81,6 +73,7 @@
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.CommentsValidator;
import com.google.gerrit.server.change.ModifyReviewersEmail;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.ReviewerModifier;
@@ -92,11 +85,6 @@
import com.google.gerrit.server.extensions.events.ReviewerAdded;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.patch.DiffSummary;
-import com.google.gerrit.server.patch.DiffSummaryKey;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.LabelPermission;
@@ -115,17 +103,14 @@
import java.time.Instant;
import java.util.ArrayList;
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.Optional;
-import java.util.Set;
import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
-import org.eclipse.jgit.lib.ObjectId;
@Singleton
public class PostReview implements RestModifyView<RevisionResource, ReviewInput> {
@@ -164,10 +149,8 @@
private final ChangeData.Factory changeDataFactory;
private final AccountCache accountCache;
private final ApprovalsUtil approvalsUtil;
- private final CommentsUtil commentsUtil;
private final DraftCommentsReader draftCommentsReader;
- private final PatchListCache patchListCache;
private final AccountResolver accountResolver;
private final ReviewerModifier reviewerModifier;
private final Metrics metrics;
@@ -181,6 +164,7 @@
private final ReviewerAdded reviewerAdded;
private final boolean strictLabels;
private final ChangeJson.Factory changeJsonFactory;
+ private final CommentsValidator commentsValidator;
@Inject
PostReview(
@@ -190,9 +174,7 @@
ChangeData.Factory changeDataFactory,
AccountCache accountCache,
ApprovalsUtil approvalsUtil,
- CommentsUtil commentsUtil,
DraftCommentsReader draftCommentsReader,
- PatchListCache patchListCache,
AccountResolver accountResolver,
ReviewerModifier reviewerModifier,
Metrics metrics,
@@ -204,15 +186,14 @@
PermissionBackend permissionBackend,
ReplyAttentionSetUpdates replyAttentionSetUpdates,
ReviewerAdded reviewerAdded,
- ChangeJson.Factory changeJsonFactory) {
+ ChangeJson.Factory changeJsonFactory,
+ CommentsValidator commentsValidator) {
this.updateFactory = updateFactory;
this.postReviewOpFactory = postReviewOpFactory;
this.changeResourceFactory = changeResourceFactory;
this.changeDataFactory = changeDataFactory;
this.accountCache = accountCache;
- this.commentsUtil = commentsUtil;
this.draftCommentsReader = draftCommentsReader;
- this.patchListCache = patchListCache;
this.approvalsUtil = approvalsUtil;
this.accountResolver = accountResolver;
this.reviewerModifier = reviewerModifier;
@@ -226,6 +207,7 @@
this.reviewerAdded = reviewerAdded;
this.strictLabels = gerritConfig.getBoolean("change", "strictLabels", false);
this.changeJsonFactory = changeJsonFactory;
+ this.commentsValidator = commentsValidator;
}
@Override
@@ -261,7 +243,7 @@
}
if (input.comments != null) {
input.comments = cleanUpComments(input.comments);
- checkComments(revision, input.comments);
+ commentsValidator.checkComments(revision, input.comments);
}
if (input.draftIdsToPublish != null) {
checkDraftIds(revision, input.draftIdsToPublish, input.drafts);
@@ -667,27 +649,6 @@
.collect(toList());
}
- private <T extends com.google.gerrit.extensions.client.Comment> void checkComments(
- RevisionResource revision, Map<String, List<T>> commentsPerPath)
- throws BadRequestException, PatchListNotAvailableException {
- logger.atFine().log("checking comments");
- Set<String> revisionFilePaths = getAffectedFilePaths(revision);
- for (Map.Entry<String, List<T>> entry : commentsPerPath.entrySet()) {
- String path = entry.getKey();
- PatchSet.Id patchSetId = revision.getPatchSet().id();
- ensurePathRefersToAvailableOrMagicFile(path, revisionFilePaths, patchSetId);
-
- List<T> comments = entry.getValue();
- for (T comment : comments) {
- ensureLineIsNonNegative(comment.line, path);
- ensureCommentNotOnMagicFilesOfAutoMerge(path, comment);
- ensureRangeIsValid(path, comment.range);
- ensureValidPatchsetLevelComment(path, comment);
- ensureValidInReplyTo(revision.getNotes(), comment.inReplyTo);
- }
- }
- }
-
/**
* Asserts that the draft IDs to publish are valid, i.e. they exist and belong to the current
* user. If the {@code draftHandling} parameter is equal to {@link DraftHandling#PUBLISH}, then
@@ -724,59 +685,6 @@
}
}
- private Set<String> getAffectedFilePaths(RevisionResource revision)
- throws PatchListNotAvailableException {
- ObjectId newId = revision.getPatchSet().commitId();
- DiffSummaryKey key =
- DiffSummaryKey.fromPatchListKey(
- PatchListKey.againstDefaultBase(newId, Whitespace.IGNORE_NONE));
- DiffSummary ds = patchListCache.getDiffSummary(key, revision.getProject());
- return new HashSet<>(ds.getPaths());
- }
-
- private static void ensurePathRefersToAvailableOrMagicFile(
- String path, Set<String> availableFilePaths, PatchSet.Id patchSetId)
- throws BadRequestException {
- if (!availableFilePaths.contains(path) && !Patch.isMagic(path)) {
- throw new BadRequestException(
- String.format("file %s not found in revision %s", path, patchSetId));
- }
- }
-
- private static void ensureLineIsNonNegative(Integer line, String path)
- throws BadRequestException {
- if (line != null && line < 0) {
- throw new BadRequestException(
- String.format("negative line number %d not allowed on %s", line, path));
- }
- }
-
- private static <T extends com.google.gerrit.extensions.client.Comment>
- void ensureCommentNotOnMagicFilesOfAutoMerge(String path, T comment)
- throws BadRequestException {
- if (Patch.isMagic(path) && comment.side == Side.PARENT && comment.parent == null) {
- throw new BadRequestException(String.format("cannot comment on %s on auto-merge", path));
- }
- }
-
- private static <T extends com.google.gerrit.extensions.client.Comment>
- void ensureValidPatchsetLevelComment(String path, T comment) throws BadRequestException {
- if (path.equals(PATCHSET_LEVEL)
- && (comment.side != null || comment.range != null || comment.line != null)) {
- throw new BadRequestException("Patchset-level comments can't have side, range, or line");
- }
- }
-
- private void ensureValidInReplyTo(ChangeNotes changeNotes, String inReplyTo)
- throws BadRequestException {
- if (inReplyTo != null
- && !commentsUtil.getPublishedHumanComment(changeNotes, inReplyTo).isPresent()
- && !commentsUtil.getRobotComment(changeNotes, inReplyTo).isPresent()) {
- throw new BadRequestException(
- String.format("Invalid inReplyTo, comment %s not found", inReplyTo));
- }
- }
-
private void checkRobotComments(
RevisionResource revision, Map<String, List<RobotCommentInput>> in)
throws BadRequestException, PatchListNotAvailableException {
@@ -786,18 +694,17 @@
for (RobotCommentInput c : e.getValue()) {
ensureRobotIdIsSet(c.robotId, commentPath);
ensureRobotRunIdIsSet(c.robotRunId, commentPath);
- ensureFixSuggestionsAreAddable(c.fixSuggestions, commentPath);
// Size is validated later, in CommentLimitsValidator.
}
}
- checkComments(revision, in);
+ commentsValidator.checkComments(revision, in);
}
private static void ensureRobotIdIsSet(String robotId, String commentPath)
throws BadRequestException {
if (robotId == null) {
throw new BadRequestException(
- String.format("robotId is missing for robot comment on %s", commentPath));
+ String.format("robotId is missing for comment on %s", commentPath));
}
}
@@ -805,131 +712,7 @@
throws BadRequestException {
if (robotRunId == null) {
throw new BadRequestException(
- String.format("robotRunId is missing for robot comment on %s", commentPath));
- }
- }
-
- private static void ensureFixSuggestionsAreAddable(
- List<FixSuggestionInfo> fixSuggestionInfos, String commentPath) throws BadRequestException {
- if (fixSuggestionInfos == null) {
- return;
- }
-
- for (FixSuggestionInfo fixSuggestionInfo : fixSuggestionInfos) {
- ensureDescriptionIsSet(commentPath, fixSuggestionInfo.description);
- ensureFixReplacementsAreAddable(commentPath, fixSuggestionInfo.replacements);
- }
- }
-
- private static void ensureDescriptionIsSet(String commentPath, String description)
- throws BadRequestException {
- if (description == null) {
- throw new BadRequestException(
- String.format(
- "A description is required for the suggested fix of the robot comment on %s",
- commentPath));
- }
- }
-
- private static void ensureFixReplacementsAreAddable(
- String commentPath, List<FixReplacementInfo> fixReplacementInfos) throws BadRequestException {
- ensureReplacementsArePresent(commentPath, fixReplacementInfos);
-
- for (FixReplacementInfo fixReplacementInfo : fixReplacementInfos) {
- ensureReplacementPathIsSetAndNotPatchsetLevel(commentPath, fixReplacementInfo.path);
- ensureRangeIsSet(commentPath, fixReplacementInfo.range);
- ensureRangeIsValid(commentPath, fixReplacementInfo.range);
- ensureReplacementStringIsSet(commentPath, fixReplacementInfo.replacement);
- }
-
- Map<String, List<FixReplacementInfo>> replacementsPerFilePath =
- fixReplacementInfos.stream().collect(groupingBy(fixReplacement -> fixReplacement.path));
- for (List<FixReplacementInfo> sameFileReplacements : replacementsPerFilePath.values()) {
- ensureRangesDoNotOverlap(commentPath, sameFileReplacements);
- }
- }
-
- private static void ensureReplacementsArePresent(
- String commentPath, List<FixReplacementInfo> fixReplacementInfos) throws BadRequestException {
- if (fixReplacementInfos == null || fixReplacementInfos.isEmpty()) {
- throw new BadRequestException(
- String.format(
- "At least one replacement is "
- + "required for the suggested fix of the robot comment on %s",
- commentPath));
- }
- }
-
- private static void ensureReplacementPathIsSetAndNotPatchsetLevel(
- String commentPath, String replacementPath) throws BadRequestException {
- if (replacementPath == null) {
- throw new BadRequestException(
- String.format(
- "A file path must be given for the replacement of the robot comment on %s",
- commentPath));
- }
- if (replacementPath.equals(PATCHSET_LEVEL)) {
- throw new BadRequestException(
- String.format(
- "A file path must not be %s for the replacement of the robot comment on %s",
- PATCHSET_LEVEL, commentPath));
- }
- }
-
- private static void ensureRangeIsSet(String commentPath, Range range) throws BadRequestException {
- if (range == null) {
- throw new BadRequestException(
- String.format(
- "A range must be given for the replacement of the robot comment on %s", commentPath));
- }
- }
-
- private static void ensureRangeIsValid(String commentPath, Range range)
- throws BadRequestException {
- if (range == null) {
- return;
- }
- if (!range.isValid()) {
- throw new BadRequestException(
- String.format(
- "Range (%s:%s - %s:%s) is not valid for the comment on %s",
- range.startLine,
- range.startCharacter,
- range.endLine,
- range.endCharacter,
- commentPath));
- }
- }
-
- private static void ensureReplacementStringIsSet(String commentPath, String replacement)
- throws BadRequestException {
- if (replacement == null) {
- throw new BadRequestException(
- String.format(
- "A content for replacement "
- + "must be indicated for the replacement of the robot comment on %s",
- commentPath));
- }
- }
-
- private static void ensureRangesDoNotOverlap(
- String commentPath, List<FixReplacementInfo> fixReplacementInfos) throws BadRequestException {
- List<Range> sortedRanges =
- fixReplacementInfos.stream()
- .map(fixReplacementInfo -> fixReplacementInfo.range)
- .sorted()
- .collect(toList());
-
- int previousEndLine = 0;
- int previousOffset = -1;
- for (Range range : sortedRanges) {
- if (range.startLine < previousEndLine
- || (range.startLine == previousEndLine && range.startCharacter < previousOffset)) {
- throw new BadRequestException(
- String.format("Replacements overlap for the robot comment on %s", commentPath));
- }
- previousEndLine = range.endLine;
- previousOffset = range.endCharacter;
+ String.format("robotRunId is missing for comment on %s", commentPath));
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
index a47e179..2be2fc4 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReviewOp.java
@@ -20,7 +20,6 @@
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.joining;
-import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;
import com.google.auto.value.AutoValue;
@@ -35,8 +34,6 @@
import com.google.common.collect.Table.Cell;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Comment;
-import com.google.gerrit.entities.FixReplacement;
-import com.google.gerrit.entities.FixSuggestion;
import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelTypes;
@@ -47,8 +44,6 @@
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
-import com.google.gerrit.extensions.common.FixReplacementInfo;
-import com.google.gerrit.extensions.common.FixSuggestionInfo;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.restapi.Url;
@@ -57,7 +52,6 @@
import com.google.gerrit.extensions.validators.CommentValidationFailure;
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.ChangeMessagesUtil;
-import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.DraftCommentsReader;
import com.google.gerrit.server.IdentifiedUser;
@@ -404,7 +398,8 @@
inputComment.side(),
inputComment.message,
inputComment.unresolved,
- parent);
+ parent,
+ CommentsUtil.createFixSuggestionsFromInput(inputComment.fixSuggestions));
} else {
// In ChangeUpdate#putDraftComment() the draft with the same ID will be deleted.
comment.writtenOn = Timestamp.from(ctx.getWhen());
@@ -510,39 +505,11 @@
robotComment.setLineNbrAndRange(robotCommentInput.line, robotCommentInput.range);
robotComment.tag = in.tag;
commentsUtil.setCommentCommitId(robotComment, ctx.getChange(), ps);
- robotComment.fixSuggestions = createFixSuggestionsFromInput(robotCommentInput.fixSuggestions);
+ robotComment.fixSuggestions =
+ CommentsUtil.createFixSuggestionsFromInput(robotCommentInput.fixSuggestions);
return robotComment;
}
- private ImmutableList<FixSuggestion> createFixSuggestionsFromInput(
- List<FixSuggestionInfo> fixSuggestionInfos) {
- if (fixSuggestionInfos == null) {
- return ImmutableList.of();
- }
-
- ImmutableList.Builder<FixSuggestion> fixSuggestions =
- ImmutableList.builderWithExpectedSize(fixSuggestionInfos.size());
- for (FixSuggestionInfo fixSuggestionInfo : fixSuggestionInfos) {
- fixSuggestions.add(createFixSuggestionFromInput(fixSuggestionInfo));
- }
- return fixSuggestions.build();
- }
-
- private FixSuggestion createFixSuggestionFromInput(FixSuggestionInfo fixSuggestionInfo) {
- List<FixReplacement> fixReplacements = toFixReplacements(fixSuggestionInfo.replacements);
- String fixId = ChangeUtil.messageUuid();
- return new FixSuggestion(fixId, fixSuggestionInfo.description, fixReplacements);
- }
-
- private List<FixReplacement> toFixReplacements(List<FixReplacementInfo> fixReplacementInfos) {
- return fixReplacementInfos.stream().map(this::toFixReplacement).collect(toList());
- }
-
- private FixReplacement toFixReplacement(FixReplacementInfo fixReplacementInfo) {
- Comment.Range range = new Comment.Range(fixReplacementInfo.range);
- return new FixReplacement(fixReplacementInfo.path, range, fixReplacementInfo.replacement);
- }
-
private Set<CommentSetEntry> readExistingComments(ChangeContext ctx) {
return commentsUtil.publishedHumanCommentsByChange(ctx.getNotes()).stream()
.map(CommentSetEntry::create)
diff --git a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
index 7f4b10f..9adf963 100644
--- a/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
+++ b/java/com/google/gerrit/server/restapi/change/ReplyAttentionSetUpdates.java
@@ -163,7 +163,8 @@
commentInput.side(),
commentInput.message,
commentInput.unresolved,
- commentInput.inReplyTo));
+ commentInput.inReplyTo,
+ CommentsUtil.createFixSuggestionsFromInput(commentInput.fixSuggestions)));
}
}
List<HumanComment> drafts = new ArrayList<>();
diff --git a/java/com/google/gerrit/server/restapi/change/RobotComments.java b/java/com/google/gerrit/server/restapi/change/RobotComments.java
index 9f81d0a..e02a39f 100644
--- a/java/com/google/gerrit/server/restapi/change/RobotComments.java
+++ b/java/com/google/gerrit/server/restapi/change/RobotComments.java
@@ -27,6 +27,7 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
+@Deprecated
@Singleton
public class RobotComments implements ChildCollection<RevisionResource, RobotCommentResource> {
private final DynamicMap<RestView<RobotCommentResource>> views;
diff --git a/java/com/google/gerrit/testing/TestCommentHelper.java b/java/com/google/gerrit/testing/TestCommentHelper.java
index 400b559..7db26e3 100644
--- a/java/com/google/gerrit/testing/TestCommentHelper.java
+++ b/java/com/google/gerrit/testing/TestCommentHelper.java
@@ -21,6 +21,7 @@
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.Comment.Range;
@@ -143,6 +144,22 @@
return in;
}
+ public static CommentInput createCommentInputWithMandatoryFields(String path) {
+ CommentInput in = new CommentInput();
+ in.message = "nit: trailing whitespace";
+ in.path = path;
+ return in;
+ }
+
+ public static CommentInput createCommentInput(
+ String path, FixSuggestionInfo... fixSuggestionInfos) {
+ CommentInput in = new CommentInput();
+ in.message = "nit: trailing whitespace";
+ in.path = path;
+ in.fixSuggestions = Arrays.asList(fixSuggestionInfos);
+ return in;
+ }
+
public void addRobotComment(String targetChangeId, RobotCommentInput robotCommentInput)
throws Exception {
addRobotComment(targetChangeId, robotCommentInput, "robot comment test");
@@ -174,4 +191,23 @@
reviewInput.tag = ChangeMessagesUtil.AUTOGENERATED_TAG_PREFIX;
return reviewInput;
}
+
+ public void addComment(String targetChangeId, CommentInput commentInput) throws Exception {
+ addComment(targetChangeId, commentInput, "comment test");
+ }
+
+ public void addComment(String targetChangeId, CommentInput commentInput, String message)
+ throws Exception {
+ ReviewInput reviewInput = createReviewInput(commentInput, message);
+ gApi.changes().id(targetChangeId).current().review(reviewInput);
+ }
+
+ private ReviewInput createReviewInput(CommentInput commentInput, String message) {
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.comments =
+ Collections.singletonMap(commentInput.path, ImmutableList.of(commentInput));
+ reviewInput.message = message;
+ reviewInput.tag = ChangeMessagesUtil.AUTOGENERATED_TAG_PREFIX;
+ return reviewInput;
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/BUILD b/javatests/com/google/gerrit/acceptance/api/revision/BUILD
index 517b041..9c6584e 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/BUILD
+++ b/javatests/com/google/gerrit/acceptance/api/revision/BUILD
@@ -4,7 +4,10 @@
srcs = [f],
group = f[:f.index(".")],
labels = ["api"],
- deps = [":revision-diff-it"],
+ deps = [
+ ":revision-diff-it",
+ "//javatests/com/google/gerrit/acceptance/server/change:util",
+ ],
) for f in glob(["*IT.java"])]
# This is needed because RevisionDiffIT has subclasses that depend on it
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/CommentWithFixIT.java b/javatests/com/google/gerrit/acceptance/api/revision/CommentWithFixIT.java
new file mode 100644
index 0000000..f4fb996
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/api/revision/CommentWithFixIT.java
@@ -0,0 +1,1369 @@
+// Copyright (C) 2024 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.api.revision;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.server.change.CommentsUtil.createFixReplacementInfo;
+import static com.google.gerrit.acceptance.server.change.CommentsUtil.createFixSuggestionInfo;
+import static com.google.gerrit.acceptance.server.change.CommentsUtil.createRange;
+import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
+import static com.google.gerrit.extensions.common.testing.CommentInfoSubject.assertThatList;
+import static com.google.gerrit.extensions.common.testing.DiffInfoSubject.assertThat;
+import static com.google.gerrit.extensions.common.testing.EditInfoSubject.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.util.stream.Collectors.toList;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Patch;
+import com.google.gerrit.extensions.api.changes.PublishChangeEditInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
+import com.google.gerrit.extensions.common.ChangeType;
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.DiffInfo;
+import com.google.gerrit.extensions.common.DiffInfo.IntraLineStatus;
+import com.google.gerrit.extensions.common.EditInfo;
+import com.google.gerrit.extensions.common.FixReplacementInfo;
+import com.google.gerrit.extensions.common.FixSuggestionInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.BinaryResult;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.testing.BinaryResultSubject;
+import com.google.gerrit.testing.TestCommentHelper;
+import com.google.inject.Inject;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import org.junit.Before;
+import org.junit.Test;
+
+public class CommentWithFixIT extends AbstractDaemonTest {
+ @Inject private TestCommentHelper testCommentHelper;
+ @Inject private ChangeOperations changeOperations;
+ @Inject private AccountOperations accountOperations;
+ @Inject private RequestScopeOperations requestScopeOperations;
+
+ private static final String PLAIN_TEXT_CONTENT_TYPE = "text/plain";
+ private static final String GERRIT_COMMIT_MESSAGE_TYPE = "text/x-gerrit-commit-message";
+
+ private static final String FILE_NAME = "file_to_fix.txt";
+ private static final String FILE_NAME2 = "another_file_to_fix.txt";
+ private static final String FILE_NAME3 = "file_without_newline_at_end.txt";
+ private static final String FILE_CONTENT =
+ "First line\nSecond line\nThird line\nFourth line\nFifth line\nSixth line"
+ + "\nSeventh line\nEighth line\nNinth line\nTenth line\n";
+ private static final String FILE_CONTENT2 = "1st line\n2nd line\n3rd line\n";
+ private static final String FILE_CONTENT3 = "1st line\n2nd line";
+ private String changeId;
+ private String commitId;
+ private FixReplacementInfo fixReplacementInfo;
+ private FixSuggestionInfo fixSuggestionInfo;
+ private CommentInput withFixCommentInput;
+
+ @Before
+ public void setUp() throws Exception {
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Provide files which can be used for fixes",
+ ImmutableMap.of(
+ FILE_NAME, FILE_CONTENT, FILE_NAME2, FILE_CONTENT2, FILE_NAME3, FILE_CONTENT3));
+ PushOneCommit.Result changeResult = push.to("refs/for/master");
+ changeId = changeResult.getChangeId();
+ commitId = changeResult.getCommit().getName();
+
+ fixReplacementInfo = createFixReplacementInfo();
+ fixSuggestionInfo = createFixSuggestionInfo(fixReplacementInfo);
+ withFixCommentInput = TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo);
+ }
+
+ @Test
+ public void fixSuggestionCannotPointToPatchsetLevel() throws Exception {
+ CommentInput input = TestCommentHelper.createCommentInput(FILE_NAME);
+ FixReplacementInfo brokenFixReplacement = createFixReplacementInfo();
+ brokenFixReplacement.path = PATCHSET_LEVEL;
+ input.fixSuggestions = ImmutableList.of(createFixSuggestionInfo(brokenFixReplacement));
+ BadRequestException ex =
+ assertThrows(
+ BadRequestException.class, () -> testCommentHelper.addComment(changeId, input));
+ assertThat(ex.getMessage()).contains("file path must not be " + PATCHSET_LEVEL);
+ }
+
+ @Test
+ public void hugeCommentIsRejected() {
+ int defaultSizeLimit = 1 << 20;
+ fixReplacementInfo.replacement = getStringFor(defaultSizeLimit + 1);
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> testCommentHelper.addComment(changeId, withFixCommentInput));
+ assertThat(thrown).hasMessageThat().contains("limit");
+ }
+
+ @Test
+ public void reasonablyLargeCommentIsAccepted() throws Exception {
+ int defaultSizeLimit = 1 << 10;
+ // Allow for a few hundred bytes in other fields.
+ fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - 666);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+ assertThat(commentInfos).hasSize(1);
+ }
+
+ @Test
+ @GerritConfig(name = "change.commentSizeLimit", value = "0")
+ public void zeroForMaximumAllowedSizeOfCommentRemovesRestriction() throws Exception {
+ int defaultSizeLimit = 1 << 10;
+ fixReplacementInfo.replacement = getStringFor(2 * defaultSizeLimit);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+ assertThat(commentInfos).hasSize(1);
+ }
+
+ @Test
+ @GerritConfig(name = "change.commentSizeLimit", value = "-1")
+ public void negativeValueForMaximumAllowedSizeOfCommentRemovesRestriction() throws Exception {
+ int defaultSizeLimit = 1 << 20;
+ fixReplacementInfo.replacement = getStringFor(2 * defaultSizeLimit);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+ assertThat(commentInfos).hasSize(1);
+ }
+
+ @Test
+ public void addedFixSuggestionCanBeRetrieved() throws Exception {
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ assertThatList(commentInfos).onlyElement().onlyFixSuggestion().isNotNull();
+ }
+
+ @Test
+ public void fixIdIsGeneratedForFixSuggestion() throws Exception {
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ assertThatList(commentInfos).onlyElement().onlyFixSuggestion().fixId().isNotEmpty();
+ assertThatList(commentInfos)
+ .onlyElement()
+ .onlyFixSuggestion()
+ .fixId()
+ .isNotEqualTo(fixSuggestionInfo.fixId);
+ }
+
+ @Test
+ public void descriptionOfFixSuggestionIsAcceptedAsIs() throws Exception {
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ assertThatList(commentInfos)
+ .onlyElement()
+ .onlyFixSuggestion()
+ .description()
+ .isEqualTo(fixSuggestionInfo.description);
+ }
+
+ @Test
+ public void descriptionOfFixSuggestionIsMandatory() {
+ fixSuggestionInfo.description = null;
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> testCommentHelper.addComment(changeId, withFixCommentInput));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "A description is required for the suggested fix of the comment on %s",
+ withFixCommentInput.path));
+ }
+
+ @Test
+ public void addedFixReplacementCanBeRetrieved() throws Exception {
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ assertThatList(commentInfos).onlyElement().onlyFixSuggestion().onlyReplacement().isNotNull();
+ }
+
+ @Test
+ public void fixReplacementsAreMandatory() {
+ fixSuggestionInfo.replacements = Collections.emptyList();
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> testCommentHelper.addComment(changeId, withFixCommentInput));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "At least one replacement is required"
+ + " for the suggested fix of the comment on %s",
+ withFixCommentInput.path));
+ }
+
+ @Test
+ public void pathOfFixReplacementIsAcceptedAsIs() throws Exception {
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+
+ assertThatList(commentInfos)
+ .onlyElement()
+ .onlyFixSuggestion()
+ .onlyReplacement()
+ .path()
+ .isEqualTo(fixReplacementInfo.path);
+ }
+
+ @Test
+ public void pathOfFixReplacementIsMandatory() {
+ fixReplacementInfo.path = null;
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> testCommentHelper.addComment(changeId, withFixCommentInput));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "A file path must be given for the replacement of the comment on %s",
+ withFixCommentInput.path));
+ }
+
+ @Test
+ public void rangeOfFixReplacementIsAcceptedAsIs() throws Exception {
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+
+ assertThatList(commentInfos)
+ .onlyElement()
+ .onlyFixSuggestion()
+ .onlyReplacement()
+ .range()
+ .isEqualTo(fixReplacementInfo.range);
+ }
+
+ @Test
+ public void rangeOfFixReplacementIsMandatory() {
+ fixReplacementInfo.range = null;
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> testCommentHelper.addComment(changeId, withFixCommentInput));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "A range must be given for the replacement of the comment on %s",
+ withFixCommentInput.path));
+ }
+
+ @Test
+ public void rangeOfFixReplacementNeedsToBeValid() {
+ fixReplacementInfo.range = createRange(13, 9, 5, 10);
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> testCommentHelper.addComment(changeId, withFixCommentInput));
+ assertThat(thrown).hasMessageThat().contains("Range (13:9 - 5:10)");
+ }
+
+ @Test
+ public void commentWithRangeAndLine_lineIsIgnored() throws Exception {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 1);
+ fixReplacementInfo1.replacement = "First modification\n";
+
+ withFixCommentInput.line = 1;
+ withFixCommentInput.range = createRange(2, 0, 3, 1);
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> comments = getComments();
+ assertThat(comments.get(0).line).isEqualTo(3);
+ }
+
+ @Test
+ public void rangesOfFixReplacementsOfSameFixSuggestionForSameFileMayNotOverlap() {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 1);
+ fixReplacementInfo1.replacement = "First modification\n";
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME;
+ fixReplacementInfo2.range = createRange(3, 0, 4, 0);
+ fixReplacementInfo2.replacement = "Second modification\n";
+
+ FixSuggestionInfo fixSuggestionInfo =
+ createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2);
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> testCommentHelper.addComment(changeId, withFixCommentInput));
+ assertThat(thrown).hasMessageThat().contains("overlap");
+ }
+
+ @Test
+ public void rangesOfFixReplacementsOfSameFixSuggestionForDifferentFileMayOverlap()
+ throws Exception {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 1);
+ fixReplacementInfo1.replacement = "First modification\n";
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME2;
+ fixReplacementInfo2.range = createRange(3, 0, 4, 0);
+ fixReplacementInfo2.replacement = "Second modification\n";
+
+ FixSuggestionInfo fixSuggestionInfo =
+ createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2);
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+ assertThatList(commentInfos).onlyElement().fixSuggestions().hasSize(1);
+ }
+
+ @Test
+ public void rangesOfFixReplacementsOfDifferentFixSuggestionsForSameFileMayOverlap()
+ throws Exception {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 1);
+ fixReplacementInfo1.replacement = "First modification\n";
+ FixSuggestionInfo fixSuggestionInfo1 = createFixSuggestionInfo(fixReplacementInfo1);
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME;
+ fixReplacementInfo2.range = createRange(3, 0, 4, 0);
+ fixReplacementInfo2.replacement = "Second modification\n";
+ FixSuggestionInfo fixSuggestionInfo2 = createFixSuggestionInfo(fixReplacementInfo2);
+
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo1, fixSuggestionInfo2);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+ assertThatList(commentInfos).onlyElement().fixSuggestions().hasSize(2);
+ }
+
+ @Test
+ public void fixReplacementsDoNotNeedToBeOrderedAccordingToRange() throws Exception {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 0);
+ fixReplacementInfo1.replacement = "First modification\n";
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME;
+ fixReplacementInfo2.range = createRange(3, 0, 4, 0);
+ fixReplacementInfo2.replacement = "Second modification\n";
+
+ FixReplacementInfo fixReplacementInfo3 = new FixReplacementInfo();
+ fixReplacementInfo3.path = FILE_NAME;
+ fixReplacementInfo3.range = createRange(4, 0, 5, 0);
+ fixReplacementInfo3.replacement = "Third modification\n";
+
+ FixSuggestionInfo fixSuggestionInfo =
+ createFixSuggestionInfo(fixReplacementInfo2, fixReplacementInfo1, fixReplacementInfo3);
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+ assertThatList(commentInfos).onlyElement().onlyFixSuggestion().replacements().hasSize(3);
+ }
+
+ @Test
+ public void replacementStringOfFixReplacementIsAcceptedAsIs() throws Exception {
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+
+ assertThatList(commentInfos)
+ .onlyElement()
+ .onlyFixSuggestion()
+ .onlyReplacement()
+ .replacement()
+ .isEqualTo(fixReplacementInfo.replacement);
+ }
+
+ @Test
+ public void replacementStringOfFixReplacementIsMandatory() {
+ fixReplacementInfo.replacement = null;
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> testCommentHelper.addComment(changeId, withFixCommentInput));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains(
+ String.format(
+ "A content for replacement must be "
+ + "indicated for the replacement of the comment on %s",
+ withFixCommentInput.path));
+ }
+
+ @Test
+ public void storedFixWithinALineCanBeApplied() throws Exception {
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "First line\nSecond line\nTModified contentrd line\nFourth line\nFifth line\n"
+ + "Sixth line\nSeventh line\nEighth line\nNinth line\nTenth line\n");
+ }
+
+ @Test
+ public void applyStoredFixAfterUpdatingPreferredEmail() throws Exception {
+ String emailOne = "email1@example.com";
+ Account.Id testUser = accountOperations.newAccount().preferredEmail(emailOne).create();
+
+ // Create change
+ Change.Id change =
+ changeOperations
+ .newChange()
+ .project(project)
+ .file(FILE_NAME)
+ .content(FILE_CONTENT)
+ .owner(testUser)
+ .create();
+
+ // Add Robot Comment to the change
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+ testCommentHelper.addComment(project + "~" + change.get(), withFixCommentInput);
+
+ // Change preferred email for the user
+ String emailTwo = "email2@example.com";
+ accountOperations.account(testUser).forUpdate().preferredEmail(emailTwo).update();
+ requestScopeOperations.setApiUser(testUser);
+
+ // Fetch Fix ID
+ List<CommentInfo> commentInfoList = gApi.changes().id(change.get()).current().commentsAsList();
+
+ List<String> fixIds = getFixIds(commentInfoList);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ // Apply fix
+ gApi.changes().id(change.get()).current().applyFix(fixId);
+
+ EditInfo editInfo = gApi.changes().id(change.get()).edit().get().orElseThrow();
+ assertThat(editInfo.commit.committer.email).isEqualTo(emailOne);
+ }
+
+ @Test
+ public void storedFixSpanningMultipleLinesCanBeApplied() throws Exception {
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content\n5";
+ fixReplacementInfo.range = createRange(3, 2, 5, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "First line\nSecond line\nThModified content\n5th line\nSixth line\nSeventh line\n"
+ + "Eighth line\nNinth line\nTenth line\n");
+ }
+
+ @Test
+ public void storedFixWithTwoCloseReplacementsOnSameFileCanBeApplied() throws Exception {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 0);
+ fixReplacementInfo1.replacement = "First modification\n";
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME;
+ fixReplacementInfo2.range = createRange(3, 0, 4, 0);
+ fixReplacementInfo2.replacement = "Some other modified content\n";
+
+ FixSuggestionInfo fixSuggestionInfo =
+ createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2);
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "First line\nFirst modification\nSome other modified content\nFourth line\nFifth line\n"
+ + "Sixth line\nSeventh line\nEighth line\nNinth line\nTenth line\n");
+ }
+
+ @Test
+ public void twoStoredFixesOnSameFileCanBeApplied() throws Exception {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 0);
+ fixReplacementInfo1.replacement = "First modification\n";
+ FixSuggestionInfo fixSuggestionInfo1 = createFixSuggestionInfo(fixReplacementInfo1);
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME;
+ fixReplacementInfo2.range = createRange(8, 0, 9, 0);
+ fixReplacementInfo2.replacement = "Some other modified content\n";
+ FixSuggestionInfo fixSuggestionInfo2 = createFixSuggestionInfo(fixReplacementInfo2);
+
+ CommentInput commentInput1 =
+ TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo1);
+ CommentInput commentInput2 =
+ TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo2);
+ testCommentHelper.addComment(changeId, commentInput1);
+ testCommentHelper.addComment(changeId, commentInput2);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ gApi.changes().id(changeId).current().applyFix(fixIds.get(0));
+ gApi.changes().id(changeId).current().applyFix(fixIds.get(1));
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "First line\nFirst modification\nThird line\nFourth line\nFifth line\nSixth line\n"
+ + "Seventh line\nSome other modified content\nNinth line\nTenth line\n");
+ }
+
+ @Test
+ public void twoConflictingStoredFixesOnSameFileCannotBeApplied() throws Exception {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 1);
+ fixReplacementInfo1.replacement = "First modification\n";
+ FixSuggestionInfo fixSuggestionInfo1 = createFixSuggestionInfo(fixReplacementInfo1);
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME;
+ fixReplacementInfo2.range = createRange(3, 0, 4, 0);
+ fixReplacementInfo2.replacement = "Some other modified content\n";
+ FixSuggestionInfo fixSuggestionInfo2 = createFixSuggestionInfo(fixReplacementInfo2);
+
+ CommentInput commentInput1 =
+ TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo1);
+ CommentInput commentInput2 =
+ TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo2);
+ testCommentHelper.addComment(changeId, commentInput1);
+ testCommentHelper.addComment(changeId, commentInput2);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ gApi.changes().id(changeId).current().applyFix(fixIds.get(0));
+ ResourceConflictException thrown =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeId).current().applyFix(fixIds.get(1)));
+ assertThat(thrown).hasMessageThat().contains("merge");
+ }
+
+ @Test
+ public void twoStoredFixesOfSameCommentCanBeApplied() throws Exception {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 0);
+ fixReplacementInfo1.replacement = "First modification\n";
+ FixSuggestionInfo fixSuggestionInfo1 = createFixSuggestionInfo(fixReplacementInfo1);
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME;
+ fixReplacementInfo2.range = createRange(8, 0, 9, 0);
+ fixReplacementInfo2.replacement = "Some other modified content\n";
+ FixSuggestionInfo fixSuggestionInfo2 = createFixSuggestionInfo(fixReplacementInfo2);
+
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo1, fixSuggestionInfo2);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ gApi.changes().id(changeId).current().applyFix(fixIds.get(0));
+ gApi.changes().id(changeId).current().applyFix(fixIds.get(1));
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "First line\nFirst modification\nThird line\nFourth line\nFifth line\nSixth line\n"
+ + "Seventh line\nSome other modified content\nNinth line\nTenth line\n");
+ }
+
+ @Test
+ public void storedFixReferringToDifferentFileThanCommentCanBeApplied() throws Exception {
+ fixReplacementInfo.path = FILE_NAME2;
+ fixReplacementInfo.range = createRange(2, 0, 3, 0);
+ fixReplacementInfo.replacement = "Modified content\n";
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME2);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo("1st line\nModified content\n3rd line\n");
+ }
+
+ @Test
+ public void storedFixInvolvingTwoFilesCanBeApplied() throws Exception {
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = FILE_NAME;
+ fixReplacementInfo1.range = createRange(2, 0, 3, 0);
+ fixReplacementInfo1.replacement = "First modification\n";
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME2;
+ fixReplacementInfo2.range = createRange(1, 0, 2, 0);
+ fixReplacementInfo2.replacement = "Different file modification\n";
+
+ FixSuggestionInfo fixSuggestionInfo =
+ createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2);
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "First line\nFirst modification\nThird line\nFourth line\nFifth line\nSixth line\n"
+ + "Seventh line\nEighth line\nNinth line\nTenth line\n");
+ Optional<BinaryResult> file2 = gApi.changes().id(changeId).edit().getFile(FILE_NAME2);
+ BinaryResultSubject.assertThat(file2)
+ .value()
+ .asString()
+ .isEqualTo("Different file modification\n2nd line\n3rd line\n");
+ }
+
+ @Test
+ public void storedFixReferringToNonExistentFileCannotBeApplied() throws Exception {
+ fixReplacementInfo.path = "a_non_existent_file.txt";
+ fixReplacementInfo.range = createRange(1, 0, 2, 0);
+ fixReplacementInfo.replacement = "Modified content\n";
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> gApi.changes().id(changeId).current().applyFix(fixId));
+ }
+
+ @Test
+ public void storedFixOnPreviousPatchSetWithoutChangeEditCannotBeApplied() throws Exception {
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ // Remember patch set and add another one.
+ String previousRevision = gApi.changes().id(changeId).get().currentRevision;
+ amendChange(changeId);
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ ResourceConflictException thrown =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeId).revision(previousRevision).applyFix(fixId));
+ assertThat(thrown).hasMessageThat().contains("current");
+ }
+
+ @Test
+ public void storedFixOnPreviousPatchSetWithExistingChangeEditCanBeApplied() throws Exception {
+ // Create an empty change edit.
+ gApi.changes().id(changeId).edit().create();
+
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ // Remember patch set and add another one.
+ String previousRevision = gApi.changes().id(changeId).get().currentRevision;
+ amendChange(changeId);
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ EditInfo editInfo = gApi.changes().id(changeId).revision(previousRevision).applyFix(fixId);
+
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo(
+ "First line\nSecond line\nTModified contentrd line\nFourth line\nFifth line\n"
+ + "Sixth line\nSeventh line\nEighth line\nNinth line\nTenth line\n");
+ assertThat(editInfo).baseRevision().isEqualTo(previousRevision);
+ }
+
+ @Test
+ public void storedFixOnCurrentPatchSetWithChangeEditOnPreviousPatchSetCannotBeApplied()
+ throws Exception {
+ // Create an empty change edit.
+ gApi.changes().id(changeId).edit().create();
+
+ // Add another patch set.
+ amendChange(changeId);
+
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ ResourceConflictException thrown =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeId).current().applyFix(fixId));
+ assertThat(thrown).hasMessageThat().contains("based");
+ }
+
+ @Test
+ public void storedFixDoesNotModifyCommitMessageOfChangeEdit() throws Exception {
+ String changeEditCommitMessage =
+ "This is the commit message of the change edit.\n\nChange-Id: " + changeId + "\n";
+ gApi.changes().id(changeId).edit().modifyCommitMessage(changeEditCommitMessage);
+
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ String fixId = Iterables.getOnlyElement(getFixIds(getComments()));
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
+ assertThat(commitMessage).isEqualTo(changeEditCommitMessage);
+ }
+
+ @Test
+ public void storedFixOnCommitMessageCanBeApplied() throws Exception {
+ // Set a dedicated commit message.
+ String footer = "\nChange-Id: " + changeId + "\n";
+ String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n" + footer;
+ gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
+ gApi.changes().id(changeId).edit().publish();
+
+ withFixCommentInput.path = Patch.COMMIT_MSG;
+ fixReplacementInfo.path = Patch.COMMIT_MSG;
+ fixReplacementInfo.replacement = "Modified line\n";
+ fixReplacementInfo.range = createRange(7, 0, 8, 0);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ String fixId = Iterables.getOnlyElement(getFixIds(getComments()));
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
+ assertThat(commitMessage).isEqualTo("Modified line\nLine 2 of commit message\n" + footer);
+ }
+
+ @Test
+ public void storedFixOnHeaderPartOfCommitMessageCannotBeApplied() throws Exception {
+ // Set a dedicated commit message.
+ String footer = "Change-Id: " + changeId;
+ String originalCommitMessage =
+ "Line 1 of commit message\nLine 2 of commit message\n" + "\n" + footer + "\n";
+ gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
+ gApi.changes().id(changeId).edit().publish();
+
+ withFixCommentInput.path = Patch.COMMIT_MSG;
+ fixReplacementInfo.path = Patch.COMMIT_MSG;
+ fixReplacementInfo.replacement = "Modified line\n";
+ fixReplacementInfo.range = createRange(1, 0, 2, 0);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ String fixId = Iterables.getOnlyElement(getFixIds(getComments()));
+
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeId).current().applyFix(fixId));
+ assertThat(exception).hasMessageThat().contains("header");
+ }
+
+ @Test
+ public void storedFixContainingSeveralModificationsOfCommitMessageCanBeApplied()
+ throws Exception {
+ // Set a dedicated commit message.
+ String footer = "\nChange-Id: " + changeId + "\n";
+ String originalCommitMessage =
+ "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n" + footer;
+ gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
+ gApi.changes().id(changeId).edit().publish();
+
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = Patch.COMMIT_MSG;
+ fixReplacementInfo1.range = createRange(7, 0, 8, 0);
+ fixReplacementInfo1.replacement = "Modified line 1\n";
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = Patch.COMMIT_MSG;
+ fixReplacementInfo2.range = createRange(9, 0, 10, 0);
+ fixReplacementInfo2.replacement = "Modified line 3\n";
+
+ FixSuggestionInfo fixSuggestionInfo =
+ createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2);
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
+ withFixCommentInput.path = Patch.COMMIT_MSG;
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ String fixId = Iterables.getOnlyElement(getFixIds(getComments()));
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
+ assertThat(commitMessage)
+ .isEqualTo("Modified line 1\nLine 2 of commit message\nModified line 3\n" + footer);
+ }
+
+ @Test
+ public void storedFixModifyingTheCommitMessageAndAFileCanBeApplied() throws Exception {
+ // Set a dedicated commit message.
+ String footer = "\nChange-Id: " + changeId + "\n";
+ String originalCommitMessage = "Line 1 of commit message\nLine 2 of commit message\n" + footer;
+ gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
+ gApi.changes().id(changeId).edit().publish();
+
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = Patch.COMMIT_MSG;
+ fixReplacementInfo1.range = createRange(7, 0, 8, 0);
+ fixReplacementInfo1.replacement = "Modified line 1\n";
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = FILE_NAME2;
+ fixReplacementInfo2.range = createRange(1, 0, 2, 0);
+ fixReplacementInfo2.replacement = "File modification\n";
+
+ FixSuggestionInfo fixSuggestionInfo =
+ createFixSuggestionInfo(fixReplacementInfo1, fixReplacementInfo2);
+ withFixCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ String fixId = Iterables.getOnlyElement(getFixIds(getComments()));
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
+ assertThat(commitMessage).isEqualTo("Modified line 1\nLine 2 of commit message\n" + footer);
+ Optional<BinaryResult> file = gApi.changes().id(changeId).edit().getFile(FILE_NAME2);
+ BinaryResultSubject.assertThat(file)
+ .value()
+ .asString()
+ .isEqualTo("File modification\n2nd line\n3rd line\n");
+ }
+
+ @Test
+ public void twoStoredFixesOnCommitMessageCanBeAppliedOneAfterTheOther() throws Exception {
+ // Set a dedicated commit message.
+ String footer = "\nChange-Id: " + changeId + "\n";
+ String originalCommitMessage =
+ "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n" + footer;
+ gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
+ gApi.changes().id(changeId).edit().publish();
+
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = Patch.COMMIT_MSG;
+ fixReplacementInfo1.range = createRange(7, 0, 8, 0);
+ fixReplacementInfo1.replacement = "Modified line 1\n";
+ FixSuggestionInfo fixSuggestionInfo1 = createFixSuggestionInfo(fixReplacementInfo1);
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = Patch.COMMIT_MSG;
+ fixReplacementInfo2.range = createRange(9, 0, 10, 0);
+ fixReplacementInfo2.replacement = "Modified line 3\n";
+ FixSuggestionInfo fixSuggestionInfo2 = createFixSuggestionInfo(fixReplacementInfo2);
+
+ CommentInput commentInput1 =
+ TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo1);
+ CommentInput commentInput2 =
+ TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo2);
+ testCommentHelper.addComment(changeId, commentInput1);
+ testCommentHelper.addComment(changeId, commentInput2);
+ List<String> fixIds = getFixIds(getComments());
+
+ gApi.changes().id(changeId).current().applyFix(fixIds.get(0));
+ gApi.changes().id(changeId).current().applyFix(fixIds.get(1));
+
+ String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage();
+ assertThat(commitMessage)
+ .isEqualTo("Modified line 1\nLine 2 of commit message\nModified line 3\n" + footer);
+ }
+
+ @Test
+ public void twoConflictingStoredFixesOnCommitMessageCanNotBeAppliedOneAfterTheOther()
+ throws Exception {
+ // Set a dedicated commit message.
+ String footer = "Change-Id: " + changeId;
+ String originalCommitMessage =
+ "Line 1 of commit message\nLine 2 of commit message\nLine 3 of commit message\n\n"
+ + footer
+ + "\n";
+ gApi.changes().id(changeId).edit().modifyCommitMessage(originalCommitMessage);
+ gApi.changes().id(changeId).edit().publish();
+
+ FixReplacementInfo fixReplacementInfo1 = new FixReplacementInfo();
+ fixReplacementInfo1.path = Patch.COMMIT_MSG;
+ fixReplacementInfo1.range = createRange(7, 0, 8, 0);
+ fixReplacementInfo1.replacement = "Modified line 1\n";
+ FixSuggestionInfo fixSuggestionInfo1 = createFixSuggestionInfo(fixReplacementInfo1);
+
+ FixReplacementInfo fixReplacementInfo2 = new FixReplacementInfo();
+ fixReplacementInfo2.path = Patch.COMMIT_MSG;
+ fixReplacementInfo2.range = createRange(7, 0, 10, 0);
+ fixReplacementInfo2.replacement = "Differently modified line 1\n";
+ FixSuggestionInfo fixSuggestionInfo2 = createFixSuggestionInfo(fixReplacementInfo2);
+
+ CommentInput commentInput1 =
+ TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo1);
+ CommentInput commentInput2 =
+ TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo2);
+ testCommentHelper.addComment(changeId, commentInput1);
+ testCommentHelper.addComment(changeId, commentInput2);
+ List<String> fixIds = getFixIds(getComments());
+
+ gApi.changes().id(changeId).current().applyFix(fixIds.get(0));
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(changeId).current().applyFix(fixIds.get(1)));
+ }
+
+ @Test
+ public void applyingStoredFixTwiceIsIdempotent() throws Exception {
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ gApi.changes().id(changeId).current().applyFix(fixId);
+ String expectedEditCommit =
+ gApi.changes().id(changeId).edit().get().map(edit -> edit.commit.commit).orElse("");
+
+ // Apply the fix again.
+ gApi.changes().id(changeId).current().applyFix(fixId);
+
+ Optional<EditInfo> editInfo = gApi.changes().id(changeId).edit().get();
+ assertThat(editInfo).value().commit().commit().isEqualTo(expectedEditCommit);
+ }
+
+ @Test
+ public void nonExistentStoredFixCannotBeApplied() throws Exception {
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+ String nonExistentFixId = fixId + "_non-existent";
+
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> gApi.changes().id(changeId).current().applyFix(nonExistentFixId));
+ }
+
+ @Test
+ public void applyingStoredFixReturnsEditInfoForCreatedChangeEdit() throws Exception {
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ EditInfo editInfo = gApi.changes().id(changeId).current().applyFix(fixId);
+
+ Optional<EditInfo> expectedEditInfo = gApi.changes().id(changeId).edit().get();
+ String expectedEditCommit = expectedEditInfo.map(edit -> edit.commit.commit).orElse("");
+ assertThat(editInfo).commit().commit().isEqualTo(expectedEditCommit);
+ String expectedBaseRevision = expectedEditInfo.map(edit -> edit.baseRevision).orElse("");
+ assertThat(editInfo).baseRevision().isEqualTo(expectedBaseRevision);
+ }
+
+ @Test
+ public void applyingStoredFixOnTopOfChangeEditReturnsEditInfoForUpdatedChangeEdit()
+ throws Exception {
+ gApi.changes().id(changeId).edit().create();
+
+ fixReplacementInfo.path = FILE_NAME;
+ fixReplacementInfo.replacement = "Modified content";
+ fixReplacementInfo.range = createRange(3, 1, 3, 3);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ EditInfo editInfo = gApi.changes().id(changeId).current().applyFix(fixId);
+
+ Optional<EditInfo> expectedEditInfo = gApi.changes().id(changeId).edit().get();
+ String expectedEditCommit = expectedEditInfo.map(edit -> edit.commit.commit).orElse("");
+ assertThat(editInfo).commit().commit().isEqualTo(expectedEditCommit);
+ String expectedBaseRevision = expectedEditInfo.map(edit -> edit.baseRevision).orElse("");
+ assertThat(editInfo).baseRevision().isEqualTo(expectedBaseRevision);
+ }
+
+ @Test
+ public void previewStoredFixWithNonexistentFixId() throws Exception {
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> gApi.changes().id(changeId).current().getFixPreview("Non existing fixId"));
+ }
+
+ @Test
+ public void previewStoredFixForCommitMsg() throws Exception {
+ String footer = "Change-Id: " + changeId;
+ updateCommitMessage(
+ changeId,
+ "Commit title\n\nCommit message line 1\nLine 2\nLine 3\nLast line\n\n" + footer + "\n");
+ FixReplacementInfo commitMsgReplacement = new FixReplacementInfo();
+ commitMsgReplacement.path = Patch.COMMIT_MSG;
+ // The test assumes that the first 5 lines is a header.
+ // Line 10 has content "Line 2"
+ commitMsgReplacement.range = createRange(10, 0, 11, 0);
+ commitMsgReplacement.replacement = "New content\n";
+
+ FixSuggestionInfo commitMsgSuggestionInfo = createFixSuggestionInfo(commitMsgReplacement);
+ CommentInput commitMsgCommentInput =
+ TestCommentHelper.createCommentInput(Patch.COMMIT_MSG, commitMsgSuggestionInfo);
+ testCommentHelper.addComment(changeId, commitMsgCommentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ Map<String, DiffInfo> fixPreview = gApi.changes().id(changeId).current().getFixPreview(fixId);
+ assertThat(fixPreview).hasSize(1);
+ assertThat(fixPreview).containsKey(Patch.COMMIT_MSG);
+
+ DiffInfo diff = fixPreview.get(Patch.COMMIT_MSG);
+ assertThat(diff).metaA().name().isEqualTo(Patch.COMMIT_MSG);
+ assertThat(diff).metaA().contentType().isEqualTo(GERRIT_COMMIT_MESSAGE_TYPE);
+ assertThat(diff).metaB().name().isEqualTo(Patch.COMMIT_MSG);
+ assertThat(diff).metaB().contentType().isEqualTo(GERRIT_COMMIT_MESSAGE_TYPE);
+
+ assertThat(diff).content().element(0).commonLines().hasSize(9);
+ // Header has a dynamic content, do not check it
+ assertThat(diff).content().element(0).commonLines().element(6).isEqualTo("Commit title");
+ assertThat(diff).content().element(0).commonLines().element(7).isEqualTo("");
+ assertThat(diff)
+ .content()
+ .element(0)
+ .commonLines()
+ .element(8)
+ .isEqualTo("Commit message line 1");
+ assertThat(diff).content().element(1).linesOfA().containsExactly("Line 2");
+ assertThat(diff).content().element(1).linesOfB().containsExactly("New content");
+ assertThat(diff)
+ .content()
+ .element(2)
+ .commonLines()
+ .containsExactly("Line 3", "Last line", "", footer, "");
+ }
+
+ private void updateCommitMessage(String changeId, String newCommitMessage) throws Exception {
+ gApi.changes().id(changeId).edit().create();
+ gApi.changes().id(changeId).edit().modifyCommitMessage(newCommitMessage);
+ PublishChangeEditInput publishInput = new PublishChangeEditInput();
+ gApi.changes().id(changeId).edit().publish(publishInput);
+ }
+
+ @Test
+ public void previewStoredFixForNonExistingFile() throws Exception {
+ FixReplacementInfo replacement = new FixReplacementInfo();
+ replacement.path = "a_non_existent_file.txt";
+ replacement.range = createRange(1, 0, 2, 0);
+ replacement.replacement = "Modified content\n";
+
+ FixSuggestionInfo fixSuggestion = createFixSuggestionInfo(replacement);
+ CommentInput commentInput = TestCommentHelper.createCommentInput(FILE_NAME2, fixSuggestion);
+ testCommentHelper.addComment(changeId, commentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> gApi.changes().id(changeId).current().getFixPreview(fixId));
+ }
+
+ @Test
+ public void previewStoredFix() throws Exception {
+ FixReplacementInfo fixReplacementInfoFile1 = new FixReplacementInfo();
+ fixReplacementInfoFile1.path = FILE_NAME;
+ fixReplacementInfoFile1.replacement = "some replacement code";
+ fixReplacementInfoFile1.range = createRange(3, 9, 8, 4);
+
+ FixReplacementInfo fixReplacementInfoFile2 = new FixReplacementInfo();
+ fixReplacementInfoFile2.path = FILE_NAME2;
+ fixReplacementInfoFile2.replacement = "New line\n";
+ fixReplacementInfoFile2.range = createRange(2, 0, 2, 0);
+
+ fixSuggestionInfo = createFixSuggestionInfo(fixReplacementInfoFile1, fixReplacementInfoFile2);
+
+ withFixCommentInput = TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo);
+
+ testCommentHelper.addComment(changeId, withFixCommentInput);
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ Map<String, DiffInfo> fixPreview = gApi.changes().id(changeId).current().getFixPreview(fixId);
+ assertThat(fixPreview).hasSize(2);
+ assertThat(fixPreview).containsKey(FILE_NAME);
+ assertThat(fixPreview).containsKey(FILE_NAME2);
+
+ DiffInfo diff = fixPreview.get(FILE_NAME);
+ assertThat(diff).intralineStatus().isEqualTo(IntraLineStatus.OK);
+ assertThat(diff).webLinks().isNull();
+ assertThat(diff).binary().isNull();
+ assertThat(diff).diffHeader().isNull();
+ assertThat(diff).changeType().isEqualTo(ChangeType.MODIFIED);
+ assertThat(diff).metaA().totalLineCount().isEqualTo(11);
+ assertThat(diff).metaA().name().isEqualTo(FILE_NAME);
+ assertThat(diff).metaA().commitId().isEqualTo(commitId);
+ assertThat(diff).metaA().contentType().isEqualTo(PLAIN_TEXT_CONTENT_TYPE);
+ assertThat(diff).metaA().webLinks().isNull();
+ assertThat(diff).metaB().totalLineCount().isEqualTo(6);
+ assertThat(diff).metaB().name().isEqualTo(FILE_NAME);
+ assertThat(diff).metaB().commitId().isNull();
+ assertThat(diff).metaB().contentType().isEqualTo(PLAIN_TEXT_CONTENT_TYPE);
+ assertThat(diff).metaB().webLinks().isNull();
+
+ assertThat(diff).content().hasSize(3);
+ assertThat(diff)
+ .content()
+ .element(0)
+ .commonLines()
+ .containsExactly("First line", "Second line");
+ assertThat(diff).content().element(0).linesOfA().isNull();
+ assertThat(diff).content().element(0).linesOfB().isNull();
+
+ assertThat(diff).content().element(1).commonLines().isNull();
+ assertThat(diff)
+ .content()
+ .element(1)
+ .linesOfA()
+ .containsExactly(
+ "Third line", "Fourth line", "Fifth line", "Sixth line", "Seventh line", "Eighth line");
+ assertThat(diff)
+ .content()
+ .element(1)
+ .linesOfB()
+ .containsExactly("Third linsome replacement codeth line");
+
+ assertThat(diff)
+ .content()
+ .element(2)
+ .commonLines()
+ .containsExactly("Ninth line", "Tenth line", "");
+ assertThat(diff).content().element(2).linesOfA().isNull();
+ assertThat(diff).content().element(2).linesOfB().isNull();
+
+ DiffInfo diff2 = fixPreview.get(FILE_NAME2);
+ assertThat(diff2).intralineStatus().isEqualTo(IntraLineStatus.OK);
+ assertThat(diff2).webLinks().isNull();
+ assertThat(diff2).binary().isNull();
+ assertThat(diff2).diffHeader().isNull();
+ assertThat(diff2).changeType().isEqualTo(ChangeType.MODIFIED);
+ assertThat(diff2).metaA().totalLineCount().isEqualTo(4);
+ assertThat(diff2).metaA().name().isEqualTo(FILE_NAME2);
+ assertThat(diff2).metaA().commitId().isEqualTo(commitId);
+ assertThat(diff2).metaA().contentType().isEqualTo(PLAIN_TEXT_CONTENT_TYPE);
+ assertThat(diff2).metaA().webLinks().isNull();
+ assertThat(diff2).metaB().totalLineCount().isEqualTo(5);
+ assertThat(diff2).metaB().name().isEqualTo(FILE_NAME2);
+ assertThat(diff2).metaB().commitId().isNull();
+ assertThat(diff2).metaA().contentType().isEqualTo(PLAIN_TEXT_CONTENT_TYPE);
+ assertThat(diff2).metaB().webLinks().isNull();
+
+ assertThat(diff2).content().hasSize(3);
+ assertThat(diff2).content().element(0).commonLines().containsExactly("1st line");
+ assertThat(diff2).content().element(0).linesOfA().isNull();
+ assertThat(diff2).content().element(0).linesOfB().isNull();
+
+ assertThat(diff2).content().element(1).commonLines().isNull();
+ assertThat(diff2).content().element(1).linesOfA().isNull();
+ assertThat(diff2).content().element(1).linesOfB().containsExactly("New line");
+
+ assertThat(diff2)
+ .content()
+ .element(2)
+ .commonLines()
+ .containsExactly("2nd line", "3rd line", "");
+ assertThat(diff2).content().element(2).linesOfA().isNull();
+ assertThat(diff2).content().element(2).linesOfB().isNull();
+ }
+
+ @Test
+ public void previewStoredFixAddNewLineAtEnd() throws Exception {
+ FixReplacementInfo replacement = new FixReplacementInfo();
+ replacement.path = FILE_NAME3;
+ replacement.range = createRange(2, 8, 2, 8);
+ replacement.replacement = "\n";
+
+ FixSuggestionInfo fixSuggestion = createFixSuggestionInfo(replacement);
+ CommentInput commentInput = TestCommentHelper.createCommentInput(FILE_NAME3, fixSuggestion);
+ testCommentHelper.addComment(changeId, commentInput);
+
+ List<CommentInfo> commentInfos = getComments();
+
+ List<String> fixIds = getFixIds(commentInfos);
+ String fixId = Iterables.getOnlyElement(fixIds);
+
+ Map<String, DiffInfo> fixPreview = gApi.changes().id(changeId).current().getFixPreview(fixId);
+
+ assertThat(fixPreview).hasSize(1);
+ assertThat(fixPreview).containsKey(FILE_NAME3);
+
+ DiffInfo diff = fixPreview.get(FILE_NAME3);
+ assertThat(diff).metaA().totalLineCount().isEqualTo(2);
+ // Original file doesn't have EOL marker at the end of file.
+ // Due to the additional EOL mark diff has one additional line
+ // This behavior is in line with ordinary get diff API.
+ assertThat(diff).metaB().totalLineCount().isEqualTo(3);
+
+ assertThat(diff).content().hasSize(2);
+ assertThat(diff).content().element(0).commonLines().containsExactly("1st line");
+ assertThat(diff).content().element(1).linesOfA().containsExactly("2nd line");
+ assertThat(diff).content().element(1).linesOfB().containsExactly("2nd line", "");
+ }
+
+ private List<CommentInfo> getComments() throws RestApiException {
+ return gApi.changes().id(changeId).current().commentsAsList();
+ }
+
+ private static String getStringFor(int numberOfBytes) {
+ char[] chars = new char[numberOfBytes];
+ // 'a' will require one byte even when mapped to a JSON string
+ Arrays.fill(chars, 'a');
+ return new String(chars);
+ }
+
+ private static List<String> getFixIds(List<CommentInfo> comments) {
+ assertThatList(comments).isNotNull();
+ return comments.stream()
+ .map(commentInfo -> commentInfo.fixSuggestions)
+ .filter(Objects::nonNull)
+ .flatMap(List::stream)
+ .map(fixSuggestionInfo -> fixSuggestionInfo.fixId)
+ .collect(toList());
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
index b31d35c..5322785d 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
@@ -18,6 +18,9 @@
import static com.google.common.collect.MoreCollectors.onlyElement;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
+import static com.google.gerrit.acceptance.server.change.CommentsUtil.createFixReplacementInfo;
+import static com.google.gerrit.acceptance.server.change.CommentsUtil.createFixSuggestionInfo;
+import static com.google.gerrit.acceptance.server.change.CommentsUtil.createRange;
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
@@ -43,7 +46,6 @@
import com.google.gerrit.extensions.api.changes.PublishChangeEditInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
-import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
@@ -489,7 +491,7 @@
.hasMessageThat()
.contains(
String.format(
- "A description is required for the suggested fix of the robot comment on %s",
+ "A description is required for the suggested fix of the comment on %s",
withFixRobotCommentInput.path));
}
@@ -518,7 +520,7 @@
.contains(
String.format(
"At least one replacement is required"
- + " for the suggested fix of the robot comment on %s",
+ + " for the suggested fix of the comment on %s",
withFixRobotCommentInput.path));
}
@@ -548,7 +550,7 @@
.hasMessageThat()
.contains(
String.format(
- "A file path must be given for the replacement of the robot comment on %s",
+ "A file path must be given for the replacement of the comment on %s",
withFixRobotCommentInput.path));
}
@@ -578,7 +580,7 @@
.hasMessageThat()
.contains(
String.format(
- "A range must be given for the replacement of the robot comment on %s",
+ "A range must be given for the replacement of the comment on %s",
withFixRobotCommentInput.path));
}
@@ -732,7 +734,7 @@
.contains(
String.format(
"A content for replacement must be "
- + "indicated for the replacement of the robot comment on %s",
+ + "indicated for the replacement of the comment on %s",
withFixRobotCommentInput.path));
}
@@ -1676,33 +1678,6 @@
assertThat(diff).content().element(1).linesOfB().containsExactly("2nd line", "");
}
- private static FixSuggestionInfo createFixSuggestionInfo(
- FixReplacementInfo... fixReplacementInfos) {
- FixSuggestionInfo newFixSuggestionInfo = new FixSuggestionInfo();
- newFixSuggestionInfo.fixId = "An ID which must be overwritten.";
- newFixSuggestionInfo.description = "A description for a suggested fix.";
- newFixSuggestionInfo.replacements = Arrays.asList(fixReplacementInfos);
- return newFixSuggestionInfo;
- }
-
- private static FixReplacementInfo createFixReplacementInfo() {
- FixReplacementInfo newFixReplacementInfo = new FixReplacementInfo();
- newFixReplacementInfo.path = FILE_NAME;
- newFixReplacementInfo.replacement = "some replacement code";
- newFixReplacementInfo.range = createRange(3, 9, 8, 4);
- return newFixReplacementInfo;
- }
-
- private static Comment.Range createRange(
- int startLine, int startCharacter, int endLine, int endCharacter) {
- Comment.Range range = new Comment.Range();
- range.startLine = startLine;
- range.startCharacter = startCharacter;
- range.endLine = endLine;
- range.endCharacter = endCharacter;
- return range;
- }
-
private List<RobotCommentInfo> getRobotComments() throws RestApiException {
return gApi.changes().id(changeId).current().robotCommentsAsList();
}
diff --git a/javatests/com/google/gerrit/acceptance/server/change/BUILD b/javatests/com/google/gerrit/acceptance/server/change/BUILD
index 4514ea3..33dfe67 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/BUILD
+++ b/javatests/com/google/gerrit/acceptance/server/change/BUILD
@@ -13,8 +13,9 @@
java_library(
name = "util",
+ testonly = 1,
srcs = ["CommentsUtil.java"],
- visibility = ["//javatests/com/google/gerrit/acceptance/api/change:__subpackages__"],
+ visibility = ["//visibility:public"],
deps = [
"//java/com/google/gerrit/acceptance:lib",
"//java/com/google/gerrit/entities",
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index ee05260..87fc94d 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -19,6 +19,7 @@
import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME;
import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT;
+import static com.google.gerrit.acceptance.server.change.CommentsUtil.createRange;
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
@@ -155,6 +156,56 @@
}
@Test
+ public void createDraftWithFixSuggestions() throws Exception {
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ String path = "file1";
+ DraftInput comment = CommentsUtil.newDraft(path, Side.REVISION, 0, "comment 1");
+ comment.fixSuggestions =
+ ImmutableList.of(
+ CommentsUtil.createFixSuggestionInfo(CommentsUtil.createFixReplacementInfo()));
+ addDraft(changeId, revId, comment);
+ Map<String, List<CommentInfo>> result = getDraftComments(changeId, revId);
+ assertThat(result).hasSize(1);
+ CommentInfo actual = Iterables.getOnlyElement(result.get(comment.path));
+ // FixId is generated, use the one provided by the server.
+ comment.fixSuggestions.get(0).fixId = actual.fixSuggestions.get(0).fixId;
+ assertThat(comment).isEqualTo(infoToDraft(path).apply(actual));
+
+ List<CommentInfo> list = getDraftCommentsAsList(changeId);
+ assertThat(list).hasSize(1);
+ actual = list.get(0);
+ assertThat(comment).isEqualTo(infoToDraft(path).apply(actual));
+
+ // Publish draft comment
+ ReviewInput reviewInput = new ReviewInput();
+ reviewInput.drafts = DraftHandling.PUBLISH_ALL_REVISIONS;
+ reviewInput.message = "bar";
+ gApi.changes().id(changeId).current().review(reviewInput);
+
+ actual = gApi.changes().id(changeId).commentsRequest().getAsList().get(0);
+ assertThat(comment).isEqualTo(infoToDraft(path).apply(actual));
+ }
+
+ @Test
+ public void createDraftWithFixInvalidSuggestions() throws Exception {
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ String path = "file1";
+ DraftInput comment = CommentsUtil.newDraft(path, Side.REVISION, 0, "comment 1");
+ comment.fixSuggestions =
+ ImmutableList.of(
+ CommentsUtil.createFixSuggestionInfo(CommentsUtil.createFixReplacementInfo()));
+ // Invalid range
+ comment.fixSuggestions.get(0).replacements.get(0).range = createRange(13, 9, 5, 10);
+ BadRequestException thrown =
+ assertThrows(BadRequestException.class, () -> addDraft(changeId, revId, comment));
+ assertThat(thrown).hasMessageThat().contains("Range (13:9 - 5:10)");
+ }
+
+ @Test
public void fireEventsForOperationsOnDrafts() throws Exception {
TestGitReferenceUpdatedListener listener = new TestGitReferenceUpdatedListener();
requestScopeOperations.setApiUser(user.id());
@@ -2236,6 +2287,7 @@
DraftInput draftInput = new DraftInput();
draftInput.path = path;
draftInput.unresolved = info.unresolved;
+ draftInput.fixSuggestions = info.fixSuggestions;
copy(info, draftInput);
return draftInput;
};
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsUtil.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsUtil.java
index f32cf32..e25ae74 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsUtil.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsUtil.java
@@ -27,6 +27,8 @@
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.client.Comment;
import com.google.gerrit.extensions.client.Side;
+import com.google.gerrit.extensions.common.FixReplacementInfo;
+import com.google.gerrit.extensions.common.FixSuggestionInfo;
import java.util.Arrays;
import java.util.HashMap;
@@ -190,4 +192,31 @@
in.omitDuplicateComments = omitDuplicateComments;
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in);
}
+
+ public static FixSuggestionInfo createFixSuggestionInfo(
+ FixReplacementInfo... fixReplacementInfos) {
+ FixSuggestionInfo newFixSuggestionInfo = new FixSuggestionInfo();
+ newFixSuggestionInfo.fixId = "An ID which must be overwritten.";
+ newFixSuggestionInfo.description = "A description for a suggested fix.";
+ newFixSuggestionInfo.replacements = Arrays.asList(fixReplacementInfos);
+ return newFixSuggestionInfo;
+ }
+
+ public static FixReplacementInfo createFixReplacementInfo() {
+ FixReplacementInfo newFixReplacementInfo = new FixReplacementInfo();
+ newFixReplacementInfo.path = FILE_NAME;
+ newFixReplacementInfo.replacement = "some replacement code";
+ newFixReplacementInfo.range = createRange(3, 9, 8, 4);
+ return newFixReplacementInfo;
+ }
+
+ public static Comment.Range createRange(
+ int startLine, int startCharacter, int endLine, int endCharacter) {
+ Comment.Range range = new Comment.Range();
+ range.startLine = startLine;
+ range.startCharacter = startCharacter;
+ range.endLine = endLine;
+ range.endCharacter = endCharacter;
+ return range;
+ }
}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
index 5f4701d..3a69820 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
@@ -31,6 +31,7 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.Comment;
+import com.google.gerrit.entities.FixSuggestion;
import com.google.gerrit.entities.HumanComment;
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.LegacySubmitRequirement;
@@ -1162,6 +1163,7 @@
.put("revId", String.class)
.put("serverId", String.class)
.put("unresolved", boolean.class)
+ .put("fixSuggestions", new TypeLiteral<List<FixSuggestion>>() {}.getType())
.build());
}
