feat(policy): DSPX-2541 implement GetKeyMappingsByFqns#3628
Conversation
Implement the client-side key-split API handler and its DB projection. Per requested value FQN it returns the attribute rule and the effective KAS keys, resolved with value > definition > namespace precedence over the SimpleKasKey model (mirroring sdk/granter.go). GetEntitleableAttributesByFqns is stubbed Unimplemented pending the decisioning migration. Adds a precedence unit test and a DB integration test. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
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 introduces the 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. Keys are mapped with care and grace, / Finding rules in every place. / Value, definition, namespace too, / Resolving keys for all of you. Footnotes
|
github-actions
Bot
added
comp:db
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![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 implements the GetKeyMappingsByFqns endpoint to resolve effective KAS keys using value, definition, and namespace precedence, alongside a stub for GetEntitleableAttributesByFqns. It also includes corresponding integration and unit tests. Feedback recommends optimizing structured logging by using slog.Any instead of string formatting for slices, and adding an early return in the database client when the input FQNs slice is empty to prevent unnecessary database queries.
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.
|
|
||
| mappings, err := s.dbClient.GetKeyMappingsByFqns(ctx, req.Msg) | ||
| if err != nil { | ||
| return nil, db.StatusifyError(ctx, s.logger, err, db.ErrTextGetRetrievalFailed, slog.String("fqns", fmt.Sprintf("%v", req.Msg.GetFqns()))) |
There was a problem hiding this comment.
Using fmt.Sprintf("%v", req.Msg.GetFqns()) inside slog.String formats the slice as a Go-specific string representation (e.g., "[fqn1 fqn2]"), which is less ideal for structured logging systems. Using slog.Any with the slice directly allows structured loggers (like JSON formatters) to serialize it as a proper JSON array, making log querying and analysis much cleaner.
| return nil, db.StatusifyError(ctx, s.logger, err, db.ErrTextGetRetrievalFailed, slog.String("fqns", fmt.Sprintf("%v", req.Msg.GetFqns()))) | |
| return nil, db.StatusifyError(ctx, s.logger, err, db.ErrTextGetRetrievalFailed, slog.Any("fqns", req.Msg.GetFqns())) |
| ctx, span := c.Start(ctx, "DB:GetKeyMappingsByFqns") | ||
| defer span.End() | ||
|
|
||
| pairs, err := c.GetAttributesByValueFqns(ctx, &attributes.GetAttributeValuesByFqnsRequest{Fqns: r.GetFqns()}) |
There was a problem hiding this comment.
If r.GetFqns() is empty, calling GetAttributesByValueFqns will result in an unnecessary database query and potential SQL errors. Adding an early return for empty input avoids an unnecessary DB roundtrip and improves performance.
| pairs, err := c.GetAttributesByValueFqns(ctx, &attributes.GetAttributeValuesByFqnsRequest{Fqns: r.GetFqns()}) | |
| if len(r.GetFqns()) == 0 { | |
| return make(map[string]*attributes.GetKeyMappingsByFqnsResponse_AttributeKeyMapping), nil | |
| } | |
| pairs, err := c.GetAttributesByValueFqns(ctx, &attributes.GetAttributeValuesByFqnsRequest{Fqns: r.GetFqns()}) |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
|
Superseded by the collapsed per-API PRs #3632 (GetKeyMappingsByFqns) and #3633 (GetEntitleableAttributesByFqns). Restructured so each new RPC ships with its proto, generated code, and handler in one PR, matching repo convention (e.g. 4c3d53d) and removing the need for the Unimplemented-handler embed. |
1 participant
Proposed Changes
GetKeyMappingsByFqns, the client-side key-split read API defined in feat(policy): DSPX-2541 add narrow attribute read APIs #3627. Per requested attribute value FQN it returns the governing attributeruleand the effective KAS keys, resolved server-side with value > definition > namespace precedence over theSimpleKasKeymodel, mirroring the client-side resolution insdk/granter.go(newGranterFromService).resolveEffectiveKasKeysand theGetKeyMappingsByFqnsDB method (a projection over the existingGetAttributesByValueFqnsretrieval).GetEntitleableAttributesByFqnsasUnimplemented; it lands in the decisioning-migration PR (it needs new SQL to fetch subject mappings, which the FQN attribute query does not return).Stacked on #3627 (base branch
DSPX-2541-policy-key-mapping-apis). It uses the new protocol/go types via the workspace; once #3627 merges andprotocol/gois released, this rebases ontomainwith aprotocol/godependency bump.Limitation: values configured only with legacy
KeyAccessServergrants (nokas_keys) return an empty key set.kas_keysis the forward model; legacy-grant handling is reconciled with the SDK migration.Checklist
Testing Instructions
go test ./service/policy/db/ -run TestResolveEffectiveKasKeys— precedence unit test.go test ./service/integration/ -run TestAttributesSuite/Test_GetKeyMappingsByFqns— DB integration test (passes against Postgres).golangci-lint run ./policy/db/... ./policy/attributes/... ./integration/...— 0 issues.Related