feat(policy): Add sort support listregisteredresources api#3312
Conversation
and proto validation
tests max items constraint against no sort, one item sort, and 2 item sort
order by -> case when for sorting fields + sqlc regen
maps enum to SQL field, as well as the cases for empty or nill sorts. sort unspecified falls back to default as per usual
nill sort, empty slice, nill element, unspecified, then every field combination asc/desc
go query function gets sortField and sortDirection, which are returned by GetRegisteredResourcesSortParams (the helper)
includes funcs for all ASC/DESC tests, default fallback, with 2 helpers for creating name and time based tests respectively + a deletion func for deferred cleanup during tests
📝 WalkthroughWalkthroughThis PR adds sorting functionality to the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 significantly enhances the ListRegisteredResources API by introducing robust sorting capabilities. Users can now specify sorting preferences for registered resources based on their name, creation timestamp, or last update timestamp, in both ascending and descending orders. This change improves the flexibility and usability of data retrieval, aligning with similar sorting features implemented in other list APIs. 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. A list of things, a growing stream, To sort them right, a coding dream. By name or date, ascending, down, Order found, throughout the town. Footnotes
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![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 to the ListRegisteredResources API, allowing results to be ordered by name, creation time, or update time in both ascending and descending directions. The changes include updates to the protobuf definitions, database queries using dynamic CASE statements, and utility functions for parameter mapping, along with new integration and unit tests. Feedback suggests evaluating the performance impact of the dynamic sorting implementation on large datasets and improving the robustness of enum mapping by handling unexpected values more explicitly.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/kasregistry/key_access_server_registry.openapi.yaml`:
- Around line 583-589: The shared enum policy.SortDirection currently contains
endpoint-specific semantics about UNSPECIFIED handling; remove the
endpoint-level behavior text (the notes about UNSPECIFIED being treated as ASC
or defaulting to endpoint request message ordering) from the
policy.SortDirection description and leave it as a neutral enum description
(e.g., "Sorting direction shared across list APIs."). Then, for each concrete
List*Request.sort field (where SORT_DIRECTION_UNSPECIFIED is relevant), add the
appropriate endpoint-specific note about fallback/default ordering and how
SORT_DIRECTION_UNSPECIFIED is interpreted for that request. Ensure references to
the enum value (SORT_DIRECTION_UNSPECIFIED / policy.SortDirection) remain but do
not assert global behavior in the enum schema.
In `@docs/openapi/policy/obligations/obligations.openapi.yaml`:
- Around line 554-560: Remove the endpoint-specific behavior text ("When a sort
field is provided, UNSPECIFIED is treated as ASC" and the request-level fallback
sentence) from the central enum schema policy.SortDirection in
obligations.openapi.yaml; this enum should be neutral and only describe the enum
values. Instead, move/restore any notes about UNSPECIFIED-to-ASC behavior and
default/fallback ordering into the specific List*Request.sort field
documentation for each endpoint that actually implements that semantics (e.g.,
the various ListPoliciesRequest.sort or ListObligationsRequest.sort fields),
ensuring each request's docs explain its default ordering and how UNSPECIFIED is
treated.
In `@docs/openapi/policy/registeredresources/registered_resources.openapi.yaml`:
- Around line 442-455: The shared enum policy.SortDirection currently documents
endpoint-specific behavior ("When a sort field is provided, UNSPECIFIED is
treated as ASC"); remove that clause from the policy.SortDirection description
and keep only generic docs for the enum values, then add the specific
interpretation note to each request's per-endpoint sort field (e.g.,
ListRegisteredResourcesRequest.sort) where the behavior applies; update the
description text for ListRegisteredResourcesRequest.sort to state how
SORT_DIRECTION_UNSPECIFIED is treated for that particular endpoint and ensure no
other endpoint-level semantics remain in the central policy.SortDirection
schema.
In `@service/policy/db/queries/registered_resources.sql`:
- Around line 132-139: The ORDER BY using CASE on `@sort_field/`@sort_direction is
unstable when multiple rows share the same sort key and created_at, causing
pagination duplicates/skips; to fix, append a deterministic tie-breaker such as
r.id ASC to the end of the ORDER BY (after the existing r.created_at DESC) so
queries like the one using r.name/r.created_at/r.updated_at with CASE clauses
always have a unique final sort column; update the ORDER BY clause that
references `@sort_field`, `@sort_direction` and r.created_at to include r.id ASC as
the last ordering term.
In `@service/policy/registeredresources/registered_resources.proto`:
- Around line 122-125: Document that when RegisteredResourcesSort.direction =
SORT_DIRECTION_UNSPECIFIED it should be treated as ASC for this request; update
the comment for the request's sort field (or the
RegisteredResourcesSort.direction field) in registered_resources.proto to
explicitly state the UNSPECIFIED->ASC fallback so the endpoint remains
self-describing (reference symbols: RegisteredResourcesSort,
RegisteredResourcesSort.direction, SORT_DIRECTION_UNSPECIFIED, repeated
RegisteredResourcesSort sort = 11).
🪄 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: 5aa9ebb8-be69-47f9-a4ed-2ec80c616090
⛔ Files ignored due to path filters (1)
protocol/go/policy/registeredresources/registered_resources.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
docs/grpc/index.htmldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamlservice/integration/registered_resources_test.goservice/policy/db/queries/registered_resources.sqlservice/policy/db/registered_resources.goservice/policy/db/registered_resources.sql.goservice/policy/db/utils.goservice/policy/db/utils_test.goservice/policy/registeredresources/registered_resources.protoservice/policy/registeredresources/registered_resources_test.go
2 participants
Proposed Changes
Resolves DSPX-2691
Changes
Proto —
service/policy/registeredresources/registered_resources.protoSortRegisteredResourcesTypeenum (UNSPECIFIED, NAME, CREATED_AT, UPDATED_AT)RegisteredResourcesSortmessage (field + direction)repeated RegisteredResourcesSort sort = 11onListRegisteredResourcesRequestwithmax_items = 1constraintSQL —
service/policy/db/queries/registered_resources.sqllistRegisteredResourcesquery for 3 fields (6 blocks total)r.created_at DESCGo —
service/policy/db/utils.go+service/policy/db/registered_resources.goGetRegisteredResourcesSortParams(): maps enum to SQL-compatible field/direction stringsListRegisteredResourceshandler wired to call mapper and pass params to sqlc querysortFieldName,sortFieldCreatedAt,sortFieldUpdatedAtalready existTests
createSortTestRegisteredResourcesandcreateNamedSortTestRegisteredResourcessuite helpersTestListRegisteredResourcesRequest_Sort)Notes
--sortflag deferred to a follow-up, consistent with feat(policy): Add sort support to ListNamespaces API #3192, feat(policy): add sort support to ListAttributes API #3223, feat(policy): add sort ListSubjectMappings API #3255, feat(policy): add sort support to ListSubjectConditionSets API #3272, and feat(policy): add sort support to listobligations api #3300r.id ASC) deferred to a follow-up refactoring ticketChecklist
Testing Instructions
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests