Skip to content

Conversation

jakedoublev
Copy link
Contributor

Proposed Changes

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

@github-actions github-actions bot added comp:db DB component comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) size/s external-contributor External Org Member labels Jun 4, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @jakedoublev, 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! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly.

This pull request, authored by jakedoublev, aims to remove the legacy support for using deprecated proto actions (STANDARD_ACTION_DECRYPT and STANDARD_ACTION_TRANSMIT) when creating or updating SubjectMapping objects in the policy database service. Previously, the code included specific logic to interpret these deprecated enums and map them to the newer ActionRead and ActionCreate actions, along with logging a warning. This PR removes that compatibility layer, enforcing the use of the current action representation methods (by ID or name).

Highlights

  • Deprecated Action Removal: The core change is the removal of code that specifically handled and translated deprecated standard action proto enums (STANDARD_ACTION_DECRYPT, STANDARD_ACTION_TRANSMIT) into their modern equivalents (ActionRead, ActionCreate) during the creation and update of SubjectMappings.
  • Code Simplification: By removing the legacy handling logic and associated variables, the code for creating and updating SubjectMappings is simplified, relying only on the current methods for specifying actions.

Changelog

  • service/policy/db/subject_mappings.go
    • Removed deprecated variables deprecatedStandardActionDecrypt and deprecatedStandardActionTransmit (lines 52-56).
    • Removed the case statement in CreateSubjectMapping that handled deprecated standard action proto enums and the associated warning log (lines 271-277).
    • Removed the case statement in UpdateSubjectMapping that handled deprecated standard action proto enums and the associated warning log (lines 478-484).
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.


Old code, now removed,
Like leaves falling from a tree,
New growth takes its place.

Footnotes

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively removes support for deprecated proto actions in SubjectMappings creation and updates, which is a positive step for code maintainability and clarity by cleaning up legacy code paths. The changes in service/policy/db/subject_mappings.go are straightforward and directly address the TODO comments related to this cleanup.

However, there are a few points regarding the PR's completeness:

  1. Testing: The checklist items "I have added or updated unit tests" and "I have added or updated integration tests (if appropriate)" are unchecked. When removing functionality, it's crucial to update tests to reflect these changes. This might involve removing tests for the deprecated behavior or ensuring that the new behavior (e.g., erroring out for actions previously handled by the deprecated cases) is correctly tested. Without updated tests, there's a risk that the test suite might provide a false sense of security or that regressions related to this change might not be caught.

  2. Documentation: The checklist item "I have added or updated documentation" is also unchecked. If any internal or external documentation referred to these deprecated actions, it should be updated to reflect their removal.

  3. PR Description: The pull request description is minimal. Providing more context on the changes, even for straightforward removals, helps reviewers and future maintainers understand the PR's scope and impact more quickly.

While the code changes themselves appear correct and well-executed, addressing the testing aspect is important before merging.

Summary of Findings

  • Removal of Deprecated Proto Action Support: The PR successfully removes the deprecated proto action handling from CreateSubjectMapping and UpdateSubjectMapping functions, simplifying the codebase.
  • Test Coverage for Removed Functionality: The PR checklist indicates that unit and integration tests have not been updated. This is a concern as removing functionality requires corresponding test updates to ensure the test suite accurately reflects the current codebase and that the new behavior (erroring on removed actions) is verified. This is considered a medium severity issue.
  • Documentation Update: The PR checklist indicates that documentation has not been updated. Relevant documentation should be checked and updated to reflect the removal of support for these deprecated actions. This is considered a low severity issue and was not commented on directly due to review settings.
  • Pull Request Description: The pull request description is very brief. A more detailed description would improve context for reviewers and for future reference. This is considered a low severity issue and was not commented on directly due to review settings.

Merge Readiness

The code changes for removing deprecated functionality are clear and correctly implemented. However, due to the unchecked items in the PR checklist, particularly regarding test updates, I recommend that this pull request not be merged until the testing aspect is addressed and confirmed. Ensuring that tests are updated to reflect the removal of this functionality is crucial for maintaining code quality and confidence. I am unable to approve this pull request; please ensure other reviewers approve this code after the concerns are addressed.

Copy link
Contributor

github-actions bot commented Jun 4, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 467.461183ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 293.315942ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 350.53388ms
Throughput 285.28 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 53.694626235s
Average Latency 534.787565ms
Throughput 93.12 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4949
Failed Requests 51
Concurrent Requests 50
Total Time 44.669842403s
Average Latency 442.434377ms
Throughput 110.79 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
51 occurrences

Standard Benchmark Metrics Skipped or Failed

@jakedoublev jakedoublev marked this pull request as ready for review June 4, 2025 15:29
@jakedoublev jakedoublev requested review from a team as code owners June 4, 2025 15:29
@jakedoublev jakedoublev enabled auto-merge June 4, 2025 15:31
@jakedoublev jakedoublev added this pull request to the merge queue Jun 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 4, 2025
@jakedoublev jakedoublev added this pull request to the merge queue Jun 4, 2025
Merged via the queue into main with commit 3660200 Jun 4, 2025
31 checks passed
@jakedoublev jakedoublev deleted the fix/action-ci branch June 4, 2025 16:10
github-merge-queue bot pushed a commit that referenced this pull request Jun 6, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.6.0](service/v0.5.5...service/v0.6.0)
(2025-06-06)


### Features

* **authz:** DSPX-894 auth svc registered resource GetEntitlement
support ([#2358](#2358))
([a199aa7](a199aa7))
* **authz:** improve v2 request proto validation
([#2357](#2357))
([f927b99](f927b99))
* **core:** DSPX-608 - Deprecate public_client_id
([#2185](#2185))
([0f58efa](0f58efa))
* **policy:** Return Simple Kas Keys from non-Key RPCs
([#2387](#2387))
([5113e0e](5113e0e))
* **policy:** Unique name for the key provider.
([#2391](#2391))
([bb58b78](bb58b78))
* **policy:** Update simple kas key
([#2378](#2378))
([09d8239](09d8239))


### Bug Fixes

* **deps:** bump github.com/opentdf/platform/protocol/go from 0.3.6 to
0.4.0 in /service
([#2399](#2399))
([1c6fa75](1c6fa75))
* **deps:** bump the external group across 1 directory with 21 updates
([#2401](#2401))
([3d0d4d1](3d0d4d1))
* **policy:** move action sub queries to CTE in sm list and match sql
([#2369](#2369))
([0fd6feb](0fd6feb))
* **policy:** protovalidate deprecated action types and removal of gRPC
gateway in subject mappings svc
([#2377](#2377))
([54a6de0](54a6de0))
* **policy:** remove gRPC gateway in policy except where needed
([#2382](#2382))
([1937acb](1937acb))
* **policy:** remove support for creation/updation of SubjectMappings
with deprecated proto actions
([#2373](#2373))
([3660200](3660200))

---
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:db DB component comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) external-contributor External Org Member size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants