From 22145cb21b9346c2a60bf4e8dd97f4a44606de4e Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Fri, 5 Apr 2024 16:55:41 -0600 Subject: 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 --- .../gerrit/acceptance/AbstractDaemonTest.java | 6 +- .../acceptance/LightweightPluginDaemonTest.java | 1 + java/com/google/gerrit/acceptance/TestPlugin.java | 2 + .../google/gerrit/server/plugins/ServerPlugin.java | 4 +- .../server/plugins/ServerPluginInfoModule.java | 11 +++ .../gerrit/server/plugins/TestServerPlugin.java | 4 + .../PluginProvidedChildRestApiBindingsIT.java | 4 +- .../PluginProvidedRootRestApiBindingsIT.java | 6 +- .../google/gerrit/acceptance/server/plugins/BUILD | 7 ++ .../server/plugins/PluginBindingsIT.java | 101 +++++++++++++++++++++ 10 files changed, 138 insertions(+), 8 deletions(-) create mode 100644 javatests/com/google/gerrit/acceptance/server/plugins/BUILD create mode 100644 javatests/com/google/gerrit/acceptance/server/plugins/PluginBindingsIT.java diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 80582a47ec..b7635e67e5 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -1904,14 +1904,15 @@ public abstract class AbstractDaemonTest { protected AutoCloseable installPlugin(String pluginName, Class sysModuleClass) throws Exception { - return installPlugin(pluginName, sysModuleClass, null, null); + return installPlugin(pluginName, sysModuleClass, null, null, null); } protected AutoCloseable installPlugin( String pluginName, @Nullable Class sysModuleClass, @Nullable Class httpModuleClass, - @Nullable Class sshModuleClass) + @Nullable Class sshModuleClass, + @Nullable Class apiModuleClass) throws Exception { checkStatic(sysModuleClass); checkStatic(httpModuleClass); @@ -1925,6 +1926,7 @@ public abstract class AbstractDaemonTest { 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 7e50b83b4f..e1bd81839e 100644 --- a/java/com/google/gerrit/acceptance/LightweightPluginDaemonTest.java +++ b/java/com/google/gerrit/acceptance/LightweightPluginDaemonTest.java @@ -45,6 +45,7 @@ public class LightweightPluginDaemonTest extends AbstractDaemonTest { 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 cafc775460..ba71c1185a 100644 --- a/java/com/google/gerrit/acceptance/TestPlugin.java +++ b/java/com/google/gerrit/acceptance/TestPlugin.java @@ -30,4 +30,6 @@ public @interface TestPlugin { 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 036285e330..f1804b3799 100644 --- a/java/com/google/gerrit/server/plugins/ServerPlugin.java +++ b/java/com/google/gerrit/server/plugins/ServerPlugin.java @@ -51,7 +51,7 @@ public class ServerPlugin extends Plugin { protected Class batchModule; protected Class sshModule; protected Class httpModule; - private Class apiModuleClass; + protected Class apiModuleClass; private Injector apiInjector; private Injector sysInjector; @@ -286,7 +286,7 @@ public class ServerPlugin extends Plugin { 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 0f7e87e3bd..739dcdca16 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.lifecycle.LifecycleModule; 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 @@ class ServerPluginInfoModule extends AbstractModule { .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 @@ class ServerPluginInfoModule extends AbstractModule { File getPluginDataAsFile(@PluginData Path pluginData) { return pluginData.toFile(); } + + private void bindNonNull(Class 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 cd5d5e38cf..a58ee3b366 100644 --- a/java/com/google/gerrit/server/plugins/TestServerPlugin.java +++ b/java/com/google/gerrit/server/plugins/TestServerPlugin.java @@ -23,6 +23,7 @@ public class TestServerPlugin extends ServerPlugin { private String sysName; private String httpName; private String sshName; + private String apiName; public TestServerPlugin( String name, @@ -32,6 +33,7 @@ public class TestServerPlugin extends ServerPlugin { String sysName, String httpName, String sshName, + String apiName, Path dataDir) throws InvalidPluginException { super( @@ -49,6 +51,7 @@ public class TestServerPlugin extends ServerPlugin { this.sysName = sysName; this.httpName = httpName; this.sshName = sshName; + this.apiName = apiName; loadGuiceModules(); } @@ -57,6 +60,7 @@ public class TestServerPlugin extends ServerPlugin { 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 d2c1430409..f211366e44 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 @@ public class PluginProvidedChildRestApiBindingsIT extends AbstractDaemonTest { @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 class PluginProvidedChildRestApiBindingsIT extends AbstractDaemonTest { 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 24ce605635..e9047e07d9 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 @@ public class PluginProvidedRootRestApiBindingsIT extends AbstractDaemonTest { @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 0000000000..25cc33fc1d --- /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 0000000000..d8def1ab01 --- /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 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 + } + } + } +} -- cgit v1.2.3