Skip to content

Commit

Permalink
Fix bug where mixin cycles were incorrectly detected (#1628)
Browse files Browse the repository at this point in the history
* Fix bug where mixin cycles were incorrectly detected when traits were filtered out on operations using a mixin

* Remove added utility methods from Model

* Removed unused test dependencies
  • Loading branch information
hpmellema authored Feb 23, 2023
1 parent 43b13da commit e05b0fd
Show file tree
Hide file tree
Showing 4 changed files with 228 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.nio.file.Paths;
import java.util.Optional;
Expand All @@ -28,9 +29,15 @@
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.ListShape;
import software.amazon.smithy.model.shapes.MapShape;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.OperationShape;
import software.amazon.smithy.model.shapes.ResourceShape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.shapes.UnionShape;
import software.amazon.smithy.model.traits.TagsTrait;

public class ExcludeShapesByTagTest {
Expand Down Expand Up @@ -80,4 +87,46 @@ public void filtersMembers() throws Exception {
MemberShape baz = result.expectShape(ShapeId.from("smithy.example#StructForMixin$baz"), MemberShape.class);
assertFalse(baz.findMemberTrait(result, "MyTrait").isPresent());
}

@Test
public void classesWithMixinsFilteredWithoutCycleError() throws Exception {
Model model = Model.assembler()
.addImport(Paths.get(getClass().getResource("mixin-cycle-test.smithy").toURI()))
.assemble()
.unwrap();
TransformContext context = TransformContext.builder()
.model(model)
.settings(Node.objectNode().withMember("tags", Node.fromStrings("filter")))
.build();
Model result = new ExcludeShapesByTag().transform(context);

ResourceShape resourceShape = result.expectShape(ShapeId.from("smithy.example#ResourceWithMixin"), ResourceShape.class);
assertFalse(resourceShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(resourceShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

OperationShape operationShape = result.expectShape(ShapeId.from("smithy.example#OperationWithMixin"), OperationShape.class);
assertFalse(operationShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(operationShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

StructureShape structureShape = result.expectShape(ShapeId.from("smithy.example#StructureWithMixin"), StructureShape.class);
assertFalse(structureShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(structureShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

UnionShape unionShape = result.expectShape(ShapeId.from("smithy.example#UnionWithMixin"), UnionShape.class);
assertFalse(unionShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(unionShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

MapShape mapShape = result.expectShape(ShapeId.from("smithy.example#MapWithMixin"), MapShape.class);
assertFalse(mapShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(mapShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

ListShape listShape = result.expectShape(ShapeId.from("smithy.example#ListWithMixin"), ListShape.class);
assertFalse(listShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(listShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

StringShape stringShape = result.expectShape(ShapeId.from("smithy.example#StringWithMixin"), StringShape.class);
assertFalse(stringShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(stringShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,16 @@
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.ListShape;
import software.amazon.smithy.model.shapes.MapShape;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.OperationShape;
import software.amazon.smithy.model.shapes.ResourceShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StringShape;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.shapes.UnionShape;
import software.amazon.smithy.model.traits.TraitDefinition;

public class ExcludeTraitsByTagTest {
Expand Down Expand Up @@ -55,4 +62,52 @@ public void removesTraitsByTagInList() throws Exception {
assertFalse(mixedMember.findMemberTrait(result, "ns.foo#corge").isPresent());
assertTrue(mixedMember.findMemberTrait(result, "ns.foo#bar").isPresent());
}

@Test
public void classesWithMixinsFilteredWithoutCycleError() throws Exception {
Model model = Model.assembler()
.addImport(Paths.get(getClass().getResource("mixin-cycle-test.smithy").toURI()))
.assemble()
.unwrap();
TransformContext context = TransformContext.builder()
.model(model)
.settings(Node.objectNode().withMember("tags", Node.fromStrings("filter")))
.build();
Model result = new ExcludeTraitsByTag().transform(context);

Set<ShapeId> traits = result.getShapesWithTrait(TraitDefinition.class).stream()
.map(Shape::getId)
.collect(Collectors.toSet());

assertFalse(traits.contains(ShapeId.from("smithy.example#filteredTrait")));
assertTrue(traits.contains(ShapeId.from("smithy.example#unfilteredTrait")));

ResourceShape resourceShape = result.expectShape(ShapeId.from("smithy.example#ResourceWithMixin"), ResourceShape.class);
assertFalse(resourceShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(resourceShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

OperationShape operationShape = result.expectShape(ShapeId.from("smithy.example#OperationWithMixin"), OperationShape.class);
assertFalse(operationShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(operationShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

StructureShape structureShape = result.expectShape(ShapeId.from("smithy.example#StructureWithMixin"), StructureShape.class);
assertFalse(structureShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(structureShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

UnionShape unionShape = result.expectShape(ShapeId.from("smithy.example#UnionWithMixin"), UnionShape.class);
assertFalse(unionShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(unionShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

MapShape mapShape = result.expectShape(ShapeId.from("smithy.example#MapWithMixin"), MapShape.class);
assertFalse(mapShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(mapShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

ListShape listShape = result.expectShape(ShapeId.from("smithy.example#ListWithMixin"), ListShape.class);
assertFalse(listShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(listShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());

StringShape stringShape = result.expectShape(ShapeId.from("smithy.example#StringWithMixin"), StringShape.class);
assertFalse(stringShape.findMemberTrait(result, "smithy.example#filteredTrait").isPresent());
assertTrue(stringShape.findMemberTrait(result, "smithy.example#unfilteredTrait").isPresent());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
$version: "2.0"

namespace smithy.example

use smithy.example1#trait1

@trait
@tags(["filter"])
structure filteredTrait {}

@trait
@tags(["unfiltered"])
structure unfilteredTrait {}

@trait(selector: "resource")
structure resourceTrait {}

// RESOURCES
@filteredTrait
@unfilteredTrait
resource ResourceWithMixin with [ ResourceMixin ] {}

@mixin
@resourceTrait
resource ResourceMixin {}

// OPERATIONS
@filteredTrait
@unfilteredTrait
operation OperationWithMixin with [ OperationMixin ] {
input := {
@required
@httpLabel
myInputField: String,
other: String
}
}

@mixin
operation OperationMixin {
errors: [
ThrottlingException
ValidationException
]
}

@error("client")
structure ThrottlingException {
@required
message: String
}

@error("client")
structure ValidationException{
@required
message: String
}

// STRUCTURES
@filteredTrait
@unfilteredTrait
structure StructureWithMixin with [ StructureMixin ] {
data: String
}

@mixin
structure StructureMixin {
field: String
}

// UNIONS
@filteredTrait
@unfilteredTrait
union UnionWithMixin with [ UnionMixin ] {
data: String
}

@mixin
union UnionMixin {
field: Integer
}

// MAPS
@filteredTrait
@unfilteredTrait
map MapWithMixin with [MapMixin]{
key: String
value: String
}

@mixin
@length(min: 1, max: 2)
map MapMixin {
key: String
value: String
}

// LISTS
@filteredTrait
@unfilteredTrait
list ListWithMixin with [ListMixin]{
member: String
}

@mixin
@length(min: 1, max: 2)
list ListMixin {
member: String
}

// STRINGS
@filteredTrait
@unfilteredTrait
string StringWithMixin with [ StringMixin ]

@pattern("^$")
@mixin
string StringMixin

Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.loader.TopologicalShapeSort;
import software.amazon.smithy.model.shapes.AbstractShapeBuilder;
Expand Down Expand Up @@ -181,12 +180,11 @@ private void updateMixins(Model model, Model.Builder builder, List<Shape> replac
// Topologically sort the updated shapes to ensure shapes are updated in order.
TopologicalShapeSort sorter = new TopologicalShapeSort();

// Add structure and union shapes that are mixins or use mixins.
Stream.concat(model.shapes(StructureShape.class), model.shapes(UnionShape.class)).forEach(shape -> {
if (shape.hasTrait(MixinTrait.class) || !shape.getMixins().isEmpty()) {
sorter.enqueue(shape);
}
});
// Add shapes that are mixins or use mixins.
model.shapes()
.filter(shape -> !shape.isMemberShape())
.filter(shape -> shape.hasTrait(MixinTrait.class) || !shape.getMixins().isEmpty())
.forEach(sorter::enqueue);

// Add _all_ of the replacements in case mixins or the Mixin trait were removed from updated shapes.
for (Shape shape : replacements) {
Expand Down

0 comments on commit e05b0fd

Please sign in to comment.