visit2rahul commented on code in PR #4422:
URL: https://github.com/apache/polaris/pull/4422#discussion_r3406999848
##########
runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -911,11 +911,33 @@ private void validateUpdateCatalogDiffOrThrow(
// so we must restore it explicitly in the updateBuilder.
updateBuilder.setDefaultBaseLocation(defaultBaseLocation);
} else {
- // 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;
}
}
+
+ // Base-location validity is an invariant on every update. Update-side
semantics are strict:
+ // unlike CatalogEntity.Builder.setStorageConfigurationInfo (which has a
create-time
+ // convenience that auto-populates allowed-locations from
default-base-location when the
+ // caller supplied none), updates that touch storage_config must declare
allowed-locations
+ // explicitly. This prevents silent loss of an existing allowed-locations
list during a
+ // partial storage_config update (e.g. role rotation).
+ if (defaultBaseLocation != null && updateRequest.getStorageConfigInfo() !=
null) {
+ List<String> submittedAllowedLocations =
+ updateRequest.getStorageConfigInfo().getAllowedLocations();
+ if (submittedAllowedLocations == null ||
submittedAllowedLocations.isEmpty()) {
+ throw new BadRequestException(
+ "Cannot update Catalog %s: when updating storage_config,
allowed-locations must"
Review Comment:
hmm.. i am good to move it to a diff PR, if you think the blast radius of
this check is higher, and it should go through design/dev discussions. Ideally
I feel we should enforce the check though, what do you think?
Or it does not make sense to enforce it? For now ill take it out.
--
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]
