Move DynamicMap<DynamicBean> binding into sys injector
This is a requirement in order to support plugin options over the
extension API, which is bound in the sys injector; otherwise, there
would be no way for the extension API implementation to access a
DynamicMap<DynamicBean> from a child injector.
The downside is it silently breaks existing plugins that bind their
DynamicBean implementations in HTTP/SSH modules. In other words, the
tests would fail if we didn't change them; the same goes for plugins.
The upside for plugin authors is that this can make plugin
implementations simpler (as in the test): plugins that aren't actually
contributing SSH commands or HTTP endpoints will no longer need to
define separate modules just to register DyamicBeans.
Overall, this change has only minor downsides for plugin authors, and
this is outweighed by the upside of making it possible to pass plugin
options to the extension API.
Change-Id: I3fd7de21565f6f65519936f47694bcf23e05a9fd
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index 00e153b..4a11270 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -866,8 +866,9 @@
Query Change APIs by implementing implementing the `ChangeAttributeFactory`
interface and registering it to the class 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 with an option registered in
-the Http and Ssh modules' `configure*()` methods.
+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.
The example below shows a plugin that adds two attributes (`exampleName` and
`changeValue`), to the change query output, when the query command is provided
@@ -878,9 +879,25 @@
public class Module extends AbstractModule {
@Override
protected void configure() {
+ // Register attribute factory.
bind(ChangeAttributeFactory.class)
.annotatedWith(Exports.named("example"))
.to(AttributeFactory.class);
+
+ // Register options for GET /changes/X/change and /changes/X/detail.
+ bind(DynamicBean.class)
+ .annotatedWith(Exports.named(GetChange.class))
+ .to(MyChangeOptions.class);
+
+ // Register options for GET /changes/?q=...
+ bind(DynamicBean.class)
+ .annotatedWith(Exports.named(QueryChanges.class))
+ .to(MyChangeOptions.class);
+
+ // Register options for ssh gerrit query.
+ bind(DynamicBean.class)
+ .annotatedWith(Exports.named(Query.class))
+ .to(MyChangeOptions.class);
}
}
@@ -889,27 +906,6 @@
public boolean all = false;
}
-public static class HttpModule extends ServletModule {
- @Override
- protected void configureServlets() {
- bind(DynamicBean.class)
- .annotatedWith(Exports.named(GetChange.class))
- .to(MyChangeOptions.class);
- bind(DynamicBean.class)
- .annotatedWith(Exports.named(QueryChanges.class))
- .to(MyChangeOptions.class);
- }
-}
-
-public static class SshModule extends PluginCommandModule {
- @Override
- protected void configureCommands() {
- bind(DynamicBean.class)
- .annotatedWith(Exports.named(Query.class))
- .to(MyChangeOptions.class);
- }
-}
-
public class AttributeFactory implements ChangeAttributeFactory {
protected MyChangeOptions options;
diff --git a/java/com/google/gerrit/httpd/plugins/HttpPluginModule.java b/java/com/google/gerrit/httpd/plugins/HttpPluginModule.java
index 937b24a..279903c 100644
--- a/java/com/google/gerrit/httpd/plugins/HttpPluginModule.java
+++ b/java/com/google/gerrit/httpd/plugins/HttpPluginModule.java
@@ -15,11 +15,9 @@
package com.google.gerrit.httpd.plugins;
import com.google.gerrit.extensions.api.lfs.LfsDefinitions;
-import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.httpd.resources.Resource;
import com.google.gerrit.httpd.resources.ResourceKey;
import com.google.gerrit.httpd.resources.ResourceWeigher;
-import com.google.gerrit.server.DynamicOptions;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.plugins.ModuleGenerator;
import com.google.gerrit.server.plugins.ReloadPluginListener;
@@ -65,7 +63,5 @@
.weigher(ResourceWeigher.class);
}
});
-
- DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class);
}
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index aa06e5b..6fea90d 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -76,6 +76,7 @@
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.CmdLineParserModule;
import com.google.gerrit.server.CreateGroupPermissionSyncer;
+import com.google.gerrit.server.DynamicOptions;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCacheImpl;
import com.google.gerrit.server.account.AccountControl;
@@ -403,6 +404,7 @@
factory(UploadValidators.Factory.class);
DynamicSet.setOf(binder(), UploadValidationListener.class);
+ DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class);
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeOperatorFactory.class);
DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeHasOperandFactory.class);
DynamicMap.mapOf(binder(), ChangeAttributeFactory.class);
diff --git a/java/com/google/gerrit/sshd/SshModule.java b/java/com/google/gerrit/sshd/SshModule.java
index 1de14b6..c5f190e 100644
--- a/java/com/google/gerrit/sshd/SshModule.java
+++ b/java/com/google/gerrit/sshd/SshModule.java
@@ -20,10 +20,8 @@
import com.google.common.base.CharMatcher;
import com.google.common.base.Splitter;
import com.google.gerrit.extensions.registration.DynamicItem;
-import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.lifecycle.LifecycleModule;
-import com.google.gerrit.server.DynamicOptions;
import com.google.gerrit.server.PeerDaemonUser;
import com.google.gerrit.server.RemotePeer;
import com.google.gerrit.server.config.GerritConfigListener;
@@ -102,7 +100,6 @@
.annotatedWith(UniqueAnnotations.create())
.to(SshPluginStarterCallback.class);
- DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class);
DynamicItem.itemOf(binder(), SshCreateCommandInterceptor.class);
listener().toInstance(registerInParentInjectors());
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java b/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java
index 1b039dc..bc657fd 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java
@@ -38,11 +38,9 @@
import com.google.gerrit.server.query.change.OutputStreamQuery;
import com.google.gerrit.server.restapi.change.GetChange;
import com.google.gerrit.server.restapi.change.QueryChanges;
-import com.google.gerrit.sshd.PluginCommandModule;
import com.google.gerrit.sshd.commands.Query;
import com.google.gson.Gson;
import com.google.inject.AbstractModule;
-import com.google.inject.servlet.ServletModule;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.List;
@@ -115,7 +113,7 @@
private String opt;
}
- static class OptionAttributeSysModule extends AbstractModule {
+ static class OptionAttributeModule extends AbstractModule {
@Override
public void configure() {
bind(ChangeAttributeFactory.class)
@@ -125,19 +123,7 @@
MyOptions opts = (MyOptions) bp.getDynamicBean(p);
return opts != null ? new MyInfo("opt " + opts.opt) : null;
});
- }
- }
-
- static class OptionAttributeSshModule extends PluginCommandModule {
- @Override
- protected void configureCommands() {
bind(DynamicBean.class).annotatedWith(Exports.named(Query.class)).to(MyOptions.class);
- }
- }
-
- static class OptionAttributeHttpModule extends ServletModule {
- @Override
- protected void configureServlets() {
bind(DynamicBean.class).annotatedWith(Exports.named(QueryChanges.class)).to(MyOptions.class);
bind(DynamicBean.class).annotatedWith(Exports.named(GetChange.class)).to(MyOptions.class);
}
@@ -276,11 +262,7 @@
// No tests for /detail via the extension API, since the extension API doesn't have that method.
// No tests for getting a single change over SSH, since the only API is the query API.
- // No test for plugin-provided options over the extension API. There are currently two separate
- // DynamicMap<DynamicBean> maps initialized in the SSH and HTTP injectors, and plugins have to
- // define separate SSH/HTTP modules and bind their DynamicBeans in each one. To use the extension
- // API, we would have to move everything into the sys injector.
- // TODO(dborowitz): Determine whether this is possible without breaking existing plugins.
+ // TODO(dborowitz): Add extension API support for passing plugin options.
private void getChangeWithOption(
PluginInfoGetter getterWithoutOptions, PluginInfoGetterWithOptions getterWithOptions)
@@ -288,12 +270,7 @@
Change.Id id = createChange().getChange().getId();
assertThat(getterWithoutOptions.call(id)).isNull();
- try (AutoCloseable ignored =
- installPlugin(
- "my-plugin",
- OptionAttributeSysModule.class,
- OptionAttributeHttpModule.class,
- OptionAttributeSshModule.class)) {
+ try (AutoCloseable ignored = installPlugin("my-plugin", OptionAttributeModule.class)) {
assertThat(getterWithoutOptions.call(id))
.containsExactly(new MyInfo("my-plugin", "opt null"));
assertThat(getterWithOptions.call(id, ImmutableListMultimap.of("my-plugin--opt", "foo")))
