[#7190] fix(catalogs): Validate and filter empty storageLocations in HadoopCatalogOperations #7238
Conversation
...atalog-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java
Outdated
Show resolved
Hide resolved
|
@mchades I have simplified the logic. throwing an exception if any blank data is found. I’ve also updated my unit tests accordingly and reverted the changes made to the web UI (no longer needed). |
| } | ||
| Map<String, Path> schemaPaths = | ||
| getAndCheckSchemaPaths(schemaIdent.name(), schemaEntity.properties()); | ||
| if (schemaPaths.isEmpty() && storageLocations.isEmpty()) { |
There was a problem hiding this comment.
only need to change this line to:
if (schemaPaths.isEmpty() && (storageLocations.isEmpty() || storageLocations.values().stream().allMatch(StringUtils::isBlank)))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@KyleLin0927 You can use the code from this commit and complete the tests.
|
@mchades Thansks for providing the code. I’ve completed the updates accordingly, but I noticed that the changes introduced caused the |
| null)); | ||
| Assertions.assertEquals("Location name must not be blank", exception.getMessage()); | ||
|
|
||
| // empty location in catalog location |
There was a problem hiding this comment.
could you plz also test the cases that:
- empty location in the schema location
- empty fileset storage location
mchades
left a comment
mchades
left a comment
There was a problem hiding this comment.
LGTM. Thanks for your contributions!
Let's wait for the CI pass.
…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`.
…ns 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`.
…ns 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`.
…ns 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`.
2 participants
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 blankWhy 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?
./gradlew clean build.