This is an automated email from the ASF dual-hosted git repository. diqiu50 pushed a commit to branch cherry-pick-65d305fda-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 1b4b0487831b8e50a092dfc83c5b1b2427b0f6e6 Author: Yuhui <[email protected]> AuthorDate: Tue Jul 21 14:24:13 2026 +0800 [#12097] improvement(core): Validate names before normalizing in CapabilityHelpers (#12098) ### What changes were proposed in this pull request? Reorder validation and normalization in `CapabilityHelpers` so `applyNameSpecification` runs on the original name before `applyCaseSensitiveOn*`/`normalizeName`. Remove the redundant re-normalization of identifiers already returned by `listSchemas`/`listTables` in `SchemaNormalizeDispatcher`/`TableNormalizeDispatcher`. ### Why are the changes needed? For catalogs whose name folding depends on the name's original form (e.g. a quoted identifier keeps case/spaces, an unquoted one gets folded), validating after normalization checks the wrong form of the name. Similarly, re-applying `normalizeName` to identifiers a catalog already returned in canonical form isn't guaranteed to be idempotent for such catalogs and can corrupt an already-correct name. Fix: #12097 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Added a unit test; existing core module tests pass. (cherry picked from commit 65d305fdaa380b6d629963ab807124cfda1d54da) --- .../gravitino/catalog/CapabilityHelpers.java | 26 +++---- .../catalog/FilesetNormalizeDispatcher.java | 17 ++--- .../catalog/FunctionNormalizeDispatcher.java | 17 ++--- .../catalog/ModelNormalizeDispatcher.java | 17 ++--- .../catalog/PartitionNormalizeDispatcher.java | 22 +++--- .../catalog/SchemaNormalizeDispatcher.java | 19 ++--- .../catalog/TableNormalizeDispatcher.java | 17 ++--- .../catalog/TopicNormalizeDispatcher.java | 17 ++--- .../gravitino/catalog/ViewNormalizeDispatcher.java | 16 ++--- .../gravitino/catalog/TestCapabilityHelpers.java | 50 +++++++++++++ .../catalog/TestPartitionNormalizeDispatcher.java | 72 +++++++++++++++++++ .../catalog/TestTableNormalizeDispatcher.java | 82 ++++++++++++++++++++++ 12 files changed, 262 insertions(+), 110 deletions(-) diff --git a/core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java b/core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java index d6d0a90e7e..eae018c2c0 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java +++ b/core/src/main/java/org/apache/gravitino/catalog/CapabilityHelpers.java @@ -252,9 +252,8 @@ public class CapabilityHelpers { } private static String[] applyCapabilities(String[] fieldName, Capability capabilities) { - String[] sensitiveOnColumnName = applyCaseSensitiveOnColumnName(fieldName, capabilities); - applyNameSpecification(Capability.Scope.COLUMN, sensitiveOnColumnName[0], capabilities); - return sensitiveOnColumnName; + applyNameSpecification(Capability.Scope.COLUMN, fieldName[0], capabilities); + return applyCaseSensitiveOnColumnName(fieldName, capabilities); } private static Transform applyCapabilities(Transform transform, Capability capabilities) { @@ -343,38 +342,42 @@ public class CapabilityHelpers { private static FilesetChange applyCapabilities( FilesetChange.RenameFileset renameFileset, Capability capabilities) { + applyNameSpecification(Capability.Scope.FILESET, renameFileset.getNewName(), capabilities); String newName = applyCaseSensitiveOnName( Capability.Scope.FILESET, renameFileset.getNewName(), capabilities); - applyNameSpecification(Capability.Scope.FILESET, newName, capabilities); return FilesetChange.rename(newName); } private static ViewChange applyCapabilities( ViewChange.RenameView renameView, Capability capabilities) { + applyNameSpecification(Capability.Scope.VIEW, renameView.getNewName(), capabilities); String newName = applyCaseSensitiveOnName(Capability.Scope.VIEW, renameView.getNewName(), capabilities); - applyNameSpecification(Capability.Scope.VIEW, newName, capabilities); return ViewChange.rename(newName); } private static TableChange applyCapabilities( TableChange.RenameTable renameTable, Capability capabilities) { + applyNameSpecification(Capability.Scope.TABLE, renameTable.getNewName(), capabilities); String newName = applyCaseSensitiveOnName(Capability.Scope.TABLE, renameTable.getNewName(), capabilities); String newSchemaName = renameTable .getNewSchemaName() - .map(s -> applyCaseSensitiveOnName(Capability.Scope.SCHEMA, s, capabilities)) + .map( + s -> { + applyNameSpecification(Capability.Scope.SCHEMA, s, capabilities); + return applyCaseSensitiveOnName(Capability.Scope.SCHEMA, s, capabilities); + }) .orElse(null); - applyNameSpecification(Capability.Scope.TABLE, newName, capabilities); return TableChange.rename(newName, newSchemaName); } private static TableChange applyCapabilities( TableChange.ColumnChange change, Capability capabilities) { + applyNameSpecification(Capability.Scope.COLUMN, change.fieldName()[0], capabilities); String[] fieldName = applyCaseSensitiveOnColumnName(change.fieldName(), capabilities); - applyNameSpecification(Capability.Scope.COLUMN, fieldName[0], capabilities); if (change instanceof TableChange.AddColumn) { return applyCapabilities((TableChange.AddColumn) change, capabilities); @@ -404,10 +407,10 @@ public class CapabilityHelpers { (TableChange.UpdateColumnPosition) change; if (updateColumnPosition.getPosition() instanceof TableChange.After) { TableChange.After afterPosition = (TableChange.After) updateColumnPosition.getPosition(); + applyNameSpecification(Capability.Scope.COLUMN, afterPosition.getColumn(), capabilities); String afterFieldName = applyCaseSensitiveOnName( Capability.Scope.COLUMN, afterPosition.getColumn(), capabilities); - applyNameSpecification(Capability.Scope.COLUMN, afterFieldName, capabilities); return TableChange.updateColumnPosition( fieldName, TableChange.ColumnPosition.after(afterFieldName)); } @@ -498,9 +501,8 @@ public class CapabilityHelpers { private static String applyCapabilitiesOnName( Capability.Scope scope, String name, Capability capabilities) { - String standardizeName = applyCaseSensitiveOnName(scope, name, capabilities); - applyNameSpecification(scope, standardizeName, capabilities); - return standardizeName; + applyNameSpecification(scope, name, capabilities); + return applyCaseSensitiveOnName(scope, name, capabilities); } public static String applyCaseSensitiveOnName( diff --git a/core/src/main/java/org/apache/gravitino/catalog/FilesetNormalizeDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/FilesetNormalizeDispatcher.java index 44a0c1c729..292edd8d27 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/FilesetNormalizeDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/FilesetNormalizeDispatcher.java @@ -24,7 +24,6 @@ import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; import java.io.IOException; 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; @@ -36,6 +35,10 @@ import org.apache.gravitino.file.FileInfo; import org.apache.gravitino.file.Fileset; import org.apache.gravitino.file.FilesetChange; +/** + * Note on list operations: names returned by list methods (e.g. {@link #listFilesets(Namespace)}) + * are assumed to already be in their canonical, legal form and are not re-normalized here. + */ public class FilesetNormalizeDispatcher implements FilesetDispatcher { private final CatalogManager catalogManager; private final FilesetDispatcher dispatcher; @@ -50,8 +53,7 @@ public class FilesetNormalizeDispatcher implements FilesetDispatcher { // The constraints of the name spec may be more strict than underlying catalog, // and for compatibility reasons, we only apply case-sensitive capabilities here. Namespace caseSensitiveNs = normalizeCaseSensitive(namespace); - NameIdentifier[] identifiers = dispatcher.listFilesets(caseSensitiveNs); - return normalizeCaseSensitive(identifiers); + return dispatcher.listFilesets(caseSensitiveNs); } @Override @@ -125,13 +127,4 @@ public class FilesetNormalizeDispatcher implements FilesetDispatcher { Capability capabilities = getCapability(filesetIdent, catalogManager); return applyCaseSensitive(filesetIdent, Capability.Scope.FILESET, capabilities); } - - private NameIdentifier[] normalizeCaseSensitive(NameIdentifier[] filesetIdents) { - if (ArrayUtils.isEmpty(filesetIdents)) { - return filesetIdents; - } - - Capability capabilities = getCapability(filesetIdents[0], catalogManager); - return applyCaseSensitive(filesetIdents, Capability.Scope.FILESET, capabilities); - } } diff --git a/core/src/main/java/org/apache/gravitino/catalog/FunctionNormalizeDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/FunctionNormalizeDispatcher.java index 810ca4ed5e..03bdf6a0cc 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/FunctionNormalizeDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/FunctionNormalizeDispatcher.java @@ -22,7 +22,6 @@ 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 org.apache.commons.lang3.ArrayUtils; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.connector.capability.Capability; @@ -37,6 +36,10 @@ import org.apache.gravitino.function.FunctionType; /** * {@code FunctionNormalizeDispatcher} normalizes function identifiers and namespaces by applying * case-sensitivity and other naming capabilities before delegating to the underlying dispatcher. + * + * <p>Note on list operations: names returned by list methods (e.g. {@link + * #listFunctions(Namespace)}) are assumed to already be in their canonical, legal form and are not + * re-normalized here. */ public class FunctionNormalizeDispatcher implements FunctionDispatcher { private final CatalogManager catalogManager; @@ -50,8 +53,7 @@ public class FunctionNormalizeDispatcher implements FunctionDispatcher { @Override public NameIdentifier[] listFunctions(Namespace namespace) throws NoSuchSchemaException { Namespace caseSensitiveNs = normalizeCaseSensitive(namespace); - NameIdentifier[] identifiers = dispatcher.listFunctions(caseSensitiveNs); - return normalizeCaseSensitive(identifiers); + return dispatcher.listFunctions(caseSensitiveNs); } @Override @@ -102,15 +104,6 @@ public class FunctionNormalizeDispatcher implements FunctionDispatcher { return applyCaseSensitive(functionIdent, Capability.Scope.FUNCTION, capabilities); } - private NameIdentifier[] normalizeCaseSensitive(NameIdentifier[] functionIdents) { - if (ArrayUtils.isEmpty(functionIdents)) { - return functionIdents; - } - - Capability capabilities = getCapability(functionIdents[0], catalogManager); - return applyCaseSensitive(functionIdents, Capability.Scope.FUNCTION, capabilities); - } - private NameIdentifier normalizeNameIdentifier(NameIdentifier functionIdent) { Capability capability = getCapability(functionIdent, catalogManager); return applyCapabilities(functionIdent, Capability.Scope.FUNCTION, capability); diff --git a/core/src/main/java/org/apache/gravitino/catalog/ModelNormalizeDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/ModelNormalizeDispatcher.java index 5bf32f892b..978680b32b 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/ModelNormalizeDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/ModelNormalizeDispatcher.java @@ -23,7 +23,6 @@ import static org.apache.gravitino.catalog.CapabilityHelpers.applyCaseSensitive; import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; 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; @@ -38,6 +37,10 @@ import org.apache.gravitino.model.ModelChange; import org.apache.gravitino.model.ModelVersion; import org.apache.gravitino.model.ModelVersionChange; +/** + * Note on list operations: names returned by list methods (e.g. {@link #listModels(Namespace)}) are + * assumed to already be in their canonical, legal form and are not re-normalized here. + */ public class ModelNormalizeDispatcher implements ModelDispatcher { private final CatalogManager catalogManager; private final ModelDispatcher dispatcher; @@ -52,8 +55,7 @@ public class ModelNormalizeDispatcher implements ModelDispatcher { // The constraints of the name spec may be more strict than underlying catalog, // and for compatibility reasons, we only apply case-sensitive capabilities here. Namespace caseSensitiveNs = normalizeCaseSensitive(namespace); - NameIdentifier[] identifiers = dispatcher.listModels(caseSensitiveNs); - return normalizeCaseSensitive(identifiers); + return dispatcher.listModels(caseSensitiveNs); } @Override @@ -181,15 +183,6 @@ public class ModelNormalizeDispatcher implements ModelDispatcher { return applyCaseSensitive(ident, Capability.Scope.MODEL, capabilities); } - private NameIdentifier[] normalizeCaseSensitive(NameIdentifier[] idents) { - if (ArrayUtils.isEmpty(idents)) { - return idents; - } - - Capability capabilities = getCapability(idents[0], catalogManager); - return applyCaseSensitive(idents, Capability.Scope.MODEL, capabilities); - } - private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) { Capability capability = getCapability(ident, catalogManager); return applyCapabilities(ident, Capability.Scope.MODEL, capability); diff --git a/core/src/main/java/org/apache/gravitino/catalog/PartitionNormalizeDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/PartitionNormalizeDispatcher.java index 899846fdb7..59f9b27c36 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/PartitionNormalizeDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/PartitionNormalizeDispatcher.java @@ -22,13 +22,17 @@ import static org.apache.gravitino.catalog.CapabilityHelpers.applyCaseSensitive; import static org.apache.gravitino.catalog.CapabilityHelpers.applyCaseSensitiveOnName; import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; -import java.util.Arrays; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.connector.capability.Capability; import org.apache.gravitino.exceptions.NoSuchPartitionException; import org.apache.gravitino.exceptions.PartitionAlreadyExistsException; import org.apache.gravitino.rel.partitions.Partition; +/** + * Note on list operations: names returned by list methods (e.g. {@link + * #listPartitionNames(NameIdentifier)}, {@link #listPartitions(NameIdentifier)}) are assumed to + * already be in their canonical, legal form and are not re-normalized here. + */ public class PartitionNormalizeDispatcher implements PartitionDispatcher { private final CatalogManager catalogManager; private final PartitionDispatcher dispatcher; @@ -42,23 +46,15 @@ public class PartitionNormalizeDispatcher implements PartitionDispatcher { @Override public String[] listPartitionNames(NameIdentifier tableIdent) { Capability capabilities = getCapability(tableIdent, catalogManager); - String[] partitionNames = - dispatcher.listPartitionNames( - applyCaseSensitive(tableIdent, Capability.Scope.TABLE, capabilities)); - return Arrays.stream(partitionNames) - .map( - partitionName -> - applyCaseSensitiveOnName(Capability.Scope.PARTITION, partitionName, capabilities)) - .toArray(String[]::new); + return dispatcher.listPartitionNames( + applyCaseSensitive(tableIdent, Capability.Scope.TABLE, capabilities)); } @Override public Partition[] listPartitions(NameIdentifier tableIdent) { Capability capabilities = getCapability(tableIdent, catalogManager); - Partition[] partitions = - dispatcher.listPartitions( - CapabilityHelpers.applyCaseSensitive(tableIdent, Capability.Scope.TABLE, capabilities)); - return applyCaseSensitive(partitions, capabilities); + return dispatcher.listPartitions( + CapabilityHelpers.applyCaseSensitive(tableIdent, Capability.Scope.TABLE, capabilities)); } @Override diff --git a/core/src/main/java/org/apache/gravitino/catalog/SchemaNormalizeDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/SchemaNormalizeDispatcher.java index b4d55e6cb9..564d777820 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/SchemaNormalizeDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/SchemaNormalizeDispatcher.java @@ -23,7 +23,6 @@ import static org.apache.gravitino.catalog.CapabilityHelpers.applyCaseSensitive; import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; import java.util.Map; -import org.apache.commons.lang3.ArrayUtils; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.Schema; @@ -34,6 +33,10 @@ import org.apache.gravitino.exceptions.NoSuchSchemaException; import org.apache.gravitino.exceptions.NonEmptySchemaException; import org.apache.gravitino.exceptions.SchemaAlreadyExistsException; +/** + * Note on list operations: names returned by list methods (e.g. {@link #listSchemas(Namespace)}) + * are assumed to already be in their canonical, legal form and are not re-normalized here. + */ public class SchemaNormalizeDispatcher implements SchemaDispatcher { private final CatalogManager catalogManager; private final SchemaDispatcher dispatcher; @@ -57,10 +60,7 @@ public class SchemaNormalizeDispatcher implements SchemaDispatcher { NameIdentifier.of(namespace.levels()))); } - NameIdentifier[] identifiers = dispatcher.listSchemas(namespace); - // The constraints of the name spec may be more strict than underlying catalog, - // and for compatibility reasons, we only apply case-sensitive capabilities here. - return normalizeCaseSensitive(identifiers); + return dispatcher.listSchemas(namespace); } @Override @@ -110,13 +110,4 @@ public class SchemaNormalizeDispatcher implements SchemaDispatcher { Capability capabilities = getCapability(schemaIdent, catalogManager); return applyCaseSensitive(schemaIdent, Capability.Scope.SCHEMA, capabilities); } - - private NameIdentifier[] normalizeCaseSensitive(NameIdentifier[] schemaIdents) { - if (ArrayUtils.isEmpty(schemaIdents)) { - return schemaIdents; - } - - Capability capabilities = getCapability(schemaIdents[0], catalogManager); - return applyCaseSensitive(schemaIdents, Capability.Scope.SCHEMA, capabilities); - } } 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..68544ba3cf 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/TableNormalizeDispatcher.java @@ -23,7 +23,6 @@ import static org.apache.gravitino.catalog.CapabilityHelpers.applyCaseSensitive; import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; 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; @@ -38,6 +37,10 @@ import org.apache.gravitino.rel.expressions.sorts.SortOrder; import org.apache.gravitino.rel.expressions.transforms.Transform; import org.apache.gravitino.rel.indexes.Index; +/** + * Note on list operations: names returned by list methods (e.g. {@link #listTables(Namespace)}) are + * assumed to already be in their canonical, legal form and are not re-normalized here. + */ public class TableNormalizeDispatcher implements TableDispatcher { private final CatalogManager catalogManager; private final TableDispatcher dispatcher; @@ -52,8 +55,7 @@ public class TableNormalizeDispatcher implements TableDispatcher { // The constraints of the name spec may be more strict than underlying catalog, // and for compatibility reasons, we only apply case-sensitive capabilities here. Namespace caseSensitiveNs = normalizeCaseSensitive(namespace); - NameIdentifier[] identifiers = dispatcher.listTables(caseSensitiveNs); - return normalizeCaseSensitive(identifiers); + return dispatcher.listTables(caseSensitiveNs); } @Override @@ -123,15 +125,6 @@ public class TableNormalizeDispatcher implements TableDispatcher { return applyCaseSensitive(tableIdent, Capability.Scope.TABLE, capability); } - private NameIdentifier[] normalizeCaseSensitive(NameIdentifier[] tableIdents) { - if (ArrayUtils.isEmpty(tableIdents)) { - return tableIdents; - } - - Capability capabilities = getCapability(tableIdents[0], catalogManager); - return applyCaseSensitive(tableIdents, Capability.Scope.TABLE, capabilities); - } - private NameIdentifier normalizeNameIdentifier(NameIdentifier tableIdent) { Capability capability = getCapability(tableIdent, catalogManager); return applyCapabilities(tableIdent, Capability.Scope.TABLE, capability); diff --git a/core/src/main/java/org/apache/gravitino/catalog/TopicNormalizeDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/TopicNormalizeDispatcher.java index a104b4f671..323d5ee285 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/TopicNormalizeDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/TopicNormalizeDispatcher.java @@ -23,7 +23,6 @@ import static org.apache.gravitino.catalog.CapabilityHelpers.applyCaseSensitive; import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; 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; @@ -34,6 +33,10 @@ import org.apache.gravitino.messaging.DataLayout; import org.apache.gravitino.messaging.Topic; import org.apache.gravitino.messaging.TopicChange; +/** + * Note on list operations: names returned by list methods (e.g. {@link #listTopics(Namespace)}) are + * assumed to already be in their canonical, legal form and are not re-normalized here. + */ public class TopicNormalizeDispatcher implements TopicDispatcher { private final CatalogManager catalogManager; private final TopicDispatcher dispatcher; @@ -48,8 +51,7 @@ public class TopicNormalizeDispatcher implements TopicDispatcher { // The constraints of the name spec may be more strict than underlying catalog, // and for compatibility reasons, we only apply case-sensitive capabilities here. Namespace caseSensitiveNs = normalizeCaseSensitive(namespace); - NameIdentifier[] identifiers = dispatcher.listTopics(caseSensitiveNs); - return normalizeCaseSensitive(identifiers); + return dispatcher.listTopics(caseSensitiveNs); } @Override @@ -98,15 +100,6 @@ public class TopicNormalizeDispatcher implements TopicDispatcher { return applyCaseSensitive(topicIdent, Capability.Scope.TOPIC, capabilities); } - private NameIdentifier[] normalizeCaseSensitive(NameIdentifier[] topicIdents) { - if (ArrayUtils.isEmpty(topicIdents)) { - return topicIdents; - } - - Capability capabilities = getCapability(topicIdents[0], catalogManager); - return applyCaseSensitive(topicIdents, Capability.Scope.TOPIC, capabilities); - } - private NameIdentifier normalizeNameIdentifier(NameIdentifier topicIdent) { Capability capability = getCapability(topicIdent, catalogManager); return applyCapabilities(topicIdent, Capability.Scope.TOPIC, capability); diff --git a/core/src/main/java/org/apache/gravitino/catalog/ViewNormalizeDispatcher.java b/core/src/main/java/org/apache/gravitino/catalog/ViewNormalizeDispatcher.java index a73f2cf6a3..5c786a1ce1 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/ViewNormalizeDispatcher.java +++ b/core/src/main/java/org/apache/gravitino/catalog/ViewNormalizeDispatcher.java @@ -24,7 +24,6 @@ import static org.apache.gravitino.catalog.CapabilityHelpers.getCapability; import java.util.Map; import javax.annotation.Nullable; -import org.apache.commons.lang3.ArrayUtils; import org.apache.gravitino.NameIdentifier; import org.apache.gravitino.Namespace; import org.apache.gravitino.connector.capability.Capability; @@ -36,6 +35,10 @@ import org.apache.gravitino.rel.Representation; import org.apache.gravitino.rel.View; import org.apache.gravitino.rel.ViewChange; +/** + * Note on list operations: names returned by list methods (e.g. {@link #listViews(Namespace)}) are + * assumed to already be in their canonical, legal form and are not re-normalized here. + */ public class ViewNormalizeDispatcher implements ViewDispatcher { private final CatalogManager catalogManager; @@ -51,8 +54,7 @@ public class ViewNormalizeDispatcher implements ViewDispatcher { // The constraints of the name spec may be more strict than underlying catalog, // and for compatibility reasons, we only apply case-sensitive capabilities here. Namespace caseSensitiveNs = normalizeCaseSensitive(namespace); - NameIdentifier[] identifiers = dispatcher.listViews(caseSensitiveNs); - return normalizeCaseSensitive(identifiers); + return dispatcher.listViews(caseSensitiveNs); } @Override @@ -108,14 +110,6 @@ public class ViewNormalizeDispatcher implements ViewDispatcher { return applyCaseSensitive(ident, Capability.Scope.VIEW, capabilities); } - private NameIdentifier[] normalizeCaseSensitive(NameIdentifier[] idents) { - if (ArrayUtils.isEmpty(idents)) { - return idents; - } - Capability capabilities = getCapability(idents[0], catalogManager); - return applyCaseSensitive(idents, Capability.Scope.VIEW, capabilities); - } - private NameIdentifier normalizeNameIdentifier(NameIdentifier ident) { Capability capability = getCapability(ident, catalogManager); return applyCapabilities(ident, Capability.Scope.VIEW, capability); diff --git a/core/src/test/java/org/apache/gravitino/catalog/TestCapabilityHelpers.java b/core/src/test/java/org/apache/gravitino/catalog/TestCapabilityHelpers.java index 02c0c051fd..529ee4992c 100644 --- a/core/src/test/java/org/apache/gravitino/catalog/TestCapabilityHelpers.java +++ b/core/src/test/java/org/apache/gravitino/catalog/TestCapabilityHelpers.java @@ -19,6 +19,8 @@ package org.apache.gravitino.catalog; import java.util.Locale; +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.rel.expressions.literals.Literal; @@ -52,6 +54,39 @@ public class TestCapabilityHelpers { } }; + /** + * Mimics Oracle's quoting convention: a quoted name (e.g. {@code "My Table"}) is unquoted with + * its case and embedded spaces preserved, while an unquoted name must match a simple word + * pattern. {@code specificationOnName} must see the name as it was originally supplied (still + * quoted) rather than the already-unquoted result of {@code normalizeName}, or a quoted name with + * a space would be wrongly rejected after normalization even though it was valid as supplied. + */ + static final Capability QUOTE_AWARE_CAPABILITY = + new Capability() { + @Override + public CapabilityResult specificationOnName(Scope scope, String name) { + if (name.startsWith("\"") && name.endsWith("\"") && name.length() >= 2) { + return CapabilityResult.SUPPORTED; + } + return name.matches("^\\w+$") + ? CapabilityResult.SUPPORTED + : CapabilityResult.unsupported("Illegal name: " + name); + } + + @Override + public CapabilityResult caseSensitiveOnName(Scope scope) { + return CapabilityResult.unsupported("folding depends on quoting"); + } + + @Override + public String normalizeName(Scope scope, String name) { + if (name.startsWith("\"") && name.endsWith("\"") && name.length() >= 2) { + return name.substring(1, name.length() - 1); + } + return name.toUpperCase(Locale.ROOT); + } + }; + @Test void testApplyCaseSensitiveOnNameHonorsCustomNormalizeName() { String normalized = @@ -92,4 +127,19 @@ public class TestCapabilityHelpers { Capability.Scope.PARTITION, null, Capability.DEFAULT); Assertions.assertNull(normalized); } + + @Test + void testApplyCapabilitiesValidatesNameBeforeNormalizing() { + // A quoted name with an embedded space is valid as supplied, but its normalized (unquoted) + // form no longer matches the plain-word pattern. specificationOnName must be checked against + // the original name, not the already-normalized one, or this would be wrongly rejected. + NameIdentifier quotedIdent = + NameIdentifier.of(Namespace.of("metalake", "catalog", "schema"), "\"My Table\""); + + NameIdentifier result = + CapabilityHelpers.applyCapabilities( + quotedIdent, Capability.Scope.TABLE, QUOTE_AWARE_CAPABILITY); + + Assertions.assertEquals("My Table", result.name()); + } } diff --git a/core/src/test/java/org/apache/gravitino/catalog/TestPartitionNormalizeDispatcher.java b/core/src/test/java/org/apache/gravitino/catalog/TestPartitionNormalizeDispatcher.java index 0151dcf248..84c9678a66 100644 --- a/core/src/test/java/org/apache/gravitino/catalog/TestPartitionNormalizeDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/catalog/TestPartitionNormalizeDispatcher.java @@ -30,6 +30,8 @@ import org.apache.gravitino.utils.NameIdentifierUtil; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Mockito; public class TestPartitionNormalizeDispatcher extends TestOperationDispatcher { @@ -90,4 +92,74 @@ public class TestPartitionNormalizeDispatcher extends TestOperationDispatcher { partitionNormalizeDispatcher.dropPartition(TABLE, partition.name().toUpperCase())); Assertions.assertFalse(partitionNormalizeDispatcher.partitionExists(TABLE, partition.name())); } + + @Test + public void testAddPartitionListPartitionsGetPartitionRoundTrip() throws Exception { + // Mirrors the correct usage pattern for a quote-aware catalog (e.g. Oracle): + // 1. addPartition is issued with a quoted, case-sensitive name, so the catalog stores the + // physical partition with its case preserved: My Partition. + // 2. listPartitionNames()/listPartitions() surface that physical name unquoted (My + // Partition) -- names are not re-folded by the core layer. + // 3. Getting the partition again requires re-quoting the case-sensitive name + // ("My Partition"), not passing the bare listed name back in: normalizeName cannot tell + // an already-canonical name apart from raw user input. + NameIdentifier tableIdent = + NameIdentifierUtil.ofTable(metalake, catalog, "schema", "quotedTable"); + PartitionDispatcher mockDispatcher = Mockito.mock(PartitionDispatcher.class); + + CatalogManager mockCatalogManager = Mockito.mock(CatalogManager.class); + CatalogManager.CatalogWrapper mockWrapper = Mockito.mock(CatalogManager.CatalogWrapper.class); + Mockito.when(mockWrapper.capabilities()) + .thenReturn(TestCapabilityHelpers.QUOTE_AWARE_CAPABILITY); + Mockito.when(mockCatalogManager.loadCatalogAndWrap(Mockito.any(NameIdentifier.class))) + .thenReturn(mockWrapper); + + PartitionNormalizeDispatcher dispatcher = + new PartitionNormalizeDispatcher(mockDispatcher, mockCatalogManager); + + // 1. Add the partition with a quoted, case-sensitive name. + Partition quotedPartition = + Partitions.identity( + "\"My Partition\"", + new String[][] {{"col1"}}, + new Literal[] {Literals.stringLiteral("v1")}, + Maps.newHashMap()); + Mockito.when(mockDispatcher.addPartition(Mockito.any(NameIdentifier.class), Mockito.any())) + .thenReturn(quotedPartition); + dispatcher.addPartition(tableIdent, quotedPartition); + + ArgumentCaptor<Partition> addedPartitionCaptor = ArgumentCaptor.forClass(Partition.class); + Mockito.verify(mockDispatcher) + .addPartition(Mockito.any(NameIdentifier.class), addedPartitionCaptor.capture()); + String physicalName = addedPartitionCaptor.getValue().name(); + Assertions.assertEquals("My Partition", physicalName); + + // 2. listPartitionNames()/listPartitions() must surface that physical name unchanged. + Partition physicalPartition = + Partitions.identity( + physicalName, + new String[][] {{"col1"}}, + new Literal[] {Literals.stringLiteral("v1")}, + Maps.newHashMap()); + Mockito.when(mockDispatcher.listPartitionNames(Mockito.any(NameIdentifier.class))) + .thenReturn(new String[] {physicalName}); + Mockito.when(mockDispatcher.listPartitions(Mockito.any(NameIdentifier.class))) + .thenReturn(new Partition[] {physicalPartition}); + + String[] listedNames = dispatcher.listPartitionNames(tableIdent); + Assertions.assertEquals(1, listedNames.length); + Assertions.assertEquals("My Partition", listedNames[0]); + + Partition[] listedPartitions = dispatcher.listPartitions(tableIdent); + Assertions.assertEquals(1, listedPartitions.length); + Assertions.assertEquals("My Partition", listedPartitions[0].name()); + + // 3. Getting the partition again requires re-quoting the listed name. + dispatcher.getPartition(tableIdent, "\"" + listedNames[0] + "\""); + + ArgumentCaptor<String> gotPartitionNameCaptor = ArgumentCaptor.forClass(String.class); + Mockito.verify(mockDispatcher) + .getPartition(Mockito.any(NameIdentifier.class), gotPartitionNameCaptor.capture()); + Assertions.assertEquals("My Partition", gotPartitionNameCaptor.getValue()); + } } 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..85649cdea1 100644 --- a/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java +++ b/core/src/test/java/org/apache/gravitino/catalog/TestTableNormalizeDispatcher.java @@ -49,6 +49,8 @@ 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.ArgumentCaptor; +import org.mockito.Mockito; public class TestTableNormalizeDispatcher extends TestOperationDispatcher { private static TableNormalizeDispatcher tableNormalizeDispatcher; @@ -194,6 +196,86 @@ public class TestTableNormalizeDispatcher extends TestOperationDispatcher { "The COLUMN name '*' is reserved. Illegal name: *", exception.getMessage()); } + @Test + public void testCreateTableListTablesLoadTableRoundTrip() throws Exception { + // Mirrors the correct usage pattern for a quote-aware catalog (e.g. Oracle): + // 1. CREATE TABLE "My Table" is issued with a quoted name, so the catalog stores the + // physical table with its case preserved: My Table. + // 2. listTables() surfaces that physical name unquoted (My Table) -- this is the behavior + // this PR's fix guarantees: the name is not re-folded by the core layer. + // 3. Loading the table again requires re-quoting the case-sensitive name ("My Table"), not + // passing the bare listed name back in: normalizeName cannot tell an already-canonical + // name apart from raw user input, so an unquoted "My Table" would be folded to MY TABLE. + Namespace tableNs = Namespace.of(metalake, catalog, "schema"); + TableDispatcher mockDispatcher = Mockito.mock(TableDispatcher.class); + Mockito.when( + mockDispatcher.createTable( + Mockito.any(NameIdentifier.class), + Mockito.any(), + Mockito.any(), + Mockito.any(), + Mockito.any(), + Mockito.any(), + Mockito.any(), + Mockito.any())) + .thenReturn(Mockito.mock(Table.class)); + Mockito.when(mockDispatcher.loadTable(Mockito.any(NameIdentifier.class))) + .thenReturn(Mockito.mock(Table.class)); + + CatalogManager mockCatalogManager = Mockito.mock(CatalogManager.class); + CatalogManager.CatalogWrapper mockWrapper = Mockito.mock(CatalogManager.CatalogWrapper.class); + Mockito.when(mockWrapper.capabilities()) + .thenReturn(TestCapabilityHelpers.QUOTE_AWARE_CAPABILITY); + Mockito.when(mockCatalogManager.loadCatalogAndWrap(Mockito.any(NameIdentifier.class))) + .thenReturn(mockWrapper); + + TableNormalizeDispatcher dispatcher = + new TableNormalizeDispatcher(mockDispatcher, mockCatalogManager); + + // 1. Create the table with a quoted, case-sensitive name. + NameIdentifier quotedCreateIdent = NameIdentifier.of(tableNs, "\"My Table\""); + dispatcher.createTable( + quotedCreateIdent, + new Column[0], + "comment", + ImmutableMap.of(), + new Transform[0], + Distributions.NONE, + new SortOrder[0], + new Index[0]); + + ArgumentCaptor<NameIdentifier> createdIdentCaptor = + ArgumentCaptor.forClass(NameIdentifier.class); + Mockito.verify(mockDispatcher) + .createTable( + createdIdentCaptor.capture(), + Mockito.any(), + Mockito.any(), + Mockito.any(), + Mockito.any(), + Mockito.any(), + Mockito.any(), + Mockito.any()); + String physicalName = createdIdentCaptor.getValue().name(); + Assertions.assertEquals("My Table", physicalName); + + // 2. listTables() must surface that physical name unchanged. + Mockito.when(mockDispatcher.listTables(Mockito.any(Namespace.class))) + .thenReturn(new NameIdentifier[] {NameIdentifier.of(tableNs, physicalName)}); + NameIdentifier[] listed = dispatcher.listTables(tableNs); + Assertions.assertEquals(1, listed.length); + Assertions.assertEquals("My Table", listed[0].name()); + + // 3. Loading the table again requires re-quoting the listed name. + NameIdentifier quotedLoadIdent = NameIdentifier.of(tableNs, "\"" + listed[0].name() + "\""); + dispatcher.loadTable(quotedLoadIdent); + + ArgumentCaptor<NameIdentifier> loadedIdentCaptor = + ArgumentCaptor.forClass(NameIdentifier.class); + Mockito.verify(mockDispatcher).loadTable(loadedIdentCaptor.capture()); + Assertions.assertEquals("My Table", loadedIdentCaptor.getValue().name()); + } + private void assertTableCaseInsensitive( NameIdentifier tableIdent, Column[] expectedColumns, Table table) { Assertions.assertEquals(tableIdent.name().toLowerCase(), table.name());
