feat(policy): add SortField proto and update PageRequest for sort support #3187
Merged
Conversation
github-actions
Bot
added
comp:db
|
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 ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdded a top-level Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 foundational changes to enable sorting capabilities for paginated requests across the service. It defines the necessary protocol buffer structures for specifying sort fields and directions, implements a utility function to parse these parameters, and includes corresponding unit tests to ensure correctness. This enhancement allows for more flexible data retrieval based on client-defined sorting preferences. 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, sorted, Order brings clarity, Code finds its true path. Footnotes
|
![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
The pull request introduces new Protobuf definitions for SortDirection and SortField, and integrates a repeated SortField sort into the PageRequest message, along with a max_items = 1 validation rule. A new Go utility function, GetSortParams, has been added to extract the sort field and direction from the PageRequest. While these changes successfully lay the groundwork for sorting functionality, the current implementation only processes the first SortField due to the max_items = 1 constraint. Furthermore, the existing database query functions do not yet utilize these new sorting parameters, meaning the sorting functionality is not actively applied in the data retrieval layer. This integration would be the next logical step to fully enable the feature.
dsm20
marked this pull request as draft
March 20, 2026 16:06
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
dsm20
changed the title
![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 `@service/policy/db/utils_test.go`:
- Around line 103-139: Add a table-driven test case in the existing cases in
utils_test.go to cover a nil pointer element in the sort slice: add an entry
named like "nil-element pointer returns fallback" with sort:
[]*policy.SortField{nil} and set expectedField to "" and expectedDir to "ASC"
(matching the unspecified-direction fallback used for policy.SortField where
Direction == SORT_DIRECTION_UNSPECIFIED); ensure the test invokes the same
conversion function under test (the test cases loop that reads policy.SortField
entries) and asserts the returned field and direction equal expectedField and
expectedDir to prevent regressions for malformed pointer-slice inputs.
In `@service/policy/selectors.proto`:
- Around line 63-69: Add buf.validate rules to the SortField.message: require
SortField.field to be non-empty (e.g., string min_len = 1 or a sensible regex
pattern) so empty strings are rejected, and add an items-level validation on
PageRequest.sort to ensure each SortField is validated (keeping the existing
max_items = 1). Update the SortField proto to include (buf.validate.rules).field
{ string { min_len: 1 } } (or string { pattern: "..." } if you want a stricter
pattern) and ensure PageRequest.sort has (buf.validate.rules).items referencing
the SortField validation so invalid/empty sort fields fail schema validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05ccb029-7537-4c61-b7dd-3e7a9e6bc3aa
⛔ Files ignored due to path filters (1)
protocol/go/policy/selectors.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
service/policy/db/utils.goservice/policy/db/utils_test.goservice/policy/selectors.proto
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
dsm20
changed the title
Contributor
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:
|
Contributor
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
March 24, 2026 18:49
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/selectors.proto`:
- Around line 55-59: The PR title says "add SortField proto and update
PageRequest for sort support" but only the SortDirection enum was added; decide
which path to take and implement accordingly: either update the PR
title/description to state only SortDirection was added (leave enum
SortDirection as-is) or complete the proto changes by adding a new message
SortField (e.g., fields like string field = 1; SortDirection direction = 2) and
adding an optional repeated or singular sort field to PageRequest (e.g.,
PageRequest { ... repeated SortField sort = X } or optional SortField sort = X),
then regenerate gRPC/Go/OpenAPI artifacts; locate symbols SortDirection,
SortField, and PageRequest in selectors.proto to add or adjust definitions and
ensure corresponding generated code/artifacts are updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89e2b4d1-22a9-4771-a10b-bf90b7f7d198
⛔ Files ignored due to path filters (1)
protocol/go/policy/selectors.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
docs/grpc/index.htmldocs/openapi/policy/selectors.openapi.yamlservice/policy/selectors.proto
c-r33d
reviewed
Mar 25, 2026
Contributor
|
Was there supposed to be a PageRequest change in this PR, and could you update the PR description please? |
Contributor
Author
Updated, old PR title was out of date. There was an amendment to the approach used, now each RPC gets a strongly typed parameter for sorting instead of having a sort field in PageRequest |
alkalescent
reviewed
Mar 25, 2026
alkalescent
previously approved these changes
Mar 25, 2026
Signed-off-by: Diego <74568547+dsm20@users.noreply.github.com>
Delete SortField message and corresponding usage in PageRequest message. Keeping SortDirection enum as per new strong typing approach.
protocol/../selectors.pb.go regenerated after running buf generate
Got rid of getSortParams (old approach) and its matching tests, from service/policy/db/utils.go and utils_test.go.
Deleted unused import line previously used in SortField implementation.
Regenerated protos and docs (was missing proto-generate from make in last commit)
dsm20
force-pushed
the
feat/DSPX-2680-add-sortfield-proto-pagerequest
branch
from
March 31, 2026 15:35
a51c3e0 to
73a98e2
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
alkalescent
approved these changes
Mar 31, 2026
dsm20
enabled auto-merge
March 31, 2026 19:46
c-r33d
approved these changes
Mar 31, 2026
This was referenced Mar 31, 2026
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Apr 1, 2026
🤖 I have created a release *beep* *boop* --- ## [0.22.0](protocol/go/v0.21.0...protocol/go/v0.22.0) (2026-04-01) ### Features * **policy:** Add sort support to ListNamespaces API ([#3192](#3192)) ([aac86cd](aac86cd)) * **policy:** add SortField proto and update PageRequest for sort support ([#3187](#3187)) ([6cf1862](6cf1862)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
This was referenced Apr 2, 2026
3 tasks
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Apr 6, 2026
Resolves [DSPX-2682](https://virtru.atlassian.net/browse/DSPX-2682), depends on #3187 ## Proposed Changes - Adds strongly-typed sort support to `ListAttributes` RPC, following the pattern established in #3192 (ListNamespaces) - Sortable fields: `name`, `created_at`, `updated_at` (ASC/DESC), with backward-compatible fallback to `created_at DESC` - Includes CASE WHEN ordering in both `listAttributesDetail` and `listAttributesSummary` SQL queries ## Changes - **Proto**: `SortAttributesType` enum, `AttributesSort` message, `sort` field on `ListAttributesRequest` (max 1, buf.validate) - **SQL**: CASE WHEN sort blocks in `attributes.sql` for both detail and summary queries - **Go**: `GetAttributesSortParams()` helper in `utils.go`, wired into `ListAttributes()` in `attributes.go` - **Tests**: 9 unit tests for the helper, 5 integration tests covering each sort field + direction + fallback ### Checklist - [x] I have added or updated unit tests - [x] I have added or updated integration tests (if appropriate) - [x] I have added or updated documentation [DSPX-2682]: https://virtru.atlassian.net/browse/DSPX-2682?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added sorting capability for attributes with options to sort by name, creation date, or last updated date in ascending or descending order. * **Documentation** * Updated API documentation with new sorting parameters and clarified timestamp parameter constraints. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Diego <74568547+dsm20@users.noreply.github.com>
This was referenced Apr 7, 2026
This was referenced Apr 15, 2026
Merged
3 tasks
JBCongdon
pushed a commit
to JBCongdon/platform
that referenced
this pull request
May 24, 2026
🤖 I have created a release *beep* *boop* --- ## [0.14.0](opentdf/platform@service/v0.13.0...service/v0.14.0) (2026-04-21) ### ⚠ BREAKING CHANGES * **sdk:** reclassify KAS 400 errors — distinguish tamper from misconfiguration ([opentdf#3166](opentdf#3166)) * **policy:** optional namespace for RRs ([opentdf#3165](opentdf#3165)) * **policy:** Namespace subject mappings and subject condition sets. ([opentdf#3143](opentdf#3143)) * **policy:** Optional namespace on actions protos, NamespacedPolicy feature flag ([opentdf#3155](opentdf#3155)) * **policy:** add namespaced actions schema and namespace-aware action queries ([opentdf#3154](opentdf#3154)) * **policy:** only require namespace on GetAction if no id provided ([opentdf#3144](opentdf#3144)) * **policy:** add namespace field to Actions proto ([opentdf#3130](opentdf#3130)) * **policy:** namespace Registered Resources ([opentdf#3111](opentdf#3111)) * **policy:** add namespace field to RegisteredResource proto ([opentdf#3110](opentdf#3110)) ### Features * **authz:** Namespaced policy in decisioning ([opentdf#3226](opentdf#3226)) ([0355934](opentdf@0355934)) * **cli:** migrate otdfctl into platform monorepo ([opentdf#3205](opentdf#3205)) ([5177bec](opentdf@5177bec)) * fix tracing ([opentdf#3242](opentdf#3242)) ([57e5680](opentdf@57e5680)) * **policy:** add GetObligationTrigger RPC ([opentdf#3318](opentdf#3318)) ([d68e39d](opentdf@d68e39d)) * **policy:** add namespace field to Actions proto ([opentdf#3130](opentdf#3130)) ([bedc9b3](opentdf@bedc9b3)) * **policy:** add namespace field to RegisteredResource proto ([opentdf#3110](opentdf#3110)) ([04fd85d](opentdf@04fd85d)) * **policy:** add namespaced actions schema and namespace-aware action queries ([opentdf#3154](opentdf#3154)) ([c0443f1](opentdf@c0443f1)) * **policy:** add sort ListSubjectMappings API ([opentdf#3255](opentdf#3255)) ([9d5d757](opentdf@9d5d757)) * **policy:** Add sort support listregisteredresources api ([opentdf#3312](opentdf#3312)) ([91a3ff3](opentdf@91a3ff3)) * **policy:** add sort support to ListAttributes API ([opentdf#3223](opentdf#3223)) ([ec3312f](opentdf@ec3312f)) * **policy:** add sort support to ListKeyAccessServer ([opentdf#3287](opentdf#3287)) ([7fae2d7](opentdf@7fae2d7)) * **policy:** Add sort support to ListNamespaces API ([opentdf#3192](opentdf#3192)) ([aac86cd](opentdf@aac86cd)) * **policy:** add sort support to listobligations api ([opentdf#3300](opentdf#3300)) ([9221cac](opentdf@9221cac)) * **policy:** add sort support to ListSubjectConditionSets API ([opentdf#3272](opentdf#3272)) ([9010f12](opentdf@9010f12)) * **policy:** add SortField proto and update PageRequest for sort support ([opentdf#3187](opentdf#3187)) ([6cf1862](opentdf@6cf1862)) * **policy:** Enforce same namespace when actions referenced downstream ([opentdf#3206](opentdf#3206)) ([4b5463a](opentdf@4b5463a)) * **policy:** namespace Registered Resources ([opentdf#3111](opentdf#3111)) ([6db1883](opentdf@6db1883)) * **policy:** Namespace subject mappings and condition sets ([opentdf#3172](opentdf#3172)) ([6deed50](opentdf@6deed50)) * **policy:** Namespace subject mappings and subject condition sets. ([opentdf#3143](opentdf#3143)) ([3006780](opentdf@3006780)) * **policy:** optional namespace for RRs ([opentdf#3165](opentdf#3165)) ([8948018](opentdf@8948018)) * **policy:** rollback migration strategy for namespaced actions ([opentdf#3235](opentdf#3235)) ([f7e5e01](opentdf@f7e5e01)) * **policy:** Seed existing namespaces with standard actions ([opentdf#3228](opentdf#3228)) ([12136b0](opentdf@12136b0)) * **policy:** Seed namespaces with standard actions on creation + namespaced actions for obligation triggers ([opentdf#3161](opentdf#3161)) ([984d76b](opentdf@984d76b)) ### Bug Fixes * **ci:** Upgrade toolchain version to 1.25.8 ([opentdf#3116](opentdf#3116)) ([e1b7882](opentdf@e1b7882)) * **core:** do not concat slashes directly in url/file paths ([opentdf#3290](opentdf#3290)) ([114c2a7](opentdf@114c2a7)) * **deps:** bump github.com/jackc/pgx/v5 from 5.7.5 to 5.9.0 in /service ([opentdf#3316](opentdf#3316)) ([017362e](opentdf@017362e)) * **deps:** bump github.com/opentdf/platform/lib/identifier from 0.2.0 to 0.3.0 in /service ([opentdf#3162](opentdf#3162)) ([8bc5dcd](opentdf@8bc5dcd)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.16.0 to 0.17.0 in /service ([opentdf#3125](opentdf#3125)) ([29fec61](opentdf@29fec61)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.17.0 to 0.21.0 in /service ([opentdf#3220](opentdf#3220)) ([e63add2](opentdf@e63add2)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.21.0 to 0.22.0 in /service ([opentdf#3248](opentdf#3248)) ([1ebce73](opentdf@1ebce73)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.22.0 to 0.23.0 in /service ([opentdf#3271](opentdf#3271)) ([3338b8e](opentdf@3338b8e)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.23.0 to 0.24.0 in /service ([opentdf#3321](opentdf#3321)) ([78e6022](opentdf@78e6022)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.24.0 to 0.25.0 in /service ([opentdf#3333](opentdf#3333)) ([3940bf8](opentdf@3940bf8)) * **deps:** bump github.com/opentdf/platform/sdk from 0.13.0 to 0.16.0 in /service ([opentdf#3356](opentdf#3356)) ([5617077](opentdf@5617077)) * **deps:** bump go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp from 1.42.0 to 1.43.0 in /service ([opentdf#3282](opentdf#3282)) ([046374a](opentdf@046374a)) * **deps:** bump go.opentelemetry.io/otel/sdk from 1.42.0 to 1.43.0 in /service ([opentdf#3281](opentdf#3281)) ([56b33f2](opentdf@56b33f2)) * **deps:** bump google.golang.org/grpc from 1.77.0 to 1.79.3 in /service ([opentdf#3176](opentdf#3176)) ([3289502](opentdf@3289502)) * **deps:** remove direct github.com/docker/docker dependency ([opentdf#3229](opentdf#3229)) ([2becb27](opentdf@2becb27)) * **deps:** upgrade testcontainers-go to resolve vulns ([opentdf#3299](opentdf#3299)) ([72c6f9b](opentdf@72c6f9b)) * **ers:** include standard JWT claims in claims mode entity resolution ([opentdf#3196](opentdf#3196)) ([6d50da1](opentdf@6d50da1)) * **ers:** ldap multi-strategy ers ([opentdf#3117](opentdf#3117)) ([d3aaf1a](opentdf@d3aaf1a)) * **policy:** deprecate ListAttributeValues in favor of existing GetAttribute ([opentdf#3108](opentdf#3108)) ([7e17c2d](opentdf@7e17c2d)) * **policy:** make obligation trigger uniqueness client-aware ([opentdf#3114](opentdf#3114)) ([9265bc3](opentdf@9265bc3)) * **policy:** omit empty attribute values from create responses ([opentdf#3193](opentdf#3193)) ([d298378](opentdf@d298378)) * **policy:** only require namespace on GetAction if no id provided ([opentdf#3144](opentdf#3144)) ([10d0c0f](opentdf@10d0c0f)) * **policy:** Optional namespace on actions protos, NamespacedPolicy feature flag ([opentdf#3155](opentdf#3155)) ([c20f039](opentdf@c20f039)) * **policy:** order List* results by created_at ([opentdf#3088](opentdf#3088)) ([ea90ac2](opentdf@ea90ac2)) * **sdk:** normalize issuer URL before OIDC discovery ([opentdf#3261](opentdf#3261)) ([61f98c9](opentdf@61f98c9)) * **sdk:** reclassify KAS 400 errors — distinguish tamper from misconfiguration ([opentdf#3166](opentdf#3166)) ([f04a385](opentdf@f04a385)) * **sdk:** remove testcontainers from consumer dependency graph ([opentdf#3129](opentdf#3129)) ([f17dcdd](opentdf@f17dcdd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
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.
Updated PR Description (was out of date)
ASC), `ASC`, and `DESC` values
This provides the shared sort direction building block for the strongly-typed sort support being added
per-service (e.g. DSPX-2681 for `ListNamespaces`). Rather than a generic `SortField` on
`PageRequest`, each list RPC defines its own typed sort parameter.
Checklist
Summary by CodeRabbit
New Features
Documentation