Repository navigation
fix(policy): narrow authorization attribute lookups - #3986
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 46 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesEntitleable attribute lookup
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The targeted lookup preserves the documented normalization, inactive-value, traversal, and hierarchy behavior. No concrete merge-blocking issue remains; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The narrower lookup preserves the inspected authorization controls and avoids fetching grants and keys that its response does not use. No material security risk introduced or worsened by this change was identified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 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. A rabbit checks the FQNs in a row, Comment |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
45c0f01 to
a3ae767
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: strantalis <strantalis@virtru.com>
a3ae767 to
7cf27e4
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: strantalis <strantalis@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The optimized path preserves existing behavior and is covered by focused integration tests.
Review effort: Balanced
Findings: None
What changed in this PR
Introduces a targeted policy lookup for entitlement authorization, avoiding unnecessary grants, keys, and resource mappings.
Changes:
- Adds an optimized SQL query for requested and hierarchical values.
- Preserves normalization, traversal, ordering, and inactive-value handling.
- Expands integration coverage for edge cases.
| File | Description |
|---|---|
service/policy/db/queries/entitleable_attributes.sql |
Defines the targeted lookup. |
service/policy/db/entitleable_attributes.sql.go |
Adds generated sqlc bindings. |
service/policy/db/entitleable_attributes.go |
Resolves and hydrates entitlement values. |
service/policy/db/attribute_fqn.go |
Uses the optimized resolver. |
service/integration/attributes_test.go |
Tests normalization, hierarchy, traversal, and inactive values. |
Files not reviewed (1)
- service/policy/db/entitleable_attributes.sql.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@jakedoublev I am comfortable with this pr. |
GetEntitleableAttributesByFqnscurrently wraps the broad attribute lookup and discards most of its result. Give it a dedicated SQL path for requested values, rule and namespace identity, and ordered active hierarchy values. Keep the targeted subject-mapping query. The attribute query reads no grants, keys, or resource mappings.Preserve missing/inactive-value errors, normalization, hierarchy order, and traversal. No API or migration change.
With the merged scale fixture, this PR alone reduced p95 at 50 concurrent requests from 8.72 seconds to 180 ms. All 800 requests passed on each implementation. Caching and experimental features were disabled. Comparison details: #3991.
Layer 1 of 6, rebased on
mainatf2635158with #3983's tests. Next: #3987.Verified: PostgreSQL integration tests, service race suite, SQL generation, formatting, and lint for the stack diff. Full repository check limitations: #3991.
Summary by CodeRabbit