feat(policy): DSPX-2754 DynamicValueMapping protos + generated code#3580
feat(policy): DSPX-2754 DynamicValueMapping protos + generated code#3580alkalescent wants to merge 2 commits into
Conversation
Protocol-first half of the dynamic attribute value entitlement work: adds the DynamicValueMapping / DynamicValueResolver messages and DynamicValueOperatorEnum to objects.proto, and a dedicated DynamicValueMappingService (policy.dynamicvaluemapping), plus regenerated protocol/go and OpenAPI/gRPC docs. No service implementation here. This lands and releases protocol/go first so the consumer PR (#3568) can require the new version and pass per-module 'go mod tidy'. Refs: DSPX-2754, DSPX-3498 Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. 📝 WalkthroughWalkthroughThis PR introduces the Dynamic Value Mapping feature, a new entitlement model enabling runtime resolution of resource value segments. It includes Protobuf domain objects and service contracts, complete OpenAPI documentation, generated Go Connect-RPC bindings, and minor formatting updates to existing specs and module configuration. ChangesDynamic Value Mapping Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 implements the protocol-first phase of the dynamic attribute value entitlement feature (DSPX-2754). By introducing new protobuf messages and a dedicated service, it establishes the foundation for resolving entitlement authority dynamically at decision time, moving away from static pre-provisioning. This change is isolated to the protocol layer to facilitate modular go module management and dependency resolution for future consumer implementations. 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. 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 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 values shift and flow in time, Dynamic logic, reason's climb. With proto structures clearly set, The policy engine's not done yet. Footnotes
|
github-actions
Bot
added
comp:policy
![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 introduces a new DynamicValueMappingService along with its CRUD operations and associated protobuf definitions, such as DynamicValueResolver and DynamicValueOperatorEnum, to support dynamic, definition-level value entitlement. The review feedback highlights several opportunities to improve validation in CreateDynamicValueMappingRequest, including ensuring consistent FQN validation, enforcing mutual exclusivity between namespace fields, and simplifying CEL expressions by leveraging standard validation rules.
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.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Address review: add namespace_id/namespace_fqn oneof, use min_len:1 + uri on FQN fields, and replace verbose CEL with direct uuid/uri rules (consistent with CreateSubjectMappingRequest; protovalidate skips unset oneof members). Refs: DSPX-2754, DSPX-3498 Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Mirror the proto validation fix from #3580 (namespace oneof, min_len:1 + uri on FQN fields, direct uuid/uri rules) so the consumer branch stays in sync. Refs: DSPX-2754, DSPX-3498 Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
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: 2
🤖 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/policy/dynamicvaluemapping/dynamic_value_mapping.proto`:
- Around line 41-42: The comment on the field namespace_id is
incorrect/misleading; update the comment for the field namespace_id (in the
dynamic value mapping proto) to remove the reference to Attribute Definition ID
and clearly state it is the Namespace ID to filter by (e.g., "Namespace ID to
filter by"), leaving the separate attribute_definition_id field’s own comment
unchanged so generated docs no longer conflate the two.
- Around line 72-75: The schema currently enforces exclusivity via
buf.validate.message.oneof (validation-only) for the fields namespace_id and
namespace_fqn, which doesn't get reflected in generated OpenAPI; change the
proto to make these mutually exclusive at the protobuf level by moving
namespace_id and namespace_fqn into a real oneof (e.g., oneof namespace_selector
{ string namespace_id = X; string namespace_fqn = Y; }) so the generated OpenAPI
includes the exclusivity, and apply the same change for the other occurrence
mentioned (lines ~107-114) where the buf.validate.message.oneof is used; keep
the field names namespace_id and namespace_fqn and ensure tags/types remain
consistent when creating the oneof.
🪄 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: 231eabb5-f6b3-45c3-8567-e28d9b8703af
⛔ Files ignored due to path filters (3)
protocol/go/policy/dynamicvaluemapping/dynamic_value_mapping.pb.gois excluded by!**/*.pb.goprotocol/go/policy/dynamicvaluemapping/dynamic_value_mapping_grpc.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/dynamicvaluemapping/dynamic_value_mapping.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamlprotocol/go/go.modprotocol/go/policy/dynamicvaluemapping/dynamicvaluemappingconnect/dynamic_value_mapping.connect.goservice/policy/dynamicvaluemapping/dynamic_value_mapping.protoservice/policy/objects.proto
| // Namespace ID, or Attribute Definition ID to filter by | ||
| string namespace_id = 1 [(buf.validate.field).cel = { |
There was a problem hiding this comment.
Fix the filter description typo.
The namespace_id comment currently says “Namespace ID, or Attribute Definition ID to filter by”, but attribute_definition_id is a separate field. This text propagates into generated docs and is misleading.
Suggested edit
- // Namespace ID, or Attribute Definition ID to filter by
+ // Namespace ID to filter by📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Namespace ID, or Attribute Definition ID to filter by | |
| string namespace_id = 1 [(buf.validate.field).cel = { | |
| // Namespace ID to filter by | |
| string namespace_id = 1 [(buf.validate.field).cel = { |
🤖 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/dynamicvaluemapping/dynamic_value_mapping.proto` around lines
41 - 42, The comment on the field namespace_id is incorrect/misleading; update
the comment for the field namespace_id (in the dynamic value mapping proto) to
remove the reference to Attribute Definition ID and clearly state it is the
Namespace ID to filter by (e.g., "Namespace ID to filter by"), leaving the
separate attribute_definition_id field’s own comment unchanged so generated docs
no longer conflate the two.
| option (buf.validate.message).oneof = { | ||
| fields: ["namespace_id", "namespace_fqn"] | ||
| required: false | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Promote namespace selector exclusivity into the schema contract (not validation-only).
namespace_id/namespace_fqn exclusivity is currently enforced via buf.validate.message.oneof, but the generated OpenAPI schema does not encode that constraint. This creates client/server contract drift: OpenAPI-based clients can send both fields and get runtime validation failures.
Proto-level approach to preserve exclusivity in downstream generated schemas
- // Optional: Namespace ID or FQN to scope the mapping to
- option (buf.validate.message).oneof = {
- fields: ["namespace_id", "namespace_fqn"]
- required: false
- };
@@
- // Optional: namespace ID or FQN for the mapping
- string namespace_id = 7 [(buf.validate.field).string.uuid = true];
- string namespace_fqn = 8 [
- (buf.validate.field).string = {
- min_len: 1
- uri: true
- }
- ];
+ // Optional: namespace ID or FQN for the mapping
+ oneof namespace_selector {
+ string namespace_id = 7 [(buf.validate.field).string.uuid = true];
+ string namespace_fqn = 8 [
+ (buf.validate.field).string = {
+ min_len: 1
+ uri: true
+ }
+ ];
+ }Based on learnings, docs/openapi/policy/*.openapi.yaml is auto-generated, so this should be fixed in proto and then regenerated (not patched directly in YAML).
Also applies to: 107-114
🤖 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/dynamicvaluemapping/dynamic_value_mapping.proto` around lines
72 - 75, The schema currently enforces exclusivity via
buf.validate.message.oneof (validation-only) for the fields namespace_id and
namespace_fqn, which doesn't get reflected in generated OpenAPI; change the
proto to make these mutually exclusive at the protobuf level by moving
namespace_id and namespace_fqn into a real oneof (e.g., oneof namespace_selector
{ string namespace_id = X; string namespace_fqn = Y; }) so the generated OpenAPI
includes the exclusivity, and apply the same change for the other occurrence
mentioned (lines ~107-114) where the buf.validate.message.oneof is used; keep
the field names namespace_id and namespace_fqn and ensure tags/types remain
consistent when creating the oneof.
1 participant
Proposed Changes
DynamicValueMapping/DynamicValueResolvermessages andDynamicValueOperatorEnumtoobjects.proto, plus a dedicatedDynamicValueMappingServicein a newpolicy.dynamicvaluemappingpackage. Includes regeneratedprotocol/goand OpenAPI/gRPC docs.Why split: the dedicated service introduces a new
protocol/gopackage. The per-modulego mod tidyCI check does not use the go workspace, so consumer modules cannot tidy against a package that is not yet in a releasedprotocol/go. Landing and releasingprotocol/gofirst lets the consumer PR require the new version and pass CI.Checklist
Testing Instructions
cd protocol/go && go build ./... && GOFLAGS=-mod=mod go mod tidy(clean)buf lint serviceRelated
Summary by CodeRabbit
New Features
Documentation