You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@polaris.apache.org by "youngrae81 (via GitHub)" <gi...@apache.org> on 2026/10/23 17:14:14 UTC

[PR] fix: return 400 instead of 500 when a policy update omits current-policy-version [polaris]

youngrae81 opened a new pull request, #5861:
URL: https://github.com/apache/polaris/pull/5861

   `UpdatePolicyRequest` declares no required fields, so `PUT .../namespaces/{namespace}/policies/{policy-name}` with a body that omits `current-policy-version` reaches `PolicyCatalogHandler.updatePolicy` with a null version. Passing it to `PolicyCatalog.updatePolicy(..., int currentPolicyVersion)` unboxed it and threw a `NullPointerException`, which the exception mapper turned into a 500.
   
   `PolicyCatalogHandler.updatePolicy` now rejects such a request with a `BadRequestException` ("current-policy-version is required to update a policy"). The check runs after authorization, so a missing policy still returns 404 and a caller without the privilege still gets 403. I did not change the spec to mark the field as required, since that would change the published API; the server-side check gives the same result for clients.
   
   `PolarisPolicyServiceIntegrationTest.testUpdatePolicyWithoutCurrentPolicyVersion` sends the update without the field and checks for a 400 naming the field and an unchanged policy (version and content). Before the fix it failed with `expected: 400 but was: 500`. It passes with the in-memory, relational JDBC and CockroachDB backends (`PolicyServiceIntegrationTest`, `JdbcPolicyServiceIT`, `CockroachPolicyServiceIT`).
   
   Fixes #5857     
   
   ## Checklist
   - [x] 🛡 Don't disclose security issues! (contact security@apache.org)
   - [x] 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #5857
   - [x] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
   - [x] 💡 Added comments for complex logic
   - [x] 🧾 Updated `CHANGELOG.md` (if needed)
   - [x] 📚 Updated documentation in `site/content/in-dev/unreleased` (if needed)
   
   This change was prepared with AI assistance; I have reviewed and tested it end to end.


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: return 400 instead of 500 when a policy update omits current-policy-version [polaris]

Posted by "flyrain (via GitHub)" <gi...@apache.org>.

flyrain commented on PR #5861:
URL: https://github.com/apache/polaris/pull/5861#issuecomment-6422882837

   Thanks @youngrae81 for the fix! Thanks @ayushtkn @vigneshio for the review!


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: return 400 instead of 500 when a policy update omits current-policy-version [polaris]

Posted by "youngrae81 (via GitHub)" <gi...@apache.org>.

youngrae81 commented on PR #5861:
URL: https://github.com/apache/polaris/pull/5861#issuecomment-6392845219

   Thanks. On the two optional asks:
   - Authz ordering: testUpdateNonExistingPolicy covers the missing-policy case, and the check sits after authorizeBasicPolicyOperationOrThrow, so a caller without POLICY_WRITE still gets 403. I kept the authz matrix test as is, since it exercises privileges rather than request validation; happy to add a deny case there if a reviewer prefers.
   - Spec description: I left policy-apis.yaml unchanged to keep this PR to the server-side fix. Documenting that the server requires the field could go together with a broader look at which UpdatePolicyRequest fields are really optional. 


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix: return 400 instead of 500 when a policy update omits current-policy-version [polaris]

Posted by "flyrain (via GitHub)" <gi...@apache.org>.

flyrain merged PR #5861:
URL: https://github.com/apache/polaris/pull/5861


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org