Fix DynamicOptions to support a custom bean

In the lifecycle listeners change (I84670dcab), a bug has been
introduced where the DynamicOptions no longer supports processing
custom bean. This change addresses that issue. Added a test case which
illustrates the use of the custom bean. Also, try-with-resources block
is used to instantiate DynamicOptions at all the places such that
the resources are closed as expected.

Change-Id: I7e9c518461408d272d0d0673b56a93b77cc6d2a8

java/com/google/gerrit/acceptance/AbstractDynamicOptionsTest.java

@@ -0,0 +1,117 @@
+// 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.acceptance;
+
+import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE;
+
+import com.google.common.collect.Lists;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.json.OutputFormat;
+import com.google.gerrit.server.DynamicOptions;
+import com.google.gerrit.sshd.CommandMetaData;
+import com.google.gerrit.sshd.CommandModule;
+import com.google.gerrit.sshd.SshCommand;
+import com.google.gson.reflect.TypeToken;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import java.io.BufferedWriter;
+import java.io.OutputStream;
+import java.io.OutputStreamWriter;
+import java.io.PrintWriter;
+import java.util.Collections;
+import java.util.List;
+
+public class AbstractDynamicOptionsTest extends AbstractDaemonTest {
+  protected static final String LS_SAMPLES = "ls-samples";
+
+  protected interface Bean {
+    void setSamples(List<String> samples);
+  }
+
+  protected static class ListSamples implements Bean, DynamicOptions.BeanReceiver {
+    protected List<String> samples = Collections.emptyList();
+
+    @Override
+    public void setSamples(List<String> samples) {
+      this.samples = samples;
+    }
+
+    public void display(OutputStream displayOutputStream) throws Exception {
+      PrintWriter stdout =
+          new PrintWriter(new BufferedWriter(new OutputStreamWriter(displayOutputStream, "UTF-8")));
+      try {
+        OutputFormat.JSON
+            .newGson()
+            .toJson(samples, new TypeToken<List<String>>() {}.getType(), stdout);
+        stdout.print('\n');
+      } finally {
+        stdout.flush();
+      }
+    }
+
+    @Override
+    public void setDynamicBean(String plugin, DynamicOptions.DynamicBean dynamicBean) {}
+  }
+
+  @CommandMetaData(name = LS_SAMPLES, runsAt = MASTER_OR_SLAVE)
+  protected static class ListSamplesCommand extends SshCommand {
+    @Inject private ListSamples impl;
+
+    @Override
+    protected void run() throws Exception {
+      impl.display(out);
+    }
+
+    @Override
+    protected void parseCommandLine(DynamicOptions pluginOptions) throws UnloggedFailure {
+      parseCommandLine(impl, pluginOptions);
+    }
+  }
+
+  public static class PluginOneSshModule extends CommandModule {
+    @Override
+    public void configure() {
+      command(LS_SAMPLES).to(ListSamplesCommand.class);
+    }
+  }
+
+  protected static class ListSamplesOptions implements DynamicOptions.BeanParseListener {
+    @Override
+    public void onBeanParseStart(String plugin, Object bean) {
+      ((Bean) bean).setSamples(Lists.newArrayList("sample1", "sample2"));
+    }
+
+    @Override
+    public void onBeanParseEnd(String plugin, Object bean) {}
+  }
+
+  protected static class PluginTwoModule extends AbstractModule {
+    @Override
+    public void configure() {
+      bind(DynamicOptions.DynamicBean.class)
+          .annotatedWith(
+              Exports.named("com.google.gerrit.acceptance.AbstractDynamicOptionsTest.ListSamples"))
+          .to(ListSamplesOptionsClassNameProvider.class);
+    }
+  }
+
+  protected static class ListSamplesOptionsClassNameProvider
+      implements DynamicOptions.ClassNameProvider {
+    @Override
+    public String getClassName() {
+      return "com.google.gerrit.acceptance.AbstractDynamicOptionsTest$ListSamplesOptions";
+    }
+  }
+}

java/com/google/gerrit/httpd/restapi/ParameterParser.java

@@ -161,6 +161,8 @@ public class ParameterParser {
       HttpServletResponse res)
       throws IOException {
     CmdLineParser clp = parserFactory.create(param);
+    pluginOptions.setBean(param);
+    pluginOptions.startLifecycleListeners();
     pluginOptions.parseDynamicBeans(clp);
     pluginOptions.setDynamicBeans();
     pluginOptions.onBeanParseStart();

java/com/google/gerrit/httpd/restapi/RestApiServlet.java

@@ -507,7 +507,7 @@ public class RestApiServlet extends HttpServlet {
           }
 
           try (DynamicOptions pluginOptions =
-              new DynamicOptions(viewData.view, globals.injector, globals.dynamicBeans)) {
+              new DynamicOptions(globals.injector, globals.dynamicBeans)) {
             if (!globals
                 .paramParser
                 .get()

java/com/google/gerrit/server/DynamicOptions.java

@@ -193,6 +193,7 @@ public class DynamicOptions implements AutoCloseable {
   protected Object bean;
   protected Map<String, DynamicBean> beansByPlugin;
   protected Injector injector;
+  protected DynamicMap<DynamicBean> dynamicBeans;
   protected LifecycleManager lifecycleManager;
 
   /**
@@ -200,7 +201,9 @@ public class DynamicOptions implements AutoCloseable {
    * this class so the following methods can be called if desired:
    *
    * <pre>
-   *    DynamicOptions pluginOptions = new DynamicOptions(bean, injector, dynamicBeans);
+   *    DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans);
+   *    pluginOptions.setBean(bean);
+   *    pluginOptions.startLifecycleListeners();
    *    pluginOptions.parseDynamicBeans(clp);
    *    pluginOptions.setDynamicBeans();
    *    pluginOptions.onBeanParseStart();
@@ -210,11 +213,15 @@ public class DynamicOptions implements AutoCloseable {
    *    pluginOptions.onBeanParseEnd();
    * </pre>
    */
-  public DynamicOptions(Object bean, Injector injector, DynamicMap<DynamicBean> dynamicBeans) {
-    this.bean = bean;
+  public DynamicOptions(Injector injector, DynamicMap<DynamicBean> dynamicBeans) {
     this.injector = injector;
+    this.dynamicBeans = dynamicBeans;
     lifecycleManager = new LifecycleManager();
     beansByPlugin = new HashMap<>();
+  }
+
+  public void setBean(Object bean) {
+    this.bean = bean;
     Class<?> beanClass =
         (bean instanceof BeanReceiver)
             ? ((BeanReceiver) bean).getExportedBeanReceiver()
@@ -226,7 +233,6 @@ public class DynamicOptions implements AutoCloseable {
         beansByPlugin.put(plugin, getDynamicBean(bean, provider.get()));
       }
     }
-    startLifecycleListeners();
   }
 
   @SuppressWarnings("unchecked")

java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -177,6 +177,8 @@ class ChangeApiImpl implements ChangeApi {
   private final Provider<GetPureRevert> getPureRevertProvider;
   private final StarredChangesUtil stars;
   private final DynamicOptionParser dynamicOptionParser;
+  private final Injector injector;
+  private final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
 
   @Inject
   ChangeApiImpl(
@@ -230,7 +232,9 @@ class ChangeApiImpl implements ChangeApi {
       Provider<GetPureRevert> getPureRevertProvider,
       StarredChangesUtil stars,
       DynamicOptionParser dynamicOptionParser,
-      @Assisted ChangeResource change) {
+      @Assisted ChangeResource change,
+      Injector injector,
+      DynamicMap<DynamicOptions.DynamicBean> dynamicBeans) {
     this.changeApi = changeApi;
     this.revert = revert;
     this.revertSubmission = revertSubmission;
@@ -282,6 +286,8 @@ class ChangeApiImpl implements ChangeApi {
     this.stars = stars;
     this.dynamicOptionParser = dynamicOptionParser;
     this.change = change;
+    this.injector = injector;
+    this.dynamicBeans = dynamicBeans;
   }
 
   @Override
@@ -500,10 +506,10 @@ class ChangeApiImpl implements ChangeApi {
   public ChangeInfo get(
       EnumSet<ListChangesOption> options, ImmutableListMultimap<String, String> pluginOptions)
       throws RestApiException {
-    try {
+    try (DynamicOptions dynamicOptions = new DynamicOptions(injector, dynamicBeans)) {
       GetChange getChange = getChangeProvider.get();
       options.forEach(getChange::addOption);
-      dynamicOptionParser.parseDynamicOptions(getChange, pluginOptions);
+      dynamicOptionParser.parseDynamicOptions(getChange, pluginOptions, dynamicOptions);
       return getChange.apply(change).value();
     } catch (Exception e) {
       throw asRestApiException("Cannot retrieve change", e);
@@ -759,8 +765,6 @@ class ChangeApiImpl implements ChangeApi {
   @Singleton
   static class DynamicOptionParser {
     private final CmdLineParser.Factory cmdLineParserFactory;
-    private final Injector injector;
-    private final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
 
     @Inject
     DynamicOptionParser(
@@ -768,14 +772,14 @@ class ChangeApiImpl implements ChangeApi {
         Injector injector,
         DynamicMap<DynamicOptions.DynamicBean> dynamicBeans) {
       this.cmdLineParserFactory = cmdLineParserFactory;
-      this.injector = injector;
-      this.dynamicBeans = dynamicBeans;
     }
 
-    void parseDynamicOptions(Object bean, ListMultimap<String, String> pluginOptions)
+    void parseDynamicOptions(
+        Object bean, ListMultimap<String, String> pluginOptions, DynamicOptions dynamicOptions)
         throws BadRequestException {
       CmdLineParser clp = cmdLineParserFactory.create(bean);
-      DynamicOptions dynamicOptions = new DynamicOptions(bean, injector, dynamicBeans);
+      dynamicOptions.setBean(bean);
+      dynamicOptions.startLifecycleListeners();
       dynamicOptions.parseDynamicBeans(clp);
       dynamicOptions.setDynamicBeans();
       dynamicOptions.onBeanParseStart();

java/com/google/gerrit/server/api/changes/ChangesImpl.java

@@ -26,15 +26,18 @@ import com.google.gerrit.extensions.api.changes.Changes;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.ChangeInput;
+import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
 import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.server.DynamicOptions;
 import com.google.gerrit.server.api.changes.ChangeApiImpl.DynamicOptionParser;
 import com.google.gerrit.server.restapi.change.ChangesCollection;
 import com.google.gerrit.server.restapi.change.CreateChange;
 import com.google.gerrit.server.restapi.change.QueryChanges;
 import com.google.inject.Inject;
+import com.google.inject.Injector;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.util.List;
@@ -46,6 +49,8 @@ class ChangesImpl implements Changes {
   private final CreateChange createChange;
   private final DynamicOptionParser dynamicOptionParser;
   private final Provider<QueryChanges> queryProvider;
+  private final Injector injector;
+  private final DynamicMap<DynamicOptions.DynamicBean> dynamicBeans;
 
   @Inject
   ChangesImpl(
@@ -53,12 +58,16 @@ class ChangesImpl implements Changes {
       ChangeApiImpl.Factory api,
       CreateChange createChange,
       DynamicOptionParser dynamicOptionParser,
-      Provider<QueryChanges> queryProvider) {
+      Provider<QueryChanges> queryProvider,
+      Injector injector,
+      DynamicMap<DynamicOptions.DynamicBean> dynamicBeans) {
     this.changes = changes;
     this.api = api;
     this.createChange = createChange;
     this.dynamicOptionParser = dynamicOptionParser;
     this.queryProvider = queryProvider;
+    this.injector = injector;
+    this.dynamicBeans = dynamicBeans;
   }
 
   @Override
@@ -123,6 +132,7 @@ class ChangesImpl implements Changes {
   }
 
   private List<ChangeInfo> get(QueryRequest q) throws RestApiException {
+    try (DynamicOptions dynamicOptions = new DynamicOptions(injector, dynamicBeans)) {
       QueryChanges qc = queryProvider.get();
       if (q.getQuery() != null) {
         qc.addQuery(q.getQuery());
@@ -133,7 +143,7 @@ class ChangesImpl implements Changes {
       for (ListChangesOption option : q.getOptions()) {
         qc.addOption(option);
       }
-    dynamicOptionParser.parseDynamicOptions(qc, q.getPluginOptions());
+      dynamicOptionParser.parseDynamicOptions(qc, q.getPluginOptions(), dynamicOptions);
 
       try {
         List<?> result = qc.apply(TopLevelResource.INSTANCE).value();
@@ -153,4 +163,5 @@ class ChangesImpl implements Changes {
         throw asRestApiException("Cannot query changes", e);
       }
     }
+  }
 }

java/com/google/gerrit/sshd/BaseCommand.java

@@ -235,6 +235,8 @@ public abstract class BaseCommand implements Command {
   protected void parseCommandLine(Object options, DynamicOptions pluginOptions)
       throws UnloggedFailure {
     final CmdLineParser clp = newCmdLineParser(options);
+    pluginOptions.setBean(options);
+    pluginOptions.startLifecycleListeners();
     pluginOptions.parseDynamicBeans(clp);
     pluginOptions.setDynamicBeans();
     pluginOptions.onBeanParseStart();
@@ -468,8 +470,7 @@ public abstract class BaseCommand implements Command {
 
           try {
             if (thunk instanceof ProjectCommandRunnable) {
-              try (DynamicOptions pluginOptions =
-                  new DynamicOptions(BaseCommand.this, injector, dynamicBeans)) {
+              try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
                 ((ProjectCommandRunnable) thunk).executeParseCommand(pluginOptions);
                 projectName = ((ProjectCommandRunnable) thunk).getProjectName();
                 thunk.run();

java/com/google/gerrit/sshd/DispatchCommand.java

@@ -72,8 +72,7 @@ final class DispatchCommand extends BaseCommand {
 
   @Override
   public void start(ChannelSession channel, Environment env) throws IOException {
-    try (DynamicOptions pluginOptions =
-        new DynamicOptions(DispatchCommand.this, injector, dynamicBeans)) {
+    try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
       parseCommandLine(pluginOptions);
       if (Strings.isNullOrEmpty(commandName)) {
         StringWriter msg = new StringWriter();

java/com/google/gerrit/sshd/SshCommand.java

@@ -50,8 +50,7 @@ public abstract class SshCommand extends BaseCommand {
   public void start(ChannelSession channel, Environment env) throws IOException {
     startThread(
         () -> {
-          try (DynamicOptions pluginOptions =
-              new DynamicOptions(SshCommand.this, injector, dynamicBeans)) {
+          try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
             parseCommandLine(pluginOptions);
             stdout = toPrintWriter(out);
             stderr = toPrintWriter(err);

java/com/google/gerrit/sshd/SuExec.java

@@ -93,7 +93,7 @@ public final class SuExec extends BaseCommand {
 
   @Override
   public void start(ChannelSession channel, Environment env) throws IOException {
-    try (DynamicOptions pluginOptions = new DynamicOptions(SuExec.this, injector, dynamicBeans)) {
+    try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
       checkCanRunAs();
       parseCommandLine(pluginOptions);
 

java/com/google/gerrit/sshd/commands/StreamEvents.java

@@ -108,8 +108,7 @@ public final class StreamEvents extends BaseCommand {
 
   @Override
   public void start(ChannelSession channel, Environment env) throws IOException {
-    try (DynamicOptions pluginOptions =
-        new DynamicOptions(StreamEvents.this, injector, dynamicBeans)) {
+    try (DynamicOptions pluginOptions = new DynamicOptions(injector, dynamicBeans)) {
       try {
         parseCommandLine(pluginOptions);
       } catch (UnloggedFailure e) {

javatests/com/google/gerrit/acceptance/ssh/DynamicOptionsIT.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.acceptance.ssh;
+
+import static com.google.gerrit.server.query.change.OutputStreamQuery.GSON;
+import static junit.framework.TestCase.assertEquals;
+
+import com.google.common.collect.Lists;
+import com.google.gerrit.acceptance.AbstractDynamicOptionsTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.UseSsh;
+import com.google.gson.reflect.TypeToken;
+import com.google.inject.Module;
+import java.io.IOException;
+import java.util.List;
+import org.junit.Test;
+
+@NoHttpd
+@UseSsh
+public class DynamicOptionsIT extends AbstractDynamicOptionsTest {
+
+  @Override
+  public Module createSshModule() {
+    return new AbstractDynamicOptionsTest.PluginOneSshModule();
+  }
+
+  @Test
+  public void testDynamicPluginOptions() throws Exception {
+    try (AutoCloseable ignored =
+        installPlugin("my-plugin", AbstractDynamicOptionsTest.PluginTwoModule.class)) {
+      List<String> samples = getSamplesList(adminSshSession.exec("ls-samples"));
+      adminSshSession.assertSuccess();
+      assertEquals(Lists.newArrayList("sample1", "sample2"), samples);
+    }
+  }
+
+  protected List<String> getSamplesList(String sshOutput) throws IOException {
+    return GSON.fromJson(sshOutput, new TypeToken<List<String>>() {}.getType());
+  }
+}