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

Up to now, the file path was implicitly limited to the file on which
the robot comment was placed. This restriction will be relaxed in the
future. In order to not break systems which use the fix suggestions at
that (later) point in time, the mandatory field 'path' is introduced
with this change.

Change-Id: I73875b97bf844dee927720765046161bbd888c02

Documentation/rest-api-changes.txt

@@ -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.

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

@@ -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;

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

@@ -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;
 }

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

@@ -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 + '\'' +
         '}';
   }

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

@@ -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;

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

@@ -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)

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

@@ -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");
   }