fix(policy): include action_attribute_values in GetRegisteredResource response#3472
Conversation
… response The getRegisteredResource SQL query built each value's JSON object with only id and value, omitting action_attribute_values even when mappings existed. Callers that authorize on (resource, action) -> attributes via GetRegisteredResource were silently denied because the field came back empty, while GetRegisteredResourceValue for the same value id returned the populated mappings. Mirror the LATERAL join pattern already used in listRegisteredResources so action_attribute_values is aggregated per resource value, and add a matching integration test (the existing list-side test masked the gap). Fixes opentdf#3471 Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughQueries for registered resources now include per-value ChangesAction-Attribute-Values propagation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
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 addresses a data inconsistency where 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 was missing a join, Data hidden, a point to adjoin. With lateral power, We fixed the dark hour, And now the results will purloin. Footnotes
|
github-actions
Bot
added
comp:db
![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 enhances the retrieval of registered resources by updating the database queries to include action attribute values. The SQL logic now utilizes a lateral join to aggregate action and attribute metadata, including namespaces and fully qualified names (FQNs), into the JSON output for resource values. A new integration test was added to verify that these attributes are correctly returned and namespaced. Feedback was provided to refactor the verification loop in the new test to reduce code duplication and improve maintainability.
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Fixes a divergence in the policy DB layer where GetRegisteredResource did not populate RegisteredResourceValue.action_attribute_values, causing authorization logic depending on (resource, action) → attributes to deny requests despite mappings existing.
Changes:
- Updated
getRegisteredResourceSQL to LATERAL-join and aggregate action/attribute-value mappings into each returned resource value. - Regenerated
sqlcbindings to reflect the updated SQL shape in the Go DB client. - Added an integration test asserting
GetRegisteredResourcereturns values with populatedaction_attribute_values.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| service/policy/db/registered_resources.sql.go | Regenerated sqlc output for getRegisteredResource to include nested action_attribute_values. |
| service/policy/db/queries/registered_resources.sql | Mirrors the existing list-query LATERAL join pattern into the single-resource query to populate action_attribute_values. |
| service/integration/registered_resources_test.go | Adds an integration test verifying GetRegisteredResource includes action/attribute mappings on returned values. |
Files not reviewed (1)
- service/policy/db/registered_resources.sql.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ests Address review feedback on both the get-side and list-side together to keep the two query/test pairs symmetric. - SQL: wrap the inner JSON_AGG in both getRegisteredResource and listRegisteredResources LATERAL subqueries with COALESCE(..., '[]'::json) so a value with no action/attribute mappings produces an empty JSON array instead of SQL NULL. protojson tolerates null for repeated fields, but the array shape is a cleaner contract for downstream consumers. - Tests: replace the duplicated if/if value-lookup with a switch and a default: FailNow branch in both Test_GetRegisteredResource_... and Test_ListRegisteredResources_... so an unexpected value id fails the test instead of being silently skipped. No behavior change for callers; sqlc binding regenerated. Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Address review feedback from @jakedoublev on opentdf#3472: - SQL: drop the LEFT JOIN against attribute_fqns for the action's namespace FQN in both getRegisteredResource and listRegisteredResources LATERAL subqueries; namespace FQN is deterministic from the name, so build it inline as CONCAT('https://', ans.name). Mirrors the pattern used by service/policy/db/queries/obligations.sql. - Tests: tighten both Test_GetRegisteredResource_... and Test_ListRegisteredResources_... so each value asserts the SPECIFIC action name and attribute value FQN that was created (e.g. val1 -> ActionNameCreate + .../attr1/value/value1), instead of merely checking that the returned action and attribute value were non-nil. Pulls the per-AAV checks out of the value-id switch so the structural assertions run once per value. Signed-off-by: Paul Flynn <pflynn-virtru@users.noreply.github.com>
Summary
GetRegisteredResourcereturned each value with onlyid,value, andfqnpopulated — theaction_attribute_valuesfield was always empty even when mappings existed. Callers that authorize on(resource, action) → attributesvia this RPC were silently denied;GetRegisteredResourceValuereturned the same value with mappings populated, exposing the divergence.Root cause: the
getRegisteredResourceSQL inservice/policy/db/queries/registered_resources.sqlbuilt each value's JSON object with onlyidandvalue.listRegisteredResourcesalready used the correctLEFT JOIN LATERALpattern; this PR mirrors it into the single-resource query and regenerates the sqlc binding. The follow-up commit also wraps the innerJSON_AGGin both queries withCOALESCE(..., '[]'::json)so a value with no mappings produces an empty array rather than SQL NULL.Fixes #3471
Why this is different from #3398
PR #3398 (also a "hydrate a nested field that some queries populate and others don't" change, on
ListAttributes) was closed with the explanation thatListAttributesis 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:GetRegisteredResourceis the only sibling that fails to populateaction_attribute_values.ListRegisteredResourcesandGetRegisteredResourceValueboth already hydrate it. The proto fieldRegisteredResourceValue.action_attribute_valuesis declared and documented; leaving it empty here is divergence, not an opt-in payload addition.value.GetActionAttributeValues()off theGetRegisteredResourceresponse 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 GetRegisteredResource omits action_attribute_values from nested values #3471 — not a payload-completeness preference.GetRegisteredResourceis a single-item lookup by id or name. The cardinality argument that motivated closing feat(policy): hydrate resource_mappings on ListAttributes values #3398 (thousands of attribute values × N mappings each, on a frequently-called paginated endpoint) does not transfer.listRegisteredResourcesreturns today.If the team would prefer to address this through a
GetRegisteredResourceWithMappingsselector or anexpand=action_attribute_valuesparameter 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
Should NOT be empty, but was [](red-green verified).TestRegisteredResourcesSuiteintegration suite passes (testcontainers/postgres) — both the new test and the existing fixture-based tests against values with no AAVs.go test ./service/policy/...unit tests pass.golangci-lint runon changed files: 0 new issues.gofumptclean on changed Go files.cd service && go test ./integration/ -run TestRegisteredResourcesSuite -count=1.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Bug Fixes