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

10 files changed, 138 insertions, 8 deletions

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<? 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 @@ 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<? 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 @@ 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<? 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 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<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
+      }
+    }
+  }
+}