Repository: smithy-lang/smithy (java)

Issue:
Loading via Smithy and Json produces different result
I have repro, [runnable via scastie](https://scastie.scala-lang.org/v0O4ihoLSXSOZHwuCjpPwg).

Given the following Smithy:

```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

Pull request:
Fix member mixin lists
#### Background
* What do these changes do? 

  Fixes two bugs regarding member shapes that have multiple mixins:

    1. Fixes a bug where mixed members that also applied local traits would drop all but the last mixin from their list of mixins.
    2. Fixes a bug where shape constructors attempting to fix missing mixins from member mixin lists would drop all but the last mixin from the list.

* Why are they important?

  Correctness is a virtue.

#### Testing
* How did you test these changes?

  The specific customer case was tested manually, unit tests derived from that were added.

#### Links
* Links to additional context, if necessary
* Issue #, if applicable (see [here](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword) for a list of keywords to use for linking issues)

  Fixes #2150

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Patch:
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ApplyMixin.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ApplyMixin.java
index fb6f6eaed..647f86b2b 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ApplyMixin.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ApplyMixin.java
@@ -99,22 +99,25 @@ final class ApplyMixin implements ShapeModifier {
                     }
                 }
             }
-            if (introducedMember == null && memberBuilders.containsKey(member.getMemberName())) {
-                MemberShape.Builder original = memberBuilders.get(member.getMemberName());
-                introducedMember = original.addMixin(member).build();
-                if (!introducedMember.getTarget().equals(member.getTarget())) {
-                    mixinMemberConflict(original, member);
+
+            if (introducedMember == null) {
+                if (memberBuilders.containsKey(member.getMemberName())) {
+                    MemberShape.Builder original = memberBuilders.get(member.getMemberName());
+                    introducedMember = original.addMixin(member).build();
+                    if (!introducedMember.getTarget().equals(member.getTarget())) {
+                        mixinMemberConflict(original, member);
+                    }
+                } else if (!introducedTraits.isEmpty()) {
+                    // Build local member copies before adding mixins if traits
+                    // were introduced to inherited mixin members.
+                    introducedMember = MemberShape.builder()
+                            .id(targetId)
+                            .target(member.getTarget())
+                            .source(member.getSourceLocation())
+                            .addTraits(introducedTraits.values())
+                            .addMixin(member)
+                            .build();
                 }
-            } else if (!introducedTraits.isEmpty()) {
-                // Build local member copies before adding mixins if traits
-                // were introduced to inherited mixin members.
-                introducedMember = MemberShape.builder()
-                        .id(targetId)
-                        .target(member.getTarget())
-                        .source(member.getSourceLocation())
-                        .addTraits(introducedTraits.values())
-                        .addMixin(member)
-                        .build();
             }
 
             if (introducedMember != null) {
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMemberUtils.java b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMemberUtils.java
index f9a775a46..3d2c5bfff 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMemberUtils.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMemberUtils.java
@@ -48,12 +48,12 @@ final class NamedMemberUtils {
                 String name = member.getMemberName();
                 if (members.get().containsKey(name)) {
                     MemberShape localMember = members.get().get(name);
-                    // Rebuild the member with the proper inherited mixin if needed.
+                    // Rebuild the member with the proper inherited mixins if needed.
                     // This catches errant cases where a member is added to a structure/union
                     // but omits the mixin members of parent shapes. Arguably, that's way too
                     // nuanced and error-prone to _not_ try to smooth over.
-                    if (localMember.getMixins().isEmpty() || !mixins.containsKey(member.getId())) {
-                        localMember = localMember.toBuilder().clearMixins().addMixin(member).build();
+                    if (localMember.getMixins().isEmpty() || !localMember.getMixins().contains(member.getId())) {
+                        localMember = rebuildMemberMixins(localMember, mixins.values());
                     }
                     computedMembers.put(name, localMember);
                 } else {
@@ -87,6 +87,14 @@ final class NamedMemberUtils {
         return Collections.unmodifiableMap(computedMembers);
     }
 
+    private static MemberShape rebuildMemberMixins(MemberShape member, Collection<Shape> mixins) {
+        MemberShape.Builder builder = member.toBuilder().clearMixins();
+        for (Shape mixin : mixins) {
+            mixin.getMember(member.getMemberName()).ifPresent(builder::addMixin);
+        }
+        return builder.build();
+    }
+
     static Set<MemberShape> flattenMixins(
             Map<String, MemberShape> members,
             Map<ShapeId, Shape> mixins,


Test patch:
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/IdlModelLoaderTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/IdlModelLoaderTest.java
index 9e8da5856..51767deef 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/IdlModelLoaderTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/IdlModelLoaderTest.java
@@ -491,4 +491,11 @@ public class IdlModelLoaderTest {
         assertThat(e.getMessage(),
                    startsWith("Syntax error at line 1, column 1: Parser exceeded maximum allowed depth of 64"));
     }
+
+    @Test
+    public void handlesMultipleMemberInheritance() {
+        ValidatedResult<Model> result = Model.assembler()
+                .addImport(getClass().getResource("error-recovery/to-dollar.smithy"))
+                .assemble();
+    }
 }
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java
index 890f4e7f0..4067b7066 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java
@@ -18,6 +18,7 @@ package software.amazon.smithy.model.loader;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.containsInRelativeOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
@@ -44,6 +45,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -77,6 +79,8 @@ import software.amazon.smithy.model.traits.DynamicTrait;
 import software.amazon.smithy.model.traits.InternalTrait;
 import software.amazon.smithy.model.traits.MediaTypeTrait;
 import software.amazon.smithy.model.traits.MixinTrait;
+import software.amazon.smithy.model.traits.PatternTrait;
+import software.amazon.smithy.model.traits.RequiredTrait;
 import software.amazon.smithy.model.traits.SensitiveTrait;
 import software.amazon.smithy.model.traits.SuppressTrait;
 import software.amazon.smithy.model.traits.TagsTrait;
@@ -1368,4 +1372,23 @@ public class ModelAssemblerTest {
                 .assemble()
                 .unwrap();
     }
+
+    @Test
+    public void handlesMultipleInheritanceForMixinMembers() {
+        Model model = Model.assembler()
+                .addImport(getClass().getResource("mixins/multiple-inheritance-with-introduction.smithy"))
+                .assemble()
+                .unwrap();
+
+        MemberShape shape = model.expectShape(ShapeId.from("com.example#FinalStructure$member"), MemberShape.class);
+
+        assertThat(shape.getMixins(), contains(
+                ShapeId.from("com.example#MixinA$member"),
+                ShapeId.from("com.example#MixinB$member")
+        ));
+        assertThat(shape.getAllTraits().keySet(),
+                containsInAnyOrder(PatternTrait.ID, RequiredTrait.ID, InternalTrait.ID));
+        String actualPattern = shape.expectTrait(PatternTrait.class).getValue();
+        assertThat(actualPattern, equalTo("baz"));
+    }
 }
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/shapes/StructureShapeTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/shapes/StructureShapeTest.java
index a85d68fd7..e9bcdc7e5 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/shapes/StructureShapeTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/shapes/StructureShapeTest.java
@@ -310,4 +310,30 @@ public class StructureShapeTest {
 
         assertThat(concrete.getMemberNames(), contains("a", "b", "c", "d"));
     }
+
+    @Test
+    public void fixesMissingMemberMixins() {
+        StringShape string = StringShape.builder().id("smithy.example#String").build();
+        StructureShape mixin1 = StructureShape.builder()
+                .id("smithy.example#Mixin1")
+                .addTrait(MixinTrait.builder().build())
+                .addMember("a", string.getId())
+                .build();
+        StructureShape mixin2 = StructureShape.builder()
+                .id("smithy.example#Mixin2")
+                .addTrait(MixinTrait.builder().build())
+                .addMember("a", string.getId())
+                .build();
+        StructureShape concrete = StructureShape.builder()
+                .id("smithy.example#Concrete")
+                .addMember("a", string.getId())
+                .addMixin(mixin1)
+                .addMixin(mixin2)
+                .build();
+
+        assertThat(concrete.getMember("a").get().getMixins(), contains(
+                ShapeId.from("smithy.example#Mixin1$a"),
+                ShapeId.from("smithy.example#Mixin2$a")
+        ));
+    }
 }
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/mixins/multiple-inheritance-with-introduction.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/mixins/multiple-inheritance-with-introduction.smithy
new file mode 100644
index 000000000..df7f64952
--- /dev/null
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/mixins/multiple-inheritance-with-introduction.smithy
@@ -0,0 +1,22 @@
+$version: "2"
+
+namespace com.example
+
+@mixin
+structure MixinA {
+    @pattern("foo")
+    @required
+    member: String
+}
+
+@mixin
+structure MixinB {
+    @pattern("bar")
+    @internal
+    member: String
+}
+
+structure FinalStructure with [MixinA, MixinB] {
+    @pattern("baz")
+    member: String
+}