Fix allof with single ref#274
Fix allof with single ref#274Jacoby6000 wants to merge 17 commits intodisneystreaming:mainfrom Jacoby6000:fix-allof-single
Conversation
| private def removeSelfReferentialMixin(definition: Definition): Definition = | ||
| definition match { | ||
| case struct @ Structure(id, _, _, hints) => | ||
| struct.copy(hints = hints.filter { | ||
| case Hint.HasMixin(`id`) => false | ||
| case _ => true | ||
| }) | ||
| case other => other | ||
| } |
There was a problem hiding this comment.
The downward propagation of hints led to scenarios in complex cases where a mixin would reference itself, causing a cyclical dependency and breaking the smithy spec. I still need to figure out a minimum repro for this so I can write a test case for it.
There was a problem hiding this comment.
Done in e8971ac, but that uncovered a new issue as well
| test("unions - Parent type explicitly referenced and extended".only) { | ||
| val jsonSchString = | ||
| """|{ | ||
| | "$id": "test.json", | ||
| | "$schema": "http://json-schema.org/draft-07/schema#", | ||
| | "title": "Test", | ||
| | "type" : "object", | ||
| | "properties" : { | ||
| | "root": { "$ref": "#/$defs/root" } | ||
| | }, | ||
| | "$defs": { | ||
| | "root": { | ||
| | "type": "object", | ||
| | "properties": { | ||
| | "s": { | ||
| | "type": "string" | ||
| | } | ||
| | } | ||
| | }, | ||
| | "subB": { | ||
| | "type": "object", | ||
| | "allOf": [ | ||
| | { "$ref": "#/$defs/root" }, | ||
| | { "type": "object", "properties": { "x": { "type": "integer" } } } | ||
| | ] | ||
| | }, | ||
| | "subA": { | ||
| | "type": "object", | ||
| | "allOf": [ | ||
| | { "$ref": "#/$defs/root" }, | ||
| | { "type": "object", "properties": { "x": { "type": "integer" } } } | ||
| | ] | ||
| | } | ||
| | } | ||
| |} | ||
| |""".stripMargin | ||
|
|
||
| val expectedString = """|namespace foo | ||
| | | ||
| |structure Root with [RootMixin] {} | ||
| | | ||
| |@mixin | ||
| |structure RootMixin { | ||
| | s: String | ||
| |} | ||
| | | ||
| |structure SubA with [RootMixin] { | ||
| | x: Integer | ||
| |} | ||
| | | ||
| |structure SubB with [RootMixin] { | ||
| | x: Integer | ||
| |} | ||
| | | ||
| |structure Test { | ||
| | root: Root | ||
| |} | ||
| | | ||
| |""".stripMargin | ||
|
|
||
| TestUtils.runConversionTest(jsonSchString, expectedString) | ||
| } |
There was a problem hiding this comment.
This test is in-line with what we would expect from current behavior, but it opens up some new issues.
- This test currently fails. The parent type (
RootMixin) is not having any field members generated. It may be that my original solution is insufficient in this case, given that this was a symptom of that problem as well. There may be a better alternative - Should the
Teststructure actually be referencingRootMixin? In the JsonSchema I am looking at, the parent type is referenced with the expectation that any sub-type can take its place. The current implementation does not allow that, becauseSubAandSubBare sub-types ofRootMixin, and notRoot.
There was a problem hiding this comment.
Looking at the test case above, I don't see anything in the expectedString smithy that seems wrong or out of place to me, perhaps you can provide an example of what you would think it should be as opposed to the current and that will help me understand what you mean
There was a problem hiding this comment.
I believe I've fixed this in 4cd9f00, also added another test case to make sure it still worked in the face of multiple mixins. Honestly I feel like there are very likely still edge cases we aren't handling properly, but let me know if this gets you unblocked on this specific issue at least
|
A few unrelated tests are failing now, I think due to some removals happening that shouldn't (e.g. things aren't marked top-level when they should be). I also will do some more cleanup because the AllOfTransform has gotten somewhat out of hand again as I handled the document cases for the openapi side and realized I needed to do some more transitive checks. Can simplify it a bit. |
|
Oops. Accidentally deleted my fork when I was cleaning some stuff up 😅 I'll push things back up shortly |
|
|
2 participants
An
allOfwith a single reference would fail to generate both the mixin trait and reference to that mixin, and the relevant fields would not be generated. This solution causes the mixin to be generated as expected.Another way to do this may be to just inline the fields from the reference, but I think this makes more sense, given that
allOfwith a single ref signals some intent to eventually extend the schema, and keeping it this way improves smithy source compatibility after the json schema changes.