visit2rahul commented on PR #4422: URL: https://github.com/apache/polaris/pull/4422#issuecomment-4511416853
Thank you @dimas-b - before I start coding, want to confirm the plan. **Builder fix** (`CatalogEntity.Builder.setStorageConfigurationInfo`, polaris-core): Replace the silent `allowedLocations.add(defaultBaseLocation)` at line 283 with up-front validation per the TODO at 275-281. Empty allowedLocations rejected (consistent with your earlier review citing `InMemoryStorageIntegration:99-100`, now applied at create-time too); non-empty must contain defaultBaseLocation as a subpath of at least one entry. **updateCatalog**: Drop the `baseChanging` / `storageConfigChanging` flags and the conditional guard. Always validate the effective defaultBaseLocation against the effective allowed-locations list (new submitted if provided, current catalog's otherwise; skip when neither side has storage config). Base-location validity becomes an unconditional invariant. **Helper placement**: Move both validation helpers from PolarisAdminService to CatalogEntity (`public static @VisibleForTesting`) so builder and updateCatalog share one source. **On the null-storage-config case** (current catalog has no storage config + no new one provided): I am thinking of keeping this as a no-op, matching the established pattern in `validateUpdateCatalogDiffOrThrow` and `getCatalogLocations`. Let me know if you'd prefer reject. **Folding in #4521** since the silent-add fix supersedes it. Sound right? -- 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]
