This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-c7a4551b-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 82a3351d7034797bd4bfa4c872ce85956f157b86 Author: Qi Yu <[email protected]> AuthorDate: Tue Nov 3 11:15:10 2026 +0800 [#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485) ### What changes were proposed in this pull request? Validate canonical and native pool settings before pool initialization or source verification, independently of whether verification is enabled. Failed catalog creation rolls back the stored entity and secrets; alteration validates before writing secrets or updating the entity. Classify PostgreSQL permission failures correctly and reject NUL comments/defaults before DDL. Failed-create privilege diagnosis releases the original connection before probing schema visibility. If diagnosis fails, its exception retains the original create failure as a suppressed exception; PostgreSQL exception conversion preserves the underlying SQL causes. ### Why are the changes needed? Invalid settings must not be persisted, and invalid input or missing privileges must not look like source connectivity failures. Fixes #14484 Fixes #14502 Consolidates #14513 into this PR. ### Does this PR introduce _any_ user-facing change? Invalid driver, boolean and pool settings fail with actionable errors; permission failures remain forbidden and NUL input is rejected as invalid input. Adds a default configuration-only catalog validation hook. No source connection is opened by static validation. ### How was this patch tested? Previous full runs: coreUnitTest: 2,692 cases, 2 skipped; JDBC common: 132; PostgreSQL: 92, Docker enabled. No failures. This follow-up: 21 targeted cases passed, including exception-cause preservation, diagnostic connection/query failures, and the live PostgreSQL least-privilege create regression. Independent cumulative review and prior component mutation regressions verified. Deployed REST/Spark/Trino end-to-end is not claimed. # Conflicts: # catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java # catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java # catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogPropertiesMetadata.java # catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceValidation.java # catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java # core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java # core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java --- .../apache/gravitino/catalog/jdbc/JdbcCatalog.java | 32 +- .../catalog/jdbc/JdbcCatalogOperations.java | 27 +- .../jdbc/JdbcCatalogPropertiesMetadata.java | 19 +- .../gravitino/catalog/jdbc/config/JdbcConfig.java | 14 +- .../catalog/jdbc/utils/DataSourceUtils.java | 156 ++++-- .../jdbc/TestJdbcCatalogPropertiesMetadata.java | 159 ++++++- .../jdbc/utils/TestDataSourceUrlValidation.java | 4 +- .../jdbc/utils/TestDataSourceValidation.java | 217 +++++++++ .../postgresql/PostgreSQLCatalogOperations.java | 3 +- .../catalog/postgresql/PostgreSqlCatalog.java | 4 +- .../PostgreSqlColumnDefaultValueConverter.java | 12 + .../converter/PostgreSqlExceptionConverter.java | 21 +- .../operation/PostgreSqlTableOperations.java | 115 ++++- .../TestPostgreSqlCatalogConfiguration.java | 28 ++ .../TestPostgreSqlColumnDefaultValueConverter.java | 14 + .../TestPostgreSqlExceptionConverter.java | 79 ++++ .../TestPostgreSqlTableCreatePrivileges.java | 165 +++++++ .../operation/TestPostgreSqlTableOperations.java | 151 ++++++ ...TestPostgreSqlTableOperationsSqlGeneration.java | 105 +++++ .../apache/gravitino/catalog/CatalogManager.java | 97 ++++ .../apache/gravitino/connector/BaseCatalog.java | 23 + .../java/org/apache/gravitino/TestCatalog.java | 10 + .../gravitino/catalog/TestCatalogManager.java | 525 +++++++++++++++++++++ docs/jdbc-catalog-connection-validation.md | 7 +- 24 files changed, 1890 insertions(+), 97 deletions(-) diff --git a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java index 7f5c2f831c..b4c8593397 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java +++ b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalog.java @@ -31,6 +31,7 @@ import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter; import org.apache.gravitino.catalog.jdbc.converter.JdbcTypeConverter; import org.apache.gravitino.catalog.jdbc.operation.JdbcDatabaseOperations; import org.apache.gravitino.catalog.jdbc.operation.JdbcTableOperations; +import org.apache.gravitino.catalog.jdbc.utils.DataSourceUtils; import org.apache.gravitino.connector.BaseCatalog; import org.apache.gravitino.connector.CatalogOperations; import org.apache.gravitino.connector.PropertiesMetadata; @@ -86,6 +87,32 @@ public abstract class JdbcCatalog extends BaseCatalog<JdbcCatalog> { return false; } + /** {@inheritDoc} */ + @Override + public JdbcCatalog withCatalogConf(Map<String, String> configuration) { + return super.withCatalogConf(prepareConfiguration(configuration)); + } + + /** + * Normalizes and validates provider-specific settings for initialization and proposed changes. + * + * @param configuration the complete proposed configuration, which must not be mutated + * @return the normalized settings; the default returns the original map + * @throws IllegalArgumentException if provider-specific settings are inconsistent + */ + protected Map<String, String> prepareConfiguration(Map<String, String> configuration) { + return configuration; + } + + /** {@inheritDoc} */ + @Override + public void validateConfiguration(Map<String, String> configuration) { + JdbcCatalogPropertiesMetadata metadata = + (JdbcCatalogPropertiesMetadata) catalogPropertiesMetadata(); + DataSourceUtils.validateConfiguration( + new JdbcConfig(metadata.jdbcProperties(prepareConfiguration(configuration)))); + } + /** * Returns the effective {@link JdbcConfig#VERIFY_ON_CREATE verify-on-create} setting for this * catalog. A relational JDBC catalog verifies its backend at create time by default; an @@ -100,7 +127,10 @@ public abstract class JdbcCatalog extends BaseCatalog<JdbcCatalog> { if (properties == null) { return true; } - return new JdbcConfig(properties).getVerifyOnCreate(); + JdbcCatalogPropertiesMetadata metadata = + (JdbcCatalogPropertiesMetadata) catalogPropertiesMetadata(); + return new JdbcConfig(metadata.jdbcProperties(prepareConfiguration(properties))) + .getVerifyOnCreate(); } /** diff --git a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java index 1286a35217..97a40d8d5b 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java +++ b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java @@ -18,8 +18,6 @@ */ package org.apache.gravitino.catalog.jdbc; -import static org.apache.gravitino.connector.BaseCatalog.CATALOG_BYPASS_PREFIX; - import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.Maps; @@ -80,8 +78,11 @@ import org.apache.gravitino.rel.expressions.distributions.Distribution; import org.apache.gravitino.rel.expressions.sorts.SortOrder; import org.apache.gravitino.rel.expressions.transforms.Transform; import org.apache.gravitino.rel.indexes.Index; +<<<<<<< HEAD import org.apache.gravitino.utils.IsolatedClassLoader; import org.apache.gravitino.utils.MapUtils; +======= +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) import org.apache.gravitino.utils.PrincipalUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -167,14 +168,7 @@ public class JdbcCatalogOperations implements CatalogOperations, SupportsSchemas this.jdbcTablePropertiesMetadata = (JdbcTablePropertiesMetadata) propertiesMetadata.tablePropertiesMetadata(); - // Key format like gravitino.bypass.a.b - Map<String, String> prefixMap = MapUtils.getPrefixMap(conf, CATALOG_BYPASS_PREFIX); - - // Hold keys that lie in JDBC_IMMUTABLE_PROPERTIES - Map<String, String> gravitinoConfig = - this.jdbcCatalogPropertiesMetadata.transformProperties(conf); - Map<String, String> resultConf = Maps.newHashMap(prefixMap); - resultConf.putAll(gravitinoConfig); + Map<String, String> resultConf = this.jdbcCatalogPropertiesMetadata.jdbcProperties(conf); this.jdbcConfig = new JdbcConfig(resultConf); this.jdbcUrl = jdbcConfig.getJdbcUrl(); @@ -228,18 +222,15 @@ public class JdbcCatalogOperations implements CatalogOperations, SupportsSchemas return null; } - return DriverManager.getDriver(jdbcUrl); + return DataSourceUtils.configuredDriver(jdbcConfig); } /** - * Initializes the configured driver class so that it registers itself with {@link DriverManager} - * before {@link #getDriver()} looks it up. + * Initializes the configured driver for provider overrides that still use {@link DriverManager}. * - * <p>{@code DriverManager} only discovers drivers on the server classpath by itself. A driver in - * the catalog's own libs directory registers only when its class is initialized, which the lazy - * connection pool does on its first connection. The create-time probe runs before that, so - * without this the lookup fails with "No suitable driver". The class is loaded by this catalog's - * class loader, which is also the loader {@code DriverManager} checks the caller against. + * <p>The default {@link #getDriver()} instantiates the configured driver directly. Legacy + * overrides, such as {@link MySQLProtocolCompatibleCatalogOperations#getDriver()}, instead use + * DriverManager lookup, which requires drivers in catalog-specific libs to register first. */ private void ensureDriverRegistered() { String driverClassName = jdbcConfig.getJdbcDriver(); diff --git a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogPropertiesMetadata.java b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogPropertiesMetadata.java index 5dc879452e..3bad98923d 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogPropertiesMetadata.java +++ b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogPropertiesMetadata.java @@ -18,6 +18,7 @@ */ package org.apache.gravitino.catalog.jdbc; +import static org.apache.gravitino.connector.BaseCatalog.CATALOG_BYPASS_PREFIX; import static org.apache.gravitino.connector.PropertyEntry.booleanPropertyEntry; import static org.apache.gravitino.connector.PropertyEntry.integerPropertyEntry; import static org.apache.gravitino.connector.PropertyEntry.longOptionalPropertyEntry; @@ -32,6 +33,7 @@ import java.util.Map; import org.apache.gravitino.catalog.jdbc.config.JdbcConfig; import org.apache.gravitino.connector.BaseCatalogPropertiesMetadata; import org.apache.gravitino.connector.PropertyEntry; +import org.apache.gravitino.utils.MapUtils; public class JdbcCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata { @@ -120,7 +122,7 @@ public class JdbcCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata false /* required */, false /* immutable */, JdbcConfig.TEST_ON_BORROW.getDefaultValue(), - true /* hidden */, + false /* hidden */, false /* reserved */), longOptionalPropertyEntry( JdbcConfig.CONNECT_TIMEOUT_MS.getKey(), @@ -134,7 +136,7 @@ public class JdbcCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata false /* required */, false /* immutable */, JdbcConfig.VERIFY_ON_CREATE.getDefaultValue(), - true /* hidden */, + false /* hidden */, false /* reserved */)); PROPERTIES_METADATA = ImmutableMap.<String, PropertyEntry<?>>builder() @@ -147,6 +149,19 @@ public class JdbcCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata return PROPERTIES_METADATA; } + /** + * Returns the effective JDBC settings used by validation and operation initialization. + * + * @param properties the catalog configuration + * @return a new map with bypass prefixes removed and canonical JDBC properties taking precedence + */ + public Map<String, String> jdbcProperties(Map<String, String> properties) { + Map<String, String> result = + Maps.newHashMap(MapUtils.getPrefixMap(properties, CATALOG_BYPASS_PREFIX)); + result.putAll(transformProperties(properties)); + return result; + } + public Map<String, String> transformProperties(Map<String, String> properties) { Map<String, String> result = Maps.newHashMap(); properties.forEach( diff --git a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/config/JdbcConfig.java b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/config/JdbcConfig.java index a6ff2aa7e9..435fe832eb 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/config/JdbcConfig.java +++ b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/config/JdbcConfig.java @@ -177,7 +177,7 @@ public class JdbcConfig extends Config { } public boolean getTestOnBorrow() { - return get(TEST_ON_BORROW); + return strictBoolean(TEST_ON_BORROW); } public long getMaxWaitMs() { @@ -199,11 +199,21 @@ public class JdbcConfig extends Config { * @return {@code true} to verify the backend at create (default); {@code false} to skip the check */ public boolean getVerifyOnCreate() { - return get(VERIFY_ON_CREATE); + return strictBoolean(VERIFY_ON_CREATE); } public JdbcConfig(Map<String, String> properties) { super(false); loadFromMap(properties, k -> true); } + + private boolean strictBoolean(ConfigEntry<Boolean> entry) { + String value = getAllConfig().get(entry.getKey()); + if (value != null + && !"true".equalsIgnoreCase(value.trim()) + && !"false".equalsIgnoreCase(value.trim())) { + throw new IllegalArgumentException(entry.getKey() + " must be true or false"); + } + return get(entry); + } } diff --git a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java index c298fbd4d1..aed0921c46 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java +++ b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/utils/DataSourceUtils.java @@ -18,6 +18,7 @@ */ package org.apache.gravitino.catalog.jdbc.utils; +import java.sql.Driver; import java.sql.SQLException; import java.time.Duration; import java.util.Map; @@ -45,6 +46,22 @@ public class DataSourceUtils { public static DataSource createDataSource(JdbcConfig jdbcConfig) throws GravitinoRuntimeException { + validateConfiguration(jdbcConfig); + try { + return createDBCPDataSource(jdbcConfig); + } catch (Exception exception) { + throw new GravitinoRuntimeException(exception, "Error creating datasource"); + } + } + + /** + * Validates JDBC settings without opening a connection or retaining a pool. + * + * @param jdbcConfig the effective JDBC configuration + * @throws IllegalArgumentException if a boolean, pool bound, driver or URL is invalid + * @throws GravitinoRuntimeException if a dangerous URL or driver setting is rejected + */ + public static void validateConfiguration(JdbcConfig jdbcConfig) { // H2 is bundled as an embedded backend and must not be used through user-facing catalog // configuration. Its INIT parameter allows arbitrary SQL (and Java code via CREATE ALIAS) // to execute at connection time, and the H2 driver class must also be blocked to prevent @@ -56,61 +73,115 @@ public class DataSourceUtils { if (jdbcConfig.getJdbcDriver().toLowerCase().startsWith("org.h2.")) { throw new GravitinoRuntimeException("H2 JDBC driver is not allowed in catalog configuration"); } +<<<<<<< HEAD +======= + // Reject DBCP2 pool properties that load arbitrary classes via reflection before handing the + // config to the factory. Kept outside the try below so the specific reason surfaces directly + // instead of being wrapped as "Error creating datasource". + rejectUnsafePoolProperties(jdbcConfig.getAllConfig()); + // Validate user configuration before the data-source builder wraps infrastructure failures. + jdbcConfig.getVerifyOnCreate(); + jdbcConfig.getTestOnBorrow(); + jdbcConfig.getConnectTimeoutMs(); + jdbcConfig.getMaxWaitMs(); + jdbcConfig.getPoolMaxIdle(); + int minSize = jdbcConfig.getPoolMinSize(); + int maxSize = jdbcConfig.getPoolMaxSize(); + if (minSize > maxSize) { + throw new IllegalArgumentException("jdbc.pool.min-size must not exceed jdbc.pool.max-size"); + } +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) try { - return createDBCPDataSource(jdbcConfig); - } catch (Exception exception) { - if (isDriverClassMissing(exception)) { - // Some JDBC drivers are not packaged with Gravitino and must be installed by the user. - // Surface a clear, actionable message naming the driver instead of a raw - // ClassNotFoundException. - throw new GravitinoRuntimeException( - exception, - "JDBC driver class '%s' was not found on the catalog classpath. Install the driver " - + "JAR in the catalog's libs directory and recreate the catalog.", - jdbcConfig.getJdbcDriver()); + JdbcUrlUtils.validateJdbcConfig( + jdbcConfig.getJdbcDriver(), jdbcConfig.getJdbcUrl(), jdbcConfig.getAllConfig()); + } catch (RuntimeException e) { + throw new GravitinoRuntimeException(e, "Error creating datasource"); + } + configuredDriver(jdbcConfig); + Properties properties = getProperties(jdbcConfig); + try { + validateNativePoolProperties(properties); + } catch (Exception invalidProperties) { + // Identify a malformed native alias without printing unrelated settings or credentials. + for (String key : properties.stringPropertyNames()) { + Properties singleProperty = new Properties(); + singleProperty.setProperty(key, properties.getProperty(key)); + try { + validateNativePoolProperties(singleProperty); + } catch (Exception invalidProperty) { + throw new IllegalArgumentException( + "Invalid native JDBC connection-pool property '" + + key + + "': " + + invalidProperty.getMessage(), + invalidProperty); + } } - throw new GravitinoRuntimeException(exception, "Error creating datasource"); + throw new IllegalArgumentException( + "Invalid native JDBC connection-pool properties; check gravitino.bypass.* settings", + invalidProperties); + } + } + + private static void validateNativePoolProperties(Properties properties) throws Exception { + // initialSize and class-loading properties were rejected before this method. The factory + // therefore only parses an uninitialized configuration bean; no connections are opened. + try (BasicDataSource ignored = BasicDataSourceFactory.createDataSource(properties)) { + // Always close the temporary bean, including on the diagnostic path for individual keys. } } /** - * Returns whether the given throwable chain indicates a missing JDBC driver class. DBCP2 reports - * an absent driver as a {@link ClassNotFoundException} (sometimes wrapped in a {@link - * SQLException} whose message is "Cannot load JDBC driver class ..."), so both the exception - * chain and that message are checked. + * Creates the configured driver and verifies that it accepts the configured JDBC URL. * - * @param throwable the throwable thrown while creating the data source - * @return {@code true} if the failure is due to a driver class that cannot be loaded + * @param config the JDBC catalog configuration + * @return the configured JDBC driver + * @throws IllegalArgumentException if the driver is absent, invalid, or incompatible with the URL */ - private static boolean isDriverClassMissing(Throwable throwable) { - for (Throwable current = throwable; current != null; current = current.getCause()) { - if (current instanceof ClassNotFoundException || current instanceof NoClassDefFoundError) { - return true; - } - String message = current.getMessage(); - if (message != null && message.contains("Cannot load JDBC driver class")) { - return true; + public static Driver configuredDriver(JdbcConfig config) { + String name = config.getJdbcDriver(); + ClassLoader loader = Thread.currentThread().getContextClassLoader(); + if (loader == null) { + loader = DataSourceUtils.class.getClassLoader(); + } + try { + Class<?> type = Class.forName(name, false, loader); + if (!Driver.class.isAssignableFrom(type)) { + throw new IllegalArgumentException( + "jdbc-driver '" + name + "' must implement java.sql.Driver"); } - if (current.getCause() == current) { - break; + Driver driver = type.asSubclass(Driver.class).getDeclaredConstructor().newInstance(); + if (!driver.acceptsURL(config.getJdbcUrl())) { + throw new IllegalArgumentException("jdbc-driver '" + name + "' does not accept jdbc-url"); } + return driver; + } catch (ClassNotFoundException e) { + throw new IllegalArgumentException( + "JDBC driver class '" + + name + + "' was not found on the catalog classpath. Install the driver JAR in the catalog's libs directory.", + e); + } catch (ReflectiveOperationException | SQLException | LinkageError e) { + throw new IllegalArgumentException( + "Invalid jdbc-driver '" + name + "': driver must be installed and usable", e); } - return false; } private static DataSource createDBCPDataSource(JdbcConfig jdbcConfig) throws Exception { +<<<<<<< HEAD JdbcUrlUtils.validateJdbcConfig( jdbcConfig.getJdbcDriver(), jdbcConfig.getJdbcUrl(), jdbcConfig.getAllConfig()); +======= + // Keep DBCP's default Connection.isValid() validation unless a validationQuery is explicitly + // configured. DBCP caches a validation query as a prepared statement, and MySQL Connector/J + // switches back to the database captured at prepare time when executing it. That switch fails + // when the captured database is empty, so healthy connections would be discarded. +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) BasicDataSource basicDataSource = BasicDataSourceFactory.createDataSource(getProperties(jdbcConfig)); String jdbcUrl = jdbcConfig.getJdbcUrl(); basicDataSource.setUrl(jdbcUrl); String driverClassName = jdbcConfig.getJdbcDriver(); - // DBCP2 loads the driver lazily on the first connection, so a missing driver would otherwise - // only surface much later (and as an opaque error). Verify the driver class is on the catalog - // classpath now, so catalog creation fails fast with a clear, actionable message. Loaded - // without initialization; the H2 driver is already rejected above. - verifyDriverPresent(driverClassName); basicDataSource.setDriverClassName(driverClassName); String userName = jdbcConfig.getUsername(); basicDataSource.setUsername(userName); @@ -137,23 +208,6 @@ public class DataSourceUtils { return basicDataSource; } - /** - * Verifies that the configured JDBC driver class is loadable from the catalog classpath. The - * class is resolved without initialization using the catalog's context class loader (falling back - * to this class's loader), so an absent driver fails catalog creation immediately rather than on - * the first connection. - * - * @param driverClassName the fully qualified JDBC driver class name - * @throws ClassNotFoundException if the driver class is not on the catalog classpath - */ - private static void verifyDriverPresent(String driverClassName) throws ClassNotFoundException { - ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); - if (classLoader == null) { - classLoader = DataSourceUtils.class.getClassLoader(); - } - Class.forName(driverClassName, false, classLoader); - } - private static Properties getProperties(JdbcConfig jdbcConfig) { Properties properties = new Properties(); properties.putAll(jdbcConfig.getAllConfig()); diff --git a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogPropertiesMetadata.java b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogPropertiesMetadata.java index a77a6be771..4a4161880b 100644 --- a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogPropertiesMetadata.java +++ b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogPropertiesMetadata.java @@ -19,9 +19,13 @@ package org.apache.gravitino.catalog.jdbc; import com.google.common.collect.ImmutableMap; +import java.sql.Connection; +import java.sql.SQLException; import java.time.Instant; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Properties; import org.apache.commons.dbcp2.BasicDataSource; import org.apache.commons.lang3.reflect.FieldUtils; import org.apache.gravitino.Catalog; @@ -45,9 +49,98 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; +import org.sqlite.JDBC; public class TestJdbcCatalogPropertiesMetadata { + @Test + void testConfigurationValidationDoesNotDependOnBackendVerification() { + for (Map<String, String> invalid : + List.of( + Map.of("jdbc-driver", "java.lang.String"), + Map.of("jdbc-driver", "missing.Driver"), + Map.of("jdbc-url", "jdbc:postgresql://localhost/unused"), + Map.of("jdbc.pool.test-on-borrow", "maybe"), + Map.of("verify-on-create", "yes"), + Map.of("jdbc.pool.min-size", "0"), + Map.of("jdbc.pool.max-size", "-1"), + Map.of("jdbc.pool.min-size", "5", "jdbc.pool.max-size", "2"), + Map.of("jdbc.pool.connect-timeout-ms", "2147483648"))) { + Map<String, String> properties = poolWaitProperties(); + properties.put("verify-on-create", "false"); + properties.putAll(invalid); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> new TestJdbcCatalog().validateConfiguration(properties), + invalid.toString()); + } + Map<String, String> unavailable = poolWaitProperties(); + unavailable.put("jdbc-driver", NoConnectionDriver.class.getName()); + unavailable.put("jdbc-url", "jdbc:sqlite:unavailable"); + unavailable.put("verify-on-create", "false"); + // A static settings check must not attempt a connection to this unavailable backend. + Assertions.assertDoesNotThrow(() -> new TestJdbcCatalog().validateConfiguration(unavailable)); + } + + @Test + void testValidationUsesTheSameBypassPrecedenceAsOperations() { + Map<String, String> properties = poolWaitProperties(); + properties.put("verify-on-create", "false"); + properties.put("gravitino.bypass.jdbc.pool.min-size", "5"); + properties.put("jdbc.pool.max-size", "2"); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> new TestJdbcCatalog().validateConfiguration(properties)); + properties.put("jdbc.pool.min-size", "1"); + // Canonical JDBC settings override prefixed settings. + Assertions.assertDoesNotThrow(() -> new TestJdbcCatalog().validateConfiguration(properties)); + properties.put("url", "ignored property"); + Assertions.assertDoesNotThrow(() -> new TestJdbcCatalog().validateConfiguration(properties)); + Assertions.assertFalse( + buildCatalog(Map.of("gravitino.bypass.verify-on-create", "false")) + .shouldValidateOnCreate()); + Assertions.assertTrue( + buildCatalog( + Map.of("verify-on-create", "true", "gravitino.bypass.verify-on-create", "false")) + .shouldValidateOnCreate()); + properties.put("gravitino.bypass.initialSize", "1"); + Assertions.assertThrows( + GravitinoRuntimeException.class, + () -> new TestJdbcCatalog().validateConfiguration(properties)); + } + + @Test + void testNativePoolPropertySyntaxIsValidatedWithoutBackendVerification() { + for (String key : List.of("maxWaitMillis", "maxIdle")) { + Map<String, String> properties = poolWaitProperties(); + properties.put("verify-on-create", "false"); + properties.put("gravitino.bypass." + key, "not-a-number"); + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> new TestJdbcCatalog().validateConfiguration(properties)); + Assertions.assertTrue(error.getMessage().contains(key)); + } + } + + @Test + void testBooleanConnectionSettingsAreVisibleAndStrict() { + JdbcCatalogPropertiesMetadata metadata = new JdbcCatalogPropertiesMetadata(); + Assertions.assertFalse(metadata.isHiddenProperty(JdbcConfig.VERIFY_ON_CREATE.getKey())); + Assertions.assertFalse(metadata.isHiddenProperty(JdbcConfig.TEST_ON_BORROW.getKey())); + for (String value : new String[] {"yes", "1", "perhaps", ""}) { + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + buildCatalog(ImmutableMap.of(JdbcConfig.VERIFY_ON_CREATE.getKey(), value)) + .shouldValidateOnCreate()); + } + Assertions.assertTrue( + buildCatalog(ImmutableMap.of("verify-on-create", "TRUE")).shouldValidateOnCreate()); + Assertions.assertFalse( + buildCatalog(ImmutableMap.of("verify-on-create", "FALSE")).shouldValidateOnCreate()); + } + @Test public void testPoolSizePropertiesAreNotHidden() { JdbcCatalogPropertiesMetadata propertiesMetadata = new JdbcCatalogPropertiesMetadata(); @@ -111,7 +204,7 @@ public class TestJdbcCatalogPropertiesMetadata { properties.put(JdbcConfig.POOL_MAX_WAIT_MS.getKey(), value); try (JdbcCatalogOperations operations = poolWaitOperations()) { Assertions.assertThrows( - GravitinoRuntimeException.class, + IllegalArgumentException.class, () -> operations.initialize( properties, poolWaitCatalogInfo(properties), new TestJdbcCatalog())); @@ -236,6 +329,70 @@ public class TestJdbcCatalogPropertiesMetadata { .isHiddenProperty(JdbcConfig.CONNECT_TIMEOUT_MS.getKey())); } +<<<<<<< HEAD +======= + /** Verifies the actual catalog configuration path for drivers without isValid support. */ + @ParameterizedTest + @CsvSource({ + "none, true", + "validationQuery, true", + "gravitino.bypass.validationQuery, true", + "none, false" + }) + public void testValidationQueryBypassForUnsupportedDriver( + String queryProperty, boolean validateOnBorrow) { + ImmutableMap.Builder<String, String> builder = + ImmutableMap.<String, String>builder() + .put(JdbcConfig.JDBC_URL.getKey(), "jdbc:sqlite::memory:") + .put(JdbcConfig.JDBC_DRIVER.getKey(), UnsupportedValidationDriver.class.getName()) + .put(JdbcConfig.USERNAME.getKey(), "user") + .put(JdbcConfig.PASSWORD.getKey(), "password") + .put(JdbcConfig.TEST_ON_BORROW.getKey(), Boolean.toString(validateOnBorrow)); + if (!queryProperty.equals("none")) { + builder.put(queryProperty, "SELECT 42"); + } + Map<String, String> properties = builder.build(); + CatalogInfo info = + new CatalogInfo( + 1L, + "jdbc_catalog", + Catalog.Type.RELATIONAL, + "jdbc-test", + "test", + properties, + AuditInfo.builder().withCreator("test").withCreateTime(Instant.EPOCH).build(), + Namespace.of("metalake")); + try (JdbcCatalogOperations operations = + new JdbcCatalogOperations( + new SqliteExceptionConverter(), + new SqliteTypeConverter(), + new SqliteDatabaseOperations(":memory:"), + new SqliteTableOperations(), + new SqliteColumnDefaultValueConverter())) { + operations.initialize(properties, info, new TestJdbcCatalog()); + if (queryProperty.equals("gravitino.bypass.validationQuery")) { + // Repeated borrows must use SQL validation, never the unsupported isValid method. + for (int i = 0; i < 3; i++) { + Assertions.assertDoesNotThrow(operations::getDiverInfo); + } + } else { + ConnectionFailedException exception = + Assertions.assertThrows(ConnectionFailedException.class, operations::getDiverInfo); + Assertions.assertTrue(exception.getMessage().contains("isValid is not supported")); + } + } + } + + /** A driver that proves settings validation does not open a connection. */ + public static class NoConnectionDriver extends JDBC { + /** {@inheritDoc} */ + @Override + public Connection connect(String url, Properties properties) throws SQLException { + throw new AssertionError("Configuration validation must not open a connection"); + } + } + +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) private static class TestJdbcCatalog extends JdbcCatalog { @Override public String shortName() { diff --git a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java index ae9548e987..2c9ff63a9b 100644 --- a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java +++ b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceUrlValidation.java @@ -203,9 +203,9 @@ public class TestDataSourceUrlValidation { properties.put(JdbcConfig.USERNAME.getKey(), "test"); properties.put(JdbcConfig.PASSWORD.getKey(), "test"); - GravitinoRuntimeException gre = + IllegalArgumentException gre = Assertions.assertThrows( - GravitinoRuntimeException.class, () -> DataSourceUtils.createDataSource(properties)); + IllegalArgumentException.class, () -> DataSourceUtils.createDataSource(properties)); Assertions.assertTrue( gre.getMessage().contains(missingDriver), "message should name the missing driver: " + gre.getMessage()); diff --git a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceValidation.java b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceValidation.java new file mode 100644 index 0000000000..95e21e5088 --- /dev/null +++ b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceValidation.java @@ -0,0 +1,217 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.gravitino.catalog.jdbc.utils; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.sql.Statement; +import java.time.Duration; +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Properties; +import javax.annotation.Nullable; +import org.apache.commons.dbcp2.BasicDataSource; +import org.apache.gravitino.catalog.jdbc.config.JdbcConfig; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.sqlite.JDBC; +import org.sqlite.SQLiteConnection; + +/** Tests default driver validation and explicitly configured SQL validation. */ +public class TestDataSourceValidation { + + @Test + void testInvalidConfigurationFailsBeforeConnection() { + for (Map<String, String> invalid : + List.of( + Map.of("jdbc-driver", "java.lang.String"), + Map.of("jdbc-driver", "missing.Driver"), + Map.of("jdbc-url", "jdbc:postgresql://localhost/db"), + Map.of("verify-on-create", "yes"), + Map.of("jdbc.pool.test-on-borrow", "maybe"), + Map.of("jdbc.pool.min-size", "0"), + Map.of("jdbc.pool.max-size", "-1"), + Map.of("jdbc.pool.min-size", "5", "jdbc.pool.max-size", "2"), + Map.of("jdbc.pool.max-wait-ms", "0"), + Map.of("jdbc.pool.connect-timeout-ms", "2147483648"))) { + IllegalArgumentException exception = + Assertions.assertThrows(IllegalArgumentException.class, () -> createDataSource(invalid)); + Assertions.assertTrue( + exception.getMessage().toLowerCase(Locale.ROOT).contains("jdbc") + || exception.getMessage().contains("verify-on-create")); + } + } + + /** Verifies default validation preserves a live physical SQLite connection across borrows. */ + @Test + public void testReuseWithDefaultValidation() throws SQLException { + try (BasicDataSource dataSource = createDataSource(Map.of())) { + Assertions.assertNull(dataSource.getValidationQuery()); + Assertions.assertTrue(dataSource.getTestOnBorrow()); + try (Connection connection = dataSource.getConnection(); + Statement statement = connection.createStatement()) { + statement.execute("CREATE TABLE pool_marker (id INTEGER)"); + statement.execute("INSERT INTO pool_marker VALUES (1)"); + } + // Each physical connection has its own in-memory database, so the marker proves reuse. + for (int i = 0; i < 3; i++) { + try (Connection connection = dataSource.getConnection(); + Statement statement = connection.createStatement(); + ResultSet result = statement.executeQuery("SELECT id FROM pool_marker")) { + Assertions.assertTrue(result.next()); + Assertions.assertEquals(1, result.getInt(1)); + } + } + } + } + + /** Verifies default validation replaces a closed physical connection. */ + @Test + public void testClosedConnectionIsReplaced() throws SQLException { + try (BasicDataSource dataSource = createDataSource(Map.of())) { + SQLiteConnection oldConnection; + try (Connection connection = dataSource.getConnection()) { + oldConnection = connection.unwrap(SQLiteConnection.class); + } + oldConnection.close(); + try (Connection connection = dataSource.getConnection()) { + Assertions.assertNotSame(oldConnection, connection.unwrap(SQLiteConnection.class)); + Assertions.assertTrue(connection.isValid(1)); + } + } + } + + /** Verifies a supplied validation query is executed instead of being overwritten. */ + @ParameterizedTest + @ValueSource(strings = {"SELECT 42", "SELECT * FROM missing_validation_table"}) + public void testExplicitValidationQuery(String query) throws SQLException { + try (BasicDataSource dataSource = createDataSource(Map.of("validationQuery", query))) { + Assertions.assertEquals(query, dataSource.getValidationQuery()); + if (query.contains("missing_validation_table")) { + Assertions.assertThrows( + SQLException.class, + () -> { + try (Connection connection = dataSource.getConnection()) { + Assertions.assertFalse(connection.isClosed()); + } + }); + } else { + try (Connection connection = dataSource.getConnection()) { + Assertions.assertTrue(connection.isValid(1)); + } + } + } + } + + /** Verifies validation settings remain configurable when using default driver validation. */ + @Test + public void testValidationSettingsPreserved() throws SQLException { + try (BasicDataSource dataSource = + createDataSource( + Map.of("validationQueryTimeout", "5", JdbcConfig.TEST_ON_BORROW.getKey(), "false"))) { + Assertions.assertNull(dataSource.getValidationQuery()); + Assertions.assertEquals( + Duration.ofSeconds(5), dataSource.getValidationQueryTimeoutDuration()); + Assertions.assertFalse(dataSource.getTestOnBorrow()); + try (Connection connection = dataSource.getConnection()) { + Assertions.assertTrue(connection.isValid(1)); + } + } + } + + /** + * Verifies that for an unknown driver family (SQLite) no connect/socket timeout connection + * property is injected and the datasource still builds and borrows a connection. This guards the + * "unknown driver -> no-op" contract of {@link JdbcConnectionTimeouts}: SQLite does not + * understand {@code connectTimeout}/{@code socketTimeout}, so injecting them would make the + * connection fail. + */ + @Test + public void testUnknownDriverInjectsNoConnectionTimeout() throws SQLException { + // JdbcConnectionTimeouts emits nothing for the SQLite driver family. + Map<String, String> props = new HashMap<>(); + props.put(JdbcConfig.JDBC_DRIVER.getKey(), "org.sqlite.JDBC"); + props.put(JdbcConfig.JDBC_URL.getKey(), "jdbc:sqlite::memory:"); + Assertions.assertTrue( + JdbcConnectionTimeouts.connectionProperties( + new JdbcConfig(props), props.get(JdbcConfig.JDBC_URL.getKey())) + .isEmpty()); + + // And the datasource built from it still borrows a working connection (no leak, no failure). + try (BasicDataSource dataSource = createDataSource(Map.of())) { + try (Connection connection = dataSource.getConnection()) { + Assertions.assertTrue(connection.isValid(1)); + } + } + } + + /** A driver fixture whose connections do not implement JDBC connection validation. */ + public static class UnsupportedValidationDriver extends JDBC { + /** + * Creates a SQLite connection that rejects calls to {@code isValid()}. + * + * @param url the JDBC URL + * @param properties the connection properties + * @return a connection without JDBC validation support + * @throws SQLException if opening the connection fails + */ + @Override + @Nullable + public Connection connect(String url, Properties properties) throws SQLException { + Connection delegate = super.connect(url, properties); + if (delegate == null) { + return null; + } + return (Connection) + Proxy.newProxyInstance( + getClass().getClassLoader(), + new Class<?>[] {Connection.class}, + (proxy, method, args) -> { + if (method.getName().equals("isValid")) { + throw new SQLFeatureNotSupportedException("isValid is not supported"); + } + try { + return method.invoke(delegate, args); + } catch (InvocationTargetException exception) { + throw exception.getCause(); + } + }); + } + } + + private static BasicDataSource createDataSource(Map<String, String> overrides) { + Map<String, String> properties = new HashMap<>(); + properties.put(JdbcConfig.JDBC_DRIVER.getKey(), "org.sqlite.JDBC"); + properties.put(JdbcConfig.JDBC_URL.getKey(), "jdbc:sqlite::memory:"); + properties.put(JdbcConfig.USERNAME.getKey(), "test"); + properties.put(JdbcConfig.PASSWORD.getKey(), "test"); + properties.put(JdbcConfig.POOL_MAX_SIZE.getKey(), "1"); + properties.put(JdbcConfig.POOL_MIN_SIZE.getKey(), "1"); + properties.putAll(overrides); + return (BasicDataSource) DataSourceUtils.createDataSource(properties); + } +} diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java index b04ac0dad0..3539039058 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSQLCatalogOperations.java @@ -19,7 +19,6 @@ package org.apache.gravitino.catalog.postgresql; import java.sql.Driver; -import java.sql.DriverManager; import java.sql.SQLException; import org.apache.gravitino.catalog.jdbc.JdbcCatalogOperations; import org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter; @@ -46,6 +45,6 @@ public class PostgreSQLCatalogOperations extends JdbcCatalogOperations { @Override protected Driver getDriver() throws SQLException { - return DriverManager.getDriver("jdbc:postgresql://dummy_address:12345/"); + return super.getDriver(); } } diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSqlCatalog.java b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSqlCatalog.java index ca8ec349ef..d7be0ab429 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSqlCatalog.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSqlCatalog.java @@ -42,8 +42,8 @@ public class PostgreSqlCatalog extends JdbcCatalog { /** {@inheritDoc} */ @Override - public JdbcCatalog withCatalogConf(Map<String, String> conf) { - return super.withCatalogConf(resolveJdbcDatabase(conf, PostgreSqlCatalog::databaseFromUrl)); + protected Map<String, String> prepareConfiguration(Map<String, String> conf) { + return resolveJdbcDatabase(conf, PostgreSqlCatalog::databaseFromUrl); } @Override diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/converter/PostgreSqlColumnDefaultValueConverter.java b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/converter/PostgreSqlColumnDefaultValueConverter.java index 65e16256a4..b115aa6fad 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/converter/PostgreSqlColumnDefaultValueConverter.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/converter/PostgreSqlColumnDefaultValueConverter.java @@ -27,6 +27,7 @@ import java.time.LocalDateTime; import java.time.LocalTime; import java.util.regex.Matcher; import java.util.regex.Pattern; +import javax.annotation.Nullable; import org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter; import org.apache.gravitino.catalog.jdbc.converter.JdbcTypeConverter; import org.apache.gravitino.rel.expressions.Expression; @@ -73,6 +74,17 @@ public class PostgreSqlColumnDefaultValueConverter extends JdbcColumnDefaultValu } } + /** {@inheritDoc} */ + @Override + @Nullable + public String fromGravitino(Expression defaultValue) { + String sql = super.fromGravitino(defaultValue); + if (sql != null && sql.indexOf('\0') >= 0) { + throw new IllegalArgumentException("PostgreSQL column default must not contain U+0000 (NUL)"); + } + return sql; + } + /** * Doubles single quotes only. With {@code standard_conforming_strings} on, the default since * PostgreSQL 9.1, a backslash in a string literal is an ordinary character. diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/converter/PostgreSqlExceptionConverter.java b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/converter/PostgreSqlExceptionConverter.java index 7934308a27..e9c721fcab 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/converter/PostgreSqlExceptionConverter.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/converter/PostgreSqlExceptionConverter.java @@ -21,6 +21,7 @@ package org.apache.gravitino.catalog.postgresql.converter; import java.sql.SQLException; import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter; import org.apache.gravitino.exceptions.ConnectionFailedException; +import org.apache.gravitino.exceptions.ForbiddenException; import org.apache.gravitino.exceptions.GravitinoRuntimeException; import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NoSuchTableException; @@ -35,6 +36,7 @@ public class PostgreSqlExceptionConverter extends JdbcExceptionConverter { private static final String INVALID_SCHEMA_NAME = "3D000"; private static final String INVALID_SCHEMA = "3F000"; private static final String UNDEFINED_TABLE = "42P01"; + private static final String INSUFFICIENT_PRIVILEGE = "42501"; /** * SQLSTATE '08' is the class code of connection exceptions See <a @@ -43,34 +45,35 @@ public class PostgreSqlExceptionConverter extends JdbcExceptionConverter { */ private static final String CONNECTION_EXCEPTION = "08"; - @SuppressWarnings("FormatStringAnnotation") @Override public GravitinoRuntimeException toGravitinoException(SQLException se) { if (null != se.getSQLState()) { switch (se.getSQLState()) { + case INSUFFICIENT_PRIVILEGE: + return new ForbiddenException(se, "%s", se.getMessage()); case DUPLICATE_DATABASE: case DUPLICATE_SCHEMA: - return new SchemaAlreadyExistsException(se.getMessage(), se); + return new SchemaAlreadyExistsException(se, "%s", se.getMessage()); case DUPLICATE_TABLE: - return new TableAlreadyExistsException(se.getMessage(), se); + return new TableAlreadyExistsException(se, "%s", se.getMessage()); case INVALID_SCHEMA_NAME: case INVALID_SCHEMA: - return new NoSuchSchemaException(se.getMessage(), se); + return new NoSuchSchemaException(se, "%s", se.getMessage()); case UNDEFINED_TABLE: - return new NoSuchTableException(se.getMessage(), se); + return new NoSuchTableException(se, "%s", se.getMessage()); default: { if (se.getSQLState().startsWith(CONNECTION_EXCEPTION)) { - return new ConnectionFailedException(se.getMessage(), se); + return new ConnectionFailedException(se, "%s", se.getMessage()); } - return new GravitinoRuntimeException(se.getMessage(), se); + return new GravitinoRuntimeException(se, "%s", se.getMessage()); } } } else { if (se.getMessage() != null && se.getMessage().contains("password authentication failed")) { - return new ConnectionFailedException(se.getMessage(), se); + return new ConnectionFailedException(se, "%s", se.getMessage()); } - return new GravitinoRuntimeException(se.getMessage(), se); + return new GravitinoRuntimeException(se, "%s", se.getMessage()); } } } diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java index 21c0f4db2a..1ad6f98ce9 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java @@ -25,6 +25,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Lists; import java.sql.Connection; import java.sql.DatabaseMetaData; +import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; import java.util.ArrayList; @@ -48,6 +49,11 @@ import org.apache.gravitino.catalog.jdbc.converter.JdbcTypeConverter; import org.apache.gravitino.catalog.jdbc.operation.DatabaseOperation; import org.apache.gravitino.catalog.jdbc.operation.JdbcTableOperations; import org.apache.gravitino.catalog.jdbc.operation.RequireDatabaseOperation; +<<<<<<< HEAD +======= +import org.apache.gravitino.catalog.jdbc.utils.JdbcConnectorUtils; +import org.apache.gravitino.exceptions.ForbiddenException; +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) import org.apache.gravitino.exceptions.NoSuchColumnException; import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NoSuchTableException; @@ -56,6 +62,7 @@ import org.apache.gravitino.rel.TableChange; import org.apache.gravitino.rel.expressions.distributions.Distribution; import org.apache.gravitino.rel.expressions.distributions.Distributions; import org.apache.gravitino.rel.expressions.literals.Literals; +import org.apache.gravitino.rel.expressions.sorts.SortOrder; import org.apache.gravitino.rel.expressions.transforms.Transform; import org.apache.gravitino.rel.indexes.Index; import org.apache.gravitino.rel.types.Types; @@ -98,6 +105,47 @@ public class PostgreSqlTableOperations extends JdbcTableOperations this.schemaOperations = (PostgreSqlSchemaOperations) databaseOperation; } + /** {@inheritDoc} */ + @Override + public void create( + String schema, + String tableName, + JdbcColumn[] columns, + @Nullable String comment, + Map<String, String> properties, + @Nullable Transform[] partitioning, + Distribution distribution, + Index[] indexes, + @Nullable SortOrder[] sortOrders) { + try { + super.create( + schema, + tableName, + columns, + comment, + properties, + partitioning, + distribution, + indexes, + sortOrders); + } catch (NoSuchSchemaException e) { + // PostgreSQL removes unusable schemas from search_path. An unqualified CREATE can therefore + // report 3F000 even when the requested schema exists but the source role has no USAGE. + boolean lacksUsage; + try { + lacksUsage = schemaLacksUsage(schema); + } catch (RuntimeException diagnosisFailure) { + diagnosisFailure.addSuppressed(e); + throw diagnosisFailure; + } + if (lacksUsage) { + throw new ForbiddenException( + e, "Insufficient source privileges for schema %s: USAGE is required", schema); + } + throw e; + } + } + @Override public List<String> listTables(String schemaName) throws NoSuchSchemaException { try (Connection connection = getConnection(schemaName)) { @@ -207,7 +255,7 @@ public class PostgreSqlTableOperations extends JdbcTableOperations .append(tableName) .append(PG_QUOTE) .append(IS) - .append(escapeSqlLiteral(comment, '\'')) + .append(escapeSqlLiteral(validateText(comment, "table comment"), '\'')) .append("';"); } Arrays.stream(columns) @@ -225,7 +273,11 @@ public class PostgreSqlTableOperations extends JdbcTableOperations .append(jdbcColumn.name()) .append(PG_QUOTE) .append(IS) - .append(escapeSqlLiteral(jdbcColumn.comment(), '\'')) + .append( + escapeSqlLiteral( + validateText( + jdbcColumn.comment(), "comment for column " + jdbcColumn.name()), + '\'')) .append("';")); // Return the generated SQL statement @@ -504,7 +556,8 @@ public class PostgreSqlTableOperations extends JdbcTableOperations private String updateCommentDefinition( TableChange.UpdateComment updateComment, JdbcTable jdbcTable) { - String newComment = StringUtils.defaultString(updateComment.getNewComment()); + String newComment = + StringUtils.defaultString(validateText(updateComment.getNewComment(), "table comment")); if (null == StringIdentifier.fromComment(newComment)) { // Detect and add Gravitino id. if (StringUtils.isNotEmpty(jdbcTable.comment())) { @@ -519,7 +572,7 @@ public class PostgreSqlTableOperations extends JdbcTableOperations + jdbcTable.name() + PG_QUOTE + IS - + escapeSqlLiteral(newComment, '\'') + + escapeSqlLiteral(validateText(newComment, "table comment"), '\'') + "';"; } @@ -705,7 +758,8 @@ public class PostgreSqlTableOperations extends JdbcTableOperations + col + PG_QUOTE + IS - + escapeSqlLiteral(addColumn.getComment(), '\'') + + escapeSqlLiteral( + validateText(addColumn.getComment(), "comment for column " + col), '\'') + "';"); } return result; @@ -727,7 +781,7 @@ public class PostgreSqlTableOperations extends JdbcTableOperations + col + PG_QUOTE + IS - + escapeSqlLiteral(newComment, '\'') + + escapeSqlLiteral(validateText(newComment, "comment for column " + col), '\'') + "';"; } @@ -779,4 +833,53 @@ public class PostgreSqlTableOperations extends JdbcTableOperations return null; } } +<<<<<<< HEAD +======= + + private boolean schemaLacksUsage(String schema) { + try (Connection connection = dataSource.getConnection(); + PreparedStatement statement = + connection.prepareStatement( + "SELECT pg_catalog.has_schema_privilege(n.oid, 'USAGE') " + + "FROM pg_catalog.pg_namespace n WHERE n.nspname = ?")) { + statement.setString(1, schema); + try (ResultSet rows = statement.executeQuery()) { + return rows.next() && !rows.getBoolean(1); + } + } catch (SQLException e) { + throw exceptionMapper.toGravitinoException(e); + } + } + + @Nullable + private static String validateText(@Nullable String text, String field) { + if (text != null && text.indexOf('\0') >= 0) { + throw new IllegalArgumentException("PostgreSQL " + field + " must not contain U+0000 (NUL)"); + } + return text; + } + + private static String quotedIdentifier(String name) { + return PG_QUOTE + name.replace(PG_QUOTE, PG_QUOTE + PG_QUOTE) + PG_QUOTE; + } + + private static void validateIndexName(@Nullable String name) { + if (name != null && name.getBytes(StandardCharsets.UTF_8).length > 63) { + throw new IllegalArgumentException( + "PostgreSQL index name must not exceed 63 UTF-8 bytes: " + name); + } + } + + @Nullable + private static String metadataPattern(DatabaseMetaData metadata, @Nullable String name) + throws SQLException { + if (name == null) { + return null; + } + String escape = metadata.getSearchStringEscape(); + return name.replace(escape, escape + escape) + .replace("%", escape + "%") + .replace("_", escape + "_"); + } +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) } diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/TestPostgreSqlCatalogConfiguration.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/TestPostgreSqlCatalogConfiguration.java index ef1e83047e..160f2d5ebe 100644 --- a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/TestPostgreSqlCatalogConfiguration.java +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/TestPostgreSqlCatalogConfiguration.java @@ -36,6 +36,34 @@ import org.junit.jupiter.params.provider.ValueSource; /** Tests database resolution before catalog operations are initialized. */ class TestPostgreSqlCatalogConfiguration { + @Test + void testProposedConfigurationRejectsDatabaseMismatchBeforeInitialization() { + Map<String, String> configuration = + new HashMap<>( + Map.of( + "jdbc-url", + "jdbc:postgresql://localhost/demo", + "jdbc-database", + "other", + "jdbc-driver", + "org.postgresql.Driver", + "jdbc-user", + "unused", + "jdbc-password", + "unused", + "verify-on-create", + "false")); + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> new PostgreSqlCatalog().validateConfiguration(configuration)); + Assertions.assertTrue(error.getMessage().contains("jdbc-database must match")); + Assertions.assertEquals("other", configuration.get("jdbc-database")); + configuration.put("jdbc-database", "demo"); + Assertions.assertDoesNotThrow( + () -> new PostgreSqlCatalog().validateConfiguration(configuration)); + } + @ParameterizedTest @CsvSource( value = { diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/converter/TestPostgreSqlColumnDefaultValueConverter.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/converter/TestPostgreSqlColumnDefaultValueConverter.java index c66b17fa35..ec1825323a 100644 --- a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/converter/TestPostgreSqlColumnDefaultValueConverter.java +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/converter/TestPostgreSqlColumnDefaultValueConverter.java @@ -28,6 +28,20 @@ public class TestPostgreSqlColumnDefaultValueConverter { private final PostgreSqlColumnDefaultValueConverter converter = new PostgreSqlColumnDefaultValueConverter(); + @Test + void testNulStringDefaultIsRejectedBeforeDdl() { + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> converter.fromGravitino(Literals.stringLiteral("before\0after"))); + Assertions.assertTrue(error.getMessage().contains("default")); + Assertions.assertTrue(error.getMessage().contains("U+0000")); + Assertions.assertEquals( + "'before\\u0000after'", + converter.fromGravitino(Literals.stringLiteral("before\\u0000after"))); + Assertions.assertEquals("NULL", converter.fromGravitino(Literals.NULL)); + } + /** Verifies long quoted text defaults can be loaded without recursive regex stack growth. */ @Test public void testLongQuotedTextLiteralIsUnescaped() { diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/converter/TestPostgreSqlExceptionConverter.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/converter/TestPostgreSqlExceptionConverter.java new file mode 100644 index 0000000000..e45514e637 --- /dev/null +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/converter/TestPostgreSqlExceptionConverter.java @@ -0,0 +1,79 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.gravitino.catalog.postgresql.converter; + +import java.sql.SQLException; +import org.apache.gravitino.exceptions.ConnectionFailedException; +import org.apache.gravitino.exceptions.ForbiddenException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullSource; +import org.junit.jupiter.params.provider.ValueSource; + +/** PostgreSQL source permission errors remain distinct from missing objects and outages. */ +public class TestPostgreSqlExceptionConverter { + @SuppressWarnings("OrphanedFormatString") // Source messages are literal, not format templates. + @ParameterizedTest + @NullSource + @ValueSource( + strings = { + "42501", "42P04", "42P06", "42P07", "3D000", "3F000", "42P01", "08006", "08P01", "XX000" + }) + void testSqlFailuresPreserveCauseAndLiteralMessage(String state) { + SQLException failure = new SQLException("source failure containing literal %s", state); + Exception error = new PostgreSqlExceptionConverter().toGravitinoException(failure); + Assertions.assertSame(failure, error.getCause()); + Assertions.assertEquals(failure.getMessage(), error.getMessage()); + } + + @SuppressWarnings("OrphanedFormatString") // Source messages are literal, not format templates. + @Test + void testAuthenticationFailureWithoutSqlStatePreservesCause() { + SQLException failure = new SQLException("password authentication failed for role %s"); + Exception error = new PostgreSqlExceptionConverter().toGravitinoException(failure); + Assertions.assertInstanceOf(ConnectionFailedException.class, error); + Assertions.assertSame(failure, error.getCause()); + Assertions.assertEquals(failure.getMessage(), error.getMessage()); + } + + @Test + void testInsufficientPrivilegePreservesCauseAndReason() { + SQLException failure = new SQLException("permission denied for schema restricted", "42501"); + Exception error = new PostgreSqlExceptionConverter().toGravitinoException(failure); + Assertions.assertInstanceOf(ForbiddenException.class, error); + Assertions.assertSame(failure, error.getCause()); + Assertions.assertEquals(failure.getMessage(), error.getMessage()); + } + + @Test + void testMissingSchemaAndConnectionFailuresRemainDistinct() { + PostgreSqlExceptionConverter converter = new PostgreSqlExceptionConverter(); + Assertions.assertInstanceOf( + NoSuchSchemaException.class, + converter.toGravitinoException(new SQLException("schema missing", "3F000"))); + for (String state : new String[] {"08006", "08P01"}) { + Assertions.assertInstanceOf( + ConnectionFailedException.class, + converter.toGravitinoException( + new SQLException("connection or protocol failure", state))); + } + } +} diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableCreatePrivileges.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableCreatePrivileges.java new file mode 100644 index 0000000000..304afcd1f9 --- /dev/null +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableCreatePrivileges.java @@ -0,0 +1,165 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.gravitino.catalog.postgresql.operation; + +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.Map; +import javax.sql.DataSource; +import org.apache.gravitino.catalog.jdbc.JdbcColumn; +import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlColumnDefaultValueConverter; +import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlExceptionConverter; +import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlTypeConverter; +import org.apache.gravitino.exceptions.ConnectionFailedException; +import org.apache.gravitino.exceptions.ForbiddenException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; +import org.apache.gravitino.rel.expressions.distributions.Distributions; +import org.apache.gravitino.rel.expressions.transforms.Transforms; +import org.apache.gravitino.rel.indexes.Indexes; +import org.apache.gravitino.rel.types.Types; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** A hidden search-path entry must not make an existing inaccessible schema look absent. */ +public class TestPostgreSqlTableCreatePrivileges { + private DataSource source; + private Connection createConnection; + private Connection probeConnection; + private Statement createStatement; + private PreparedStatement probeStatement; + private ResultSet rows; + private PostgreSqlTableOperations operations; + + @BeforeEach + void setUp() throws Exception { + source = mock(DataSource.class); + createConnection = mock(Connection.class); + probeConnection = mock(Connection.class); + createStatement = mock(Statement.class); + probeStatement = mock(PreparedStatement.class); + rows = mock(ResultSet.class); + when(source.getConnection()).thenReturn(createConnection, probeConnection); + when(createConnection.createStatement()).thenReturn(createStatement); + when(createStatement.executeUpdate(anyString())) + .thenThrow(new SQLException("no schema has been selected to create in", "3F000")); + when(probeConnection.prepareStatement(anyString())).thenReturn(probeStatement); + when(probeStatement.executeQuery()).thenReturn(rows); + operations = new PostgreSqlTableOperations(); + operations.initialize( + source, + new PostgreSqlExceptionConverter(), + new PostgreSqlTypeConverter(), + new PostgreSqlColumnDefaultValueConverter(), + Map.of("jdbc-database", "database")); + } + + @Test + void testExistingSchemaWithoutUsageIsForbidden() throws Exception { + when(rows.next()).thenReturn(true); + when(rows.getBoolean(1)).thenReturn(false); + ForbiddenException error = Assertions.assertThrows(ForbiddenException.class, this::create); + Assertions.assertTrue(error.getMessage().contains("restricted")); + Assertions.assertTrue(error.getMessage().contains("USAGE")); + verify(probeStatement).setString(1, "restricted"); + verify(createConnection).close(); + verify(probeConnection).close(); + verify(probeStatement).close(); + verify(rows).close(); + } + + @Test + void testMissingSchemaRemainsMissing() throws Exception { + when(rows.next()).thenReturn(false); + Assertions.assertThrows(NoSuchSchemaException.class, this::create); + verify(probeConnection).close(); + } + + @Test + void testExistingUsableSchemaDoesNotInventPermissionFailure() throws Exception { + when(rows.next()).thenReturn(true); + when(rows.getBoolean(1)).thenReturn(true); + Assertions.assertThrows(NoSuchSchemaException.class, this::create); + verify(probeConnection).close(); + } + + @Test + void testSuccessfulCreateDoesNotBorrowForPermissionProbe() throws Exception { + doReturn(0).when(createStatement).executeUpdate(anyString()); + Assertions.assertDoesNotThrow(this::create); + verify(source, times(1)).getConnection(); + verify(createConnection).close(); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void testProbeFailurePreservesCreateFailure(boolean queryFailure) throws Exception { + SQLException probeFailure = new SQLException("connection lost", "08006"); + if (queryFailure) { + when(probeStatement.executeQuery()).thenThrow(probeFailure); + } else { + when(source.getConnection()).thenReturn(createConnection).thenThrow(probeFailure); + } + ConnectionFailedException error = + Assertions.assertThrows(ConnectionFailedException.class, this::create); + Assertions.assertSame(probeFailure, error.getCause()); + Assertions.assertEquals(1, error.getSuppressed().length); + NoSuchSchemaException createFailure = + Assertions.assertInstanceOf(NoSuchSchemaException.class, error.getSuppressed()[0]); + SQLException createCause = + Assertions.assertInstanceOf(SQLException.class, createFailure.getCause()); + Assertions.assertEquals("3F000", createCause.getSQLState()); + Assertions.assertEquals("no schema has been selected to create in", createCause.getMessage()); + verify(createConnection).close(); + if (queryFailure) { + verify(probeConnection).close(); + verify(probeStatement).close(); + } + } + + private void create() { + operations.create( + "restricted", + "t", + new JdbcColumn[] { + JdbcColumn.builder() + .withName("id") + .withType(Types.IntegerType.get()) + .withNullable(true) + .build() + }, + null, + Map.of(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + Indexes.EMPTY_INDEXES); + } +} diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java index 6dfa17fe6e..1f2a234565 100644 --- a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java @@ -29,6 +29,7 @@ import java.util.List; import java.util.Map; import java.util.stream.Collectors; import javax.sql.DataSource; +import org.apache.commons.dbcp2.BasicDataSource; import org.apache.commons.lang3.StringUtils; import org.apache.gravitino.catalog.jdbc.JdbcColumn; import org.apache.gravitino.catalog.jdbc.JdbcTable; @@ -37,7 +38,9 @@ import org.apache.gravitino.catalog.jdbc.utils.DataSourceUtils; import org.apache.gravitino.catalog.jdbc.utils.JdbcConnectorUtils; import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlColumnDefaultValueConverter; import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlTypeConverter; +import org.apache.gravitino.exceptions.ForbiddenException; import org.apache.gravitino.exceptions.GravitinoRuntimeException; +import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NoSuchTableException; import org.apache.gravitino.rel.Column; import org.apache.gravitino.rel.TableChange; @@ -61,6 +64,154 @@ public class TestPostgreSqlTableOperations extends TestPostgreSql { private static Type VARCHAR = Types.VarCharType.of(255); private static Type INT = Types.IntegerType.get(); + @Test + void testLeastPrivilegeCreateDistinguishesMissingSchemaFromForbidden() throws SQLException { + String role = RandomNameUtils.genRandomName("limited_role"); + String schema = RandomNameUtils.genRandomName("restricted_schema"); + String table = RandomNameUtils.genRandomName("permission_table"); + try (Connection admin = DATA_SOURCE.getConnection()) { + JdbcConnectorUtils.executeUpdate( + admin, "CREATE ROLE " + role + " LOGIN PASSWORD 'test-only-password'"); + JdbcConnectorUtils.executeUpdate(admin, "CREATE SCHEMA " + schema); + try { + JdbcConnectorUtils.executeUpdate(admin, "REVOKE ALL ON SCHEMA " + schema + " FROM PUBLIC"); + Map<String, String> properties = new HashMap<>(); + properties.put("jdbc-driver", CONTAINER.getDriverClassName()); + properties.put("jdbc-url", CONTAINER.getJdbcUrl()); + properties.put("jdbc-database", admin.getCatalog()); + properties.put("jdbc-user", role); + properties.put("jdbc-password", "test-only-password"); + properties.put("jdbc.pool.max-size", "1"); + properties.put("jdbc.pool.min-size", "1"); + try (BasicDataSource limitedSource = + (BasicDataSource) DataSourceUtils.createDataSource(properties)) { + PostgreSqlTableOperations limited = new PostgreSqlTableOperations(); + limited.initialize( + limitedSource, + JDBC_EXCEPTION_CONVERTER, + new PostgreSqlTypeConverter(), + new PostgreSqlColumnDefaultValueConverter(), + properties); + JdbcColumn[] columns = { + JdbcColumn.builder().withName("id").withType(INT).withNullable(true).build() + }; + Assertions.assertThrows( + ForbiddenException.class, + () -> + limited.create( + schema, + table, + columns, + null, + Map.of(), + null, + Distributions.NONE, + Indexes.EMPTY_INDEXES)); + JdbcConnectorUtils.executeUpdate( + admin, "GRANT USAGE ON SCHEMA " + schema + " TO " + role); + Assertions.assertThrows( + ForbiddenException.class, + () -> + limited.create( + schema, + table, + columns, + null, + Map.of(), + null, + Distributions.NONE, + Indexes.EMPTY_INDEXES)); + JdbcConnectorUtils.executeUpdate( + admin, "GRANT CREATE ON SCHEMA " + schema + " TO " + role); + Assertions.assertDoesNotThrow( + () -> + limited.create( + schema, + table, + columns, + null, + Map.of(), + null, + Distributions.NONE, + Indexes.EMPTY_INDEXES)); + Assertions.assertEquals(table, TABLE_OPERATIONS.load(schema, table).name()); + String absent = RandomNameUtils.genRandomName("missing_schema"); + Assertions.assertThrows( + NoSuchSchemaException.class, + () -> + limited.create( + absent, + table, + columns, + null, + Map.of(), + null, + Distributions.NONE, + Indexes.EMPTY_INDEXES)); + } + } finally { + JdbcConnectorUtils.executeUpdate(admin, "DROP SCHEMA IF EXISTS " + schema + " CASCADE"); + JdbcConnectorUtils.executeUpdate(admin, "DROP ROLE IF EXISTS " + role); + } + } + } + + @Test + void testNulValidationPreventsCreateAndPartialAlterDdl() { + String table = RandomNameUtils.genRandomName("nul_validation"); + String invalidTable = RandomNameUtils.genRandomName("nul_create"); + JdbcColumn[] columns = { + JdbcColumn.builder().withName("col1").withType(VARCHAR).withNullable(true).build() + }; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + TABLE_OPERATIONS.create( + TEST_DB_NAME, + invalidTable, + columns, + "bad\0comment", + Map.of(), + null, + Distributions.NONE, + Indexes.EMPTY_INDEXES)); + Assertions.assertFalse(TABLE_OPERATIONS.listTables(TEST_DB_NAME).contains(invalidTable)); + TABLE_OPERATIONS.create( + TEST_DB_NAME, + table, + columns, + "original", + Map.of(), + null, + Distributions.NONE, + Indexes.EMPTY_INDEXES); + try { + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + TABLE_OPERATIONS.alterTable( + TEST_DB_NAME, + table, + TableChange.addColumn(new String[] {"must_not_exist"}, INT), + TableChange.updateComment("bad\0comment"))); + JdbcTable loaded = TABLE_OPERATIONS.load(TEST_DB_NAME, table); + Assertions.assertEquals(1, loaded.columns().length); + Assertions.assertEquals("original", loaded.comment()); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + TABLE_OPERATIONS.alterTable( + TEST_DB_NAME, + table, + TableChange.updateColumnDefaultValue( + new String[] {"col1"}, Literals.stringLiteral("bad\0default")))); + Assertions.assertEquals( + Literals.NULL, TABLE_OPERATIONS.load(TEST_DB_NAME, table).columns()[0].defaultValue()); + } finally { + TABLE_OPERATIONS.drop(TEST_DB_NAME, table); + } + } + @Test public void testOperationTable() { String tableName = RandomNameUtils.genRandomName("op_table_"); diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java index 00958b7e93..7e312665c7 100644 --- a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java @@ -23,6 +23,7 @@ import org.apache.gravitino.catalog.jdbc.JdbcColumn; import org.apache.gravitino.catalog.jdbc.JdbcTable; import org.apache.gravitino.catalog.jdbc.converter.JdbcColumnDefaultValueConverter; import org.apache.gravitino.catalog.jdbc.converter.JdbcExceptionConverter; +import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlColumnDefaultValueConverter; import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlTypeConverter; import org.apache.gravitino.dto.requests.TableUpdateRequest; import org.apache.gravitino.rel.TableChange; @@ -45,6 +46,11 @@ public class TestPostgreSqlTableOperationsSqlGeneration { super.columnDefaultValueConverter = new JdbcColumnDefaultValueConverter(); } + private TestablePostgreSqlTableOperations(PostgreSqlColumnDefaultValueConverter converter) { + this(); + super.columnDefaultValueConverter = converter; + } + public String createTableSql(String tableName, JdbcColumn[] columns) { return createTableSql(tableName, columns, "comment"); } @@ -81,6 +87,105 @@ public class TestPostgreSqlTableOperationsSqlGeneration { } } + @ParameterizedTest + @ValueSource(strings = {"table", "column", "alterTable", "alterColumn", "addColumn"}) + void testNulCommentsAreRejectedBeforeDdl(String path) { + TestablePostgreSqlTableOperations ops = new TestablePostgreSqlTableOperations(); + JdbcColumn column = + JdbcColumn.builder() + .withName("col1") + .withType(Types.StringType.get()) + .withNullable(true) + .build(); + String bad = "before\0after"; + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + switch (path) { + case "table": + ops.createTableSql("t", new JdbcColumn[] {column}, bad); + break; + case "column": + ops.createTableSql( + "t", + new JdbcColumn[] { + JdbcColumn.builder() + .withName("col1") + .withType(Types.StringType.get()) + .withNullable(true) + .withComment(bad) + .build() + }); + break; + case "alterTable": + ops.alterTableSql("t", TableChange.updateComment(bad)); + break; + case "alterColumn": + ops.alterTableSql( + "t", TableChange.updateColumnComment(new String[] {"col1"}, bad)); + break; + case "addColumn": + ops.alterTableSql( + "t", + TableChange.addColumn(new String[] {"col2"}, Types.StringType.get(), bad)); + break; + default: + throw new AssertionError(path); + } + }); + Assertions.assertTrue(error.getMessage().contains("comment")); + Assertions.assertTrue(error.getMessage().contains("U+0000")); + } + + @ParameterizedTest + @ValueSource(strings = {"create", "add", "alter"}) + void testNulDefaultsAreRejectedOnEveryDdlPath(String path) { + TestablePostgreSqlTableOperations ops = + new TestablePostgreSqlTableOperations(new PostgreSqlColumnDefaultValueConverter()); + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> { + switch (path) { + case "create": + ops.createTableSql( + "t", + new JdbcColumn[] { + JdbcColumn.builder() + .withName("col1") + .withType(Types.StringType.get()) + .withNullable(true) + .withDefaultValue(Literals.stringLiteral("bad\0default")) + .build() + }); + break; + case "add": + ops.alterTableSql( + "t", + TableChange.addColumn( + new String[] {"col2"}, + Types.StringType.get(), + null, + TableChange.ColumnPosition.defaultPos(), + true, + false, + Literals.stringLiteral("bad\0default"))); + break; + case "alter": + ops.alterTableSql( + "t", + TableChange.updateColumnDefaultValue( + new String[] {"col1"}, Literals.stringLiteral("bad\0default"))); + break; + default: + throw new AssertionError(path); + } + }); + Assertions.assertTrue(error.getMessage().contains("default")); + Assertions.assertTrue(error.getMessage().contains("U+0000")); + } + @ParameterizedTest @ValueSource(booleans = {false, true}) void testDeleteIndexesDropsOnlyConstraints(boolean ifExists) { diff --git a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java index 724a391288..71234d8565 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java +++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java @@ -1278,6 +1278,69 @@ public class CatalogManager implements CatalogDispatcher, Closeable { }); } +<<<<<<< HEAD +======= + private CatalogEntity alterCatalogUnderLock(NameIdentifier ident, CatalogChange... changes) + throws NoSuchCatalogException, IllegalArgumentException, IOException { + SecretMaterialsHolder writtenSecretMaterials = new SecretMaterialsHolder(); + boolean alterCommitted = false; + try (CatalogLease lease = acquireCatalogLease(ident)) { + CatalogEntity updatedCatalog = + store.update( + ident, + CatalogEntity.class, + EntityType.CATALOG, + existing -> { + Map<String, String> currentProperties = + existing.getProperties() == null + ? new HashMap<>() + : new HashMap<>(existing.getProperties()); + + // Validate a transient plaintext view before secret writes. Failed validation + // must not overwrite or delete a previously stored write-through secret. + CatalogChange[] proposedChanges = + SecretAlterChanges.prepareCatalogChangesForTest( + secretManager, existing.id(), changes); + CatalogEntity proposed = + updateEntity( + newCatalogBuilder(ident.namespace(), existing), + new HashMap<>(currentProperties), + proposedChanges) + .build(); + Map<String, String> proposedConfiguration = + secretManager.toPlaintextProperties(proposed.getProperties()); + lease + .wrapper() + .classLoader + .withClassLoader( + cl -> { + lease.catalog().validateConfiguration(proposedConfiguration); + return null; + }, + IllegalArgumentException.class); + + Pair<CatalogChange[], List<SecretMaterial>> secretResult = + SecretAlterChanges.prepareCatalogChanges( + secretManager, currentProperties, existing.id(), changes); + writtenSecretMaterials.set(secretResult.getRight()); + return updateEntity( + newCatalogBuilder(ident.namespace(), existing), + new HashMap<>(currentProperties), + secretResult.getLeft()) + .build(); + }); + alterCommitted = true; + return updatedCatalog; + } catch (NoSuchEntityException e) { + throw new NoSuchCatalogException(CATALOG_DOES_NOT_EXIST_MSG, ident); + } finally { + if (!alterCommitted) { + secretManager.rollbackSecrets(writtenSecretMaterials.get()); + } + } + } + +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) @Override public boolean dropCatalog(NameIdentifier ident, boolean force) throws NonEmptyEntityException, CatalogInUseException { @@ -1702,6 +1765,7 @@ public class CatalogManager implements CatalogDispatcher, Closeable { IsolatedClassLoader classLoader = createClassLoader(provider, conf); BaseCatalog<?> catalog = createBaseCatalog(classLoader, entity); +<<<<<<< HEAD CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); // Validate catalog properties and initialize the config classLoader.withClassLoader( @@ -1718,6 +1782,39 @@ public class CatalogManager implements CatalogDispatcher, Closeable { "Catalog {} has credential providers that cannot vend credentials: {}", entity.nameIdentifier(), e.getMessage()); +======= + /** + * Creates the catalog instance, validates properties, and preloads property/capability values + * into the given wrapper. + */ + private CatalogWrapper initCatalogWrapper( + CatalogWrapper wrapper, + CatalogEntity entity, + @Nullable Map<String, String> propsToValidate, + boolean validateBackend) { + BaseCatalog<?> catalog = createBaseCatalog(wrapper.classLoader, entity); + wrapper.catalog = catalog; + try { + wrapper.classLoader.withClassLoader( + cl -> { + validatePropertyForCreate(catalog.catalogPropertiesMetadata(), propsToValidate); + catalog.validateCredentialProviders(propsToValidate); + if (propsToValidate != null) { + catalog.validateConfiguration(); + } + if (propsToValidate == null) { + // Loading stays lenient for catalogs persisted with unsupported providers, so only + // warn about them. Providers persisted as a secret are checked by their plaintext. + try { + catalog.validateCredentialProviders( + secretManager.toPlaintextProperties(entity.getProperties())); + } catch (IllegalArgumentException e) { + LOG.warn( + "Catalog {} has credential providers that cannot vend credentials: {}", + entity.nameIdentifier(), + e.getMessage()); + } +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) } } diff --git a/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java b/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java index 6d23332d80..98e4f584fe 100644 --- a/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java +++ b/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java @@ -217,6 +217,29 @@ public abstract class BaseCatalog<T extends BaseCatalog> return ops; } + /** + * Validates the initialized configuration without contacting the catalog backend. + * + * @throws IllegalArgumentException if the configuration is invalid + */ + public final void validateConfiguration() { + validateConfiguration(conf); + } + + /** + * Validates the complete proposed catalog configuration without contacting its backend. + * + * <p>Called on explicit create and alter operations, independently of optional backend + * verification, with the catalog ClassLoader installed. The configuration may differ from the + * current entity on alter. Implementations must not mutate it, initialize operations, open + * connections, or retain resources. The default accepts the configuration for compatibility. + * Existing catalogs are not validated on ordinary reads. + * + * @param configuration the complete effective configuration, with secret references resolved + * @throws IllegalArgumentException if the configuration is invalid + */ + public void validateConfiguration(Map<String, String> configuration) {} + /** * Whether to validate this catalog against its backend at create time rather than on first use. * diff --git a/core/src/test/java/org/apache/gravitino/TestCatalog.java b/core/src/test/java/org/apache/gravitino/TestCatalog.java index 6d5a27bee0..b306f91157 100644 --- a/core/src/test/java/org/apache/gravitino/TestCatalog.java +++ b/core/src/test/java/org/apache/gravitino/TestCatalog.java @@ -65,6 +65,16 @@ public class TestCatalog extends BaseCatalog<TestCatalog> { return new TestCatalogOperations(config); } + /** {@inheritDoc} */ + @Override + public void validateConfiguration(Map<String, String> configuration) { + int minimum = Integer.parseInt(configuration.getOrDefault("validation-min", "0")); + int maximum = Integer.parseInt(configuration.getOrDefault("validation-max", "2")); + if (minimum > maximum) { + throw new IllegalArgumentException("validation-min must not exceed validation-max"); + } + } + @Override public boolean shouldValidateOnCreate() { Map<String, String> properties = entity().getProperties(); diff --git a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java index 4633fd8e23..940a297df0 100644 --- a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java +++ b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java @@ -522,6 +522,186 @@ public class TestCatalogManager { } @Test +<<<<<<< HEAD +======= + void testCreateCatalogReturnsNoSuchMetalakeWhenParentDisappears() throws Exception { + InMemoryEntityStore store = Mockito.spy(new InMemoryEntityStore()); + store.initialize(config); + store.put(metalakeEntity, true); + + NoSuchEntityException missingMetalake = + new NoSuchEntityException( + NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE, + EntityType.METALAKE.name().toLowerCase(), + metalake); + Mockito.doThrow(missingMetalake).when(store).put(any(CatalogEntity.class), eq(false)); + + CatalogManager manager = + new CatalogManager(config, store, new RandomIdGenerator(), new SecretManager(config)); + NameIdentifier ident = NameIdentifier.of(metalake, "concurrent_parent_drop"); + Map<String, String> props = + ImmutableMap.of( + PROPERTY_KEY1, "value1", PROPERTY_KEY2, "value2", PROPERTY_KEY5_PREFIX + "1", "value3"); + + try { + NoSuchMetalakeException exception = + Assertions.assertThrows( + NoSuchMetalakeException.class, + () -> + manager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", props)); + Assertions.assertSame(missingMetalake, exception.getCause()); + Mockito.verify(store, Mockito.never()).delete(ident, EntityType.CATALOG, true); + } finally { + manager.close(); + store.close(); + } + } + + @Test + public void testCreateCatalogValidatesBackendConnection() { + Map<String, String> okProps = + Maps.newHashMap( + ImmutableMap.of( + PROPERTY_KEY1, + "value1", + PROPERTY_KEY2, + "value2", + PROPERTY_KEY5_PREFIX + "1", + "value3", + TestCatalogOperations.VALIDATE_ON_CREATE, + "true")); + + // Opted in + backend resolves: creation succeeds. + NameIdentifier okIdent = NameIdentifier.of("metalake", "validate-ok"); + Assertions.assertDoesNotThrow( + () -> + catalogManager.createCatalog( + okIdent, Catalog.Type.RELATIONAL, provider, "comment", okProps)); + Assertions.assertNotNull(catalogManager.getCatalogCache().getIfPresent(okIdent)); + + // Opted in + backend cannot be resolved: creation fails fast and leaves nothing behind. + NameIdentifier failIdent = NameIdentifier.of("metalake", "validate-fail"); + Map<String, String> failProps = + Maps.newHashMap( + ImmutableMap.of( + PROPERTY_KEY1, + "value1", + PROPERTY_KEY2, + "value2", + PROPERTY_KEY5_PREFIX + "1", + "value3", + TestCatalogOperations.VALIDATE_ON_CREATE, + "true", + TestCatalogOperations.FAIL_INITIALIZE, + "true")); + Throwable failure = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.createCatalog( + failIdent, Catalog.Type.RELATIONAL, provider, "comment", failProps)); + Assertions.assertTrue( + failure.getMessage().contains("backend rejected catalog configuration"), + failure.getMessage()); + Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(failIdent)); + // Rolled back, so the identifier is reusable. + Assertions.assertDoesNotThrow( + () -> + catalogManager.createCatalog( + failIdent, Catalog.Type.RELATIONAL, provider, "comment", okProps)); + + // Not opted in: the connection is not validated at create even if it would fail. + NameIdentifier skipIdent = NameIdentifier.of("metalake", "validate-skip"); + Map<String, String> skipProps = + Maps.newHashMap( + ImmutableMap.of( + PROPERTY_KEY1, + "value1", + PROPERTY_KEY2, + "value2", + PROPERTY_KEY5_PREFIX + "1", + "value3", + TestCatalogOperations.FAIL_INITIALIZE, + "true")); + Assertions.assertDoesNotThrow( + () -> + catalogManager.createCatalog( + skipIdent, Catalog.Type.RELATIONAL, provider, "comment", skipProps)); + Assertions.assertNotNull(catalogManager.getCatalogCache().getIfPresent(skipIdent)); + } + + @Test + void testConfigurationValidationOnCreateWithoutBackendVerification() { + NameIdentifier ident = NameIdentifier.of("metalake", "static-invalid-create"); + Map<String, String> properties = + Maps.newHashMap( + ImmutableMap.of( + PROPERTY_KEY1, + "value1", + PROPERTY_KEY2, + "value2", + PROPERTY_KEY5_PREFIX + "1", + "value3", + "validation-min", + "3", + "validation-max", + "2", + TestCatalogOperations.FAIL_INITIALIZE, + "true")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + catalogManager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", properties)); + Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(ident)); + properties.put("validation-max", "4"); + // The identifier is reusable after validation rollback. No backend initialization is needed. + Assertions.assertDoesNotThrow( + () -> + catalogManager.createCatalog( + ident, Catalog.Type.RELATIONAL, provider, "comment", properties)); + } + + @Test + void testConfigurationValidationOnAlterUsesCompleteProposedProperties() { + NameIdentifier ident = NameIdentifier.of("metalake", "static-invalid-alter"); + Map<String, String> properties = + Maps.newHashMap( + ImmutableMap.of( + PROPERTY_KEY1, + "value1", + PROPERTY_KEY2, + "value2", + PROPERTY_KEY5_PREFIX + "1", + "value3", + "validation-min", + "1", + "validation-max", + "2", + TestCatalogOperations.FAIL_INITIALIZE, + "true")); + catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider, "comment", properties); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> catalogManager.alterCatalog(ident, CatalogChange.setProperty("validation-min", "3"))); + Assertions.assertEquals( + "1", catalogManager.loadCatalog(ident).properties().get("validation-min")); + Assertions.assertDoesNotThrow( + () -> + catalogManager.alterCatalog( + ident, + CatalogChange.setProperty("validation-min", "3"), + CatalogChange.setProperty("validation-max", "4"))); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> catalogManager.alterCatalog(ident, CatalogChange.removeProperty("validation-max"))); + Assertions.assertEquals( + "4", catalogManager.loadCatalog(ident).properties().get("validation-max")); + } + + @Test +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) public void testCreateCatalogVerifiesBackendViaTestConnection() throws Exception { // Opted in via validateOnCreate() force-probe, and the probe reports the backend unreachable: // creation must propagate ConnectionFailedException and roll back both the cache and the store. @@ -1872,6 +2052,351 @@ public class TestCatalogManager { Assertions.assertEquals(v, testProps.get(k)); }); +<<<<<<< HEAD Assertions.assertFalse(testProps.containsKey(ID_KEY), "`gravitino.identifier` is missing"); +======= + Assertions.assertFalse( + testProps.containsKey(ID_KEY), + "`gravitino.identifier` is reserved+hidden and should be omitted from responses"); + } + + @Test + void testSecrets() throws Exception { + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + new CatalogManager(config, entityStore, new RandomIdGenerator(), secrets)) { + NameIdentifier ident = NameIdentifier.of("metalake", "secret_ok"); + Catalog catalog = + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(PROPERTY_KEY4, new SecretBinding("memory", "s3cr3t")), + Map.of()); + Assertions.assertEquals( + HiddenPropertyMaskUtils.MASKED_VALUE, catalog.properties().get(PROPERTY_KEY4)); + String urn = + entityStore + .get(ident, EntityType.CATALOG, CatalogEntity.class) + .getProperties() + .get(PROPERTY_KEY4); + Assertions.assertTrue(SecretPropertyUtils.isSecretProperty(PROPERTY_KEY4, urn)); + Assertions.assertEquals("s3cr3t", secrets.readSecret(SecretUrn.parse(urn))); + + manager.alterCatalog(ident, CatalogChange.removeProperty(PROPERTY_KEY4)); + Assertions.assertFalse( + entityStore + .get(ident, EntityType.CATALOG, CatalogEntity.class) + .getProperties() + .containsKey(PROPERTY_KEY4)); + Assertions.assertThrows( + IllegalArgumentException.class, () -> secrets.readSecret(SecretUrn.parse(urn))); + + Assertions.assertTrue(manager.dropCatalog(ident, true)); + Assertions.assertThrows( + IllegalArgumentException.class, () -> secrets.readSecret(SecretUrn.parse(urn))); + } + } + + @Test + void testCredentialProvidersCannotBeSecret() throws Exception { + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + new CatalogManager(config, entityStore, new RandomIdGenerator(), secrets)) { + NameIdentifier ident = NameIdentifier.of("metalake", "secret_credential_providers"); + SecretReference reference = new SecretReference("reference", Map.of("path", "providers")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of( + CredentialConstants.CREDENTIAL_PROVIDERS, + new SecretBinding("memory", "gcs-token")), + Map.of())); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(), + Map.of(CredentialConstants.CREDENTIAL_PROVIDERS, reference))); + Assertions.assertFalse(manager.catalogExists(ident)); + + manager.createCatalog(ident, Catalog.Type.RELATIONAL, provider, "comment", catalogProps()); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + manager.alterCatalog( + ident, + CatalogChange.setSecretBinding( + CredentialConstants.CREDENTIAL_PROVIDERS, + new SecretBinding("memory", "gcs-token")))); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + manager.alterCatalog( + ident, + CatalogChange.setSecretReference( + CredentialConstants.CREDENTIAL_PROVIDERS, reference))); + Assertions.assertTrue(manager.dropCatalog(ident, true)); + } + } + + @Test + void testLoadWarnsForSecretCredentialProviders() throws Exception { + LoggerContext context = + (LoggerContext) LogManager.getContext(CatalogManager.class.getClassLoader(), false); + Configuration logConfig = context.getConfiguration(); + List<LogEvent> events = new ArrayList<>(); + AbstractAppender appender = + new AbstractAppender("credentialProvidersCapture", null, null, true, null) { + @Override + public void append(LogEvent event) { + events.add(event.toImmutable()); + } + }; + appender.start(); + LoggerConfig logger = new LoggerConfig(CatalogManager.class.getName(), Level.WARN, false); + logger.addAppender(appender, Level.WARN, null); + logConfig.addLogger(CatalogManager.class.getName(), logger); + context.updateLoggers(); + + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + new CatalogManager(config, entityStore, new RandomIdGenerator(), secrets)) { + // A catalog persisted before credential providers were rejected as secrets. + NameIdentifier ident = NameIdentifier.of("metalake", "legacy_secret_providers"); + long id = new RandomIdGenerator().nextId(); + SecretUrn urn = writeThroughUrn("catalog", id, CredentialConstants.CREDENTIAL_PROVIDERS); + secrets.writeSecrets(List.of(new SecretMaterial(urn, "gcs-token"))); + Map<String, String> props = new HashMap<>(catalogProps()); + props.put(CredentialConstants.CREDENTIAL_PROVIDERS, urn.toString()); + entityStore.put( + CatalogEntity.builder() + .withId(id) + .withName(ident.name()) + .withNamespace(ident.namespace()) + .withType(Catalog.Type.RELATIONAL) + .withProvider(provider) + .withComment("comment") + .withProperties(props) + .withAuditInfo( + AuditInfo.builder().withCreator("test").withCreateTime(Instant.now()).build()) + .build()); + + Assertions.assertDoesNotThrow(() -> manager.loadCatalog(ident)); + Assertions.assertTrue( + events.stream() + .anyMatch(e -> e.getMessage().getFormattedMessage().contains("gcs-token"))); + Assertions.assertTrue(manager.dropCatalog(ident, true)); + } finally { + logConfig.removeLogger(CatalogManager.class.getName()); + context.updateLoggers(); + appender.stop(); + } + } + + @Test + void testInvalidConfigurationAlterDoesNotMutateSecrets() throws Exception { + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + new CatalogManager(config, entityStore, new RandomIdGenerator(), secrets)) { + NameIdentifier ident = NameIdentifier.of("metalake", "invalid_settings_secret"); + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(PROPERTY_KEY4, new SecretBinding("memory", "original-secret")), + Map.of()); + CatalogEntity original = entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class); + String urn = original.getProperties().get(PROPERTY_KEY4); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + manager.alterCatalog( + ident, + CatalogChange.setProperty("validation-min", "3"), + CatalogChange.setSecretBinding( + PROPERTY_KEY4, new SecretBinding("memory", "replacement")))); + Assertions.assertEquals("original-secret", secrets.readSecret(SecretUrn.parse(urn))); + Assertions.assertEquals( + original.getProperties(), + entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class).getProperties()); + } + } + + @Test + void testConnectionChangesDoNotMutateSecrets() throws Exception { + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + Mockito.spy( + new CatalogManager(config, entityStore, new RandomIdGenerator(), secrets))) { + NameIdentifier ident = NameIdentifier.of("metalake", "secret_connection_test"); + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(PROPERTY_KEY4, new SecretBinding("memory", "stored-secret")), + Map.of()); + CatalogEntity stored = entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class); + String storedUrn = stored.getProperties().get(PROPERTY_KEY4); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + SecretUrn proposedUrn = writeThroughUrn("catalog", stored.id(), PROPERTY_KEY2); + + CatalogManager.CatalogWrapper temporaryWrapper = + Mockito.mock(CatalogManager.CatalogWrapper.class); + AtomicReference<CatalogEntity> effectiveEntity = new AtomicReference<>(); + Mockito.doAnswer( + invocation -> { + effectiveEntity.set(invocation.getArgument(0)); + return temporaryWrapper; + }) + .when(manager) + .createCatalogWrapper(any(CatalogEntity.class), eq(null)); + Mockito.doReturn(null).when(temporaryWrapper).doWithCatalogOps(any()); + + manager.testConnection( + ident, + CatalogChange.setSecretBinding( + PROPERTY_KEY4, new SecretBinding("memory", "temporary-secret")), + CatalogChange.setSecretBinding( + PROPERTY_KEY2, new SecretBinding("memory", "temporary-new-secret"))); + Assertions.assertEquals( + "temporary-secret", effectiveEntity.get().getProperties().get(PROPERTY_KEY4)); + Assertions.assertEquals( + "temporary-new-secret", effectiveEntity.get().getProperties().get(PROPERTY_KEY2)); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + Assertions.assertThrows( + IllegalArgumentException.class, () -> secrets.readSecret(proposedUrn)); + + manager.testConnection(ident, CatalogChange.removeProperty(PROPERTY_KEY4)); + Assertions.assertFalse(effectiveEntity.get().getProperties().containsKey(PROPERTY_KEY4)); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + + manager.testConnection( + ident, + CatalogChange.setSecretReference( + PROPERTY_KEY4, new SecretReference("reference", Map.of("path", "external-secret")))); + String referenceUrn = effectiveEntity.get().getProperties().get(PROPERTY_KEY4); + Assertions.assertTrue(SecretPropertyUtils.isSecretProperty(PROPERTY_KEY4, referenceUrn)); + Assertions.assertEquals( + "resolved-external-secret", + secrets.toPlaintextProperties(effectiveEntity.get().getProperties()).get(PROPERTY_KEY4)); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> manager.testConnection(ident, CatalogChange.setProperty(PROPERTY_KEY4, storedUrn))); + CatalogEntity storedAfter = entityStore.get(ident, EntityType.CATALOG, CatalogEntity.class); + Assertions.assertEquals(stored.getProperties(), storedAfter.getProperties()); + Assertions.assertEquals("stored-secret", secrets.readSecret(SecretUrn.parse(storedUrn))); + Mockito.verify(manager, Mockito.times(4)).acquireCatalogLease(ident); + Mockito.verify(temporaryWrapper, Mockito.times(3)).close(); + } + } + + @Test + void testSecretRollback() throws Exception { + try (SecretManager secrets = memorySecretManager()) { + IdGenerator ids = new AtomicLong(4242L)::getAndIncrement; + CatalogManager manager = Mockito.spy(new CatalogManager(config, entityStore, ids, secrets)); + NameIdentifier ident = NameIdentifier.of("metalake", "secret_fail"); + Mockito.doThrow(new RuntimeException("init failed")) + .when(manager) + .createCatalogWrapper(any(CatalogEntity.class), any()); + SecretUrn urn = writeThroughUrn("catalog", 4242L, PROPERTY_KEY4); + Assertions.assertThrows( + RuntimeException.class, + () -> + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(PROPERTY_KEY4, new SecretBinding("memory", "x")), + Map.of())); + Assertions.assertFalse(entityStore.exists(ident, EntityType.CATALOG)); + Assertions.assertThrows(IllegalArgumentException.class, () -> secrets.readSecret(urn)); + manager.close(); + } + } + + @Test + void testSecretNoMetalake() throws Exception { + try (SecretManager secrets = memorySecretManager(); + CatalogManager manager = + new CatalogManager( + config, entityStore, new AtomicLong(4343L)::getAndIncrement, secrets)) { + NameIdentifier ident = NameIdentifier.of("missing_metalake", "secret_catalog"); + SecretUrn urn = writeThroughUrn("catalog", 4343L, PROPERTY_KEY4); + Assertions.assertThrows( + NoSuchMetalakeException.class, + () -> + manager.createCatalog( + ident, + Catalog.Type.RELATIONAL, + provider, + "comment", + catalogProps(), + Map.of(PROPERTY_KEY4, new SecretBinding("memory", "x")), + Map.of())); + Assertions.assertThrows(IllegalArgumentException.class, () -> secrets.readSecret(urn)); + } + } + + private static Map<String, String> catalogProps() { + return ImmutableMap.of( + "provider", + "test", + PROPERTY_KEY1, + "value1", + PROPERTY_KEY2, + "value2", + PROPERTY_KEY5_PREFIX + "1", + "value3"); + } + + private static SecretUrn writeThroughUrn(String entityType, long entityId, String key) { + return SecretUrn.buildWriteThrough( + "memory", + Map.of( + SecretConstants.ATTR_ENTITY_TYPE, entityType, + SecretConstants.ATTR_ENTITY_ID, String.valueOf(entityId), + SecretConstants.ATTR_PROPERTY_KEY, key)); + } + + private static SecretManager memorySecretManager() { + Config c = new Config(false) {}; + Properties p = new Properties(); + p.setProperty(SecretProviderRegistry.GRAVITINO_SECRET_PROVIDERS, "memory,reference"); + p.setProperty( + SecretProviderRegistry.GRAVITINO_SECRET_PROVIDER_PREFIX + + "memory." + + SecretProviderRegistry.CLASS_NAME, + InMemorySecretsProvider.class.getName()); + p.setProperty( + SecretProviderRegistry.GRAVITINO_SECRET_PROVIDER_PREFIX + + "reference." + + SecretProviderRegistry.CLASS_NAME, + TestReferenceSecretsProvider.class.getName()); + c.loadFromProperties(p); + return new SecretManager(c); +>>>>>>> c7a4551b0 ([#14484] fix(jdbc): validate configuration and classify PostgreSQL failures (#14485)) } } diff --git a/docs/jdbc-catalog-connection-validation.md b/docs/jdbc-catalog-connection-validation.md index 13cedaafd7..c5ba0307f0 100644 --- a/docs/jdbc-catalog-connection-validation.md +++ b/docs/jdbc-catalog-connection-validation.md @@ -65,7 +65,7 @@ it, and closes it. If it fails, the create is rejected and rolled back (the cata persisted), and the caller receives a connection error that names the backend `host:port` and the underlying driver message. -To register a catalog whose backend is temporarily unavailable, set the hidden catalog property +To register a catalog whose backend is temporarily unavailable, set the catalog property `verify-on-create` to `false`, which skips the create-time check: ```json @@ -74,6 +74,11 @@ To register a catalog whose backend is temporarily unavailable, set the hidden c } ``` +This setting skips only the backend connection probe. Driver/URL compatibility, boolean values, +and pool bounds are still checked on catalog creation and alteration, without opening a database +connection. Invalid alterations leave the stored configuration unchanged. + + The default is `true` (verify on create). The connection is still opened lazily on first use when the check is skipped.
