Core: Validate default-base-location against storage config on catalog update#4422
Open
visit2rahul wants to merge 3 commits into
Open
Core: Validate default-base-location against storage config on catalog update#4422visit2rahul wants to merge 3 commits into
visit2rahul wants to merge 3 commits into
Conversation
…g update (apache#4355) Reject catalog updates where the new default-base-location is not within any of the allowed locations in the existing storage configuration. Previously, updating only the default-base-location property without also updating storageConfigInfo would leave the catalog in an inconsistent state where namespace and table creation would fail. The fix adds a validation check in updateCatalog that verifies the new base location is a child of at least one allowed location when the storage config itself is not being updated.
jbonofre
reviewed
May 13, 2026
| } | ||
| } | ||
|
|
||
| private void validateBaseLocationAgainstStorageConfig( |
Member
There was a problem hiding this comment.
nit: maybe worth to add a test on this path to verify.
|
|
||
| // If the storage config is not being updated, validate that the new base location | ||
| // is allowed by the existing storage configuration to prevent inconsistent state. | ||
| if (updateRequest.getStorageConfigInfo() == null) { |
Member
There was a problem hiding this comment.
Maybe we can also check if the new location is different from the current one. It will accelerate and avoid any side effect.
Suggested change
| if (updateRequest.getStorageConfigInfo() == null) { | |
| if (updateRequest.getStorageConfigInfo() == null && !newDefaultBaseLocation.equals(currentCatalogEntity.getBaseLocation())) { |
That could be done before defaultBaseLocation = newDefaultBaseLocation; also.
Author
|
@jbonofre thanks for the review — addressed both points (latest in cc58e79):
Will look at the failing regression once CI re-runs. |
…dation path - Make validateBaseLocationAgainstStorageConfig package-private static for direct unit testing (no PolarisAdminService construction needed) - Replace stub StorageLocation::isChildOf tests with tests that call the validation method directly, covering: child / exact match / outside-allowed rejection (BadRequestException with expected message), multi-allowed, scheme mismatch, null storage config, null/empty allowed locations
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4355
Summary
Adds validation to reject catalog updates where the new
default-base-locationis not within any of theallowedLocationsconfigured in the catalog's existing storage configuration.Previously, updating only the
default-base-locationproperty without also providingstorageConfigInfowould leave the catalog in an inconsistent state — the property would be accepted but subsequent namespace and table creation would fail because the new location wasn't inallowedLocations.Change
Added
validateBaseLocationAgainstStorageConfig()inPolarisAdminService.updateCatalog()that checks the new base location is a child of at least one allowed storage location when the storage config itself is not being updated.Scenario
default-base-location=s3://foo/,allowedLocations=["s3://foo/"]default-base-location=s3://bar/without updating storage configTest Plan
polaris-runtime-service:compileJavapassespolaris-runtime-service:test(admin tests) passes