feat(policy)!: undo subject mapping operator decomposition#3685
Conversation
The operator decomposition split SubjectMappingOperatorEnum into ConditionComparisonOperatorEnum + ConditionQuantifierEnum (+ case_insensitive) and rewired DynamicValueResolver onto the comparison enum. The decomposed evaluator was never adopted on main, so the added surface is unused complexity. Undo the decomposition while keeping the DynamicValueMapping feature: - Remove ConditionComparisonOperatorEnum and ConditionQuantifierEnum. - Remove the comparison, quantifier, and case_insensitive fields from Condition and restore Condition.operator to required. - DynamicValueResolver now uses SubjectMappingOperatorEnum. NOT_IN is rejected at the buf.validate boundary because dynamic resolution is existential. - Drop case_insensitive everywhere. - Remove the Comparison*/Quantifier* shorthand consts and regenerate protocol/go and the policy docs. BREAKING CHANGE: ConditionComparisonOperatorEnum and ConditionQuantifierEnum are removed; Condition.comparison/quantifier/case_insensitive are removed and operator is required again; DynamicValueResolver.comparison is replaced by operator (SubjectMappingOperatorEnum). Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
github-actions
Bot
added
comp:policy
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoves Condition operator consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 simplifies the policy evaluation logic by undoing the previously introduced decomposition of subject mapping operators. Since the decomposed evaluator was not adopted in the main codebase, this change removes the unnecessary complexity and restores the required operator field in the Condition message, ensuring a cleaner and more maintainable schema. 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 complex paths we tried to tread, / With extra enums, we were led. / But simple code is best to keep, / So back to basics, while we 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 simplifies policy conditions by removing the decomposed comparison, quantifier, and case_insensitive fields from both Condition and DynamicValueResolver messages, reverting back to using SubjectMappingOperatorEnum directly. The corresponding enums ConditionComparisonOperatorEnum and ConditionQuantifierEnum have been deleted, and the documentation has been updated accordingly. The reviewer suggests reserving field number 3 and the names comparison and case_insensitive in the DynamicValueResolver message to prevent future incompatible reuse, similar to what was done for the Condition message.
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:
|
![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 `@docs/openapi/policy/dynamicvaluemapping/dynamic_value_mapping.openapi.yaml`:
- Around line 635-644: The schema for operator currently excludes the wrong enum
value type, so update the `operator` definition in
`dynamic_value_mapping.openapi.yaml` to block the string value used by
`policy.SubjectMappingOperatorEnum` rather than the numeric placeholder. Adjust
the `operator` schema near the `policy.SubjectMappingOperatorEnum` reference so
`not.enum` matches the actual string token for
`SUBJECT_MAPPING_OPERATOR_ENUM_NOT_IN`, keeping the description and `$ref`
aligned with the enum definition.
In `@service/policy/objects.proto`:
- Around line 213-228: The DynamicValueResolver message removed field 3 without
reserving its tag/name, so update that message to reserve the deleted
case_insensitive field the same way Condition reserves removed fields. Add the
reservation in DynamicValueResolver alongside subject_external_selector_value
and operator so the old wire/JSON contract cannot be reused later.
🪄 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: eb32223c-a262-40d3-a3a3-df6154df3d92
⛔ Files ignored due to path filters (1)
protocol/go/policy/objects.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
docs/grpc/index.htmldocs/openapi/policy/actions/actions.openapi.yamldocs/openapi/policy/attributes/attributes.openapi.yamldocs/openapi/policy/dynamicvaluemapping/dynamic_value_mapping.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.yamlprotocol/go/internal/policy/enums.goprotocol/go/internal/policy/enums_test.goprotocol/go/policy/enums.gen.goservice/policy/objects.proto
💤 Files with no reviewable changes (3)
- protocol/go/internal/policy/enums_test.go
- protocol/go/policy/enums.gen.go
- protocol/go/internal/policy/enums.go
Remove the reserved field numbers/names added for the removed comparison, quantifier, and case_insensitive fields. The decomposed proto surface had no consumers, so reserving the numbers is unnecessary. Regenerated protocol/go. 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:
|
|
3 participants
Summary
Undo the subject mapping operator decomposition while keeping the
DynamicValueMappingfeature. The decomposed evaluator was never adopted onmain(EvaluateConditionstill switches onoperator), so the added enum andfield surface is unused complexity.
Changes
ConditionComparisonOperatorEnumandConditionQuantifierEnum.comparison,quantifier, andcase_insensitivefields fromCondition; restoreCondition.operatorto required. Reserve the removedfield numbers/names (4/5/6) to prevent reuse.
DynamicValueResolvernow usesSubjectMappingOperatorEnum operator.NOT_INis rejected at the
buf.validateboundary because dynamic resolution isexistential over the resolved entity values.
case_insensitiveeverywhere.Comparison*/Quantifier*shorthand consts; regenerateprotocol/goand the policy docs.Breaking Changes
ConditionComparisonOperatorEnum/ConditionQuantifierEnumare removed;Condition.comparison/quantifier/case_insensitiveare removed andoperatoris required again;DynamicValueResolver.comparisonis replaced byoperator. The DVM messages have no consumers yet, so breaking them (includingfield numbers) is acceptable.
Verification
buf lintclean;buf breakingflags only the intended removals.make proto-generateproduces a focused diff (no unrelated doc churn).protocol/go,service, andsdkmodules build;protocol/gotests pass.Follow-up
The
DSPX-2754dynamic attribute values implementation branch will be updatedto consume these reverted protos (operator-based DVM evaluation, DB column
rename) in a dependent change.
Summary by CodeRabbit
Breaking Changes
operatorfield instead of separate comparison, quantifier, and case-sensitivity options.NOT_INis not supported for dynamic resolution.Documentation