visit2rahul commented on code in PR #4422:
URL: https://github.com/apache/polaris/pull/4422#discussion_r3278276675
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -882,6 +882,34 @@ private void validateUpdateCatalogDiffOrThrow(
}
}
+ // Package-private static for direct unit testing in
BaseLocationValidationTest.
+ static void validateBaseLocationAgainstStorageConfig(
+ CatalogEntity catalogEntity, String newDefaultBaseLocation, String
catalogName) {
+ PolarisStorageConfigurationInfo storageConfig =
catalogEntity.getStorageConfigurationInfo();
+ if (storageConfig == null) {
+ return;
+ }
+
+ List<String> allowedLocations = storageConfig.getAllowedLocations();
+ if (allowedLocations == null || allowedLocations.isEmpty()) {
+ return;
Review Comment:
Thank you @dimas-b, you are right. I read
InMemoryStorageIntegration#L99-L100 - anyMatch on an empty list returns false,
so the runtime semantic for empty allowed-locations is "deny all," not "no
constraint." Our validation should match.
Pushed 571314e9d tightening validateBaseLocationAgainstStorageConfig:
- empty allowedLocations -> BadRequestException (matches runtime)
- null allowedLocations -> also BadRequestException (same semantic; the
runtime path would NPE without the early reject anyway)
- null storage config -> still no-op (unchanged; matches the established
pattern in validateUpdateCatalogDiffOrThrow / getCatalogLocations)
testEmptyAllowedLocationsSkipsValidation and
testNullAllowedLocationsSkipsValidation are renamed and flipped to assert
rejection. testNullStorageConfigSkipsValidation kept as-is.
@jbonofre @dimas-b please review as your time permits. Seems the CI workflow
on this push needs committer approval to start - happy to iterate once it runs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
