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 8867d8256b [#7190] fix(catalogs): Validate and filter empty
storageLocations in HadoopCatalogOperations (#7238)
8867d8256b is described below
commit 8867d8256b84d75ca1d44dcd19930b723ad507d6
Author: Kyle Lin <[email protected]>
AuthorDate: Thu Jun 5 16:40:05 2025 +0800
[#7190] fix(catalogs): Validate and filter empty storageLocations in
HadoopCatalogOperations (#7238)
### What changes were proposed in this pull request?
Add validation: throw error if StorageLocation is null or blank `Storage
location for name '" + e.getKey() + "' must not be null or blank`
### Why are the changes needed?
Fixes #7190
### Does this PR introduce any user-facing change?
No. APIs and behavior are unchanged, only error messages for invalid
input are clearer.
### How was this patch tested?
- Added new unit tests.
- Verified with `./gradlew clean build`.
---
.../catalog/hadoop/HadoopCatalogOperations.java | 32 +++++++++++-
.../hadoop/TestHadoopCatalogOperations.java | 58 +++++++++++++++++++---
2 files changed, 83 insertions(+), 7 deletions(-)
diff --git
a/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java
b/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java
index c6b98791c1..fab9b4b9f6 100644
---
a/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java
+++
b/catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java
@@ -294,6 +294,10 @@ public class HadoopCatalogOperations extends
ManagedSchemaOperations
if (StringUtils.isBlank(name)) {
throw new IllegalArgumentException("Location name must not be
blank");
}
+ if (StringUtils.isBlank(path)) {
+ throw new IllegalArgumentException(
+ "Storage location must not be blank for location name: " +
name);
+ }
});
// Check if the fileset already existed in cache first. If it does, it
means the fileset is
@@ -1018,7 +1022,19 @@ public class HadoopCatalogOperations extends
ManagedSchemaOperations
}));
}
+ private boolean existBlankValue(Map<String, String> properties, String key) {
+ return properties.containsKey(key) &&
StringUtils.isBlank(properties.get(key));
+ }
+
private Map<String, Path> getAndCheckCatalogStorageLocations(Map<String,
String> properties) {
+ if (existBlankValue(properties, HadoopCatalogPropertiesMetadata.LOCATION))
{
+ // If the properties contain the catalog property "location", it must
not be blank.
+ throw new IllegalArgumentException(
+ "The value of the catalog property "
+ + HadoopCatalogPropertiesMetadata.LOCATION
+ + " must not be blank");
+ }
+
ImmutableMap.Builder<String, Path> catalogStorageLocations =
ImmutableMap.builder();
String unnamedLocation =
(String)
@@ -1033,11 +1049,17 @@ public class HadoopCatalogOperations extends
ManagedSchemaOperations
properties.forEach(
(k, v) -> {
- if (k.startsWith(PROPERTY_MULTIPLE_LOCATIONS_PREFIX) &&
StringUtils.isNotBlank(v)) {
+ if (k.startsWith(PROPERTY_MULTIPLE_LOCATIONS_PREFIX)) {
String locationName =
k.substring(PROPERTY_MULTIPLE_LOCATIONS_PREFIX.length());
if (StringUtils.isBlank(locationName)) {
throw new IllegalArgumentException("Location name must not be
blank");
}
+
+ if (StringUtils.isBlank(v)) {
+ throw new IllegalArgumentException(
+ "Location value must not be blank for " + "location name: "
+ locationName);
+ }
+
checkPlaceholderValue(v);
catalogStorageLocations.put(locationName, new
Path((ensureTrailingSlash(v))));
}
@@ -1047,6 +1069,14 @@ public class HadoopCatalogOperations extends
ManagedSchemaOperations
private Map<String, Path> getAndCheckSchemaPaths(
String schemaName, Map<String, String> schemaProps) {
+ if (existBlankValue(schemaProps, HadoopSchemaPropertiesMetadata.LOCATION))
{
+ // If the properties contain the schema property "location", it must not
be blank.
+ throw new IllegalArgumentException(
+ "The value of the schema property "
+ + HadoopSchemaPropertiesMetadata.LOCATION
+ + " must not be blank");
+ }
+
Map<String, Path> schemaPaths = new HashMap<>();
catalogStorageLocations.forEach(
(name, path) -> {
diff --git
a/catalogs/catalog-hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/TestHadoopCatalogOperations.java
b/catalogs/catalog-hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/TestHadoopCatalogOperations.java
index 41f7b2aee9..ff4924381e 100644
---
a/catalogs/catalog-hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/TestHadoopCatalogOperations.java
+++
b/catalogs/catalog-hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/TestHadoopCatalogOperations.java
@@ -50,6 +50,7 @@ import java.io.IOException;
import java.net.ConnectException;
import java.nio.file.Paths;
import java.util.Arrays;
+import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.UUID;
@@ -383,15 +384,15 @@ public class TestHadoopCatalogOperations {
String name = "schema28";
String comment = "comment28";
String catalogPath = "";
- Schema schema = createSchema(name, comment, catalogPath, null);
- Assertions.assertEquals(name, schema.name());
- Assertions.assertEquals(comment, schema.comment());
Throwable exception =
Assertions.assertThrows(
- SchemaAlreadyExistsException.class,
- () -> createSchema(name, comment, catalogPath, null));
- Assertions.assertEquals("Schema m1.c1.schema28 already exists",
exception.getMessage());
+ IllegalArgumentException.class, () -> createSchema(name, comment,
catalogPath, null));
+ Assertions.assertEquals(
+ "The value of the catalog property "
+ + HadoopCatalogPropertiesMetadata.LOCATION
+ + " must not be blank",
+ exception.getMessage());
}
@Test
@@ -1598,6 +1599,51 @@ public class TestHadoopCatalogOperations {
null));
Assertions.assertEquals("Location name must not be blank",
exception.getMessage());
+ // empty location in catalog location
+ Map<String, String> illegalLocations2 =
+ new HashMap<String, String>() {
+ {
+ put(PROPERTY_MULTIPLE_LOCATIONS_PREFIX + "v1", null);
+ }
+ };
+ exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ ops.initialize(
+ illegalLocations2,
+ randomCatalogInfo("m1", "c1"),
+ HADOOP_PROPERTIES_METADATA));
+ Assertions.assertEquals(
+ "Location value must not be blank for location name: v1",
exception.getMessage());
+
+ // empty location in schema location
+ exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ createMultiLocationSchema(
+ "s1", "comment", ImmutableMap.of(),
ImmutableMap.of("location", "")));
+ Assertions.assertEquals(
+ "The value of the schema property location must not be blank",
exception.getMessage());
+
+ // empty fileset storage location
+ exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ createMultiLocationFileset(
+ "fileset_test",
+ "s1",
+ null,
+ Fileset.Type.MANAGED,
+ ImmutableMap.of(),
+ ImmutableMap.of("location1", ""),
+ null));
+ Assertions.assertEquals(
+ "Storage location must not be blank for location name: location1",
+ exception.getMessage());
+
// storage location is parent of schema location
Schema multipLocationSchema =
createMultiLocationSchema(
