feat(policy): add sort support to ListAttributes API #3223
Open
feat(policy): add sort support to ListAttributes API #3223
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds sorting support to the ListAttributes API by introducing new protobuf message types ( 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 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 |
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 introduces robust sorting capabilities to the ListAttributes API. By defining new Protobuf enums and messages, it enables clients to request sorted results in a type-safe manner. The backend implementation leverages SQL CASE statements to handle dynamic ordering, maintaining backward compatibility by defaulting to the existing created_at DESC behavior when no sort is specified. 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 flow in rhyme. With name or date, the order shifts, As code improves with these new gifts. Footnotes
|
github-actions
bot
added
comp:db
Contributor
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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces sorting capabilities for listing attributes. It adds new protobuf definitions for sort fields and directions, updates the database layer to handle dynamic sorting via SQL CASE statements, and includes comprehensive integration and unit tests. The review feedback identifies a potential issue where the updated listAttributesSummary query might return zero results if callers do not provide a non-zero limit, and suggests a minor refactor to simplify the switch logic in the sorting utility function.
Contributor
dsm20
force-pushed
the
feat/DSPX-2682-add-sort-listattributes
branch
from
21a1139 to
60d31d8
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dsm20
added a commit
that referenced
this pull request
Mar 31, 2026
in response to: #3223 (comment) SQL (service/policy/db/queries/attributes.sql): - Added @no_limit::boolean param to listAttributesSummary query - LIMIT @limit_ → LIMIT CASE WHEN @no_limit::boolean THEN NULL ELSE @limit_ END Go (service/policy/db/attributes.go): - GetAttributesByNamespace now passes NoLimit: true so deactivate/reactivate namespace operations fetch all attributes instead of LIMIT 0 returning zero rows
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
dsm20
added a commit
that referenced
this pull request
Mar 31, 2026
in response to: #3223 (comment) SQL (service/policy/db/queries/attributes.sql): - Added @no_limit::boolean param to listAttributesSummary query - LIMIT @limit_ → LIMIT CASE WHEN @no_limit::boolean THEN NULL ELSE @limit_ END Go (service/policy/db/attributes.go): - GetAttributesByNamespace now passes NoLimit: true so deactivate/reactivate namespace operations fetch all attributes instead of LIMIT 0 returning zero rows
dsm20
force-pushed
the
feat/DSPX-2682-add-sort-listattributes
branch
from
97d844e to
1317939
Compare
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
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
Author
|
Regenerated protos and docs (was missing proto-generate from make in last commit)
added SortAttributesType enum, AttributesSort message, and contraint to ListAttributesRequest. Also regenerated protos.
listAttributesSummary and listAttributesDetail get case-when queries instead of the hardcoded ORDER BY. also regen sqlc
Added helper function in utils.go to map enum fields to SQL fields. Request now calls the helper to aquire sortField and sortDirection before executing query.
same tests as listnamespaces. also ran formatting, linter, and compile test.
in response to: #3223 (comment) SQL (service/policy/db/queries/attributes.sql): - Added @no_limit::boolean param to listAttributesSummary query - LIMIT @limit_ → LIMIT CASE WHEN @no_limit::boolean THEN NULL ELSE @limit_ END Go (service/policy/db/attributes.go): - GetAttributesByNamespace now passes NoLimit: true so deactivate/reactivate namespace operations fetch all attributes instead of LIMIT 0 returning zero rows
cast result to bigint instead of text, test wasn't liking my previous changes. also regen sqlc
GetAttributesByNamespace (used by DeactivateNamespace and UnsafeReactivateNamespace) was calling listAttributesSummary which now has LIMIT/OFFSET for pagination. With Go zero values, LIMIT 0 returned zero rows, silently breaking namespace deactivation/reactivation checks. Added a @no_limit boolean parameter — when true, LIMIT evaluates to NULL (no limit). Cast @limit_ to ::bigint to avoid pgx encoding errors.
added buf/validate/validate.proto import to selectors.proto (tests failing), regen'd docs, added ad.id ASC tiebreaker in listAttributesDetail and listAttributesSummary to fix non-deterministic behavior as noted by the rabbit. Also fixed legacy change that snuck in during a previous version issue (policy.SortField)
accidental commit
pagerequest is supposed to handle limits/offset, not sorting. this was a design decision to migrate to the enum approach
(after last change)
Test_GetNamespacesSortParams had a dupe test for nil element. thanks coderabbit
These snuck in as artifacts from the previous version of the repo pre-sorting changes. Issues with version control on my end. Changes represent current iteration aligning with main
and its mention in attributes.sql, revert back to original LIMIT @limit_
reuseable and more efficient, also a bit of futureproofing
Created three helpers to help sharpen the flow (create namespace, create attributes, list with sort, assert)
dsm20
force-pushed
the
feat/DSPX-2682-add-sort-listattributes
branch
from
dc68efe to
bdfcec3
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
.github/workflows/checks.yaml
Outdated
| - name: govulncheck | ||
| id: govulncheck | ||
| continue-on-error: true | ||
| continue-on-error: false |
Contributor
There was a problem hiding this comment.
🚫 [actionlint] reported by reviewdog 🐶
key "continue-on-error" is duplicated in element of "steps" section. previously defined at line:74,col:9 [syntax-check]
dsm20
force-pushed
the
feat/DSPX-2682-add-sort-listattributes
branch
from
bdfcec3 to
18c1c46
Compare
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
covers no sort, 1 item (valid), and 2 items (exceeds max_items = 1)
Contributor
Author
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
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
3 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-2682, depends on #3187
Proposed Changes
ListAttributesRPC, following thepattern established in feat(policy): Add sort support to ListNamespaces API #3192 (ListNamespaces)
name,created_at,updated_at(ASC/DESC), withbackward-compatible fallback to
created_at DESClistAttributesDetailandlistAttributesSummarySQL queriesChanges
SortAttributesTypeenum,AttributesSortmessage,sortfield on
ListAttributesRequest(max 1, buf.validate)attributes.sqlfor both detail andsummary queries
GetAttributesSortParams()helper inutils.go, wired intoListAttributes()inattributes.goeach sort field + direction + fallback
Checklist
Summary by CodeRabbit
Release Notes
New Features
Documentation