feat(policy): support inline obligation triggers on attribute value create#3432
Merged
jakedoublev merged 14 commits intomainfrom May 5, 2026
Merged
Conversation
…reate Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
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 support for inline obligation triggers when creating attribute values. By allowing these triggers to be defined at the time of creation, the system ensures that the necessary associations are established atomically, improving both the developer experience and the reliability of policy enforcement configurations. 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. A value created with care, Obligations now linked in the air. Atomic and swift, A transactional gift, To keep all our policies fair. Footnotes
|
github-actions
Bot
added
comp:db
📝 WalkthroughWalkthroughThis PR introduces inline obligation-trigger creation during attribute-value creation via a new ChangesObligation Trigger Cross-Namespace Support
Sequence DiagramsequenceDiagram
participant Client
participant AttrValueSvc as AttributeValue Service
participant TriggerSvc as ObligationTrigger Service
participant DB as Database
Client->>AttrValueSvc: CreateAttributeValue(name, obligations=[])
Note over Client,AttrValueSvc: obligations contain obligation_triggers
AttrValueSvc->>DB: Upsert AttributeValue FQN
DB-->>AttrValueSvc: Created ID
loop For each obligation_trigger
AttrValueSvc->>TriggerSvc: CreateObligationTrigger(attr_value_id, obligation_value, action)
Note over TriggerSvc: Derive trigger_namespace from attribute_value
TriggerSvc->>DB: Resolve action in trigger_namespace
DB-->>TriggerSvc: Action ID
TriggerSvc->>DB: Validate action namespace == trigger_namespace
TriggerSvc->>DB: Insert trigger with derived namespace
DB-->>TriggerSvc: Trigger created
TriggerSvc-->>AttrValueSvc: Trigger result
end
AttrValueSvc->>DB: GetAttributeValue(ID)
Note over DB: Join attribute_definitions and attribute_namespaces<br/>Enrich triggers with namespace from attribute_value
DB-->>AttrValueSvc: AttributeValue with triggers [namespace fields]
AttrValueSvc-->>Client: Created AttributeValue with obligations
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 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 |
![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 introduces the capability to create obligation triggers inline during the creation of an attribute value. The changes include updates to the Protobuf definitions, documentation, and database logic to handle these triggers within a transaction, along with new integration and unit tests. Feedback suggests adding a metadata field to the trigger request for consistency and optimizing the database loop used for trigger creation to improve performance.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/grpc/index.html`:
- Around line 8391-8397: Update the documentation for the obligation_triggers
field (AttributeValueObligationTriggerRequest) to state its transactional
semantics: explicitly note that creation of obligation_triggers is atomic with
the attribute value creation (all-or-nothing), so either the attribute value and
all provided obligation_triggers are created together or none are created;
ensure the wording appears alongside the existing description for
obligation_triggers so clients don’t assume partial success.
In `@docs/openapi/authorization/authorization.openapi.yaml`:
- Around line 96-98: The documented numeric bounds for
google.protobuf.Timestamp.seconds are incorrect; update the description for
Timestamp.seconds to use the canonical epoch-second min and max values: set the
minimum to -62135596800 and the maximum to 253402300799 (corresponding to
0001-01-01T00:00:00Z and 9999-12-31T23:59:59Z respectively) wherever the old
±315576000000 bounds appear (e.g., in the Timestamp.seconds description entries
for google.protobuf.Timestamp).
- Around line 112-116: Update the two duplicated descriptions that refer to
createdAt.nanos to use "timestamp" terminology instead of "duration": locate the
description text for the field named createdAt.nanos and replace phrases like
"duration" and "the nanosecond portion of the duration" with wording that
clarifies this is the nanosecond component of a timestamp (e.g., "the nanosecond
portion of the timestamp, not an alternative to seconds"), ensure the note about
negative seconds and valid range (0 to 999,999,999) remains intact, and apply
the same change to both occurrences of createdAt.nanos so both descriptions
match.
In `@service/policy/db/attribute_values.go`:
- Around line 61-71: The loop creating obligation triggers currently sets
AttributeValue to &common.IdFqnIdentifier{Id: createdID} and omits the computed
FQN returned by upsertAttributeValueFqn; update the code to capture the FQN from
the upsertAttributeValueFqn result (the []upsertAttributeValueFqnRow returned
earlier) and pass it into CreateObligationTrigger by populating
AttributeValue.Fqn (i.e., use &common.IdFqnIdentifier{Id: createdID, Fqn:
<capturedFqn>}) so CreateObligationTrigger (and its SQL using
pgtypeText(r.GetAttributeValue().GetFqn())) receives the FQN fallback value.
🪄 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: db35657f-4386-45c2-9555-3ea6fa049f8b
⛔ Files ignored due to path filters (3)
protocol/go/authorization/authorization.pb.gois excluded by!**/*.pb.goprotocol/go/policy/attributes/attributes.pb.gois excluded by!**/*.pb.goprotocol/go/policy/objects.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamlservice/integration/attribute_values_test.goservice/policy/attributes/attributes.protoservice/policy/attributes/attributes_test.goservice/policy/db/attribute_values.go
💤 Files with no reviewable changes (2)
- docs/openapi/policy/objects.openapi.yaml
- docs/openapi/policy/subjectmapping/subject_mapping.openapi.yaml
Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Contributor
X-Test Failure Report✅ java-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
…ation Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
…ibute-value-obligation-triggers
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
X-Test Failure Reportcukes-report |
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
Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
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
…ibute-value-obligation-triggers
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
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:
|
…ous SQL joins Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Contributor
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: jakedoublev <jake.vanvorhis@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
service/policy/db/obligations.go (1)
775-792: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the direct namespace lookup instead of hydrating two full resources.
getAttributeValueNamespaceIDdoes aGetAttributeValueand then aGetAttributejust to recover one UUID. On the new inline-trigger path, that turns multi-trigger creates into N× extra DB round-trips even thoughgetAttributeValueNamespaceIDsalready returnsad.namespace_iddirectly. Query that helper here, or add a single-row variant, soCreateObligationTriggerstays cheap on bulk inline trigger creation.🤖 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 `@service/policy/db/obligations.go` around lines 775 - 792, getAttributeValueNamespaceID currently hydrates two full resources via GetAttributeValue and GetAttribute to return a single namespace id, causing extra DB round-trips; change it to use the existing helper that returns ad.namespace_id directly (getAttributeValueNamespaceIDs) or add a new single-row helper (e.g., GetAttributeValueNamespaceIDDirect) and call that from getAttributeValueNamespaceID so CreateObligationTrigger no longer does N× extra queries—replace the GetAttributeValue/GetAttribute calls with the direct namespace-id query and return its id (preserve the same error wrapping behavior using db.WrapIfKnownInvalidQueryErr).service/policy/db/queries/attribute_values.sql (1)
6-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope trigger aggregation by attribute value too.
obligation_triggers_aggstill collapses rows onot.obligation_value_idalone, sogetAttributeValuereuses the same trigger list for every attribute value that references that obligation value. With the newnamespacepayload, this will return other attribute values' source namespaces/triggers in the wrongGetAttributeValueresponse. Carryot.attribute_value_idthrough the trigger/value aggregations and join on it inattribute_obligationsso the nested trigger set stays scoped to the requested attribute value.🤖 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 `@service/policy/db/queries/attribute_values.sql` around lines 6 - 61, The trigger aggregation is currently grouped only by ot.obligation_value_id so triggers get shared across different attribute values; update obligation_triggers_agg to SELECT and GROUP BY ot.attribute_value_id as well (keep ot.obligation_value_id), propagate that attribute_value_id into obligation_values_agg (include it in the SELECT and GROUP BY alongside ov.obligation_definition_id), and then adjust the downstream join in attribute_obligations to join on both obligation_definition_id and attribute_value_id so each attribute value receives only its scoped triggers.service/policy/db/queries/obligations.sql (1)
731-769: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReuse
trigger_ns_fqnsfor the output FQN to avoid two sources of truth in this query.In
listObligationTriggersyou LEFT JOINattribute_fqns trigger_ns_fqns(line 765) and filter the result againsttrigger_ns_fqns.fqn(line 769), but the JSON output at line 734 builds the namespace FQN asCONCAT('https://', trigger_ns.name). The two should be equal in practice, but having one canonical-table value drive filtering and a hardcoded reconstruction drive the response is unnecessary divergence — any future change to FQN normalization (e.g., scheme, casing) would have to be updated in both places to stay consistent. Since the JOIN is already paid for here, prefer the canonical column for output as well.♻️ Suggested change
'attribute_value', JSON_BUILD_OBJECT( 'id', av.id, 'value', av.value, 'fqn', COALESCE(av_fqns.fqn, '') ), 'namespace', JSON_BUILD_OBJECT( 'id', trigger_ns.id, 'name', trigger_ns.name, - 'fqn', CONCAT('https://', trigger_ns.name) + 'fqn', COALESCE(trigger_ns_fqns.fqn, CONCAT('https://', trigger_ns.name)) ),Note: the other 7 trigger queries use
CONCAT(...)deliberately to avoid an extraLEFT JOIN attribute_fqnsper row (PR description: "remove extraneous joins"), so this suggestion is scoped tolistObligationTriggersonly, where the join is already present.🤖 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 `@service/policy/db/queries/obligations.sql` around lines 731 - 769, The JSON output for the trigger namespace FQN in listObligationTriggers currently constructs the FQN with CONCAT('https://', trigger_ns.name) inside the JSON_BUILD_OBJECT; instead use the canonical value returned by the existing LEFT JOIN (trigger_ns_fqns.fqn) so filtering and output come from the same source. Update the JSON_BUILD_OBJECT that builds 'fqn' (inside the 'namespace' object) to use trigger_ns_fqns.fqn (or COALESCE(trigger_ns_fqns.fqn, CONCAT(...)) if you want a fallback) instead of concatenating trigger_ns.name, leaving the JOIN on attribute_fqns trigger_ns_fqns and the WHERE condition unchanged.
♻️ Duplicate comments (1)
docs/grpc/index.html (1)
8405-8411:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRe-add transactional semantics for
obligation_triggersdocs.Line 8409 describes contents but not all-or-nothing behavior. This can still mislead clients into expecting partial success.
✏️ Suggested wording
- <td><p>Optional -Existing obligation values to trigger for the newly created attribute value. </p></td> + <td><p>Optional +Existing obligation values to trigger for the newly created attribute value. +Creation is transactional with attribute value creation; if any trigger fails validation or persistence, the request fails and no records are created. </p></td>🤖 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 `@docs/grpc/index.html` around lines 8405 - 8411, The documentation for the field obligation_triggers (AttributeValueObligationTriggerRequest) omits that processing is transactional/all-or-nothing; update the table cell describing obligation_triggers to explicitly state that the provided AttributeValueObligationTriggerRequest entries are applied atomically—either all obligation triggers succeed for the new attribute value or none are applied—and that on failure the operation is rolled back and an error is returned; mention this transactional behavior and error semantics in the same description text for obligation_triggers so clients do not assume partial success.
🤖 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 `@service/integration/attribute_values_test.go`:
- Around line 1247-1292: The test currently checks for an error and nil
createdValue but does not assert that no attribute value was persisted; after
the existing s.Require().ErrorIs(err, db.ErrNotFound) add a verification that no
attribute value exists for attrDef.GetId() with Value
"test_value_missing_obligation_fqn" (e.g. call the appropriate read/list API
such as PolicyClient.ListAttributeValues or
PolicyClient.GetAttributeValuesByAttributeId using s.ctx and attrDef.GetId(),
then assert the returned list does not contain that value or is empty) to ensure
the failed CreateAttributeValue (with inline obligation trigger) rolled back
completely.
---
Outside diff comments:
In `@service/policy/db/obligations.go`:
- Around line 775-792: getAttributeValueNamespaceID currently hydrates two full
resources via GetAttributeValue and GetAttribute to return a single namespace
id, causing extra DB round-trips; change it to use the existing helper that
returns ad.namespace_id directly (getAttributeValueNamespaceIDs) or add a new
single-row helper (e.g., GetAttributeValueNamespaceIDDirect) and call that from
getAttributeValueNamespaceID so CreateObligationTrigger no longer does N× extra
queries—replace the GetAttributeValue/GetAttribute calls with the direct
namespace-id query and return its id (preserve the same error wrapping behavior
using db.WrapIfKnownInvalidQueryErr).
In `@service/policy/db/queries/attribute_values.sql`:
- Around line 6-61: The trigger aggregation is currently grouped only by
ot.obligation_value_id so triggers get shared across different attribute values;
update obligation_triggers_agg to SELECT and GROUP BY ot.attribute_value_id as
well (keep ot.obligation_value_id), propagate that attribute_value_id into
obligation_values_agg (include it in the SELECT and GROUP BY alongside
ov.obligation_definition_id), and then adjust the downstream join in
attribute_obligations to join on both obligation_definition_id and
attribute_value_id so each attribute value receives only its scoped triggers.
In `@service/policy/db/queries/obligations.sql`:
- Around line 731-769: The JSON output for the trigger namespace FQN in
listObligationTriggers currently constructs the FQN with CONCAT('https://',
trigger_ns.name) inside the JSON_BUILD_OBJECT; instead use the canonical value
returned by the existing LEFT JOIN (trigger_ns_fqns.fqn) so filtering and output
come from the same source. Update the JSON_BUILD_OBJECT that builds 'fqn'
(inside the 'namespace' object) to use trigger_ns_fqns.fqn (or
COALESCE(trigger_ns_fqns.fqn, CONCAT(...)) if you want a fallback) instead of
concatenating trigger_ns.name, leaving the JOIN on attribute_fqns
trigger_ns_fqns and the WHERE condition unchanged.
---
Duplicate comments:
In `@docs/grpc/index.html`:
- Around line 8405-8411: The documentation for the field obligation_triggers
(AttributeValueObligationTriggerRequest) omits that processing is
transactional/all-or-nothing; update the table cell describing
obligation_triggers to explicitly state that the provided
AttributeValueObligationTriggerRequest entries are applied atomically—either all
obligation triggers succeed for the new attribute value or none are applied—and
that on failure the operation is rolled back and an error is returned; mention
this transactional behavior and error semantics in the same description text for
obligation_triggers so clients do not assume partial success.
🪄 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: a8572483-1b51-4b97-b9e8-650e54696b27
⛔ Files ignored due to path filters (3)
protocol/go/policy/attributes/attributes.pb.gois excluded by!**/*.pb.goprotocol/go/policy/objects.pb.gois excluded by!**/*.pb.goprotocol/go/policy/obligations/obligations.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
docs/grpc/index.htmldocs/openapi/policy/actions/actions.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/obligations/obligations.openapi.yamldocs/openapi/policy/registeredresources/registered_resources.openapi.yamldocs/openapi/policy/resourcemapping/resource_mapping.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamldocs/openapi/policy/unsafe/unsafe.openapi.yamlservice/integration/attribute_values_test.goservice/integration/obligation_triggers_test.goservice/policy/attributes/attributes.protoservice/policy/attributes/attributes_test.goservice/policy/db/attribute_values.goservice/policy/db/attribute_values.sql.goservice/policy/db/obligations.goservice/policy/db/obligations.sql.goservice/policy/db/queries/attribute_values.sqlservice/policy/db/queries/obligations.sqlservice/policy/objects.protoservice/policy/obligations/obligations.proto
Contributor
ryanulit
approved these changes
May 5, 2026
jakedoublev
deleted the
feat/DSPX-3160-inline-attribute-value-obligation-triggers
branch
This was referenced May 5, 2026
This was referenced May 6, 2026
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
2 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.
DSPX-3160
Summary
This updates obligation trigger creation so the trigger’s source namespace comes from the attribute value side, and the action must resolve/validate in that same namespace. That allows a trigger under one namespace to point at an obligation value in another namespace, including when triggers are created inline during CreateAttributeValue.
Changes
Summary by CodeRabbit
New Features
Documentation