smithy-lang/smithy

Loading via Smithy and Json produces different result

Closed this issue · 5 comments

I have repro, runnable via scastie.

Given the following Smithy:

$version: "2.0"

namespace a_test

@mixin
structure A {
    @required
    aMember: String
}

@mixin
structure B {
    @required
    aMember: String
}

structure MyStruct with [A, B] {
}

When you load this model through a ModelAssembler and extract the mixins of a_test#MyStruct$aMember, you'll get:
[a_test#A$aMember, a_test#B$aMember], but if you serialize to json and then load it again, you'll get: [a_test#B$aMember].

I'd like to fix it, but I'm not sure which one is at fault. I'd think the json one is at fault, and I believe the faulty logic is in software.amazon.smithy.model.loader.ApplyMixin.

Let me know which one is wrong, and I can work on a fix

Hmm I can confirm. The following is the JSON that's produced.

{
    "smithy": "2.0",
    "shapes": {
        "a_test#A": {
            "type": "structure",
            "members": {
                "aMember": {
                    "target": "smithy.api#String",
                    "traits": {
                        "smithy.api#required": {}
                    }
                }
            },
            "traits": {
                "smithy.api#mixin": {}
            }
        },
        "a_test#B": {
            "type": "structure",
            "members": {
                "aMember": {
                    "target": "smithy.api#String",
                    "traits": {
                        "smithy.api#required": {}
                    }
                }
            },
            "traits": {
                "smithy.api#mixin": {}
            }
        },
        "a_test#MyStruct": {
            "type": "structure",
            "mixins": [
                {
                    "target": "a_test#A"
                },
                {
                    "target": "a_test#B"
                }
            ],
            "members": {}
        },
        "a_test#MyStruct$aMember": {
            "type": "apply",
            "traits": {
                "smithy.api#required": {}
            }
        }
    }
}

That apply seems out of place, but it's not necessarily wrong.

I found the problem. So that rogue apply statement is the thing that revealed the bug, though it isn't itself the real issue. This issue will surface for any mixin where multiple mixins affect the same member and a trait is apparently introduced by the final shape.

The problem is right here. This block will effectively overwrite the existing builder. There's a block above that which specifically wants to account for multiple mixins, but the result is being ignored in this case.

The saga continues. It seems there was some broad assumption that members wouldn't be touched by multiple mixins.

Thanks a bunch @JordonPhillips, I certainly did not realize the serialized model was invalid.

It's not invalid, semantically it is equivalent, it's just a bit strange. There's probably a good reason for doing that which I'm forgetting at the moment