Allow only Table level drop permissions to drop Generic Table#4779
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates dropGenericTable authorization to resolve the table entity (enabling table-level grants to apply) and adds a dedicated authz test to validate behavior when privileges are granted at the table level.
Changes:
- Switch
dropGenericTableauthorization from namespace-based to table-entity-based checks. - Add a table-level grant test case for
dropGenericTable, including grant/revoke plumbing for table privileges.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| runtime/service/src/main/java/org/apache/polaris/service/catalog/generic/GenericTableCatalogHandler.java | Updates drop authorization to use a table-like authorizer tied to the table entity/subtype. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalogHandlerAuthzTest.java | Adds a new test factory validating drop behavior with table-level privilege grants and supporting helper builder. |
| public boolean dropGenericTable(TableIdentifier identifier) { | ||
| PolarisAuthorizableOperation op = PolarisAuthorizableOperation.DROP_TABLE_WITHOUT_PURGE; | ||
| authorizeCreateTableLikeUnderNamespaceOperationOrThrow(op, identifier); | ||
| authorizeBasicTableLikeOperationOrThrow(op, PolarisEntitySubType.GENERIC_TABLE, identifier); | ||
|
|
||
| return this.genericTableCatalog.dropGenericTable(identifier); | ||
| } |
There was a problem hiding this comment.
for drop the table should exist only, if it doesn't exist, that seems problem only it is same behaviour as in IcebergCatalog
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
Thanx @dimas-b I had to rebase the PR due to merge conflict by a recent commit, nothing changes it was conflict in the import in the test class |
|
Thanx @flyrain for the review. I have added an entry in the change log |
|
thanks @ayushtkn , there is a merge conflict after the new commit. |
github-project-automation
Bot
moved this from Ready to merge
to Done
in Basic Kanban Board
Fixes: #4778
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)