Add a comment validator that blocks patchset level Depends-on comments This comment validator is added so that 3.5 users don't accidentally post Depends-on comment as patchset level comment. Also update docs and add functional tests. Change-Id: I145daa6857314049ea776b89ad62a0e4b65a94f9
diff --git a/src/main/java/com/googlesource/gerrit/plugins/depends/on/DependsOnCommentValidator.java b/src/main/java/com/googlesource/gerrit/plugins/depends/on/DependsOnCommentValidator.java new file mode 100644 index 0000000..1c3b3bc --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/depends/on/DependsOnCommentValidator.java
@@ -0,0 +1,41 @@ +// Copyright (C) 2022 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.googlesource.gerrit.plugins.depends.on; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.extensions.validators.CommentForValidation; +import com.google.gerrit.extensions.validators.CommentValidationContext; +import com.google.gerrit.extensions.validators.CommentValidationFailure; +import com.google.gerrit.extensions.validators.CommentValidator; +import com.googlesource.gerrit.plugins.depends.on.formats.Comment; + +public class DependsOnCommentValidator implements CommentValidator { + + @Override + public ImmutableList<CommentValidationFailure> validateComments( + CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) { + ImmutableList.Builder<CommentValidationFailure> failures = ImmutableList.builder(); + for (CommentForValidation c : comments) { + if (!CommentForValidation.CommentType.CHANGE_MESSAGE.equals(c.getType()) + && Comment.hasDependsOn(c.getText())) { + failures.add( + c.failValidation( + "Depends-on tags as a patchset level comment are not" + + " supported. See depends-on plugin documentation.")); + } + } + return failures.build(); + } +}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/depends/on/Module.java b/src/main/java/com/googlesource/gerrit/plugins/depends/on/Module.java index 7736734..21644c1 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/depends/on/Module.java +++ b/src/main/java/com/googlesource/gerrit/plugins/depends/on/Module.java
@@ -16,6 +16,7 @@ import com.google.gerrit.extensions.annotations.Exports; import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.extensions.validators.CommentValidator; import com.google.gerrit.server.DynamicOptions.DynamicBean; import com.google.gerrit.server.events.EventListener; import com.google.gerrit.server.query.change.ChangeQueryBuilder.ChangeOperatorFactory; @@ -38,5 +39,8 @@ bind(DynamicBean.class) .annotatedWith(Exports.named(QueryChanges.class)) .to(ChangeMessageStore.class); + bind(CommentValidator.class) + .annotatedWith(Exports.named(DependsOnCommentValidator.class.getSimpleName())) + .to(DependsOnCommentValidator.class); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/depends/on/formats/Comment.java b/src/main/java/com/googlesource/gerrit/plugins/depends/on/formats/Comment.java index 6703965..e911560 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/depends/on/formats/Comment.java +++ b/src/main/java/com/googlesource/gerrit/plugins/depends/on/formats/Comment.java
@@ -47,6 +47,15 @@ return Optional.empty(); } + public static boolean hasDependsOn(String comment) { + for (String line : comment.split("\n", -1)) { + if (DEPENDS_ON_PATTERN.matcher(line).find()) { + return true; + } + } + return false; + } + public static StringBuilder getMessages(Set<DependsOn> dependsons) { StringBuilder dependencies = new StringBuilder("Depends-on:"); for (DependsOn dep : dependsons) {
diff --git a/src/main/resources/Documentation/about.md b/src/main/resources/Documentation/about.md index 930a0a6..ba7906f 100644 --- a/src/main/resources/Documentation/about.md +++ b/src/main/resources/Documentation/about.md
@@ -2,14 +2,14 @@ ======== This plugin provides a way to mark a change dependent on other change(s). To -mark a change dependent, post a comment with the list of dependencies under a -change in the Gerrit page (not an inline diff comment) in a single line with -the format `Depends-on: c1 c2 c3 ....` where c1, c2 and c3 are gerrit change -numbers. Any number of changes can be provided after the `Depends-on:` tag. -The `Depends-on:` tag is case-sensitive. Only changes listed in the most -recent `Depends-on:` tag are considered as valid dependencies and older tags -are ignored. To remove existing dependencies, a `Depends-on:` tag with no -changes must be added. +mark a change dependent, post a change level comment with the list of dependencies +under a change in the Gerrit page (not an inline diff comment or patchset level comment) +in a single line with the format `Depends-on: c1 c2 c3 ....` where c1, c2 and c3 +are gerrit change numbers. Any number of changes can be provided after the +`Depends-on:` tag. The `Depends-on:` tag is case-sensitive. Only changes listed +in the most recent `Depends-on:` tag are considered as valid dependencies and +older tags are ignored. To remove existing dependencies, a `Depends-on:` tag with +no changes must be added. PROPAGATION -----------
diff --git a/test/test_dependson.sh b/test/test_dependson.sh index bf55203..cc68ced 100755 --- a/test/test_dependson.sh +++ b/test/test_dependson.sh
@@ -51,6 +51,13 @@ tail -n +2 | jq --raw-output '._number' } +review_change() { # change commit_sha input_json [args...] > API_response + local change=$1 commit_sha=$2 data=$3 ; shift 3 + curl -X POST --netrc --silent --header 'Content-Type: application/json' \ + "$@" --data "$data" \ + "http://$SERVER:8080/a/changes/$change/revisions/$commit_sha/review" +} + create_change() { # branch file [commit_message] > changenum local branch=$1 tmpfile=$2 msg=$3 out rtn local content=$RANDOM dest=refs/for/$branch @@ -173,4 +180,37 @@ actual=$(get_depends_on_tag "$dest_change") result_out "propagate depends-on" "$expected" "$actual" + +# ------------------------- Depends-on comment validator test --------------------------- +change=$(create_change "$SRC_REF_BRANCH" "$FILE_A") || exit +commit=$(mygit log -1 --pretty=format:"%H") +gssh gerrit review --message \ + \'"Depends-on: 10 30 Ieace383c14de79bf202c85063d5a46a0580724dd 20"\' "$change",1 +result "depends-on change level comment - SSH" + +expected=$(cat <<EOF +error: One or more comments were rejected in validation: Depends-on tags as a \ +patchset level comment are not supported. See depends-on plugin documentation. + +fatal: one or more reviews failed; review output above +EOF +) + +out=$(echo '{"comments": {"/PATCHSET_LEVEL":[{"message": "Depends-on: 10 30"}]}}' | \ + gssh gerrit review --json "$change",1 2>&1) +result_out "depends-on patchset level comment - SSH" "$expected" "$out" + +review_change "$change" "$commit" '{"message" : "Depends-on: 10 30"}' "--fail" > /dev/null 2>&1 +result "depends-on change level comment - REST" + +expected=$(cat <<EOF +One or more comments were rejected in validation: Depends-on tags as a \ +patchset level comment are not supported. See depends-on plugin documentation. +EOF +) + +out=$(review_change "$change" "$commit" \ + '{"comments": {"/PATCHSET_LEVEL":[{"message": "Depends-on: 10 30"}]}}') +result_out "depends-on patchset level comment - REST" "$expected" "$out" + exit $RESULT