This is an automated email from the ASF dual-hosted git repository. jerryshao pushed a commit to branch branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 66e831a3fcf093861f03e9f85392bfaef1d4f2bf Author: Qi Yu <[email protected]> AuthorDate: Mon Nov 2 22:36:55 2026 +0800 [#14523] fix(postgresql): handle native identifiers and generated indexes safely (#14527) Bind schema-comment queries, quote table/column/constraint identifiers consistently, escape exact JDBC metadata patterns, enforce PostgreSQL UTF-8 name limits and let PostgreSQL name unnamed constraints. Route existing native table deletion through provider-specific validation while preserving strict creation validation and other catalogs' defaults. Existing quote/backslash/wildcard names must remain data or identifiers, overlong names must not truncate silently, and unnamed indexes must not collide under a literal null name. Fixes #14523 Fixes #14488 Fixes #14494 Consolidates #14489, #14495 into this PR. Safe native-name table load/alter/drop and exact schema/table metadata matching; actionable UTF-8 and index conflict errors. Adds a default existing-name capability method. PostgreSQL purge, arbitrary native-name schema CREATE/DROP and general column-name validation expansion remain outside scope. Full coreUnitTest: 2,691 cases, 2 skipped; PostgreSQL: 88, Docker enabled, no failures. Combined regressions cover quoted native names with unnamed constraints, original bound parameters plus escaped metadata filters, and strict creation/default-provider validation. Mutation/restored-green verification and independent cumulative review completed. Deployed REST/Trino/Spark end-to-end is not claimed. (cherry picked from commit 207972fe44a95a42ebbffe68d12cdc51d2649230) --- catalogs/catalog-jdbc-postgresql/build.gradle.kts | 1 + .../postgresql/PostgreSqlCatalogCapability.java | 35 +++- .../operation/PostgreSqlSchemaOperations.java | 26 ++- .../operation/PostgreSqlTableOperations.java | 224 +++++++++++---------- .../TestPostgreSqlCatalogCapability.java | 39 ++++ .../operation/TestPostgreSqlSchemaOperations.java | 26 +++ ...estPostgreSqlSchemaOperationsSqlGeneration.java | 46 +++++ .../operation/TestPostgreSqlTableOperations.java | 136 +++++++++++++ ...TestPostgreSqlTableOperationsSqlGeneration.java | 75 +++++++ .../catalog/TableNormalizeDispatcher.java | 15 +- .../gravitino/connector/capability/Capability.java | 15 ++ .../catalog/TestTableNormalizeDispatcher.java | 53 +++++ 12 files changed, 569 insertions(+), 122 deletions(-) diff --git a/catalogs/catalog-jdbc-postgresql/build.gradle.kts b/catalogs/catalog-jdbc-postgresql/build.gradle.kts index a394596ea4..b19ae32ac7 100644 --- a/catalogs/catalog-jdbc-postgresql/build.gradle.kts +++ b/catalogs/catalog-jdbc-postgresql/build.gradle.kts @@ -51,6 +51,7 @@ dependencies { testImplementation(libs.awaitility) testImplementation(libs.junit.jupiter.api) testImplementation(libs.junit.jupiter.params) + testImplementation(libs.mockito.core) testImplementation(libs.mysql.driver) testImplementation(libs.postgresql.driver) testImplementation(libs.testcontainers) diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java index 3d671565c2..07f5c1e144 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSqlCatalogCapability.java @@ -18,7 +18,10 @@ */ package org.apache.gravitino.catalog.postgresql; +import java.nio.charset.StandardCharsets; +import java.util.Locale; import java.util.Set; +import org.apache.gravitino.MetadataObjects; import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.connector.capability.CapabilityResult; @@ -40,14 +43,40 @@ public class PostgreSqlCatalogCapability implements Capability { private static final Set<String> POSTGRESQL_RESERVED_WORDS = Set.of("pg_catalog", "information_schema"); + /** {@inheritDoc} */ + @Override + public CapabilityResult specificationOnExistingName(Scope scope, String name) { + if (scope != Scope.TABLE && scope != Scope.SCHEMA) { + return specificationOnName(scope, name); + } + if (MetadataObjects.METADATA_OBJECT_RESERVED_NAME.equals(name)) { + return CapabilityResult.unsupported("The metadata wildcard name is reserved"); + } + if (name.isEmpty() + || name.indexOf('\0') >= 0 + || name.getBytes(StandardCharsets.UTF_8).length > 63) { + return CapabilityResult.unsupported( + "PostgreSQL native names must contain 1 to 63 UTF-8 bytes and no NUL"); + } + if (scope == Scope.SCHEMA + && POSTGRESQL_RESERVED_WORDS.contains(name.toLowerCase(Locale.ROOT))) { + return CapabilityResult.unsupported("PostgreSQL system schema names are reserved"); + } + return CapabilityResult.SUPPORTED; + } + @Override public CapabilityResult specificationOnName(Scope scope, String name) { - if (!name.matches(POSTGRESQL_NAME_PATTERN)) { + if (!name.matches(POSTGRESQL_NAME_PATTERN) + || name.getBytes(StandardCharsets.UTF_8).length > 63) { return CapabilityResult.unsupported( - String.format("The %s name '%s' is illegal.", scope, name)); + String.format( + "The %s name '%s' is illegal. Names must use supported characters and contain at most 63 UTF-8 bytes.", + scope, name)); } - if (scope == Scope.SCHEMA && POSTGRESQL_RESERVED_WORDS.contains(name.toLowerCase())) { + if (scope == Scope.SCHEMA + && POSTGRESQL_RESERVED_WORDS.contains(name.toLowerCase(Locale.ROOT))) { return CapabilityResult.unsupported( String.format("The %s name '%s' is reserved and cannot be used.", scope, name)); } diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java index 3996297b01..57d62c7457 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlSchemaOperations.java @@ -131,7 +131,15 @@ public class PostgreSqlSchemaOperations extends JdbcDatabaseOperations { */ private ResultSet getSchema(Connection connection, String schemaName) throws SQLException { final DatabaseMetaData metaData = connection.getMetaData(); - return metaData.getSchemas(connection.getCatalog(), schemaName); + String escape = metaData.getSearchStringEscape(); + String pattern = + schemaName == null + ? null + : schemaName + .replace(escape, escape + escape) + .replace("%", escape + "%") + .replace("_", escape + "_"); + return metaData.getSchemas(connection.getCatalog(), pattern); } @Override @@ -154,7 +162,8 @@ public class PostgreSqlSchemaOperations extends JdbcDatabaseOperations { @Override protected String generateDatabaseExistSql(String databaseName) { return String.format( - "SELECT n.datname FROM pg_catalog.pg_database n where n.datname='%s'", databaseName); + "SELECT n.datname FROM pg_catalog.pg_database n where n.datname='%s'", + databaseName.replace("'", "''")); } @Override @@ -178,17 +187,16 @@ public class PostgreSqlSchemaOperations extends JdbcDatabaseOperations { return false; } - private String getShowSchemaCommentSql(String schema) { - return String.format( - "SELECT obj_description(n.oid, 'pg_namespace') AS comment\n" - + "FROM pg_catalog.pg_namespace n\n" - + "WHERE n.nspname = '%s';\n", - schema); + private String getShowSchemaCommentSql() { + return ("SELECT obj_description(n.oid, 'pg_namespace') AS comment\n" + + "FROM pg_catalog.pg_namespace n\n" + + "WHERE n.nspname = ?;\n"); } private String getSchemaComment(String schema, Connection connection) throws SQLException { try (PreparedStatement preparedStatement = - connection.prepareStatement(getShowSchemaCommentSql(schema))) { + connection.prepareStatement(getShowSchemaCommentSql())) { + preparedStatement.setString(1, schema); try (ResultSet resultSet = preparedStatement.executeQuery()) { if (resultSet.next()) { return resultSet.getString("comment"); diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java b/catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java index 21c0f4db2a..b0fa38be8e 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 @@ -23,6 +23,7 @@ import static org.apache.gravitino.catalog.jdbc.utils.JdbcConnectorUtils.escapeS import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.Lists; +import java.nio.charset.StandardCharsets; import java.sql.Connection; import java.sql.DatabaseMetaData; import java.sql.ResultSet; @@ -33,6 +34,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.stream.Collectors; +import javax.annotation.Nullable; import javax.sql.DataSource; import org.apache.commons.collections4.MapUtils; import org.apache.commons.lang3.ArrayUtils; @@ -48,9 +50,11 @@ import org.apache.gravitino.catalog.jdbc.converter.JdbcTypeConverter; import org.apache.gravitino.catalog.jdbc.operation.DatabaseOperation; import org.apache.gravitino.catalog.jdbc.operation.JdbcTableOperations; import org.apache.gravitino.catalog.jdbc.operation.RequireDatabaseOperation; +import org.apache.gravitino.catalog.jdbc.utils.JdbcConnectorUtils; import org.apache.gravitino.exceptions.NoSuchColumnException; import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NoSuchTableException; +import org.apache.gravitino.exceptions.TableAlreadyExistsException; import org.apache.gravitino.rel.Column; import org.apache.gravitino.rel.TableChange; import org.apache.gravitino.rel.expressions.distributions.Distribution; @@ -171,16 +175,14 @@ public class PostgreSqlTableOperations extends JdbcTableOperations StringBuilder sqlBuilder = new StringBuilder(); sqlBuilder .append("CREATE TABLE ") - .append(PG_QUOTE) - .append(tableName) - .append(PG_QUOTE) + .append(quotedIdentifier(tableName)) .append(" (") .append(NEW_LINE); // Add columns for (int i = 0; i < columns.length; i++) { JdbcColumn column = columns[i]; - sqlBuilder.append(" ").append(PG_QUOTE).append(column.name()).append(PG_QUOTE); + sqlBuilder.append(" ").append(quotedIdentifier(column.name())); appendColumnDefinition(column, sqlBuilder); // Add a comma for the next column, unless it's the last one @@ -203,9 +205,7 @@ public class PostgreSqlTableOperations extends JdbcTableOperations sqlBuilder .append(NEW_LINE) .append(TABLE_COMMENT) - .append(PG_QUOTE) - .append(tableName) - .append(PG_QUOTE) + .append(quotedIdentifier(tableName)) .append(IS) .append(escapeSqlLiteral(comment, '\'')) .append("';"); @@ -217,13 +217,9 @@ public class PostgreSqlTableOperations extends JdbcTableOperations sqlBuilder .append(NEW_LINE) .append(COLUMN_COMMENT) - .append(PG_QUOTE) - .append(tableName) - .append(PG_QUOTE) + .append(quotedIdentifier(tableName)) .append(".") - .append(PG_QUOTE) - .append(jdbcColumn.name()) - .append(PG_QUOTE) + .append(quotedIdentifier(jdbcColumn.name())) .append(IS) .append(escapeSqlLiteral(jdbcColumn.comment(), '\'')) .append("';")); @@ -238,18 +234,19 @@ public class PostgreSqlTableOperations extends JdbcTableOperations @VisibleForTesting static void appendIndexesSql(Index[] indexes, StringBuilder sqlBuilder) { for (Index index : indexes) { + validateIndexName(index.name()); String fieldStr = getIndexFieldStr(index.fieldNames()); sqlBuilder.append(",").append(NEW_LINE); switch (index.type()) { case PRIMARY_KEY: if (StringUtils.isNotEmpty(index.name())) { - sqlBuilder.append("CONSTRAINT ").append(PG_QUOTE).append(index.name()).append(PG_QUOTE); + sqlBuilder.append("CONSTRAINT ").append(quotedIdentifier(index.name())); } sqlBuilder.append(" PRIMARY KEY (").append(fieldStr).append(")"); break; case UNIQUE_KEY: if (StringUtils.isNotEmpty(index.name())) { - sqlBuilder.append("CONSTRAINT ").append(PG_QUOTE).append(index.name()).append(PG_QUOTE); + sqlBuilder.append("CONSTRAINT ").append(quotedIdentifier(index.name())); } sqlBuilder.append(" UNIQUE (").append(fieldStr).append(")"); break; @@ -267,7 +264,7 @@ public class PostgreSqlTableOperations extends JdbcTableOperations throw new IllegalArgumentException( "Index does not support complex fields in PostgreSQL"); } - return PG_QUOTE + colNames[0] + PG_QUOTE; + return quotedIdentifier(colNames[0]); }) .collect(Collectors.joining(", ")); } @@ -300,18 +297,14 @@ public class PostgreSqlTableOperations extends JdbcTableOperations @Override protected String generateRenameTableSql(String oldTableName, String newTableName) { return ALTER_TABLE - + PG_QUOTE - + oldTableName - + PG_QUOTE + + quotedIdentifier(oldTableName) + " RENAME TO " - + PG_QUOTE - + newTableName - + PG_QUOTE; + + quotedIdentifier(newTableName); } @Override protected String generateDropTableSql(String tableName) { - return "DROP TABLE " + PG_QUOTE + tableName + PG_QUOTE; + return "DROP TABLE " + quotedIdentifier(tableName); } @Override @@ -380,14 +373,10 @@ public class PostgreSqlTableOperations extends JdbcTableOperations && Column.DEFAULT_VALUE_NOT_SET.equals(pendingColumn.defaultValue())) { alterSql.add( ALTER_TABLE - + PG_QUOTE - + tableName - + PG_QUOTE + + quotedIdentifier(tableName) + " " + ALTER_COLUMN - + PG_QUOTE - + updateColumnNullability.fieldName()[0] - + PG_QUOTE + + quotedIdentifier(updateColumnNullability.fieldName()[0]) + " DROP DEFAULT;"); } } else if (change instanceof TableChange.AddIndex) { @@ -427,7 +416,7 @@ public class PostgreSqlTableOperations extends JdbcTableOperations return String.format( "ALTER TABLE %s %s %s %s;", - PG_QUOTE + tableName + PG_QUOTE, ALTER_COLUMN, PG_QUOTE + fieldName + PG_QUOTE, action); + quotedIdentifier(tableName), ALTER_COLUMN, quotedIdentifier(fieldName), action); } @VisibleForTesting @@ -435,28 +424,20 @@ public class PostgreSqlTableOperations extends JdbcTableOperations // Gravitino creates primary and unique indexes as constraints. Dropping the constraint // also removes its backing index, so a second DROP INDEX would fail after a successful drop. return String.format( - "ALTER TABLE %s%s%s DROP CONSTRAINT %s%s%s%s;", - PG_QUOTE, - tableName, - PG_QUOTE, + "ALTER TABLE %s DROP CONSTRAINT %s%s;", + quotedIdentifier(tableName), deleteIndex.isIfExists() ? "IF EXISTS " : "", - PG_QUOTE, - deleteIndex.getName(), - PG_QUOTE); + quotedIdentifier(deleteIndex.getName())); } @VisibleForTesting static String addIndexDefinition(String tableName, TableChange.AddIndex addIndex) { + validateIndexName(addIndex.getName()); StringBuilder sqlBuilder = new StringBuilder(); - sqlBuilder - .append("ALTER TABLE ") - .append(PG_QUOTE) - .append(tableName) - .append(PG_QUOTE) - .append(" ADD CONSTRAINT ") - .append(PG_QUOTE) - .append(addIndex.getName()) - .append(PG_QUOTE); + sqlBuilder.append("ALTER TABLE ").append(quotedIdentifier(tableName)).append(" ADD"); + if (StringUtils.isNotEmpty(addIndex.getName())) { + sqlBuilder.append(" CONSTRAINT ").append(quotedIdentifier(addIndex.getName())); + } switch (addIndex.getType()) { case PRIMARY_KEY: sqlBuilder.append(" PRIMARY KEY "); @@ -479,25 +460,17 @@ public class PostgreSqlTableOperations extends JdbcTableOperations String col = updateColumnNullability.fieldName()[0]; if (updateColumnNullability.nullable()) { return ALTER_TABLE - + PG_QUOTE - + tableName - + PG_QUOTE + + quotedIdentifier(tableName) + " " + ALTER_COLUMN - + PG_QUOTE - + col - + PG_QUOTE + + quotedIdentifier(col) + " DROP NOT NULL;"; } else { return ALTER_TABLE - + PG_QUOTE - + tableName - + PG_QUOTE + + quotedIdentifier(tableName) + " " + ALTER_COLUMN - + PG_QUOTE - + col - + PG_QUOTE + + quotedIdentifier(col) + " SET NOT NULL;"; } } @@ -515,9 +488,7 @@ public class PostgreSqlTableOperations extends JdbcTableOperations } } return TABLE_COMMENT - + PG_QUOTE - + jdbcTable.name() - + PG_QUOTE + + quotedIdentifier(jdbcTable.name()) + IS + escapeSqlLiteral(newComment, '\'') + "';"; @@ -539,13 +510,9 @@ public class PostgreSqlTableOperations extends JdbcTableOperations } } return ALTER_TABLE - + PG_QUOTE - + table.name() - + PG_QUOTE + + quotedIdentifier(table.name()) + " DROP COLUMN " - + PG_QUOTE - + deleteColumn.fieldName()[0] - + PG_QUOTE + + quotedIdentifier(deleteColumn.fieldName()[0]) + ";"; } @@ -565,14 +532,11 @@ public class PostgreSqlTableOperations extends JdbcTableOperations throw new NoSuchColumnException("Column %s does not exist.", col); } - StringBuilder sqlBuilder = - new StringBuilder(ALTER_TABLE + PG_QUOTE + jdbcTable.name() + PG_QUOTE); + StringBuilder sqlBuilder = new StringBuilder(ALTER_TABLE + quotedIdentifier(jdbcTable.name())); sqlBuilder .append("\n") .append(ALTER_COLUMN) - .append(PG_QUOTE) - .append(col) - .append(PG_QUOTE) + .append(quotedIdentifier(col)) .append(" SET DEFAULT ") .append( columnDefaultValueConverter.fromGravitino( @@ -595,23 +559,18 @@ public class PostgreSqlTableOperations extends JdbcTableOperations if (null == column) { throw new NoSuchColumnException("Column %s does not exist.", col); } - StringBuilder sqlBuilder = - new StringBuilder(ALTER_TABLE + PG_QUOTE + jdbcTable.name() + PG_QUOTE); + StringBuilder sqlBuilder = new StringBuilder(ALTER_TABLE + quotedIdentifier(jdbcTable.name())); sqlBuilder .append("\n") .append(ALTER_COLUMN) - .append(PG_QUOTE) - .append(col) - .append(PG_QUOTE) + .append(quotedIdentifier(col)) .append(" SET DATA TYPE ") .append(typeConverter.fromGravitino(updateColumnType.getNewDataType())); if (!column.nullable()) { sqlBuilder .append(",\n") .append(ALTER_COLUMN) - .append(PG_QUOTE) - .append(col) - .append(PG_QUOTE) + .append(quotedIdentifier(col)) .append(" SET NOT NULL"); } return sqlBuilder.append(";").toString(); @@ -623,19 +582,13 @@ public class PostgreSqlTableOperations extends JdbcTableOperations throw new UnsupportedOperationException(POSTGRESQL_NOT_SUPPORT_NESTED_COLUMN_MSG); } return ALTER_TABLE - + PG_QUOTE - + tableName - + PG_QUOTE + + quotedIdentifier(tableName) + " RENAME COLUMN " - + PG_QUOTE - + renameColumn.fieldName()[0] - + PG_QUOTE + + quotedIdentifier(renameColumn.fieldName()[0]) + SPACE + "TO" + SPACE - + PG_QUOTE - + renameColumn.getNewName() - + PG_QUOTE + + quotedIdentifier(renameColumn.getNewName()) + ";"; } @@ -650,14 +603,10 @@ public class PostgreSqlTableOperations extends JdbcTableOperations StringBuilder columnDefinition = new StringBuilder(); columnDefinition .append(ALTER_TABLE) - .append(PG_QUOTE) - .append(lazyLoadTable.name()) - .append(PG_QUOTE) + .append(quotedIdentifier(lazyLoadTable.name())) .append(SPACE) .append("ADD COLUMN ") - .append(PG_QUOTE) - .append(col) - .append(PG_QUOTE) + .append(quotedIdentifier(col)) .append(SPACE) .append(typeConverter.fromGravitino(addColumn.getDataType())) .append(SPACE); @@ -697,13 +646,9 @@ public class PostgreSqlTableOperations extends JdbcTableOperations if (StringUtils.isNotEmpty(addColumn.getComment())) { result.add( COLUMN_COMMENT - + PG_QUOTE - + lazyLoadTable.name() - + PG_QUOTE + + quotedIdentifier(lazyLoadTable.name()) + "." - + PG_QUOTE - + col - + PG_QUOTE + + quotedIdentifier(col) + IS + escapeSqlLiteral(addColumn.getComment(), '\'') + "';"); @@ -719,13 +664,9 @@ public class PostgreSqlTableOperations extends JdbcTableOperations } String col = updateColumnComment.fieldName()[0]; return COLUMN_COMMENT - + PG_QUOTE - + tableName - + PG_QUOTE + + quotedIdentifier(tableName) + "." - + PG_QUOTE - + col - + PG_QUOTE + + quotedIdentifier(col) + IS + escapeSqlLiteral(newComment, '\'') + "';"; @@ -744,6 +685,29 @@ public class PostgreSqlTableOperations extends JdbcTableOperations return metaData.getPrimaryKeys(metaData.getConnection().getCatalog(), schemaName, tableName); } + /** + * {@inheritDoc} + * + * <p>PostgreSQL uses the duplicate-relation SQLSTATE for index and constraint name conflicts as + * well as table conflicts. During an alter this is an invalid change, not an existing table. + */ + @Override + public void alterTable(String schema, String tableName, TableChange... changes) { + try { + super.alterTable(schema, tableName, changes); + } catch (TableAlreadyExistsException e) { + throw new IllegalArgumentException( + "PostgreSQL relation name conflict while altering table " + + schema + + "." + + tableName + + ": an index or constraint may already use this name. Choose an unused name. " + + "PostgreSQL reason: " + + e.getMessage(), + e); + } + } + @Override protected Connection getConnection(String schema) throws SQLException { Connection connection = dataSource.getConnection(); @@ -752,18 +716,37 @@ public class PostgreSqlTableOperations extends JdbcTableOperations return connection; } + /** {@inheritDoc} */ + @Override + protected ResultSet getTables(Connection connection) throws SQLException { + DatabaseMetaData metadata = connection.getMetaData(); + return metadata.getTables( + connection.getCatalog(), + metadataPattern(metadata, connection.getSchema()), + null, + JdbcConnectorUtils.getTableTypes()); + } + @Override protected ResultSet getTable(Connection connection, String schema, String tableName) throws SQLException { DatabaseMetaData metaData = connection.getMetaData(); - return metaData.getTables(connection.getCatalog(), schema, tableName, null); + return metaData.getTables( + connection.getCatalog(), + metadataPattern(metaData, schema), + metadataPattern(metaData, tableName), + null); } @Override protected ResultSet getColumns(Connection connection, String schema, String tableName) throws SQLException { DatabaseMetaData metaData = connection.getMetaData(); - return metaData.getColumns(connection.getCatalog(), schema, tableName, null); + return metaData.getColumns( + connection.getCatalog(), + metadataPattern(metaData, schema), + metadataPattern(metaData, tableName), + null); } @Override @@ -779,4 +762,27 @@ public class PostgreSqlTableOperations extends JdbcTableOperations return null; } } + + private static String quotedIdentifier(String name) { + return PG_QUOTE + name.replace(PG_QUOTE, PG_QUOTE + PG_QUOTE) + PG_QUOTE; + } + + private static void validateIndexName(@Nullable String name) { + if (name != null && name.getBytes(StandardCharsets.UTF_8).length > 63) { + throw new IllegalArgumentException( + "PostgreSQL index name must not exceed 63 UTF-8 bytes: " + name); + } + } + + @Nullable + private static String metadataPattern(DatabaseMetaData metadata, @Nullable String name) + throws SQLException { + if (name == null) { + return null; + } + String escape = metadata.getSearchStringEscape(); + return name.replace(escape, escape + escape) + .replace("%", escape + "%") + .replace("_", escape + "_"); + } } diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/TestPostgreSqlCatalogCapability.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/TestPostgreSqlCatalogCapability.java index f0acb184da..6f33006344 100644 --- a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/TestPostgreSqlCatalogCapability.java +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/TestPostgreSqlCatalogCapability.java @@ -27,6 +27,37 @@ public class TestPostgreSqlCatalogCapability { private final PostgreSqlCatalogCapability capability = new PostgreSqlCatalogCapability(); + @Test + void testNativeDeletionNamesRemainDistinctFromCreationNames() { + for (Capability.Scope scope : + new Capability.Scope[] {Capability.Scope.TABLE, Capability.Scope.SCHEMA}) { + Assertions.assertTrue( + capability.specificationOnExistingName(scope, "native\"name").supported()); + Assertions.assertFalse(capability.specificationOnName(scope, "native\"name").supported()); + Assertions.assertFalse( + capability.specificationOnExistingName(scope, "bad\0name").supported()); + Assertions.assertFalse(capability.specificationOnExistingName(scope, "").supported()); + Assertions.assertFalse( + capability.specificationOnExistingName(scope, "表".repeat(22)).supported()); + } + Assertions.assertFalse( + capability.specificationOnExistingName(Capability.Scope.SCHEMA, "pg_catalog").supported()); + Assertions.assertFalse( + capability.specificationOnExistingName(Capability.Scope.TABLE, "*").supported()); + } + + @Test + void testNamesAreLimitedByUtf8Bytes() { + for (Capability.Scope scope : Capability.Scope.values()) { + Assertions.assertTrue(capability.specificationOnName(scope, "表".repeat(21)).supported()); + Assertions.assertFalse( + capability.specificationOnName(scope, "表".repeat(21) + "a").supported()); + Assertions.assertTrue( + capability.specificationOnName(scope, "é".repeat(31) + "a").supported()); + Assertions.assertFalse(capability.specificationOnName(scope, "é".repeat(32)).supported()); + } + } + @Test void testValidNames() { // testing valid names for all scopes @@ -178,6 +209,14 @@ public class TestPostgreSqlCatalogCapability { Assertions.assertTrue(result.unsupportedMessage().contains("illegal")); } + @Test + void testByteLimitDiagnostic() { + CapabilityResult result = + capability.specificationOnName(Capability.Scope.TABLE, "表".repeat(22)); + Assertions.assertFalse(result.supported()); + Assertions.assertTrue(result.unsupportedMessage().contains("63 UTF-8 bytes")); + } + @Test void testMixedValidAndInvalidCharacters() { // names that start valid but contain invalid characters should be rejected diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlSchemaOperations.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlSchemaOperations.java index ff557b38ec..f59d7259a2 100644 --- a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlSchemaOperations.java +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlSchemaOperations.java @@ -39,6 +39,32 @@ import org.junit.jupiter.api.Test; @Tag("gravitino-docker-test") public class TestPostgreSqlSchemaOperations extends TestPostgreSql { + @Test + void testQuotedSchemaNamesLoadWithoutExecutingSql() throws SQLException { + for (String schema : new String[] {"quoted';CREATE SCHEMA injected_marker;--"}) { + DATABASE_OPERATIONS.create(schema, "safe comment", Map.of()); + try { + Assertions.assertEquals("safe comment", DATABASE_OPERATIONS.load(schema).comment()); + Assertions.assertEquals(schema, DATABASE_OPERATIONS.load(schema).name()); + Assertions.assertFalse(DATABASE_OPERATIONS.listDatabases().contains("injected_marker")); + } finally { + DATABASE_OPERATIONS.delete(schema, true); + } + } + } + + @Test + void testBackslashSchemaNameLoads() { + String schema = "back\\slash_%"; + DATABASE_OPERATIONS.create(schema, "comment", Map.of()); + try { + Assertions.assertEquals(schema, DATABASE_OPERATIONS.load(schema).name()); + Assertions.assertEquals("comment", DATABASE_OPERATIONS.load(schema).comment()); + } finally { + DATABASE_OPERATIONS.delete(schema, true); + } + } + @Test public void testBaseOperationSchema() { String databaseName = RandomNameUtils.genRandomName("ct_db"); diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlSchemaOperationsSqlGeneration.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlSchemaOperationsSqlGeneration.java index c961b49537..9251ecf93a 100644 --- a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlSchemaOperationsSqlGeneration.java +++ b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlSchemaOperationsSqlGeneration.java @@ -18,7 +18,20 @@ */ package org.apache.gravitino.catalog.postgresql.operation; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.sql.Connection; +import java.sql.DatabaseMetaData; +import java.sql.PreparedStatement; +import java.sql.ResultSet; import java.util.Collections; +import javax.sql.DataSource; +import org.apache.gravitino.catalog.jdbc.config.JdbcConfig; +import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlExceptionConverter; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -37,4 +50,37 @@ public class TestPostgreSqlSchemaOperationsSqlGeneration { + "IS E'owner\\\\''s comment; DROP SCHEMA marker; --'", sql); } + + @Test + void testSchemaNameIsBoundAsData() throws Exception { + DataSource source = mock(DataSource.class); + Connection connection = mock(Connection.class); + DatabaseMetaData metadata = mock(DatabaseMetaData.class); + ResultSet schemas = mock(ResultSet.class); + PreparedStatement statement = mock(PreparedStatement.class); + ResultSet comments = mock(ResultSet.class); + String name = "a\\b_%'; SELECT 1; --"; + when(source.getConnection()).thenReturn(connection); + when(connection.getMetaData()).thenReturn(metadata); + when(metadata.getSearchStringEscape()).thenReturn("\\"); + when(connection.getCatalog()).thenReturn("db"); + when(metadata.getSchemas(eq("db"), anyString())).thenReturn(schemas); + when(schemas.next()).thenReturn(true, false); + when(schemas.getString(1)).thenReturn(name); + when(connection.prepareStatement(anyString())).thenReturn(statement); + when(statement.executeQuery()).thenReturn(comments); + PostgreSqlSchemaOperations operations = new PostgreSqlSchemaOperations(); + operations.initialize( + source, + new PostgreSqlExceptionConverter(), + Collections.singletonMap(JdbcConfig.JDBC_DATABASE.getKey(), "db")); + Assertions.assertEquals(name, operations.load(name).name()); + verify(metadata).getSchemas("db", "a\\\\b\\_\\%'; SELECT 1; --"); + verify(statement).setString(1, name); + verify(connection) + .prepareStatement( + "SELECT obj_description(n.oid, 'pg_namespace') AS comment\n" + + "FROM pg_catalog.pg_namespace n\nWHERE n.nspname = ?;\n"); + verify(statement).close(); + } } diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperations.java index 6dfa17fe6e..1db20ecf54 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 @@ -39,12 +39,14 @@ import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlColumnDefault import org.apache.gravitino.catalog.postgresql.converter.PostgreSqlTypeConverter; import org.apache.gravitino.exceptions.GravitinoRuntimeException; import org.apache.gravitino.exceptions.NoSuchTableException; +import org.apache.gravitino.exceptions.TableAlreadyExistsException; import org.apache.gravitino.rel.Column; import org.apache.gravitino.rel.TableChange; import org.apache.gravitino.rel.expressions.distributions.Distributions; import org.apache.gravitino.rel.expressions.literals.Literals; import org.apache.gravitino.rel.expressions.transforms.Transforms; import org.apache.gravitino.rel.indexes.Index; +import org.apache.gravitino.rel.indexes.Index.IndexType; import org.apache.gravitino.rel.indexes.Indexes; import org.apache.gravitino.rel.types.Type; import org.apache.gravitino.rel.types.Types; @@ -61,6 +63,140 @@ public class TestPostgreSqlTableOperations extends TestPostgreSql { private static Type VARCHAR = Types.VarCharType.of(255); private static Type INT = Types.IntegerType.get(); + @ParameterizedTest + @ValueSource(strings = {"quoted\";CREATE TABLE injected_marker (id int);--"}) + void testQuotedTableNamesStayIdentifiersDuringAlter(String name) { + TABLE_OPERATIONS.create( + TEST_DB_NAME, + name, + new JdbcColumn[] { + JdbcColumn.builder().withName("id").withType(INT).withNullable(true).build() + }, + "comment", + Map.of(), + null, + Distributions.NONE, + Indexes.EMPTY_INDEXES); + try { + TABLE_OPERATIONS.alterTable( + TEST_DB_NAME, name, TableChange.renameColumn(new String[] {"id"}, "id\"quoted")); + Assertions.assertEquals( + "id\"quoted", TABLE_OPERATIONS.load(TEST_DB_NAME, name).columns()[0].name()); + TABLE_OPERATIONS.alterTable(TEST_DB_NAME, name, TableChange.updateComment("updated")); + Assertions.assertEquals("updated", TABLE_OPERATIONS.load(TEST_DB_NAME, name).comment()); + Assertions.assertFalse(TABLE_OPERATIONS.listTables(TEST_DB_NAME).contains("injected_marker")); + } finally { + TABLE_OPERATIONS.drop(TEST_DB_NAME, name); + } + } + + @Test + void testIndexNameConflictDoesNotReportExistingTable() throws SQLException { + String table = RandomNameUtils.genRandomName("index_conflict"); + String index = RandomNameUtils.genRandomName("index_name"); + try (Connection connection = DATA_SOURCE.getConnection()) { + JdbcConnectorUtils.executeUpdate( + connection, "CREATE TABLE " + TEST_DB_NAME + "." + table + "(id int, other int)"); + try { + TABLE_OPERATIONS.alterTable( + TEST_DB_NAME, + table, + TableChange.addIndex(IndexType.UNIQUE_KEY, index, new String[][] {{"id"}})); + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + TABLE_OPERATIONS.alterTable( + TEST_DB_NAME, + table, + TableChange.addIndex( + IndexType.UNIQUE_KEY, index, new String[][] {{"other"}}))); + Assertions.assertTrue(error.getMessage().contains(index)); + Assertions.assertTrue(error.getMessage().contains("index or constraint")); + Assertions.assertTrue(error.getCause() instanceof TableAlreadyExistsException); + Assertions.assertEquals(1, TABLE_OPERATIONS.load(TEST_DB_NAME, table).index().length); + } finally { + TABLE_OPERATIONS.drop(TEST_DB_NAME, table); + } + } + } + + @Test + void testUnnamedUniqueIndexesDoNotCollideAcrossTables() { + String first = "quoted\"" + RandomNameUtils.genRandomName("unnamed_first"); + String second = RandomNameUtils.genRandomName("unnamed_second"); + for (String name : new String[] {first, second}) { + TABLE_OPERATIONS.create( + TEST_DB_NAME, + name, + new JdbcColumn[] { + JdbcColumn.builder().withName("id").withType(INT).withNullable(true).build() + }, + null, + Map.of(), + null, + Distributions.NONE, + Indexes.EMPTY_INDEXES); + } + try { + for (String name : new String[] {first, second}) { + TABLE_OPERATIONS.alterTable( + TEST_DB_NAME, + name, + TableChange.addIndex(IndexType.UNIQUE_KEY, null, new String[][] {{"id"}})); + JdbcTable table = TABLE_OPERATIONS.load(TEST_DB_NAME, name); + Assertions.assertEquals(1, table.index().length); + Assertions.assertNotEquals("null", table.index()[0].name()); + } + } finally { + TABLE_OPERATIONS.drop(TEST_DB_NAME, first); + TABLE_OPERATIONS.drop(TEST_DB_NAME, second); + } + } + + @Test + void testListTablesEscapesSchemaPattern() throws SQLException { + String schema = "list\\schema_%"; + String neighbor = "listXschema_neighbor"; + try (Connection connection = DATA_SOURCE.getConnection()) { + JdbcConnectorUtils.executeUpdate(connection, "CREATE SCHEMA \"" + schema + "\""); + JdbcConnectorUtils.executeUpdate(connection, "CREATE SCHEMA " + neighbor); + try { + JdbcConnectorUtils.executeUpdate( + connection, "CREATE TABLE \"" + schema + "\".actual_table(id int)"); + JdbcConnectorUtils.executeUpdate( + connection, "CREATE TABLE " + neighbor + ".wrong_table(id int)"); + Assertions.assertEquals(List.of("actual_table"), TABLE_OPERATIONS.listTables(schema)); + } finally { + JdbcConnectorUtils.executeUpdate(connection, "DROP SCHEMA \"" + schema + "\" CASCADE"); + JdbcConnectorUtils.executeUpdate(connection, "DROP SCHEMA " + neighbor + " CASCADE"); + } + } + } + + @Test + void testBackslashTableNameLoadsAndAlters() { + String name = "back\\slash_%"; + TABLE_OPERATIONS.create( + TEST_DB_NAME, + name, + new JdbcColumn[] { + JdbcColumn.builder().withName("id").withType(INT).withNullable(true).build() + }, + "comment", + Map.of(), + null, + Distributions.NONE, + Indexes.EMPTY_INDEXES); + try { + Assertions.assertEquals("id", TABLE_OPERATIONS.load(TEST_DB_NAME, name).columns()[0].name()); + TABLE_OPERATIONS.alterTable(TEST_DB_NAME, name, TableChange.updateComment("updated")); + Assertions.assertEquals("updated", TABLE_OPERATIONS.load(TEST_DB_NAME, name).comment()); + } finally { + TABLE_OPERATIONS.drop(TEST_DB_NAME, name); + } + } + @Test public void testOperationTable() { String tableName = RandomNameUtils.genRandomName("op_table_"); diff --git a/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java b/catalogs/catalog-jdbc-postgresql/src/test/java/org/apache/gravitino/catalog/postgresql/operation/TestPostgreSqlTableOperationsSqlGeneration.java index 00958b7e93..3f2e81eaa7 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 @@ -29,6 +29,8 @@ import org.apache.gravitino.rel.TableChange; import org.apache.gravitino.rel.expressions.distributions.Distributions; import org.apache.gravitino.rel.expressions.literals.Literals; import org.apache.gravitino.rel.expressions.transforms.Transforms; +import org.apache.gravitino.rel.indexes.Index; +import org.apache.gravitino.rel.indexes.Index.IndexType; import org.apache.gravitino.rel.indexes.Indexes; import org.apache.gravitino.rel.types.Types; import org.junit.jupiter.api.Assertions; @@ -81,6 +83,79 @@ public class TestPostgreSqlTableOperationsSqlGeneration { } } + @Test + void testQuotedTableAndColumnNamesCannotEscapeIdentifiers() { + TestablePostgreSqlTableOperations ops = new TestablePostgreSqlTableOperations(); + String name = "t\"; DROP TABLE victim; --"; + String quoted = "\"t\"\"; DROP TABLE victim; --\""; + Assertions.assertEquals("DROP TABLE " + quoted, ops.generateDropTableSql(name)); + Assertions.assertEquals( + "ALTER TABLE " + quoted + " RENAME TO " + quoted, ops.generateRenameTableSql(name, name)); + Assertions.assertEquals( + "ALTER TABLE " + quoted + " RENAME COLUMN " + quoted + " TO " + quoted + ";", + ops.alterTableSql(name, TableChange.renameColumn(new String[] {name}, name))); + Assertions.assertEquals( + "ALTER TABLE " + quoted + " DROP CONSTRAINT " + quoted + ";", + ops.alterTableSql(name, TableChange.deleteIndex(name, false))); + Assertions.assertEquals( + quoted, PostgreSqlTableOperations.getIndexFieldStr(new String[][] {{name}})); + String sql = + ops.createTableSql( + name, + new JdbcColumn[] { + JdbcColumn.builder() + .withName(name) + .withType(Types.IntegerType.get()) + .withNullable(true) + .build() + }); + Assertions.assertTrue(sql.startsWith("CREATE TABLE " + quoted + " (")); + Assertions.assertTrue(sql.contains(" " + quoted + " int4")); + } + + @Test + void testOversizedIndexNamesAreRejectedOnCreateAndAlter() { + String name = "表".repeat(22); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + PostgreSqlTableOperations.addIndexDefinition( + "t", + (TableChange.AddIndex) + TableChange.addIndex(IndexType.UNIQUE_KEY, name, new String[][] {{"col1"}}))); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + PostgreSqlTableOperations.appendIndexesSql( + new Index[] {Indexes.unique(name, new String[][] {{"col1"}})}, + new StringBuilder())); + } + + @Test + void testUnnamedIndexesKeepNativeTableAndColumnQuotes() { + for (String name : new String[] {null, ""}) { + Assertions.assertEquals( + "ALTER TABLE \"native\"\"table\" ADD UNIQUE (\"quoted\"\"column\");", + PostgreSqlTableOperations.addIndexDefinition( + "native\"table", + (TableChange.AddIndex) + TableChange.addIndex( + IndexType.UNIQUE_KEY, name, new String[][] {{"quoted\"column"}}))); + } + } + + @Test + void testUnnamedIndexesUsePostgreSqlGeneratedNames() { + for (String name : new String[] {null, ""}) { + Assertions.assertEquals( + "ALTER TABLE \"t\" ADD UNIQUE (\"col1\");", + PostgreSqlTableOperations.addIndexDefinition( + "t", + (TableChange.AddIndex) + TableChange.addIndex(IndexType.UNIQUE_KEY, name, new String[][] {{"col1"}}))); + } + } + @ParameterizedTest @ValueSource(booleans = {false, true}) void testDeleteIndexesDropsOnlyConstraints(boolean ifExists) { diff --git a/core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java index 12fd1cb626..9becd180f8 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java @@ -22,11 +22,13 @@ import static org.apache.gravitino.catalog.CapabilityHelpers.applyCapabilities; import static org.apache.gravitino.catalog.CapabilityHelpers.applyCaseSensitive; import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; +import com.google.common.base.Preconditions; import java.util.Map; import org.apache.commons.lang3.ArrayUtils; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.connector.capability.CapabilityResult; import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NoSuchTableException; import org.apache.gravitino.exceptions.TableAlreadyExistsException; @@ -134,6 +136,17 @@ public class TableNormalizeDispatcher implements TableDispatcher { private NameIdentifier normalizeNameIdentifier(NameIdentifier tableIdent) { Capability capability = getCapability(tableIdent, catalogManager); - return applyCapabilities(tableIdent, Capability.Scope.TABLE, capability); + Namespace namespace = tableIdent.namespace(); + validateExistingName( + Capability.Scope.SCHEMA, namespace.level(namespace.length() - 1), capability); + validateExistingName(Capability.Scope.TABLE, tableIdent.name(), capability); + return applyCaseSensitive(tableIdent, Capability.Scope.TABLE, capability); + } + + private static void validateExistingName( + Capability.Scope scope, String name, Capability capability) { + CapabilityResult result = capability.specificationOnExistingName(scope, name); + Preconditions.checkArgument( + result.supported(), result.unsupportedMessage() + " Illegal name: " + name); } } diff --git a/core/src/main/java/org/apache/gravitino/connector/capability/Capability.java b/core/src/main/java/org/apache/gravitino/connector/capability/Capability.java index f787a16d4f..c48e1766ed 100644 --- a/core/src/main/java/org/apache/gravitino/connector/capability/Capability.java +++ b/core/src/main/java/org/apache/gravitino/connector/capability/Capability.java @@ -84,6 +84,21 @@ public interface Capability { return DEFAULT.specificationOnName(scope, name); } + /** + * Checks a table or schema name on an existing-table deletion path. + * + * <p>The default preserves the create-time restrictions. A catalog may accept additional native + * names only when its deletion operations safely quote them. This does not relax creation or + * rename validation. + * + * @param scope the table or schema scope + * @param name the existing name to check + * @return whether the existing name is supported + */ + default CapabilityResult specificationOnExistingName(Scope scope, String name) { + return specificationOnName(scope, name); + } + /** * Check if the metadata entity is fully managed by Gravitino for the passed-in scope. This is * used to determine whether Gravitino should manage the lifecycle of the metadata and related diff --git a/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java b/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java index 1401f990ec..169d8f65fa 100644 --- a/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java @@ -28,6 +28,9 @@ import org.apache.gravitino.MetadataObjects; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.TestColumn; +import org.apache.gravitino.connector.BaseCatalog; +import org.apache.gravitino.connector.capability.Capability; +import org.apache.gravitino.connector.capability.CapabilityResult; import org.apache.gravitino.exceptions.TableAlreadyExistsException; import org.apache.gravitino.rel.Column; import org.apache.gravitino.rel.Table; @@ -49,6 +52,7 @@ import org.apache.gravitino.rel.types.Types; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.mockito.Mockito; public class TestTableNormalizeDispatcher extends TestOperationDispatcher { private static TableNormalizeDispatcher tableNormalizeDispatcher; @@ -65,6 +69,55 @@ public class TestTableNormalizeDispatcher extends TestOperationDispatcher { TestTableOperationDispatcher.schemaOperationDispatcher, catalogManager); } + @Test + void testDeletionCanUseProviderValidatedNativeNamesWithoutRelaxingCreation() throws Exception { + TableDispatcher delegate = Mockito.mock(TableDispatcher.class); + CatalogManager manager = Mockito.mock(CatalogManager.class); + BaseCatalog<?> catalog = Mockito.mock(BaseCatalog.class); + Capability capability = + new Capability() { + @Override + public CapabilityResult specificationOnExistingName(Scope scope, String name) { + return CapabilityResult.SUPPORTED; + } + }; + Mockito.when(catalog.capability()).thenReturn(capability); + CatalogTestUtils.mockDoWithCatalog(manager, catalog); + TableNormalizeDispatcher dispatcher = new TableNormalizeDispatcher(delegate, manager); + NameIdentifier nativeIdent = + NameIdentifier.of("metalake", "catalog", "native schema", "native\"table"); + dispatcher.dropTable(nativeIdent); + dispatcher.purgeTable(nativeIdent); + Mockito.verify(delegate).dropTable(nativeIdent); + Mockito.verify(delegate).purgeTable(nativeIdent); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + dispatcher.createTable( + nativeIdent, + new Column[0], + null, + Map.of(), + new Transform[0], + Distributions.NONE, + new SortOrder[0], + new Index[0])); + } + + @Test + void testDeletionKeepsDefaultNameRestrictions() throws Exception { + TableDispatcher delegate = Mockito.mock(TableDispatcher.class); + CatalogManager manager = Mockito.mock(CatalogManager.class); + BaseCatalog<?> catalog = Mockito.mock(BaseCatalog.class); + Mockito.when(catalog.capability()).thenReturn(Capability.DEFAULT); + CatalogTestUtils.mockDoWithCatalog(manager, catalog); + TableNormalizeDispatcher dispatcher = new TableNormalizeDispatcher(delegate, manager); + NameIdentifier invalid = NameIdentifier.of("metalake", "catalog", "schema", "native\"table"); + Assertions.assertThrows(IllegalArgumentException.class, () -> dispatcher.dropTable(invalid)); + Assertions.assertThrows(IllegalArgumentException.class, () -> dispatcher.purgeTable(invalid)); + Mockito.verifyNoInteractions(delegate); + } + @Test public void testNameCaseInsensitive() { Namespace tableNs = Namespace.of(metalake, catalog, "schema81");
