feat(policy): hydrate resource_mappings on ListAttributes values#3398
feat(policy): hydrate resource_mappings on ListAttributes values#3398KevLehman wants to merge 2 commits into
Conversation
ListAttributes previously returned each Value with only id/value/fqn/active, forcing clients that need terms or groups to follow up with GetAttributeValuesByFqns. Extend listAttributesDetail with a pre-aggregated value_resource_mappings CTE (mirrors listAttributesByDefOrValueFqns) and emit 'resource_mappings' in the per-value JSON so the response aligns with the already-documented OpenAPI shape and with GetAttributeValuesByFqns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Kevin Aleman <kaleman960@gmail.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 51 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds support for hydrating Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/policy/db/queries/attributes.sql`:
- Around line 6-26: The value_resource_mappings CTE currently aggregates the
entire resource_mappings table (resource_mappings rm) which can be wasteful; to
scope it, join or filter rm by only the attribute_value_id /
attribute_definition IDs used by the outer query (e.g., restrict rm via the avt
subquery or by joining against attribute_definitions referenced later) so the
CTE only aggregates mappings for the selected
attribute_definitions/attribute_values instead of the whole table; adjust the
FROM/JOIN in value_resource_mappings to reference avt or the
attribute_definition id set so the GROUP BY and produced res_maps remain
identical but are computed only for relevant rows.
🪄 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: 608783a4-6e12-49e8-ba21-57b0c1bd889e
📒 Files selected for processing (3)
service/integration/attributes_test.goservice/policy/db/attributes.sql.goservice/policy/db/queries/attributes.sql
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 updates the ListAttributes service to automatically hydrate resource_mappings for attribute values. By modifying the underlying SQL query to include a pre-aggregated CTE, the change allows clients to retrieve necessary label and term data in a single round trip, eliminating the need for subsequent batch-hydration calls and ensuring consistency with the documented API schema. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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 was slow and quite lean, Now mappings are clearly seen. With a CTE join, We save every coin, And keep the API pristine. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the listAttributesDetail SQL query to include resource mapping data for attribute values and adds an integration test to verify this hydration. The review feedback identifies a significant performance concern regarding the use of a global Common Table Expression (CTE) for aggregating resource mappings, which could lead to degradation as the database grows. It is recommended to replace the global CTE with a correlated subquery within the JSON construction to ensure aggregation only occurs for the filtered results.
Restrict the aggregation in the new CTE to resource_mappings whose attribute_value belongs to an attribute_definition matching the same active/namespace_id/namespace_name filters already applied to the outer listAttributesDetail query. Without this, the CTE hash-aggregated the entire resource_mappings table on every call even when the caller narrowed the list by namespace or state. Same rows and JSON shape are produced; work is bounded by the caller's filter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Kevin Aleman <kaleman960@gmail.com>
|
@KevLehman Thanks so much for contributing! We discussed as a team and, while this may be a useful thing to have back from |
|
Hey, thanks for checking. Would you be open to do this change on the API only? when calling v2.ListAttributes? That way, we would have the information needed without having to do a 2nd (+n) paginated call to ListResourceMappings and map locally |
|
Hi @KevLehman — thanks again for the PR! Of the use cases you list (UI pickers, classifiers, policy editors), the classifier path is exactly what the Virtru Data Security Platform Tagging Service is built for. Instead of clients pulling label/term data and resolving locally, you send content (or already-extracted tags) to the Tagging Service and get back normalized attribute FQNs — synonym dictionary and resource mappings stay server-side, no client enumeration needed. Hand-rolled in client:
With the Tagging Service:
One thing worth calling out: keeping that logic server-side means changes to the synonym dictionary, resource mappings, or extraction rules land as Tagging Service config inside DSP — no client redeploy, no SDK upgrade, and no version skew between what your client thinks the labels are and what the policy says they are. We're also actively working on an SDK helper that wraps Glad to walk through the Tagging Service fit if useful. -- Mary |
… response (opentdf#3472) ## Summary `GetRegisteredResource` returned each value with only `id`, `value`, and `fqn` populated — the `action_attribute_values` field was always empty even when mappings existed. Callers that authorize on `(resource, action) → attributes` via this RPC were silently denied; `GetRegisteredResourceValue` returned the same value with mappings populated, exposing the divergence. Root cause: the `getRegisteredResource` SQL in `service/policy/db/queries/registered_resources.sql` built each value's JSON object with only `id` and `value`. `listRegisteredResources` already used the correct `LEFT JOIN LATERAL` pattern; this PR mirrors it into the single-resource query and regenerates the sqlc binding. The follow-up commit also wraps the inner `JSON_AGG` in both queries with `COALESCE(..., '[]'::json)` so a value with no mappings produces an empty array rather than SQL NULL. Fixes opentdf#3471 ## Why this is different from opentdf#3398 PR opentdf#3398 (also a "hydrate a nested field that some queries populate and others don't" change, on `ListAttributes`) was closed with the explanation that `ListAttributes` is on a lot of hot paths and the team would rather pursue selectors/expanders than expand the default response payload. That concern is real for paginated list RPCs but does not apply to this change: - **Bug fix, not feature expansion.** `GetRegisteredResource` is the only sibling that fails to populate `action_attribute_values`. `ListRegisteredResources` and `GetRegisteredResourceValue` both already hydrate it. The proto field `RegisteredResourceValue.action_attribute_values` is declared and documented; leaving it empty here is divergence, not an opt-in payload addition. - **Silent authorization failures in production.** Downstream callers that read `value.GetActionAttributeValues()` off the `GetRegisteredResource` response see an empty slice and deny the request with `"no attribute mappings found for resource"`, even though the mappings exist in the database and are returned by every sibling RPC. This is the bug described in opentdf#3471 — not a payload-completeness preference. - **Not a hot-path list endpoint.** `GetRegisteredResource` is a single-item lookup by id or name. The cardinality argument that motivated closing opentdf#3398 (thousands of attribute values × N mappings each, on a frequently-called paginated endpoint) does not transfer. - **No proto change, no new field, no new opt-in toggle.** The shape this PR returns is already what `listRegisteredResources` returns today. If the team would prefer to address this through a `GetRegisteredResourceWithMappings` selector or an `expand=action_attribute_values` parameter instead of fixing the divergence inline, happy to redirect — but the silent-denial bug is real and the rest of the policy DB queries already treat populating this field as the default. ## Test plan - [x] Wrote failing integration test first; confirmed it fails against unfixed SQL with `Should NOT be empty, but was []` (red-green verified). - [x] Confirmed it passes after the SQL fix. - [x] Full `TestRegisteredResourcesSuite` integration suite passes (testcontainers/postgres) — both the new test and the existing fixture-based tests against values with no AAVs. - [x] `go test ./service/policy/...` unit tests pass. - [x] `golangci-lint run` on changed files: 0 new issues. - [x] `gofumpt` clean on changed Go files. - [ ] Reviewer: re-verify with `cd service && go test ./integration/ -run TestRegisteredResourcesSuite -count=1`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added integration test for retrieving registered resources with action attribute values. * Enhanced existing test with stricter validation of namespaced action information and attribute identifiers. * **Bug Fixes** * Registered resources now properly return action attribute values with complete namespace details when fetched. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/opentdf/platform/pull/3472) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com> Co-authored-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Summary
policy.attributes.AttributesService/ListAttributesnow returnsresource_mappingson everyValue, matching the shape already produced byGetAttributeValuesByFqnsand already advertised in the generated OpenAPI spec.listAttributesDetailwith a pre-aggregatedvalue_resource_mappingsCTE (same pattern used bylistAttributesByDefOrValueFqns), so mappings are hydrated without introducing row-explosion through the mainGROUP BY.Before / after
Before (trimmed):
{ "id": "…", "value": "topsecret", "fqn": "https://demo.com/attr/classification/value/topsecret", "active": true }After:
{ "id": "…", "value": "topsecret", "fqn": "https://demo.com/attr/classification/value/topsecret", "active": true, "resourceMappings": [ { "id": "…", "terms": ["ts", "top-secret", "top_secret"] } ] }Reproduction:
grpcurl -plaintext -d '{"pagination":{"limit":500}}' \ localhost:8080 policy.attributes.AttributesService/ListAttributesWhy
Clients that need label/term data (UI pickers, classifiers, policy editors) currently have to enumerate FQNs via
ListAttributesand then batch‑hydrate them throughGetAttributeValuesByFqns(250 FQN cap per call). The OpenAPI spec already documentsresourceMappingsonListAttributes, so the current server response is inconsistent with the schema clients read. Hydrating directly collapses the two round trips into one.Implementation notes
listAttributesDetailgets a new CTE (value_resource_mappings) keyed byattribute_value_id, producing aJSON_AGGof{id, terms, group}per value — exactly the shapelistAttributesByDefOrValueFqnsemits.avtsubquery byav.id. Row count per attribute definition is unchanged (one row per value), soJSON_AGG … FILTER (WHERE avt.id IS NOT NULL)and theGROUP BY ad.id, n.name, fqns.fqn+COUNT(*) OVER()pagination continue to behave the same.Valuealready declaresrepeated ResourceMapping resource_mappings = 10;, so the Go DAO (attributesValuesProtojson→protojson.Unmarshal) picks up the new field automatically. No scanning code changed.GetAttributeis intentionally out of scope — it uses the separategetAttributequery and was left unchanged.Out of scope
subject_mappings/kas_keys/grants/obligations/ value-levelmetadatahydration onListAttributes. Each is its own JOIN and its own tradeoff, tracked as follow-ups.Test plan
go build ./service/...go vet ./service/policy/... ./service/integration/...golangci-lint run -c .golangci.yaml ./policy/db/...— 0 issuesgo test ./service/policy/...— 761 passedgo test -run '^TestAttributesSuite$' ./service/integration/...— 92 passed (includes newTest_ListAttributes_HydratesResourceMappings)go test -run '^TestAttributeFqnSuite$' ./service/integration/...— 38 passedgo test -run '^TestResourceMappingsSuite$|^TestAttributeValuesSuite$|^TestNamespacesSuite$' ./service/integration/...— 161 passed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests