Add support for --depends-on--all query option
Add --depends-on--all option to gerrit query SSH command and REST API.
This option will add a plugin section to the output which displays all
the depends-ons from the latest Depends-on comment on the change. This
option helps in building a depends-on web UI in future. Also add
functional tests and documentation.
Change-Id: I713f7765f47d115d499299f449fe08a70dd1a532
diff --git a/src/main/java/com/googlesource/gerrit/plugins/depends/on/ChangeMessageStore.java b/src/main/java/com/googlesource/gerrit/plugins/depends/on/ChangeMessageStore.java
index 5bfccd5..6901c90 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/depends/on/ChangeMessageStore.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/depends/on/ChangeMessageStore.java
@@ -85,7 +85,11 @@
*
* <p>return empty set means no dependencies found.
*/
- public Set<DependsOn> load(Change.Id cid) {
+ public Set<DependsOn> load(Change.Id cid) throws StorageException {
+ return loadWithOrder(cid).stream().collect(Collectors.toSet());
+ }
+
+ public List<DependsOn> loadWithOrder(Change.Id cid) throws StorageException {
ChangeNotes changeNote = changeNotesFactory.createCheckedUsingIndexLookup(cid);
List<ChangeMessage> messages = cmUtil.byChange(changeNote);
List<ChangeMessage> sortedChangeMessages =
@@ -93,12 +97,12 @@
.sorted(Comparator.comparing(ChangeMessage::getWrittenOn).reversed())
.collect(Collectors.toCollection(ArrayList::new));
for (ChangeMessage message : sortedChangeMessages) {
- Optional<Set<DependsOn>> deps = Comment.from(message.getMessage());
+ Optional<List<DependsOn>> deps = Comment.from(message.getMessage());
if (deps.isPresent()) {
return deps.get();
}
}
- return Collections.emptySet();
+ return Collections.emptyList();
}
/** If needed, create a comment on the change with a DependsOn for the dependencies. */
diff --git a/src/main/java/com/googlesource/gerrit/plugins/depends/on/DependsOnAttributeFactory.java b/src/main/java/com/googlesource/gerrit/plugins/depends/on/DependsOnAttributeFactory.java
new file mode 100644
index 0000000..3badb76
--- /dev/null
+++ b/src/main/java/com/googlesource/gerrit/plugins/depends/on/DependsOnAttributeFactory.java
@@ -0,0 +1,92 @@
+// 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.gerrit.common.Nullable;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.common.PluginDefinedInfo;
+import com.google.gerrit.server.DynamicOptions;
+import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory;
+import com.google.gerrit.server.query.change.ChangeData;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import javax.inject.Inject;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class DependsOnAttributeFactory implements ChangePluginDefinedInfoFactory {
+ private static final Logger log = LoggerFactory.getLogger(DependsOnAttributeFactory.class);
+ protected static final String FAILED_TO_LOAD_DEPENDS_ON = "Failed to load Depends-on";
+
+ protected final ChangeMessageStore changeMessageStore;
+
+ @Inject
+ DependsOnAttributeFactory(ChangeMessageStore changeMessageStore) {
+ this.changeMessageStore = changeMessageStore;
+ }
+
+ @Override
+ public Map<Change.Id, PluginDefinedInfo> createPluginDefinedInfos(
+ Collection<ChangeData> cds, DynamicOptions.BeanProvider beanProvider, String plugin) {
+ if (!((Module.MyQueryOptions) beanProvider.getDynamicBean(plugin)).all) {
+ return Collections.emptyMap();
+ }
+ Map<Change.Id, PluginDefinedInfo> dependsOnAttributesByChange = new HashMap<>();
+ for (ChangeData changeData : cds) {
+ try {
+ List<DependsOn> dependsOns = changeMessageStore.loadWithOrder(changeData.getId());
+ if (dependsOns.size() > 0) {
+ DependsOnPluginAttributes dependsOnPluginAttributes = new DependsOnPluginAttributes();
+ dependsOnPluginAttributes.addDependsOns(dependsOns);
+ dependsOnAttributesByChange.put(changeData.getId(), dependsOnPluginAttributes);
+ }
+ } catch (StorageException e) {
+ log.error(FAILED_TO_LOAD_DEPENDS_ON, e);
+ PluginDefinedInfo pluginDefinedInfo = new PluginDefinedInfo();
+ pluginDefinedInfo.message = FAILED_TO_LOAD_DEPENDS_ON;
+ dependsOnAttributesByChange.put(changeData.getId(), pluginDefinedInfo);
+ }
+ }
+ return dependsOnAttributesByChange;
+ }
+
+ protected static class DependsOnPluginAttributes extends PluginDefinedInfo {
+ public List<DependsOnAttribute> dependsOns = new ArrayList<>();
+
+ public void addDependsOns(List<DependsOn> dependsOns) {
+ for (DependsOn dependsOn : dependsOns) {
+ this.dependsOns.add(new DependsOnAttribute(dependsOn));
+ }
+ }
+ }
+
+ protected static class DependsOnAttribute {
+ @Nullable protected Integer changeNumber;
+ @Nullable protected String unresolved;
+
+ public DependsOnAttribute(DependsOn dependsOn) {
+ if (dependsOn.isResolved()) {
+ changeNumber = dependsOn.id().get();
+ } else {
+ unresolved = dependsOn.key().get();
+ }
+ }
+ }
+}
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 21644c1..b6eb1e1 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
@@ -14,16 +14,27 @@
package com.googlesource.gerrit.plugins.depends.on;
+import com.google.gerrit.entities.BranchNameKey;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.exceptions.StorageException;
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.change.ChangePluginDefinedInfoFactory;
import com.google.gerrit.server.events.EventListener;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeQueryBuilder.ChangeOperatorFactory;
import com.google.gerrit.server.restapi.change.GetChange;
import com.google.gerrit.server.restapi.change.QueryChanges;
import com.google.gerrit.sshd.commands.Query;
import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.googlesource.gerrit.plugins.depends.on.extensions.DependencyResolver;
+import java.util.Set;
+import org.kohsuke.args4j.Option;
public class Module extends AbstractModule {
@Override
@@ -32,15 +43,39 @@
.annotatedWith(Exports.named(InDependsOnOperator.FIELD))
.to(InDependsOnOperator.class);
DynamicSet.bind(binder(), EventListener.class).to(CoreListener.class);
- bind(DynamicBean.class)
- .annotatedWith(Exports.named(GetChange.class))
- .to(ChangeMessageStore.class);
- bind(DynamicBean.class).annotatedWith(Exports.named(Query.class)).to(ChangeMessageStore.class);
+ bind(ChangePluginDefinedInfoFactory.class)
+ .annotatedWith(Exports.named("depends-ons"))
+ .to(DependsOnAttributeFactory.class);
+ bind(DynamicBean.class).annotatedWith(Exports.named(GetChange.class)).to(MyQueryOptions.class);
+ bind(DynamicBean.class).annotatedWith(Exports.named(Query.class)).to(MyQueryOptions.class);
bind(DynamicBean.class)
.annotatedWith(Exports.named(QueryChanges.class))
- .to(ChangeMessageStore.class);
+ .to(MyQueryOptions.class);
bind(CommentValidator.class)
.annotatedWith(Exports.named(DependsOnCommentValidator.class.getSimpleName()))
.to(DependsOnCommentValidator.class);
}
+
+ public static class MyQueryOptions implements DependencyResolver {
+ @Option(name = "--all", usage = "Include all depends-on in the output")
+ public boolean all = false;
+
+ protected final ChangeMessageStore changeMessageStore;
+
+ @Inject
+ public MyQueryOptions(ChangeMessageStore changeMessageStore) {
+ this.changeMessageStore = changeMessageStore;
+ }
+
+ @Override
+ public boolean resolveDependencies(PatchSet.Id patchSetId, Set<Set<BranchNameKey>> deliverables)
+ throws InvalidChangeOperationException, StorageException, NoSuchChangeException {
+ return changeMessageStore.resolveDependencies(patchSetId, deliverables);
+ }
+
+ @Override
+ public boolean hasUnresolvedDependsOn(Change.Id changeId) throws StorageException {
+ return changeMessageStore.hasUnresolvedDependsOn(changeId);
+ }
+ }
}
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 e911560..8b35bcd 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
@@ -16,6 +16,7 @@
import com.googlesource.gerrit.plugins.depends.on.DependsOn;
import java.util.Arrays;
+import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Matcher;
@@ -30,7 +31,7 @@
protected static final Pattern DEPENDS_ON_PATTERN = Pattern.compile("^Depends-on:(.*)$");
/** return empty Optional instance means no dependencies found */
- public static Optional<Set<DependsOn>> from(String comment) {
+ public static Optional<List<DependsOn>> from(String comment) {
for (String line : comment.split("\n", -1)) {
Matcher match = DEPENDS_ON_PATTERN.matcher(line);
if (match.find()) {
@@ -41,7 +42,7 @@
Arrays.stream(changes.split("\\s+", -1)) // -> ["1234", "4444"]
.filter(c -> !c.isEmpty())
.map(c -> DependsOn.create(c))
- .collect(Collectors.toSet()));
+ .collect(Collectors.toList()));
}
}
return Optional.empty();
diff --git a/src/main/resources/Documentation/change-search-attributes.md b/src/main/resources/Documentation/change-search-attributes.md
new file mode 100644
index 0000000..cd8e7cf
--- /dev/null
+++ b/src/main/resources/Documentation/change-search-attributes.md
@@ -0,0 +1,45 @@
+Search Attributes
+=================
+
+Change Query Output
+-------------------
+It is possible to add a dependsOns section to the query output of changes
+using the @PLUGIN@ plugin switches. The following switches are available:
+
+**\-\-@PLUGIN@\-\-all**
+
+This switch is meant to be used to show all depends-ons from the latest
+Depends-on comment on the change. The switch output order matches the
+order from the change comment.
+
+When a change has one or more depends-on associated with it, query output will
+have a "dependsOns" section under the plugins section like below:
+
+```
+ $ ssh -x -p 29418 example.com gerrit query --@PLUGIN@--all \
+ --format JSON change:144193 | head -1 | json_pp
+ {
+ "id" : "Ifc577c2660c26220c39df57627ca1053c3f2067c",
+ ...
+ "plugins" : [
+ {
+ "name" : "@PLUGIN@",
+ "dependsOns" : [
+ {
+ "changeNumber": 732
+ },
+ {
+ "changeNumber": 733
+ },
+ {
+ "unresolved": "Ieace383c14de79bf202c85063d5a46a0580724dd"
+ },
+ {
+ "changeNumber": 734
+ }
+ ],
+ "name": "depends-on"
+ }
+ ]
+ }
+```
diff --git a/src/test/java/com/googlesource/gerrit/plugins/depends/on/DependsOnParsingTest.java b/src/test/java/com/googlesource/gerrit/plugins/depends/on/DependsOnParsingTest.java
index 52de2a7..491d572 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/depends/on/DependsOnParsingTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/depends/on/DependsOnParsingTest.java
@@ -16,8 +16,8 @@
import com.google.gerrit.testing.InMemoryModule;
import com.googlesource.gerrit.plugins.depends.on.formats.Comment;
+import java.util.List;
import java.util.Optional;
-import java.util.Set;
import junit.framework.TestCase;
import org.junit.Test;
@@ -68,21 +68,21 @@
@Test
public void testParseNoneComment() {
String comment = "My Very Educated Mother Just Served Us Nothing!";
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertFalse(deps.isPresent());
}
@Test
public void testParseEmptyComment() {
String comment = "Depends-on:";
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertTrue(deps.get().size() == 0);
}
@Test
public void testParseOneNumComment() {
String comment = "Depends-on:" + NUM;
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
for (DependsOn dep : deps.get()) {
assertTrue(NUM.equals("" + dep.id().get()));
return;
@@ -93,7 +93,7 @@
@Test
public void testParseOneKeyComment() {
String comment = "Depends-on:" + KEY;
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
for (DependsOn dep : deps.get()) {
assertTrue(KEY.equals("" + dep.key().get()));
return;
@@ -104,7 +104,7 @@
@Test
public void testParseTwoNumsComment() {
String comment = "Depends-on:" + NUM + " " + NUM2;
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertTrue(deps.get().size() == 2);
int found = 0;
for (DependsOn dep : deps.get()) {
@@ -117,7 +117,7 @@
@Test
public void testParseTwoKeyComments() {
String comment = "Depends-on:" + KEY + " " + KEY2;
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertTrue(deps.get().size() == 2);
int found = 0;
for (DependsOn dep : deps.get()) {
@@ -130,7 +130,7 @@
@Test
public void testParseNumAndKeyComment() {
String comment = "Depends-on:" + NUM + " " + KEY;
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertTrue(deps.get().size() == 2);
int found = 0;
for (DependsOn dep : deps.get()) {
@@ -146,7 +146,7 @@
public void testParseTwoNumsCommaComment() {
String comment = "Depends-on:" + NUM + "," + NUM2;
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertTrue(deps.get().size() == 2);
int found = 0;
for (DependsOn dep : deps.get()) {
@@ -158,7 +158,7 @@
public void testParseTwoNumsCommaSpaceComment() {
String comment = "Depends-on:" + NUM + ", " + NUM2;
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertTrue(deps.get().size() == 2);
int found = 0;
for (DependsOn dep : deps.get()) {
@@ -170,7 +170,7 @@
public void testParseTwoNumsWhiteComment() {
String comment = "Depends-on:" + NUM + ", \t" + NUM2;
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertTrue(deps.get().size() == 2);
int found = 0;
for (DependsOn dep : deps.get()) {
@@ -183,7 +183,7 @@
public void testParseTwoNumsNewLineWhiteComment() {
// Should stop processing at newline
String comment = "Depends-on:" + NUM + ", \t\n" + NUM2;
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertTrue(deps.get().size() == 1);
for (DependsOn dep : deps.get()) {
assertTrue(NUM.equals("" + dep.id().get()));
@@ -192,7 +192,7 @@
public void testParseEmbeddedComment() {
String comment = "Patch Set 2:\n\nDepends-on:" + NUM + " " + NUM2 + "\nHey\n";
- Optional<Set<DependsOn>> deps = Comment.from(comment);
+ Optional<List<DependsOn>> deps = Comment.from(comment);
assertTrue(deps.get().size() == 2);
int found = 0;
for (DependsOn dep : deps.get()) {
diff --git a/test/test_dependson.sh b/test/test_dependson.sh
index cc68ced..7e78684 100755
--- a/test/test_dependson.sh
+++ b/test/test_dependson.sh
@@ -171,7 +171,7 @@
GIT_DIR=$REPO_DIR/.git
FILE_A=$REPO_DIR/fileA
-# ------------------------- Depends-on Test ---------------------------
+# ------------------------- Depends-on propagation Test ---------------------------
base_change=$(create_change "$SRC_REF_BRANCH" "$FILE_A") || exit
src_change=$(create_change "$SRC_REF_BRANCH" "$FILE_A") || exit
gssh gerrit review --message \'"Depends-on: $base_change"\' "$src_change",1
@@ -180,7 +180,6 @@
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")
@@ -213,4 +212,19 @@
'{"comments": {"/PATCHSET_LEVEL":[{"message": "Depends-on: 10 30"}]}}')
result_out "depends-on patchset level comment - REST" "$expected" "$out"
+# ------------------------- Depends-on query Test ---------------------------
+change=$(create_change "$SRC_REF_BRANCH" "$FILE_A") || exit
+gssh gerrit review --message \
+ \'"Depends-on: 10 30 Ieace383c14de79bf202c85063d5a46a0580724dd 20"\' "$change",1
+out=$(query "$change" --depends-on--all | jq --raw-output '.plugins[0].dependsOns')
+result "depends-on query"
+result_out "depends-on query output 1" "10" "$(echo "$out" | jq '.[0].changeNumber')"
+result_out "depends-on query output 2" "30" "$(echo "$out" | jq '.[1].changeNumber')"
+result_out "depends-on query output 3" "Ieace383c14de79bf202c85063d5a46a0580724dd" \
+ "$(echo "$out" | jq --raw-output '.[2].unresolved')"
+result_out "depends-on query output 4" "20" "$(echo "$out" | jq '.[3].changeNumber')"
+result_out "depends-on query output datatype" "number number string number" \
+ "$(echo "$out" | jq --raw-output 'map(select(.changeNumber!=null).changeNumber,
+ select(.unresolved!=null).unresolved | type) | join(" ")')"
+
exit $RESULT
