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
