feat(sdk): resolve key splits via GetKeyMappingsByFqns#3699
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe granter now prefers server-side key mappings, falls back to attribute-based grants when mappings are unavailable or incomplete, and centralizes the legacy grant-resolution path. Tests cover unimplemented, empty, and mixed mapping cases. ChangesGranter Key Mapping Fallback
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
comp:sdk
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes the SDK's key split resolution process by migrating from a full attribute value walk to a more direct RPC-based approach. It introduces a new service call while maintaining backward compatibility through strategic fallbacks, ensuring that existing grant-based policies and older platform versions continue to function correctly. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The granter now calls out with speed, / To fetch the keys that we all need. / If old platforms fail to comply, / The fallback path will satisfy. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors newGranterFromService in sdk/granter.go to resolve KAS grants using the more efficient GetKeyMappingsByFqns endpoint, falling back to GetAttributeValuesByFqns for older platforms or unconfigured keys. It also adds corresponding unit tests to verify this fallback behavior. The review feedback identifies potential nil pointer dereference panics when handling RPC response maps, suggests iterating over the requested FQNs to ensure all are processed or properly fell back, and recommends returning zero-value structs on error paths to follow Go best practices.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
alkalescent
force-pushed
the
DSPX-2541-attribute-read-apis
branch
from
a40a66d to
3239ed4
Compare
alkalescent
force-pushed
the
DSPX-2541-sdk-granter
branch
from
d00f147 to
5968161
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Switch newGranterFromService to resolve effective KAS keys through the new GetKeyMappingsByFqns RPC (server-side value > definition > namespace precedence). Values that return no mapped keys (legacy-grant-only) fall back per-value to GetAttributeValuesByFqns, and older platforms that do not expose the RPC (Unimplemented) fall back entirely, so behavior is preserved for un-migrated policy and older servers. Adds fallback parity unit tests. Signed-off-by: Krish Suchak <[email protected]>
alkalescent
force-pushed
the
DSPX-2541-sdk-granter
branch
from
5968161 to
132f66f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/granter.go`:
- Around line 432-435: The grant aggregation in
GetKeyMappingsByFqns/GetFqnKeyMappings only processes entries returned by the
response map, so requested FQNs that are missing or empty can be skipped
entirely. Update the loop that builds grants/fallback to iterate over the
original fqns list, normalize each key the same way, and for each requested FQN
either map it from the response or add it to fallback when the mapping is absent
or empty. Add a test covering a successful RPC with an empty response map for a
requested FQN to verify the granter still falls back correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b162f14-812e-430d-a865-1a59b75d0470
📒 Files selected for processing (2)
sdk/granter.gosdk/granter_test.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
- iterate requested FQNs (not the response map) in newGranterFromService so a
value omitted from GetKeyMappingsByFqns falls back instead of being dropped;
rely on nil-safe protobuf getters (no redundant present/nil guard)
- return granter{} on error paths
- test the FQN-omitted-from-response fallback path
Signed-off-by: Krish Suchak <[email protected]>
alkalescent
force-pushed
the
DSPX-2541-sdk-granter
branch
from
eb23dff to
a1b8636
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
c-r33d
left a comment
c-r33d
left a comment
There was a problem hiding this comment.
I don't see any issues. Are there any tests that mix grants and mappings together? I looked through the old granter_test.go code and don't really see any. Do you think it would be useful to add at least one test where we have 2 fqns with mappings and 2 with grant only?
Signed-off-by: Krish Suchak <[email protected]>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: Krish Suchak <[email protected]>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: Krish Suchak <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/granter_test.go`:
- Around line 914-918: The test is deduplicating the grant plan by KAS, which
hides duplicate mapped-key entries and lost split IDs. Update the assertion in
the granter test to validate the full `plan` shape instead of `maps.Keys(got)`,
or at minimum assert per-entry counts and that mapped-key items carry the
expected non-empty split IDs using the relevant plan-building logic in
`granter_test.go`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98ea7c93-3151-4e0d-a44c-6afadf64b7cc
📒 Files selected for processing (2)
sdk/granter.gosdk/granter_test.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: Krish Suchak <[email protected]>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
4 participants
Proposed Changes
Switch the SDK granter to resolve key splits through the new
GetKeyMappingsByFqnsRPC instead of walking the full attribute set fromGetAttributeValuesByFqns.newGranterFromServicenow callsGetKeyMappingsByFqnsand feeds the returned per-value rule + effective KAS keys (server-side value > definition > namespace precedence) into the existing split planner.GetAttributeValuesByFqns.Unimplemented) fall back entirely.TestReasonerKeyMappingFallbackcovering both fallback paths; existing reasoner tests now exercise the new path (mapped keys) and the fallback (grants) and still pass.Stack
Depends on #3634 (proto + generated +
sdkconnectwrapper). Base is the #3634 branch; rebase ontomainand bumpsdk/go.modto the releasedprotocol/goonce #3634 merges.Follow-up
An xtest in
opentdf/testswill prove the new go SDK produces the same TDF key splits as a previous released SDK (cross-SDK parity).Summary by CodeRabbit
New Features
Bug Fixes
Tests