From 4e82fd693e08f6b341ac7d4dc2a1ee6c875fb1d4 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Sat, 13 Apr 2024 08:11:02 +0000 Subject: Revert "Ensure plugin modules are bound in the baseInjector" This reverts commit 22145cb21b9346c2a60bf4e8dd97f4a44606de4e. Reason for revert: Gitiles does load anymore after this change Release-Notes: Fix Gitiles not loading because of injection issues Change-Id: Iba4d33faacec7d24ad461f83521ee59a632f2085 --- .../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, 8 insertions(+), 138 deletions(-) delete mode 100644 javatests/com/google/gerrit/acceptance/server/plugins/BUILD delete 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 7491068090..b4b99350d1 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -1905,15 +1905,14 @@ public abstract class AbstractDaemonTest { protected AutoCloseable installPlugin(String pluginName, Class sysModuleClass) throws Exception { - return installPlugin(pluginName, sysModuleClass, null, null, null); + return installPlugin(pluginName, sysModuleClass, null, null); } protected AutoCloseable installPlugin( String pluginName, @Nullable Class sysModuleClass, @Nullable Class httpModuleClass, - @Nullable Class sshModuleClass, - @Nullable Class apiModuleClass) + @Nullable Class sshModuleClass) throws Exception { checkStatic(sysModuleClass); checkStatic(httpModuleClass); @@ -1927,7 +1926,6 @@ 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 e1bd81839e..7e50b83b4f 100644 --- a/java/com/google/gerrit/acceptance/LightweightPluginDaemonTest.java +++ b/java/com/google/gerrit/acceptance/LightweightPluginDaemonTest.java @@ -45,7 +45,6 @@ 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 ba71c1185a..cafc775460 100644 --- a/java/com/google/gerrit/acceptance/TestPlugin.java +++ b/java/com/google/gerrit/acceptance/TestPlugin.java @@ -30,6 +30,4 @@ 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 f1804b3799..036285e330 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; - protected Class apiModuleClass; + private 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)) - .orElseGet(() -> Guice.createInjector(modules)); + .orElse(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 739dcdca16..0f7e87e3bd 100644 --- a/java/com/google/gerrit/server/plugins/ServerPluginInfoModule.java +++ b/java/com/google/gerrit/server/plugins/ServerPluginInfoModule.java @@ -22,7 +22,6 @@ 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; @@ -52,10 +51,6 @@ 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() { @@ -100,10 +95,4 @@ 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 a58ee3b366..cd5d5e38cf 100644 --- a/java/com/google/gerrit/server/plugins/TestServerPlugin.java +++ b/java/com/google/gerrit/server/plugins/TestServerPlugin.java @@ -23,7 +23,6 @@ public class TestServerPlugin extends ServerPlugin { private String sysName; private String httpName; private String sshName; - private String apiName; public TestServerPlugin( String name, @@ -33,7 +32,6 @@ public class TestServerPlugin extends ServerPlugin { String sysName, String httpName, String sshName, - String apiName, Path dataDir) throws InvalidPluginException { super( @@ -51,7 +49,6 @@ public class TestServerPlugin extends ServerPlugin { this.sysName = sysName; this.httpName = httpName; this.sshName = sshName; - this.apiName = apiName; loadGuiceModules(); } @@ -60,7 +57,6 @@ 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 f211366e44..d2c1430409 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)) { + try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, MyPluginSysModule.class, null, null)) { 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)) { + try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, MyPluginSysModule.class, null, null)) { 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 e9047e07d9..24ce605635 100644 --- a/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java @@ -208,16 +208,14 @@ public class PluginProvidedRootRestApiBindingsIT extends AbstractDaemonTest { @Test public void testEndpoints() throws Exception { - try (AutoCloseable ignored = - installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, null, null)) { + try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, null)) { RestApiCallHelper.execute(adminRestSession, TEST_CALLS.asList()); } } @Test public void testOptionOnSingletonIsIgnored() throws Exception { - try (AutoCloseable ignored = - installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, null, null)) { + try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, 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 deleted file mode 100644 index 25cc33fc1d..0000000000 --- a/javatests/com/google/gerrit/acceptance/server/plugins/BUILD +++ /dev/null @@ -1,7 +0,0 @@ -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 deleted file mode 100644 index d8def1ab01..0000000000 --- a/javatests/com/google/gerrit/acceptance/server/plugins/PluginBindingsIT.java +++ /dev/null @@ -1,101 +0,0 @@ -// 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