This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch branch-1.3
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.3 by this push:
new 8e80829c86 [Cherry-pick to branch-1.3] [#14484] fix(jdbc): validate
configuration and classify PostgreSQL failures (#14485) (#14553)
8e80829c86 is described below
commit 8e80829c86feb7bd024f5185f05323014261c665
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Tue Nov 3 17:24:37 2026 +0800
[Cherry-pick to branch-1.3] [#14484] fix(jdbc): validate configuration and
classify PostgreSQL failures (#14485) (#14553)
**Cherry-pick Information:**
- Original commit: c7a4551b092e2f2dc2cbee99ca76735e2d8d770f (#14485)
- Target branch: `branch-1.3`
- Status: Conflicts resolved.
Backports static JDBC configuration validation and PostgreSQL failure
classification. The catalog alter hook is adapted to the existing 1.3
lifecycle and persistence flow; main-only secret-management APIs and
their tests are excluded. Includes the native pool-property guard
required to keep static validation connection-free, plus missing test
dependencies and fixtures.
Validation: Spotless and `git diff --check` passed. JDBC common: 103
tests; PostgreSQL: 266 tests (including PostgreSQL 12–16 Docker
integration suites); filtered `TestCatalogManager`: 39 tests;
`TestCatalogWrapperLease`: 18 tests. All passed with no skips. The two
manager configuration-validation regression tests also failed as
expected with the validation calls removed, then passed after
restoration.
Fresh-context review of f08ff43933 found no blocking or important
issues.
---------
Co-authored-by: Qi Yu <[email protected]>
---
.../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 | 199 +++++++++++++++------
.../jdbc/TestJdbcCatalogPropertiesMetadata.java | 104 ++++++++++-
.../jdbc/utils/TestDataSourceUrlValidation.java | 4 +-
.../jdbc/utils/TestDataSourceValidation.java | 88 +++++++++
catalogs/catalog-jdbc-postgresql/build.gradle.kts | 2 +
.../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 | 50 ++++--
.../apache/gravitino/connector/BaseCatalog.java | 23 +++
.../java/org/apache/gravitino/TestCatalog.java | 10 ++
.../gravitino/catalog/TestCatalogManager.java | 69 +++++++
.../gravitino/connector/TestCatalogOperations.java | 9 +-
docs/jdbc-catalog-connection-validation.md | 7 +-
26 files changed, 1204 insertions(+), 118 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..c5f067cc3c 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.sorts.SortOrder;
import org.apache.gravitino.rel.expressions.transforms.Transform;
import org.apache.gravitino.rel.indexes.Index;
import org.apache.gravitino.utils.IsolatedClassLoader;
-import org.apache.gravitino.utils.MapUtils;
import org.apache.gravitino.utils.PrincipalUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -167,14 +164,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 +218,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..1d3e5d03c8 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,8 +18,10 @@
*/
package org.apache.gravitino.catalog.jdbc.utils;
+import java.sql.Driver;
import java.sql.SQLException;
import java.time.Duration;
+import java.util.List;
import java.util.Map;
import java.util.Properties;
import javax.sql.DataSource;
@@ -36,6 +38,32 @@ import org.apache.gravitino.utils.JdbcUrlUtils;
*/
public class DataSourceUtils {
+ // DBCP2 connection-pool properties that must never come from catalog
configuration. The whole
+ // config map is handed to BasicDataSourceFactory, so allowing these would
either run arbitrary
+ // code or let a raw property override the validated canonical connection
fields:
+ // - connectionFactoryClassName / evictionPolicyClassName /
driverClassName: their values are
+ // class names the factory loads and instantiates via reflection
(Class.forName + newInstance)
+ // when the pool creates connections (remote code execution). The
legitimate driver is set
+ // separately from the "jdbc-driver" property via an explicit setter, so
the raw
+ // "driverClassName" key is never needed.
+ // - url / username / password: the connection identity is validated
(JdbcUrlUtils, the H2
+ // guard) and applied from the canonical
"jdbc-url"/"jdbc-user"/"jdbc-password" properties via
+ // explicit setters. A raw DBCP key here would reach the factory
unvalidated and, once the
+ // pool initializes, take effect before those setters run — e.g. a raw
"url" could smuggle an
+ // H2 or otherwise-denied URL past the guards.
+ // - initialSize: defense in depth. A value > 0 makes the factory eagerly
open a connection
+ // during createDataSource; blocking it keeps pool creation lazy so our
explicit url/driver
+ // setters, not factory-time eager init, control how connections are
created.
+ private static final List<String> UNSAFE_POOL_PROPERTIES =
+ List.of(
+ "connectionFactoryClassName",
+ "evictionPolicyClassName",
+ "driverClassName",
+ "url",
+ "username",
+ "password",
+ "initialSize");
+
/** SQL statements for database connection pool testing. */
private static final String POOL_TEST_QUERY = "SELECT 1";
@@ -45,6 +73,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 +100,102 @@ public class DataSourceUtils {
if (jdbcConfig.getJdbcDriver().toLowerCase().startsWith("org.h2.")) {
throw new GravitinoRuntimeException("H2 JDBC driver is not allowed in
catalog configuration");
}
+ // 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();
+ 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());
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 +222,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());
@@ -177,6 +245,31 @@ public class DataSourceUtils {
return decoded;
}
+ /**
+ * Rejects DBCP2 connection-pool properties that would let catalog
configuration run arbitrary
+ * classes on the server ({@code connectionFactoryClassName}, {@code
evictionPolicyClassName},
+ * {@code driverClassName}) or override the validated canonical connection
identity ({@code url},
+ * {@code username}, {@code password}), plus {@code initialSize} (blocked as
defense in depth to
+ * keep pool creation lazy).
+ *
+ * @param config the JDBC configuration properties forwarded to the DBCP2
factory
+ * @throws GravitinoRuntimeException if an unsafe connection-pool property
is present
+ */
+ private static void rejectUnsafePoolProperties(Map<String, String> config) {
+ if (config == null) {
+ return;
+ }
+ for (String key : config.keySet()) {
+ for (String unsafe : UNSAFE_POOL_PROPERTIES) {
+ if (unsafe.equalsIgnoreCase(key)) {
+ throw new GravitinoRuntimeException(
+ "Unsafe JDBC connection pool property '%s' is not allowed in
catalog configuration",
+ key);
+ }
+ }
+ }
+ }
+
public static void closeDataSource(DataSource dataSource) {
if (null != dataSource) {
try {
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..235bcb5ff4 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,15 @@ public class TestJdbcCatalogPropertiesMetadata {
.isHiddenProperty(JdbcConfig.CONNECT_TIMEOUT_MS.getKey()));
}
+ /** 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 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..6ccec6c2f8
--- /dev/null
+++
b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/utils/TestDataSourceValidation.java
@@ -0,0 +1,88 @@
+/*
+ * 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.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import org.apache.commons.dbcp2.BasicDataSource;
+import org.apache.gravitino.catalog.jdbc.config.JdbcConfig;
+import org.apache.gravitino.exceptions.GravitinoRuntimeException;
+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;
+
+/** Tests JDBC configuration validation before any backend connection. */
+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"));
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "connectionFactoryClassName",
+ "evictionPolicyClassName",
+ "driverClassName",
+ "url",
+ "username",
+ "password",
+ "initialSize",
+ "INITIALSIZE"
+ })
+ void testUnsafeNativePoolPropertiesAreRejected(String key) {
+ GravitinoRuntimeException error =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class, () ->
createDataSource(Map.of(key, "1")));
+ Assertions.assertTrue(error.getMessage().contains(key));
+ }
+
+ 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/build.gradle.kts
b/catalogs/catalog-jdbc-postgresql/build.gradle.kts
index b19ae32ac7..6dde4d3b4e 100644
--- a/catalogs/catalog-jdbc-postgresql/build.gradle.kts
+++ b/catalogs/catalog-jdbc-postgresql/build.gradle.kts
@@ -48,6 +48,8 @@ dependencies {
testImplementation(project(":server"))
testImplementation(project(":server-common"))
+ testImplementation(libs.commons.dbcp2)
+ testImplementation(libs.mockito.core)
testImplementation(libs.awaitility)
testImplementation(libs.junit.jupiter.api)
testImplementation(libs.junit.jupiter.params)
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 b0fa38be8e..194c7dd66b 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 724a391288..967759479d 100644
--- a/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
+++ b/core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java
@@ -1214,23 +1214,36 @@ public class CatalogManager implements
CatalogDispatcher, Closeable {
lifecycleLock.readLock().lock();
try {
checkOpen();
- CatalogEntity updatedCatalog =
- store.update(
- ident,
- CatalogEntity.class,
- EntityType.CATALOG,
- catalog -> {
- CatalogEntity.Builder newCatalogBuilder =
- newCatalogBuilder(ident.namespace(), catalog);
-
- Map<String, String> newProps =
- catalog.getProperties() == null
- ? new HashMap<>()
- : new HashMap<>(catalog.getProperties());
- newCatalogBuilder = updateEntity(newCatalogBuilder,
newProps, changes);
-
- return newCatalogBuilder.build();
- });
+ CatalogEntity updatedCatalog;
+ try (CatalogLease lease = acquireCatalogLease(ident)) {
+ updatedCatalog =
+ store.update(
+ ident,
+ CatalogEntity.class,
+ EntityType.CATALOG,
+ catalog -> {
+ CatalogEntity.Builder newCatalogBuilder =
+ newCatalogBuilder(ident.namespace(), catalog);
+
+ Map<String, String> newProps =
+ catalog.getProperties() == null
+ ? new HashMap<>()
+ : new HashMap<>(catalog.getProperties());
+ newCatalogBuilder = updateEntity(newCatalogBuilder,
newProps, changes);
+
+ CatalogEntity proposed = newCatalogBuilder.build();
+ lease
+ .wrapper()
+ .classLoader
+ .withClassLoader(
+ cl -> {
+
lease.catalog().validateConfiguration(proposed.getProperties());
+ return null;
+ },
+ IllegalArgumentException.class);
+ return proposed;
+ });
+ }
// Invalidate after store.update() so that any background thread
that tries to reload
// the old catalog identifier from the store (after the
invalidate) will get
// NoSuchCatalogException instead of stale data. Invalidating
before the update creates
@@ -1708,6 +1721,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.
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..e373ef717a 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
+ * @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..0d2287d985 100644
--- a/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
+++ b/core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java
@@ -521,6 +521,75 @@ public class TestCatalogManager {
Assertions.assertNull(catalogManager.getCatalogCache().getIfPresent(failedIdent));
}
+ @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:
diff --git
a/core/src/test/java/org/apache/gravitino/connector/TestCatalogOperations.java
b/core/src/test/java/org/apache/gravitino/connector/TestCatalogOperations.java
index 40aa8caabe..7611a30778 100644
---
a/core/src/test/java/org/apache/gravitino/connector/TestCatalogOperations.java
+++
b/core/src/test/java/org/apache/gravitino/connector/TestCatalogOperations.java
@@ -134,6 +134,9 @@ public class TestCatalogOperations
public static final String FAIL_CREATE = "fail-create";
+ /** Forces operations initialization to fail without contacting a backend. */
+ public static final String FAIL_INITIALIZE = "fail-initialize";
+
public static final String FAIL_TEST = "need-fail";
public static final String VALIDATE_ON_CREATE = "validate-on-create";
@@ -155,7 +158,11 @@ public class TestCatalogOperations
@Override
public void initialize(
Map<String, String> config, CatalogInfo info, HasPropertyMetadata
propertyMetadata)
- throws RuntimeException {}
+ throws RuntimeException {
+ if (Boolean.parseBoolean(config.get(FAIL_INITIALIZE))) {
+ throw new IllegalArgumentException("backend rejected catalog
configuration");
+ }
+ }
@Override
public void close() throws IOException {}
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.
