core: rename Resolvable.CATALOG_ROLES to Resolvable.CALLER_CATALOG_ROLES#3916
core: rename Resolvable.CATALOG_ROLES to Resolvable.CALLER_CATALOG_ROLES#3916sungwy merged 2 commits intoapache:mainfrom
Conversation
…equested CATALOG_ROLES
| * catalog. | ||
| * | ||
| * <p>Note: this currently also covers requested {@code CATALOG_ROLE} names because they share the | ||
| * same {@link PolarisResolutionManifest} registration surface ({@code addTopLevelName}). Although |
There was a problem hiding this comment.
An alternative is to introduce a larger scope change to add REQUESTED_CATALOG_ROLES as a separate enum, and also to add a different registration surface to the PolarisResolutionManifest for requested catalog roles
There was a problem hiding this comment.
It looks like the latest javadoc does not mention this at all... Has the problem been resolved? 😅
There was a problem hiding this comment.
Pull request overview
This PR clarifies resolver selection semantics by renaming the Resolvable option for resolving caller-activated catalog roles, and updates resolver planning so that requesting CATALOG_ROLE as a top-level entity triggers reference catalog resolution when needed.
Changes:
- Renamed
Resolvable.CATALOG_ROLEStoResolvable.CALLER_CATALOG_ROLESand updated related comments. - Updated
Resolver.ResolvePlanplanning to treat requestedCATALOG_ROLEname resolution as reference-catalog-dependent. - Updated/added unit tests around selection-based resolution behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolvable.java | Renames the resolvable constant and clarifies selection semantics in Javadoc. |
| polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java | Extends selection planning to resolve the reference catalog when requested top-level entities include CATALOG_ROLE. |
| polaris-core/src/test/java/org/apache/polaris/core/persistence/ResolverTest.java | Updates existing test for the rename and adds coverage for the new catalog-role/top-level selection behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
polaris-core/src/test/java/org/apache/polaris/core/persistence/ResolverTest.java
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolvable.java
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolvable.java
Outdated
Show resolved
Hide resolved
sungwy
changed the title
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| CALLER_PRINCIPAL_ROLES, | ||
| /** Resolve catalog-role entities (e.g., roles attached in the reference catalog). */ | ||
| CATALOG_ROLES, | ||
| /** Resolve caller-activated catalog-role entities in the reference catalog. */ |
There was a problem hiding this comment.
nit "caller-activated" is understandable, but still a bit cryptic IMHO 😅 How about "Catalog Role entities referenced by the active PolarisPrincipal"?
The "activated" part is generally handled during request authentication, so by the time the request reaches AuthZ, the Principal already has a fixed set of role names in it, IIRC. WDYT?
There was a problem hiding this comment.
That's exactly what I meant by that - caller-activated as in, the catalog-roles that were activated by the caller...
There was a problem hiding this comment.
since this is a nit, and it would be nice to get this in before the release, I will proceed to merge the change in for now
There was a problem hiding this comment.
This distinction between "activated" vs "granted" comes from the very initial stages of Polaris, afaik.
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
Resolvable enum was recently introduced to enable the caller to select specific entities for resolution rather than resolving everything. See PR: #3760
This PR renames CATALOG_ROLES to CALLER_CATALOG_ROLES instead to make it obvious whether we are resolving the principal's activated catalog roles vs the catalog roles that are requested as the target of the action (like creating a CATALOG_ROLE).
Also added a private helper to detect if CATALOG_ROLE is requested as a part of the top level entities and would require referenced catalog resolution.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)