refactor(policy): decouple sort field/direction mapping and add SQL-level sort defaults#3402
Merged
Conversation
removed endpoint-specific behavior comment/doc from enum. this behavior may change between endpoints and shouldn't be established as the de facto standard. kept the note referring the behavior documentation to the specific list request
each field in the __Sort messages now has defined_only which only allows defined enum values to be passed in
all comments for the sort field are now standardized to declare unspecified maps to ASC, and no sort + unspecified maps to the specific default ordering of the endpoint
dsm20
marked this pull request as draft
April 27, 2026 15:42
|
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:
📝 WalkthroughWalkthroughStandardizes list-sorting semantics and validation: removes the global note treating UNSPECIFIED as ASC, adds per-request defaulting docs, enforces enum-defined-only validation on sort fields/directions in multiple protos, and changes Go/SQL helpers to let query SQL determine per-request default ordering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant API as API Handler
participant GoSvc as Go Sorting Helpers
participant DB as PostgreSQL
Client->>API: List request (optional sort)
API->>GoSvc: pass sort param array
alt sort omitted
GoSvc->>DB: call SQL with empty sort params
DB->>DB: params CTE -> resolved_field=created_at, resolved_direction=DESC
else field specified, direction UNSPECIFIED
GoSvc->>DB: pass resolved_field=<col>, resolved_direction=""
DB->>DB: params CTE -> resolved_direction = ASC for provided field
else field UNSPECIFIED, direction specified
GoSvc->>DB: pass resolved_field=created_at, resolved_direction=<ASC|DESC>
end
DB->>API: rows ordered per p.resolved_field / p.resolved_direction
API->>Client: return ordered list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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 improves the robustness and clarity of the policy service API definitions. By enforcing strict enum validation on sorting parameters and unifying the documentation for default sorting behaviors, the changes ensure more predictable API interactions and better developer experience. 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 proto files were cleaned with care, To make the sorting logic fair. With validation now in place, No bad values leave a trace. Footnotes
|
github-actions
Bot
added
comp:policy
![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 standardizes sorting documentation across various list APIs and introduces buf.validate.field constraints to ensure enum values are defined in the protobuf definitions. While the standardization is helpful, the changes removed specific default ordering details (such as created_at DESC) from several proto files and the generated HTML documentation. Feedback focuses on restoring these specific implementation details to maintain API transparency for consumers.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Signed-off-by: Diego <74568547+dsm20@users.noreply.github.com>
![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 `@docs/grpc/index.html`:
- Line 7506: The docs claim that each List* request defines default ordering but
the endpoint docs only say the service applies a default without specifying what
it is; update the documentation so that for every List* request and endpoint
referenced (any docs mentioning the 'sort' field or the UNSPECIFIED enum value)
you explicitly state the implemented default ordering used when 'sort' is
omitted or 'field' == UNSPECIFIED — e.g., "default order: <field>
ascending/descending" — and ensure the List* request comment and the endpoint
description use the same precise wording so clients have a deterministic,
reproducible default for pagination; search for occurrences of 'sort',
'UNSPECIFIED', and "default ordering" in the generated docs and edit the List*
request and corresponding endpoint sections to include the concrete default
order.
🪄 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: ad4776be-1dd5-431f-826c-786c1ee83415
⛔ Files ignored due to path filters (9)
protocol/go/authorization/authorization.pb.gois excluded by!**/*.pb.goprotocol/go/policy/attributes/attributes.pb.gois excluded by!**/*.pb.goprotocol/go/policy/kasregistry/key_access_server_registry.pb.gois excluded by!**/*.pb.goprotocol/go/policy/namespaces/namespaces.pb.gois excluded by!**/*.pb.goprotocol/go/policy/objects.pb.gois excluded by!**/*.pb.goprotocol/go/policy/obligations/obligations.pb.gois excluded by!**/*.pb.goprotocol/go/policy/registeredresources/registered_resources.pb.gois excluded by!**/*.pb.goprotocol/go/policy/selectors.pb.gois excluded by!**/*.pb.goprotocol/go/policy/subjectmapping/subject_mapping.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamldocs/openapi/policy/namespaces/namespaces.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/selectors.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamlservice/policy/attributes/attributes.protoservice/policy/kasregistry/key_access_server_registry.protoservice/policy/namespaces/namespaces.protoservice/policy/obligations/obligations.protoservice/policy/registeredresources/registered_resources.protoservice/policy/selectors.protoservice/policy/subjectmapping/subject_mapping.proto
💤 Files with no reviewable changes (1)
- docs/openapi/policy/objects.openapi.yaml
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
trying to resolve versioning issue with buf on local setup
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
dsm20
marked this pull request as ready for review
April 27, 2026 19:05
c-r33d
reviewed
Apr 28, 2026
the bug: Get*SortParams previously returned "", "" when field was unspecified, ignoring the user's direction preference. This was a logic error causing silent disregarding of direction in the case the sort field was unspecified. the fix: getSortDirection now maps UNSPECIFIED to "", instead of ASC. Added new helper, get*SortField, which maps sort fields to strings, with the same handling for unspecified as getSortDirection, mapping unspecified field to "". This marks a new approach where the SQL handles all the defaults, the go layer simply maps unspecified fields to empty strings which are then handled in SQL. Added Get*SortParams wrapper that first checks if there is a *Sort struct (to account for the no-sort behavior of created_at DESC, or "", "" in Go. This function calls get*SortField and get*SortDirection and returns their values. This function is called in namespaces.go or equivalent to build the query.
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/utils.go`:
- Around line 78-85: Add targeted unit tests for GetNamespacesSortParams in
service/policy/db/utils_test.go that assert the new independent resolution
behavior: call GetNamespacesSortParams with (1) a slice containing a
NamespacesSort having a concrete field and SORT_DIRECTION_UNSPECIFIED and assert
it returns (resolvedField, "") using getNamespacesSortField(expected) semantics,
(2) a slice containing a NamespacesSort with field UNSPECIFIED and direction
DESC and assert it returns ("", "DESC") (i.e., getSortDirection(expected)
result), and (3) a slice containing a nil element and assert it returns ("", "")
to preserve current behavior; reference GetNamespacesSortParams,
getNamespacesSortField, and getSortDirection in test assertions and run go test
./... to verify.
🪄 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: d0d61bb1-acfe-4adc-95ea-9045a42d81df
📒 Files selected for processing (1)
service/policy/db/utils.go
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
same changes as namespaces: add get*SortField helper that correctly maps unspecified to "", removed nil verification from Get*SortParams (not needed)
shortened prefix, was failing in CI because the prefix + timestamp was too long(?)
Contributor
X-Test Failure Report✅ js-v0.9.0 |
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:
|
dsm20
marked this pull request as ready for review
May 1, 2026 18:44
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Around line 8868-8872: Update the "Sort defaults" paragraph in the docs (the
block that lists "direction UNSPECIFIED defaults..." and "field UNSPECIFIED
defaults...") to explicitly mention the SQL deterministic tie-breaker by adding
a short line such as "Tiebreaker: id ASC" after the existing defaults; apply the
same edit to every other matching "Sort defaults" occurrence in the document so
the pagination docs and implementation (id ASC tie-breaker) are aligned.
In `@docs/openapi/policy/attributes/attributes.openapi.yaml`:
- Around line 760-764: The shared enum schema policy.SortDirection currently
includes request-level default/fallback ordering guidance; remove that
fallback-ordering semantics from the policy.SortDirection description so it only
generically defines the enum values (e.g., ASC, DESC, UNSPECIFIED) and their
meaning, and ensure any default-ordering matrix or fallback behavior remains
documented only on the request-level field ListAttributesRequest.sort (update
that request's docs if needed). Locate and edit the description text in the
policy.SortDirection schema and delete the sentences about endpoint
defaults/fallback ordering, leaving only neutral enum documentation.
In `@docs/openapi/policy/kasregistry/key_access_server_registry.openapi.yaml`:
- Around line 1646-1651: The documentation for ListKeyAccessServersRequest.sort
explains defaulting rules but those defaults are not exposed on the flattened
query parameters; update the OpenAPI spec so the same default behavior is copied
to the GET query params "sort.field" and "sort.direction" (or add a clear note
in the /key-access-servers operation description) so OpenAPI consumers see: (1)
direction UNSPECIFIED defaults to DESC for the specified field, (2) field
UNSPECIFIED defaults to created_at with the specified direction, and (3) both
UNSPECIFIED or omitted defaults to created_at DESC; reference
ListKeyAccessServersRequest.sort, the query params sort.field and
sort.direction, and the /key-access-servers operation when making the change.
🪄 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: 4fc2a726-8220-49be-9bba-d6a280c1e412
⛔ Files ignored due to path filters (6)
protocol/go/policy/attributes/attributes.pb.gois excluded by!**/*.pb.goprotocol/go/policy/kasregistry/key_access_server_registry.pb.gois excluded by!**/*.pb.goprotocol/go/policy/namespaces/namespaces.pb.gois excluded by!**/*.pb.goprotocol/go/policy/obligations/obligations.pb.gois excluded by!**/*.pb.goprotocol/go/policy/registeredresources/registered_resources.pb.gois excluded by!**/*.pb.goprotocol/go/policy/subjectmapping/subject_mapping.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (32)
docs/grpc/index.htmldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/kasregistry/key_access_server_registry.openapi.yamldocs/openapi/policy/namespaces/namespaces.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamlservice/integration/attributes_test.goservice/integration/kas_registry_key_test.goservice/integration/kas_registry_test.goservice/integration/namespaces_test.goservice/integration/obligations_test.goservice/integration/registered_resources_test.goservice/integration/subject_mappings_test.goservice/policy/attributes/attributes.protoservice/policy/db/attributes.sql.goservice/policy/db/key_access_server_registry.sql.goservice/policy/db/namespaces.sql.goservice/policy/db/obligations.sql.goservice/policy/db/queries/attributes.sqlservice/policy/db/queries/key_access_server_registry.sqlservice/policy/db/queries/namespaces.sqlservice/policy/db/queries/obligations.sqlservice/policy/db/queries/registered_resources.sqlservice/policy/db/queries/subject_mappings.sqlservice/policy/db/registered_resources.sql.goservice/policy/db/subject_mappings.sql.goservice/policy/kasregistry/key_access_server_registry.protoservice/policy/namespaces/namespaces.protoservice/policy/obligations/obligations.protoservice/policy/registeredresources/registered_resources.protoservice/policy/subjectmapping/subject_mapping.proto
c-r33d
reviewed
May 3, 2026
c-r33d
reviewed
May 3, 2026
c-r33d
left a comment
c-r33d
left a comment
Contributor
There was a problem hiding this comment.
Looks good overall, I would add a couple nil checks around db utils methods when calling sort[0].Get.... While the Get methods are nil safe, it's probably good to have a small test where the Sort[] has one nil object.
unit tests passing in nil for sort, which resolves to the default "", ""
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
makes a listRPC request with no sort field, which in turn passes getSort() an empty array, defaulting to the params "", "" via get*SortParams helper
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
elizabethhealy
approved these changes
May 4, 2026
dsm20
added a commit
that referenced
this pull request
May 4, 2026
#3402 added more integration tests that needed cleanup calls
JBCongdon
pushed a commit
to JBCongdon/platform
that referenced
this pull request
May 24, 2026
…ests, add test cleanup (opentdf#3403) ## Proposed Changes Consolidates integration test helpers across all policy sort test suites, adds proper test cleanup, and adds tiebreaker-specific integration tests that verify the `id ASC` fallback produces deterministic ordering. ## Changes ### Integration Test Helpers -- service/integration/utils.go - `forceDeleteRows`: hard-deletes rows by ID via raw SQL, bypassing the API's soft-delete/deactivate limitation for test cleanup - `forceCreatedAtTie`: sets `created_at` to a fixed timestamp for given IDs, guaranteeing that the `ORDER BY` tiebreaker (`id ASC`) determines sort order ### Integration Test Consolidation -- service/integration/*_test.go - Refactored `createSortTest*` helpers from `(label string)` to `(prefixes []string)` across all RPCs, giving each test explicit control over resource names - Added `defer` cleanup calls to all sort integration tests via `deleteSortTest*` helpers - Added `Test_List*_SortTieBreaker_CreatedAtWithIDFallback` for all 8 RPCs: Namespaces, Attributes, KeyAccessServers, KasKeys, Obligations, RegisteredResources, SubjectMappings, SubjectConditionSets ### Tiebreaker Test Strategy Each tiebreaker test: 1. Creates 3 resources 2. Forces identical `created_at` timestamps via `forceCreatedAtTie` 3. Sorts the UUIDs lexicographically to get expected order 4. Lists with `created_at ASC` and asserts rows match the UUID-sorted order This proves the `id ASC` tiebreaker added in opentdf#3402 produces deterministic results when the primary sort column ties. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Tests** * Standardized sort test helpers across integration tests to ensure consistent attribute, key, namespace, obligation, and resource creation with deterministic timestamps. * Added tie-breaker tests to validate deterministic ordering behavior when creation timestamps are equal. * Enhanced test utilities for database cleanup and timestamp management. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.
refactor(policy): decouple sort field/direction mapping and add SQL-level sort defaults
Proposed Changes
Fixes a silent bug where providing a sort direction with an UNSPECIFIED field caused the direction to be discarded. Refactors the Go sort mappers to be pure enum-to-string translators, moves all default logic into SQL via CTEs, and documents all default behavior at the proto level.
Bug Fixed
Get*SortParams previously coupled field and direction mapping. When the sort field was UNSPECIFIED, both field AND direction were returned as empty strings (return "", ""), silently discarding any user-provided direction. A user sending field=UNSPECIFIED, direction=ASC would get created_at DESC instead of created_at ASC.
Sort Defaults
All defaults are now resolved in SQL via a CTE:
Changes
Go Sort Mappers -- service/policy/db/utils.go
SQL Queries -- service/policy/db/queries/*.sql
Added CTE-based sort default resolution to all 9 list queries (8 RPCs):
Also added id ASC tiebreaker and p.resolved_field, p.resolved_direction to GROUP BY where needed.
Affected queries: listNamespaces, listAttributesDetail, listAttributesSummary, listObligations, listRegisteredResources, listKeyAccessServers, listKeys, listSubjectConditionSets, listSubjectMappings
Proto Docs -- all List*Request sort field comments
Replaced vague "the service applies its default ordering" with explicit defaults:
Unit Tests -- service/policy/db/utils_test.go
Updated all 8 test functions:
Integration Tests -- service/integration/*_test.go
Replaced 8 FallsBackToDefault tests with 24 targeted default tests (3 per RPC):
Checklist
Testing Instructions
Unit tests: go test ./service/policy/db/... -run "Sort"
Summary by CodeRabbit
Release Notes
Improvements
created_at DESCwhen omitted,created_atwith specified direction when field is unspecified, andDESCwith specified field when direction is unspecified.Documentation