visit2rahul commented on code in PR #4422:
URL: https://github.com/apache/polaris/pull/4422#discussion_r3282181133
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -918,6 +952,15 @@ private void validateUpdateCatalogDiffOrThrow(
// New base location is already in the updated properties; we'll also
potentially
// plumb it into the logic for setting an updated
StorageConfigurationInfo.
defaultBaseLocation = newDefaultBaseLocation;
+
+ // If the storage config is not being updated and the location
actually changed,
+ // validate that the new base location is allowed by the existing
storage configuration
+ // to prevent inconsistent state.
+ if (updateRequest.getStorageConfigInfo() == null
+ &&
!newDefaultBaseLocation.equals(currentCatalogEntity.getBaseLocation())) {
Review Comment:
Thank you @dimas-b, you are right - the asymmetry on the
new-storage-config-provided case was a real hole. Pushed a28670530:
- New helper `validateBaseLocationAgainstAllowedList(List<String>, ...)`
takes the allowed-list directly; the existing
`validateBaseLocationAgainstStorageConfig(CatalogEntity, ...)` becomes a thin
delegate.
- Validation now runs BEFORE `setStorageConfigurationInfo` against the
USER-SUBMITTED allowed list when storage config is provided. Validating
post-build would have been a no-op because
`CatalogEntity.Builder.setStorageConfigurationInfo` silently appends
`defaultBaseLocation` to the allowed-locations set (see TODO at lines 275-281
of CatalogEntity.java).
- Hoisted `newDefaultBaseLocation` read out of the properties-if block so
validation can see it when properties is null.
- Removed the old asymmetric validation block.
- 4 new unit tests on the raw-list helper directly.
The silent-add itself in `CatalogEntity.Builder` is out of scope for this
PR; the TODO calls for a proper source-level fix and I will file a follow-up
issue to track that.
@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]
