docs(adr): bounded attribute value responses#3747
Conversation
Adds a proposed ADR evaluating five options for addressing unbounded JSON_AGG on attribute values in the Attributes Service (issue #3744). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Eugene Yakhnenko <eugene.yakhnenko@virtru.com>
Reduce verbosity, add rejected alternatives section, clarify that options 1 and 4 are breaking changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Eugene Yakhnenko <eugene.yakhnenko@virtru.com>
📝 WalkthroughWalkthroughAdds an ADR proposing bounded Attributes Service v2 responses, with values omitted from attribute listings and single-attribute fetches and retrieved through a paginated ChangesAttributes Service v2 decision
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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 an Architecture Decision Record (ADR) to resolve performance bottlenecks in the Attributes Service. The current implementation relies on unbounded JSON aggregation, which negatively impacts database performance and response latency. The document outlines various strategies to implement pagination while maintaining backward compatibility and adhering to established platform design patterns. Highlights
New Features🧠 You can now enable Memory (public preview) 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. 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 the 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 counterproductive. 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 query grows wide and deep, / The database starts to weep. / We seek a new way, / To keep lag at bay, / And let the pagination sleep. Footnotes
|
![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 proposes an Architecture Decision Record (ADR) to address performance issues caused by unbounded attribute value responses in the Attributes Service, recommending a v2 API with a dedicated, paginated ListAttributeValues RPC. The review feedback suggests clarifying that ListAttributeValues was deprecated rather than completely removed in PR #3108, and highlights the SQL complexity of paginating across multiple attribute IDs in a single query as an additional drawback of the chosen option.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| `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. |
There was a problem hiding this comment.
The ListAttributeValues RPC is still defined in service/policy/attributes/attributes.proto (line 543) but is marked as deprecated. 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.
| `ListAttributeValues` was removed in [PR #3108](https://github.com/opentdf/platform/pull/3108), leaving no paginated way to fetch values. | |
| `ListAttributeValues` was deprecated in [PR #3108](https://github.com/opentdf/platform/pull/3108) (though the RPC definition remains in the v1 proto), leaving no active paginated way to fetch values. |
| * 🟩 **Good**, multi-attribute fetch in one paginated call avoids N+1 | ||
| * 🟩 **Good**, v2 versioning follows authorization v2 pattern, non-breaking | ||
| * 🟨 **Neutral**, consumers need two calls for attribute + values | ||
| * 🟥 **Bad**, new SQL queries partially duplicate existing ones (minus values join) |
There was a problem hiding this comment.
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.
| * 🟥 **Bad**, new SQL queries partially duplicate existing ones (minus values join) | |
| * 🟥 **Bad**, new SQL queries partially duplicate existing ones (minus values join)\n* 🟥 **Bad**, pagination logic across multiple `attribute_ids` in a single query is complex to implement efficiently |
| ## More Information | ||
|
|
||
| * [Issue #3744](https://github.com/opentdf/platform/issues/3744) | ||
| * [PR #3108](https://github.com/opentdf/platform/pull/3108) removed `ListAttributeValues` |
There was a problem hiding this comment.
Update this reference to reflect that the RPC was deprecated rather than completely removed.
| * [PR #3108](https://github.com/opentdf/platform/pull/3108) removed `ListAttributeValues` | |
| * [PR #3108](https://github.com/opentdf/platform/pull/3108) deprecated `ListAttributeValues` |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adr/decisions/2026-07-10-attributes-v2-bounded-value-responses.md`:
- Around line 21-35: Revise the ADR’s “Decision Drivers” and related decision
sections to scope elimination of unbounded JSON_AGG explicitly to v2, or
document how unchanged v1 RPCs are mitigated. For the v1 compatibility
discussion and Option 3, state the residual risk and define migration,
deprecation, or operational guardrails for v1 consumers, ensuring the selected
option’s scope matches its stated driver.
- Around line 41-48: Revise the Option 1 assessment to distinguish request
compatibility from response semantics: state that omitting values_pagination
preserves existing callers, while opting in introduces a different response
shape requiring explicit pagination metadata and semantics. Remove the
contradictory “breaking change disguised as an optional parameter” wording and
update the rejection rationale accordingly.
- Around line 69-76: Revise “Option 4: Application-layer cap” to accurately
distinguish database-side and application-side capping: remove the claim that
LIMIT can be used directly inside JSON_AGG, describe pre-aggregation subqueries,
LATERAL joins, or window filtering as required for database-side limits, and
separately describe truncating or erroring after fetching the full result.
Update the pros and cons to reflect each approach before evaluating them.
🪄 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: de1b98e8-d2c4-4646-ae5e-2a9fc8914abb
📒 Files selected for processing (1)
adr/decisions/2026-07-10-attributes-v2-bounded-value-responses.md
| ## 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 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Scope the “eliminate unbounded JSON_AGG” driver to the supported version.
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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adr/decisions/2026-07-10-attributes-v2-bounded-value-responses.md` around
lines 21 - 35, Revise the ADR’s “Decision Drivers” and related decision sections
to scope elimination of unbounded JSON_AGG explicitly to v2, or document how
unchanged v1 RPCs are mitigated. For the v1 compatibility discussion and Option
3, state the residual risk and define migration, deprecation, or operational
guardrails for v1 consumers, ensuring the selected option’s scope matches its
stated driver.
| 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 |
There was a problem hiding this comment.
🗄️ 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adr/decisions/2026-07-10-attributes-v2-bounded-value-responses.md` around
lines 41 - 48, Revise the Option 1 assessment to distinguish request
compatibility from response semantics: state that omitting values_pagination
preserves existing callers, while opting in introduces a different response
shape requiring explicit pagination metadata and semantics. Remove the
contradictory “breaking change disguised as an optional parameter” wording and
update the rejection rationale accordingly.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Add Decision Outcome section, cap repeated attribute_ids input, note ListAttributesByFqns replaced by ListAttributes with FQN filter, trim rejected alternatives, reclassify two-call cost as Bad, fix Option 4 SQL analysis. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Eugene Yakhnenko <eugene.yakhnenko@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
| * 🟩 **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) |
There was a problem hiding this comment.
| * 🟥 **Bad**, new SQL queries partially duplicate existing ones (minus values join) | |
| * 🟥 **Bad**, new SQL queries partially duplicate existing ones (minus values join) | |
| * 🟥 **Bad**, offset/limit pagination means deep offsets get expensive |
There was a problem hiding this comment.
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.
offset/limit to a deep value is an inherent problem of pagination that is applied in the rest of list endpoints as well.
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
2 participants
Summary
JSON_AGGon attribute values in the Attributes ServiceRelated
Summary by CodeRabbit