fix(go): handle proto direct assignments in DSL transform#621
Conversation
|
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughDirect-assignment usersets are now emitted with an explicit ChangesDirect assignment round-trip
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes the Go TransformJSONProtoToDSL transformer to correctly recognize direct assignments when the protobuf Userset oneof is set to Userset_This but the inner DirectUserset message is nil (a case that can occur for in-memory models produced by TransformModuleFilesToModel). It also ensures the DSL→proto transformer always populates the DirectUserset sentinel for direct-assignment rewrites, and adds round-trip coverage for modular models.
Changes:
- Detect direct assignments by checking the
Userset_Thisoneof case (instead ofGetThis() != nil). - Populate
DirectUsersetwhen creating direct-assignment rewrites in the DSL→proto transformer. - Add module round-trip tests for
TransformJSONProtoToDSLwith and without source information.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/go/transformer/jsontodsl.go | Adds isDirectAssignment and uses it throughout parsing/validation to correctly detect Userset_This even when DirectUserset is nil. |
| pkg/go/transformer/dsltojson.go | Ensures direct-assignment rewrites explicitly set Userset_This.This to an empty DirectUserset{} sentinel. |
| pkg/go/transformer/jsontodsl_test.go | Adds modular-model round-trip tests for TransformJSONProtoToDSL (with/without source info). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Detect direct assignments by the Userset_This oneof case instead of requiring GetThis to return a populated DirectUserset. Populate DirectUserset when transforming DSL to proto and cover module model round trips through TransformJSONProtoToDSL.
2953d54 to
7f33cfc
Compare
Description
Fixes
TransformJSONProtoToDSLfor in-memory authorization models produced byTransformModuleFilesToModelwhen relations contain directassignments.
What problem is being solved?
Direct assignments are represented by the
Userset_Thisprotobuf oneof case. The previous implementation detected direct assignments withGetThis() != nil, which can fail when the oneof case is set but the innerDirectUsersetsentinel is nil.This caused valid module models to fail when round-tripping through:
How is it being solved?
TransformJSONProtoToDSL now detects direct assignments by checking whether the userset is the Userset_This oneof case, instead of
requiring GetThis() to return a populated DirectUserset.
The DSL-to-proto transformer also now populates DirectUserset when creating direct-assignment rewrites.
What changes are made to solve it?
References
closes #620
Review Checklist
mainSummary by CodeRabbit
Bug Fixes
Tests