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
diff --git a/java/com/google/gerrit/acceptance/AbstractDynamicOptionsTest.java b/java/com/google/gerrit/acceptance/AbstractDynamicOptionsTest.java
new file mode 100644
index 0000000..a4ed80a
--- /dev/null
+++ b/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";
+ }
+ }
+}
diff --git a/java/com/google/gerrit/httpd/restapi/ParameterParser.java b/java/com/google/gerrit/httpd/restapi/ParameterParser.java
index 95d99f0..326cab8 100644
--- a/java/com/google/gerrit/httpd/restapi/ParameterParser.java
+++ b/java/com/google/gerrit/httpd/restapi/ParameterParser.java
@@ -161,6 +161,8 @@
HttpServletResponse res)
throws IOException {
CmdLineParser clp = parserFactory.create(param);
+ pluginOptions.setBean(param);
+ pluginOptions.startLifecycleListeners();
pluginOptions.parseDynamicBeans(clp);
pluginOptions.setDynamicBeans();
pluginOptions.onBeanParseStart();
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 4d55b36..0e525ce 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -507,7 +507,7 @@
}
try (DynamicOptions pluginOptions =
- new DynamicOptions(viewData.view, globals.injector, globals.dynamicBeans)) {
+ new DynamicOptions(globals.injector, globals.dynamicBeans)) {
if (!globals
.paramParser
.get()
diff --git a/java/com/google/gerrit/server/DynamicOptions.java b/java/com/google/gerrit/server/DynamicOptions.java
index 1d36ff0..db0aa70 100644
--- a/java/com/google/gerrit/server/DynamicOptions.java
+++ b/java/com/google/gerrit/server/DynamicOptions.java
@@ -193,6 +193,7 @@
protected Object bean;
protected Map<String, DynamicBean> beansByPlugin;
protected Injector injector;
+ protected DynamicMap<DynamicBean> dynamicBeans;
protected LifecycleManager lifecycleManager;
/**
@@ -200,7 +201,9 @@
* 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 @@
* 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 @@
beansByPlugin.put(plugin, getDynamicBean(bean, provider.get()));
}
}
- startLifecycleListeners();
}
@SuppressWarnings("unchecked")
diff --git a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
index 0992bcd..d349dda 100644
--- a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
@@ -177,6 +177,8 @@
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 @@
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 @@
this.stars = stars;
this.dynamicOptionParser = dynamicOptionParser;
this.change = change;
+ this.injector = injector;
+ this.dynamicBeans = dynamicBeans;
}
@Override
@@ -500,10 +506,10 @@
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 @@
@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 @@
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();
diff --git a/java/com/google/gerrit/server/api/changes/ChangesImpl.java b/java/com/google/gerrit/server/api/changes/ChangesImpl.java
index d6ef61c..0596524 100644
--- a/java/com/google/gerrit/server/api/changes/ChangesImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangesImpl.java
@@ -26,15 +26,18 @@
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 @@
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 @@
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,34 +132,36 @@
}
private List<ChangeInfo> get(QueryRequest q) throws RestApiException {
- QueryChanges qc = queryProvider.get();
- if (q.getQuery() != null) {
- qc.addQuery(q.getQuery());
- }
- qc.setLimit(q.getLimit());
- qc.setStart(q.getStart());
- qc.setNoLimit(q.getNoLimit());
- for (ListChangesOption option : q.getOptions()) {
- qc.addOption(option);
- }
- dynamicOptionParser.parseDynamicOptions(qc, q.getPluginOptions());
-
- try {
- List<?> result = qc.apply(TopLevelResource.INSTANCE).value();
- if (result.isEmpty()) {
- return ImmutableList.of();
+ try (DynamicOptions dynamicOptions = new DynamicOptions(injector, dynamicBeans)) {
+ QueryChanges qc = queryProvider.get();
+ if (q.getQuery() != null) {
+ qc.addQuery(q.getQuery());
}
+ qc.setLimit(q.getLimit());
+ qc.setStart(q.getStart());
+ qc.setNoLimit(q.getNoLimit());
+ for (ListChangesOption option : q.getOptions()) {
+ qc.addOption(option);
+ }
+ dynamicOptionParser.parseDynamicOptions(qc, q.getPluginOptions(), dynamicOptions);
- // Check type safety of result; the extension API should be safer than the
- // REST API in this case, since it's intended to be used in Java.
- Object first = requireNonNull(result.iterator().next());
- checkState(first instanceof ChangeInfo);
- @SuppressWarnings("unchecked")
- List<ChangeInfo> infos = (List<ChangeInfo>) result;
+ try {
+ List<?> result = qc.apply(TopLevelResource.INSTANCE).value();
+ if (result.isEmpty()) {
+ return ImmutableList.of();
+ }
- return ImmutableList.copyOf(infos);
- } catch (Exception e) {
- throw asRestApiException("Cannot query changes", e);
+ // Check type safety of result; the extension API should be safer than the
+ // REST API in this case, since it's intended to be used in Java.
+ Object first = requireNonNull(result.iterator().next());
+ checkState(first instanceof ChangeInfo);
+ @SuppressWarnings("unchecked")
+ List<ChangeInfo> infos = (List<ChangeInfo>) result;
+
+ return ImmutableList.copyOf(infos);
+ } catch (Exception e) {
+ throw asRestApiException("Cannot query changes", e);
+ }
}
}
}
diff --git a/java/com/google/gerrit/sshd/BaseCommand.java b/java/com/google/gerrit/sshd/BaseCommand.java
index 0dbae0a..fe03770 100644
--- a/java/com/google/gerrit/sshd/BaseCommand.java
+++ b/java/com/google/gerrit/sshd/BaseCommand.java
@@ -235,6 +235,8 @@
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 @@
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();
diff --git a/java/com/google/gerrit/sshd/DispatchCommand.java b/java/com/google/gerrit/sshd/DispatchCommand.java
index a45cd31..54171a3 100644
--- a/java/com/google/gerrit/sshd/DispatchCommand.java
+++ b/java/com/google/gerrit/sshd/DispatchCommand.java
@@ -72,8 +72,7 @@
@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();
diff --git a/java/com/google/gerrit/sshd/SshCommand.java b/java/com/google/gerrit/sshd/SshCommand.java
index 3ef7061..c94b25c 100644
--- a/java/com/google/gerrit/sshd/SshCommand.java
+++ b/java/com/google/gerrit/sshd/SshCommand.java
@@ -50,8 +50,7 @@
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);
diff --git a/java/com/google/gerrit/sshd/SuExec.java b/java/com/google/gerrit/sshd/SuExec.java
index bf785bb..3c6e8c2 100644
--- a/java/com/google/gerrit/sshd/SuExec.java
+++ b/java/com/google/gerrit/sshd/SuExec.java
@@ -93,7 +93,7 @@
@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);
diff --git a/java/com/google/gerrit/sshd/commands/StreamEvents.java b/java/com/google/gerrit/sshd/commands/StreamEvents.java
index 188cc83..c47d24c 100644
--- a/java/com/google/gerrit/sshd/commands/StreamEvents.java
+++ b/java/com/google/gerrit/sshd/commands/StreamEvents.java
@@ -108,8 +108,7 @@
@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) {
diff --git a/javatests/com/google/gerrit/acceptance/ssh/DynamicOptionsIT.java b/javatests/com/google/gerrit/acceptance/ssh/DynamicOptionsIT.java
new file mode 100644
index 0000000..c0f2b36
--- /dev/null
+++ b/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());
+ }
+}
