fix(policy): move action sub queries to CTE in sm list and match sql

[strantalis]

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

[gemini-code-assist]

Hello @strantalis, I'm Gemini Code Assist¹! 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. This PR focuses on optimizing the database queries used for listing and matching subject mappings within the policy service. The primary change involves refactoring the SQL queries to replace subqueries fetching associated actions with a Common Table Expression (CTE). This approach is generally favored for potentially improving query performance and readability, especially when the same subquery logic is repeated or when the results of the subquery are needed multiple times within the main query. The Go code interacting with these queries has also been updated to correctly handle the data structure returned by the refactored queries. Additionally, there are minor changes to a test file and a comment update in a model struct.

Highlights

• SQL Query Optimization: The listSubjectMappings and matchSubjectMappings SQL queries in query.sql have been refactored to use a subject_actions CTE. This CTE pre-calculates the standard and custom actions associated with each subject mapping, allowing the main query to join with it instead of executing subqueries for each row.
• Go Code Updates: The Go functions ListSubjectMappings and GetMatchedSubjectMappings in subject_mappings.go have been updated to correctly process the action data returned by the refactored SQL queries. This includes explicitly marshaling the action data (which comes back as interface{}) into JSON bytes before unmarshalling it into the expected protobuf structure.
• Test File Refactoring: The integration test file subject_mappings_test.go had its test suite setup function TestSubjectMappingSuite moved from the end of the file to the beginning for better organization.
• Model Comment Update: A clarifying comment was added to the SelectorValues field in the SubjectConditionSet struct in models.go to explain its purpose and how it's maintained.

Changelog

• service/integration/subject_mappings_test.go
  • Moved the TestSubjectMappingSuite function definition from the end of the file to the beginning (added lines 61-66, removed lines 1774-1779).
• service/policy/db/models.go
  • Added a comment to the SelectorValues field in the SubjectConditionSet struct to clarify its purpose (updated line 379).
• service/policy/db/query.sql
  • Refactored the listSubjectMappings query to introduce a subject_actions CTE for fetching actions and updated the main SELECT and GROUP BY clauses to use the CTE (added lines 1215-1228, removed lines 1221-1232, updated lines 1235-1236, updated lines 1252-1264).
  • Refactored the matchSubjectMappings query to introduce the subject_actions CTE for fetching actions and updated the main SELECT and GROUP BY clauses to use the CTE (added lines 1297-1310, removed lines 1286-1297, updated lines 1314-1315, updated lines 1339-1343).
• service/policy/db/subject_mappings.go
  • Updated ListSubjectMappings to explicitly marshal action data from interface{} to JSON bytes before unmarshalling (added lines 407-417, updated line 408).
  • Updated GetMatchedSubjectMappings to explicitly marshal action data from interface{} to JSON bytes before unmarshalling (added lines 571-581, updated line 560).

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.

---

SQL queries run,
Subqueries were the way,
Now CTEs shine.

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

[gemini-code-assist]

Code Review

This pull request refactors the SQL queries for listSubjectMappings and matchSubjectMappings to use Common Table Expressions (CTEs) for fetching associated actions. This is a good improvement for query readability and potentially for performance. The Go code in service/policy/db/subject_mappings.go has been correctly adapted to handle the changes in data types returned by sqlc for these modified queries.

Overall, the changes are positive. The main point to address is confirming the test coverage for these modifications.

Summary of Findings

• SQL Query Refactoring: The use of Common Table Expressions (CTEs) in listSubjectMappings and matchSubjectMappings to aggregate actions is a good improvement. It enhances query readability and can sometimes offer performance benefits over correlated subqueries. This was not commented on directly due to review settings.
• SQL GROUP BY Clauses: The GROUP BY clauses in the modified SQL queries have been updated to explicitly include all non-aggregated columns from the SELECT list that are involved in constructing JSON objects. This is good practice for clarity and correctness. This was not commented on directly due to review settings.
• SQL Style: The change from true to TRUE in SQL WHERE clauses is a minor stylistic improvement, aligning with standard SQL keyword casing. This was not commented on directly due to review settings.
• Model Documentation: The comment for SelectorValues in service/policy/db/models.go has been updated to be more descriptive, which is a good documentation improvement. This was not commented on directly due to review settings.
• Test File Organization: The TestSubjectMappingSuite function in service/integration/subject_mappings_test.go was moved to an earlier position in the file. This is a minor organizational change. This was not commented on directly due to review settings.

Merge Readiness

The pull request introduces beneficial refactoring to SQL queries and necessary adaptations in the Go code. However, before merging, it's important to address the test coverage. The checklist items for tests in the PR description are unchecked, and confirmation is needed regarding whether existing tests are sufficient or if updates/new tests are required for these changes. I recommend addressing the testing aspect before this PR is merged. As an AI, I am not authorized to approve pull requests.