fix(policy): DSPX-1151 update of registered resource value always clears existing action attribute values #2325
Conversation
github-actions
bot
added
external-contributor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @ryanulit, 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!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request.
This PR addresses a bug (DSPX-1151) in the policy service where updating a registered resource value would incorrectly clear any existing associated action attribute values. The core issue was that the update logic unconditionally deleted existing action attribute values before attempting to add new ones. This meant that if an update request didn't include action attribute values, the existing ones would still be wiped out.
The fix implemented in this PR modifies the update logic to only perform the deletion and re-creation of action attribute values if the update request explicitly provides a non-empty list of them. This ensures that existing action attribute values are preserved when an update occurs without specifying new ones.
Additionally, the integration test for updating registered resource values has been enhanced to include action attribute values during both creation and update steps. This updated test now correctly verifies that after an update with new action attribute values, the resource value contains the expected new set, confirming the fix behaves as intended when action attribute values are provided in the update.
Highlights
- Conditional Update Logic: Modifies the database update logic for registered resource values to only delete and re-create action attribute values if the update request provides a non-empty list.
- Integration Test Enhancement: Updates the
Test_UpdateRegisteredResourceValue_Succeedsintegration test to include action attribute values and verify the correct state after an update.
Changelog
- service/integration/registered_resources_test.go
- Added action attribute values to the
CreateRegisteredResourceValueRequestinTest_UpdateRegisteredResourceValue_Succeeds(lines 1058-1067). - Updated assertion to expect 1 action attribute value after creation (line 1092).
- Added logic to define and store expected action attribute values for the update step (lines 1094-1117).
- Updated
UpdateRegisteredResourceValueRequestto use the new set of action attribute values (line 1127). - Modified assertions after the update to check for 2 action attribute values and verify their content against the expected set (lines 1148-1156).
- Added action attribute values to the
- service/policy/db/registered_resources.go
- Wrapped the logic for deleting and creating action attribute values within a check
if len(actionAttrValues) > 0in theUpdateRegisteredResourceValuefunction (lines 434-444).
- Wrapped the logic for deleting and creating action attribute values within a check
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A value updated, a bug took hold,
Attributes vanished, a story untold.
Now code checks the list,
With a conditional twist,
Existing ones stay, brave and bold.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request addresses bug DSPX-1151, where updating a registered resource value would always clear existing action attribute values (AAVs). The core fix in service/policy/db/registered_resources.go correctly modifies the logic to only clear and recreate AAVs if new ones are explicitly provided in the update request. This effectively prevents unintentional clearing.
The accompanying changes in service/integration/registered_resources_test.go update the Test_UpdateRegisteredResourceValue_Succeeds test to reflect this new behavior, specifically testing the scenario where AAVs are overwritten when new ones are supplied. This is a good step.
However, to ensure comprehensive testing of the fix, a scenario where an update occurs without providing AAVs (to verify preservation of existing AAVs) is currently missing. Additionally, there's a minor opportunity for code refinement in the database logic.
It would also be beneficial for future maintainability if the pull request description could be expanded with details on the changes and the checklist filled out.
Summary of Findings
- Missing Test Case for AAV Preservation: The tests verify that AAVs are overwritten when new ones are provided in an update. However, a crucial scenario is missing: testing that existing AAVs are preserved when an update request is made without specifying any
ActionAttributeValues. This is key to fully validating the fix for DSPX-1151. - Code Refinement in DB Logic: In
UpdateRegisteredResourceValue,r.GetActionAttributeValues()is called a second time within the conditional block, even though its result is already stored in theactionAttrValuesvariable. Reusing the variable would be slightly cleaner. - Pull Request Housekeeping: The pull request description is currently minimal, and the checklist is unchecked. Expanding the description with details about the changes and ensuring the checklist is completed would be beneficial for reviewers and future maintainability.
Merge Readiness
This pull request correctly addresses the core issue of unintentional AAV clearing. However, due to the high severity issue regarding the missing test case for AAV preservation, I recommend that these changes not be merged until this test is added. Addressing the minor code refinement would also be good. I am not authorized to approve pull requests, so please ensure further review and approval from authorized team members after addressing the feedback.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
Error Summary:
Standard Benchmark Metrics Skipped or Failed |
ryanulit
changed the title
ryanulit
dismissed
gemini-code-assist[bot]’s stale review
remaining requested change was already done in PR
ryanulit
deleted the
DSPX-1151-rrv-update-deletes-existing-without-new-provided
branch
🤖 I have created a release *beep* *boop* --- ## [0.5.4](service/v0.5.3...service/v0.5.4) (2025-05-29) ### Features * **authz:** access pdp v2 with actions ([#2264](#2264)) ([7afefb7](7afefb7)) * **authz:** logic for authz v2 (actions within ABAC decisioning) ([#2146](#2146)) ([0fdc259](0fdc259)) * **policy:** Default Platform Keys ([#2254](#2254)) ([d7447fe](d7447fe)) * **policy:** Update key status's and UpdateKey rpc. ([#2315](#2315)) ([7908db9](7908db9)) ### Bug Fixes * **policy:** DSPX-1151 update of registered resource value always clears existing action attribute values ([#2325](#2325)) ([ca94425](ca94425)) * **policy:** Ensure non active keys cannot be assigned. ([#2321](#2321)) ([207d10d](207d10d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: Elizabeth Healy <[email protected]>
…ars existing action attribute values (opentdf#2325) ### Proposed Changes * fixes bug where action attribute values are blindly cleared during an update, even if no new action attribute values are provided in the update request ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions
🤖 I have created a release *beep* *boop* --- ## [0.5.4](opentdf/platform@service/v0.5.3...service/v0.5.4) (2025-05-29) ### Features * **authz:** access pdp v2 with actions ([opentdf#2264](opentdf#2264)) ([7afefb7](opentdf@7afefb7)) * **authz:** logic for authz v2 (actions within ABAC decisioning) ([opentdf#2146](opentdf#2146)) ([0fdc259](opentdf@0fdc259)) * **policy:** Default Platform Keys ([opentdf#2254](opentdf#2254)) ([d7447fe](opentdf@d7447fe)) * **policy:** Update key status's and UpdateKey rpc. ([opentdf#2315](opentdf#2315)) ([7908db9](opentdf@7908db9)) ### Bug Fixes * **policy:** DSPX-1151 update of registered resource value always clears existing action attribute values ([opentdf#2325](opentdf#2325)) ([ca94425](opentdf@ca94425)) * **policy:** Ensure non active keys cannot be assigned. ([opentdf#2321](opentdf#2321)) ([207d10d](opentdf@207d10d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com> Co-authored-by: Elizabeth Healy <[email protected]>
Proposed Changes
Checklist
Testing Instructions