dimas-b commented on code in PR #4422:
URL: https://github.com/apache/polaris/pull/4422#discussion_r3282212171
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -882,6 +882,50 @@ 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;
+ }
+ validateBaseLocationAgainstAllowedList(
+ storageConfig.getAllowedLocations(), newDefaultBaseLocation,
catalogName);
+ }
+
+ // Package-private static for direct unit testing in
BaseLocationValidationTest.
+ // Validates against a raw allowed-locations list, used when the caller has
a list in hand
+ // (e.g. user-submitted StorageConfigInfo on update) and wants to validate
BEFORE the builder
+ // potentially mutates it. See
CatalogEntity.Builder.setStorageConfigurationInfo, which silently
+ // appends defaultBaseLocation to the allowed-locations set; validating
against the post-build
+ // entity is a no-op for the new-storage-config case.
Review Comment:
Please use javadoc comment.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -920,6 +968,35 @@ private void validateUpdateCatalogDiffOrThrow(
defaultBaseLocation = newDefaultBaseLocation;
}
}
+
+ // Validate the (effective) defaultBaseLocation against the (effective)
allowed-locations
+ // BEFORE the builder runs. Two cases:
+ // - storageConfigInfo provided: validate against the USER-SUBMITTED
allowed list, since
+ // CatalogEntity.Builder.setStorageConfigurationInfo silently appends
defaultBaseLocation
+ // to allowedLocations (see TODO in that method), so validating
post-build would be a
+ // no-op for this case.
+ // - storageConfigInfo not provided and base is changing: validate
against the current
+ // allowed list.
+ // Skip entirely when there's no storage config at all (legacy null), or
when neither the
+ // base nor the storage config is being modified.
+ boolean storageConfigChanging = updateRequest.getStorageConfigInfo() !=
null;
+ boolean baseChanging =
+ !Strings.isNullOrEmpty(newDefaultBaseLocation)
+ &&
!newDefaultBaseLocation.equals(currentCatalogEntity.getBaseLocation());
Review Comment:
This logic (while correct) is confusing, because in the vicinity of this
code it is not apparent how `newDefaultBaseLocation` is connected to
`defaultBaseLocation` being validated below ... at least it looks confusing to
me 😅
I'd propose establishing "changed" flags at the place where the change
happens.
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -882,6 +882,50 @@ private void validateUpdateCatalogDiffOrThrow(
}
}
+ // Package-private static for direct unit testing in
BaseLocationValidationTest.
Review Comment:
in Polaris, the custom is to use `@VisibleForTesting` (without a comment) in
these cases.
--
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]
