Fix namespace property updates that set a location always failing - #5474
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Fixes namespace property update validation so that, when custom namespace locations are disabled, restating the existing default location is accepted while genuinely custom locations are still rejected.
Changes:
- Adjust
setPropertiesto validate against the updated namespace entity and the resolved parent path (instead of validating the pre-update entity against itself). - Add a regression test covering both “restate default location” (allowed) and “different location” (rejected) behaviors.
- Document the fix in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergAllowedLocationTest.java | Adds a regression test ensuring default-location restatement is accepted and custom locations are rejected. |
| runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java | Fixes validation inputs by using the updated entity and resolved parent path. |
| CHANGELOG.md | Documents the behavior change and the previous failure mode. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
setProperties validated the namespace against itself. It passed the namespace's own resolved path as the parent, so the validator took the namespace as its own parent and expected a location one level deeper than the real one, and it passed the pre-update entity, so the location checked was the old one rather than the one the request asked for. With ALLOW_NAMESPACE_CUSTOM_LOCATION at its default of false, any updateNamespaceProperties carrying a location failed with HTTP 400 naming a doubled path. Both arguments have to change together: correcting only the parent would leave the check reading the old location, which always matches the default, so a custom location in the request would never be examined.
2fdf984 to
1518023
Compare
| - Updating a namespace's properties no longer fails with HTTP 400 when the request restates the | ||
| namespace's own default location. With custom namespace locations disabled, the update validated | ||
| the namespace's *pre-update* entity against the namespace *itself* as its own parent, so the | ||
| expected default was computed as `<namespace location>/<namespace name>` and never matched. Any | ||
| `updateNamespaceProperties` carrying the base-location property was rejected, and the expected | ||
| location named in the error was wrong even when the rejection itself was correct. |
There was a problem hiding this comment.
nit: can we have a more human readable version? :-)
3de0565
With
ALLOW_NAMESPACE_CUSTOM_LOCATIONat its default of false, everyupdateNamespacePropertiesrequest carrying the base-location property is rejected withHTTP 400— including one that restates the location the namespace already has, which is by definition not a custom location.The error names a location that cannot exist:
ns/ns/is the namespace nested inside itself.The check expects the namespace to check plus a resolved path whose leaf is that namespace's parent. The call in
setPropertiessupplied neither:resolvedEntities is the namespace's own resolved path, so
getResolvedLeafEntity()returns the namespace itself. The expected default is then computed as<namespace location>/<namespace name>, which can never equal the namespace's location — hence the doubled path and the unconditional rejection.entity is the pre-update entity, so the location validated is the old one rather than the one the request asks for.
Both call sites were introduced together in Disable custom namespace locations #2422 ("Disable custom namespace locations"), and the create path in that same commit passes the parent correctly:
Both arguments have to change together. Correcting only the parent would leave the check reading the pre-update location, which always equals the default, so a genuinely custom location in the request would never be examined — the endpoint would silently stop enforcing the flag. The test asserts both directions.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)