feat(policy): DSPX-2541 implement GetEntitleableAttributesByFqns#3629
feat(policy): DSPX-2541 implement GetEntitleableAttributesByFqns#3629alkalescent wants to merge 1 commit into
Conversation
Implement the server-side decisioning read API. Per requested attribute value FQN it returns the attribute rule, value id, the definition's ordered value FQNs (for hierarchy rule logic), and the value-level subject mappings. Adds a lean getSubjectMappingsByValueFqns SQL query so subject mappings are fetched by FQN in a single roundtrip instead of the full-policy load. Adds an 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 implements 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. The query runs with speed and grace, To find the FQN in its place. With mappings grouped and rules in sight, The policy engine shines so bright. 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 GetEntitleableAttributesByFqns endpoint to retrieve attribute rules, value identities, ordered value FQNs, and subject mappings for entitlement resolution. It includes a new database query, service and DB client implementations, and integration tests. The feedback focuses on performance optimizations, such as casting actions to jsonb in SQL to avoid redundant JSON marshaling in Go, adding an early return for empty FQN inputs, and using slog.Any for cleaner logging of slices.
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.
| SELECT | ||
| sm.id, | ||
| fqns.fqn AS value_fqn, | ||
| sa.standard_actions, | ||
| sa.custom_actions, |
There was a problem hiding this comment.
By explicitly casting sa.standard_actions and sa.custom_actions to jsonb in the SQL query, sqlc will generate []byte fields in the Go struct instead of interface{}. This avoids the need to marshal them back to JSON in Go, improving performance and type safety.
SELECT
sm.id,
fqns.fqn AS value_fqn,
sa.standard_actions::jsonb AS standard_actions,
sa.custom_actions::jsonb AS custom_actions,| SELECT | ||
| sm.id, | ||
| fqns.fqn AS value_fqn, | ||
| sa.standard_actions, | ||
| sa.custom_actions, |
There was a problem hiding this comment.
Update the generated query constant to match the SQL cast to jsonb.
| SELECT | |
| sm.id, | |
| fqns.fqn AS value_fqn, | |
| sa.standard_actions, | |
| sa.custom_actions, | |
| SELECT | |
| sm.id, | |
| fqns.fqn AS value_fqn, | |
| sa.standard_actions::jsonb AS standard_actions, | |
| sa.custom_actions::jsonb AS custom_actions, |
| type getSubjectMappingsByValueFqnsRow struct { | ||
| ID string `json:"id"` | ||
| ValueFqn string `json:"value_fqn"` | ||
| StandardActions interface{} `json:"standard_actions"` | ||
| CustomActions interface{} `json:"custom_actions"` | ||
| Metadata []byte `json:"metadata"` | ||
| SubjectConditionSet []byte `json:"subject_condition_set"` | ||
| AttributeValue []byte `json:"attribute_value"` | ||
| } |
There was a problem hiding this comment.
Update the generated struct fields to []byte to match the jsonb cast in the SQL query.
| type getSubjectMappingsByValueFqnsRow struct { | |
| ID string `json:"id"` | |
| ValueFqn string `json:"value_fqn"` | |
| StandardActions interface{} `json:"standard_actions"` | |
| CustomActions interface{} `json:"custom_actions"` | |
| Metadata []byte `json:"metadata"` | |
| SubjectConditionSet []byte `json:"subject_condition_set"` | |
| AttributeValue []byte `json:"attribute_value"` | |
| } | |
| type getSubjectMappingsByValueFqnsRow struct { | |
| ID string `json:"id"` | |
| ValueFqn string `json:"value_fqn"` | |
| StandardActions []byte `json:"standard_actions"` | |
| CustomActions []byte `json:"custom_actions"` | |
| Metadata []byte `json:"metadata"` | |
| SubjectConditionSet []byte `json:"subject_condition_set"` | |
| AttributeValue []byte `json:"attribute_value"` | |
| } |
| stdActionsBytes, err := json.Marshal(row.StandardActions) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal standard actions: %w", err) | ||
| } | ||
| customActionsBytes, err := json.Marshal(row.CustomActions) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal custom actions: %w", err) | ||
| } | ||
| actions := []*policy.Action{} | ||
| if err := unmarshalAllActionsProto(stdActionsBytes, customActionsBytes, &actions); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Since row.StandardActions and row.CustomActions are now []byte (thanks to the jsonb cast in SQL), we can completely eliminate the redundant json.Marshal calls. This significantly improves performance by avoiding reflection and extra allocations.
| stdActionsBytes, err := json.Marshal(row.StandardActions) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to marshal standard actions: %w", err) | |
| } | |
| customActionsBytes, err := json.Marshal(row.CustomActions) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to marshal custom actions: %w", err) | |
| } | |
| actions := []*policy.Action{} | |
| if err := unmarshalAllActionsProto(stdActionsBytes, customActionsBytes, &actions); err != nil { | |
| return nil, err | |
| } | |
| actions := []*policy.Action{} | |
| if err := unmarshalAllActionsProto(row.StandardActions, row.CustomActions, &actions); err != nil { | |
| return nil, err | |
| } |
| fqns := r.GetFqns() | ||
| normalized := make([]string, len(fqns)) | ||
| for i, fqn := range fqns { | ||
| normalized[i] = strings.ToLower(fqn) | ||
| } |
There was a problem hiding this comment.
If the requested slice of FQNs is empty, we can return early and avoid executing any database queries.
fqns := r.GetFqns()
if len(fqns) == 0 {
return make(map[string]*attributes.GetEntitleableAttributesByFqnsResponse_EntitleableAttribute), nil
}
normalized := make([]string, len(fqns))
for i, fqn := range fqns {
normalized[i] = strings.ToLower(fqn)
}|
|
||
| entitleable, err := s.dbClient.GetEntitleableAttributesByFqns(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.
Use slog.Any instead of slog.String with fmt.Sprintf to log the slice of FQNs. This is cleaner, more idiomatic, and avoids string formatting overhead.
| 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())) |
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
GetEntitleableAttributesByFqns, the server-side decisioning read API defined in feat(policy): DSPX-2541 add narrow attribute read APIs #3627 (replaces theUnimplementedstub from feat(policy): DSPX-2541 implement GetKeyMappingsByFqns #3628). Per requested attribute value FQN it returns the attributerule, the value id, the definition's ordered value FQNs (for hierarchy rule logic), and the value-level subject mappings.getSubjectMappingsByValueFqnsSQL query (sqlc) so subject mappings are fetched by FQN in a single roundtrip, instead of the fullListAllSubjectMappingsload the entitlement path uses today. The handler runs two selective queries (attribute FQN lookup + subject-mapping-by-FQN) and groups results into an FQN-keyed map.Stacked on #3628 (base
DSPX-2541-attribute-read-api-handlers). This delivers the API only; migrating the v1/v2 decisioning + PDP consumers onto it and adding namespace-scoped fetch is a separate PR (it depends on the tenancy model, still under discussion).Checklist
Testing Instructions
go test ./service/integration/ -run TestAttributesSuite/Test_GetEntitleableAttributesByFqns— passes against Postgres (verifies rule, value id, ordered definition FQNs, and subject mappings grouped by FQN).golangci-lint run ./policy/db/... ./policy/attributes/... ./integration/...— 0 issues.Related