From 30d6c7d3daeea66d7150b3537d1ee4b5f3ff8178 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Thu, 9 Mar 2017 13:51:02 +0100 Subject: [PATCH] Add REST endpoint to apply a suggested fix of a robot comment To apply a suggested fix, we manually compute the Git tree which results from applying the described modifications to the patch set. That Git tree is transformed into a change edit if none exists or merged with an existing one. Similar to change edits, fixes may only be applied for the current patch set if no change edit exists. If one exists, fixes may only be applied for the patch set on which the change edit is based on. To fail as early as possible, we now reject suggested fixes which include overlapping replacements when they are provided as review input. Change-Id: Iabf0d0af025b0878e839956d16f29693b1e74eee --- Documentation/rest-api-changes.txt | 73 ++- .../api/revision/RobotCommentsIT.java | 528 +++++++++++++++++- .../extensions/api/changes/RevisionApi.java | 17 + .../gerrit/extensions/client/Comment.java | 14 +- .../gerrit/reviewdb/client/Comment.java | 14 +- gerrit-server/BUILD | 18 +- .../server/api/changes/RevisionApiImpl.java | 18 + .../google/gerrit/server/change/ApplyFix.java | 83 +++ .../gerrit/server/change/FileContentUtil.java | 11 +- .../gerrit/server/change/FixResource.java | 42 ++ .../google/gerrit/server/change/Fixes.java | 71 +++ .../google/gerrit/server/change/Module.java | 5 + .../gerrit/server/change/PostReview.java | 32 +- .../server/edit/ChangeEditModifier.java | 125 ++++- .../tree/ChangeFileContentModification.java | 11 + .../fixes/FixReplacementInterpreter.java | 145 +++++ .../gerrit/server/fixes/LineIdentifier.java | 110 ++++ .../gerrit/server/fixes/StringModifier.java | 77 +++ .../ChangeFileContentModificationSubject.java | 69 +++ .../edit/tree/TreeModificationSubject.java | 49 ++ .../fixes/FixReplacementInterpreterTest.java | 252 +++++++++ .../server/fixes/LineIdentifierTest.java | 257 +++++++++ .../server/fixes/StringModifierTest.java | 106 ++++ .../restapi/BinaryResultSubject.java | 10 + 24 files changed, 2096 insertions(+), 41 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/change/ApplyFix.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/change/FixResource.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/change/Fixes.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/fixes/FixReplacementInterpreter.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/fixes/LineIdentifier.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/fixes/StringModifier.java create mode 100644 gerrit-server/src/test/java/com/google/gerrit/server/edit/tree/ChangeFileContentModificationSubject.java create mode 100644 gerrit-server/src/test/java/com/google/gerrit/server/edit/tree/TreeModificationSubject.java create mode 100644 gerrit-server/src/test/java/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java create mode 100644 gerrit-server/src/test/java/com/google/gerrit/server/fixes/LineIdentifierTest.java create mode 100644 gerrit-server/src/test/java/com/google/gerrit/server/fixes/StringModifierTest.java diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index b52582638a..003c933bf1 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -4256,6 +4256,72 @@ returned that describes the robot comment. } ---- +[[apply-fix]] +=== Apply Fix +-- +'POST /changes/<>/revisions/<>/fixes/<>/apply' +-- + +Applies a suggested fix by creating a change edit which includes the +modifications indicated by the fix suggestion. If a change edit already exists, +it will be updated accordingly. A fix can only be applied if no change edit +exists and the fix refers to the current patch set, or the fix refers to the +patch set on which the change edit is based. + +.Request +---- + POST /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/revisions/674ac754f91e64a0efb8087e59a176484bd534d1/fixes/8f605a55_f6aa4ecc/apply HTTP/1.0 +---- + +If the fix was successfully applied, an <> describing the +resulting change edit is returned. + +.Response +---- + HTTP/1.1 200 OK + Content-Disposition: attachment + Content-Type: application/json; charset=UTF-8 + + )]}' + { + "commit":{ + "parents":[ + { + "commit":"1eee2c9d8f352483781e772f35dc586a69ff5646", + } + ], + "author":{ + "name":"John Doe", + "email":"john.doe@example.com", + "date":"2013-05-07 15:21:27.000000000", + "tz":120 + }, + "committer":{ + "name":"Jane Doe", + "email":"jane.doe@example.com", + "date":"2013-05-07 15:35:43.000000000", + "tz":120 + }, + "subject":"Implement feature X", + "message":"Implement feature X\n\nWith this feature ..." + }, + "base_revision":"674ac754f91e64a0efb8087e59a176484bd534d1" + } +---- + +If the application failed e.g. due to conflicts with an existing change edit, +the response "`409 Conflict`" including an error message in the response body +is returned. + +.Response +---- + HTTP/1.1 409 Conflict + Content-Disposition: attachment + Content-Type: text/plain; charset=UTF-8 + + The existing change edit could not be merged with another tree. +---- + [[list-files]] === List Files -- @@ -5592,7 +5658,7 @@ The `EditInfo` entity contains information about a change edit. |`commit` ||The commit of change edit as link:#commit-info[CommitInfo] entity. |`base_revision`||The revision of the patch set the change edit is based on. -|`fetch` || +|`fetch` |optional| Information about how to fetch this patch set. The fetch information is provided as a map that maps the protocol name ("`git`", "`http`", "`ssh`") to link:#fetch-info[FetchInfo] entities. @@ -5686,7 +5752,10 @@ replaced by another content. |`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 <> indicating which content -of the file should be replaced. +of the file should be replaced. Lines in the file are assumed to be separated +by the line feed character, the carriage return character, the carriage return +followed by the line feed character, or one of the other Unicode linebreak +sequences supported by Java. |`replacement` |The content which should be used instead of the current one. |========================== diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java index 11df473fef..e4d83fa921 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java @@ -16,10 +16,12 @@ package com.google.gerrit.acceptance.api.revision; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; -import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME; import static com.google.gerrit.acceptance.PushOneCommit.SUBJECT; +import static com.google.gerrit.extensions.common.EditInfoSubject.assertThat; import static com.google.gerrit.extensions.common.RobotCommentInfoSubject.assertThatList; +import static java.util.stream.Collectors.toList; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AcceptanceTestRequestScope; @@ -28,22 +30,33 @@ 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.common.ChangeInfo; +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.common.RobotCommentInfo; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.BinaryResult; +import com.google.gerrit.extensions.restapi.BinaryResultSubject; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import org.hamcrest.core.StringContains; +import java.util.Objects; +import java.util.Optional; import org.junit.Before; import org.junit.Test; public class RobotCommentsIT extends AbstractDaemonTest { + private static final String FILE_NAME = "file_to_fix.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 String changeId; private FixReplacementInfo fixReplacementInfo; private FixSuggestionInfo fixSuggestionInfo; @@ -51,7 +64,8 @@ public class RobotCommentsIT extends AbstractDaemonTest { @Before public void setUp() throws Exception { - PushOneCommit.Result changeResult = createChange(); + PushOneCommit.Result changeResult = + createChange("Provide a file which can be used for fixes", FILE_NAME, FILE_CONTENT); changeId = changeResult.getChangeId(); fixReplacementInfo = createFixReplacementInfo(); @@ -313,10 +327,87 @@ public class RobotCommentsIT extends AbstractDaemonTest { fixReplacementInfo.range = createRange(13, 9, 5, 10); exception.expect(BadRequestException.class); - exception.expectMessage(new StringContains("Range (13:9 - 5:10)")); + exception.expectMessage("Range (13:9 - 5:10)"); addRobotComment(changeId, withFixRobotCommentInput); } + @Test + public void rangesOfFixReplacementsOfSameFixSuggestionMayNotOverlap() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + 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); + withFixRobotCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo); + + exception.expect(BadRequestException.class); + exception.expectMessage("overlap"); + addRobotComment(changeId, withFixRobotCommentInput); + } + + @Test + public void rangesOfFixReplacementsOfDifferentFixSuggestionsMayOverlap() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + 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); + + withFixRobotCommentInput.fixSuggestions = + ImmutableList.of(fixSuggestionInfo1, fixSuggestionInfo2); + + addRobotComment(changeId, withFixRobotCommentInput); + + List robotCommentInfos = getRobotComments(); + assertThatList(robotCommentInfos).onlyElement().fixSuggestions().hasSize(2); + } + + @Test + public void fixReplacementsDoNotNeedToBeOrderedAccordingToRange() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + 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); + withFixRobotCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo); + + addRobotComment(changeId, withFixRobotCommentInput); + + List robotCommentInfos = getRobotComments(); + assertThatList(robotCommentInfos).onlyElement().onlyFixSuggestion().replacements().hasSize(3); + } + @Test public void replacementStringOfFixReplacementIsAcceptedAsIs() throws Exception { assume().that(notesMigration.enabled()).isTrue(); @@ -348,6 +439,405 @@ public class RobotCommentsIT extends AbstractDaemonTest { addRobotComment(changeId, withFixRobotCommentInput); } + @Test + public void fixWithinALineCanBeApplied() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixReplacementInfo.path = FILE_NAME; + fixReplacementInfo.replacement = "Modified content"; + fixReplacementInfo.range = createRange(3, 1, 3, 3); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + gApi.changes().id(changeId).current().applyFix(fixId); + + Optional 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 fixSpanningMultipleLinesCanBeApplied() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixReplacementInfo.path = FILE_NAME; + fixReplacementInfo.replacement = "Modified content\n5"; + fixReplacementInfo.range = createRange(3, 2, 5, 3); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + gApi.changes().id(changeId).current().applyFix(fixId); + + Optional 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 fixWithTwoCloseReplacementsOnSameFileCanBeApplied() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + 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); + withFixRobotCommentInput.fixSuggestions = ImmutableList.of(fixSuggestionInfo); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + gApi.changes().id(changeId).current().applyFix(fixId); + + Optional 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 twoFixesOnSameFileCanBeApplied() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + 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); + + RobotCommentInput robotCommentInput1 = createRobotCommentInput(fixSuggestionInfo1); + RobotCommentInput robotCommentInput2 = createRobotCommentInput(fixSuggestionInfo2); + addRobotComment(changeId, robotCommentInput1); + addRobotComment(changeId, robotCommentInput2); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + gApi.changes().id(changeId).current().applyFix(fixIds.get(0)); + gApi.changes().id(changeId).current().applyFix(fixIds.get(1)); + + Optional 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 twoConflictingFixesOnSameFileCannotBeApplied() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + 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); + + RobotCommentInput robotCommentInput1 = createRobotCommentInput(fixSuggestionInfo1); + RobotCommentInput robotCommentInput2 = createRobotCommentInput(fixSuggestionInfo2); + addRobotComment(changeId, robotCommentInput1); + addRobotComment(changeId, robotCommentInput2); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + gApi.changes().id(changeId).current().applyFix(fixIds.get(0)); + exception.expect(ResourceConflictException.class); + exception.expectMessage("merge"); + gApi.changes().id(changeId).current().applyFix(fixIds.get(1)); + } + + @Test + public void twoFixesOfSameRobotCommentCanBeApplied() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + 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); + + withFixRobotCommentInput.fixSuggestions = + ImmutableList.of(fixSuggestionInfo1, fixSuggestionInfo2); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + gApi.changes().id(changeId).current().applyFix(fixIds.get(0)); + gApi.changes().id(changeId).current().applyFix(fixIds.get(1)); + + Optional 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 fixOnPreviousPatchSetWithoutChangeEditCannotBeApplied() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixReplacementInfo.path = FILE_NAME; + fixReplacementInfo.replacement = "Modified content"; + fixReplacementInfo.range = createRange(3, 1, 3, 3); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + // Remember patch set and add another one. + String previousRevision = gApi.changes().id(changeId).get().currentRevision; + amendChange(changeId); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + exception.expect(ResourceConflictException.class); + exception.expectMessage("current"); + gApi.changes().id(changeId).revision(previousRevision).applyFix(fixId); + } + + @Test + public void fixOnPreviousPatchSetWithExistingChangeEditCanBeApplied() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + // 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); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + // Remember patch set and add another one. + String previousRevision = gApi.changes().id(changeId).get().currentRevision; + amendChange(changeId); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + EditInfo editInfo = gApi.changes().id(changeId).revision(previousRevision).applyFix(fixId); + + Optional 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 fixOnCurrentPatchSetWithChangeEditOnPreviousPatchSetCannotBeApplied() + throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + // 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); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + exception.expect(ResourceConflictException.class); + exception.expectMessage("based"); + gApi.changes().id(changeId).current().applyFix(fixId); + } + + @Test + public void fixDoesNotModifyCommitMessageOfChangeEdit() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + String changeEditCommitMessage = "This is the commit message of the change edit.\n"; + gApi.changes().id(changeId).edit().modifyCommitMessage(changeEditCommitMessage); + + fixReplacementInfo.path = FILE_NAME; + fixReplacementInfo.replacement = "Modified content"; + fixReplacementInfo.range = createRange(3, 1, 3, 3); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + gApi.changes().id(changeId).current().applyFix(fixId); + + String commitMessage = gApi.changes().id(changeId).edit().getCommitMessage(); + assertThat(commitMessage).isEqualTo(changeEditCommitMessage); + } + + @Test + public void applyingFixTwiceIsIdempotent() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixReplacementInfo.path = FILE_NAME; + fixReplacementInfo.replacement = "Modified content"; + fixReplacementInfo.range = createRange(3, 1, 3, 3); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + 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 = gApi.changes().id(changeId).edit().get(); + assertThat(editInfo).value().commit().commit().isEqualTo(expectedEditCommit); + } + + @Test + public void nonExistentFixCannotBeApplied() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixReplacementInfo.path = FILE_NAME; + fixReplacementInfo.replacement = "Modified content"; + fixReplacementInfo.range = createRange(3, 1, 3, 3); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + String nonExistentFixId = fixId + "_non-existent"; + + exception.expect(ResourceNotFoundException.class); + gApi.changes().id(changeId).current().applyFix(nonExistentFixId); + } + + @Test + public void applyingFixReturnsEditInfoForCreatedChangeEdit() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixReplacementInfo.path = FILE_NAME; + fixReplacementInfo.replacement = "Modified content"; + fixReplacementInfo.range = createRange(3, 1, 3, 3); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + EditInfo editInfo = gApi.changes().id(changeId).current().applyFix(fixId); + + Optional 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 applyingFixOnTopOfChangeEditReturnsEditInfoForUpdatedChangeEdit() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + gApi.changes().id(changeId).edit().create(); + + fixReplacementInfo.path = FILE_NAME; + fixReplacementInfo.replacement = "Modified content"; + fixReplacementInfo.range = createRange(3, 1, 3, 3); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + EditInfo editInfo = gApi.changes().id(changeId).current().applyFix(fixId); + + Optional 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 createdChangeEditIsBasedOnCurrentPatchSet() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + String currentRevision = gApi.changes().id(changeId).get().currentRevision; + + fixReplacementInfo.path = FILE_NAME; + fixReplacementInfo.replacement = "Modified content"; + fixReplacementInfo.range = createRange(3, 1, 3, 3); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + List fixIds = getFixIds(robotCommentInfos); + String fixId = Iterables.getOnlyElement(fixIds); + + EditInfo editInfo = gApi.changes().id(changeId).current().applyFix(fixId); + + assertThat(editInfo).baseRevision().isEqualTo(currentRevision); + } + @Test public void robotCommentsNotSupportedWithoutNoteDb() throws Exception { assume().that(notesMigration.enabled()).isFalse(); @@ -355,7 +845,7 @@ public class RobotCommentsIT extends AbstractDaemonTest { RobotCommentInput in = createRobotCommentInput(); ReviewInput reviewInput = new ReviewInput(); Map> robotComments = new HashMap<>(); - robotComments.put(FILE_NAME, Collections.singletonList(in)); + robotComments.put(in.path, ImmutableList.of(in)); reviewInput.robotComments = robotComments; reviewInput.message = "comment test"; @@ -389,7 +879,7 @@ public class RobotCommentsIT extends AbstractDaemonTest { } } - private RobotCommentInput createRobotCommentInputWithMandatoryFields() { + private static RobotCommentInput createRobotCommentInputWithMandatoryFields() { RobotCommentInput in = new RobotCommentInput(); in.robotId = "happyRobot"; in.robotRunId = "1"; @@ -399,7 +889,8 @@ public class RobotCommentsIT extends AbstractDaemonTest { return in; } - private RobotCommentInput createRobotCommentInput(FixSuggestionInfo... fixSuggestionInfos) { + private static RobotCommentInput createRobotCommentInput( + FixSuggestionInfo... fixSuggestionInfos) { RobotCommentInput in = createRobotCommentInputWithMandatoryFields(); in.url = "http://www.happy-robot.com"; in.properties = new HashMap<>(); @@ -409,7 +900,8 @@ public class RobotCommentsIT extends AbstractDaemonTest { return in; } - private FixSuggestionInfo createFixSuggestionInfo(FixReplacementInfo... fixReplacementInfos) { + 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."; @@ -417,15 +909,15 @@ public class RobotCommentsIT extends AbstractDaemonTest { return newFixSuggestionInfo; } - private FixReplacementInfo createFixReplacementInfo() { + private static FixReplacementInfo createFixReplacementInfo() { FixReplacementInfo newFixReplacementInfo = new FixReplacementInfo(); newFixReplacementInfo.path = FILE_NAME; newFixReplacementInfo.replacement = "some replacement code"; - newFixReplacementInfo.range = createRange(3, 12, 15, 4); + newFixReplacementInfo.range = createRange(3, 9, 8, 4); return newFixReplacementInfo; } - private Comment.Range createRange( + private static Comment.Range createRange( int startLine, int startCharacter, int endLine, int endCharacter) { Comment.Range range = new Comment.Range(); range.startLine = startLine; @@ -439,8 +931,7 @@ public class RobotCommentsIT extends AbstractDaemonTest { throws Exception { ReviewInput reviewInput = new ReviewInput(); reviewInput.robotComments = - Collections.singletonMap( - robotCommentInput.path, Collections.singletonList(robotCommentInput)); + Collections.singletonMap(robotCommentInput.path, ImmutableList.of(robotCommentInput)); reviewInput.message = "robot comment test"; gApi.changes().id(targetChangeId).current().review(reviewInput); } @@ -470,4 +961,15 @@ public class RobotCommentsIT extends AbstractDaemonTest { assertThat(c.path).isNull(); } } + + private static List getFixIds(List robotComments) { + assertThatList(robotComments).isNotNull(); + return robotComments + .stream() + .map(robotCommentInfo -> robotCommentInfo.fixSuggestions) + .filter(Objects::nonNull) + .flatMap(List::stream) + .map(fixSuggestionInfo -> fixSuggestionInfo.fixId) + .collect(toList()); + } } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java index 5dd4ba415f..996999550a 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/RevisionApi.java @@ -18,6 +18,7 @@ import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ActionInfo; import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.extensions.common.CommitInfo; +import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.common.FileInfo; import com.google.gerrit.extensions.common.MergeableInfo; import com.google.gerrit.extensions.common.RobotCommentInfo; @@ -86,6 +87,17 @@ public interface RevisionApi { List robotCommentsAsList() throws RestApiException; + /** + * Applies the indicated fix by creating a new change edit or integrating the fix with the + * existing change edit. If no change edit exists before this call, the fix must refer to the + * current patch set. If a change edit exists, the fix must refer to the patch set on which the + * change edit is based. + * + * @param fixId the ID of the fix which should be applied + * @throws RestApiException if the fix couldn't be applied + */ + EditInfo applyFix(String fixId) throws RestApiException; + DraftApi createDraft(DraftInput in) throws RestApiException; DraftApi draft(String id) throws RestApiException; @@ -254,6 +266,11 @@ public interface RevisionApi { throw new NotImplementedException(); } + @Override + public EditInfo applyFix(String fixId) { + throw new NotImplementedException(); + } + @Override public Map> drafts() { throw new NotImplementedException(); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java index 2225a99368..330799706b 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/client/Comment.java @@ -15,6 +15,7 @@ package com.google.gerrit.extensions.client; import java.sql.Timestamp; +import java.util.Comparator; import java.util.Objects; public abstract class Comment { @@ -36,7 +37,13 @@ public abstract class Comment { public String message; public Boolean unresolved; - public static class Range { + public static class Range implements Comparable { + private static final Comparator RANGE_COMPARATOR = + Comparator.comparingInt(range -> range.startLine) + .thenComparingInt(range -> range.startCharacter) + .thenComparingInt(range -> range.endLine) + .thenComparingInt(range -> range.endCharacter); + public int startLine; // 1-based, inclusive public int startCharacter; // 0-based, inclusive public int endLine; // 1-based, exclusive @@ -81,6 +88,11 @@ public abstract class Comment { + endCharacter + '}'; } + + @Override + public int compareTo(Range otherRange) { + return RANGE_COMPARATOR.compare(this, otherRange); + } } public short side() { diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Comment.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Comment.java index cadd52cc0c..4b3c652268 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Comment.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Comment.java @@ -15,6 +15,7 @@ package com.google.gerrit.reviewdb.client; import java.sql.Timestamp; +import java.util.Comparator; import java.util.Objects; /** @@ -130,7 +131,13 @@ public class Comment { } } - public static class Range { + public static class Range implements Comparable { + private static final Comparator RANGE_COMPARATOR = + Comparator.comparingInt(range -> range.startLine) + .thenComparingInt(range -> range.startChar) + .thenComparingInt(range -> range.endLine) + .thenComparingInt(range -> range.endChar); + public int startLine; // 1-based, inclusive public int startChar; // 0-based, inclusive public int endLine; // 1-based, exclusive @@ -186,6 +193,11 @@ public class Comment { .append('}') .toString(); } + + @Override + public int compareTo(Range otherRange) { + return RANGE_COMPARATOR.compare(this, otherRange); + } } public Key key; diff --git a/gerrit-server/BUILD b/gerrit-server/BUILD index a8ac50d759..6648f3a53c 100644 --- a/gerrit-server/BUILD +++ b/gerrit-server/BUILD @@ -138,6 +138,21 @@ java_library( ], ) +CUSTOM_TRUTH_SUBJECTS = glob([ + "src/test/java/com/google/gerrit/server/**/*Subject.java", +]) + +java_library( + name = "custom-truth-subjects", + testonly = 1, + srcs = CUSTOM_TRUTH_SUBJECTS, + deps = [ + ":server", + "//gerrit-extension-api:api", + "//lib:truth", + ], +) + PROLOG_TEST_CASE = [ "src/test/java/com/google/gerrit/rules/PrologTestCase.java", ] @@ -213,11 +228,12 @@ junit_tests( size = "large", srcs = glob( ["src/test/java/**/*.java"], - exclude = TESTUTIL + PROLOG_TESTS + PROLOG_TEST_CASE + QUERY_TESTS, + exclude = TESTUTIL + CUSTOM_TRUTH_SUBJECTS + PROLOG_TESTS + PROLOG_TEST_CASE + QUERY_TESTS, ), resources = glob(["src/test/resources/com/google/gerrit/server/**/*"]), visibility = ["//visibility:public"], deps = TESTUTIL_DEPS + [ + ":custom-truth-subjects", ":testutil", "//gerrit-antlr:query_exception", "//gerrit-patch-jgit:server", diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java index 43be8df9fd..22ae15628c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java @@ -33,6 +33,7 @@ import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ActionInfo; import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.extensions.common.CommitInfo; +import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.common.FileInfo; import com.google.gerrit.extensions.common.MergeableInfo; import com.google.gerrit.extensions.common.RobotCommentInfo; @@ -41,6 +42,7 @@ import com.google.gerrit.extensions.restapi.BinaryResult; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.server.change.ApplyFix; import com.google.gerrit.server.change.CherryPick; import com.google.gerrit.server.change.Comments; import com.google.gerrit.server.change.CreateDraftComment; @@ -48,6 +50,7 @@ import com.google.gerrit.server.change.DeleteDraftPatchSet; import com.google.gerrit.server.change.DraftComments; import com.google.gerrit.server.change.FileResource; import com.google.gerrit.server.change.Files; +import com.google.gerrit.server.change.Fixes; import com.google.gerrit.server.change.GetDescription; import com.google.gerrit.server.change.GetMergeList; import com.google.gerrit.server.change.GetPatch; @@ -109,6 +112,8 @@ class RevisionApiImpl implements RevisionApi { private final FileApiImpl.Factory fileApi; private final ListRevisionComments listComments; private final ListRobotComments listRobotComments; + private final ApplyFix applyFix; + private final Fixes fixes; private final ListRevisionDrafts listDrafts; private final CreateDraftComment createDraft; private final DraftComments drafts; @@ -147,6 +152,8 @@ class RevisionApiImpl implements RevisionApi { FileApiImpl.Factory fileApi, ListRevisionComments listComments, ListRobotComments listRobotComments, + ApplyFix applyFix, + Fixes fixes, ListRevisionDrafts listDrafts, CreateDraftComment createDraft, DraftComments drafts, @@ -184,6 +191,8 @@ class RevisionApiImpl implements RevisionApi { this.listComments = listComments; this.robotComments = robotComments; this.listRobotComments = listRobotComments; + this.applyFix = applyFix; + this.fixes = fixes; this.listDrafts = listDrafts; this.createDraft = createDraft; this.drafts = drafts; @@ -426,6 +435,15 @@ class RevisionApiImpl implements RevisionApi { } } + @Override + public EditInfo applyFix(String fixId) throws RestApiException { + try { + return applyFix.apply(fixes.parse(revision, IdString.fromDecoded(fixId)), null).value(); + } catch (OrmException | IOException e) { + throw new RestApiException("Cannot apply fix", e); + } + } + @Override public List draftsAsList() throws RestApiException { try { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ApplyFix.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ApplyFix.java new file mode 100644 index 0000000000..ecd4699cff --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ApplyFix.java @@ -0,0 +1,83 @@ +// Copyright (C) 2017 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 com.google.gerrit.extensions.common.EditInfo; +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.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.edit.ChangeEdit; +import com.google.gerrit.server.edit.ChangeEditJson; +import com.google.gerrit.server.edit.ChangeEditModifier; +import com.google.gerrit.server.edit.tree.TreeModification; +import com.google.gerrit.server.fixes.FixReplacementInterpreter; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.project.InvalidChangeOperationException; +import com.google.gerrit.server.project.ProjectState; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.io.IOException; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; + +@Singleton +public class ApplyFix implements RestModifyView { + + private final GitRepositoryManager gitRepositoryManager; + private final FixReplacementInterpreter fixReplacementInterpreter; + private final ChangeEditModifier changeEditModifier; + private final ChangeEditJson changeEditJson; + + @Inject + public ApplyFix( + GitRepositoryManager gitRepositoryManager, + FixReplacementInterpreter fixReplacementInterpreter, + ChangeEditModifier changeEditModifier, + ChangeEditJson changeEditJson) { + this.gitRepositoryManager = gitRepositoryManager; + this.fixReplacementInterpreter = fixReplacementInterpreter; + this.changeEditModifier = changeEditModifier; + this.changeEditJson = changeEditJson; + } + + @Override + public Response apply(FixResource fixResource, Void nothing) + throws AuthException, OrmException, ResourceConflictException, IOException, + ResourceNotFoundException { + RevisionResource revisionResource = fixResource.getRevisionResource(); + Project.NameKey project = revisionResource.getProject(); + ProjectState projectState = revisionResource.getControl().getProjectControl().getProjectState(); + PatchSet patchSet = revisionResource.getPatchSet(); + ObjectId patchSetCommitId = ObjectId.fromString(patchSet.getRevision().get()); + + try (Repository repository = gitRepositoryManager.openRepository(project)) { + TreeModification treeModification = + fixReplacementInterpreter.toTreeModification( + repository, projectState, patchSetCommitId, fixResource.getFixReplacements()); + ChangeEdit changeEdit = + changeEditModifier.combineWithModifiedPatchSetTree( + repository, revisionResource.getControl(), patchSet, treeModification); + EditInfo editInfo = changeEditJson.toEditInfo(changeEdit, false); + return Response.ok(editInfo); + } catch (InvalidChangeOperationException e) { + throw new ResourceConflictException(e.getMessage()); + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java index 732848ec0a..166197e5cf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java @@ -69,8 +69,15 @@ public class FileContentUtil { public BinaryResult getContent(ProjectState project, ObjectId revstr, String path) throws ResourceNotFoundException, IOException { - try (Repository repo = openRepository(project); - RevWalk rw = new RevWalk(repo)) { + try (Repository repo = openRepository(project)) { + return getContent(repo, project, revstr, path); + } + } + + public BinaryResult getContent( + Repository repo, ProjectState project, ObjectId revstr, String path) + throws IOException, ResourceNotFoundException { + try (RevWalk rw = new RevWalk(repo)) { RevCommit commit = rw.parseCommit(revstr); ObjectReader reader = rw.getObjectReader(); TreeWalk tw = TreeWalk.forPath(reader, path, commit.getTree()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/FixResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/FixResource.java new file mode 100644 index 0000000000..08e278542f --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/FixResource.java @@ -0,0 +1,42 @@ +// Copyright (C) 2017 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 com.google.gerrit.extensions.restapi.RestResource; +import com.google.gerrit.extensions.restapi.RestView; +import com.google.gerrit.reviewdb.client.FixReplacement; +import com.google.inject.TypeLiteral; +import java.util.List; + +public class FixResource implements RestResource { + public static final TypeLiteral> FIX_KIND = + new TypeLiteral>() {}; + + private final List fixReplacements; + private final RevisionResource revisionResource; + + public FixResource(RevisionResource revisionResource, List fixReplacements) { + this.fixReplacements = fixReplacements; + this.revisionResource = revisionResource; + } + + public List getFixReplacements() { + return fixReplacements; + } + + public RevisionResource getRevisionResource() { + return revisionResource; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Fixes.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Fixes.java new file mode 100644 index 0000000000..af9f60a2d5 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Fixes.java @@ -0,0 +1,71 @@ +// Copyright (C) 2017 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 com.google.gerrit.extensions.registration.DynamicMap; +import com.google.gerrit.extensions.restapi.ChildCollection; +import com.google.gerrit.extensions.restapi.IdString; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; +import com.google.gerrit.extensions.restapi.RestView; +import com.google.gerrit.reviewdb.client.FixSuggestion; +import com.google.gerrit.reviewdb.client.RobotComment; +import com.google.gerrit.server.CommentsUtil; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.util.List; +import java.util.Objects; + +@Singleton +public class Fixes implements ChildCollection { + + private final DynamicMap> views; + private final CommentsUtil commentsUtil; + + @Inject + Fixes(DynamicMap> views, CommentsUtil commentsUtil) { + this.views = views; + this.commentsUtil = commentsUtil; + } + + @Override + public RestView list() throws ResourceNotFoundException { + throw new ResourceNotFoundException(); + } + + @Override + public FixResource parse(RevisionResource revisionResource, IdString id) + throws ResourceNotFoundException, OrmException { + String fixId = id.get(); + ChangeNotes changeNotes = revisionResource.getNotes(); + + List robotComments = + commentsUtil.robotCommentsByPatchSet(changeNotes, revisionResource.getPatchSet().getId()); + for (RobotComment robotComment : robotComments) { + for (FixSuggestion fixSuggestion : robotComment.fixSuggestions) { + if (Objects.equals(fixId, fixSuggestion.fixId)) { + return new FixResource(revisionResource, fixSuggestion.replacements); + } + } + } + throw new ResourceNotFoundException(id); + } + + @Override + public DynamicMap> views() { + return views; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java index aca6ef1fe5..145a40631c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java @@ -19,6 +19,7 @@ import static com.google.gerrit.server.change.ChangeResource.CHANGE_KIND; import static com.google.gerrit.server.change.CommentResource.COMMENT_KIND; import static com.google.gerrit.server.change.DraftCommentResource.DRAFT_COMMENT_KIND; import static com.google.gerrit.server.change.FileResource.FILE_KIND; +import static com.google.gerrit.server.change.FixResource.FIX_KIND; import static com.google.gerrit.server.change.ReviewerResource.REVIEWER_KIND; import static com.google.gerrit.server.change.RevisionResource.REVISION_KIND; import static com.google.gerrit.server.change.RobotCommentResource.ROBOT_COMMENT_KIND; @@ -40,12 +41,14 @@ public class Module extends RestApiModule { bind(DraftComments.class); bind(Comments.class); bind(RobotComments.class); + bind(Fixes.class); bind(Files.class); bind(Votes.class); DynamicMap.mapOf(binder(), CHANGE_KIND); DynamicMap.mapOf(binder(), COMMENT_KIND); DynamicMap.mapOf(binder(), ROBOT_COMMENT_KIND); + DynamicMap.mapOf(binder(), FIX_KIND); DynamicMap.mapOf(binder(), DRAFT_COMMENT_KIND); DynamicMap.mapOf(binder(), FILE_KIND); DynamicMap.mapOf(binder(), REVIEWER_KIND); @@ -128,6 +131,8 @@ public class Module extends RestApiModule { child(REVISION_KIND, "robotcomments").to(RobotComments.class); get(ROBOT_COMMENT_KIND).to(GetRobotComment.class); + child(REVISION_KIND, "fixes").to(Fixes.class); + post(FIX_KIND, "apply").to(ApplyFix.class); child(REVISION_KIND, "files").to(Files.class); put(FILE_KIND, "reviewed").to(PutReviewed.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index 76cc7e8855..34a6657038 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -50,6 +50,7 @@ import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling; import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput; import com.google.gerrit.extensions.api.changes.ReviewResult; import com.google.gerrit.extensions.api.changes.ReviewerInfo; +import com.google.gerrit.extensions.client.Comment.Range; import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.common.AccountInfo; @@ -566,6 +567,7 @@ public class PostReview implements RestModifyView ensureRangeIsValid(commentPath, fixReplacementInfo.range); ensureReplacementStringIsSet(commentPath, fixReplacementInfo.replacement); } + ensureRangesDoNotOverlap(commentPath, fixReplacementInfos); } private void ensureReplacementsArePresent( @@ -600,9 +602,7 @@ public class PostReview implements RestModifyView } } - private void ensureRangeIsSet( - String commentPath, com.google.gerrit.extensions.client.Comment.Range range) - throws BadRequestException { + private void ensureRangeIsSet(String commentPath, Range range) throws BadRequestException { if (range == null) { throw new BadRequestException( String.format( @@ -610,9 +610,7 @@ public class PostReview implements RestModifyView } } - private void ensureRangeIsValid( - String commentPath, com.google.gerrit.extensions.client.Comment.Range range) - throws BadRequestException { + private void ensureRangeIsValid(String commentPath, Range range) throws BadRequestException { if (range == null) { return; } @@ -639,6 +637,28 @@ public class PostReview implements RestModifyView } } + private static void ensureRangesDoNotOverlap( + String commentPath, List fixReplacementInfos) throws BadRequestException { + List 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; + } + } + /** Used to compare Comments with CommentInput comments. */ @AutoValue abstract static class CommentSetEntry { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java index 3d1fb79a4e..d5d973b10e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java @@ -52,6 +52,7 @@ import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.merge.MergeStrategy; @@ -116,8 +117,7 @@ public class ChangeEditModifier { PatchSet currentPatchSet = lookupCurrentPatchSet(changeControl); ObjectId patchSetCommitId = getPatchSetCommitId(currentPatchSet); - createEditReference( - repository, changeControl, currentPatchSet, patchSetCommitId, TimeUtil.nowTs()); + createEdit(repository, changeControl, currentPatchSet, patchSetCommitId, TimeUtil.nowTs()); } /** @@ -218,9 +218,9 @@ public class ChangeEditModifier { createCommit(repository, basePatchSetCommit, baseTree, newCommitMessage, nowTimestamp); if (optionalChangeEdit.isPresent()) { - updateEditReference(repository, optionalChangeEdit.get(), newEditCommit, nowTimestamp); + updateEdit(repository, optionalChangeEdit.get(), newEditCommit, nowTimestamp); } else { - createEditReference(repository, changeControl, basePatchSet, newEditCommit, nowTimestamp); + createEdit(repository, changeControl, basePatchSet, newEditCommit, nowTimestamp); } } @@ -316,9 +316,62 @@ public class ChangeEditModifier { createCommit(repository, basePatchSetCommit, newTreeId, commitMessage, nowTimestamp); if (optionalChangeEdit.isPresent()) { - updateEditReference(repository, optionalChangeEdit.get(), newEditCommit, nowTimestamp); + updateEdit(repository, optionalChangeEdit.get(), newEditCommit, nowTimestamp); } else { - createEditReference(repository, changeControl, basePatchSet, newEditCommit, nowTimestamp); + createEdit(repository, changeControl, basePatchSet, newEditCommit, nowTimestamp); + } + } + + /** + * Applies the indicated modification to the specified patch set. If a change edit exists and is + * based on the same patch set, the modified patch set tree is merged with the change edit. If the + * change edit doesn't exist, a new one will be created. + * + * @param repository the affected Git repository + * @param changeControl the {@code ChangeControl} of the change to which the patch set belongs + * @param patchSet the {@code PatchSet} which should be modified + * @param treeModification the modification which should be applied + * @return the resulting {@code ChangeEdit} + * @throws AuthException if the user isn't authenticated or not allowed to use change edits + * @throws InvalidChangeOperationException if the existing change edit is based on another patch + * set or no change edit exists but the specified patch set isn't the current one + * @throws MergeConflictException if the modified patch set tree can't be merged with an existing + * change edit + */ + public ChangeEdit combineWithModifiedPatchSetTree( + Repository repository, + ChangeControl changeControl, + PatchSet patchSet, + TreeModification treeModification) + throws AuthException, IOException, InvalidChangeOperationException, MergeConflictException, + OrmException { + ensureAuthenticatedAndPermitted(changeControl); + + Optional optionalChangeEdit = lookupChangeEdit(changeControl); + ensureAllowedPatchSet(changeControl, optionalChangeEdit, patchSet); + + RevCommit patchSetCommit = lookupCommit(repository, patchSet); + ObjectId newTreeId = createNewTree(repository, patchSetCommit, treeModification); + + if (optionalChangeEdit.isPresent()) { + ChangeEdit changeEdit = optionalChangeEdit.get(); + newTreeId = merge(repository, changeEdit, newTreeId); + if (ObjectId.equals(newTreeId, changeEdit.getEditCommit().getTree())) { + // Modification is already contained in the change edit. + return changeEdit; + } + } + + String commitMessage = + optionalChangeEdit.map(ChangeEdit::getEditCommit).orElse(patchSetCommit).getFullMessage(); + Timestamp nowTimestamp = TimeUtil.nowTs(); + ObjectId newEditCommit = + createCommit(repository, patchSetCommit, newTreeId, commitMessage, nowTimestamp); + + if (optionalChangeEdit.isPresent()) { + return updateEdit(repository, optionalChangeEdit.get(), newEditCommit, nowTimestamp); + } else { + return createEdit(repository, changeControl, patchSet, newEditCommit, nowTimestamp); } } @@ -340,7 +393,31 @@ public class ChangeEditModifier { } } - private String getWellFormedCommitMessage(String commitMessage) { + private static void ensureAllowedPatchSet( + ChangeControl changeControl, Optional optionalChangeEdit, PatchSet patchSet) + throws InvalidChangeOperationException { + if (optionalChangeEdit.isPresent()) { + ChangeEdit changeEdit = optionalChangeEdit.get(); + if (!isBasedOn(changeEdit, patchSet)) { + throw new InvalidChangeOperationException( + String.format( + "Only the patch set %s on which the existing change edit is based may be modified " + + "(specified patch set: %s)", + changeEdit.getBasePatchSet().getId(), patchSet.getId())); + } + } else { + PatchSet.Id patchSetId = patchSet.getId(); + PatchSet.Id currentPatchSetId = changeControl.getChange().currentPatchSetId(); + if (!patchSetId.equals(currentPatchSetId)) { + throw new InvalidChangeOperationException( + String.format( + "A change edit may only be created for the current patch set %s (and not for %s)", + currentPatchSetId, patchSetId)); + } + } + } + + private static String getWellFormedCommitMessage(String commitMessage) { String wellFormedMessage = Strings.nullToEmpty(commitMessage).trim(); checkState(!wellFormedMessage.isEmpty(), "Commit message cannot be null or empty"); wellFormedMessage = wellFormedMessage + "\n"; @@ -372,8 +449,13 @@ public class ChangeEditModifier { private static RevCommit lookupCommit(Repository repository, PatchSet patchSet) throws IOException { ObjectId patchSetCommitId = getPatchSetCommitId(patchSet); + return lookupCommit(repository, patchSetCommitId); + } + + private static RevCommit lookupCommit(Repository repository, ObjectId commitId) + throws IOException { try (RevWalk revWalk = new RevWalk(repository)) { - return revWalk.parseCommit(patchSetCommitId); + return revWalk.parseCommit(commitId); } } @@ -390,7 +472,7 @@ public class ChangeEditModifier { return newTreeId; } - private ObjectId merge(Repository repository, ChangeEdit changeEdit, ObjectId newTreeId) + private static ObjectId merge(Repository repository, ChangeEdit changeEdit, ObjectId newTreeId) throws IOException, MergeConflictException { PatchSet basePatchSet = changeEdit.getBasePatchSet(); ObjectId basePatchSetCommitId = getPatchSetCommitId(basePatchSet); @@ -436,17 +518,22 @@ public class ChangeEditModifier { return ObjectId.fromString(patchSet.getRevision().get()); } - private void createEditReference( + private ChangeEdit createEdit( Repository repository, ChangeControl changeControl, PatchSet basePatchSet, - ObjectId newEditCommit, + ObjectId newEditCommitId, Timestamp timestamp) throws IOException, OrmException { Change change = changeControl.getChange(); String editRefName = getEditRefName(change, basePatchSet); - updateReference(repository, editRefName, ObjectId.zeroId(), newEditCommit, timestamp); + updateReference(repository, editRefName, ObjectId.zeroId(), newEditCommitId, timestamp); reindex(change); + + RevCommit newEditCommit = lookupCommit(repository, newEditCommitId); + Ref ref = repository.getRefDatabase().exactRef(editRefName); + return new ChangeEdit( + currentUser.get().asIdentifiedUser(), change, ref, newEditCommit, basePatchSet); } private String getEditRefName(Change change, PatchSet basePatchSet) { @@ -454,13 +541,21 @@ public class ChangeEditModifier { return RefNames.refsEdit(me.getAccountId(), change.getId(), basePatchSet.getId()); } - private void updateEditReference( - Repository repository, ChangeEdit changeEdit, ObjectId newEditCommit, Timestamp timestamp) + private ChangeEdit updateEdit( + Repository repository, ChangeEdit changeEdit, ObjectId newEditCommitId, Timestamp timestamp) throws IOException, OrmException { String editRefName = changeEdit.getRefName(); RevCommit currentEditCommit = changeEdit.getEditCommit(); - updateReference(repository, editRefName, currentEditCommit, newEditCommit, timestamp); + updateReference(repository, editRefName, currentEditCommit, newEditCommitId, timestamp); reindex(changeEdit.getChange()); + + RevCommit newEditCommit = lookupCommit(repository, newEditCommitId); + return new ChangeEdit( + changeEdit.getUser(), + changeEdit.getChange(), + changeEdit.getRef(), + newEditCommit, + changeEdit.getBasePatchSet()); } private void updateReference( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java index dc353093f4..3f71e84e54 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/tree/ChangeFileContentModification.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.edit.tree; import static com.google.common.base.Preconditions.checkNotNull; import static org.eclipse.jgit.lib.Constants.OBJ_BLOB; +import com.google.common.annotations.VisibleForTesting; import com.google.common.io.ByteStreams; import com.google.gerrit.extensions.restapi.RawInput; import java.io.IOException; @@ -53,6 +54,16 @@ public class ChangeFileContentModification implements TreeModification { return Collections.singletonList(changeContentEdit); } + @VisibleForTesting + String getFilePath() { + return filePath; + } + + @VisibleForTesting + RawInput getNewContent() { + return newContent; + } + /** A {@code PathEdit} which changes the contents of a file. */ private static class ChangeContent extends DirCacheEditor.PathEdit { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/fixes/FixReplacementInterpreter.java b/gerrit-server/src/main/java/com/google/gerrit/server/fixes/FixReplacementInterpreter.java new file mode 100644 index 0000000000..f89665a8d8 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/fixes/FixReplacementInterpreter.java @@ -0,0 +1,145 @@ +// Copyright (C) 2017 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.fixes; + +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; + +import com.google.gerrit.common.RawInputUtil; +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.reviewdb.client.Comment; +import com.google.gerrit.reviewdb.client.FixReplacement; +import com.google.gerrit.server.change.FileContentUtil; +import com.google.gerrit.server.edit.tree.ChangeFileContentModification; +import com.google.gerrit.server.edit.tree.TreeModification; +import com.google.gerrit.server.project.ProjectState; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; + +/** An interpreter for {@code FixReplacement}s. */ +@Singleton +public class FixReplacementInterpreter { + + private static final Comparator ASC_RANGE_FIX_REPLACEMENT_COMPARATOR = + Comparator.comparing(fixReplacement -> fixReplacement.range); + + private final FileContentUtil fileContentUtil; + + @Inject + public FixReplacementInterpreter(FileContentUtil fileContentUtil) { + this.fileContentUtil = fileContentUtil; + } + + /** + * Transforms the given {@code FixReplacement}s into a {@code TreeModification}. + * + * @param repository the affected Git repository + * @param projectState the affected project + * @param patchSetCommitId the patch set which should be modified + * @param fixReplacements the replacements which should be applied + * @return a {@code TreeModification} representing the given replacements + * @throws ResourceNotFoundException if a file to which one of the replacements refers doesn't + * exist + * @throws ResourceConflictException if the replacements can't be transformed into a {@code + * TreeModification} + */ + public TreeModification toTreeModification( + Repository repository, + ProjectState projectState, + ObjectId patchSetCommitId, + List fixReplacements) + throws ResourceNotFoundException, IOException, ResourceConflictException { + checkNotNull(fixReplacements, "Fix replacements must not be null"); + checkArgument(!fixReplacements.isEmpty(), "Fix replacements must not be empty"); + + // For a fix suggestion, we allow only fix replacements for the same file. + String filePath = fixReplacements.get(0).path; + return toTreeModification( + repository, projectState, patchSetCommitId, filePath, fixReplacements); + } + + private TreeModification toTreeModification( + Repository repository, + ProjectState projectState, + ObjectId patchSetCommitId, + String filePath, + List fixReplacements) + throws ResourceNotFoundException, IOException, ResourceConflictException { + String fileContent = getFileContent(repository, projectState, patchSetCommitId, filePath); + String newFileContent = getNewFileContent(fileContent, fixReplacements); + return new ChangeFileContentModification(filePath, RawInputUtil.create(newFileContent)); + } + + private String getFileContent( + Repository repository, ProjectState projectState, ObjectId patchSetCommitId, String filePath) + throws ResourceNotFoundException, IOException { + try (BinaryResult fileContent = + fileContentUtil.getContent(repository, projectState, patchSetCommitId, filePath)) { + return fileContent.asString(); + } + } + + private static String getNewFileContent(String fileContent, List fixReplacements) + throws ResourceConflictException { + List sortedReplacements = new ArrayList<>(fixReplacements); + sortedReplacements.sort(ASC_RANGE_FIX_REPLACEMENT_COMPARATOR); + + LineIdentifier lineIdentifier = new LineIdentifier(fileContent); + StringModifier fileContentModifier = new StringModifier(fileContent); + for (FixReplacement fixReplacement : sortedReplacements) { + Comment.Range range = fixReplacement.range; + try { + int startLineIndex = lineIdentifier.getStartIndexOfLine(range.startLine); + int startLineLength = lineIdentifier.getLengthOfLine(range.startLine); + + int endLineIndex = lineIdentifier.getStartIndexOfLine(range.endLine); + int endLineLength = lineIdentifier.getLengthOfLine(range.endLine); + + if (range.startChar > startLineLength || range.endChar > endLineLength) { + throw new ResourceConflictException( + String.format( + "Range %s refers to a non-existent offset (start line length: %s," + + " end line length: %s)", + toString(range), startLineLength, endLineLength)); + } + + int startIndex = startLineIndex + range.startChar; + int endIndex = endLineIndex + range.endChar; + fileContentModifier.replace(startIndex, endIndex, fixReplacement.replacement); + } catch (StringIndexOutOfBoundsException e) { + // Most of the StringIndexOutOfBoundsException should never occur because we reject fix + // replacements for invalid ranges. However, we can't cover all cases for efficiency + // reasons. For instance, we don't determine the number of lines in a file. That's why we + // need to map this exception and thus provide a meaningful error. + throw new ResourceConflictException( + String.format("Cannot apply fix replacement for range %s", toString(range)), e); + } + } + return fileContentModifier.getResult(); + } + + private static String toString(Comment.Range range) { + return String.format( + "(%s:%s - %s:%s)", range.startLine, range.startChar, range.endLine, range.endChar); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/fixes/LineIdentifier.java b/gerrit-server/src/main/java/com/google/gerrit/server/fixes/LineIdentifier.java new file mode 100644 index 0000000000..c32d822196 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/fixes/LineIdentifier.java @@ -0,0 +1,110 @@ +// Copyright (C) 2017 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.fixes; + +import static com.google.common.base.Preconditions.checkNotNull; + +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * An identifier of lines in a string. Lines are sequences of characters which are separated by any + * Unicode linebreak sequence as defined by the regular expression {@code \R}. If data for several + * lines is requested, calls which are ordered according to ascending line numbers are the most + * efficient. + */ +class LineIdentifier { + + private static final Pattern LINE_SEPARATOR_PATTERN = Pattern.compile("\\R"); + private final Matcher lineSeparatorMatcher; + + private int nextLineNumber; + private int nextLineStartIndex; + private int currentLineStartIndex; + private int currentLineEndIndex; + + LineIdentifier(String string) { + checkNotNull(string); + lineSeparatorMatcher = LINE_SEPARATOR_PATTERN.matcher(string); + reset(); + } + + /** + * Returns the start index of the indicated line within the given string. Start indices are + * zero-based while line numbers are one-based. + * + *

Note: Requesting data for several lines is more efficient if those calls occur with + * increasing line number. + * + * @param lineNumber the line whose start index should be determined + * @return the start index of the line + * @throws StringIndexOutOfBoundsException if the line number is negative, zero or greater than + * the identified number of lines + */ + public int getStartIndexOfLine(int lineNumber) { + findLine(lineNumber); + return currentLineStartIndex; + } + + /** + * Returns the length of the indicated line in the given string. The character(s) used to separate + * lines aren't included in the count. Line numbers are one-based. + * + *

Note: Requesting data for several lines is more efficient if those calls occur with + * increasing line number. + * + * @param lineNumber the line whose length should be determined + * @return the length of the line + * @throws StringIndexOutOfBoundsException if the line number is negative, zero or greater than + * the identified number of lines + */ + public int getLengthOfLine(int lineNumber) { + findLine(lineNumber); + return currentLineEndIndex - currentLineStartIndex; + } + + private void findLine(int targetLineNumber) { + if (targetLineNumber <= 0) { + throw new StringIndexOutOfBoundsException("Line number must be positive"); + } + if (targetLineNumber < nextLineNumber) { + reset(); + } + while (nextLineNumber < targetLineNumber + 1 && lineSeparatorMatcher.find()) { + currentLineStartIndex = nextLineStartIndex; + currentLineEndIndex = lineSeparatorMatcher.start(); + nextLineStartIndex = lineSeparatorMatcher.end(); + nextLineNumber++; + } + + // End of string + if (nextLineNumber == targetLineNumber) { + currentLineStartIndex = nextLineStartIndex; + currentLineEndIndex = lineSeparatorMatcher.regionEnd(); + } + if (nextLineNumber < targetLineNumber) { + throw new StringIndexOutOfBoundsException( + String.format("Line %d isn't available", targetLineNumber)); + } + } + + private void reset() { + nextLineNumber = 1; + nextLineStartIndex = 0; + currentLineStartIndex = 0; + currentLineEndIndex = 0; + lineSeparatorMatcher.reset(); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/fixes/StringModifier.java b/gerrit-server/src/main/java/com/google/gerrit/server/fixes/StringModifier.java new file mode 100644 index 0000000000..ccd40b3d4f --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/fixes/StringModifier.java @@ -0,0 +1,77 @@ +// Copyright (C) 2017 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.fixes; + +import static com.google.common.base.Preconditions.checkNotNull; + +/** + * A modifier of a string. It allows to replace multiple parts of a string by indicating those parts + * with indices based on the unmodified string. There is one limitation though: Replacements which + * affect lower indices of the string must be specified before replacements for higher indices. + */ +class StringModifier { + + private final StringBuilder stringBuilder; + + private int characterShift = 0; + private int previousEndOffset = Integer.MIN_VALUE; + + StringModifier(String string) { + checkNotNull(string, "string must not be null"); + stringBuilder = new StringBuilder(string); + } + + /** + * Replaces part of the string with another content. When called multiple times, the calls must be + * ordered according to increasing start indices. Overlapping replacement regions aren't + * supported. + * + * @param startIndex the beginning index in the unmodified string (inclusive) + * @param endIndex the ending index in the unmodified string (exclusive) + * @param replacement the string which should be used instead of the original content + * @throws StringIndexOutOfBoundsException if the start index is smaller than the end index of a + * previous call of this method + */ + public void replace(int startIndex, int endIndex, String replacement) { + checkNotNull(replacement, "replacement string must not be null"); + if (previousEndOffset > startIndex) { + throw new StringIndexOutOfBoundsException( + String.format( + "Not supported to replace the content starting at index %s after previous " + + "replacement which ended at index %s", + startIndex, previousEndOffset)); + } + int shiftedStartIndex = startIndex + characterShift; + int shiftedEndIndex = endIndex + characterShift; + if (shiftedEndIndex > stringBuilder.length()) { + throw new StringIndexOutOfBoundsException( + String.format("end %s > length %s", shiftedEndIndex, stringBuilder.length())); + } + stringBuilder.replace(shiftedStartIndex, shiftedEndIndex, replacement); + + int replacedContentLength = endIndex - startIndex; + characterShift += replacement.length() - replacedContentLength; + previousEndOffset = endIndex; + } + + /** + * Returns the modified string including all specified replacements. + * + * @return the modified string + */ + public String getResult() { + return stringBuilder.toString(); + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/edit/tree/ChangeFileContentModificationSubject.java b/gerrit-server/src/test/java/com/google/gerrit/server/edit/tree/ChangeFileContentModificationSubject.java new file mode 100644 index 0000000000..801b2b03d9 --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/server/edit/tree/ChangeFileContentModificationSubject.java @@ -0,0 +1,69 @@ +// Copyright (C) 2017 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.edit.tree; + +import static com.google.common.truth.Truth.assertAbout; + +import com.google.common.io.CharStreams; +import com.google.common.truth.FailureStrategy; +import com.google.common.truth.StringSubject; +import com.google.common.truth.Subject; +import com.google.common.truth.SubjectFactory; +import com.google.common.truth.Truth; +import com.google.gerrit.extensions.restapi.RawInput; +import java.io.IOException; +import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; + +public class ChangeFileContentModificationSubject + extends Subject { + + private static final SubjectFactory< + ChangeFileContentModificationSubject, ChangeFileContentModification> + MODIFICATION_SUBJECT_FACTORY = + new SubjectFactory< + ChangeFileContentModificationSubject, ChangeFileContentModification>() { + @Override + public ChangeFileContentModificationSubject getSubject( + FailureStrategy failureStrategy, ChangeFileContentModification modification) { + return new ChangeFileContentModificationSubject(failureStrategy, modification); + } + }; + + public static ChangeFileContentModificationSubject assertThat( + ChangeFileContentModification modification) { + return assertAbout(MODIFICATION_SUBJECT_FACTORY).that(modification); + } + + private ChangeFileContentModificationSubject( + FailureStrategy failureStrategy, ChangeFileContentModification modification) { + super(failureStrategy, modification); + } + + public StringSubject filePath() { + isNotNull(); + return Truth.assertThat(actual().getFilePath()).named("filePath"); + } + + public StringSubject newContent() throws IOException { + isNotNull(); + RawInput newContent = actual().getNewContent(); + Truth.assertThat(newContent).named("newContent").isNotNull(); + String contentString = + CharStreams.toString( + new InputStreamReader(newContent.getInputStream(), StandardCharsets.UTF_8)); + return Truth.assertThat(contentString).named("newContent"); + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/edit/tree/TreeModificationSubject.java b/gerrit-server/src/test/java/com/google/gerrit/server/edit/tree/TreeModificationSubject.java new file mode 100644 index 0000000000..00c5a69dcc --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/server/edit/tree/TreeModificationSubject.java @@ -0,0 +1,49 @@ +// Copyright (C) 2017 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.edit.tree; + +import static com.google.common.truth.Truth.assertAbout; + +import com.google.common.truth.FailureStrategy; +import com.google.common.truth.Subject; +import com.google.common.truth.SubjectFactory; + +public class TreeModificationSubject extends Subject { + + private static final SubjectFactory + TREE_MODIFICATION_SUBJECT_FACTORY = + new SubjectFactory() { + @Override + public TreeModificationSubject getSubject( + FailureStrategy failureStrategy, TreeModification treeModification) { + return new TreeModificationSubject(failureStrategy, treeModification); + } + }; + + public static TreeModificationSubject assertThat(TreeModification treeModification) { + return assertAbout(TREE_MODIFICATION_SUBJECT_FACTORY).that(treeModification); + } + + private TreeModificationSubject( + FailureStrategy failureStrategy, TreeModification treeModification) { + super(failureStrategy, treeModification); + } + + public ChangeFileContentModificationSubject asChangeFileContentModification() { + isInstanceOf(ChangeFileContentModification.class); + return ChangeFileContentModificationSubject.assertThat( + (ChangeFileContentModification) actual()); + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java new file mode 100644 index 0000000000..79baafb1b5 --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/server/fixes/FixReplacementInterpreterTest.java @@ -0,0 +1,252 @@ +// Copyright (C) 2017 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.fixes; + +import static com.google.gerrit.server.edit.tree.TreeModificationSubject.assertThat; +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.replay; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.extensions.restapi.BinaryResult; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.reviewdb.client.Comment.Range; +import com.google.gerrit.reviewdb.client.FixReplacement; +import com.google.gerrit.server.change.FileContentUtil; +import com.google.gerrit.server.edit.tree.TreeModification; +import com.google.gerrit.server.project.ProjectState; +import org.easymock.EasyMock; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +public class FixReplacementInterpreterTest { + + @Rule public ExpectedException expectedException = ExpectedException.none(); + + private final FileContentUtil fileContentUtil = createMock(FileContentUtil.class); + private final Repository repository = createMock(Repository.class); + private final ProjectState projectState = createMock(ProjectState.class); + private final ObjectId patchSetCommitId = createMock(ObjectId.class); + private final String filePath = "an/arbitrary/file.txt"; + + private FixReplacementInterpreter fixReplacementInterpreter; + + @Before + public void setUp() { + fixReplacementInterpreter = new FixReplacementInterpreter(fileContentUtil); + } + + @Test + public void treeModificationTargetsCorrectFile() throws Exception { + FixReplacement fixReplacement = + new FixReplacement(filePath, new Range(1, 1, 3, 2), "Modified content"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + TreeModification treeModification = toTreeModification(fixReplacement); + assertThat(treeModification).asChangeFileContentModification().filePath().isEqualTo(filePath); + } + + @Test + public void replacementsCanDeleteALine() throws Exception { + FixReplacement fixReplacement = new FixReplacement(filePath, new Range(2, 0, 3, 0), ""); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + TreeModification treeModification = toTreeModification(fixReplacement); + assertThat(treeModification) + .asChangeFileContentModification() + .newContent() + .isEqualTo("First line\nThird line\n"); + } + + @Test + public void replacementsCanAddALine() throws Exception { + FixReplacement fixReplacement = + new FixReplacement(filePath, new Range(2, 0, 2, 0), "A new line\n"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + TreeModification treeModification = toTreeModification(fixReplacement); + assertThat(treeModification) + .asChangeFileContentModification() + .newContent() + .isEqualTo("First line\nA new line\nSecond line\nThird line\n"); + } + + @Test + public void replacementsMaySpanMultipleLines() throws Exception { + FixReplacement fixReplacement = new FixReplacement(filePath, new Range(1, 6, 3, 1), "and t"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + TreeModification treeModification = toTreeModification(fixReplacement); + assertThat(treeModification) + .asChangeFileContentModification() + .newContent() + .isEqualTo("First and third line\n"); + } + + @Test + public void replacementsMayOccurOnSameLine() throws Exception { + FixReplacement fixReplacement1 = new FixReplacement(filePath, new Range(2, 0, 2, 6), "A"); + FixReplacement fixReplacement2 = + new FixReplacement(filePath, new Range(2, 7, 2, 11), "modification"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + TreeModification treeModification = toTreeModification(fixReplacement1, fixReplacement2); + assertThat(treeModification) + .asChangeFileContentModification() + .newContent() + .isEqualTo("First line\nA modification\nThird line\n"); + } + + @Test + public void replacementsMayTouch() throws Exception { + FixReplacement fixReplacement1 = + new FixReplacement(filePath, new Range(1, 6, 2, 7), "modified "); + FixReplacement fixReplacement2 = new FixReplacement(filePath, new Range(2, 7, 3, 5), "content"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + TreeModification treeModification = toTreeModification(fixReplacement1, fixReplacement2); + assertThat(treeModification) + .asChangeFileContentModification() + .newContent() + .isEqualTo("First modified content line\n"); + } + + @Test + public void replacementsCanAddContentAtEndOfFile() throws Exception { + FixReplacement fixReplacement = + new FixReplacement(filePath, new Range(4, 0, 4, 0), "New content"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + TreeModification treeModification = toTreeModification(fixReplacement); + assertThat(treeModification) + .asChangeFileContentModification() + .newContent() + .isEqualTo("First line\nSecond line\nThird line\nNew content"); + } + + @Test + public void lineSeparatorCanBeChanged() throws Exception { + FixReplacement fixReplacement = new FixReplacement(filePath, new Range(2, 11, 3, 0), "\r"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + TreeModification treeModification = toTreeModification(fixReplacement); + assertThat(treeModification) + .asChangeFileContentModification() + .newContent() + .isEqualTo("First line\nSecond line\rThird line\n"); + } + + @Test + public void replacementsDoNotNeedToBeOrderedAccordingToRange() throws Exception { + FixReplacement fixReplacement1 = + new FixReplacement(filePath, new Range(1, 0, 2, 0), "1st modification\n"); + FixReplacement fixReplacement2 = + new FixReplacement(filePath, new Range(3, 0, 4, 0), "2nd modification\n"); + FixReplacement fixReplacement3 = + new FixReplacement(filePath, new Range(4, 0, 5, 0), "3rd modification\n"); + mockFileContent(filePath, "First line\nSecond line\nThird line\nFourth line\nFifth line\n"); + + replay(fileContentUtil); + TreeModification treeModification = + toTreeModification(fixReplacement2, fixReplacement1, fixReplacement3); + assertThat(treeModification) + .asChangeFileContentModification() + .newContent() + .isEqualTo( + "1st modification\nSecond line\n2nd modification\n3rd modification\nFifth line\n"); + } + + @Test + public void replacementsMustNotReferToNotExistingLine() throws Exception { + FixReplacement fixReplacement = + new FixReplacement(filePath, new Range(5, 0, 5, 0), "A new line\n"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + + expectedException.expect(ResourceConflictException.class); + toTreeModification(fixReplacement); + } + + @Test + public void replacementsMustNotReferToZeroLine() throws Exception { + FixReplacement fixReplacement = + new FixReplacement(filePath, new Range(0, 0, 0, 0), "A new line\n"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + + expectedException.expect(ResourceConflictException.class); + toTreeModification(fixReplacement); + } + + @Test + public void replacementsMustNotReferToNotExistingOffsetOfIntermediateLine() throws Exception { + FixReplacement fixReplacement = + new FixReplacement(filePath, new Range(1, 0, 1, 11), "modified"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + + expectedException.expect(ResourceConflictException.class); + toTreeModification(fixReplacement); + } + + @Test + public void replacementsMustNotReferToNotExistingOffsetOfLastLine() throws Exception { + FixReplacement fixReplacement = + new FixReplacement(filePath, new Range(3, 0, 3, 11), "modified"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + + expectedException.expect(ResourceConflictException.class); + toTreeModification(fixReplacement); + } + + @Test + public void replacementsMustNotReferToNegativeOffset() throws Exception { + FixReplacement fixReplacement = + new FixReplacement(filePath, new Range(1, -1, 1, 5), "modified"); + mockFileContent(filePath, "First line\nSecond line\nThird line\n"); + + replay(fileContentUtil); + + expectedException.expect(ResourceConflictException.class); + toTreeModification(fixReplacement); + } + + private void mockFileContent(String filePath, String fileContent) throws Exception { + EasyMock.expect( + fileContentUtil.getContent(repository, projectState, patchSetCommitId, filePath)) + .andReturn(BinaryResult.create(fileContent)); + } + + private TreeModification toTreeModification(FixReplacement... fixReplacements) throws Exception { + return fixReplacementInterpreter.toTreeModification( + repository, projectState, patchSetCommitId, ImmutableList.copyOf(fixReplacements)); + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/fixes/LineIdentifierTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/fixes/LineIdentifierTest.java new file mode 100644 index 0000000000..f6383462ee --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/server/fixes/LineIdentifierTest.java @@ -0,0 +1,257 @@ +// Copyright (C) 2017 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.fixes; + +import static com.google.common.truth.Truth.assertThat; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +public class LineIdentifierTest { + + @Rule public ExpectedException expectedException = ExpectedException.none(); + + @Test + public void lineNumberMustBePositive() { + LineIdentifier lineIdentifier = new LineIdentifier("First line\nSecond line"); + expectedException.expect(StringIndexOutOfBoundsException.class); + expectedException.expectMessage("positive"); + lineIdentifier.getStartIndexOfLine(0); + } + + @Test + public void lineNumberMustIndicateAnAvailableLine() { + LineIdentifier lineIdentifier = new LineIdentifier("First line\nSecond line"); + expectedException.expect(StringIndexOutOfBoundsException.class); + expectedException.expectMessage("Line 3 isn't available"); + lineIdentifier.getStartIndexOfLine(3); + } + + @Test + public void startIndexOfFirstLineIsRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n1234567"); + int startIndex = lineIdentifier.getStartIndexOfLine(1); + assertThat(startIndex).isEqualTo(0); + } + + @Test + public void lengthOfFirstLineIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n1234567"); + int lineLength = lineIdentifier.getLengthOfLine(1); + assertThat(lineLength).isEqualTo(8); + } + + @Test + public void startIndexOfSecondLineIsRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n1234567"); + int startIndex = lineIdentifier.getStartIndexOfLine(2); + assertThat(startIndex).isEqualTo(9); + } + + @Test + public void lengthOfSecondLineIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n1234567"); + int lineLength = lineIdentifier.getLengthOfLine(2); + assertThat(lineLength).isEqualTo(3); + } + + @Test + public void startIndexOfLastLineIsRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n1234567"); + int startIndex = lineIdentifier.getStartIndexOfLine(3); + assertThat(startIndex).isEqualTo(13); + } + + @Test + public void lengthOfLastLineIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n1234567"); + int lineLength = lineIdentifier.getLengthOfLine(3); + assertThat(lineLength).isEqualTo(7); + } + + @Test + public void emptyFirstLineIsRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("\n123\n1234567"); + int startIndex = lineIdentifier.getStartIndexOfLine(1); + assertThat(startIndex).isEqualTo(0); + } + + @Test + public void lengthOfEmptyFirstLineIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("\n123\n1234567"); + int lineLength = lineIdentifier.getLengthOfLine(1); + assertThat(lineLength).isEqualTo(0); + } + + @Test + public void emptyIntermediaryLineIsRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n\n1234567"); + int startIndex = lineIdentifier.getStartIndexOfLine(2); + assertThat(startIndex).isEqualTo(9); + } + + @Test + public void lengthOfEmptyIntermediaryLineIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n\n1234567"); + int lineLength = lineIdentifier.getLengthOfLine(2); + assertThat(lineLength).isEqualTo(0); + } + + @Test + public void lineAfterIntermediaryLineIsRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n\n1234567"); + int startIndex = lineIdentifier.getStartIndexOfLine(3); + assertThat(startIndex).isEqualTo(10); + } + + @Test + public void emptyLastLineIsRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n"); + int startIndex = lineIdentifier.getStartIndexOfLine(3); + assertThat(startIndex).isEqualTo(13); + } + + @Test + public void lengthOfEmptyLastLineIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n"); + int lineLength = lineIdentifier.getLengthOfLine(3); + assertThat(lineLength).isEqualTo(0); + } + + @Test + public void startIndexOfSingleLineIsRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678"); + int startIndex = lineIdentifier.getStartIndexOfLine(1); + assertThat(startIndex).isEqualTo(0); + } + + @Test + public void lengthOfSingleLineIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678"); + int lineLength = lineIdentifier.getLengthOfLine(1); + assertThat(lineLength).isEqualTo(8); + } + + @Test + public void startIndexOfSingleEmptyLineIsRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier(""); + int startIndex = lineIdentifier.getStartIndexOfLine(1); + assertThat(startIndex).isEqualTo(0); + } + + @Test + public void lengthOfSingleEmptyLineIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier(""); + int lineLength = lineIdentifier.getLengthOfLine(1); + assertThat(lineLength).isEqualTo(0); + } + + @Test + public void lookingUpSubsequentLinesIsPossible() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n1234567\n12"); + + int firstLineStartIndex = lineIdentifier.getStartIndexOfLine(1); + assertThat(firstLineStartIndex).isEqualTo(0); + + int secondLineStartIndex = lineIdentifier.getStartIndexOfLine(2); + assertThat(secondLineStartIndex).isEqualTo(9); + } + + @Test + public void lookingUpNotSubsequentLinesInAscendingOrderIsPossible() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n1234567\n12"); + + int firstLineStartIndex = lineIdentifier.getStartIndexOfLine(1); + assertThat(firstLineStartIndex).isEqualTo(0); + + int fourthLineStartIndex = lineIdentifier.getStartIndexOfLine(4); + assertThat(fourthLineStartIndex).isEqualTo(21); + } + + @Test + public void lookingUpNotSubsequentLinesInDescendingOrderIsPossible() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\n123\n1234567\n12"); + + int fourthLineStartIndex = lineIdentifier.getStartIndexOfLine(4); + assertThat(fourthLineStartIndex).isEqualTo(21); + + int secondLineStartIndex = lineIdentifier.getStartIndexOfLine(2); + assertThat(secondLineStartIndex).isEqualTo(9); + } + + @Test + public void linesSeparatedByOnlyCarriageReturnAreRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\r123\r12"); + int startIndex = lineIdentifier.getStartIndexOfLine(2); + assertThat(startIndex).isEqualTo(9); + } + + @Test + public void lengthOfLinesSeparatedByOnlyCarriageReturnIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\r123\r12"); + int lineLength = lineIdentifier.getLengthOfLine(2); + assertThat(lineLength).isEqualTo(3); + } + + @Test + public void linesSeparatedByLineFeedAndCarriageReturnAreRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\r\n123\r\n12"); + int startIndex = lineIdentifier.getStartIndexOfLine(2); + assertThat(startIndex).isEqualTo(10); + } + + @Test + public void lengthOfLinesSeparatedByLineFeedAndCarriageReturnIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\r\n123\r\n12"); + int lineLength = lineIdentifier.getLengthOfLine(2); + assertThat(lineLength).isEqualTo(3); + } + + @Test + public void linesSeparatedByMixtureOfCarriageReturnAndLineFeedAreRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\r123\r\n12\n123456\r\n1234"); + int startIndex = lineIdentifier.getStartIndexOfLine(5); + assertThat(startIndex).isEqualTo(25); + } + + @Test + public void linesSeparatedBySomeUnicodeLinebreakCharacterAreRecognized() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\u2029123\u202912"); + int startIndex = lineIdentifier.getStartIndexOfLine(2); + assertThat(startIndex).isEqualTo(9); + } + + @Test + public void lengthOfLinesSeparatedBySomeUnicodeLinebreakCharacterIsCorrect() { + LineIdentifier lineIdentifier = new LineIdentifier("12345678\u2029123\u202912"); + int lineLength = lineIdentifier.getLengthOfLine(2); + assertThat(lineLength).isEqualTo(3); + } + + @Test + public void blanksAreNotInterpretedAsLineSeparators() { + LineIdentifier lineIdentifier = new LineIdentifier("1 2345678\n123\n12"); + int startIndex = lineIdentifier.getStartIndexOfLine(2); + assertThat(startIndex).isEqualTo(10); + } + + @Test + public void tabsAreNotInterpretedAsLineSeparators() { + LineIdentifier lineIdentifier = new LineIdentifier("123\t45678\n123\n12"); + int startIndex = lineIdentifier.getStartIndexOfLine(2); + assertThat(startIndex).isEqualTo(10); + } +} diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/fixes/StringModifierTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/fixes/StringModifierTest.java new file mode 100644 index 0000000000..d23e928c8c --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/server/fixes/StringModifierTest.java @@ -0,0 +1,106 @@ +// Copyright (C) 2017 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.fixes; + +import static com.google.common.truth.Truth.assertThat; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +public class StringModifierTest { + + @Rule public ExpectedException expectedException = ExpectedException.none(); + + private final String originalString = "This is the original, unmodified string."; + private StringModifier stringModifier; + + @Before + public void setUp() { + stringModifier = new StringModifier(originalString); + } + + @Test + public void singlePartIsReplaced() { + stringModifier.replace(0, 11, "An"); + String modifiedString = stringModifier.getResult(); + assertThat(modifiedString).isEqualTo("An original, unmodified string."); + } + + @Test + public void twoPartsCanBeReplacedWithInsertionFirst() { + stringModifier.replace(5, 5, "string "); + stringModifier.replace(8, 39, "a modified version"); + String modifiedString = stringModifier.getResult(); + assertThat(modifiedString).isEqualTo("This string is a modified version."); + } + + @Test + public void twoPartsCanBeReplacedWithDeletionFirst() { + stringModifier.replace(0, 8, ""); + stringModifier.replace(12, 32, "modified"); + String modifiedString = stringModifier.getResult(); + assertThat(modifiedString).isEqualTo("the modified string."); + } + + @Test + public void replacedPartsMayTouch() { + stringModifier.replace(0, 8, ""); + stringModifier.replace(8, 32, "The modified"); + String modifiedString = stringModifier.getResult(); + assertThat(modifiedString).isEqualTo("The modified string."); + } + + @Test + public void replacedPartsMustNotOverlap() { + stringModifier.replace(0, 9, ""); + expectedException.expect(StringIndexOutOfBoundsException.class); + stringModifier.replace(8, 32, "The modified"); + } + + @Test + public void startIndexMustNotBeGreaterThanEndIndex() { + expectedException.expect(StringIndexOutOfBoundsException.class); + stringModifier.replace(10, 9, "something"); + } + + @Test + public void startIndexMustNotBeNegative() { + expectedException.expect(StringIndexOutOfBoundsException.class); + stringModifier.replace(-1, 9, "something"); + } + + @Test + public void newContentCanBeInsertedAtEndOfString() { + stringModifier.replace( + originalString.length(), originalString.length(), " And this an addition."); + String modifiedString = stringModifier.getResult(); + assertThat(modifiedString) + .isEqualTo("This is the original, unmodified string. And this an addition."); + } + + @Test + public void startIndexMustNotBeGreaterThanLengthOfString() { + expectedException.expect(StringIndexOutOfBoundsException.class); + stringModifier.replace(originalString.length() + 1, originalString.length() + 1, "something"); + } + + @Test + public void endIndexMustNotBeGreaterThanLengthOfString() { + expectedException.expect(StringIndexOutOfBoundsException.class); + stringModifier.replace(8, originalString.length() + 1, "something"); + } +} diff --git a/gerrit-test-util/src/main/java/com/google/gerrit/extensions/restapi/BinaryResultSubject.java b/gerrit-test-util/src/main/java/com/google/gerrit/extensions/restapi/BinaryResultSubject.java index 989ab0f1f8..30ac49638e 100644 --- a/gerrit-test-util/src/main/java/com/google/gerrit/extensions/restapi/BinaryResultSubject.java +++ b/gerrit-test-util/src/main/java/com/google/gerrit/extensions/restapi/BinaryResultSubject.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertAbout; import com.google.common.truth.FailureStrategy; import com.google.common.truth.PrimitiveByteArraySubject; +import com.google.common.truth.StringSubject; import com.google.common.truth.Subject; import com.google.common.truth.SubjectFactory; import com.google.common.truth.Truth; @@ -51,6 +52,15 @@ public class BinaryResultSubject extends Subject