Bind ChangeAttributeFactory as a DynamicSet
DynamicMaps allow a view of plugin-provided bindings keyed by export
names. The main use case for an export name is to provide something with
that name that is visible to an end user, such as a new SSH command. In
the case of ChangeAttributeFactory, the export name is entirely unused;
it's just an arbitrary string that the plugin author provides, which has
to be unique to their plugin, but is otherwise meaningless.
By using DynamicSet instead of DynamicMap for these bindings, we
eliminate one line of boilerplate for plugin authors, and avoid
confusing them with the impression that this arbitrary string has
meaning.
The good news is that old plugins that use an explicit, arbitrary export
name will continue to work, because DynamicSet is implemented internally
by using separate binding annotations per instance. If callers use the
DynamicSet interface, they get a unique annotation for free, but it
still works if they explicitly pass the export annotation.
Convert most tests in PluginFieldsIT to demonstrate the new way, but
leave one test that uses the old explicit export to ensure that it still
works.
Change-Id: Iaf7e6ac8a53be115f1bc01e716e0fb3c04033754
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index 4a11270..1d10025 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -864,7 +864,7 @@
Plugins can provide additional attributes to be returned from the Get Change and
Query Change APIs by implementing implementing the `ChangeAttributeFactory`
-interface and registering it to the class in the plugin module's `configure()`
+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
@@ -880,8 +880,7 @@
@Override
protected void configure() {
// Register attribute factory.
- bind(ChangeAttributeFactory.class)
- .annotatedWith(Exports.named("example"))
+ DynamicSet.bind(binder(), ChangeAttributeFactory.class)
.to(AttributeFactory.class);
// Register options for GET /changes/X/change and /changes/X/detail.
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index 494da86..b700835 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -112,8 +112,8 @@
bind(new TypeLiteral<List<CommentLinkInfo>>() {})
.toProvider(CommentLinkProvider.class)
.in(SINGLETON);
- bind(new TypeLiteral<DynamicMap<ChangeAttributeFactory>>() {})
- .toInstance(DynamicMap.emptyMap());
+ bind(new TypeLiteral<DynamicSet<ChangeAttributeFactory>>() {})
+ .toInstance(DynamicSet.emptySet());
bind(new TypeLiteral<DynamicMap<RestView<CommitResource>>>() {})
.toInstance(DynamicMap.emptyMap());
bind(String.class)
diff --git a/java/com/google/gerrit/server/change/ChangeAttributeFactory.java b/java/com/google/gerrit/server/change/ChangeAttributeFactory.java
index 1f91853..95355cf 100644
--- a/java/com/google/gerrit/server/change/ChangeAttributeFactory.java
+++ b/java/com/google/gerrit/server/change/ChangeAttributeFactory.java
@@ -25,13 +25,9 @@
* <p>Register a {@code ChangeAttributeFactory} in a plugin {@code Module} like this:
*
* <pre>
- * bind(ChangeAttributeFactory.class)
- * .annotatedWith(Exports.named("export-name"))
- * .to(YourClass.class);
+ * DynamicSet.bind(binder(), ChangeAttributeFactory.class).to(YourClass.class);
* </pre>
*
- * The export name can be anything unique to your plugin; the name is not exposed.
- *
* <p>See the <a
* href="https://gerrit-review.googlesource.com/Documentation/dev-plugins.html#query_attributes">plugin
* developer documentation for more details and examples.
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 6fea90d..e7baa55 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -407,7 +407,7 @@
DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class);
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeOperatorFactory.class);
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeHasOperandFactory.class);
- DynamicMap.mapOf(binder(), ChangeAttributeFactory.class);
+ DynamicSet.setOf(binder(), ChangeAttributeFactory.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 7bdfcab..f9263a9 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
@@ -20,7 +20,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.gerrit.extensions.common.PluginDefinedInfo;
-import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.Extension;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.QueryOptions;
@@ -81,7 +81,7 @@
ChangeIndexCollection indexes,
ChangeIndexRewriter rewriter,
ChangeNotes.Factory notesFactory,
- DynamicMap<ChangeAttributeFactory> attributeFactories,
+ DynamicSet<ChangeAttributeFactory> attributeFactories,
PermissionBackend permissionBackend,
ProjectCache projectCache,
Provider<AnonymousUser> anonymousUserProvider) {
@@ -103,7 +103,7 @@
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.forEach(e -> factoriesBuilder.put(e.getPluginName(), e.get()));
+ attributeFactories.entries().forEach(e -> factoriesBuilder.put(e.getPluginName(), e.get()));
attributeFactoriesByPlugin = factoriesBuilder.build();
}
diff --git a/java/com/google/gerrit/server/restapi/change/GetChange.java b/java/com/google/gerrit/server/restapi/change/GetChange.java
index 461a669..c8641d5 100644
--- a/java/com/google/gerrit/server/restapi/change/GetChange.java
+++ b/java/com/google/gerrit/server/restapi/change/GetChange.java
@@ -20,7 +20,7 @@
import com.google.gerrit.extensions.client.ListOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.PluginDefinedInfo;
-import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.DynamicOptions;
@@ -43,7 +43,7 @@
DynamicOptions.BeanReceiver,
DynamicOptions.BeanProvider {
private final ChangeJson.Factory json;
- private final DynamicMap<ChangeAttributeFactory> attrFactories;
+ private final DynamicSet<ChangeAttributeFactory> attrFactories;
private final EnumSet<ListChangesOption> options = EnumSet.noneOf(ListChangesOption.class);
private final Map<String, DynamicBean> dynamicBeans = new HashMap<>();
@@ -58,7 +58,7 @@
}
@Inject
- GetChange(ChangeJson.Factory json, DynamicMap<ChangeAttributeFactory> attrFactories) {
+ GetChange(ChangeJson.Factory json, DynamicSet<ChangeAttributeFactory> attrFactories) {
this.json = json;
this.attrFactories = attrFactories;
}
@@ -87,6 +87,7 @@
}
private ImmutableList<PluginDefinedInfo> buildPluginInfo(ChangeData cd) {
- return PluginDefinedAttributesFactories.createAll(cd, this, Streams.stream(attrFactories));
+ return PluginDefinedAttributesFactories.createAll(
+ cd, this, Streams.stream(attrFactories.entries()));
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java b/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java
index d8e609f..d65cf0b 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java
@@ -31,6 +31,7 @@
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.json.OutputFormat;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.DynamicOptions.DynamicBean;
@@ -41,6 +42,7 @@
import com.google.gerrit.sshd.commands.Query;
import com.google.gson.Gson;
import com.google.inject.AbstractModule;
+import com.google.inject.Module;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.List;
@@ -93,15 +95,21 @@
static class NullAttributeModule extends AbstractModule {
@Override
public void configure() {
- bind(ChangeAttributeFactory.class)
- .annotatedWith(Exports.named("null"))
- .toInstance((cd, bp, p) -> null);
+ DynamicSet.bind(binder(), ChangeAttributeFactory.class).toInstance((cd, bp, p) -> null);
}
}
static class SimpleAttributeModule extends AbstractModule {
@Override
public void configure() {
+ DynamicSet.bind(binder(), ChangeAttributeFactory.class)
+ .toInstance((cd, bp, p) -> new MyInfo("change " + cd.getId()));
+ }
+ }
+
+ static class SimpleAttributeWithExplicitExportModule extends AbstractModule {
+ @Override
+ public void configure() {
bind(ChangeAttributeFactory.class)
.annotatedWith(Exports.named("simple"))
.toInstance((cd, bp, p) -> new MyInfo("change " + cd.getId()));
@@ -116,8 +124,7 @@
static class OptionAttributeModule extends AbstractModule {
@Override
public void configure() {
- bind(ChangeAttributeFactory.class)
- .annotatedWith(Exports.named("simple"))
+ DynamicSet.bind(binder(), ChangeAttributeFactory.class)
.toInstance(
(cd, bp, p) -> {
MyOptions opts = (MyOptions) bp.getDynamicBean(p);
@@ -217,11 +224,26 @@
// No tests for getting a single change over SSH, since the only API is the query API.
+ @Test
+ public void getChangeApiWithSimpleAttributeWithExplicitExport() throws Exception {
+ // For backwards compatibility with old plugins, allow modules to bind into the
+ // DynamicSet<ChangeAttributeFactory> as if it were a DynamicMap. We only need one variant of
+ // this test to prove that the mapping works.
+ getChangeWithSimpleAttribute(
+ id -> pluginInfoFromChangeInfo(gApi.changes().id(id.toString()).get()),
+ SimpleAttributeWithExplicitExportModule.class);
+ }
+
private void getChangeWithSimpleAttribute(PluginInfoGetter getter) throws Exception {
+ getChangeWithSimpleAttribute(getter, SimpleAttributeModule.class);
+ }
+
+ private void getChangeWithSimpleAttribute(
+ PluginInfoGetter getter, Class<? extends Module> moduleClass) throws Exception {
Change.Id id = createChange().getChange().getId();
assertThat(getter.call(id)).isNull();
- try (AutoCloseable ignored = installPlugin("my-plugin", SimpleAttributeModule.class)) {
+ try (AutoCloseable ignored = installPlugin("my-plugin", moduleClass)) {
assertThat(getter.call(id)).containsExactly(new MyInfo("my-plugin", "change " + id));
}
