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

Documentation/dev-plugins.txt

@@ -866,8 +866,9 @@ 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()`
 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 @@ the `--myplugin-name--all` option.
 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 class MyChangeOptions implements DynamicBean {
   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;
 

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 @@ public class HttpPluginModule extends ServletModule {
                 .weigher(ResourceWeigher.class);
           }
         });
-
-    DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class);
   }
 }

java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -76,6 +76,7 @@ import com.google.gerrit.server.AnonymousUser;
 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 @@ public class GerritGlobalModule extends FactoryModule {
     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);

java/com/google/gerrit/sshd/SshModule.java

@@ -20,10 +20,8 @@ import static com.google.inject.Scopes.SINGLETON;
 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 @@ public class SshModule extends LifecycleModule {
         .annotatedWith(UniqueAnnotations.create())
         .to(SshPluginStarterCallback.class);
 
-    DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class);
     DynamicItem.itemOf(binder(), SshCreateCommandInterceptor.class);
 
     listener().toInstance(registerInParentInjectors());

javatests/com/google/gerrit/acceptance/api/change/PluginFieldsIT.java

@@ -38,11 +38,9 @@ import com.google.gerrit.server.change.ChangeAttributeFactory;
 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 @@ public class PluginFieldsIT extends AbstractDaemonTest {
     private String opt;
   }
 
-  static class OptionAttributeSysModule extends AbstractModule {
+  static class OptionAttributeModule extends AbstractModule {
     @Override
     public void configure() {
       bind(ChangeAttributeFactory.class)
@@ -125,19 +123,7 @@ public class PluginFieldsIT extends AbstractDaemonTest {
                 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 @@ public class PluginFieldsIT extends AbstractDaemonTest {
   // 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 @@ public class PluginFieldsIT extends AbstractDaemonTest {
     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")))