Skip to content

[#7190] fix(catalogs): Validate and filter empty storageLocations in HadoopCatalogOperations #7238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mchades merged 8 commits into from
Jun 5, 2025
Merged

[#7190] fix(catalogs): Validate and filter empty storageLocations in HadoopCatalogOperations #7238

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5cde221
fix: solve NPE at HadoopCatalogOperations
KyleLin0927 May 23, 2025
4e329a3
improvement: modify web UI default-location-name input rule
KyleLin0927 May 23, 2025
93302cc
test: add unit test
KyleLin0927 May 24, 2025
503b659
fix: modify validation logic
KyleLin0927 May 24, 2025
ed4c882
Revert "improvement: modify web UI default-location-name input rule"
KyleLin0927 May 24, 2025
84cb6ca
refactor the logic
KyleLin0927 May 30, 2025
6fa394a
modify the test
KyleLin0927 May 30, 2025
70452d3
add tests
KyleLin0927 Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...log-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,12 @@ public Fileset createMultipleLocationFileset(

// Either catalog property "location", or schema property "location", or storageLocation must be
// set for managed fileset.
for (Map.Entry<String, String> e : storageLocations.entrySet()) {
if (StringUtils.isBlank(e.getValue())) {
throw new IllegalArgumentException(
"Storage location for name '" + e.getKey() + "' must not be null or blank");
}
}
Map<String, Path> schemaPaths =
getAndCheckSchemaPaths(schemaIdent.name(), schemaEntity.properties());
if (schemaPaths.isEmpty() && storageLocations.isEmpty()) {
Copy link
Contributor

@mchades mchades May 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only need to change this line to:
if (schemaPaths.isEmpty() && (storageLocations.isEmpty() || storageLocations.values().stream().allMatch(StringUtils::isBlank)))

Copy link
Contributor Author

@KyleLin0927 KyleLin0927 May 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for suggestion. I applied this change, but during testing, I encountered a NullPointerException when the input storageLocations map contained a mix of valid values and blank/null values.

Copy link
Contributor

@mchades mchades May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPE from where?

Copy link
Contributor Author

@KyleLin0927 KyleLin0927 May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may refer to the unit test I added in this PR (TestHadoopCatalogOperations.java line 1559) If I only use

if (schemaPaths.isEmpty() && (storageLocations.isEmpty() || storageLocations.values().stream().allMatch(StringUtils::isBlank)))

and the storageLocations input contains both a valid entry and a blank value, it will still trigger an NPE in calculateFilesetPaths. If you think this test is not appropriate, I will remove it.

image

Copy link
Contributor

@mchades mchades May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me think it over.

Copy link
Contributor

@mchades mchades May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KyleLin0927 You can use the code from this commit and complete the tests.

mchades@926f49e

Expand Down
52 changes: 52 additions & 0 deletions ...hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/TestHadoopCatalogOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1519,6 +1519,58 @@ public void testCreateMultipleLocationsWithExceptions() throws IOException {
.contains(
"Default location name must be set and must be one of the fileset locations"),
"Exception message: " + exception.getMessage());

Schema emptySchema =
createMultiLocationSchema(
"emptySchema", "no-location", ImmutableMap.of(), ImmutableMap.of());

// storage location value is blank
exception =
Assertions.assertThrows(
IllegalArgumentException.class,
() ->
createMultiLocationFileset(
"fs2",
emptySchema.name(),
null,
Fileset.Type.MANAGED,
ImmutableMap.of(),
ImmutableMap.of("v1", ""),
ImmutableMap.of()));
Assertions.assertEquals(
"Storage location for name 'v1' must not be null or blank", exception.getMessage());

// storage location value is blank
exception =
Assertions.assertThrows(
IllegalArgumentException.class,
() ->
createMultiLocationFileset(
"fs3",
emptySchema.name(),
null,
Fileset.Type.MANAGED,
ImmutableMap.of(),
ImmutableMap.of("v1", " "),
ImmutableMap.of()));
Assertions.assertEquals(
"Storage location for name 'v1' must not be null or blank", exception.getMessage());

// two storage locations, one valid and one is blank
exception =
Assertions.assertThrows(
IllegalArgumentException.class,
() ->
createMultiLocationFileset(
"fs5",
emptySchema.name(),
null,
Fileset.Type.MANAGED,
ImmutableMap.of(),
ImmutableMap.of("v1", TEST_ROOT_PATH + "/valid", "v2", ""),
ImmutableMap.of()));
Assertions.assertEquals(
"Storage location for name 'v2' must not be null or blank", exception.getMessage());
}
}

Expand Down