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

Fix replacing shapes when traits applied to mixed-in members #1509

Merged
merged 1 commit into from
Nov 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.junit.jupiter.api.Assertions.assertFalse;

import java.nio.file.Paths;
import java.util.Optional;
Expand All @@ -27,6 +28,7 @@
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.model.shapes.EnumShape;
import software.amazon.smithy.model.shapes.IntEnumShape;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.traits.TagsTrait;
Expand Down Expand Up @@ -73,5 +75,9 @@ public void filtersMembers() throws Exception {

IntEnumShape bar = result.expectShape(ShapeId.from("smithy.example#Bar"), IntEnumShape.class);
assertThat(bar.members().size(), is(1));

// Mixin members are retained, but excluded traits are removed.
MemberShape baz = result.expectShape(ShapeId.from("smithy.example#StructForMixin$baz"), MemberShape.class);
assertFalse(baz.findMemberTrait(result, "MyTrait").isPresent());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import software.amazon.smithy.build.TransformContext;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.traits.TraitDefinition;
Expand All @@ -47,5 +48,11 @@ public void removesTraitsByTagInList() throws Exception {

assertFalse(traits.contains(ShapeId.from("ns.foo#quux")));
assertTrue(traits.contains(ShapeId.from("ns.foo#bar")));

// Mixin members are retained, but tagged traits are excluded.
MemberShape mixedMember = result.expectShape(ShapeId.from("ns.foo#MyOperationInput$mixedMember"),
MemberShape.class);
assertFalse(mixedMember.findMemberTrait(result, "ns.foo#corge").isPresent());
assertTrue(mixedMember.findMemberTrait(result, "ns.foo#bar").isPresent());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,19 @@ intEnum Bar {
@enumValue(2)
KEEP
}

@trait
@tags(["filter"])
string MyTrait

@mixin
structure MyMixin {
@required
baz: String
}

structure StructForMixin with [MyMixin] {
bar: String
}

apply StructForMixin$baz @MyTrait("hello")
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,21 @@
]
}
},
"ns.foo#corge": {
"type": "structure",
"members": {
"member": {
"target": "ns.foo#CorgeTraitShapeMember"
}
},
"traits": {
"smithy.api#trait": {},
"smithy.api#tags": [
"foo",
"qux"
]
}
},
"ns.foo#MyService": {
"type": "service",
"version": "2017-01-19",
Expand All @@ -46,6 +61,22 @@
"target": "ns.foo#MyOperationInput"
}
},
"ns.foo#MyMixin": {
"type": "structure",
"members": {
"mixedMember": {
"target": "smithy.api#String",
"traits": {
"ns.foo#bar": {
"member": "baz"
}
}
}
},
"traits": {
"smithy.api#mixin": {}
}
},
"ns.foo#MyOperationInput": {
"type": "structure",
"members": {
Expand All @@ -55,7 +86,12 @@
"buzz": {
"target": "ns.foo#Include2"
}
}
},
"mixins": [
{
"target": "ns.foo#MyMixin"
}
]
},
"ns.foo#Exclude1": {
"type": "string",
Expand Down Expand Up @@ -84,6 +120,17 @@
},
"ns.foo#QuuxTraitShapeMember": {
"type": "string"
},
"ns.foo#CorgeTraitShapeMember": {
"type": "string"
},
"ns.foo#MyOperationInput$mixedMember": {
"type": "apply",
"traits": {
"ns.foo#corge": {
"member": "hi"
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
/**
* Topologically sorts shapes based on their dependencies (i.e., mixins).
*
* <p>While this class is reusable, is is also stateful; shapes and edges
* <p>While this class is reusable, it is also stateful; shapes and edges
* are enqueued, and when sorted, all shapes and edges are dequeued.
*/
public final class TopologicalShapeSort {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -189,7 +190,12 @@ private void updateMixins(Model model, Model.Builder builder, List<Shape> replac

// Add _all_ of the replacements in case mixins or the Mixin trait were removed from updated shapes.
for (Shape shape : replacements) {
sorter.enqueue(shape);
// Enqueue member shapes with empty dependencies since the parent will be enqueued with them.
if (shape.isMemberShape()) {
sorter.enqueue(shape.getId(), Collections.emptySet());
} else {
sorter.enqueue(shape);
}
}

List<ShapeId> sorted = sorter.dequeueSortedShapes();
Expand Down