feat(policy): add sort ListSubjectMappings API#3255
Conversation
📝 WalkthroughWalkthroughAdds single-field sorting to ListSubjectMappings: proto/schemas for sort, mapping of sort to SQL field/direction, query ORDER BY made dynamic, utilities and tests updated, and docs/OpenAPI extended. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Policy API
participant DBClient as PolicyDBClient
participant DB as Database
Client->>API: ListSubjectMappingsRequest(sort)
API->>DBClient: GetSubjectMappingsSortParams(sort)
DBClient->>DB: listSubjectMappings(namespace, sort_field, sort_direction, limit, offset)
DB-->>DBClient: rows (ordered by chosen field/direction)
DBClient-->>API: subject mappings
API-->>Client: ListSubjectMappingsResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
bot
added
comp:db
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 introduces sorting capabilities to the ListSubjectMappings API. It defines the necessary protocol buffer structures, implements the mapping logic in the Go service layer, and updates the underlying SQL queries to handle dynamic sorting parameters. The changes ensure that API consumers can sort results by creation or update timestamps while maintaining backward compatibility. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. Ignored Files
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 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 counter productive. 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 list was static, fixed in time, Now sorting makes it feel sublime. By date or update, order flows, As logic in the database grows. Footnotes
|
![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 introduces sorting capabilities for listing subject mappings, allowing users to sort by creation and update timestamps in both ascending and descending order. The changes include updates to the protobuf definitions, database queries, and utility functions, supported by comprehensive integration and unit tests. Review feedback highlights opportunities to reduce code duplication in the new integration tests by extracting a helper method for subject mapping creation and to simplify a switch statement within the sorting utility function.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
define SortSubjectMappingsType enum and SubjectMappingsSort message for strongly-typed sort on ListSubjectMappings RPC
following the pattern, change the ORDER BY to incorporate CASE WHEN structuring with the sort fields
helper maps proto enum values to strings, handler passes strings to sqlc
implement unit tests for all sort functions including nil, empty slice, nill element ([nill])
added 5 integration tests: CreatedAt ASC/DESC, UpdatedAt ASC/DESC, and then FallsBackToDefault (CreatedAt DESC)
ran buf generate originally, but needed to do 'make proto-generate' to capture grpc and openai docs
CREATED_AT and UPDATED_AT are used in this case for the protovalidate test which covers 3 cases: no sort (valid), one item sort (also valid), and two sort items (invalid, since max_items is 1 in the proto).
added helper (createSortTestSubjectMappings()), then refactored existing 5 sort cases to use the helper. added neccesary fmt import. improved existing test (UpdatedAt_ASC). this list API only requires 1 helper since it only uses time based tests.
created_at and updated_at sort fields are now constants defined at the beginning of utils.go. This is because goconst will fail on the case that these strings are being used more than 3 times without their own variable.
dsm20
force-pushed
the
feat/DSPX-2685-add-sort-listsubjectmappings
branch
from
234f300 to
7970007
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml`:
- Around line 1580-1586: Update the description for the "sort" property in the
policy.subjectmapping.SubjectMappings schema to explicitly state the server's
default/fallback ordering when the field is omitted or empty (e.g., "When not
provided, results are ordered by <primaryField> ascending, then by
<secondaryField> descending" or whatever the server actually implements); keep
the existing maxItems note and add a sentence describing deterministic behavior
so clients can rely on the default order, referencing the sort array and the
SubjectMappingsSort item type.
In `@service/policy/db/utils.go`:
- Around line 317-321: The helper currently maps any non-DESC value to "ASC",
causing invalid/future policy.SortDirection values to be treated as ascending;
change the logic so only an explicit SORT_DIRECTION_ASC returns "ASC" and
SORT_DIRECTION_DESC returns "DESC", otherwise return an empty/invalid direction
(e.g., "" or nil-equivalent) so callers can detect invalid directions and fall
back to the default ordering (created_at DESC); update the branch that checks
s.GetDirection() (and the local variable direction) to only set "ASC" when
s.GetDirection() == policy.SortDirection_SORT_DIRECTION_ASC and "DESC" when ==
policy.SortDirection_SORT_DIRECTION_DESC, leaving direction empty for all other
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21c7e93f-ce5e-435f-8469-3dbec3932a87
⛔ Files ignored due to path filters (1)
protocol/go/policy/subjectmapping/subject_mapping.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
docs/grpc/index.htmldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamlservice/integration/subject_mappings_test.goservice/policy/db/queries/subject_mappings.sqlservice/policy/db/subject_mappings.goservice/policy/db/subject_mappings.sql.goservice/policy/db/utils.goservice/policy/db/utils_test.goservice/policy/subjectmapping/subject_mapping.protoservice/policy/subjectmapping/subject_mapping_test.go
was not using getSortDirection here, incorrect.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
2 participants
Resolves DSPX-2685
Proposed Changes
ListSubjectMappingsRPC, following thepattern established in feat(policy): add sort support to ListAttributes API #3223 (ListAttributes) and feat(policy): Add sort support to ListNamespaces API #3192 (ListNamespaces)
created_at,updated_at(ASC/DESC), withbackward-compatible fallback to
created_at DESCChanges
Proto —
service/policy/subjectmapping/subject_mapping.protoSortSubjectMappingsTypeenum (UNSPECIFIED,CREATED_AT,UPDATED_AT)SubjectMappingsSortmessage (field + direction)repeated SubjectMappingsSort sort = 11onListSubjectMappingsRequestwithmax_items = 1constraintSQL —
service/policy/db/queries/subject_mappings.sqlcreated_atandupdated_at(ASC/DESC each)sm.created_at DESC+ tiebreakersm.id ASCGo —
service/policy/db/utils.go+service/policy/db/subject_mappings.goGetSubjectMappingsSortParams(): maps enum to SQL-compatible field/direction stringsListSubjectMappingshandler wired to call mapper and pass params to sqlc querysortFieldCreatedAt/sortFieldUpdatedAtconstants inutils.goto resolvegoconstlint across all sort helpers (slightly out of scope but necessary to avoid goconst errors)Tests
createSortTestSubjectMappingssuite helperTest_ListSubjectMappingsRequest_Sort)Notes
namesort is listed in the ticket but thesubject_mappingstable has nonamecolumnotdfctl --sortflag deferred to a follow-up, consistent with feat(policy): Add sort support to ListNamespaces API #3192 and feat(policy): add sort support to ListAttributes API #3223Checklist
Summary by CodeRabbit
New Features
Documentation
Tests