opendev/gerrit
Code Issues Proposed changes

Merge "Make the file path for the replacement of a robot comment explicit"

Browse Source
This commit is contained in:
ekempin 2016-12-19 17:35:10 +00:00 committed by Gerrit Code Review
commit b4de53f109
7 changed files with 106 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
Documentation/rest-api-changes.txt
View File

@ -5476,13 +5476,14 @@ should be modified. They should refer to non-overlapping regions.
[[fix-replacement-info]]
=== FixReplacementInfo
The `FixReplacementInfo` entity describes how the content of a file (which is
further specified by the surrounding context) should be replaced by another
content.
The `FixReplacementInfo` entity describes how the content of a file should be
replaced by another content.
[options="header",cols="1,6"]
|==========================
|Field Name |Description
|`path` |The path of the file which should be modified. Modifications
are only allowed for the file on which the corresponding comment was placed.
|`range` |A <<comment-range,CommentRange>> indicating which content
of the file should be replaced.
|`replacement` |The content which should be used instead of the current one.

39
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
View File

@ -214,6 +214,44 @@ public class RobotCommentsIT extends AbstractDaemonTest {
addRobotComment(changeId, withFixRobotCommentInput);
}
@Test
public void pathOfFixReplacementIsAcceptedAsIs() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
addRobotComment(changeId, withFixRobotCommentInput);
List<RobotCommentInfo> robotCommentInfos = getRobotComments();
assertThatList(robotCommentInfos).onlyElement().onlyFixSuggestion()
.onlyReplacement().path().isEqualTo(fixReplacementInfo.path);
}
@Test
public void pathOfFixReplacementIsMandatory() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
fixReplacementInfo.path = null;
exception.expect(BadRequestException.class);
exception.expectMessage(String.format("A file path must be given for the "
+ "replacement of the robot comment on %s",
withFixRobotCommentInput.path));
addRobotComment(changeId, withFixRobotCommentInput);
}
@Test
public void pathOfFixReplacementMustReferToFileOfComment() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
fixReplacementInfo.path = "anotherFile.txt";
exception.expect(BadRequestException.class);
exception.expectMessage(String.format("Replacements may only be specified "
+ "for the file %s on which the robot comment was added",
withFixRobotCommentInput.path));
addRobotComment(changeId, withFixRobotCommentInput);
}
@Test
public void rangeOfFixReplacementIsAcceptedAsIs() throws Exception {
assume().that(notesMigration.enabled()).isTrue();
@ -331,6 +369,7 @@ public class RobotCommentsIT extends AbstractDaemonTest {
private FixReplacementInfo createFixReplacementInfo() {
FixReplacementInfo newFixReplacementInfo = new FixReplacementInfo();
newFixReplacementInfo.path = FILE_NAME;
newFixReplacementInfo.replacement = "some replacement code";
newFixReplacementInfo.range = createRange(3, 12, 15, 4);
return newFixReplacementInfo;

1
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FixReplacementInfo.java
View File

@ -17,6 +17,7 @@ package com.google.gerrit.extensions.common;
import com.google.gerrit.extensions.client.Comment;
public class FixReplacementInfo {
public String path;
public Comment.Range range;
public String replacement;
}

7
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/FixReplacement.java
View File

@ -15,10 +15,12 @@
package com.google.gerrit.reviewdb.client;
public class FixReplacement {
public String path;
public Comment.Range range;
public String replacement;
public FixReplacement(Comment.Range range, String replacement) {
public FixReplacement(String path, Comment.Range range, String replacement) {
this.path = path;
this.range = range;
this.replacement = replacement;
}
@ -26,7 +28,8 @@ public class FixReplacement {
@Override
public String toString() {
return "FixReplacement{" +
"range=" + range +
"path='" + path + '\'' +
", range=" + range +
", replacement='" + replacement + '\'' +
'}';
}

1
gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java
View File

@ -217,6 +217,7 @@ class CommentJson {
private FixReplacementInfo toFixReplacementInfo(
FixReplacement fixReplacement) {
FixReplacementInfo fixReplacementInfo = new FixReplacementInfo();
fixReplacementInfo.path = fixReplacement.path;
fixReplacementInfo.range = toRange(fixReplacement.range);
fixReplacementInfo.replacement = fixReplacement.replacement;
return fixReplacementInfo;

80
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@ -521,82 +521,105 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
Map<String, List<RobotCommentInput>> in)
throws BadRequestException, OrmException {
for (Map.Entry<String, List<RobotCommentInput>> e : in.entrySet()) {
String path = e.getKey();
String commentPath = e.getKey();
for (RobotCommentInput c : e.getValue()) {
ensureRobotIdIsSet(c.robotId, path);
ensureRobotRunIdIsSet(c.robotRunId, path);
ensureFixSuggestionsAreAddable(c.fixSuggestions, path);
ensureRobotIdIsSet(c.robotId, commentPath);
ensureRobotRunIdIsSet(c.robotRunId, commentPath);
ensureFixSuggestionsAreAddable(c.fixSuggestions, commentPath);
}
}
checkComments(revision, in);
}
private void ensureRobotIdIsSet(String robotId, String path)
private void ensureRobotIdIsSet(String robotId, String commentPath)
throws BadRequestException {
if (robotId == null) {
throw new BadRequestException(String
.format("robotId is missing for robot comment on %s", path));
.format("robotId is missing for robot comment on %s", commentPath));
}
}
private void ensureRobotRunIdIsSet(String robotRunId, String path)
private void ensureRobotRunIdIsSet(String robotRunId, String commentPath)
throws BadRequestException {
if (robotRunId == null) {
throw new BadRequestException(String
.format("robotRunId is missing for robot comment on %s", path));
.format("robotRunId is missing for robot comment on %s",
commentPath));
}
}
private void ensureFixSuggestionsAreAddable(
List<FixSuggestionInfo> fixSuggestionInfos, String path)
List<FixSuggestionInfo> fixSuggestionInfos, String commentPath)
throws BadRequestException {
if (fixSuggestionInfos == null) {
return;
}
for (FixSuggestionInfo fixSuggestionInfo : fixSuggestionInfos) {
ensureDescriptionIsSet(path, fixSuggestionInfo.description);
ensureFixReplacementsAreAddable(path, fixSuggestionInfo.replacements);
ensureDescriptionIsSet(commentPath, fixSuggestionInfo.description);
ensureFixReplacementsAreAddable(commentPath,
fixSuggestionInfo.replacements);
}
}
private void ensureDescriptionIsSet(String path, String description)
private 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", path));
+ "for the suggested fix of the robot comment on %s", commentPath));
}
}
private void ensureFixReplacementsAreAddable(String path,
private void ensureFixReplacementsAreAddable(String commentPath,
List<FixReplacementInfo> fixReplacementInfos) throws BadRequestException {
ensureReplacementsArePresent(path, fixReplacementInfos);
ensureReplacementsArePresent(commentPath, fixReplacementInfos);
for (FixReplacementInfo fixReplacementInfo : fixReplacementInfos) {
ensureRangeIsSet(path, fixReplacementInfo.range);
ensureRangeIsValid(path, fixReplacementInfo.range);
ensureReplacementStringIsSet(path, fixReplacementInfo.replacement);
ensureReplacementPathIsSet(commentPath, fixReplacementInfo.path);
ensureReplacementPathRefersToFileOfComment(commentPath,
fixReplacementInfo.path);
ensureRangeIsSet(commentPath, fixReplacementInfo.range);
ensureRangeIsValid(commentPath, fixReplacementInfo.range);
ensureReplacementStringIsSet(commentPath, fixReplacementInfo.replacement);
}
}
private void ensureReplacementsArePresent(String path,
private 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", path));
+ "required for the suggested fix of the robot comment on %s",
commentPath));
}
}
private void ensureRangeIsSet(String path,
private void ensureReplacementPathIsSet(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));
}
}
private void ensureReplacementPathRefersToFileOfComment(String commentPath,
String replacementPath) throws BadRequestException {
if (!Objects.equals(commentPath, replacementPath)) {
throw new BadRequestException(String.format("Replacements may only be "
+ "specified for the file %s on which the robot comment was added",
commentPath));
}
}
private void ensureRangeIsSet(String commentPath,
com.google.gerrit.extensions.client.Comment.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", path));
+ "for the replacement of the robot comment on %s", commentPath));
}
}
private void ensureRangeIsValid(String path,
private void ensureRangeIsValid(String commentPath,
com.google.gerrit.extensions.client.Comment.Range range)
throws BadRequestException {
if (range == null) {
@ -606,16 +629,16 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
throw new BadRequestException(String.format("Range (%s:%s - %s:%s) is not"
+ " valid for the replacement of the robot comment on %s",
range.startLine, range.startCharacter, range.endLine,
range.endCharacter, path));
range.endCharacter, commentPath));
}
}
private void ensureReplacementStringIsSet(String path, String replacement)
throws BadRequestException {
private 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",
path));
commentPath));
}
}
@ -864,7 +887,8 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
private FixReplacement toFixReplacement(
FixReplacementInfo fixReplacementInfo) {
Comment.Range range = new Comment.Range(fixReplacementInfo.range);
return new FixReplacement(range, fixReplacementInfo.replacement);
return new FixReplacement(fixReplacementInfo.path, range,
fixReplacementInfo.replacement);
}
private Set<CommentSetEntry> readExistingComments(ChangeContext ctx)

4
gerrit-test-util/src/main/java/com/google/gerrit/extensions/common/FixReplacementInfoSubject.java
View File

@ -49,6 +49,10 @@ public class FixReplacementInfoSubject
super(failureStrategy, fixReplacementInfo);
}
public StringSubject path() {
return Truth.assertThat(actual().path).named("path");
}
public RangeSubject range() {
return RangeSubject.assertThat(actual().range).named("range");
}