dimas-b commented on code in PR #4422:
URL: https://github.com/apache/polaris/pull/4422#discussion_r3422499590
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##########
@@ -266,21 +304,52 @@ public Builder setDefaultBaseLocation(String
defaultBaseLocation) {
return this;
}
+ public Builder validateDefaultBaseLocation() {
Review Comment:
Does it have to be `public`? It is currently used internally in this class 🤔
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/CatalogEntity.java:
##########
@@ -266,21 +304,52 @@ public Builder setDefaultBaseLocation(String
defaultBaseLocation) {
return this;
}
+ public Builder validateDefaultBaseLocation() {
+ String defaultBaseLocation = properties.get(DEFAULT_BASE_LOCATION_KEY);
+ if (defaultBaseLocation == null) {
+ return this;
+ }
+ String configStr =
+
internalProperties.get(PolarisEntityConstants.getStorageConfigInfoPropertyName());
+ if (configStr == null) {
+ return this;
+ }
+ PolarisStorageConfigurationInfo storageConfig =
+ PolarisStorageConfigurationInfo.deserialize(configStr);
+ if (storageConfig == null) {
+ return this;
+ }
+ List<String> allowedLocations = storageConfig.getAllowedLocations();
+ if (allowedLocations != null && !allowedLocations.isEmpty()) {
+ validateBaseLocationAgainstAllowedList(allowedLocations,
defaultBaseLocation);
+ }
+ return this;
+ }
+
public Builder setStorageConfigurationInfo(
RealmConfig realmConfig, StorageConfigInfo storageConfigModel, String
defaultBaseLocation) {
if (storageConfigModel != null) {
- PolarisStorageConfigurationInfo config;
- Set<String> allowedLocations = new
HashSet<>(storageConfigModel.getAllowedLocations());
-
- // TODO: Reconsider whether this should actually just be a check
up-front or if we
- // actually want to silently add to the allowed locations. Maybe
ideally we only
- // add to the allowedLocations if allowedLocations is empty for the
simple case,
- // but if the caller provided allowedLocations explicitly, then we
just verify that
- // the defaultBaseLocation is at least a subpath of one of the
allowedLocations.
if (defaultBaseLocation == null) {
throw new BadRequestException("Must specify default base location");
}
- allowedLocations.add(defaultBaseLocation);
+ // Asymmetric semantics for the simple-create case vs. explicit input:
+ // - If the caller supplied no allowed-locations, default to the
catalog's
+ // default-base-location. This is the convenience for naive create
requests.
+ // (Update-time callers in PolarisAdminService enforce strictness
before reaching
+ // this method, so the auto-populate only kicks in at create time.)
+ // - If the caller supplied an explicit allowed-locations list,
validate that the
+ // default-base-location is a subpath of at least one entry and
store the list
+ // as-is. No silent additions to the user-supplied list.
+ List<String> userAllowedLocations =
storageConfigModel.getAllowedLocations();
+ Set<String> allowedLocations;
+ if (userAllowedLocations == null || userAllowedLocations.isEmpty()) {
+ allowedLocations = new HashSet<>();
+ allowedLocations.add(defaultBaseLocation);
+ } else {
+ validateBaseLocationAgainstAllowedList(userAllowedLocations,
defaultBaseLocation);
Review Comment:
This will be validated in `.build()` won't it? Is there a use case for
validating `defaultBaseLocation` before `build()` is called?
--
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]
