Merge "Add edge cases tests and fixes for getFixPreview"

java/com/google/gerrit/server/patch/PatchScriptBuilder.java

@@ -28,6 +28,7 @@ import com.google.gerrit.entities.Patch.ChangeType;
 import com.google.gerrit.entities.Patch.PatchType;
 import com.google.gerrit.extensions.client.DiffPreferencesInfo;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.server.fixes.FixCalculator;
 import com.google.gerrit.server.mime.FileTypeRegistry;
 import com.google.gerrit.server.patch.DiffContentCalculator.DiffCalculatorResult;
@@ -116,9 +117,12 @@ class PatchScriptBuilder {
 
   PatchScript toPatchScript(
       Repository git, ObjectId baseId, String fileName, List<FixReplacement> fixReplacements)
-      throws IOException, ResourceConflictException {
+      throws IOException, ResourceConflictException, ResourceNotFoundException {
     SidesResolver sidesResolver = new SidesResolver(git, ComparisonType.againstOtherPatchSet());
     PatchSide a = resolveSideA(git, sidesResolver, fileName, baseId);
+    if (a.mode == FileMode.MISSING) {
+      throw new ResourceNotFoundException(String.format("File %s not found", fileName));
+    }
     FixCalculator.FixResult fixResult = FixCalculator.calculateFix(a.src, fixReplacements);
     PatchSide b =
         new PatchSide(

java/com/google/gerrit/server/patch/PatchScriptFactoryForAutoFix.java

@@ -25,6 +25,7 @@ import com.google.gerrit.entities.PatchSet;
 import com.google.gerrit.extensions.client.DiffPreferencesInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.server.git.LargeObjectException;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.permissions.ChangePermission;
@@ -93,7 +94,7 @@ public class PatchScriptFactoryForAutoFix implements Callable<PatchScript> {
   @Override
   public PatchScript call()
       throws LargeObjectException, AuthException, InvalidChangeOperationException, IOException,
-          PermissionBackendException {
+          PermissionBackendException, ResourceNotFoundException {
 
     try {
       permissionBackend.currentUser().change(notes).check(ChangePermission.READ);
@@ -108,7 +109,7 @@ public class PatchScriptFactoryForAutoFix implements Callable<PatchScript> {
     return createPatchScript();
   }
 
-  private PatchScript createPatchScript() throws LargeObjectException {
+  private PatchScript createPatchScript() throws LargeObjectException, ResourceNotFoundException {
     checkState(patchSet.id().get() != 0, "edit not supported for left side");
     PatchScriptBuilder b = newBuilder();
     try {

java/com/google/gerrit/server/restapi/change/GetFixPreview.java

@@ -109,7 +109,7 @@ public class GetFixPreview implements RestReadView<FixResource> {
       String fileName,
       ImmutableList<FixReplacement> fixReplacements)
       throws PermissionBackendException, AuthException, LargeObjectException,
-          InvalidChangeOperationException, IOException {
+          InvalidChangeOperationException, IOException, ResourceNotFoundException {
     PatchScriptFactoryForAutoFix psf =
         patchScriptFactoryFactory.create(
             git, notes, fileName, patchSet, fixReplacements, DiffPreferencesInfo.defaults());

javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java

@@ -28,6 +28,8 @@ 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.entities.Patch;
+import com.google.gerrit.extensions.api.changes.PublishChangeEditInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput;
 import com.google.gerrit.extensions.client.Comment;
 import com.google.gerrit.extensions.common.ChangeInfo;
@@ -53,20 +55,22 @@ import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 
 public class RobotCommentsIT extends AbstractDaemonTest {
   @Inject private TestCommentHelper testCommentHelper;
 
   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;
@@ -81,7 +85,8 @@ public class RobotCommentsIT extends AbstractDaemonTest {
             admin.newIdent(),
             testRepo,
             "Provide files which can be used for fixes",
-            ImmutableMap.of(FILE_NAME, FILE_CONTENT, FILE_NAME2, FILE_CONTENT2));
+            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();
@@ -994,20 +999,76 @@ public class RobotCommentsIT extends AbstractDaemonTest {
   }
 
   @Test
-  @Ignore
+  public void getFixPreviewForCommitMsg() throws Exception {
-  public void getFixPreviewForNonExistingFile() throws Exception {
+    updateCommitMessage(
-    // Not implemented yet.
+        changeId, "Commit title\n\nCommit message line 1\nLine 2\nLine 3\nLast line\n");
-    fixReplacementInfo.path = "a_non_existent_file.txt";
+    FixReplacementInfo commitMsgReplacement = new FixReplacementInfo();
-    fixReplacementInfo.range = createRange(1, 0, 2, 0);
+    commitMsgReplacement.path = Patch.COMMIT_MSG;
-    fixReplacementInfo.replacement = "Modified content\n";
+    // 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);
+    RobotCommentInput commitMsgRobotCommentInput =
+        TestCommentHelper.createRobotCommentInput(Patch.COMMIT_MSG, commitMsgSuggestionInfo);
+    testCommentHelper.addRobotComment(changeId, commitMsgRobotCommentInput);
+
+    List<RobotCommentInfo> robotCommentInfos = getRobotComments();
+
+    List<String> fixIds = getFixIds(robotCommentInfos);
+    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", "");
+  }
+
+  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 getFixPreviewForNonExistingFile() 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);
+    RobotCommentInput commentInput =
+        TestCommentHelper.createRobotCommentInput(FILE_NAME2, fixSuggestion);
+    testCommentHelper.addRobotComment(changeId, commentInput);
 
-    testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput);
     List<RobotCommentInfo> robotCommentInfos = getRobotComments();
     List<String> fixIds = getFixIds(robotCommentInfos);
     String fixId = Iterables.getOnlyElement(fixIds);
 
     assertThrows(
-        BadRequestException.class,
+        ResourceNotFoundException.class,
         () -> gApi.changes().id(changeId).current().getFixPreview(fixId));
   }
 
@@ -1121,6 +1182,41 @@ public class RobotCommentsIT extends AbstractDaemonTest {
     assertThat(diff2).content().element(2).linesOfB().isNull();
   }
 
+  @Test
+  public void getFixPreviewAddNewLineAtEnd() throws Exception {
+    FixReplacementInfo replacement = new FixReplacementInfo();
+    replacement.path = FILE_NAME3;
+    replacement.range = createRange(2, 8, 2, 8);
+    replacement.replacement = "\n";
+
+    FixSuggestionInfo fixSuggestion = createFixSuggestionInfo(replacement);
+    RobotCommentInput commentInput =
+        TestCommentHelper.createRobotCommentInput(FILE_NAME3, fixSuggestion);
+    testCommentHelper.addRobotComment(changeId, commentInput);
+
+    List<RobotCommentInfo> robotCommentInfos = getRobotComments();
+
+    List<String> fixIds = getFixIds(robotCommentInfos);
+    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 static FixSuggestionInfo createFixSuggestionInfo(
       FixReplacementInfo... fixReplacementInfos) {
     FixSuggestionInfo newFixSuggestionInfo = new FixSuggestionInfo();