feat(policy): add sort support to listkaskeys#3344
Conversation
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 ListKeys RPC, enabling more flexible data retrieval for key access server registries. The changes include updates to the protocol definitions, database query logic, and application-level mapping, ensuring that sorting is both type-safe and performant. The implementation maintains backward compatibility by defaulting to the existing ordering behavior when no sort parameters are provided. 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 keys were scattered, hard to find, With no clear order in the mind. Now sorted by ID or by date, They line up straight, they look just great. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces sorting capability to the KAS registry's ListKeys API by adding a new sort parameter type with field and direction specification in proto, mapping sort parameters to SQL through a helper function, and updating the database query to use dynamic ORDER BY clauses based on the provided sort criteria. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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 ListKeys endpoint in the KAS registry. It adds a new sort field to the ListKeysRequest protobuf message, allowing users to sort by key ID, creation time, or update time in either ascending or descending order. The implementation includes updates to the database layer using dynamic SQL sorting, utility functions for parameter mapping, and comprehensive integration and unit tests. Feedback was provided regarding the SQL ORDER BY clause, specifically suggesting an optimization to avoid redundant sorting criteria when a specific order is explicitly requested.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
also regen SQL and docs
maps sort fields. also included unit test func
same as other listAPIs, tests no sort, 1 item sort, and 2 items (invalid)
two helpers: creation of timestamped keys and named keys to test time-based and name-based sorting respectively. covers key ID, created at, updated at (all ASC/DESC), and default fallback
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/kasregistry/key_access_server_registry.proto`:
- Around line 80-83: KasKeysSort's enum fields (KasKeysSort.field and
KasKeysSort.direction) lack defined_only validation, allowing numeric
out-of-range values to pass; update the proto message KasKeysSort to annotate
both SortKasKeysType field and policy.SortDirection direction with
[(validate.rules).enum.defined_only = true]; also extend the sort validation
unit test that targets KasKeysSort to include cases with invalid numeric enum
values to assert the validator rejects them.
🪄 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: 16631ee5-459e-4796-b439-8606ab18791a
⛔ Files ignored due to path filters (3)
protocol/go/authorization/authorization.pb.gois excluded by!**/*.pb.goprotocol/go/policy/kasregistry/key_access_server_registry.pb.gois excluded by!**/*.pb.goprotocol/go/policy/objects.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamlservice/integration/kas_registry_key_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
💤 Files with no reviewable changes (3)
- docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml
- docs/openapi/authorization/authorization.openapi.yaml
- docs/openapi/policy/objects.openapi.yaml
dsm20
force-pushed
the
add-sort-support-kaskeys-api
branch
from
32e6de3 to
222b85f
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
had buf 1.68.1 generating docs, wasn't matching CI version. regenerated with correct version
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
2 participants
Proposed Changes
Resolves DSPX-2694
Changes
Proto —
service/policy/kasregistry/key_access_server_registry.protoSortKasKeysTypeenum (UNSPECIFIED, KEY_ID, CREATED_AT, UPDATED_AT)KasKeysSortmessage (field + direction)repeated KasKeysSort sort = 11onListKeysRequestwithmax_items = 1constraintSQL —
service/policy/db/queries/key_access_server_registry.sqllistKeysquery for 3 fields (6 blocks total)kask.created_at DESCGo —
service/policy/db/utils.go+service/policy/db/key_access_server_registry.goGetKasKeysSortParams(): maps enum to SQL-compatible field/direction stringsListKeyshandler wired to call mapper and pass params to sqlc querysortFieldKeyIDaddedTests
createSortTestKasKeysandcreateKeyIdSortTestKasKeyssuite helpersTest_ListKeysRequest_Sort)Notes
--sortflag deferred to a follow-up, consistent with all prior sort PRskask.id ASC) deferred to a follow-up refactoring ticketChecklist
Summary by CodeRabbit
New Features
Documentation
Tests