commit 0ddd3d2071f43ff6e702c34183568cec19f0af45 Author: Adithya Chakilam Date: Thu Sep 3 14:46:17 2020 -0500 Allow plugins to provide bulk PluginDefinedInfos in query results Create a ChangePluginDefinedInfoFactory interface to allow list of ChangeData for plugins attribute to get bulk data to provide additional attributes to be output in a change query result. A common use case for this is for a class implementing ChangePluginDefinedInfoFactory to handle many changes instead of a single change, for which all have plugin-specific results added in the query output. This results into decrease of roundtrip time for the queries and increases the performance for bulk queries. Change-Id: I294f29557e98d6ff4f4d82ff3d83c4a743aa622d diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index b4ae469..8f81775 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -864,13 +864,15 @@ boolean json; [[query_attributes]] === Change Attributes === +==== ChangePluginDefinedInfoFactory + Plugins can provide additional attributes to be returned from the Get Change and -Query Change APIs by implementing implementing the `ChangeAttributeFactory` -interface and adding it to the `DynamicSet` in the plugin module's `configure()` -method. The new attribute(s) will be output under a `plugin` attribute in the -change output. This can be further controlled by registering a class containing -@Option declarations as a `DynamicBean`, annotated with the with HTTP/SSH -commands on which the options should be available. +Query Change APIs by implementing the `ChangePluginDefinedInfoFactory` interface +and adding it to the `DynamicSet` in the plugin module's `configure()` method. +The new attribute(s) will be output under a `plugin` attribute in the change +output. This can be further controlled by registering a class containing @Option +declarations as a `DynamicBean`, annotated with the HTTP/SSH commands on +which the options should be available. The example below shows a plugin that adds two attributes (`exampleName` and `changeValue`), to the change query output, when the query command is provided @@ -882,7 +884,7 @@ public class Module extends AbstractModule { @Override protected void configure() { // Register attribute factory. - DynamicSet.bind(binder(), ChangeAttributeFactory.class) + DynamicSet.bind(binder(), ChangePluginDefinedInfoFactory.class) .to(AttributeFactory.class); // Register options for GET /changes/X/change and /changes/X/detail. @@ -907,7 +909,7 @@ public class MyChangeOptions implements DynamicBean { public boolean all = false; } -public class AttributeFactory implements ChangeAttributeFactory { +public class AttributeFactory implements ChangePluginDefinedInfoFactory { protected MyChangeOptions options; public class PluginAttribute extends PluginDefinedInfo { @@ -921,14 +923,17 @@ public class AttributeFactory implements ChangeAttributeFactory { } @Override - public PluginDefinedInfo create(ChangeData c, BeanProvider bp, String plugin) { + public Map createPluginDefinedInfos( + Collection cds, BeanProvider bp, String plugin) { if (options == null) { options = (MyChangeOptions) bp.getDynamicBean(plugin); } + Map out = new HashMap<>(); if (options.all) { - return new PluginAttribute(c); + cds.forEach(cd -> out.put(cd.getId(), new PluginAttribute(cd))); + return out; } - return null; + return ImmutableMap.of(); } } ---- @@ -970,10 +975,20 @@ Output: } ---- -Implementors of the `ChangeAttributeFactory` interface should check whether -they need to contribute to the link:#change-etag-computation[change ETag -computation] to prevent callers using ETags from potentially seeing outdated -plugin attributes. +Runtime exceptions generated by the implementors of ChangePluginDefinedInfoFactory +are encapsulated in PluginDefinedInfo objects which are part of SSH/REST query output. + +==== ChangeAttributeFactory + +Alternatively, there is also `ChangeAttributeFactory` which takes in one single +`ChangeData` at a time. `ChangePluginDefinedInfoFactory` should be preferred +over this as it handles many changes at once which also decreases the round-trip +time for queries resulting in performance increase for bulk queries. + +Implementors of the `ChangePluginDefinedInfoFactory` and `ChangeAttributeFactory` +interfaces should check whether they need to contribute to the +link:#change-etag-computation[change ETag computation] to prevent callers using +ETags from potentially seeing outdated plugin attributes. [[simple-configuration]] == Simple Configuration in `gerrit.config` diff --git a/java/com/google/gerrit/acceptance/AbstractPluginFieldsTest.java b/java/com/google/gerrit/acceptance/AbstractPluginFieldsTest.java index 020602b..a91bc49 100644 --- a/java/com/google/gerrit/acceptance/AbstractPluginFieldsTest.java +++ b/java/com/google/gerrit/acceptance/AbstractPluginFieldsTest.java @@ -21,26 +21,36 @@ import static java.util.Objects.requireNonNull; import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableListMultimap; +import com.google.gerrit.acceptance.testsuite.change.ChangeOperations; import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Change; import com.google.gerrit.extensions.annotations.Exports; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.server.DynamicOptions; import com.google.gerrit.server.DynamicOptions.DynamicBean; import com.google.gerrit.server.change.ChangeAttributeFactory; +import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory; +import com.google.gerrit.server.query.change.ChangeData; 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.gson.Gson; import com.google.gson.reflect.TypeToken; import com.google.inject.AbstractModule; +import com.google.inject.Inject; import com.google.inject.Module; +import java.util.Collection; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Objects; import org.kohsuke.args4j.Option; public class AbstractPluginFieldsTest extends AbstractDaemonTest { + @Inject private ChangeOperations changeOperations; + protected static class MyInfo extends PluginDefinedInfo { @Nullable String theAttribute; @@ -91,6 +101,70 @@ public class AbstractPluginFieldsTest extends AbstractDaemonTest { } } + protected static class PluginDefinedSimpleAttributeModule extends AbstractModule { + @Override + public void configure() { + DynamicSet.bind(binder(), ChangePluginDefinedInfoFactory.class) + .toInstance( + (cds, bp, p) -> { + Map out = new HashMap<>(); + cds.forEach(cd -> out.put(cd.getId(), new MyInfo("change " + cd.getId()))); + return out; + }); + } + } + + protected static class PluginDefinedBulkExceptionModule extends AbstractModule { + @Override + protected void configure() { + DynamicSet.bind(binder(), ChangePluginDefinedInfoFactory.class) + .toInstance( + (cds, bp, p) -> { + throw new RuntimeException("Sample Exception"); + }); + } + } + + protected static class PluginDefinedChangesByCommitBulkAttributeModule extends AbstractModule { + @Override + public void configure() { + DynamicSet.bind(binder(), ChangePluginDefinedInfoFactory.class) + .toInstance( + (cds, bp, p) -> { + Map out = new HashMap<>(); + cds.forEach( + cd -> + out.put( + cd.getId(), + !cd.commitMessage().contains("no-info") + ? new MyInfo("change " + cd.getId()) + : null)); + return out; + }); + } + } + + protected static class PluginDefinedSingleCallBulkAttributeModule extends AbstractModule { + @Override + public void configure() { + DynamicSet.bind(binder(), ChangePluginDefinedInfoFactory.class) + .to(SingleCallBulkFactoryAttribute.class); + } + } + + protected static class SingleCallBulkFactoryAttribute implements ChangePluginDefinedInfoFactory { + public static int timesCreateCalled = 0; + + @Override + public Map createPluginDefinedInfos( + Collection cds, DynamicOptions.BeanProvider beanProvider, String plugin) { + timesCreateCalled++; + Map out = new HashMap<>(); + cds.forEach(cd -> out.put(cd.getId(), new MyInfo("change " + cd.getId()))); + return out; + } + } + private static class MyOptions implements DynamicBean { @Option(name = "--opt") private String opt; @@ -111,6 +185,32 @@ public class AbstractPluginFieldsTest extends AbstractDaemonTest { } } + public static class BulkAttributeFactoryWithOption implements ChangePluginDefinedInfoFactory { + protected MyOptions opts; + + @Override + public Map createPluginDefinedInfos( + Collection cds, DynamicOptions.BeanProvider beanProvider, String plugin) { + if (opts == null) { + opts = (MyOptions) beanProvider.getDynamicBean(plugin); + } + Map out = new HashMap<>(); + cds.forEach(cd -> out.put(cd.getId(), new MyInfo("opt " + opts.opt))); + return out; + } + } + + protected static class PluginDefinedOptionAttributeModule extends AbstractModule { + @Override + public void configure() { + DynamicSet.bind(binder(), ChangePluginDefinedInfoFactory.class) + .to(BulkAttributeFactoryWithOption.class); + bind(DynamicBean.class).annotatedWith(Exports.named(Query.class)).to(MyOptions.class); + bind(DynamicBean.class).annotatedWith(Exports.named(QueryChanges.class)).to(MyOptions.class); + bind(DynamicBean.class).annotatedWith(Exports.named(GetChange.class)).to(MyOptions.class); + } + } + protected void getChangeWithNullAttribute(PluginInfoGetter getter) throws Exception { Change.Id id = createChange().getChange().getId(); assertThat(getter.call(id)).isNull(); @@ -138,6 +238,113 @@ public class AbstractPluginFieldsTest extends AbstractDaemonTest { assertThat(getter.call(id)).isNull(); } + protected void getSingleChangeWithPluginDefinedBulkAttribute(BulkPluginInfoGetterWithId getter) + throws Exception { + Change.Id id = createChange().getChange().getId(); + + Map> pluginInfos = getter.call(id); + assertThat(pluginInfos.get(id)).isNull(); + + try (AutoCloseable ignored = + installPlugin("my-plugin", PluginDefinedSimpleAttributeModule.class)) { + pluginInfos = getter.call(id); + assertThat(pluginInfos.get(id)).containsExactly(new MyInfo("my-plugin", "change " + id)); + } + + pluginInfos = getter.call(id); + assertThat(pluginInfos.get(id)).isNull(); + } + + protected void getMultipleChangesWithPluginDefinedBulkAttribute(BulkPluginInfoGetter getter) + throws Exception { + Change.Id id1 = createChange().getChange().getId(); + Change.Id id2 = createChange().getChange().getId(); + + Map> pluginInfos = getter.call(); + assertThat(pluginInfos.get(id1)).isNull(); + assertThat(pluginInfos.get(id2)).isNull(); + + try (AutoCloseable ignored = + installPlugin("my-plugin", PluginDefinedSimpleAttributeModule.class)) { + pluginInfos = getter.call(); + assertThat(pluginInfos.get(id1)).containsExactly(new MyInfo("my-plugin", "change " + id1)); + assertThat(pluginInfos.get(id2)).containsExactly(new MyInfo("my-plugin", "change " + id2)); + } + + pluginInfos = getter.call(); + assertThat(pluginInfos.get(id1)).isNull(); + assertThat(pluginInfos.get(id2)).isNull(); + } + + protected void getChangesByCommitMessageWithPluginDefinedBulkAttribute( + BulkPluginInfoGetter getter) throws Exception { + Change.Id changeWithNoInfo = changeOperations.newChange().commitMessage("no-info").create(); + Change.Id changeWithInfo = changeOperations.newChange().commitMessage("info").create(); + + Map> pluginInfos = getter.call(); + assertThat(pluginInfos.get(changeWithNoInfo)).isNull(); + assertThat(pluginInfos.get(changeWithInfo)).isNull(); + + try (AutoCloseable ignored = + installPlugin("my-plugin", PluginDefinedChangesByCommitBulkAttributeModule.class)) { + pluginInfos = getter.call(); + assertThat(pluginInfos.get(changeWithNoInfo)).isNull(); + assertThat(pluginInfos.get(changeWithInfo)) + .containsExactly(new MyInfo("my-plugin", "change " + changeWithInfo)); + } + + pluginInfos = getter.call(); + assertThat(pluginInfos.get(changeWithNoInfo)).isNull(); + assertThat(pluginInfos.get(changeWithInfo)).isNull(); + } + + protected void getMultipleChangesWithPluginDefinedBulkAndChangeAttributes( + BulkPluginInfoGetter getter) throws Exception { + Change.Id id1 = createChange().getChange().getId(); + Change.Id id2 = createChange().getChange().getId(); + + Map> pluginInfos = getter.call(); + assertThat(pluginInfos.get(id1)).isNull(); + assertThat(pluginInfos.get(id2)).isNull(); + + try (AutoCloseable ignored = + installPlugin("my-plugin-1", PluginDefinedSimpleAttributeModule.class); + AutoCloseable ignored1 = installPlugin("my-plugin-2", SimpleAttributeModule.class)) { + pluginInfos = getter.call(); + assertThat(pluginInfos.get(id1)).contains(new MyInfo("my-plugin-1", "change " + id1)); + assertThat(pluginInfos.get(id1)).contains(new MyInfo("my-plugin-2", "change " + id1)); + assertThat(pluginInfos.get(id2)).contains(new MyInfo("my-plugin-1", "change " + id2)); + assertThat(pluginInfos.get(id2)).contains(new MyInfo("my-plugin-2", "change " + id2)); + } + + pluginInfos = getter.call(); + assertThat(pluginInfos.get(id1)).isNull(); + assertThat(pluginInfos.get(id2)).isNull(); + } + + protected void getMultipleChangesWithPluginDefinedBulkAttributeInSingleCall( + BulkPluginInfoGetter getter) throws Exception { + Change.Id id1 = createChange().getChange().getId(); + Change.Id id2 = createChange().getChange().getId(); + int timesCalled = SingleCallBulkFactoryAttribute.timesCreateCalled; + + Map> pluginInfos = getter.call(); + assertThat(pluginInfos.get(id1)).isNull(); + assertThat(pluginInfos.get(id2)).isNull(); + + try (AutoCloseable ignored = + installPlugin("my-plugin", PluginDefinedSingleCallBulkAttributeModule.class)) { + pluginInfos = getter.call(); + assertThat(pluginInfos.get(id1)).containsExactly(new MyInfo("my-plugin", "change " + id1)); + assertThat(pluginInfos.get(id2)).containsExactly(new MyInfo("my-plugin", "change " + id2)); + assertThat(SingleCallBulkFactoryAttribute.timesCreateCalled).isEqualTo(timesCalled + 1); + } + + pluginInfos = getter.call(); + assertThat(pluginInfos.get(id1)).isNull(); + assertThat(pluginInfos.get(id2)).isNull(); + } + protected void getChangeWithOption( PluginInfoGetter getterWithoutOptions, PluginInfoGetterWithOptions getterWithOptions) throws Exception { @@ -154,17 +361,61 @@ public class AbstractPluginFieldsTest extends AbstractDaemonTest { assertThat(getterWithoutOptions.call(id)).isNull(); } - protected static List pluginInfoFromSingletonList(List changeInfos) { + protected void getChangeWithPluginDefinedBulkAttributeOption( + BulkPluginInfoGetterWithId getterWithoutOptions, + BulkPluginInfoGetterWithIdAndOptions getterWithOptions) + throws Exception { + Change.Id id = createChange().getChange().getId(); + assertThat(getterWithoutOptions.call(id).get(id)).isNull(); + + try (AutoCloseable ignored = + installPlugin("my-plugin", PluginDefinedOptionAttributeModule.class)) { + assertThat(getterWithoutOptions.call(id).get(id)) + .containsExactly(new MyInfo("my-plugin", "opt null")); + assertThat( + getterWithOptions.call(id, ImmutableListMultimap.of("my-plugin--opt", "foo")).get(id)) + .containsExactly(new MyInfo("my-plugin", "opt foo")); + } + + assertThat(getterWithoutOptions.call(id).get(id)).isNull(); + } + + protected void getChangeWithPluginDefinedBulkAttributeWithException( + BulkPluginInfoGetterWithId getter) throws Exception { + Change.Id id = createChange().getChange().getId(); + assertThat(getter.call(id).get(id)).isNull(); + + try (AutoCloseable ignored = + installPlugin("my-plugin", PluginDefinedBulkExceptionModule.class)) { + PluginDefinedInfo errorInfo = new PluginDefinedInfo(); + List outputInfos = getter.call(id).get(id); + assertThat(outputInfos).hasSize(1); + assertThat(outputInfos.get(0).name).isEqualTo("my-plugin"); + assertThat(outputInfos.get(0).message).isEqualTo("Something went wrong in plugin: my-plugin"); + } + + assertThat(getter.call(id).get(id)).isNull(); + } + + protected static List pluginInfoFromSingletonList( + List changeInfos) { assertThat(changeInfos).hasSize(1); return pluginInfoFromChangeInfo(changeInfos.get(0)); } - protected static List pluginInfoFromChangeInfo(ChangeInfo changeInfo) { + protected static List pluginInfoFromChangeInfo(ChangeInfo changeInfo) { List pluginInfo = changeInfo.plugins; if (pluginInfo == null) { return null; } - return pluginInfo.stream().map(MyInfo.class::cast).collect(toImmutableList()); + return pluginInfo.stream().map(PluginDefinedInfo.class::cast).collect(toImmutableList()); + } + + protected static Map> pluginInfosFromChangeInfos( + List changeInfos) { + Map> out = new HashMap<>(); + changeInfos.forEach(ci -> out.put(Change.id(ci._number), pluginInfoFromChangeInfo(ci))); + return out; } /** @@ -180,7 +431,8 @@ public class AbstractPluginFieldsTest extends AbstractDaemonTest { * @param plugins list of {@code MyInfo} objects, each as a raw map returned from Gson. * @return decoded list of {@code MyInfo}s. */ - protected static List decodeRawPluginsList(Gson gson, @Nullable Object plugins) { + protected static List decodeRawPluginsList( + Gson gson, @Nullable Object plugins) { if (plugins == null) { return null; } @@ -188,14 +440,44 @@ public class AbstractPluginFieldsTest extends AbstractDaemonTest { return gson.fromJson(gson.toJson(plugins), new TypeToken>() {}.getType()); } + protected static Map> getPluginInfosFromChangeInfos( + Gson gson, List> changeInfos) { + Map> out = new HashMap<>(); + changeInfos.forEach( + change -> { + Double changeId = + (Double) + (change.get("number") != null ? change.get("number") : change.get("_number")); + out.put( + Change.id(changeId.intValue()), decodeRawPluginsList(gson, change.get("plugins"))); + }); + return out; + } + @FunctionalInterface protected interface PluginInfoGetter { - List call(Change.Id id) throws Exception; + List call(Change.Id id) throws Exception; + } + + @FunctionalInterface + protected interface BulkPluginInfoGetter { + Map> call() throws Exception; + } + + @FunctionalInterface + protected interface BulkPluginInfoGetterWithId { + Map> call(Change.Id id) throws Exception; + } + + @FunctionalInterface + protected interface BulkPluginInfoGetterWithIdAndOptions { + Map> call( + Change.Id id, ImmutableListMultimap pluginOptions) throws Exception; } @FunctionalInterface protected interface PluginInfoGetterWithOptions { - List call(Change.Id id, ImmutableListMultimap pluginOptions) + List call(Change.Id id, ImmutableListMultimap pluginOptions) throws Exception; } } diff --git a/java/com/google/gerrit/extensions/common/PluginDefinedInfo.java b/java/com/google/gerrit/extensions/common/PluginDefinedInfo.java index e6fef0f..69bfa2c 100644 --- a/java/com/google/gerrit/extensions/common/PluginDefinedInfo.java +++ b/java/com/google/gerrit/extensions/common/PluginDefinedInfo.java @@ -16,4 +16,5 @@ package com.google.gerrit.extensions.common; public class PluginDefinedInfo { public String name; + public String message; } diff --git a/java/com/google/gerrit/server/change/ChangeAttributeFactory.java b/java/com/google/gerrit/server/change/ChangeAttributeFactory.java index 95355cf..663d7aa 100644 --- a/java/com/google/gerrit/server/change/ChangeAttributeFactory.java +++ b/java/com/google/gerrit/server/change/ChangeAttributeFactory.java @@ -32,6 +32,7 @@ import com.google.gerrit.server.query.change.ChangeData; * href="https://gerrit-review.googlesource.com/Documentation/dev-plugins.html#query_attributes">plugin * developer documentation for more details and examples. */ +@Deprecated public interface ChangeAttributeFactory { /** diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index 014955c..8323cfd 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java @@ -39,6 +39,7 @@ import static java.util.stream.Collectors.toList; import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Throwables; +import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; @@ -67,6 +68,7 @@ import com.google.gerrit.extensions.common.AttentionSetInfo; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeMessageInfo; import com.google.gerrit.extensions.common.LabelInfo; +import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.extensions.common.ProblemInfo; import com.google.gerrit.extensions.common.ReviewerUpdateInfo; import com.google.gerrit.extensions.common.RevisionInfo; @@ -156,13 +158,17 @@ public class ChangeJson { } public ChangeJson create(Iterable options) { - return factory.create(options, Optional.empty()); + return factory.create(options, Optional.empty(), Optional.empty()); } public ChangeJson create( Iterable options, - PluginDefinedAttributesFactory pluginDefinedAttributesFactory) { - return factory.create(options, Optional.of(pluginDefinedAttributesFactory)); + PluginDefinedAttributesFactory pluginDefinedAttributesFactory, + PluginDefinedInfosFactory pluginDefinedInfosFactory) { + return factory.create( + options, + Optional.of(pluginDefinedAttributesFactory), + Optional.of(pluginDefinedInfosFactory)); } public ChangeJson create(ListChangesOption first, ListChangesOption... rest) { @@ -173,7 +179,8 @@ public class ChangeJson { public interface AssistedFactory { ChangeJson create( Iterable options, - Optional pluginDefinedAttributesFactory); + Optional pluginDefinedAttributesFactory, + Optional pluginDefinedInfosFactory); } @Singleton @@ -220,6 +227,7 @@ public class ChangeJson { private final Metrics metrics; private final RevisionJson revisionJson; private final Optional pluginDefinedAttributesFactory; + private final Optional pluginDefinedInfosFactory; private final boolean includeMergeable; private final boolean lazyLoad; @@ -243,7 +251,8 @@ public class ChangeJson { RevisionJson.Factory revisionJsonFactory, @GerritServerConfig Config cfg, @Assisted Iterable options, - @Assisted Optional pluginDefinedAttributesFactory) { + @Assisted Optional pluginDefinedAttributesFactory, + @Assisted Optional pluginDefinedInfosFactory) { this.userProvider = user; this.changeDataFactory = cdf; this.permissionBackend = permissionBackend; @@ -261,6 +270,7 @@ public class ChangeJson { this.includeMergeable = MergeabilityComputationBehavior.fromConfig(cfg).includeInApi(); this.lazyLoad = containsAnyOf(this.options, REQUIRE_LAZY_LOAD); this.pluginDefinedAttributesFactory = pluginDefinedAttributesFactory; + this.pluginDefinedInfosFactory = pluginDefinedInfosFactory; logger.atFine().log("options = %s", options); } @@ -279,12 +289,12 @@ public class ChangeJson { } public ChangeInfo format(ChangeData cd) { - return format(cd, Optional.empty(), true); + return format(cd, Optional.empty(), true, getPluginInfos(cd)); } public ChangeInfo format(RevisionResource rsrc) { ChangeData cd = changeDataFactory.create(rsrc.getNotes()); - return format(cd, Optional.of(rsrc.getPatchSet().id()), true); + return format(cd, Optional.of(rsrc.getPatchSet().id()), true, getPluginInfos(cd)); } public List> format(List> in) @@ -293,8 +303,10 @@ public class ChangeJson { accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); List> res = new ArrayList<>(in.size()); Map cache = Maps.newHashMapWithExpectedSize(in.size()); + ImmutableListMultimap pluginInfosByChange = + getPluginInfos(in.stream().flatMap(e -> e.entities().stream()).collect(toList())); for (QueryResult r : in) { - List infos = toChangeInfos(r.entities(), cache); + List infos = toChangeInfos(r.entities(), cache, pluginInfosByChange); if (!infos.isEmpty() && r.more()) { infos.get(infos.size() - 1)._moreChanges = true; } @@ -309,8 +321,9 @@ public class ChangeJson { accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); ensureLoaded(in); List out = new ArrayList<>(in.size()); + ImmutableListMultimap pluginInfosByChange = getPluginInfos(in); for (ChangeData cd : in) { - out.add(format(cd, Optional.empty(), false)); + out.add(format(cd, Optional.empty(), false, pluginInfosByChange.get(cd.getId()))); } accountLoader.fill(); return out; @@ -326,7 +339,8 @@ public class ChangeJson { } return checkOnly(changeDataFactory.create(project, id)); } - return format(changeDataFactory.create(notes), Optional.empty(), true); + ChangeData cd = changeDataFactory.create(notes); + return format(cd, Optional.empty(), true, getPluginInfos(cd)); } private static Collection requirementsFor(ChangeData cd) { @@ -358,15 +372,18 @@ public class ChangeJson { } private ChangeInfo format( - ChangeData cd, Optional limitToPsId, boolean fillAccountLoader) { + ChangeData cd, + Optional limitToPsId, + boolean fillAccountLoader, + List pluginInfosForChange) { try { if (fillAccountLoader) { accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS)); - ChangeInfo res = toChangeInfo(cd, limitToPsId); + ChangeInfo res = toChangeInfo(cd, limitToPsId, pluginInfosForChange); accountLoader.fill(); return res; } - return toChangeInfo(cd, limitToPsId); + return toChangeInfo(cd, limitToPsId, pluginInfosForChange); } catch (PatchListNotAvailableException | GpgException | IOException @@ -404,7 +421,9 @@ public class ChangeJson { } private List toChangeInfos( - List changes, Map cache) { + List changes, + Map cache, + ImmutableListMultimap pluginInfosByChange) { try (Timer0.Context ignored = metrics.toChangeInfosLatency.start()) { List changeInfos = new ArrayList<>(changes.size()); for (int i = 0; i < changes.size(); i++) { @@ -425,7 +444,7 @@ public class ChangeJson { // Compute and cache if possible try { ensureLoaded(Collections.singleton(cd)); - info = format(cd, Optional.empty(), false); + info = format(cd, Optional.empty(), false, pluginInfosByChange.get(cd.getId())); changeInfos.add(info); if (isCacheable) { cache.put(Change.id(info._number), info); @@ -480,14 +499,18 @@ public class ChangeJson { return info; } - private ChangeInfo toChangeInfo(ChangeData cd, Optional limitToPsId) + private ChangeInfo toChangeInfo( + ChangeData cd, + Optional limitToPsId, + List pluginInfosForChange) throws PatchListNotAvailableException, GpgException, PermissionBackendException, IOException { try (Timer0.Context ignored = metrics.toChangeInfoLatency.start()) { - return toChangeInfoImpl(cd, limitToPsId); + return toChangeInfoImpl(cd, limitToPsId, pluginInfosForChange); } } - private ChangeInfo toChangeInfoImpl(ChangeData cd, Optional limitToPsId) + private ChangeInfo toChangeInfoImpl( + ChangeData cd, Optional limitToPsId, List pluginInfos) throws PatchListNotAvailableException, GpgException, PermissionBackendException, IOException { ChangeInfo out = new ChangeInfo(); CurrentUser user = userProvider.get(); @@ -589,6 +612,15 @@ public class ChangeJson { if (pluginDefinedAttributesFactory.isPresent()) { out.plugins = pluginDefinedAttributesFactory.get().create(cd); } + + if (!pluginInfos.isEmpty()) { + if (out.plugins == null) { + out.plugins = pluginInfos; + } else { + out.plugins = new ArrayList<>(out.plugins); + out.plugins.addAll(pluginInfos); + } + } out.revertOf = cd.change().getRevertOf() != null ? cd.change().getRevertOf().get() : null; out.submissionId = cd.change().getSubmissionId(); out.cherryPickOfChange = @@ -819,4 +851,16 @@ public class ChangeJson { } return map; } + + private List getPluginInfos(ChangeData cd) { + return getPluginInfos(Collections.singleton(cd)).get(cd.getId()); + } + + private ImmutableListMultimap getPluginInfos( + Collection cds) { + if (pluginDefinedInfosFactory.isPresent()) { + return pluginDefinedInfosFactory.get().createPluginDefinedInfos(cds); + } + return ImmutableListMultimap.of(); + } } diff --git a/java/com/google/gerrit/server/change/ChangePluginDefinedInfoFactory.java b/java/com/google/gerrit/server/change/ChangePluginDefinedInfoFactory.java new file mode 100644 index 0000000..c6ceb61 --- /dev/null +++ b/java/com/google/gerrit/server/change/ChangePluginDefinedInfoFactory.java @@ -0,0 +1,52 @@ +// Copyright (C) 2020 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.entities.Change; +import com.google.gerrit.extensions.common.PluginDefinedInfo; +import com.google.gerrit.server.DynamicOptions.BeanProvider; +import com.google.gerrit.server.query.change.ChangeData; +import java.util.Collection; +import java.util.Map; + +/** + * Interface for plugins to provide additional fields in {@link + * com.google.gerrit.extensions.common.ChangeInfo ChangeInfo}. + * + *

Register a {@code ChangePluginDefinedInfoFactory} in a plugin {@code Module} like this: + * + * 

+ * DynamicSet.bind(binder(), ChangePluginDefinedInfoFactory.class).to(YourClass.class);
+ *
+ * + *

See the + * plugin developer documentation for more details and examples. + */ +public interface ChangePluginDefinedInfoFactory { + + /** + * Create a plugin-provided info field for each of the provided {@link ChangeData}s. + * + *

Typically, implementations will subclass {@code PluginDefinedInfo} to add additional fields. + * + * @param cds changes. + * @param beanProvider provider of {@code DynamicBean}s, which may be used for reading options. + * @param plugin plugin name. + * @return map of the plugin's special info for each change + */ + Map createPluginDefinedInfos( + Collection cds, BeanProvider beanProvider, String plugin); +} diff --git a/java/com/google/gerrit/server/change/PluginDefinedAttributesFactories.java b/java/com/google/gerrit/server/change/PluginDefinedAttributesFactories.java index 9928125..b474dab 100644 --- a/java/com/google/gerrit/server/change/PluginDefinedAttributesFactories.java +++ b/java/com/google/gerrit/server/change/PluginDefinedAttributesFactories.java @@ -18,12 +18,15 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static java.util.concurrent.TimeUnit.MINUTES; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.Nullable; +import com.google.gerrit.entities.Change; import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.extensions.registration.Extension; import com.google.gerrit.server.DynamicOptions.BeanProvider; import com.google.gerrit.server.query.change.ChangeData; +import java.util.Collection; import java.util.Objects; import java.util.stream.Stream; @@ -60,5 +63,44 @@ public class PluginDefinedAttributesFactories { return pdi; } + public static ImmutableListMultimap createAll( + Collection cds, + BeanProvider beanProvider, + Stream> infoFactories) { + ImmutableListMultimap.Builder pluginInfosByChangeBuilder = + ImmutableListMultimap.builder(); + infoFactories.forEach( + e -> tryCreate(cds, beanProvider, e.getPluginName(), e.get(), pluginInfosByChangeBuilder)); + ImmutableListMultimap result = pluginInfosByChangeBuilder.build(); + return result; + } + + private static void tryCreate( + Collection cds, + BeanProvider beanProvider, + String plugin, + ChangePluginDefinedInfoFactory infoFactory, + ImmutableListMultimap.Builder pluginInfosByChangeBuilder) { + try { + infoFactory + .createPluginDefinedInfos(cds, beanProvider, plugin) + .forEach( + (id, pdi) -> { + if (pdi != null) { + pdi.name = plugin; + pluginInfosByChangeBuilder.put(id, pdi); + } + }); + } catch (RuntimeException ex) { + /* Propagate runtime exceptions as structured API data types so that queries don't fail. */ + logger.atWarning().atMostEvery(1, MINUTES).withCause(ex).log( + "error populating attribute on changes from plugin %s", plugin); + PluginDefinedInfo errorInfo = new PluginDefinedInfo(); + errorInfo.name = plugin; + errorInfo.message = "Something went wrong in plugin: " + plugin; + cds.forEach(cd -> pluginInfosByChangeBuilder.put(cd.getId(), errorInfo)); + } + } + private PluginDefinedAttributesFactories() {} } diff --git a/java/com/google/gerrit/server/change/PluginDefinedInfosFactory.java b/java/com/google/gerrit/server/change/PluginDefinedInfosFactory.java new file mode 100644 index 0000000..db57e29 --- /dev/null +++ b/java/com/google/gerrit/server/change/PluginDefinedInfosFactory.java @@ -0,0 +1,42 @@ +// Copyright (C) 2020 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.common.collect.ImmutableListMultimap; +import com.google.gerrit.entities.Change; +import com.google.gerrit.extensions.common.PluginDefinedInfo; +import com.google.gerrit.server.query.change.ChangeData; +import java.util.Collection; + +/** + * Interface to generate {@code PluginDefinedInfo}s from registered {@code + * ChangePluginDefinedInfoFactory}s. + * + *

See the + * plugin developer documentation for more details and examples. + */ +public interface PluginDefinedInfosFactory { + + /** + * Create a plugin-provided info field from all the plugins for each of the provided {@link + * ChangeData}s. + * + * @param cds changes. + * @return map of the all plugin's special infos for each change. + */ + ImmutableListMultimap createPluginDefinedInfos( + Collection cds); +} diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index 22d02d2..78dd38c 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -110,6 +110,7 @@ import com.google.gerrit.server.change.ChangeETagComputation; import com.google.gerrit.server.change.ChangeFinder; import com.google.gerrit.server.change.ChangeJson; import com.google.gerrit.server.change.ChangeKindCacheImpl; +import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory; import com.google.gerrit.server.change.LabelsJson; import com.google.gerrit.server.change.MergeabilityCacheImpl; import com.google.gerrit.server.change.ReviewerSuggestion; @@ -437,6 +438,7 @@ public class GerritGlobalModule extends FactoryModule { DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeHasOperandFactory.class); DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeIsOperandFactory.class); DynamicSet.setOf(binder(), ChangeAttributeFactory.class); + DynamicSet.setOf(binder(), ChangePluginDefinedInfoFactory.class); install(new GitwebConfig.LegacyModule(cfg)); diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java index 40c0477..370bc75 100644 --- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java +++ b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java @@ -19,6 +19,7 @@ import static com.google.gerrit.server.query.change.ChangeQueryBuilder.FIELD_LIM import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; +import com.google.gerrit.entities.Change; import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.registration.Extension; @@ -33,15 +34,20 @@ import com.google.gerrit.server.DynamicOptions; import com.google.gerrit.server.DynamicOptions.DynamicBean; import com.google.gerrit.server.account.AccountLimits; import com.google.gerrit.server.change.ChangeAttributeFactory; +import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory; import com.google.gerrit.server.change.PluginDefinedAttributesFactories; import com.google.gerrit.server.change.PluginDefinedAttributesFactory; +import com.google.gerrit.server.change.PluginDefinedInfosFactory; import com.google.gerrit.server.index.change.ChangeIndexCollection; import com.google.gerrit.server.index.change.ChangeIndexRewriter; import com.google.gerrit.server.index.change.ChangeSchemaDefinitions; import com.google.gerrit.server.index.change.IndexedChangeQuery; import com.google.inject.Inject; import com.google.inject.Provider; +import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Set; @@ -52,11 +58,13 @@ import java.util.Set; * holding on to a single instance. */ public class ChangeQueryProcessor extends QueryProcessor - implements DynamicOptions.BeanReceiver, DynamicOptions.BeanProvider { + implements DynamicOptions.BeanReceiver, DynamicOptions.BeanProvider, PluginDefinedInfosFactory { private final Provider userProvider; private final ImmutableListMultimap attributeFactoriesByPlugin; private final ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory; private final Map dynamicBeans = new HashMap<>(); + private final List> + changePluginDefinedInfoFactoriesByPlugin = new ArrayList<>(); static { // It is assumed that basic rewrites do not touch visibleto predicates. @@ -74,7 +82,8 @@ public class ChangeQueryProcessor extends QueryProcessor ChangeIndexCollection indexes, ChangeIndexRewriter rewriter, DynamicSet attributeFactories, - ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory) { + ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory, + DynamicSet changePluginDefinedInfoFactories) { super( metricMaker, ChangeSchemaDefinitions.INSTANCE, @@ -88,10 +97,15 @@ public class ChangeQueryProcessor extends QueryProcessor ImmutableListMultimap.Builder factoriesBuilder = ImmutableListMultimap.builder(); + ImmutableListMultimap.Builder infosFactoriesBuilder = + ImmutableListMultimap.builder(); // Eagerly call Extension#get() rather than storing Extensions, since that method invokes the // Provider on every call, which could be expensive if we invoke it once for every change. attributeFactories.entries().forEach(e -> factoriesBuilder.put(e.getPluginName(), e.get())); attributeFactoriesByPlugin = factoriesBuilder.build(); + changePluginDefinedInfoFactories + .entries() + .forEach(e -> changePluginDefinedInfoFactoriesByPlugin.add(e)); } @Override @@ -128,6 +142,17 @@ public class ChangeQueryProcessor extends QueryProcessor .map(e -> new Extension<>(e.getKey(), e::getValue))); } + public PluginDefinedInfosFactory getInfosFactory() { + return this::createPluginDefinedInfos; + } + + @Override + public ImmutableListMultimap createPluginDefinedInfos( + Collection cds) { + return PluginDefinedAttributesFactories.createAll( + cds, this, changePluginDefinedInfoFactoriesByPlugin.stream()); + } + @Override protected Predicate enforceVisibility(Predicate pred) { return new AndChangeSource( diff --git a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java index 02e8434..b931457 100644 --- a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java +++ b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java @@ -17,11 +17,14 @@ package com.google.gerrit.server.query.change; import static com.google.common.base.Preconditions.checkState; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.collect.ImmutableListMultimap; import com.google.common.flogger.FluentLogger; +import com.google.gerrit.entities.Change; import com.google.gerrit.entities.LabelTypes; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; import com.google.gerrit.exceptions.StorageException; +import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.index.query.QueryResult; import com.google.gerrit.server.DynamicOptions; @@ -97,6 +100,8 @@ public class OutputStreamQuery { private OutputStream outputStream = DisabledOutputStream.INSTANCE; private PrintWriter out; + private ImmutableListMultimap pluginInfosByChange = + ImmutableListMultimap.of(); @Inject OutputStreamQuery( @@ -207,6 +212,7 @@ public class OutputStreamQuery { Map repos = new HashMap<>(); Map revWalks = new HashMap<>(); QueryResult results = queryProcessor.query(queryBuilder.parse(queryString)); + pluginInfosByChange = queryProcessor.createPluginDefinedInfos(results.entities()); try { for (ChangeData d : results.entities()) { show(buildChangeAttribute(d, repos, revWalks)); @@ -325,6 +331,15 @@ public class OutputStreamQuery { } c.plugins = queryProcessor.getAttributesFactory().create(d); + List pluginInfos = pluginInfosByChange.get(d.getId()); + if (!pluginInfos.isEmpty()) { + if (c.plugins == null) { + c.plugins = pluginInfos; + } else { + c.plugins = new ArrayList<>(c.plugins); + c.plugins.addAll(pluginInfos); + } + } return c; } diff --git a/java/com/google/gerrit/server/restapi/change/GetChange.java b/java/com/google/gerrit/server/restapi/change/GetChange.java index c28741b..1ef3c4b 100644 --- a/java/com/google/gerrit/server/restapi/change/GetChange.java +++ b/java/com/google/gerrit/server/restapi/change/GetChange.java @@ -15,7 +15,9 @@ package com.google.gerrit.server.restapi.change; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.Streams; +import com.google.gerrit.entities.Change; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.client.ListOption; import com.google.gerrit.extensions.common.ChangeInfo; @@ -27,11 +29,13 @@ import com.google.gerrit.server.DynamicOptions; import com.google.gerrit.server.DynamicOptions.DynamicBean; import com.google.gerrit.server.change.ChangeAttributeFactory; import com.google.gerrit.server.change.ChangeJson; +import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.PluginDefinedAttributesFactories; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.query.change.ChangeData; import com.google.inject.Inject; +import java.util.Collection; import java.util.EnumSet; import java.util.HashMap; import java.util.Map; @@ -43,6 +47,7 @@ public class GetChange DynamicOptions.BeanProvider { private final ChangeJson.Factory json; private final DynamicSet attrFactories; + private final DynamicSet pdiFactories; private final EnumSet options = EnumSet.noneOf(ListChangesOption.class); private final Map dynamicBeans = new HashMap<>(); @@ -57,9 +62,13 @@ public class GetChange } @Inject - GetChange(ChangeJson.Factory json, DynamicSet attrFactories) { + GetChange( + ChangeJson.Factory json, + DynamicSet attrFactories, + DynamicSet pdiFactories) { this.json = json; this.attrFactories = attrFactories; + this.pdiFactories = pdiFactories; } @Override @@ -82,11 +91,17 @@ public class GetChange } private ChangeJson newChangeJson() { - return json.create(options, this::buildPluginInfo); + return json.create(options, this::buildPluginInfo, this::createPluginDefinedInfos); } private ImmutableList buildPluginInfo(ChangeData cd) { return PluginDefinedAttributesFactories.createAll( cd, this, Streams.stream(attrFactories.entries())); } + + private ImmutableListMultimap createPluginDefinedInfos( + Collection cds) { + return PluginDefinedAttributesFactories.createAll( + cds, this, Streams.stream(pdiFactories.entries())); + } } diff --git a/java/com/google/gerrit/server/restapi/change/QueryChanges.java b/java/com/google/gerrit/server/restapi/change/QueryChanges.java index 878e714..0fec476 100644 --- a/java/com/google/gerrit/server/restapi/change/QueryChanges.java +++ b/java/com/google/gerrit/server/restapi/change/QueryChanges.java @@ -165,7 +165,9 @@ public class QueryChanges implements RestReadView, DynamicOpti int cnt = queries.size(); List> results = queryProcessor.query(qb.parse(queries)); List> res = - json.create(options, queryProcessor.getAttributesFactory()).format(results); + json.create( + options, queryProcessor.getAttributesFactory(), queryProcessor.getInfosFactory()) + .format(results); for (int n = 0; n < cnt; n++) { List info = res.get(n); if (results.get(n).more() && !info.isEmpty()) { diff --git a/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java b/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java index d5089ff..31198d5 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java @@ -19,6 +19,7 @@ import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.extensions.annotations.Exports; import com.google.gerrit.server.change.ChangeAttributeFactory; import com.google.inject.AbstractModule; +import java.util.Arrays; import org.junit.Test; @NoHttpd @@ -50,6 +51,18 @@ public class PluginFieldsIT extends AbstractPluginFieldsTest { } @Test + public void querySingleChangeWithBulkAttribute() throws Exception { + getSingleChangeWithPluginDefinedBulkAttribute( + id -> pluginInfosFromChangeInfos(gApi.changes().query(id.toString()).get())); + } + + @Test + public void getSingleChangeWithBulkAttribute() throws Exception { + getSingleChangeWithPluginDefinedBulkAttribute( + id -> pluginInfosFromChangeInfos(Arrays.asList(gApi.changes().id(id.toString()).get()))); + } + + @Test public void queryChangeWithOption() throws Exception { getChangeWithOption( id -> pluginInfoFromSingletonList(gApi.changes().query(id.toString()).get()), @@ -65,6 +78,53 @@ public class PluginFieldsIT extends AbstractPluginFieldsTest { (id, opts) -> pluginInfoFromChangeInfo(gApi.changes().id(id.get()).get(opts))); } + @Test + public void queryChangeWithOptionBulkAttribute() throws Exception { + getChangeWithPluginDefinedBulkAttributeOption( + id -> pluginInfosFromChangeInfos(gApi.changes().query(id.toString()).get()), + (id, opts) -> + pluginInfosFromChangeInfos( + gApi.changes().query(id.toString()).withPluginOptions(opts).get())); + } + + @Test + public void getChangeWithOptionBulkAttribute() throws Exception { + getChangeWithPluginDefinedBulkAttributeOption( + id -> pluginInfosFromChangeInfos(Arrays.asList(gApi.changes().id(id.get()).get())), + (id, opts) -> + pluginInfosFromChangeInfos(Arrays.asList(gApi.changes().id(id.get()).get(opts)))); + } + + @Test + public void queryMultipleChangesWithPluginDefinedAttribute() throws Exception { + getMultipleChangesWithPluginDefinedBulkAttribute( + () -> pluginInfosFromChangeInfos(gApi.changes().query("status:open").get())); + } + + @Test + public void queryChangesByCommitMessageWithPluginDefinedBulkAttribute() throws Exception { + getChangesByCommitMessageWithPluginDefinedBulkAttribute( + () -> pluginInfosFromChangeInfos(gApi.changes().query("status:open").get())); + } + + @Test + public void getMultipleChangesWithPluginDefinedAndChangeAttributes() throws Exception { + getMultipleChangesWithPluginDefinedBulkAndChangeAttributes( + () -> pluginInfosFromChangeInfos(gApi.changes().query("status:open").get())); + } + + @Test + public void getMultipleChangesWithPluginDefinedAttributeInSingleCall() throws Exception { + getMultipleChangesWithPluginDefinedBulkAttributeInSingleCall( + () -> pluginInfosFromChangeInfos(gApi.changes().query("status:open").get())); + } + + @Test + public void getChangeWithPluginDefinedException() throws Exception { + getChangeWithPluginDefinedBulkAttributeWithException( + id -> pluginInfosFromChangeInfos(Arrays.asList(gApi.changes().id(id.get()).get()))); + } + static class SimpleAttributeWithExplicitExportModule extends AbstractModule { @Override public void configure() { diff --git a/javatests/com/google/gerrit/acceptance/rest/change/PluginFieldsIT.java b/javatests/com/google/gerrit/acceptance/rest/change/PluginFieldsIT.java index 7649316..17bf37e 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/PluginFieldsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/PluginFieldsIT.java @@ -22,9 +22,11 @@ import com.google.gerrit.acceptance.AbstractPluginFieldsTest; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Change; +import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.json.OutputFormat; import com.google.gson.Gson; import com.google.gson.reflect.TypeToken; +import java.util.Arrays; import java.util.List; import java.util.Map; import org.junit.Test; @@ -68,6 +70,24 @@ public class PluginFieldsIT extends AbstractPluginFieldsTest { } @Test + public void querySingleChangeWithBulkAttribute() throws Exception { + getSingleChangeWithPluginDefinedBulkAttribute( + id -> pluginInfosFromChangeInfos(adminRestSession.get(changeQueryUrl(id)))); + } + + @Test + public void pluginDefinedGetChangeWithSimpleAttribute() throws Exception { + getSingleChangeWithPluginDefinedBulkAttribute( + id -> pluginInfoMapFromChangeInfo(adminRestSession.get(changeUrl(id)))); + } + + @Test + public void pluginDefinedGetChangeDetailWithSimpleAttribute() throws Exception { + getSingleChangeWithPluginDefinedBulkAttribute( + id -> pluginInfoMapFromChangeInfo(adminRestSession.get(changeDetailUrl(id)))); + } + + @Test public void queryChangeWithOption() throws Exception { getChangeWithOption( id -> pluginInfoFromSingletonList(adminRestSession.get(changeQueryUrl(id))), @@ -88,6 +108,57 @@ public class PluginFieldsIT extends AbstractPluginFieldsTest { (id, opts) -> pluginInfoFromChangeInfo(adminRestSession.get(changeDetailUrl(id, opts)))); } + @Test + public void pluginDefinedQueryChangeWithOption() throws Exception { + getChangeWithPluginDefinedBulkAttributeOption( + id -> pluginInfosFromChangeInfos(adminRestSession.get(changeQueryUrl(id))), + (id, opts) -> pluginInfosFromChangeInfos(adminRestSession.get(changeQueryUrl(id, opts)))); + } + + @Test + public void pluginDefinedGetChangeWithOption() throws Exception { + getChangeWithPluginDefinedBulkAttributeOption( + id -> pluginInfoMapFromChangeInfo(adminRestSession.get(changeUrl(id))), + (id, opts) -> pluginInfoMapFromChangeInfo(adminRestSession.get(changeUrl(id, opts)))); + } + + @Test + public void pluginDefinedGetChangeDetailWithOption() throws Exception { + getChangeWithPluginDefinedBulkAttributeOption( + id -> pluginInfoMapFromChangeInfo(adminRestSession.get(changeDetailUrl(id))), + (id, opts) -> pluginInfoMapFromChangeInfo(adminRestSession.get(changeDetailUrl(id, opts)))); + } + + @Test + public void queryMultipleChangesWithPluginDefinedAttribute() throws Exception { + getMultipleChangesWithPluginDefinedBulkAttribute( + () -> pluginInfosFromChangeInfos(adminRestSession.get("/changes/?q=status:open"))); + } + + @Test + public void queryChangesByCommitMessageWithPluginDefinedBulkAttribute() throws Exception { + getChangesByCommitMessageWithPluginDefinedBulkAttribute( + () -> pluginInfosFromChangeInfos(adminRestSession.get("/changes/?q=status:open"))); + } + + @Test + public void getMultipleChangesWithPluginDefinedAndChangeAttributes() throws Exception { + getMultipleChangesWithPluginDefinedBulkAndChangeAttributes( + () -> pluginInfosFromChangeInfos(adminRestSession.get("/changes/?q=status:open"))); + } + + @Test + public void getMultipleChangesWithPluginDefinedAttributeInSingleCall() throws Exception { + getMultipleChangesWithPluginDefinedBulkAttributeInSingleCall( + () -> pluginInfosFromChangeInfos(adminRestSession.get("/changes/?q=status:open"))); + } + + @Test + public void getChangeWithPluginDefinedException() throws Exception { + getChangeWithPluginDefinedBulkAttributeWithException( + id -> pluginInfoMapFromChangeInfo(adminRestSession.get(changeUrl(id)))); + } + private String changeQueryUrl(Change.Id id) { return changeQueryUrl(id, ImmutableListMultimap.of()); } @@ -133,7 +204,8 @@ public class PluginFieldsIT extends AbstractPluginFieldsTest { } @Nullable - private static List pluginInfoFromSingletonList(RestResponse res) throws Exception { + private static List pluginInfoFromSingletonList(RestResponse res) + throws Exception { res.assertOK(); List> changeInfos = GSON.fromJson(res.getReader(), new TypeToken>>() {}.getType()); @@ -142,10 +214,28 @@ public class PluginFieldsIT extends AbstractPluginFieldsTest { } @Nullable - private List pluginInfoFromChangeInfo(RestResponse res) throws Exception { + private List pluginInfoFromChangeInfo(RestResponse res) throws Exception { res.assertOK(); Map changeInfo = GSON.fromJson(res.getReader(), new TypeToken>() {}.getType()); return decodeRawPluginsList(GSON, changeInfo.get("plugins")); } + + @Nullable + private Map> pluginInfoMapFromChangeInfo(RestResponse res) + throws Exception { + res.assertOK(); + Map changeInfo = + GSON.fromJson(res.getReader(), new TypeToken>() {}.getType()); + return getPluginInfosFromChangeInfos(GSON, Arrays.asList(changeInfo)); + } + + @Nullable + private Map> pluginInfosFromChangeInfos(RestResponse res) + throws Exception { + res.assertOK(); + List> changeInfos = + GSON.fromJson(res.getReader(), new TypeToken>>() {}.getType()); + return getPluginInfosFromChangeInfos(GSON, changeInfos); + } } diff --git a/javatests/com/google/gerrit/acceptance/ssh/PluginChangeFieldsIT.java b/javatests/com/google/gerrit/acceptance/ssh/PluginChangeFieldsIT.java index 4bf7c19..38293f9 100644 --- a/javatests/com/google/gerrit/acceptance/ssh/PluginChangeFieldsIT.java +++ b/javatests/com/google/gerrit/acceptance/ssh/PluginChangeFieldsIT.java @@ -23,6 +23,7 @@ import com.google.gerrit.acceptance.AbstractPluginFieldsTest; import com.google.gerrit.acceptance.UseSsh; import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Change; +import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.server.query.change.OutputStreamQuery; import com.google.gson.Gson; import com.google.gson.reflect.TypeToken; @@ -52,12 +53,55 @@ public class PluginChangeFieldsIT extends AbstractPluginFieldsTest { } @Test + public void querySingleChangeWithBulkAttribute() throws Exception { + getSingleChangeWithPluginDefinedBulkAttribute( + id -> pluginInfosFromList(adminSshSession.exec(changeQueryCmd(id)))); + } + + @Test public void queryChangeWithOption() throws Exception { getChangeWithOption( id -> pluginInfoFromSingletonList(adminSshSession.exec(changeQueryCmd(id))), (id, opts) -> pluginInfoFromSingletonList(adminSshSession.exec(changeQueryCmd(id, opts)))); } + @Test + public void queryPluginDefinedAttributeChangeWithOption() throws Exception { + getChangeWithPluginDefinedBulkAttributeOption( + id -> pluginInfosFromList(adminSshSession.exec(changeQueryCmd(id))), + (id, opts) -> pluginInfosFromList(adminSshSession.exec(changeQueryCmd(id, opts)))); + } + + @Test + public void queryMultipleChangesWithPluginDefinedAttribute() throws Exception { + getMultipleChangesWithPluginDefinedBulkAttribute( + () -> pluginInfosFromList(adminSshSession.exec("gerrit query --format json status:open"))); + } + + @Test + public void queryChangesByCommitMessageWithPluginDefinedBulkAttribute() throws Exception { + getChangesByCommitMessageWithPluginDefinedBulkAttribute( + () -> pluginInfosFromList(adminSshSession.exec("gerrit query --format json status:open"))); + } + + @Test + public void getMultipleChangesWithPluginDefinedAndChangeAttributes() throws Exception { + getMultipleChangesWithPluginDefinedBulkAndChangeAttributes( + () -> pluginInfosFromList(adminSshSession.exec("gerrit query --format json status:open"))); + } + + @Test + public void getMultipleChangesWithPluginDefinedAttributeInSingleCall() throws Exception { + getMultipleChangesWithPluginDefinedBulkAttributeInSingleCall( + () -> pluginInfosFromList(adminSshSession.exec("gerrit query --format json status:open"))); + } + + @Test + public void getChangeWithPluginDefinedException() throws Exception { + getChangeWithPluginDefinedBulkAttributeWithException( + id -> pluginInfosFromList(adminSshSession.exec(changeQueryCmd(id)))); + } + private String changeQueryCmd(Change.Id id) { return changeQueryCmd(id, ImmutableListMultimap.of()); } @@ -72,7 +116,22 @@ public class PluginChangeFieldsIT extends AbstractPluginFieldsTest { } @Nullable - private static List pluginInfoFromSingletonList(String sshOutput) throws Exception { + private static List pluginInfoFromSingletonList(String sshOutput) + throws Exception { + List> changeAttrs = getChangeAttrs(sshOutput); + + assertThat(changeAttrs).hasSize(1); + return decodeRawPluginsList(GSON, changeAttrs.get(0).get("plugins")); + } + + @Nullable + private static Map> pluginInfosFromList(String sshOutput) + throws Exception { + List> changeAttrs = getChangeAttrs(sshOutput); + return getPluginInfosFromChangeInfos(GSON, changeAttrs); + } + + private static List> getChangeAttrs(String sshOutput) throws Exception { List> changeAttrs = new ArrayList<>(); for (String line : CharStreams.readLines(new StringReader(sshOutput))) { Map changeAttr = @@ -81,8 +140,6 @@ public class PluginChangeFieldsIT extends AbstractPluginFieldsTest { changeAttrs.add(changeAttr); } } - - assertThat(changeAttrs).hasSize(1); - return decodeRawPluginsList(GSON, changeAttrs.get(0).get("plugins")); + return changeAttrs; } }