Conversation
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
🚥 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.
| createMapping := func(email string) string { | ||
| scs := &subjectmapping.SubjectConditionSetCreate{ | ||
| SubjectSets: []*policy.SubjectSet{ | ||
| { | ||
| ConditionGroups: []*policy.ConditionGroup{ | ||
| { | ||
| BooleanOperator: policy.ConditionBooleanTypeEnum_CONDITION_BOOLEAN_TYPE_ENUM_AND, | ||
| Conditions: []*policy.Condition{ | ||
| { | ||
| SubjectExternalSelectorValue: ".email", | ||
| Operator: policy.SubjectMappingOperatorEnum_SUBJECT_MAPPING_OPERATOR_ENUM_IN, | ||
| SubjectExternalValues: []string{email}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| created, err := s.db.PolicyClient.CreateSubjectMapping(s.ctx, &subjectmapping.CreateSubjectMappingRequest{ | ||
| AttributeValueId: fixtureAttrValID, | ||
| NewSubjectConditionSet: scs, | ||
| Actions: []*policy.Action{actionRead}, | ||
| }) | ||
| s.Require().NoError(err) | ||
| s.Require().NotEmpty(created.GetId()) | ||
| return created.GetId() | ||
| } |
There was a problem hiding this comment.
There's significant code duplication across the new test functions (Test_ListSubjectMappings_SortByCreatedAt_ASC, Test_ListSubjectMappings_SortByCreatedAt_DESC, etc.). The createMapping local function and its setup variables are identical in all of them.
To improve maintainability and reduce redundancy, consider extracting this logic into a helper method on the SubjectMappingsSuite struct. This will make the tests cleaner and easier to manage.
For example:
func (s *SubjectMappingsSuite) createMappingForSortTest(email string) string {
fixtureAttrValID := s.f.GetAttributeValueKey("example.net/attr/attr1/value/value2").ID
actionRead := s.f.GetStandardAction(policydb.ActionRead.String())
scs := &subjectmapping.SubjectConditionSetCreate{
SubjectSets: []*policy.SubjectSet{
{
ConditionGroups: []*policy.ConditionGroup{
{
BooleanOperator: policy.ConditionBooleanTypeEnum_CONDITION_BOOLEAN_TYPE_ENUM_AND,
Conditions: []*policy.Condition{
{
SubjectExternalSelectorValue: ".email",
Operator: policy.SubjectMappingOperatorEnum_SUBJECT_MAPPING_OPERATOR_ENUM_IN,
SubjectExternalValues: []string{email},
},
},
},
},
},
},
}
created, err := s.db.PolicyClient.CreateSubjectMapping(s.ctx, &subjectmapping.CreateSubjectMappingRequest{
AttributeValueId: fixtureAttrValID,
NewSubjectConditionSet: scs,
Actions: []*policy.Action{actionRead},
})
s.Require().NoError(err)
s.Require().NotEmpty(created.GetId())
return created.GetId()
}Then, each test can simply call s.createMappingForSortTest(...).
| switch s.GetField() { | ||
| case subjectmapping.SortSubjectMappingsType_SORT_SUBJECT_MAPPINGS_TYPE_CREATED_AT: | ||
| field = "created_at" | ||
| case subjectmapping.SortSubjectMappingsType_SORT_SUBJECT_MAPPINGS_TYPE_UPDATED_AT: | ||
| field = "updated_at" | ||
| case subjectmapping.SortSubjectMappingsType_SORT_SUBJECT_MAPPINGS_TYPE_UNSPECIFIED: | ||
| return "", "" | ||
| default: | ||
| return "", "" | ||
| } |
There was a problem hiding this comment.
The switch statement can be simplified. The case for UNSPECIFIED and the default case have identical logic. You can remove the UNSPECIFIED case entirely and let default handle it, making the code more concise.
switch s.GetField() {
case subjectmapping.SortSubjectMappingsType_SORT_SUBJECT_MAPPINGS_TYPE_CREATED_AT:
field = "created_at"
case subjectmapping.SortSubjectMappingsType_SORT_SUBJECT_MAPPINGS_TYPE_UPDATED_AT:
field = "updated_at"
default:
return "", ""
}There was a problem hiding this comment.
@gemini-code-assist this pattern of including both UNSPECIFIED and default is already established and serves more to document the fact that UNSPECIFIED is being recognized/accounted for, and the default is there just as a safety. Same returns.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
1 participant
Proposed Changes
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 queryTests
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
Testing Instructions
Summary by CodeRabbit
Release Notes