This is an automated email from the ASF dual-hosted git repository.
mchades pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 65d305fdaa [#12097] improvement(core): Validate names before
normalizing in CapabilityHelpers (#12098)
65d305fdaa is described below
commit 65d305fdaa380b6d629963ab807124cfda1d54da
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.
---
.../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());
