Ensure plugin modules are bound in the baseInjector
It's required to do an explicit bind when using child injectors in order
to prevent just-in-time bindings from being promoted to an ancestor
injector. This is a well documented issue in Guice [1] that is only
mentioned a couple places in the javadocs, most prominently at [2].
In Gerrit this became a problem with the introduction of the plugin
ApiModule. That module is expected to be the base for other plugins, but
shouldn't get any bindings promoted from the per-plugin
ServerPluginInfoModule. Adding explicit bind() calls in
ServerPluginInfoModule for each of the plugin modules prevents that from
happening, as seen in the new PluginBindingsIT suite.
Also fix an issue with creating child injectors using a buggy
Optional.orElse() pattern when creating the new root injector for a
plugin. This was causing two new injectors to always be created for the
ServerPluginInfoModule.
[1] https://github.com/google/guice/issues/973
[2] https://google.github.io/guice/api-docs/latest/javadoc/com/google/inject/Injector.html#createChildInjector(java.lang.Iterable)
Bug: Issue 332512505
Change-Id: I8633083d97d28ea9d1e61a90bb86efdb399c12d0
Release-Notes: Fix installing plugins injecting the injector when an ApiModule plugin is also installed
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 80582a4..b7635e6 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1904,14 +1904,15 @@
protected AutoCloseable installPlugin(String pluginName, Class<? extends Module> sysModuleClass)
throws Exception {
- return installPlugin(pluginName, sysModuleClass, null, null);
+ return installPlugin(pluginName, sysModuleClass, null, null, null);
}
protected AutoCloseable installPlugin(
String pluginName,
@Nullable Class<? extends Module> sysModuleClass,
@Nullable Class<? extends Module> httpModuleClass,
- @Nullable Class<? extends Module> sshModuleClass)
+ @Nullable Class<? extends Module> sshModuleClass,
+ @Nullable Class<? extends Module> apiModuleClass)
throws Exception {
checkStatic(sysModuleClass);
checkStatic(httpModuleClass);
@@ -1925,6 +1926,7 @@
sysModuleClass != null ? sysModuleClass.getName() : null,
httpModuleClass != null ? httpModuleClass.getName() : null,
sshModuleClass != null ? sshModuleClass.getName() : null,
+ apiModuleClass != null ? apiModuleClass.getName() : null,
sitePaths.data_dir.resolve(pluginName));
plugin.start(pluginGuiceEnvironment);
pluginGuiceEnvironment.onStartPlugin(plugin);
diff --git a/java/com/google/gerrit/acceptance/LightweightPluginDaemonTest.java b/java/com/google/gerrit/acceptance/LightweightPluginDaemonTest.java
index 7e50b83..e1bd818 100644
--- a/java/com/google/gerrit/acceptance/LightweightPluginDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/LightweightPluginDaemonTest.java
@@ -45,6 +45,7 @@
testPlugin.sysModule(),
testPlugin.httpModule(),
testPlugin.sshModule(),
+ testPlugin.apiModule(),
tempDataDir.getRoot().toPath());
plugin.start(env);
diff --git a/java/com/google/gerrit/acceptance/TestPlugin.java b/java/com/google/gerrit/acceptance/TestPlugin.java
index cafc775..ba71c11 100644
--- a/java/com/google/gerrit/acceptance/TestPlugin.java
+++ b/java/com/google/gerrit/acceptance/TestPlugin.java
@@ -30,4 +30,6 @@
String httpModule() default "";
String sshModule() default "";
+
+ String apiModule() default "";
}
diff --git a/java/com/google/gerrit/server/plugins/ServerPlugin.java b/java/com/google/gerrit/server/plugins/ServerPlugin.java
index 036285e..f1804b3 100644
--- a/java/com/google/gerrit/server/plugins/ServerPlugin.java
+++ b/java/com/google/gerrit/server/plugins/ServerPlugin.java
@@ -51,7 +51,7 @@
protected Class<? extends Module> batchModule;
protected Class<? extends Module> sshModule;
protected Class<? extends Module> httpModule;
- private Class<? extends Module> apiModuleClass;
+ protected Class<? extends Module> apiModuleClass;
private Injector apiInjector;
private Injector sysInjector;
@@ -286,7 +286,7 @@
modules.add(new ServerPluginInfoModule(this, env.getServerMetrics()));
return apiInjector
.map(injector -> injector.createChildInjector(modules))
- .orElse(Guice.createInjector(modules));
+ .orElseGet(() -> Guice.createInjector(modules));
}
private Injector newRootInjectorWithApiModule(
diff --git a/java/com/google/gerrit/server/plugins/ServerPluginInfoModule.java b/java/com/google/gerrit/server/plugins/ServerPluginInfoModule.java
index 0f7e87e..739dcdc 100644
--- a/java/com/google/gerrit/server/plugins/ServerPluginInfoModule.java
+++ b/java/com/google/gerrit/server/plugins/ServerPluginInfoModule.java
@@ -22,6 +22,7 @@
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.server.PluginUser;
import com.google.inject.AbstractModule;
+import com.google.inject.Module;
import com.google.inject.Provides;
import com.google.inject.ProvisionException;
import java.io.File;
@@ -51,6 +52,10 @@
.annotatedWith(PluginCanonicalWebUrl.class)
.toInstance(plugin.getPluginCanonicalWebUrl());
bind(Plugin.class).toInstance(plugin);
+ bindNonNull(plugin.batchModule);
+ bindNonNull(plugin.sysModule);
+ bindNonNull(plugin.sshModule);
+ bindNonNull(plugin.httpModule);
install(
new LifecycleModule() {
@@ -95,4 +100,10 @@
File getPluginDataAsFile(@PluginData Path pluginData) {
return pluginData.toFile();
}
+
+ private void bindNonNull(Class<? extends Module> module) {
+ if (module != null) {
+ bind(module);
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/plugins/TestServerPlugin.java b/java/com/google/gerrit/server/plugins/TestServerPlugin.java
index cd5d5e3..a58ee3b 100644
--- a/java/com/google/gerrit/server/plugins/TestServerPlugin.java
+++ b/java/com/google/gerrit/server/plugins/TestServerPlugin.java
@@ -23,6 +23,7 @@
private String sysName;
private String httpName;
private String sshName;
+ private String apiName;
public TestServerPlugin(
String name,
@@ -32,6 +33,7 @@
String sysName,
String httpName,
String sshName,
+ String apiName,
Path dataDir)
throws InvalidPluginException {
super(
@@ -49,6 +51,7 @@
this.sysName = sysName;
this.httpName = httpName;
this.sshName = sshName;
+ this.apiName = apiName;
loadGuiceModules();
}
@@ -57,6 +60,7 @@
this.sysModule = load(sysName, classLoader);
this.httpModule = load(httpName, classLoader);
this.sshModule = load(sshName, classLoader);
+ this.apiModuleClass = load(apiName, classLoader);
} catch (ClassNotFoundException e) {
throw new InvalidPluginException("Unable to load plugin Guice Modules", e);
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedChildRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedChildRestApiBindingsIT.java
index d2c1430..f211366 100644
--- a/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedChildRestApiBindingsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedChildRestApiBindingsIT.java
@@ -169,7 +169,7 @@
@Test
public void testRevisionEndpoints() throws Exception {
PatchSet.Id patchSetId = createChange().getPatchSetId();
- try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, MyPluginSysModule.class, null, null)) {
+ try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, MyPluginSysModule.class)) {
RestApiCallHelper.execute(
adminRestSession,
REVISION_TEST_CALLS.asList(),
@@ -182,7 +182,7 @@
public void testRobotCommentEndpoints() throws Exception {
PatchSet.Id patchSetId = createChange().getPatchSetId();
String robotCommentUuid = createRobotComment(patchSetId.changeId());
- try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, MyPluginSysModule.class, null, null)) {
+ try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, MyPluginSysModule.class)) {
RestApiCallHelper.execute(
adminRestSession,
ROBOTCOMMENT_TEST_CALLS.asList(),
diff --git a/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java
index 24ce605..e9047e0 100644
--- a/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java
@@ -208,14 +208,16 @@
@Test
public void testEndpoints() throws Exception {
- try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, null)) {
+ try (AutoCloseable ignored =
+ installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, null, null)) {
RestApiCallHelper.execute(adminRestSession, TEST_CALLS.asList());
}
}
@Test
public void testOptionOnSingletonIsIgnored() throws Exception {
- try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, null)) {
+ try (AutoCloseable ignored =
+ installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, null, null)) {
RestApiCallHelper.execute(
adminRestSession,
RestCall.get("/plugins/" + PLUGIN_NAME + "/test-collection/1/detail?crash=xyz"));
diff --git a/javatests/com/google/gerrit/acceptance/server/plugins/BUILD b/javatests/com/google/gerrit/acceptance/server/plugins/BUILD
new file mode 100644
index 0000000..25cc33f
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/plugins/BUILD
@@ -0,0 +1,7 @@
+load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
+
+acceptance_tests(
+ srcs = glob(["*IT.java"]),
+ group = "server_plugins",
+ labels = ["server"],
+)
diff --git a/javatests/com/google/gerrit/acceptance/server/plugins/PluginBindingsIT.java b/javatests/com/google/gerrit/acceptance/server/plugins/PluginBindingsIT.java
new file mode 100644
index 0000000..d8def1a
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/plugins/PluginBindingsIT.java
@@ -0,0 +1,101 @@
+// Copyright (C) 2024 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.server.plugins;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.server.config.GerritIsReplica;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Injector;
+import com.google.inject.Key;
+import org.junit.Test;
+
+public class PluginBindingsIT extends AbstractDaemonTest {
+ public static class TestPluginApiModule extends AbstractModule {}
+
+ public static class TestPluginSysModule extends AbstractModule {}
+
+ public static class PluginInjectsInjectorModule extends AbstractModule {
+ private final Injector injector;
+
+ @Inject
+ PluginInjectsInjectorModule(Injector injector) {
+ this.injector = injector;
+ }
+
+ @Override
+ protected void configure() {
+ Key<String> pluginNameKey = Key.get(String.class, PluginName.class);
+ injector.getInstance(pluginNameKey);
+ }
+ }
+
+ public static class PluginInjectsGerritReplicaModule extends AbstractModule {
+ private final boolean isReplica;
+
+ @Inject
+ PluginInjectsGerritReplicaModule(@GerritIsReplica boolean isReplica) {
+ this.isReplica = isReplica;
+ }
+
+ @Override
+ protected void configure() {
+ assertThat(isReplica).isFalse();
+ }
+ }
+
+ @Test
+ public void testCanInstallPluginInjectingInjector() throws Exception {
+ try (AutoCloseable ignored =
+ installPlugin("my-plugin-injecting-injector", PluginInjectsInjectorModule.class)) {
+ // test passes so long as no exception is thrown
+ }
+ }
+
+ @Test
+ public void testCanInstallPluginInjectingInjectorAfterInstallingApiModule() throws Exception {
+ try (AutoCloseable ignored =
+ installPlugin(
+ "my-api-plugin", TestPluginSysModule.class, null, null, TestPluginApiModule.class)) {
+ try (AutoCloseable ignored2 =
+ installPlugin("my-plugin-injecting-injector", PluginInjectsInjectorModule.class)) {
+ // test passes so long as no exception is thrown
+ }
+ }
+ }
+
+ @Test
+ public void testCanInstallPluginInjectingReplica() throws Exception {
+ try (AutoCloseable ignored =
+ installPlugin("my-plugin-injecting-replica", PluginInjectsGerritReplicaModule.class)) {
+ // test passes so long as no exception is thrown
+ }
+ }
+
+ @Test
+ public void testCanInstallPluginInjectingReplicaAfterInstallingApiModule() throws Exception {
+ try (AutoCloseable ignored =
+ installPlugin(
+ "my-api-plugin", TestPluginSysModule.class, null, null, TestPluginApiModule.class)) {
+ try (AutoCloseable ignored2 =
+ installPlugin("my-plugin-injecting-replica", PluginInjectsGerritReplicaModule.class)) {
+ // test passes so long as no exception is thrown
+ }
+ }
+ }
+}
