-
Notifications
You must be signed in to change notification settings - Fork 37
docs(adr): bounded attribute value responses #3747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,108 @@ | ||||||||||||
| --- | ||||||||||||
| status: proposed | ||||||||||||
| date: 2026-07-10 | ||||||||||||
| tags: | ||||||||||||
| - policy | ||||||||||||
| - attributes | ||||||||||||
| - pagination | ||||||||||||
| - performance | ||||||||||||
| driver: Eugene Yakhnenko | ||||||||||||
| --- | ||||||||||||
| # Bounded attribute value responses in the Attributes Service | ||||||||||||
|
|
||||||||||||
| ## Context and Problem Statement | ||||||||||||
|
|
||||||||||||
| `ListAttributes`, `GetAttribute`, and `ListAttributesByFqns` return all attribute values inline via unbounded `JSON_AGG`. High-cardinality attributes cause Postgres memory pressure, oversized gRPC responses, and high latency. | ||||||||||||
|
|
||||||||||||
| `ListAttributeValues` was removed in [PR #3108](https://github.com/opentdf/platform/pull/3108), leaving no paginated way to fetch values. | ||||||||||||
|
|
||||||||||||
| See [issue #3744](https://github.com/opentdf/platform/issues/3744). | ||||||||||||
|
|
||||||||||||
| ## Decision Drivers | ||||||||||||
|
|
||||||||||||
| * Eliminate unbounded `JSON_AGG` on attribute values | ||||||||||||
| * Provide paginated value retrieval | ||||||||||||
| * No breaking changes to v1 consumers | ||||||||||||
| * Follow existing platform patterns (`ListRegisteredResourceValues`, authorization v2) | ||||||||||||
| * Support multi-attribute value fetching in a single call | ||||||||||||
|
|
||||||||||||
| ## Considered Options | ||||||||||||
|
|
||||||||||||
| 1. **Modify v1 RPCs**: add `values_pagination` to existing requests | ||||||||||||
| 2. **v2 with inline sub-pagination**: v2 `GetAttribute` returns paginated values inline, `ListAttributes` strips values | ||||||||||||
| 3. **v2 with dedicated ListAttributeValues**: v2 strips values from all attribute RPCs, new `ListAttributeValues` RPC with pagination | ||||||||||||
| 4. **Application-layer cap**: `LIMIT` inside `JSON_AGG`, no API changes | ||||||||||||
| 5. **Do nothing**: accept current behavior | ||||||||||||
|
Comment on lines
+21
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift Scope the “eliminate unbounded Option 3 only bounds v2 responses; unchanged v1 RPCs remain unbounded. The ADR should explicitly accept that residual risk and define migration, deprecation, or operational guardrails—or change the driver to “eliminate unbounded aggregation in v2.” As written, the proposed decision does not satisfy its primary driver for existing v1 consumers. (github.com) Also applies to: 58-68 🤖 Prompt for AI Agents |
||||||||||||
|
|
||||||||||||
| ## Pros and Cons of the Options | ||||||||||||
|
|
||||||||||||
| ### Option 1: Modify v1 RPCs | ||||||||||||
|
|
||||||||||||
| Add optional `values_pagination` to v1 requests. Default returns all values for backward compatibility. | ||||||||||||
|
|
||||||||||||
| * 🟩 **Good**, no new service version or RPCs | ||||||||||||
| * 🟩 **Good**, existing consumers unaffected | ||||||||||||
| * 🟥 **Bad**, default path still unbounded | ||||||||||||
| * 🟥 **Bad**, per-attribute pagination cursors inside a list response is complex | ||||||||||||
| * 🟥 **Bad**, Postgres still joins and aggregates all values unless SQL is restructured | ||||||||||||
| * 🟥 **Bad**, using the optional field changes the response shape, making it a breaking change disguised as an optional parameter | ||||||||||||
|
Comment on lines
+41
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Separate backward compatibility from opt-in response semantics. Line [41] says existing consumers remain unaffected when the field is omitted, but Line [48] calls the opt-in behavior a breaking change. Adding an optional request field is compatible for existing callers; the new mode may still require clear documentation of pagination metadata and response semantics. The current wording weakens the rejection rationale for Option 1. 🤖 Prompt for AI Agents |
||||||||||||
|
|
||||||||||||
| ### Option 2: v2 with inline sub-pagination on GetAttribute | ||||||||||||
|
|
||||||||||||
| v2 `ListAttributes` strips values. v2 `GetAttribute` returns attribute + paginated values. | ||||||||||||
|
|
||||||||||||
| * 🟩 **Good**, single call returns attribute + first page of values | ||||||||||||
| * 🟥 **Bad**, SQL still joins values even when caller only needs attribute metadata | ||||||||||||
| * 🟥 **Bad**, no existing platform pattern for inline sub-resource pagination | ||||||||||||
|
|
||||||||||||
| ### Option 3: v2 with dedicated ListAttributeValues RPC | ||||||||||||
|
|
||||||||||||
| v2 `ListAttributes` and `GetAttribute` return attributes without values. Separate `ListAttributeValues` RPC with state/sort/search filters, accepts `repeated attribute_ids`. | ||||||||||||
|
|
||||||||||||
| * 🟩 **Good**, attribute queries fully decoupled from values: no `JSON_AGG` | ||||||||||||
| * 🟩 **Good**, follows `ListRegisteredResourceValues` pattern | ||||||||||||
| * 🟩 **Good**, multi-attribute fetch in one paginated call avoids N+1 | ||||||||||||
| * 🟩 **Good**, v2 versioning follows authorization v2 pattern, non-breaking | ||||||||||||
| * 🟥 **Bad**, consumers need two calls for attribute + values | ||||||||||||
| * 🟥 **Bad**, new SQL queries partially duplicate existing ones (minus values join) | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Paginating across multiple attribute IDs in a single query can introduce significant SQL complexity and potential performance overhead, especially when dealing with high-cardinality attributes and sorting/filtering. It would be beneficial to list this as a potential drawback or complexity to address during implementation.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for taking a look! That's technically 100% correct. Adding it here though would unfairly weight against this option when Options 1 and 2 would have the same issue. We could mitigate it by providing next cursor pagination but that should be done for all pagination lists and sounds like a separate (and very valid) ADR |
||||||||||||
|
|
||||||||||||
| ### Option 4: Application-layer cap | ||||||||||||
|
|
||||||||||||
| Cap values via `LIMIT` in `JSON_AGG` (truncate) or error when a threshold is exceeded. No API changes. | ||||||||||||
|
|
||||||||||||
| * 🟩 **Good**, minimal code change | ||||||||||||
| * 🟥 **Bad**, breaking change either way: truncation drops data silently, erroring fails previously working calls | ||||||||||||
| * 🟥 **Bad**, no pagination mechanism to retrieve beyond the cap | ||||||||||||
| * 🟥 **Bad**, requires SQL restructuring (e.g., lateral subqueries) to avoid full aggregation before capping | ||||||||||||
|
|
||||||||||||
| ### Option 5: Do nothing | ||||||||||||
|
|
||||||||||||
| Operators manage attribute cardinality through governance. | ||||||||||||
|
|
||||||||||||
| * 🟩 **Good**, zero effort | ||||||||||||
| * 🟥 **Bad**, no guardrail: one high-cardinality attribute can degrade the service | ||||||||||||
| * 🟥 **Bad**, no paginated value retrieval exists | ||||||||||||
| * 🟥 **Bad**, risk is invisible to operators until an outage | ||||||||||||
|
|
||||||||||||
| ## Decision Outcome | ||||||||||||
|
|
||||||||||||
| Chosen option: **Option 3, v2 with dedicated ListAttributeValues RPC**. Attribute queries are fully decoupled from value queries, follows existing platform patterns, and is non-breaking. | ||||||||||||
|
|
||||||||||||
| `repeated attribute_ids` is capped (e.g., 100) and validated server-side to prevent shifting unbounded pressure from output to input. Pagination applies across all matched values flattened. | ||||||||||||
|
|
||||||||||||
| `ListAttributesByFqns` is replaced by v2 `ListAttributes` with an FQN filter. | ||||||||||||
|
|
||||||||||||
| ### Rejected Alternatives | ||||||||||||
|
|
||||||||||||
| * **Option 1: Modify v1 RPCs**: breaking change disguised as an optional field. | ||||||||||||
| * **Option 4: Application-layer cap**: breaking change with no pagination path forward. | ||||||||||||
|
|
||||||||||||
| ## Validation | ||||||||||||
|
|
||||||||||||
| {To be filled after decision is accepted.} | ||||||||||||
|
|
||||||||||||
| ## More Information | ||||||||||||
|
|
||||||||||||
| * [Issue #3744](https://github.com/opentdf/platform/issues/3744) | ||||||||||||
| * [PR #3108](https://github.com/opentdf/platform/pull/3108) removed `ListAttributeValues` | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||
| * Platform patterns: `ListRegisteredResourceValues`, `authorization/v2`, `PageRequest`/`PageResponse` | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ListAttributeValuesRPC is still defined inservice/policy/attributes/attributes.proto(line 543) but is marked asdeprecated. To be precise, it was deprecated rather than completely removed from the API definition. Clarifying this distinction helps avoid confusion for readers looking at the current proto files.