Ensure module instances are not shared by multiple JDBC catalogs

When a JdbcPlugin is instantiated, creating an instance of a module at that time causes the instance to be shared by all connectors provided by the plugin. This is problematic when the module extends AbstractConfigurationAwareModule, as it holds a reference to the ConfigurationFactory, which is set dynamically during bootstrap. If catalogs are loaded concurrently, this can lead to situations where a connector accesses the configuration of another connector.
trinodb · Nov 7, 2024 · 33b6886 · 33b6886
1 parent a68ed81
commit 33b6886
Show file tree

Hide file tree

Showing 25 changed files with 35 additions and 33 deletions.
diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcConnectorFactory.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcConnectorFactory.java
@@ -26,6 +26,7 @@
 import io.trino.spi.type.TypeManager;
 
 import java.util.Map;
+import java.util.function.Supplier;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Strings.isNullOrEmpty;
@@ -36,13 +37,13 @@ public class JdbcConnectorFactory
         implements ConnectorFactory
 {
     private final String name;
-    private final Module module;
+    private final Supplier<Module> moduleSupplier;
 
-    public JdbcConnectorFactory(String name, Module module)
+    public JdbcConnectorFactory(String name, Supplier<Module> moduleSupplier)
     {
         checkArgument(!isNullOrEmpty(name), "name is null or empty");
         this.name = name;
-        this.module = module;
+        this.moduleSupplier = requireNonNull(moduleSupplier, "moduleSupplier is null");
     }
 
     @Override
@@ -55,7 +56,6 @@ public String getName()
     public Connector create(String catalogName, Map<String, String> requiredConfig, ConnectorContext context)
     {
         requireNonNull(requiredConfig, "requiredConfig is null");
-        requireNonNull(module, "module is null");
         checkStrictSpiVersionMatch(context, this);
 
         Bootstrap app = new Bootstrap(
@@ -65,7 +65,7 @@ public Connector create(String catalogName, Map<String, String> requiredConfig,
                 binder -> binder.bind(OpenTelemetry.class).toInstance(context.getOpenTelemetry()),
                 binder -> binder.bind(CatalogName.class).toInstance(new CatalogName(catalogName)),
                 new JdbcModule(),
-                module);
+                requireNonNull(moduleSupplier.get(), "moduleSupplier.get() is null"));
 
         Injector injector = app
                 .doNotInitializeLogging()

diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPlugin.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcPlugin.java
@@ -19,6 +19,8 @@
 import io.trino.spi.Plugin;
 import io.trino.spi.connector.ConnectorFactory;
 
+import java.util.function.Supplier;
+
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Strings.isNullOrEmpty;
 import static io.airlift.configuration.ConfigurationAwareModule.combine;
@@ -28,23 +30,23 @@ public class JdbcPlugin
         implements Plugin
 {
     private final String name;
-    private final Module module;
+    private final Supplier<Module> moduleSupplier;
 
-    public JdbcPlugin(String name, Module module)
+    public JdbcPlugin(String name, Supplier<Module> moduleSupplier)
     {
         checkArgument(!isNullOrEmpty(name), "name is null or empty");
         this.name = name;
-        this.module = requireNonNull(module, "module is null");
+        this.moduleSupplier = requireNonNull(moduleSupplier, "moduleSupplier is null");
     }
 
     @Override
     public Iterable<ConnectorFactory> getConnectorFactories()
     {
         return ImmutableList.of(new JdbcConnectorFactory(
                 name,
-                combine(
+                () -> combine(
                         new CredentialProviderModule(),
                         new ExtraCredentialsBasedIdentityCacheMappingModule(),
-                        module)));
+                        moduleSupplier.get())));
     }
 }
diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/H2QueryRunner.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/H2QueryRunner.java
@@ -81,7 +81,7 @@ public static QueryRunner createH2QueryRunner(
 
             createSchema(properties, "tpch");
 
-            queryRunner.installPlugin(new JdbcPlugin("base_jdbc", module));
+            queryRunner.installPlugin(new JdbcPlugin("base_jdbc", () -> module));
             queryRunner.createCatalog("jdbc", "base_jdbc", properties);
 
             copyTpchTables(queryRunner, "tpch", TINY_SCHEMA_NAME, tables);

diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcPlugin.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcPlugin.java
@@ -49,7 +49,7 @@ public void testRuleBasedIdentifierCanBeUsedTogetherWithCacheBased()
 
     private static ConnectorFactory getConnectorFactory()
     {
-        Plugin plugin = new JdbcPlugin("jdbc", new TestingH2JdbcModule());
+        Plugin plugin = new JdbcPlugin("jdbc", TestingH2JdbcModule::new);
         return getOnlyElement(plugin.getConnectorFactories());
     }
 }
diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJmxStats.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJmxStats.java
@@ -37,7 +37,7 @@ public class TestJmxStats
     public void testJmxStatsExposure()
             throws Exception
     {
-        Plugin plugin = new JdbcPlugin("base_jdbc", new TestingH2JdbcModule());
+        Plugin plugin = new JdbcPlugin("base_jdbc", TestingH2JdbcModule::new);
         ConnectorFactory factory = getOnlyElement(plugin.getConnectorFactories());
         factory.create(
                 "test",

diff --git a/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHousePlugin.java b/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHousePlugin.java
@@ -20,6 +20,6 @@ public class ClickHousePlugin
 {
     public ClickHousePlugin()
     {
-        super("clickhouse", new ClickHouseClientModule());
+        super("clickhouse", ClickHouseClientModule::new);
     }
 }
diff --git a/plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcPlugin.java b/plugin/trino-druid/src/main/java/io/trino/plugin/druid/DruidJdbcPlugin.java
@@ -20,6 +20,6 @@ public class DruidJdbcPlugin
 {
     public DruidJdbcPlugin()
     {
-        super("druid", new DruidJdbcClientModule());
+        super("druid", DruidJdbcClientModule::new);
     }
 }
diff --git a/plugin/trino-example-jdbc/src/main/java/io/trino/plugin/example/ExamplePlugin.java b/plugin/trino-example-jdbc/src/main/java/io/trino/plugin/example/ExamplePlugin.java
@@ -20,6 +20,6 @@ public class ExamplePlugin
 {
     public ExamplePlugin()
     {
-        super("example_jdbc", new ExampleClientModule());
+        super("example_jdbc", ExampleClientModule::new);
     }
 }
diff --git a/plugin/trino-exasol/src/main/java/io/trino/plugin/exasol/ExasolPlugin.java b/plugin/trino-exasol/src/main/java/io/trino/plugin/exasol/ExasolPlugin.java
@@ -20,6 +20,6 @@ public class ExasolPlugin
 {
     public ExasolPlugin()
     {
-        super("exasol", new ExasolClientModule());
+        super("exasol", ExasolClientModule::new);
     }
 }
diff --git a/plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgnitePlugin.java b/plugin/trino-ignite/src/main/java/io/trino/plugin/ignite/IgnitePlugin.java
@@ -20,6 +20,6 @@ public class IgnitePlugin
 {
     public IgnitePlugin()
     {
-        super("ignite", new IgniteClientModule());
+        super("ignite", IgniteClientModule::new);
     }
 }
diff --git a/plugin/trino-mariadb/src/main/java/io/trino/plugin/mariadb/MariaDbPlugin.java b/plugin/trino-mariadb/src/main/java/io/trino/plugin/mariadb/MariaDbPlugin.java
@@ -20,6 +20,6 @@ public class MariaDbPlugin
 {
     public MariaDbPlugin()
     {
-        super("mariadb", new MariaDbClientModule());
+        super("mariadb", MariaDbClientModule::new);
     }
 }
diff --git a/plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlPlugin.java b/plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlPlugin.java
@@ -20,6 +20,6 @@ public class MySqlPlugin
 {
     public MySqlPlugin()
     {
-        super("mysql", new MySqlClientModule());
+        super("mysql", MySqlClientModule::new);
     }
 }
diff --git a/plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OraclePlugin.java b/plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OraclePlugin.java
@@ -20,6 +20,6 @@ public class OraclePlugin
 {
     public OraclePlugin()
     {
-        super("oracle", new OracleClientModule());
+        super("oracle", OracleClientModule::new);
     }
 }
diff --git a/plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlPlugin.java b/plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlPlugin.java
@@ -22,6 +22,6 @@ public class PostgreSqlPlugin
 {
     public PostgreSqlPlugin()
     {
-        super("postgresql", combine(new PostgreSqlClientModule(), new PostgreSqlConnectionFactoryModule()));
+        super("postgresql", () -> combine(new PostgreSqlClientModule(), new PostgreSqlConnectionFactoryModule()));
     }
 }
diff --git a/...gresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionAccesses.java b/...gresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionAccesses.java
@@ -60,7 +60,7 @@ protected QueryRunner createQueryRunner()
                 .setAdditionalSetup(runner -> {
                     runner.installPlugin(new JdbcPlugin(
                             "counting_postgresql",
-                            combine(new PostgreSqlClientModule(), new TestingPostgreSqlModule(connectionFactory))));
+                            () -> combine(new PostgreSqlClientModule(), new TestingPostgreSqlModule(connectionFactory))));
                     runner.createCatalog("counting_postgresql", "counting_postgresql", ImmutableMap.of(
                             "connection-url", postgreSqlServer.getJdbcUrl(),
                             "connection-user", postgreSqlServer.getUser(),

diff --git a/...gresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java b/...gresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java
@@ -58,7 +58,7 @@ protected QueryRunner createQueryRunner()
                 .setAdditionalSetup(runner -> {
                     runner.installPlugin(new JdbcPlugin(
                             "counting_postgresql",
-                            combine(new PostgreSqlClientModule(), new TestingPostgreSqlModule(connectionFactory))));
+                            () -> combine(new PostgreSqlClientModule(), new TestingPostgreSqlModule(connectionFactory))));
                     runner.createCatalog("counting_postgresql", "counting_postgresql", ImmutableMap.of(
                             "connection-url", postgreSqlServer.getJdbcUrl(),
                             "connection-user", postgreSqlServer.getUser(),

diff --git a/plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftPlugin.java b/plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftPlugin.java
@@ -20,6 +20,6 @@ public class RedshiftPlugin
 {
     public RedshiftPlugin()
     {
-        super("redshift", new RedshiftClientModule());
+        super("redshift", RedshiftClientModule::new);
     }
 }
diff --git a/plugin/trino-singlestore/src/main/java/io/trino/plugin/singlestore/SingleStorePlugin.java b/plugin/trino-singlestore/src/main/java/io/trino/plugin/singlestore/SingleStorePlugin.java
@@ -21,6 +21,6 @@ public class SingleStorePlugin
 {
     public SingleStorePlugin()
     {
-        super("singlestore", new SingleStoreClientModule());
+        super("singlestore", SingleStoreClientModule::new);
     }
 }
diff --git a/plugin/trino-snowflake/src/main/java/io/trino/plugin/snowflake/SnowflakePlugin.java b/plugin/trino-snowflake/src/main/java/io/trino/plugin/snowflake/SnowflakePlugin.java
@@ -20,6 +20,6 @@ public class SnowflakePlugin
 {
     public SnowflakePlugin()
     {
-        super("snowflake", new SnowflakeClientModule());
+        super("snowflake", SnowflakeClientModule::new);
     }
 }
diff --git a/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerPlugin.java b/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerPlugin.java
@@ -22,6 +22,6 @@ public class SqlServerPlugin
 {
     public SqlServerPlugin()
     {
-        super("sqlserver", combine(new SqlServerClientModule(), new SqlServerConnectionFactoryModule()));
+        super("sqlserver", () -> combine(new SqlServerClientModule(), new SqlServerConnectionFactoryModule()));
     }
 }
diff --git a/...qlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerJdbcConnectionAccesses.java b/...qlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerJdbcConnectionAccesses.java
@@ -61,7 +61,7 @@ protected QueryRunner createQueryRunner()
                 .setAdditionalSetup(runner -> {
                     runner.installPlugin(new JdbcPlugin(
                             "counting_sqlserver",
-                            combine(new SqlServerClientModule(), new TestingSqlServerModule(connectionFactory))));
+                            () -> combine(new SqlServerClientModule(), new TestingSqlServerModule(connectionFactory))));
                     runner.createCatalog("counting_sqlserver", "counting_sqlserver", ImmutableMap.of(
                             "connection-url", sqlServer.getJdbcUrl(),
                             "connection-user", sqlServer.getUsername(),

diff --git a/...qlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerJdbcConnectionCreation.java b/...qlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerJdbcConnectionCreation.java
@@ -59,7 +59,7 @@ protected QueryRunner createQueryRunner()
                 .setAdditionalSetup(runner -> {
                     runner.installPlugin(new JdbcPlugin(
                             "counting_sqlserver",
-                            combine(new SqlServerClientModule(), new TestingSqlServerModule(connectionFactory))));
+                            () -> combine(new SqlServerClientModule(), new TestingSqlServerModule(connectionFactory))));
                     runner.createCatalog("counting_sqlserver", "counting_sqlserver", ImmutableMap.of(
                             "connection-url", sqlServer.getJdbcUrl(),
                             "connection-user", sqlServer.getUsername(),

diff --git a/plugin/trino-vertica/src/main/java/io/trino/plugin/vertica/VerticaPlugin.java b/plugin/trino-vertica/src/main/java/io/trino/plugin/vertica/VerticaPlugin.java
@@ -20,6 +20,6 @@ public class VerticaPlugin
 {
     public VerticaPlugin()
     {
-        super("vertica", new VerticaClientModule());
+        super("vertica", VerticaClientModule::new);
     }
 }
diff --git a/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java b/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java
@@ -272,7 +272,7 @@ public ScalarFunctionImplementation getScalarFunctionImplementation(FunctionId f
                 }))
                 .build()));
         queryRunner.createCatalog("mock", "mock");
-        queryRunner.installPlugin(new JdbcPlugin("base_jdbc", new TestingH2JdbcModule()));
+        queryRunner.installPlugin(new JdbcPlugin("base_jdbc", TestingH2JdbcModule::new));
         queryRunner.createCatalog("jdbc", "base_jdbc", TestingH2JdbcModule.createProperties());
         for (String tableName : ImmutableList.of("orders", "nation", "region", "lineitem")) {
             queryRunner.execute(format("CREATE TABLE %1$s AS SELECT * FROM tpch.tiny.%1$s WITH NO DATA", tableName));

diff --git a/testing/trino-tests/src/test/java/io/trino/tests/TestQueryAssertions.java b/testing/trino-tests/src/test/java/io/trino/tests/TestQueryAssertions.java
@@ -75,7 +75,7 @@ protected void configureCatalog(QueryRunner queryRunner)
         queryRunner.installPlugin(new TpchPlugin());
         queryRunner.createCatalog("tpch", "tpch", ImmutableMap.of());
 
-        queryRunner.installPlugin(new JdbcPlugin("base_jdbc", new TestingH2JdbcModule()));
+        queryRunner.installPlugin(new JdbcPlugin("base_jdbc", TestingH2JdbcModule::new));
         Map<String, String> jdbcConfigurationProperties = TestingH2JdbcModule.createProperties();
         queryRunner.createCatalog("jdbc", "base_jdbc", jdbcConfigurationProperties);