This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new c7a4551b09 [#14484] fix(jdbc): validate configuration and classify
PostgreSQL failures (#14485)
c7a4551b09 is described below
commit c7a4551b092e2f2dc2cbee99ca76735e2d8d770f
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.
---
.../apache/gravitino/catalog/jdbc/JdbcCatalog.java | 32 +++-
.../catalog/jdbc/JdbcCatalogOperations.java | 25 +---
.../jdbc/JdbcCatalogPropertiesMetadata.java | 19 ++-
.../gravitino/catalog/jdbc/config/JdbcConfig.java | 14 +-
.../catalog/jdbc/utils/DataSourceUtils.java | 144 +++++++++++-------
.../jdbc/TestJdbcCatalogPropertiesMetadata.java | 104 ++++++++++++-
.../jdbc/utils/TestDataSourceUrlValidation.java | 4 +-
.../jdbc/utils/TestDataSourceValidation.java | 24 +++
.../postgresql/PostgreSQLCatalogOperations.java | 3 +-
.../catalog/postgresql/PostgreSqlCatalog.java | 4 +-
.../PostgreSqlColumnDefaultValueConverter.java | 12 ++
.../converter/PostgreSqlExceptionConverter.java | 21 +--
.../operation/PostgreSqlTableOperations.java | 85 ++++++++++-
.../TestPostgreSqlCatalogConfiguration.java | 28 ++++
.../TestPostgreSqlColumnDefaultValueConverter.java | 14 ++
.../TestPostgreSqlExceptionConverter.java | 79 ++++++++++
.../TestPostgreSqlTableCreatePrivileges.java | 165 +++++++++++++++++++++
.../operation/TestPostgreSqlTableOperations.java | 151 +++++++++++++++++++
...TestPostgreSqlTableOperationsSqlGeneration.java | 105 +++++++++++++
.../apache/gravitino/catalog/CatalogManager.java | 43 ++++--
.../apache/gravitino/connector/BaseCatalog.java | 23 +++
.../java/org/apache/gravitino/TestCatalog.java | 10 ++
.../gravitino/catalog/TestCatalogManager.java | 100 +++++++++++++
docs/jdbc-catalog-connection-validation.md | 7 +-
24 files changed, 1105 insertions(+), 111 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 95bbd66a92..556f7a826a 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 631b53db12..ebe9679478 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;
@@ -81,7 +79,6 @@ 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;
-import org.apache.gravitino.utils.MapUtils;
import org.apache.gravitino.utils.PrincipalUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -168,14 +165,7 @@ public class JdbcCatalogOperations
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();
@@ -221,18 +211,15 @@ public class JdbcCatalogOperations
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 be57c26f3e..6b79ec3c5d 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 {
@@ -129,7 +131,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(),
@@ -143,7 +145,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()
@@ -156,6 +158,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 d6426e6f54..c13d56aadc 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
@@ -198,7 +198,7 @@ public class JdbcConfig extends Config {
}
public boolean getTestOnBorrow() {
- return get(TEST_ON_BORROW);
+ return strictBoolean(TEST_ON_BORROW);
}
public long getMaxWaitMs() {
@@ -220,11 +220,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 b2f61e19ff..de1aa71997 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.List;
@@ -70,6 +71,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
@@ -88,51 +105,94 @@ public class DataSourceUtils {
// 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");
+ }
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 {
- 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
@@ -142,11 +202,6 @@ public class DataSourceUtils {
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);
@@ -181,23 +236,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 60a4e95a34..dfadcc6e03 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;
@@ -47,9 +51,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();
@@ -113,7 +206,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()));
@@ -297,6 +390,15 @@ public class TestJdbcCatalogPropertiesMetadata {
}
}
+ /** 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");
+ }
+ }
+
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 d11e87a179..c40f29cc35 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
@@ -453,9 +453,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
index 40db43de67..95e21e5088 100644
---
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
@@ -27,6 +27,8 @@ 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;
@@ -42,6 +44,28 @@ 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 {
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 01c8438503..fa42e943eb 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
@@ -26,6 +26,7 @@ import com.google.common.collect.Lists;
import java.nio.charset.StandardCharsets;
import java.sql.Connection;
import java.sql.DatabaseMetaData;
+import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
@@ -51,6 +52,7 @@ import
org.apache.gravitino.catalog.jdbc.operation.DatabaseOperation;
import org.apache.gravitino.catalog.jdbc.operation.JdbcTableOperations;
import org.apache.gravitino.catalog.jdbc.operation.RequireDatabaseOperation;
import org.apache.gravitino.catalog.jdbc.utils.JdbcConnectorUtils;
+import org.apache.gravitino.exceptions.ForbiddenException;
import org.apache.gravitino.exceptions.NoSuchColumnException;
import org.apache.gravitino.exceptions.NoSuchSchemaException;
import org.apache.gravitino.exceptions.NoSuchTableException;
@@ -60,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;
@@ -102,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 +251,7 @@ public class PostgreSqlTableOperations extends
JdbcTableOperations
.append(TABLE_COMMENT)
.append(quotedIdentifier(tableName))
.append(IS)
- .append(escapeSqlLiteral(comment, '\''))
+ .append(escapeSqlLiteral(validateText(comment, "table comment"),
'\''))
.append("';");
}
Arrays.stream(columns)
@@ -221,7 +265,11 @@ public class PostgreSqlTableOperations extends
JdbcTableOperations
.append(".")
.append(quotedIdentifier(jdbcColumn.name()))
.append(IS)
- .append(escapeSqlLiteral(jdbcColumn.comment(), '\''))
+ .append(
+ escapeSqlLiteral(
+ validateText(
+ jdbcColumn.comment(), "comment for column " +
jdbcColumn.name()),
+ '\''))
.append("';"));
// Return the generated SQL statement
@@ -477,7 +525,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())) {
@@ -490,7 +539,7 @@ public class PostgreSqlTableOperations extends
JdbcTableOperations
return TABLE_COMMENT
+ quotedIdentifier(jdbcTable.name())
+ IS
- + escapeSqlLiteral(newComment, '\'')
+ + escapeSqlLiteral(validateText(newComment, "table comment"), '\'')
+ "';";
}
@@ -650,7 +699,8 @@ public class PostgreSqlTableOperations extends
JdbcTableOperations
+ "."
+ quotedIdentifier(col)
+ IS
- + escapeSqlLiteral(addColumn.getComment(), '\'')
+ + escapeSqlLiteral(
+ validateText(addColumn.getComment(), "comment for column " +
col), '\'')
+ "';");
}
return result;
@@ -668,7 +718,7 @@ public class PostgreSqlTableOperations extends
JdbcTableOperations
+ "."
+ quotedIdentifier(col)
+ IS
- + escapeSqlLiteral(newComment, '\'')
+ + escapeSqlLiteral(validateText(newComment, "comment for column " +
col), '\'')
+ "';";
}
@@ -763,6 +813,29 @@ public class PostgreSqlTableOperations extends
JdbcTableOperations
}
}
+ 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;
}
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 1db20ecf54..54ecea04c4 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.exceptions.TableAlreadyExistsException;
import org.apache.gravitino.rel.Column;
@@ -197,6 +200,154 @@ public class TestPostgreSqlTableOperations extends
TestPostgreSql {
}
}
+ @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 3f2e81eaa7..ef659ec49b 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;
@@ -47,6 +48,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");
}
@@ -156,6 +162,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 2e1bd9f0a8..d0e850b402 100644
--- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
+++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
@@ -1280,7 +1280,7 @@ public class CatalogManager implements CatalogDispatcher,
Closeable {
throws NoSuchCatalogException, IllegalArgumentException, IOException {
SecretMaterialsHolder writtenSecretMaterials = new SecretMaterialsHolder();
boolean alterCommitted = false;
- try {
+ try (CatalogLease lease = acquireCatalogLease(ident)) {
CatalogEntity updatedCatalog =
store.update(
ident,
@@ -1292,20 +1292,38 @@ public class CatalogManager implements
CatalogDispatcher, Closeable {
? 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());
- CatalogChange[] effectiveChanges = secretResult.getLeft();
-
- CatalogEntity.Builder newCatalogBuilder =
- newCatalogBuilder(ident.namespace(), existing);
-
- Map<String, String> newProps =
- existing.getProperties() == null
- ? new HashMap<>()
- : new HashMap<>(existing.getProperties());
- return updateEntity(newCatalogBuilder, newProps,
effectiveChanges).build();
+ return updateEntity(
+ newCatalogBuilder(ident.namespace(), existing),
+ new HashMap<>(currentProperties),
+ secretResult.getLeft())
+ .build();
});
alterCommitted = true;
return updatedCatalog;
@@ -1847,6 +1865,9 @@ public class CatalogManager implements CatalogDispatcher,
Closeable {
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.
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 22491e48b9..afa2daaec0 100644
--- a/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
+++ b/core/src/main/java/org/apache/gravitino/connector/BaseCatalog.java
@@ -222,6 +222,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 e86579c433..31bdae35c1 100644
--- a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
+++ b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
@@ -746,6 +746,75 @@ public class TestCatalogManager {
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
public void testCreateCatalogVerifiesBackendViaTestConnection() throws
Exception {
// Opted in via validateOnCreate() force-probe, and the probe reports the
backend unreachable:
@@ -2280,6 +2349,37 @@ public class TestCatalogManager {
}
}
+ @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();
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.
