diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index d60385c2a9..3e0da06e86 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -4743,6 +4743,10 @@ The name of the label. === \{file-id\} The path of the file. +[[fix-id]] +=== \{fix-id\} +UUID of a suggested fix. + [[revision-id]] === \{revision-id\} Identifier that uniquely identifies one revision of a change. @@ -5445,6 +5449,36 @@ merged into the destination branch as this exact SHA-1. If not, insert a new patch set referring to this commit. |========================== +[[fix-suggestion-info]] +=== FixSuggestionInfo +The `FixSuggestionInfo` entity represents a suggested fix. + +[options="header",cols="1,^1,5"] +|========================== +|Field Name ||Description +|`fix_id` |generated, don't set|The <> of the suggested +fix. It will be generated automatically and hence will be ignored if it's set +for input objects. +|`description` ||A description of the suggested fix. +|`replacements` ||A list of <> +entities indicating how the content of the file on which the comment was placed +should be modified. They should refer to non-overlapping regions. +|========================== + +[[fix-replacement-info]] +=== FixReplacementInfo +The `FixReplacementInfo` entity describes how the content of a file (which is +further specified by the surrounding context) should be replaced by another +content. + +[options="header",cols="1,6"] +|========================== +|Field Name |Description +|`range` |A <> indicating which content +of the file should be replaced. +|`replacement` |The content which should be used instead of the current one. +|========================== + [[git-person-info]] === GitPersonInfo The `GitPersonInfo` entity contains information about the @@ -5967,12 +6001,14 @@ In addition `RobotCommentInfo` has the following fields: [options="header",cols="1,^1,5"] |=========================== -|Field Name ||Description -|`robot_id` ||The ID of the robot that generated this comment. -|`robot_run_id` ||An ID of the run of the robot. -|`url` |optional|URL to more information. -|`properties` |optional| -Robot specific properties as map that maps arbitrary keys to values. +|Field Name ||Description +|`robot_id` ||The ID of the robot that generated this comment. +|`robot_run_id` ||An ID of the run of the robot. +|`url` |optional|URL to more information. +|`properties` |optional|Robot specific properties as map that maps arbitrary +keys to values. +|`fix_suggestions`|optional|Suggested fixes for this robot comment as a list of +<> entities. |=========================== [[robot-comment-input]] diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ListSubject.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ListSubject.java new file mode 100644 index 0000000000..4a704de9ae --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ListSubject.java @@ -0,0 +1,93 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance; + +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.truth.Truth.assertAbout; + +import com.google.common.truth.FailureStrategy; +import com.google.common.truth.IterableSubject; +import com.google.common.truth.Subject; +import com.google.common.truth.SubjectFactory; + +import java.util.List; +import java.util.function.Function; + +public class ListSubject, E> extends IterableSubject { + + private final Function elementAssertThatFunction; + + @SuppressWarnings("unchecked") + public static , E> ListSubject assertThat( + List list, Function elementAssertThatFunction) { + // The ListSubjectFactory always returns ListSubjects. + // -> Casting is appropriate. + return (ListSubject) assertAbout( + new ListSubjectFactory<>(elementAssertThatFunction)).that(list); + } + + private ListSubject(FailureStrategy failureStrategy, List list, + Function elementAssertThatFunction) { + super(failureStrategy, list); + this.elementAssertThatFunction = elementAssertThatFunction; + } + + public S element(int index) { + checkArgument(index >= 0, "index(%s) must be >= 0", index); + // The constructor only accepts lists. + // -> Casting is appropriate. + @SuppressWarnings("unchecked") + List list = (List) actual(); + isNotNull(); + if (index >= list.size()) { + fail("has an element at index " + index); + } + return elementAssertThatFunction.apply(list.get(index)); + } + + public S onlyElement() { + isNotNull(); + hasSize(1); + return element(0); + } + + @SuppressWarnings("unchecked") + @Override + public ListSubject named(String s, Object... objects) { + // This object is returned which is of type ListSubject. + // -> Casting is appropriate. + return (ListSubject) super.named(s, objects); + } + + private static class ListSubjectFactory, T> + extends SubjectFactory> { + + private Function elementAssertThatFunction; + + ListSubjectFactory(Function elementAssertThatFunction) { + this.elementAssertThatFunction = elementAssertThatFunction; + } + + @SuppressWarnings("unchecked") + @Override + public ListSubject getSubject(FailureStrategy failureStrategy, + Iterable objects) { + // The constructor of ListSubject only accepts lists. + // -> Casting is appropriate. + return new ListSubject<>(failureStrategy, (List) objects, + elementAssertThatFunction); + } + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/RangeSubject.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/RangeSubject.java new file mode 100644 index 0000000000..51f4ac47cd --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/RangeSubject.java @@ -0,0 +1,62 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance; + +import static com.google.common.truth.Truth.assertAbout; + +import com.google.common.truth.FailureStrategy; +import com.google.common.truth.IntegerSubject; +import com.google.common.truth.Subject; +import com.google.common.truth.SubjectFactory; +import com.google.common.truth.Truth; +import com.google.gerrit.extensions.client.Comment; + +public class RangeSubject extends Subject { + + private static final SubjectFactory + RANGE_SUBJECT_FACTORY = + new SubjectFactory() { + @Override + public RangeSubject getSubject(FailureStrategy failureStrategy, + Comment.Range range) { + return new RangeSubject(failureStrategy, range); + } + }; + + public static RangeSubject assertThat(Comment.Range range) { + return assertAbout(RANGE_SUBJECT_FACTORY) + .that(range); + } + + private RangeSubject(FailureStrategy failureStrategy, Comment.Range range) { + super(failureStrategy, range); + } + + public IntegerSubject startLine() { + return Truth.assertThat(actual().startLine).named("startLine"); + } + + public IntegerSubject startCharacter() { + return Truth.assertThat(actual().startCharacter).named("startCharacter"); + } + + public IntegerSubject endLine() { + return Truth.assertThat(actual().endLine).named("endLine"); + } + + public IntegerSubject endCharacter() { + return Truth.assertThat(actual().endCharacter).named("endCharacter"); + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/BUCK b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/BUCK index 76ae637a0e..5bb3bd9f0d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/BUCK +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/BUCK @@ -4,4 +4,11 @@ acceptance_tests( group = 'api_revision', srcs = glob(['*IT.java']), labels = ['api'], + deps = [':custom_truth_subjects'], +) + +java_library( + name = 'custom_truth_subjects', + srcs = glob(['*Subject.java']), + deps = ['//gerrit-acceptance-tests:lib'], ) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/BUILD b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/BUILD index 4f15ec0c58..cd12955eed 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/BUILD +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/BUILD @@ -4,4 +4,12 @@ acceptance_tests( srcs = glob(["*IT.java"]), group = "api_revision", labels = ["api"], + deps = [":custom_truth_subjects"], +) + +java_library( + name = "custom_truth_subjects", + testonly = 1, + srcs = glob(["*Subject.java"]), + deps = ["//gerrit-acceptance-tests:lib"], ) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/FixReplacementInfoSubject.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/FixReplacementInfoSubject.java new file mode 100644 index 0000000000..1df0aecf60 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/FixReplacementInfoSubject.java @@ -0,0 +1,60 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.api.revision; + +import static com.google.common.truth.Truth.assertAbout; + +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.acceptance.RangeSubject; +import com.google.gerrit.extensions.common.FixReplacementInfo; + +public class FixReplacementInfoSubject + extends Subject { + + private static final SubjectFactory FIX_REPLACEMENT_INFO_SUBJECT_FACTORY = + new SubjectFactory() { + @Override + public FixReplacementInfoSubject getSubject( + FailureStrategy failureStrategy, + FixReplacementInfo fixReplacementInfo) { + return new FixReplacementInfoSubject(failureStrategy, + fixReplacementInfo); + } + }; + + public static FixReplacementInfoSubject assertThat( + FixReplacementInfo fixReplacementInfo) { + return assertAbout(FIX_REPLACEMENT_INFO_SUBJECT_FACTORY) + .that(fixReplacementInfo); + } + + private FixReplacementInfoSubject(FailureStrategy failureStrategy, + FixReplacementInfo fixReplacementInfo) { + super(failureStrategy, fixReplacementInfo); + } + + public RangeSubject range() { + return RangeSubject.assertThat(actual().range).named("range"); + } + + public StringSubject replacement() { + return Truth.assertThat(actual().replacement).named("replacement"); + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/FixSuggestionInfoSubject.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/FixSuggestionInfoSubject.java new file mode 100644 index 0000000000..6d086f11d8 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/FixSuggestionInfoSubject.java @@ -0,0 +1,71 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.api.revision; + +import static com.google.common.truth.Truth.assertAbout; + +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.acceptance.ListSubject; +import com.google.gerrit.extensions.common.FixReplacementInfo; +import com.google.gerrit.extensions.common.FixSuggestionInfo; + +public class FixSuggestionInfoSubject + extends Subject { + + private static final SubjectFactory FIX_SUGGESTION_INFO_SUBJECT_FACTORY = + new SubjectFactory() { + @Override + public FixSuggestionInfoSubject getSubject( + FailureStrategy failureStrategy, + FixSuggestionInfo fixSuggestionInfo) { + return new FixSuggestionInfoSubject(failureStrategy, + fixSuggestionInfo); + } + }; + + public static FixSuggestionInfoSubject assertThat( + FixSuggestionInfo fixSuggestionInfo) { + return assertAbout(FIX_SUGGESTION_INFO_SUBJECT_FACTORY) + .that(fixSuggestionInfo); + } + + private FixSuggestionInfoSubject(FailureStrategy failureStrategy, + FixSuggestionInfo fixSuggestionInfo) { + super(failureStrategy, fixSuggestionInfo); + } + + public StringSubject fixId() { + return Truth.assertThat(actual().fixId).named("fixId"); + } + + public ListSubject replacements() { + return ListSubject.assertThat(actual().replacements, + FixReplacementInfoSubject::assertThat).named("replacements"); + } + + public FixReplacementInfoSubject onlyReplacement() { + return replacements().onlyElement(); + } + + public StringSubject description() { + return Truth.assertThat(actual().description).named("description"); + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentInfoSubject.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentInfoSubject.java new file mode 100644 index 0000000000..1a13698911 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RobotCommentInfoSubject.java @@ -0,0 +1,69 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.api.revision; + +import static com.google.common.truth.Truth.assertAbout; + +import com.google.common.truth.FailureStrategy; +import com.google.common.truth.Subject; +import com.google.common.truth.SubjectFactory; +import com.google.gerrit.acceptance.ListSubject; +import com.google.gerrit.extensions.common.FixSuggestionInfo; +import com.google.gerrit.extensions.common.RobotCommentInfo; + +import java.util.List; + +class RobotCommentInfoSubject + extends Subject { + + private static final SubjectFactory + ROBOT_COMMENT_INFO_SUBJECT_FACTORY = + new SubjectFactory() { + @Override + public RobotCommentInfoSubject getSubject( + FailureStrategy failureStrategy, + RobotCommentInfo robotCommentInfo) { + return new RobotCommentInfoSubject(failureStrategy, robotCommentInfo); + } + }; + + public static ListSubject assertThatList( + List robotCommentInfos) { + return ListSubject.assertThat(robotCommentInfos, + RobotCommentInfoSubject::assertThat).named("robotCommentInfos"); + } + + public static RobotCommentInfoSubject assertThat( + RobotCommentInfo robotCommentInfo) { + return assertAbout(ROBOT_COMMENT_INFO_SUBJECT_FACTORY) + .that(robotCommentInfo); + } + + private RobotCommentInfoSubject(FailureStrategy failureStrategy, + RobotCommentInfo robotCommentInfo) { + super(failureStrategy, robotCommentInfo); + } + + public ListSubject fixSuggestions() { + return ListSubject.assertThat(actual().fixSuggestions, + FixSuggestionInfoSubject::assertThat).named("fixSuggestions"); + } + + public FixSuggestionInfoSubject onlyFixSuggestion() { + return fixSuggestions().onlyElement(); + } +} 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 9bba97d61d..480e197b25 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 @@ -17,18 +17,25 @@ 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.api.revision.RobotCommentInfoSubject.assertThatList; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.extensions.client.Comment; +import com.google.gerrit.extensions.common.FixReplacementInfo; +import com.google.gerrit.extensions.common.FixSuggestionInfo; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput; import com.google.gerrit.extensions.common.RobotCommentInfo; +import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; +import com.google.gerrit.extensions.restapi.RestApiException; import org.junit.Before; import org.junit.Test; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -36,11 +43,18 @@ import java.util.Map; public class RobotCommentsIT extends AbstractDaemonTest { private String changeId; + private FixReplacementInfo fixReplacementInfo; + private FixSuggestionInfo fixSuggestionInfo; + private RobotCommentInput withFixRobotCommentInput; @Before public void setUp() throws Exception { PushOneCommit.Result changeResult = createChange(); changeId = changeResult.getChangeId(); + + fixReplacementInfo = createFixReplacementInfo(); + fixSuggestionInfo = createFixSuggestionInfo(fixReplacementInfo); + withFixRobotCommentInput = createRobotCommentInput(fixSuggestionInfo); } @Test @@ -99,10 +113,7 @@ public class RobotCommentsIT extends AbstractDaemonTest { RobotCommentInput robotCommentInput = createRobotCommentInput(); addRobotComment(changeId, robotCommentInput); - List robotCommentInfos = gApi.changes() - .id(changeId) - .current() - .robotCommentsAsList(); + List robotCommentInfos = getRobotComments(); RobotCommentInfo robotCommentInfo = Iterables.getOnlyElement(robotCommentInfos); @@ -130,6 +141,145 @@ public class RobotCommentsIT extends AbstractDaemonTest { assertRobotComment(comment, in, false); } + @Test + public void addedFixSuggestionCanBeRetrieved() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + assertThatList(robotCommentInfos).onlyElement() + .onlyFixSuggestion().isNotNull(); + } + + @Test + public void fixIdIsGeneratedForFixSuggestion() + throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + assertThatList(robotCommentInfos).onlyElement() + .onlyFixSuggestion().fixId().isNotEmpty(); + assertThatList(robotCommentInfos).onlyElement() + .onlyFixSuggestion().fixId().isNotEqualTo(fixSuggestionInfo.fixId); + } + + @Test + public void descriptionOfFixSuggestionIsAcceptedAsIs() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + assertThatList(robotCommentInfos).onlyElement().onlyFixSuggestion() + .description().isEqualTo(fixSuggestionInfo.description); + } + + @Test + public void descriptionOfFixSuggestionIsMandatory() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixSuggestionInfo.description = null; + + exception.expect(BadRequestException.class); + exception.expectMessage(String.format("A description is required for the " + + "suggested fix of the robot comment on %s", + withFixRobotCommentInput.path)); + addRobotComment(changeId, withFixRobotCommentInput); + } + + @Test + public void addedFixReplacementCanBeRetrieved() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + addRobotComment(changeId, withFixRobotCommentInput); + List robotCommentInfos = getRobotComments(); + + assertThatList(robotCommentInfos).onlyElement().onlyFixSuggestion() + .onlyReplacement().isNotNull(); + } + + @Test + public void fixReplacementsAreMandatory() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixSuggestionInfo.replacements = Collections.emptyList(); + + exception.expect(BadRequestException.class); + exception.expectMessage(String.format("At least one replacement is required" + + " for the suggested fix of the robot comment on %s", + withFixRobotCommentInput.path)); + addRobotComment(changeId, withFixRobotCommentInput); + } + + @Test + public void rangeOfFixReplacementIsAcceptedAsIs() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + addRobotComment(changeId, withFixRobotCommentInput); + + List robotCommentInfos = getRobotComments(); + + assertThatList(robotCommentInfos).onlyElement().onlyFixSuggestion() + .onlyReplacement().range().isEqualTo(fixReplacementInfo.range); + } + + @Test + public void rangeOfFixReplacementIsMandatory() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixReplacementInfo.range = null; + + exception.expect(BadRequestException.class); + exception.expectMessage(String.format("A range must be given for the " + + "replacement of the robot comment on %s", + withFixRobotCommentInput.path)); + addRobotComment(changeId, withFixRobotCommentInput); + } + + @Test + public void rangeOfFixReplacementNeedsToBeValid() throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixReplacementInfo.range = createRange(13, 9, 5, 10); + + exception.expect(BadRequestException.class); + exception.expectMessage(String.format("Range (13:9 - 5:10) is not " + + "valid for the replacement of the robot comment on %s", + withFixRobotCommentInput.path)); + addRobotComment(changeId, withFixRobotCommentInput); + } + + @Test + public void replacementStringOfFixReplacementIsAcceptedAsIs() + throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + addRobotComment(changeId, withFixRobotCommentInput); + + List robotCommentInfos = getRobotComments(); + + assertThatList(robotCommentInfos).onlyElement() + .onlyFixSuggestion().onlyReplacement() + .replacement().isEqualTo(fixReplacementInfo.replacement); + } + + @Test + public void replacementStringOfFixReplacementIsMandatory() + throws Exception { + assume().that(notesMigration.enabled()).isTrue(); + + fixReplacementInfo.replacement = null; + + exception.expect(BadRequestException.class); + exception.expectMessage(String.format("A content for replacement must be " + + "indicated for the replacement of the robot comment on %s", + withFixRobotCommentInput.path)); + addRobotComment(changeId, withFixRobotCommentInput); + } + @Test public void robotCommentsNotSupportedWithoutNoteDb() throws Exception { assume().that(notesMigration.enabled()).isFalse(); @@ -159,15 +309,43 @@ public class RobotCommentsIT extends AbstractDaemonTest { return in; } - private RobotCommentInput createRobotCommentInput() { + private RobotCommentInput createRobotCommentInput( + FixSuggestionInfo... fixSuggestionInfos) { RobotCommentInput in = createRobotCommentInputWithMandatoryFields(); in.url = "http://www.happy-robot.com"; in.properties = new HashMap<>(); in.properties.put("key1", "value1"); in.properties.put("key2", "value2"); + in.fixSuggestions = Arrays.asList(fixSuggestionInfos); return in; } + private FixSuggestionInfo createFixSuggestionInfo( + FixReplacementInfo... fixReplacementInfos) { + FixSuggestionInfo newFixSuggestionInfo = new FixSuggestionInfo(); + newFixSuggestionInfo.fixId = "An ID which must be overwritten."; + newFixSuggestionInfo.description = "A description for a suggested fix."; + newFixSuggestionInfo.replacements = Arrays.asList(fixReplacementInfos); + return newFixSuggestionInfo; + } + + private FixReplacementInfo createFixReplacementInfo() { + FixReplacementInfo newFixReplacementInfo = new FixReplacementInfo(); + newFixReplacementInfo.replacement = "some replacement code"; + newFixReplacementInfo.range = createRange(3, 12, 15, 4); + return newFixReplacementInfo; + } + + private Comment.Range createRange(int startLine, int startCharacter, + int endLine, int endCharacter) { + Comment.Range range = new Comment.Range(); + range.startLine = startLine; + range.startCharacter = startCharacter; + range.endLine = endLine; + range.endCharacter = endCharacter; + return range; + } + private void addRobotComment(String targetChangeId, RobotCommentInput robotCommentInput) throws Exception { ReviewInput reviewInput = new ReviewInput(); @@ -180,6 +358,13 @@ public class RobotCommentsIT extends AbstractDaemonTest { .review(reviewInput); } + private List getRobotComments() throws RestApiException { + return gApi.changes() + .id(changeId) + .current() + .robotCommentsAsList(); + } + private void assertRobotComment(RobotCommentInfo c, RobotCommentInput expected) { assertRobotComment(c, expected, true); diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java index 472559b520..2638cda934 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ReviewInput.java @@ -18,6 +18,7 @@ import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER; import com.google.gerrit.extensions.client.Comment; import com.google.gerrit.extensions.client.ReviewerState; +import com.google.gerrit.extensions.common.FixSuggestionInfo; import com.google.gerrit.extensions.restapi.DefaultInput; import java.util.ArrayList; @@ -103,6 +104,7 @@ public class ReviewInput { public String robotRunId; public String url; public Map properties; + public List fixSuggestions; } public ReviewInput message(String msg) { 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 78ffb82d55..f03b1f778d 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 @@ -41,6 +41,15 @@ public abstract class Comment { public int endLine; public int endCharacter; + public boolean isValid() { + return startLine >= 0 + && startCharacter >= 0 + && endLine >= 0 + && endCharacter >= 0 + && startLine <= endLine + && (startLine != endLine || startCharacter <= endCharacter); + } + @Override public boolean equals(Object o) { if (o instanceof Range) { @@ -57,6 +66,16 @@ public abstract class Comment { public int hashCode() { return Objects.hash(startLine, startCharacter, endLine, endCharacter); } + + @Override + public String toString() { + return "Range{" + + "startLine=" + startLine + + ", startCharacter=" + startCharacter + + ", endLine=" + endLine + + ", endCharacter=" + endCharacter + + '}'; + } } public short side() { diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FixReplacementInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FixReplacementInfo.java new file mode 100644 index 0000000000..988aa54340 --- /dev/null +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FixReplacementInfo.java @@ -0,0 +1,22 @@ +// Copyright (C) 2016 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.extensions.common; + +import com.google.gerrit.extensions.client.Comment; + +public class FixReplacementInfo { + public Comment.Range range; + public String replacement; +} diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FixSuggestionInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FixSuggestionInfo.java new file mode 100644 index 0000000000..7ba7fcc572 --- /dev/null +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/FixSuggestionInfo.java @@ -0,0 +1,23 @@ +// Copyright (C) 2016 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.extensions.common; + +import java.util.List; + +public class FixSuggestionInfo { + public String fixId; + public String description; + public List replacements; +} diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RobotCommentInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RobotCommentInfo.java index 9028a1d81c..8d8731f617 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RobotCommentInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/RobotCommentInfo.java @@ -14,6 +14,7 @@ package com.google.gerrit.extensions.common; +import java.util.List; import java.util.Map; public class RobotCommentInfo extends CommentInfo { @@ -21,4 +22,5 @@ public class RobotCommentInfo extends CommentInfo { public String robotRunId; public String url; public Map properties; + public List fixSuggestions; } diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/FixReplacement.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/FixReplacement.java new file mode 100644 index 0000000000..63e8abbe05 --- /dev/null +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/FixReplacement.java @@ -0,0 +1,33 @@ +// Copyright (C) 2016 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.reviewdb.client; + +public class FixReplacement { + public Comment.Range range; + public String replacement; + + public FixReplacement(Comment.Range range, String replacement) { + this.range = range; + this.replacement = replacement; + } + + @Override + public String toString() { + return "FixReplacement{" + + "range=" + range + + ", replacement='" + replacement + '\'' + + '}'; + } +} diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/FixSuggestion.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/FixSuggestion.java new file mode 100644 index 0000000000..7af647af22 --- /dev/null +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/FixSuggestion.java @@ -0,0 +1,39 @@ +// Copyright (C) 2016 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.reviewdb.client; + +import java.util.List; + +public class FixSuggestion { + public String fixId; + public String description; + public List replacements; + + public FixSuggestion(String fixId, String description, + List replacements) { + this.fixId = fixId; + this.description = description; + this.replacements = replacements; + } + + @Override + public String toString() { + return "FixSuggestion{" + + "fixId='" + fixId + '\'' + + ", description='" + description + '\'' + + ", replacements=" + replacements + + '}'; + } +} diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RobotComment.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RobotComment.java index ecb952ab56..86adc8b100 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RobotComment.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RobotComment.java @@ -15,6 +15,7 @@ package com.google.gerrit.reviewdb.client; import java.sql.Timestamp; +import java.util.List; import java.util.Map; import java.util.Objects; @@ -23,6 +24,7 @@ public class RobotComment extends Comment { public String robotRunId; public String url; public Map properties; + public List fixSuggestions; public RobotComment(Key key, Account.Id author, Timestamp writtenOn, short side, String message, String serverId, String robotId, @@ -54,6 +56,8 @@ public class RobotComment extends Comment { .append("tag=").append(Objects.toString(tag, "")).append(',') .append("url=").append(url).append(',') .append("properties=").append(properties != null ? properties : "") + .append("fixSuggestions=") + .append(fixSuggestions != null ? fixSuggestions : "") .append('}') .toString(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java index 54f73bc872..82d932c60a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CommentJson.java @@ -21,9 +21,13 @@ import com.google.common.collect.FluentIterable; import com.google.gerrit.extensions.client.Comment.Range; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.common.CommentInfo; +import com.google.gerrit.extensions.common.FixReplacementInfo; +import com.google.gerrit.extensions.common.FixSuggestionInfo; import com.google.gerrit.extensions.common.RobotCommentInfo; import com.google.gerrit.extensions.restapi.Url; import com.google.gerrit.reviewdb.client.Comment; +import com.google.gerrit.reviewdb.client.FixReplacement; +import com.google.gerrit.reviewdb.client.FixSuggestion; import com.google.gerrit.reviewdb.client.RobotComment; import com.google.gerrit.server.account.AccountLoader; import com.google.gwtorm.server.OrmException; @@ -34,6 +38,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.TreeMap; +import java.util.stream.Collectors; class CommentJson { @@ -147,7 +152,7 @@ class CommentJson { } } - private Range toRange(Comment.Range commentRange) { + protected Range toRange(Comment.Range commentRange) { Range range = null; if (commentRange != null) { range = new Range(); @@ -181,10 +186,40 @@ class CommentJson { rci.robotRunId = c.robotRunId; rci.url = c.url; rci.properties = c.properties; + rci.fixSuggestions = toFixSuggestionInfos(c.fixSuggestions); fillCommentInfo(c, rci, loader); return rci; } + private List toFixSuggestionInfos( + List fixSuggestions) { + if (fixSuggestions.isEmpty()) { + return null; + } + + return fixSuggestions.stream() + .map(this::toFixSuggestionInfo) + .collect(Collectors.toList()); + } + + private FixSuggestionInfo toFixSuggestionInfo(FixSuggestion fixSuggestion) { + FixSuggestionInfo fixSuggestionInfo = new FixSuggestionInfo(); + fixSuggestionInfo.fixId = fixSuggestion.fixId; + fixSuggestionInfo.description = fixSuggestion.description; + fixSuggestionInfo.replacements = fixSuggestion.replacements.stream() + .map(this::toFixReplacementInfo) + .collect(Collectors.toList()); + return fixSuggestionInfo; + } + + private FixReplacementInfo toFixReplacementInfo( + FixReplacement fixReplacement) { + FixReplacementInfo fixReplacementInfo = new FixReplacementInfo(); + fixReplacementInfo.range = toRange(fixReplacement.range); + fixReplacementInfo.replacement = fixReplacement.replacement; + return fixReplacementInfo; + } + private RobotCommentFormatter() { } } 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 8cfeff00f8..52ae372caf 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 @@ -51,6 +51,8 @@ import com.google.gerrit.extensions.api.changes.ReviewerInfo; import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.common.AccountInfo; +import com.google.gerrit.extensions.common.FixReplacementInfo; +import com.google.gerrit.extensions.common.FixSuggestionInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; @@ -64,6 +66,8 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.Comment; +import com.google.gerrit.reviewdb.client.FixReplacement; +import com.google.gerrit.reviewdb.client.FixSuggestion; import com.google.gerrit.reviewdb.client.LabelId; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchLineComment.Status; @@ -73,6 +77,7 @@ import com.google.gerrit.reviewdb.client.RobotComment; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeMessagesUtil; +import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchSetUtil; @@ -111,6 +116,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; @Singleton public class PostReview implements RestModifyView { @@ -507,19 +513,102 @@ public class PostReview implements RestModifyView for (Map.Entry> e : in.entrySet()) { String path = e.getKey(); for (RobotCommentInput c : e.getValue()) { - if (c.robotId == null) { - throw new BadRequestException(String - .format("robotId is missing for robot comment on %s", path)); - } - if (c.robotRunId == null) { - throw new BadRequestException(String - .format("robotRunId is missing for robot comment on %s", path)); - } + ensureRobotIdIsSet(c.robotId, path); + ensureRobotRunIdIsSet(c.robotRunId, path); + ensureFixSuggestionsAreAddable(c.fixSuggestions, path); } } checkComments(revision, in); } + private void ensureRobotIdIsSet(String robotId, String path) + throws BadRequestException { + if (robotId == null) { + throw new BadRequestException(String + .format("robotId is missing for robot comment on %s", path)); + } + } + + private void ensureRobotRunIdIsSet(String robotRunId, String path) + throws BadRequestException { + if (robotRunId == null) { + throw new BadRequestException(String + .format("robotRunId is missing for robot comment on %s", path)); + } + } + + private void ensureFixSuggestionsAreAddable( + List fixSuggestionInfos, String path) + throws BadRequestException { + if (fixSuggestionInfos == null) { + return; + } + + for (FixSuggestionInfo fixSuggestionInfo : fixSuggestionInfos) { + ensureDescriptionIsSet(path, fixSuggestionInfo.description); + ensureFixReplacementsAreAddable(path, fixSuggestionInfo.replacements); + } + } + + private void ensureDescriptionIsSet(String path, String description) + throws BadRequestException { + if (description == null) { + throw new BadRequestException(String.format("A description is required " + + "for the suggested fix of the robot comment on %s", path)); + } + } + + private void ensureFixReplacementsAreAddable(String path, + List fixReplacementInfos) throws BadRequestException { + ensureReplacementsArePresent(path, fixReplacementInfos); + + for (FixReplacementInfo fixReplacementInfo : fixReplacementInfos) { + ensureRangeIsSet(path, fixReplacementInfo.range); + ensureRangeIsValid(path, fixReplacementInfo.range); + ensureReplacementStringIsSet(path, fixReplacementInfo.replacement); + } + } + + private void ensureReplacementsArePresent(String path, + List fixReplacementInfos) throws BadRequestException { + if (fixReplacementInfos == null || fixReplacementInfos.isEmpty()) { + throw new BadRequestException(String.format("At least one replacement is " + + "required for the suggested fix of the robot comment on %s", path)); + } + } + + private void ensureRangeIsSet(String path, + com.google.gerrit.extensions.client.Comment.Range range) + throws BadRequestException { + if (range == null) { + throw new BadRequestException(String.format("A range must be given " + + "for the replacement of the robot comment on %s", path)); + } + } + + private void ensureRangeIsValid(String path, + com.google.gerrit.extensions.client.Comment.Range range) + throws BadRequestException { + if (range == null) { + return; + } + if (!range.isValid()) { + throw new BadRequestException(String.format("Range (%s:%s - %s:%s) is not" + + " valid for the replacement of the robot comment on %s", + range.startLine, range.startCharacter, range.endLine, + range.endCharacter, path)); + } + } + + private void ensureReplacementStringIsSet(String path, String replacement) + throws BadRequestException { + if (replacement == null) { + throw new BadRequestException(String.format("A content for replacement " + + "must be indicated for the replacement of the robot comment on %s", + path)); + } + } + /** * Used to compare Comments with CommentInput comments. */ @@ -679,34 +768,86 @@ public class PostReview implements RestModifyView return false; } + List newRobotComments = getNewRobotComments(ctx); + commentsUtil.putRobotComments(ctx.getUpdate(psId), newRobotComments); + comments.addAll(newRobotComments); + return !newRobotComments.isEmpty(); + } + + private List getNewRobotComments(ChangeContext ctx) + throws OrmException { List toAdd = new ArrayList<>(in.robotComments.size()); Set existingIds = in.omitDuplicateComments ? readExistingRobotComments(ctx) : Collections.emptySet(); - for (Map.Entry> ent : in.robotComments.entrySet()) { + for (Map.Entry> ent : + in.robotComments.entrySet()) { String path = ent.getKey(); for (RobotCommentInput c : ent.getValue()) { - RobotComment e = commentsUtil.newRobotComment( - ctx, path, psId, c.side(), c.message, c.robotId, c.robotRunId); - e.parentUuid = Url.decode(c.inReplyTo); - e.url = c.url; - e.properties = c.properties; - e.setLineNbrAndRange(c.line, c.range); - e.tag = in.tag; - setCommentRevId(e, patchListCache, ctx.getChange(), ps); - + RobotComment e = createRobotCommentFromInput(ctx, path, c); if (existingIds.contains(CommentSetEntry.create(e))) { continue; } toAdd.add(e); } } + return toAdd; + } - commentsUtil.putRobotComments(ctx.getUpdate(psId), toAdd); - comments.addAll(toAdd); - return !toAdd.isEmpty(); + private RobotComment createRobotCommentFromInput(ChangeContext ctx, + String path, RobotCommentInput robotCommentInput) throws OrmException { + RobotComment robotComment = commentsUtil.newRobotComment( + ctx, path, psId, robotCommentInput.side(), robotCommentInput.message, + robotCommentInput.robotId, robotCommentInput.robotRunId); + robotComment.parentUuid = Url.decode(robotCommentInput.inReplyTo); + robotComment.url = robotCommentInput.url; + robotComment.properties = robotCommentInput.properties; + robotComment.setLineNbrAndRange(robotCommentInput.line, + robotCommentInput.range); + robotComment.tag = in.tag; + setCommentRevId(robotComment, patchListCache, ctx.getChange(), ps); + robotComment.fixSuggestions = createFixSuggestionsFromInput(ctx, + robotCommentInput.fixSuggestions); + return robotComment; + } + + private List createFixSuggestionsFromInput(ChangeContext ctx, + List fixSuggestionInfos) throws OrmException { + if (fixSuggestionInfos == null) { + return Collections.emptyList(); + } + + List fixSuggestions = + new ArrayList<>(fixSuggestionInfos.size()); + for (FixSuggestionInfo fixSuggestionInfo : fixSuggestionInfos) { + fixSuggestions.add( + createFixSuggestionFromInput(ctx, fixSuggestionInfo)); + } + return fixSuggestions; + } + + private FixSuggestion createFixSuggestionFromInput(ChangeContext ctx, + FixSuggestionInfo fixSuggestionInfo) throws OrmException { + List fixReplacements = + toFixReplacements(fixSuggestionInfo.replacements); + String fixId = ChangeUtil.messageUUID(ctx.getDb()); + return new FixSuggestion(fixId, fixSuggestionInfo.description, + fixReplacements); + } + + private List toFixReplacements( + List fixReplacementInfos) { + return fixReplacementInfos.stream() + .map(this::toFixReplacement) + .collect(Collectors.toList()); + } + + private FixReplacement toFixReplacement( + FixReplacementInfo fixReplacementInfo) { + Comment.Range range = new Comment.Range(fixReplacementInfo.range); + return new FixReplacement(range, fixReplacementInfo.replacement); } private Set readExistingComments(ChangeContext ctx)