feat(policy): cache SubjectConditionSet selectors in dedicated column maintained via trigger #2320
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…very level for trigger to consume
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.
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 team, Gemini here with a summary of this pull request from @jakedoublev.
The primary goal of this PR is to improve the performance of the MatchSubjectMappings RPC, which is crucial for entitlements. It achieves this by introducing a caching mechanism for the selectors used within Subject Condition Sets. Instead of parsing the condition JSONB column on every read operation in the matchSubjectMappings query, the relevant selector values are now extracted and stored in a dedicated selector_values array column on the subject_condition_set table.
This new selector_values column is automatically maintained via a database trigger whenever a Subject Condition Set is inserted or updated. The matchSubjectMappings query is updated to leverage this cached array using PostgreSQL's array overlap operator (&&), which is significantly faster, especially when combined with a GIN index added to the new column.
Additionally, this PR includes improvements to the protovalidate implementation for creating Subject Mappings and Subject Condition Sets, ensuring that required fields and structures are present.
Highlights
- Performance Optimization: Introduced a caching mechanism for subject selectors in the database to speed up the
MatchSubjectMappingsRPC. - Database Schema Changes: Added a new
selector_valuesarray column to thesubject_condition_settable and implemented a trigger to keep it updated. - Database Indexing: Added a GIN index on the new
selector_valuescolumn and other relevant indices to optimize thematchSubjectMappingsquery. - Improved Validation: Enhanced
protovalidaterules and tests for Subject Mapping and Subject Condition Set creation requests.
Changelog
Click here to see the changelog
- protocol/go/policy/subjectmapping/subject_mapping.pb.go
- Updated generated Go code for the subject mapping protobuf definitions.
- service/integration/subject_mappings_test.go
- Updated
TestDeleteSubjectConditionSetto use a more complex Subject Condition Set structure, likely to ensure compatibility with the new caching/validation logic.
- Updated
- service/policy/db/migrations/20250527000000_match_subject_mappings_func.md
- Added documentation for the database migration, explaining the cached selectors approach via trigger and the addition of indices.
- service/policy/db/migrations/20250527000000_match_subject_mappings_func.sql
- Created a SQL function
extract_selector_valuesto parse selector values from JSONB. - Added the
selector_valuesTEXT array column to thesubject_condition_settable (line 19). - Created a trigger
update_selector_valuesto populateselector_valuesbefore inserts or updates (line 24). - Added an
UPDATEstatement to populate the new column for existing rows (line 29). - Added a GIN index on
subject_condition_set.selector_values(line 32). - Added several other indices on related tables (
subject_mappings,attribute_values,attribute_fqns,subject_mapping_actions,attribute_namespaces,attribute_definitions) (lines 35-57). - Included
goose Downstatements to revert these changes (lines 64-76).
- Created a SQL function
- service/policy/db/models.go
- Added the
SelectorValues []stringfield to theSubjectConditionSetstruct to map the new database column (line 379).
- Added the
- service/policy/db/query.sql
- Modified the
matchSubjectMappingsquery to use theselector_valuescached column with the array overlap operator&&instead of parsing theconditionJSONB column (line 1314).
- Modified the
- service/policy/db/query.sql.go
- Updated generated Go code for the database queries to reflect the change in
matchSubjectMappings.
- Updated generated Go code for the database queries to reflect the change in
- service/policy/subjectmapping/subject_condition_set_test.go
- Added unit tests to validate
CreateSubjectConditionSetRequestusingprotovalidate, covering various invalid scenarios (lines 11-176) and a valid scenario (lines 178-203).
- Added unit tests to validate
- service/policy/subjectmapping/subject_mapping.proto
- Added a
requiredvalidation rule usingbuf.validateto thesubject_condition_setfield inCreateSubjectConditionSetRequest(line 146).
- Added a
- service/policy/subjectmapping/subject_mapping_test.go
- Added unit tests to validate the
NewSubjectConditionSetfield withinCreateSubjectMappingRequestusingprotovalidate, covering various invalid scenarios (lines 36-204).
- Added unit tests to validate the
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.
Data flows like streams,
Selectors cached, faster dreams.
Queries now take flight.
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. ↩
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
service/policy/db/migrations/20250527000000_match_subject_mappings_func.md
Show resolved
Hide resolved
service/policy/db/migrations/20250527000000_match_subject_mappings_func.md
Show resolved
Hide resolved
There was a problem hiding this comment.
Code Review
This pull request introduces a well-thought-out caching mechanism for selectors in subject condition sets, which is a great step towards optimizing the MatchSubjectMappings RPC. The use of a database trigger to maintain this cache is a solid approach. Additionally, the improvements to protovalidate for subject mappings and condition sets, along with the new validation tests, significantly enhance the robustness of these critical components.
Overall, the changes are beneficial, but there's a critical issue in the SQL migration's Down script and a test case that should be enabled due to changes made in this PR.
Summary of Findings
- SQL Migration Error: The
Downscript in the SQL migration20250527000000_match_subject_mappings_func.sqlhas incorrect syntax for dropping theselector_valuescolumn. This is a critical issue that will prevent successful rollback of the migration. - Test Coverage for Validation: A test case in
service/policy/subjectmapping/subject_condition_set_test.gorelated toCreateSubjectConditionSetRequestvalidation is currently commented out. The protovalidate change it depends on seems to be part of this PR, so the test should be enabled. - Performance Improvement: The core change to cache selectors in a dedicated column and update the
matchSubjectMappingsquery to use this cache is a significant performance enhancement. The use of a GIN index on the newselector_valuescolumn is appropriate. - Protovalidate Enhancements: The PR improves protovalidate rules for subject mapping and subject condition set creation, which is a good step for data integrity. The accompanying tests for these validations are also well-written.
- Database Migration for Existing Data: The migration includes a statement
UPDATE subject_condition_set SET condition = condition;to populate the newselector_valuescolumn for existing rows. While effective, its performance characteristics on very large tables should be noted.
Merge Readiness
This pull request introduces valuable performance and validation improvements. However, due to the critical SQL migration error in the Down script, and a pending test case enablement, I recommend that these changes not be merged until these issues are addressed. Once the SQL syntax is corrected and the test case is enabled and passing, this PR will be in much better shape. As a reviewer, I am not authorized to approve pull requests, so please ensure further review and approval from other team members after these changes are made.
service/policy/db/migrations/20250527000000_match_subject_mappings_func.sql
Outdated
Show resolved
Hide resolved
service/policy/db/migrations/20250527000000_match_subject_mappings_func.sql
Show resolved
Hide resolved
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 |
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 |
jakedoublev
changed the title
strantalis
approved these changes
May 30, 2025
This was referenced May 30, 2025
jakedoublev
added a commit
that referenced
this pull request
May 30, 2025
… maintained via trigger (#2320) ### Proposed Changes * Maintain a cache of selectors utilized for the `MatchSubjectMappings` RPC to power fetching resolve-able entitlement objects * Improve protovalidate implementation of subject mappings/subject condition sets creation * Add a new db integration test for `MatchSubjectMappings` that ensures updation to the selectors ### 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-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 3, 2025
🤖 I have created a release *beep* *boop* --- ## [0.5.5](service/v0.5.4...service/v0.5.5) (2025-05-30) ### Features * adds basic config root key manager ([#2303](#2303)) ([dd0d22f](dd0d22f)) * **policy:** cache SubjectConditionSet selectors in dedicated column maintained via trigger ([#2320](#2320)) ([215791f](215791f)) * **policy:** map and merge grants and keys ([#2324](#2324)) ([abf770f](abf770f)) ### Bug Fixes * **deps:** bump github.com/opentdf/platform/sdk from 0.4.5 to 0.4.7 in /service in the internal group ([#2334](#2334)) ([7f5a182](7f5a182)) * **deps:** Updates to major ver of protovalidate ([#2284](#2284)) ([39ad3c9](39ad3c9)) --- 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>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jun 5, 2025
🤖 I have created a release *beep* *boop* --- ## [0.4.0](protocol/go/v0.3.6...protocol/go/v0.4.0) (2025-06-05) ### Features * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **policy:** cache SubjectConditionSet selectors in dedicated column maintained via trigger ([#2320](#2320)) ([215791f](215791f)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * **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)) --- 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>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 9, 2025
🤖 I have created a release *beep* *boop* --- ## [0.7.0](protocol/go/v0.6.2...protocol/go/v0.7.0) (2025-08-08) ### ⚠ BREAKING CHANGES * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) * **core:** Require go 1.23+ ([#1979](#1979)) ### Features * add ability to retrieve policy resources by id or name ([#1901](#1901)) ([deb4455](deb4455)) * **authz:** authz v2, ers v2 protos and gencode for ABAC with actions & registered resource ([#2124](#2124)) ([ea7992a](ea7992a)) * **authz:** improve v2 request proto validation ([#2357](#2357)) ([f927b99](f927b99)) * **authz:** sensible request limit upper bounds ([#2526](#2526)) ([b3093cc](b3093cc)) * **core:** adds bulk rewrap to sdk and service ([#1835](#1835)) ([11698ae](11698ae)) * **core:** EXPERIMENTAL: EC-wrapped key support ([#1902](#1902)) ([652266f](652266f)) * **core:** Require go 1.23+ ([#1979](#1979)) ([164c922](164c922)) * **core:** v2 ERS with proto updates ([#2210](#2210)) ([a161ef8](a161ef8)) * **policy:** add enhanced standard/custom actions protos ([#2020](#2020)) ([bbac53f](bbac53f)) * **policy:** Add legacy keys. ([#2613](#2613)) ([57370b0](57370b0)) * **policy:** Add list key mappings rpc. ([#2533](#2533)) ([fbc2724](fbc2724)) * **policy:** add obligation protos ([#2579](#2579)) ([50882e1](50882e1)) * **policy:** Add validation to delete keys ([#2576](#2576)) ([cc169d9](cc169d9)) * **policy:** add values to CreateObligationRequest ([#2614](#2614)) ([94535cc](94535cc)) * **policy:** adds new public keys table ([#1836](#1836)) ([cad5048](cad5048)) * **policy:** Allow the deletion of a key. ([#2575](#2575)) ([82b96f0](82b96f0)) * **policy:** cache SubjectConditionSet selectors in dedicated column maintained via trigger ([#2320](#2320)) ([215791f](215791f)) * **policy:** Change return type for delete key proto. ([#2566](#2566)) ([c1ae924](c1ae924)) * **policy:** Default Platform Keys ([#2254](#2254)) ([d7447fe](d7447fe)) * **policy:** disable kas grants in favor of key mappings ([#2220](#2220)) ([30f8cf5](30f8cf5)) * **policy:** DSPX-1018 NDR retrieval by FQN support ([#2131](#2131)) ([0001041](0001041)) * **policy:** DSPX-1057 registered resource action attribute values (protos only) ([#2217](#2217)) ([6375596](6375596)) * **policy:** DSPX-893 NDR define crud protos ([#2056](#2056)) ([55a5c27](55a5c27)) * **policy:** DSPX-902 NDR service crud protos only (1/2) ([#2092](#2092)) ([24b6cb5](24b6cb5)) * **policy:** Finish resource mapping groups ([#2224](#2224)) ([5ff754e](5ff754e)) * **policy:** key management crud ([#2110](#2110)) ([4c3d53d](4c3d53d)) * **policy:** Key management proto ([#2115](#2115)) ([561f853](561f853)) * **policy:** Modify get request to search for keys by kasid with keyid. ([#2147](#2147)) ([780d2e4](780d2e4)) * **policy:** Return KAS Key structure ([#2172](#2172)) ([7f97b99](7f97b99)) * **policy:** Return Simple Kas Keys from non-Key RPCs ([#2387](#2387)) ([5113e0e](5113e0e)) * **policy:** rotate keys rpc ([#2180](#2180)) ([0d00743](0d00743)) * **policy:** Update key status's and UpdateKey rpc. ([#2315](#2315)) ([7908db9](7908db9)) * **policy:** Update simple kas key ([#2378](#2378)) ([09d8239](09d8239)) ### Bug Fixes * add pagination to list public key mappings response ([#1889](#1889)) ([9898fbd](9898fbd)) * **core:** Allow 521 curve to be used ([#2485](#2485)) ([aaf43dc](aaf43dc)) * **core:** Fixes protoJSON parse bug on ec rewrap ([#1943](#1943)) ([9bebfd0](9bebfd0)) * **core:** Update fixtures and flattening in sdk and service ([#1827](#1827)) ([d6d6a7a](d6d6a7a)) * **deps:** bump toolchain in /lib/fixtures and /examples to resolve CVE GO-2025-3563 ([#2061](#2061)) ([9c16843](9c16843)) * **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 new public keys rpc's ([#1962](#1962)) ([5049bab](5049bab)) * **policy:** remove predefined rules in actions protos ([#2069](#2069)) ([060f059](060f059)) * **policy:** return kas uri on keys for definition, namespace and values ([#2186](#2186)) ([6c55fb8](6c55fb8)) * **sdk:** Fix compatibility between bulk and non-bulk rewrap ([#1914](#1914)) ([74abbb6](74abbb6)) * update key_mode to provide more context ([#2226](#2226)) ([44d0805](44d0805)) --- 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: Krish Suchak <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
MatchSubjectMappingsRPC to power fetching resolve-able entitlement objectsMatchSubjectMappingsthat ensures updation to the selectorsChecklist
Testing Instructions