diff --git a/docs/source/1.0/spec/core/model.rst b/docs/source/1.0/spec/core/model.rst index 6f0d09cea01..5c013f7e524 100644 --- a/docs/source/1.0/spec/core/model.rst +++ b/docs/source/1.0/spec/core/model.rst @@ -967,10 +967,12 @@ Smithy allows recursive shape definitions with the following limitations: that shapes that are typically impossible to define in various programming languages are not defined in Smithy models (for example, you can't define a recursive list in Java ``List` structure members. +3. To ensure a value can be provided for a union, recursive unions MUST + contain at least one path through its members that is not recursive + or steps through a list, set, map, or optional structure member. The following recursive shape definition is **valid**: diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ShapeRecursionValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ShapeRecursionValidator.java index 61175dff3cc..8a55c78d65e 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ShapeRecursionValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ShapeRecursionValidator.java @@ -17,7 +17,9 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.StringJoiner; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.selector.PathFinder; @@ -26,6 +28,7 @@ import software.amazon.smithy.model.shapes.MemberShape; import software.amazon.smithy.model.shapes.SetShape; import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShapeVisitor; import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.shapes.UnionShape; import software.amazon.smithy.model.traits.RequiredTrait; @@ -113,16 +116,75 @@ private String formatPath(PathFinder.Path path) { } private void validateUnions(Model model, List events) { - nextUnion: for (UnionShape union : model.getUnionShapes()) { - // Don't claim the union is recursive if it's empty (which is also invalid). - if (!union.getAllMembers().isEmpty()) { - for (MemberShape member : union.getAllMembers().values()) { - if (!member.getTarget().equals(member.getContainer())) { - continue nextUnion; + UnionTerminatesVisitor visitor = new UnionTerminatesVisitor(model); + for (UnionShape union : model.getUnionShapes()) { + // Don't evaluate empty unions since that's a different error. + if (!union.members().isEmpty() && !union.accept(visitor)) { + events.add(error(union, "It is impossible to create instances of this recursive union")); + } + visitor.reset(); + } + } + + private static final class UnionTerminatesVisitor extends ShapeVisitor.Default { + + private final Set visited = new HashSet<>(); + private final Model model; + + UnionTerminatesVisitor(Model model) { + this.model = model; + } + + void reset() { + this.visited.clear(); + } + + @Override + protected Boolean getDefault(Shape shape) { + return true; + } + + @Override + public Boolean structureShape(StructureShape shape) { + if (shape.members().isEmpty()) { + return true; + } + + // If the structure has any non-required members, then it terminates. + for (MemberShape member : shape.members()) { + if (!member.isRequired()) { + return true; + } + } + + // Now check if any of the required members terminate. + for (MemberShape member : shape.members()) { + if (member.isRequired()) { + if (memberShape(member)) { + return true; } } - events.add(error(union, "Unions must contain at least one member that is not directly recursive")); } + + return false; + } + + @Override + public Boolean unionShape(UnionShape shape) { + for (MemberShape member : shape.members()) { + if (memberShape(member)) { + return true; + } + } + + return false; + } + + @Override + public Boolean memberShape(MemberShape shape) { + return visited.add(shape) + ? model.expectShape(shape.getTarget()).accept(this) + : false; } } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-union.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-union.errors new file mode 100644 index 00000000000..749d16bcbc7 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-union.errors @@ -0,0 +1,4 @@ +[ERROR] smithy.example#RecursiveUnionA: It is impossible to create instances of this recursive union | ShapeRecursion +[ERROR] smithy.example#RecursiveUnionB: It is impossible to create instances of this recursive union | ShapeRecursion +[ERROR] smithy.example#RecursiveUnion: It is impossible to create instances of this recursive union | ShapeRecursion +[ERROR] smithy.example#RecursiveNext: It is impossible to create instances of this recursive union | ShapeRecursion diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-union.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-union.smithy new file mode 100644 index 00000000000..45a84a0415e --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-union.smithy @@ -0,0 +1,67 @@ +$version: "1.0" + +namespace smithy.example + +// This is impossible because there is no non-recursive branch along +// the recursive path back to itself. +union RecursiveUnionA { + b: RecursiveUnionB +} + +union RecursiveUnionB { + a: RecursiveUnionA +} + +// These unions are fine because there is a non-recursive branch along the +// recursive path back to itself: +// 1. {"e": {"str": "hi"}} +// 2. {"d": {"c": {"e": {"str": "hi"}}}} +union RecursiveUnionC { + d: RecursiveUnionD, + e: OkUnion +} + +union RecursiveUnionD { + c: RecursiveUnionC +} + +union OkUnion { + str: String +} + +// This is fine too because on the of the recursive branches contains a list +// which gives the type a size. +union RecursiveUnionE { + f: RecursiveUnionF +} + +union RecursiveUnionF { + e: EList +} + +list EList { + member: RecursiveUnionE +} + +// It's impossible to provide a value for this union. +union RecursiveUnion { + a: RecursiveUnion, + b: RecursiveUnion, +} + +union NotFullyRecursiveUnion { + a: RecursiveUnion, + b: RecursiveUnion, + c: String +} + +// This structure is invalid and the union is invalid +structure Start { + @required + foo: RecursiveNext +} + +union RecursiveNext { + a: Start, + b: Start +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/union-cant-be-fully-recursive.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/union-cant-be-fully-recursive.errors deleted file mode 100644 index eb2cc1b45ff..00000000000 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/union-cant-be-fully-recursive.errors +++ /dev/null @@ -1 +0,0 @@ -[ERROR] smithy.example#RecursiveUnion: Unions must contain at least one member that is not directly recursive | ShapeRecursion diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/union-cant-be-fully-recursive.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/union-cant-be-fully-recursive.smithy deleted file mode 100644 index dcc897c16e4..00000000000 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/union-cant-be-fully-recursive.smithy +++ /dev/null @@ -1,15 +0,0 @@ -$version: "1.0" - -namespace smithy.example - -// It's impossible to provide a value for this union. -union RecursiveUnion { - a: RecursiveUnion, - b: RecursiveUnion, -} - -union NotFullyRecursiveUnion { - a: RecursiveUnion, - b: RecursiveUnion, - c: String -} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.json b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.json index 6c07fd0823e..d435dbed7d2 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.json @@ -12,8 +12,8 @@ "com.foo#B": { "type": "union", "members": { - "a": { - "target": "com.foo#A" + "str": { + "target": "smithy.api#String" } } }, diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.smithy index 12d733e68ef..72aad05c1e6 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.smithy +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.smithy @@ -5,7 +5,7 @@ union A { } union B { - a: A + str: String } union C