feat(policy): add sort support to listobligations api#3300
feat(policy): add sort support to listobligations api#3300
Conversation
has buf.validate as per usual and comments for docs. regen protos as well
api-level tests for no items, 1 item, and 2 items sort (pass pass fail)
fqn and name in utils
in listObligations query, remove ORDER BY replace with CASE WHEN block. FQN is assembled here. also regenerated sqlc
this is getting saved for a later PR since its inconsistent across listAPIs
12 unit tests, full coverage) in utils_test.go
wire sort params into the go DB function in /db/obligations.go
full coverage (created_at, updated_at, name, fqn. two helpers for time based and name based sorting. uses assertIDsInOrder helper across each integration test
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 ListObligations API, enabling clients to request ordered data based on specific fields. The changes involve updating the protocol definitions, database query logic, and service layer to handle sort parameters, while maintaining backward compatibility with existing default ordering. 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 obligations list is long, To sort them right, not wrong. With fields defined and SQL set, The best results you'll surely get. Footnotes
|
github-actions
bot
added
comp:db
📝 WalkthroughWalkthroughThis PR adds sorting functionality to the ListObligations API by introducing new proto message types, OpenAPI schemas, and SQL query logic. Changes span proto definitions, database layer modifications, documentation updates, and comprehensive test coverage for sort behavior across multiple fields and directions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
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)
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
dsm20
changed the title
![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 the ListObligations endpoint, allowing results to be ordered by name, FQN, creation date, or update date. The implementation includes updates to the protobuf definitions, service logic to parse sort parameters, and SQL queries utilizing CASE statements for dynamic ordering. Comprehensive integration and unit tests have been added to verify the new functionality. Feedback highlights that the SQL implementation lacks the 'id ASC' tie-breaker mentioned in the documentation, which is necessary to ensure deterministic pagination when multiple records share the same timestamp.
| CASE WHEN @sort_field::text = 'created_at' AND @sort_direction::text = 'DESC' THEN od.created_at END DESC, | ||
| CASE WHEN @sort_field::text = 'updated_at' AND @sort_direction::text = 'ASC' THEN od.updated_at END ASC, | ||
| CASE WHEN @sort_field::text = 'updated_at' AND @sort_direction::text = 'DESC' THEN od.updated_at END DESC, | ||
| od.created_at DESC |
There was a problem hiding this comment.
The ORDER BY clause is missing the id ASC tie-breaker. The documentation in the proto file (and the generated gRPC docs) explicitly states that the default ordering includes id ASC as a tie-breaker. Without a unique tie-breaker, pagination results can be non-deterministic if multiple records share the same created_at timestamp. Adding od.id ASC here will ensure stable sorting for both the default case and as a final tie-breaker for other sort fields.
od.created_at DESC,
od.id ASC
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/grpc/index.html`:
- Line 13206: Restore a non-empty description for the GetObligationRequest entry
in the generated docs by replacing the empty paragraph at the
GetObligationRequest section with a short summary (e.g., "Request message to
retrieve obligation details by ID and optional filters") so the documentation
regains context; locate the GetObligationRequest heading in the generated HTML
and update the empty <p>...</p> into a concise one-line description matching the
request purpose.
- Around line 13573-13580: The docs claim a tie-breaker "id ASC (tie-breaker)"
for ListObligationsRequest.sort/ObligationsSort which is not implemented; update
the HTML table cell describing the sort field in docs/grpc/index.html to remove
or neutralize the specific tie-breaker language and instead state the concrete
default ordering that matches the service (e.g., "created_at DESC" and leave
tie-breaking as implementation-defined or deferred), ensuring the text for the
sort field (the <td> describing ObligationsSort) matches actual endpoint
behavior.
In `@docs/openapi/policy/obligations/obligations.openapi.yaml`:
- Around line 1787-1790: The docs for ListObligationsRequest.sort currently
assert a tie-breaker "then id ASC (tie-breaker)" which is not implemented;
update the description for the ListObligationsRequest.sort field by removing the
tie-breaker clause and any wording that implies a stable secondary ordering,
leaving only the documented default ordering that matches current endpoint
behavior (e.g., "created_at DESC" or the existing default phrasing) so the
OpenAPI text no longer guarantees id ASC as a tie-breaker.
- Around line 547-553: Update the policy.SortDirection schema to document how
SORT_DIRECTION_UNSPECIFIED should be treated: add an endpoint-neutral
description stating that SORT_DIRECTION_UNSPECIFIED maps to ascending order
(UNSPECIFIED => ASC) and note that concrete per-endpoint default ordering should
still be documented in each request's docs; reference the enum value
SORT_DIRECTION_UNSPECIFIED and the schema name policy.SortDirection so the
change is applied to the central enum definition rather than individual
endpoints.
In `@service/integration/obligations_test.go`:
- Around line 1990-2000: The helper createNamedSortTestObligations currently
always uses nsExampleCom so it creates all obligations in one namespace and
masks FQN-based sorting bugs; modify createNamedSortTestObligations to create
obligations across multiple namespaces (e.g., alternate between nsExampleCom and
another test namespace obtained via s.getNamespaceData or accept a namespace
list) by fetching distinct namespaceIDs with s.getNamespaceData (or use existing
namespace fixtures) and using those namespaceIDs when calling
s.createObligation; update the related fixtures/tests (including the similar
code around lines 2036-2064) so the generated obligations span at least two
namespaces to properly exercise full FQN sorting.
In `@service/policy/db/queries/obligations.sql`:
- Around line 190-199: The ORDER BY lacks a final unique tie-breaker, causing
nondeterministic pagination; update the ORDER BY in obligations.sql (the CASE
expressions and final default) to always append od.id ASC as the ultimate
deterministic tie-breaker (so the current tail "od.created_at DESC" becomes
"od.created_at DESC, od.id ASC" and ensure id ASC is applied after any
CASE-driven sort to satisfy the contract "created_at DESC, then id 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: a2eecd90-c407-411a-a178-4b5469d1885b
⛔ Files ignored due to path filters (1)
protocol/go/policy/obligations/obligations.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
docs/grpc/index.htmldocs/openapi/policy/obligations/obligations.openapi.yamlservice/integration/obligations_test.goservice/policy/db/obligations.goservice/policy/db/obligations.sql.goservice/policy/db/queries/obligations.sqlservice/policy/db/utils.goservice/policy/db/utils_test.goservice/policy/obligations/obligations.protoservice/policy/obligations/obligations_test.go
|
|
||
| <h3 id="policy.obligations.GetObligationRequest">GetObligationRequest</h3> | ||
| <p>Definitions</p> | ||
| <p></p> |
There was a problem hiding this comment.
Restore a non-empty description for GetObligationRequest.
Line 13206 is now empty, which drops useful context from the docs. Please keep at least a short request summary.
Suggested doc fix
- <p></p>
+ <p>Retrieve an obligation by ID or FQN.</p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p></p> | |
| <p>Retrieve an obligation by ID or FQN.</p> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/grpc/index.html` at line 13206, Restore a non-empty description for the
GetObligationRequest entry in the generated docs by replacing the empty
paragraph at the GetObligationRequest section with a short summary (e.g.,
"Request message to retrieve obligation details by ID and optional filters") so
the documentation regains context; locate the GetObligationRequest heading in
the generated HTML and update the empty <p>...</p> into a concise one-line
description matching the request purpose.
| <tr> | ||
| <td>sort</td> | ||
| <td><a href="#policy.obligations.ObligationsSort">ObligationsSort</a></td> | ||
| <td>repeated</td> | ||
| <td><p>Optional - CONSTRAINT: max 1 item | ||
| Default ordering when omitted or when the first entry's field is UNSPECIFIED: | ||
| created_at DESC, then id ASC (tie-breaker). </p></td> | ||
| </tr> |
There was a problem hiding this comment.
ListObligationsRequest.sort documents an unimplemented tie-breaker.
Line 13579 states id ASC (tie-breaker), but this PR explicitly defers tie-breaker behavior. This creates a docs/API behavior mismatch.
Suggested doc fix
- <td><p>Optional - CONSTRAINT: max 1 item
-Default ordering when omitted or when the first entry's field is UNSPECIFIED:
-created_at DESC, then id ASC (tie-breaker). </p></td>
+ <td><p>Optional - CONSTRAINT: max 1 item
+Default ordering when omitted or when the first entry's field is UNSPECIFIED:
+created_at DESC. </p></td>Based on learnings: document concrete defaults on the specific List* request field, but keep them aligned with actual endpoint behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/grpc/index.html` around lines 13573 - 13580, The docs claim a
tie-breaker "id ASC (tie-breaker)" for
ListObligationsRequest.sort/ObligationsSort which is not implemented; update the
HTML table cell describing the sort field in docs/grpc/index.html to remove or
neutralize the specific tie-breaker language and instead state the concrete
default ordering that matches the service (e.g., "created_at DESC" and leave
tie-breaking as implementation-defined or deferred), ensuring the text for the
sort field (the <td> describing ObligationsSort) matches actual endpoint
behavior.
| policy.SortDirection: | ||
| type: string | ||
| title: SortDirection | ||
| enum: | ||
| - SORT_DIRECTION_UNSPECIFIED | ||
| - SORT_DIRECTION_ASC | ||
| - SORT_DIRECTION_DESC |
There was a problem hiding this comment.
Document SORT_DIRECTION_UNSPECIFIED behavior on policy.SortDirection.
Line 551 introduces SORT_DIRECTION_UNSPECIFIED, but this schema currently does not define how it is interpreted. Add an endpoint-neutral description here (UNSPECIFIED => ASC) and keep endpoint-specific defaults in request docs.
Suggested doc patch
policy.SortDirection:
type: string
title: SortDirection
+ description: |-
+ Endpoint-neutral sort direction.
+ SORT_DIRECTION_UNSPECIFIED is treated as ascending.
+ For default ordering when `sort` is omitted or `field` is UNSPECIFIED,
+ see the specific List* request schema.
enum:
- SORT_DIRECTION_UNSPECIFIED
- SORT_DIRECTION_ASC
- SORT_DIRECTION_DESCBased on learnings: keep SortDirection endpoint-neutral and document UNSPECIFIED=>ASC, while directing concrete default ordering to per-request docs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| policy.SortDirection: | |
| type: string | |
| title: SortDirection | |
| enum: | |
| - SORT_DIRECTION_UNSPECIFIED | |
| - SORT_DIRECTION_ASC | |
| - SORT_DIRECTION_DESC | |
| policy.SortDirection: | |
| type: string | |
| title: SortDirection | |
| description: |- | |
| Endpoint-neutral sort direction. | |
| SORT_DIRECTION_UNSPECIFIED is treated as ascending. | |
| For default ordering when `sort` is omitted or `field` is UNSPECIFIED, | |
| see the specific List* request schema. | |
| enum: | |
| - SORT_DIRECTION_UNSPECIFIED | |
| - SORT_DIRECTION_ASC | |
| - SORT_DIRECTION_DESC |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/openapi/policy/obligations/obligations.openapi.yaml` around lines 547 -
553, Update the policy.SortDirection schema to document how
SORT_DIRECTION_UNSPECIFIED should be treated: add an endpoint-neutral
description stating that SORT_DIRECTION_UNSPECIFIED maps to ascending order
(UNSPECIFIED => ASC) and note that concrete per-endpoint default ordering should
still be documented in each request's docs; reference the enum value
SORT_DIRECTION_UNSPECIFIED and the schema name policy.SortDirection so the
change is applied to the central enum definition rather than individual
endpoints.
| description: |- | ||
| Optional - CONSTRAINT: max 1 item | ||
| Default ordering when omitted or when the first entry's field is UNSPECIFIED: | ||
| created_at DESC, then id ASC (tie-breaker). |
There was a problem hiding this comment.
Remove the unimplemented tie-breaker guarantee from ListObligationsRequest.sort docs.
Line 1790 documents then id ASC (tie-breaker), but this PR explicitly defers tie-breaker behavior. Keeping this line overstates current guarantees and can mislead clients relying on stable ordering.
Suggested doc patch
sort:
type: array
items:
$ref: '#/components/schemas/policy.obligations.ObligationsSort'
title: sort
maxItems: 1
description: |-
Optional - CONSTRAINT: max 1 item
Default ordering when omitted or when the first entry's field is UNSPECIFIED:
- created_at DESC, then id ASC (tie-breaker).
+ created_at DESC.Based on learnings: concrete defaults should be documented at the request level, and those defaults must match current endpoint behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: |- | |
| Optional - CONSTRAINT: max 1 item | |
| Default ordering when omitted or when the first entry's field is UNSPECIFIED: | |
| created_at DESC, then id ASC (tie-breaker). | |
| description: |- | |
| Optional - CONSTRAINT: max 1 item | |
| Default ordering when omitted or when the first entry's field is UNSPECIFIED: | |
| created_at DESC. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/openapi/policy/obligations/obligations.openapi.yaml` around lines 1787 -
1790, The docs for ListObligationsRequest.sort currently assert a tie-breaker
"then id ASC (tie-breaker)" which is not implemented; update the description for
the ListObligationsRequest.sort field by removing the tie-breaker clause and any
wording that implies a stable secondary ordering, leaving only the documented
default ordering that matches current endpoint behavior (e.g., "created_at DESC"
or the existing default phrasing) so the OpenAPI text no longer guarantees id
ASC as a tie-breaker.
| func (s *ObligationsSuite) createNamedSortTestObligations(prefixes []string) []string { | ||
| namespaceID, _, _ := s.getNamespaceData(nsExampleCom) | ||
| suffix := time.Now().UnixNano() | ||
| ids := make([]string, len(prefixes)) | ||
| for i, prefix := range prefixes { | ||
| name := fmt.Sprintf("%s-%d", prefix, suffix) | ||
| obl := s.createObligation(namespaceID, name, nil) | ||
| ids[i] = obl.GetId() | ||
| } | ||
| return ids | ||
| } |
There was a problem hiding this comment.
Make the FQN sort fixtures span multiple namespaces.
This helper always creates obligations under example.com, so the FQN sort tests below collapse into name-sort tests. A regression that orders by od.name instead of the full fqns.fqn || '/obl/' || lower(od.name) expression would still pass.
As per coding guidelines, "Run go test ./... or make test and ensure all existing tests pass; add tests for new functionality".
Also applies to: 2036-2064
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/integration/obligations_test.go` around lines 1990 - 2000, The helper
createNamedSortTestObligations currently always uses nsExampleCom so it creates
all obligations in one namespace and masks FQN-based sorting bugs; modify
createNamedSortTestObligations to create obligations across multiple namespaces
(e.g., alternate between nsExampleCom and another test namespace obtained via
s.getNamespaceData or accept a namespace list) by fetching distinct namespaceIDs
with s.getNamespaceData (or use existing namespace fixtures) and using those
namespaceIDs when calling s.createObligation; update the related fixtures/tests
(including the similar code around lines 2036-2064) so the generated obligations
span at least two namespaces to properly exercise full FQN sorting.
1 participant
Proposed Changes
Resolves DSPX-2690
Changes
Proto —
service/policy/obligations/obligations.protoSortObligationsTypeenum (UNSPECIFIED, NAME, FQN, CREATED_AT, UPDATED_AT)ObligationsSortmessage (field + direction)repeated ObligationsSort sort = 11onListObligationsRequestwithmax_items = 1constraintSQL —
service/policy/db/queries/obligations.sqllistObligationsquery for 4 fields (8 blocks total)fqns.fqn || '/obl/' || LOWER(od.name)since obligation FQNs aren't stored inattribute_fqnsod.created_at DESCGo —
service/policy/db/utils.go+service/policy/db/obligations.goGetObligationsSortParams(): maps enum to SQL-compatible field/direction stringsListObligationshandler wired to call mapper and pass params to sqlc querysortFieldNameandsortFieldFQNconstants inutils.go(same pattern assortFieldCreatedAt/sortFieldUpdatedAtfrom feat(policy): add sort ListSubjectMappings API #3255)Tests
createSortTestObligationsandcreateNamedSortTestObligationssuite helpersTest_ListObligationsRequest_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, and feat(policy): add sort support to ListSubjectConditionSets API #3272od.id ASC) deferred to a follow-up refactoring ticketChecklist
Testing Instructions
Summary by CodeRabbit
New Features
Tests
Documentation