Skip to content

Commit 806de75

Browse files
KyleLin0927hdygxsj
authored andcommitted
[apache#7190] fix(catalogs): Validate and filter empty storageLocations in HadoopCatalogOperations (apache#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 apache#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`.
1 parent ddba253 commit 806de75

File tree

2 files changed

+83
-7
lines changed

2 files changed

+83
-7
lines changed

catalogs/catalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,10 @@ public Fileset createMultipleLocationFileset(
294294
if (StringUtils.isBlank(name)) {
295295
throw new IllegalArgumentException("Location name must not be blank");
296296
}
297+
if (StringUtils.isBlank(path)) {
298+
throw new IllegalArgumentException(
299+
"Storage location must not be blank for location name: " + name);
300+
}
297301
});
298302

299303
// Check if the fileset already existed in cache first. If it does, it means the fileset is
@@ -1018,7 +1022,19 @@ private void validateLocationHierarchy(
10181022
}));
10191023
}
10201024

1025+
private boolean existBlankValue(Map<String, String> properties, String key) {
1026+
return properties.containsKey(key) && StringUtils.isBlank(properties.get(key));
1027+
}
1028+
10211029
private Map<String, Path> getAndCheckCatalogStorageLocations(Map<String, String> properties) {
1030+
if (existBlankValue(properties, HadoopCatalogPropertiesMetadata.LOCATION)) {
1031+
// If the properties contain the catalog property "location", it must not be blank.
1032+
throw new IllegalArgumentException(
1033+
"The value of the catalog property "
1034+
+ HadoopCatalogPropertiesMetadata.LOCATION
1035+
+ " must not be blank");
1036+
}
1037+
10221038
ImmutableMap.Builder<String, Path> catalogStorageLocations = ImmutableMap.builder();
10231039
String unnamedLocation =
10241040
(String)
@@ -1033,11 +1049,17 @@ private Map<String, Path> getAndCheckCatalogStorageLocations(Map<String, String>
10331049

10341050
properties.forEach(
10351051
(k, v) -> {
1036-
if (k.startsWith(PROPERTY_MULTIPLE_LOCATIONS_PREFIX) && StringUtils.isNotBlank(v)) {
1052+
if (k.startsWith(PROPERTY_MULTIPLE_LOCATIONS_PREFIX)) {
10371053
String locationName = k.substring(PROPERTY_MULTIPLE_LOCATIONS_PREFIX.length());
10381054
if (StringUtils.isBlank(locationName)) {
10391055
throw new IllegalArgumentException("Location name must not be blank");
10401056
}
1057+
1058+
if (StringUtils.isBlank(v)) {
1059+
throw new IllegalArgumentException(
1060+
"Location value must not be blank for " + "location name: " + locationName);
1061+
}
1062+
10411063
checkPlaceholderValue(v);
10421064
catalogStorageLocations.put(locationName, new Path((ensureTrailingSlash(v))));
10431065
}
@@ -1047,6 +1069,14 @@ private Map<String, Path> getAndCheckCatalogStorageLocations(Map<String, String>
10471069

10481070
private Map<String, Path> getAndCheckSchemaPaths(
10491071
String schemaName, Map<String, String> schemaProps) {
1072+
if (existBlankValue(schemaProps, HadoopSchemaPropertiesMetadata.LOCATION)) {
1073+
// If the properties contain the schema property "location", it must not be blank.
1074+
throw new IllegalArgumentException(
1075+
"The value of the schema property "
1076+
+ HadoopSchemaPropertiesMetadata.LOCATION
1077+
+ " must not be blank");
1078+
}
1079+
10501080
Map<String, Path> schemaPaths = new HashMap<>();
10511081
catalogStorageLocations.forEach(
10521082
(name, path) -> {

catalogs/catalog-hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/TestHadoopCatalogOperations.java

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
import java.net.ConnectException;
5151
import java.nio.file.Paths;
5252
import java.util.Arrays;
53+
import java.util.HashMap;
5354
import java.util.Map;
5455
import java.util.Set;
5556
import java.util.UUID;
@@ -383,15 +384,15 @@ public void testCreateSchemaWithEmptyCatalogLocation() throws IOException {
383384
String name = "schema28";
384385
String comment = "comment28";
385386
String catalogPath = "";
386-
Schema schema = createSchema(name, comment, catalogPath, null);
387-
Assertions.assertEquals(name, schema.name());
388-
Assertions.assertEquals(comment, schema.comment());
389387

390388
Throwable exception =
391389
Assertions.assertThrows(
392-
SchemaAlreadyExistsException.class,
393-
() -> createSchema(name, comment, catalogPath, null));
394-
Assertions.assertEquals("Schema m1.c1.schema28 already exists", exception.getMessage());
390+
IllegalArgumentException.class, () -> createSchema(name, comment, catalogPath, null));
391+
Assertions.assertEquals(
392+
"The value of the catalog property "
393+
+ HadoopCatalogPropertiesMetadata.LOCATION
394+
+ " must not be blank",
395+
exception.getMessage());
395396
}
396397

397398
@Test
@@ -1598,6 +1599,51 @@ public void testCreateMultipleLocationsWithExceptions() throws IOException {
15981599
null));
15991600
Assertions.assertEquals("Location name must not be blank", exception.getMessage());
16001601

1602+
// empty location in catalog location
1603+
Map<String, String> illegalLocations2 =
1604+
new HashMap<String, String>() {
1605+
{
1606+
put(PROPERTY_MULTIPLE_LOCATIONS_PREFIX + "v1", null);
1607+
}
1608+
};
1609+
exception =
1610+
Assertions.assertThrows(
1611+
IllegalArgumentException.class,
1612+
() ->
1613+
ops.initialize(
1614+
illegalLocations2,
1615+
randomCatalogInfo("m1", "c1"),
1616+
HADOOP_PROPERTIES_METADATA));
1617+
Assertions.assertEquals(
1618+
"Location value must not be blank for location name: v1", exception.getMessage());
1619+
1620+
// empty location in schema location
1621+
exception =
1622+
Assertions.assertThrows(
1623+
IllegalArgumentException.class,
1624+
() ->
1625+
createMultiLocationSchema(
1626+
"s1", "comment", ImmutableMap.of(), ImmutableMap.of("location", "")));
1627+
Assertions.assertEquals(
1628+
"The value of the schema property location must not be blank", exception.getMessage());
1629+
1630+
// empty fileset storage location
1631+
exception =
1632+
Assertions.assertThrows(
1633+
IllegalArgumentException.class,
1634+
() ->
1635+
createMultiLocationFileset(
1636+
"fileset_test",
1637+
"s1",
1638+
null,
1639+
Fileset.Type.MANAGED,
1640+
ImmutableMap.of(),
1641+
ImmutableMap.of("location1", ""),
1642+
null));
1643+
Assertions.assertEquals(
1644+
"Storage location must not be blank for location name: location1",
1645+
exception.getMessage());
1646+
16011647
// storage location is parent of schema location
16021648
Schema multipLocationSchema =
16031649
createMultiLocationSchema(

0 commit comments

Comments
 (0)