visit2rahul commented on code in PR #4422:
URL: https://github.com/apache/polaris/pull/4422#discussion_r3282760339
##########
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:
Thank you @dimas-b. Done in 2c8471e3c - switched both helpers to
@VisibleForTesting (added the Guava import).
##########
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:
Thank you @dimas-b. Done in 2c8471e3c - converted to /** ... */ javadoc with
@throws.
##########
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:
Thank you @dimas-b. Good suggestion - done in 2c8471e3c. `baseChanging` is
now declared at method scope and set inside the if-block at the place where
`defaultBaseLocation` is mutated. The validation block below just reads the
flag, no more re-derivation from a hoisted local. Please review as your time
permits.
--
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]
