Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loading via Smithy and Json produces different result #2150

Closed
daddykotex opened this issue Feb 20, 2024 · 5 comments · Fixed by #2157
Closed

Loading via Smithy and Json produces different result #2150

daddykotex opened this issue Feb 20, 2024 · 5 comments · Fixed by #2157
Labels
bug This issue is a bug.

Comments

@daddykotex
Copy link
Contributor

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

@JordonPhillips
Copy link
Contributor

JordonPhillips commented Feb 22, 2024

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.

@JordonPhillips JordonPhillips added the bug This issue is a bug. label Feb 22, 2024
@JordonPhillips
Copy link
Contributor

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.

@JordonPhillips
Copy link
Contributor

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

@daddykotex
Copy link
Contributor Author

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

@JordonPhillips
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants