feat(policy): add sort support to ListKeyAccessServer#3287
Merged
Conversation
github-actions
bot
added
comp:db
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds single-field sorting to ListKeyAccessServers across API surface: docs, OpenAPI, proto, SQL ORDER BY, Go DB plumbing, mapping helpers, unit and integration tests; preserves default ordering when unspecified (created_at DESC, id ASC tie-breaker). Changes
Sequence DiagramsequenceDiagram
participant Client
participant APIHandler as API Handler
participant SortMapper as Sort Mapper
participant DBQuery as DB Query
participant Database
Client->>APIHandler: ListKeyAccessServers(sort, offset, limit)
APIHandler->>SortMapper: GetKeyAccessServersSortParams(sort)
SortMapper->>SortMapper: map enum field -> SQL column
SortMapper->>SortMapper: map enum direction -> "ASC"/"DESC"
SortMapper-->>APIHandler: (sortField, sortDirection)
APIHandler->>DBQuery: listKeyAccessServers(sortField, sortDirection, offset, limit)
DBQuery->>Database: SELECT ... ORDER BY CASE WHEN `@sort_field`=... THEN ... END, kas.created_at DESC, kas.id ASC LIMIT/OFFSET
Database-->>DBQuery: rows
DBQuery-->>APIHandler: results
APIHandler-->>Client: ListKeyAccessServersResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
dsm20
changed the title
Contributor
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 enhances the 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. Data flows, a stream untold, But order brings a story bold. By name or date, a choice is made, In sorted lists, true sense is laid. Footnotes
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
![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/kasregistry/key_access_server_registry.openapi.yaml`:
- Around line 576-582: Update the policy.SortDirection enum documentation to
explicitly state that the value SORT_DIRECTION_UNSPECIFIED should be interpreted
as ascending order (equivalent to SORT_DIRECTION_ASC); modify the enum
description for policy.SortDirection to note that UNSPECIFIED defaults to ASC
for endpoint-neutral sorting guidance while leaving any concrete List* request
field comments to define endpoint-specific default ordering.
In `@service/integration/kas_registry_test.go`:
- Around line 880-892: The test uses assertIDsInDescendingOrder to verify ASC
ordering which is confusing; rename the helper function
assertIDsInDescendingOrder to a neutral name like assertIDsInOrder (or similar)
and update all calls (e.g., in Test_ListKeyAccessServers_SortByCreatedAt_ASC) to
use the new name, keeping the same signature and behavior in the function
definition so tests still pass; alternatively, if you prefer not to rename, add
a one-line comment above assertIDsInDescendingOrder clarifying that it verifies
that the provided IDs appear in the given order (not necessarily descending) and
update callers to include a brief comment where used in ASC tests (e.g.,
Test_ListKeyAccessServers_SortByCreatedAt_ASC).
🪄 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: 2f2bc052-f008-41a1-b141-5365e6faaa5d
⛔ Files ignored due to path filters (1)
protocol/go/policy/kasregistry/key_access_server_registry.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
docs/grpc/index.htmldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamlservice/integration/kas_registry_test.goservice/policy/db/key_access_server_registry.goservice/policy/db/key_access_server_registry.sql.goservice/policy/db/queries/key_access_server_registry.sqlservice/policy/db/utils.goservice/policy/db/utils_test.goservice/policy/kasregistry/key_access_server_registry.protoservice/policy/kasregistry/key_access_server_registry_test.go
Contributor
protovalidate tests for no sort, 1 sort, and 2 item sort (invalid).
replace order by with case when for all sortable fields/directions, then regen sqlc
not going with constants yet for the new sort field (whereas updated and created have constants), may need to re-examine in future
sortField and sortDirection
created two helpers: createSortTestKeyAccessServers (makes 3 KAS with 5ms gap for time testing), createNamedSortTestKeyAccessServers (unique names for name sorting). Then added 9 tests (created_at, updated_at, name, uri, unspecified).
replaces name sort field
dsm20
force-pushed
the
feat/DSPX-2689-listkeyaccessserver-sort
branch
from
9a0f669 to
231ef54
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
assertIDsinOrder() instead of older one
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/policy/db/queries/key_access_server_registry.sql`:
- Around line 104-105: The DESC sort for name currently allows NULL names to
sort first; change the ORDER BY expression for the DESC branch so NULLs come
last by appending "NULLS LAST" to the CASE expression (i.e. update the CASE WHEN
`@sort_field`::text = 'name' AND `@sort_direction`::text = 'DESC' THEN kas.name END
DESC to CASE WHEN `@sort_field`::text = 'name' AND `@sort_direction`::text = 'DESC'
THEN kas.name END DESC NULLS LAST) so unnamed (NULL) KAS rows are excluded from
the front of name DESC results.
🪄 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: d0232261-557e-45d2-980e-24b3d1d0b3b2
⛔ Files ignored due to path filters (1)
protocol/go/policy/kasregistry/key_access_server_registry.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
docs/grpc/index.htmldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamlservice/integration/kas_registry_test.goservice/policy/db/key_access_server_registry.goservice/policy/db/key_access_server_registry.sql.goservice/policy/db/queries/key_access_server_registry.sqlservice/policy/db/utils.goservice/policy/db/utils_test.goservice/policy/kasregistry/key_access_server_registry.protoservice/policy/kasregistry/key_access_server_registry_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/policy/db/queries/key_access_server_registry.sql`:
- Around line 104-105: The CASE clauses using backticked parameters should be
changed to use the plain parameter identifiers so they match the other clauses:
replace `@sort_field` and `@sort_direction` (the backticked forms) with
`@sort_field` and `@sort_direction` in the CASE statements that produce "kas.name
END ASC NULLS LAST" and "kas.name END DESC NULLS LAST" so the parameter syntax
is consistent with the other CASE branches and sqlc codegen expectations.
🪄 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: ffe41e1b-836b-47b1-8ef9-f1e5e3fc1243
📒 Files selected for processing (1)
service/policy/db/queries/key_access_server_registry.sql
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dsm20
force-pushed
the
feat/DSPX-2689-listkeyaccessserver-sort
branch
from
4248fe5 to
fafd04d
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
c-r33d
reviewed
Apr 13, 2026
c-r33d
reviewed
Apr 13, 2026
following pattern + feedback
c-r33d
previously approved these changes
Apr 13, 2026
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
c-r33d
approved these changes
Apr 14, 2026
This was referenced Apr 14, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Resolves DSPX-2689
Proposed Changes
pattern established in feat(policy): add sort support to ListAttributes API #3223 (ListAttributes), feat(policy): add sort ListSubjectMappings API #3255 (ListSubjectMappings), and feat(policy): add sort support to ListSubjectConditionSets API #3272 (ListSubjectConditionSets)
backward-compatible fallback to created_at DESC
Changes
Proto — service/policy/kasregistry/key_access_server_registry.proto
max_items = 1 constraint
SQL — service/policy/db/queries/key_access_server_registry.sql
Go — service/policy/db/utils.go + service/policy/db/key_access_server_registry.go
avoid goconst errors, same pattern as sortFieldCreatedAt/sortFieldUpdatedAt from feat(policy): add sort ListSubjectMappings API #3255)
Tests
createSortTestKeyAccessServers and createNamedSortTestKeyAccessServers suite helpers
Notes
Checklist
Testing Instructions
Summary by CodeRabbit
New Features
Documentation
Tests