Merge "Ensure plugin modules are bound in the baseInjector" into stable-3.9

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