diff --git a/docs/source/spec/core.rst b/docs/source/spec/core.rst index 0ed490bed9f..22b8cdcc9ef 100644 --- a/docs/source/spec/core.rst +++ b/docs/source/spec/core.rst @@ -415,8 +415,7 @@ list The :dfn:`list` type represents a homogeneous collection of values. A list is defined using a :token:`list_statement`. A list statement consists of the shape named followed by an object with a single key-value pair of "member" -that defines the :ref:`member ` of the list. The member of a list -cannot target the containing list shape. +that defines the :ref:`member ` of the list. The following example defines a list with a string member from the :ref:`prelude `: @@ -514,8 +513,7 @@ set The :dfn:`set` type represents an unordered collection of unique homogeneous values. A set is defined using a :token:`set_statement` that consists of the shape named followed by an object with a single key-value pair of "member" -that defines the :ref:`member ` of the set. The member of a set -cannot target the containing set shape. +that defines the :ref:`member ` of the set. The following example defines a set of strings: @@ -947,6 +945,82 @@ with the box trait. Members that target strings, timestamps, and aggregate shapes are always considered boxed and have no default values. +Recursive shape definitions +``````````````````````````` + +Smithy allows for recursive shape definitions with the following constraint: +the member of a list, set, or map cannot directly or transitively target its +containing shape unless one or more members in the path from the container +back to itself targets a structure or union shape. This ensures 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 ``ListThis check removes an entire class of problems from things like + * code generators where a list of itself or a list of maps of itself + * is impossible to define. + */ +public class ShapeRecursionValidator extends AbstractValidator { + + @Override + public List validate(Model model) { + ShapeIndex index = model.getShapeIndex(); + return index.shapes() + .map(shape -> validateShape(index, shape)) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + } + + private ValidationEvent validateShape(ShapeIndex index, Shape shape) { + return new RecursiveNeighborVisitor(index, shape).visit(shape); + } + + private final class RecursiveNeighborVisitor extends ShapeVisitor.Default { + + private final ShapeIndex index; + private final Shape root; + private final Set visited = new HashSet<>(); + private final Deque context = new ArrayDeque<>(); + + RecursiveNeighborVisitor(ShapeIndex index, Shape root) { + this.root = root; + this.index = index; + } + + ValidationEvent visit(Shape shape) { + ValidationEvent event = hasShapeBeenVisited(shape); + return event != null ? event : shape.accept(this); + } + + private ValidationEvent hasShapeBeenVisited(Shape shape) { + if (!visited.contains(shape.getId())) { + return null; + } + + return error(shape, String.format( + "Found invalid shape recursion: %s. A recursive list, set, or map shape is only valid if " + + "an intermediate reference is through a union or structure.", + String.join(" > ", context))); + } + + @Override + protected ValidationEvent getDefault(Shape shape) { + return null; + } + + @Override + public ValidationEvent listShape(ListShape shape) { + return validateMember(shape, shape.getMember()); + } + + @Override + public ValidationEvent setShape(SetShape shape) { + return validateMember(shape, shape.getMember()); + } + + @Override + public ValidationEvent mapShape(MapShape shape) { + return validateMember(shape, shape.getValue()); + } + + private ValidationEvent validateMember(Shape container, MemberShape member) { + ValidationEvent event = null; + Shape target = index.getShape(member.getTarget()).orElse(null); + + if (target != null) { + // Add to the visited set and the context deque before visiting, + // the remove from them after done visiting this shape. + visited.add(container.getId()); + // Eventually, this would look like: member-id > shape-id[ > member-id > shape-id [ > [...]] + context.addLast(member.getId().toString()); + context.addLast(member.getTarget().toString()); + event = visit(target); + context.removeLast(); + context.removeLast(); + visited.remove(container.getId()); + } + + return event; + } + } +} diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java index ffafc7857cf..ff7da0c4399 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/TargetValidator.java @@ -47,7 +47,6 @@ public class TargetValidator extends AbstractValidator { private static final Set INVALID_MEMBER_TARGETS = SetUtils.of( ShapeType.SERVICE, ShapeType.RESOURCE, ShapeType.OPERATION, ShapeType.MEMBER); - private static final Set INVALID_RECURSIVE_MEMBER_TARGETS = SetUtils.of(ShapeType.LIST, ShapeType.SET); @Override public List validate(Model model) { @@ -85,10 +84,6 @@ private Optional validateTarget(ShapeIndex index, Shape shape, if (INVALID_MEMBER_TARGETS.contains(target.getType())) { return Optional.of(error(shape, format( "Members cannot target %s shapes, but found %s", target.getType(), target))); - } else if (target.getId().equals(shape.getId().withoutMember()) - && INVALID_RECURSIVE_MEMBER_TARGETS.contains(target.getType())) { - return Optional.of(error(shape, format( - "%s shape members cannot target their container", target.getType()))); } case MAP_KEY: return target.asMemberShape().flatMap(m -> validateMapKey(shape, m.getTarget(), index)); diff --git a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator index 0b6648908c7..1ec659528aa 100644 --- a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator +++ b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator @@ -25,6 +25,7 @@ software.amazon.smithy.model.validation.validators.ResourceIdentifierValidator software.amazon.smithy.model.validation.validators.ResourceLifecycleValidator software.amazon.smithy.model.validation.validators.ServiceValidator software.amazon.smithy.model.validation.validators.ShapeIdConflictValidator +software.amazon.smithy.model.validation.validators.ShapeRecursionValidator software.amazon.smithy.model.validation.validators.SingleOperationBindingValidator software.amazon.smithy.model.validation.validators.SingleResourceBindingValidator software.amazon.smithy.model.validation.validators.TargetValidator diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion.errors new file mode 100644 index 00000000000..9ced1367a7c --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion.errors @@ -0,0 +1,12 @@ +[ERROR] ns.foo#IndirectRecursiveList: Found invalid shape recursion: ns.foo#IndirectRecursiveList$member > ns.foo#IndirectRecursiveListIntermediate1 > ns.foo#IndirectRecursiveListIntermediate1$member > ns.foo#IndirectRecursiveListIntermediate2 > ns.foo#IndirectRecursiveListIntermediate2$member > ns.foo#IndirectRecursiveList. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#IndirectRecursiveListIntermediate1: Found invalid shape recursion: ns.foo#IndirectRecursiveListIntermediate1$member > ns.foo#IndirectRecursiveListIntermediate2 > ns.foo#IndirectRecursiveListIntermediate2$member > ns.foo#IndirectRecursiveList > ns.foo#IndirectRecursiveList$member > ns.foo#IndirectRecursiveListIntermediate1. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#IndirectRecursiveListIntermediate2: Found invalid shape recursion: ns.foo#IndirectRecursiveListIntermediate2$member > ns.foo#IndirectRecursiveList > ns.foo#IndirectRecursiveList$member > ns.foo#IndirectRecursiveListIntermediate1 > ns.foo#IndirectRecursiveListIntermediate1$member > ns.foo#IndirectRecursiveListIntermediate2. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#IndirectRecursiveMap: Found invalid shape recursion: ns.foo#IndirectRecursiveMap$value > ns.foo#IndirectRecursiveMapIntermediate1 > ns.foo#IndirectRecursiveMapIntermediate1$value > ns.foo#IndirectRecursiveMapIntermediate2 > ns.foo#IndirectRecursiveMapIntermediate2$value > ns.foo#IndirectRecursiveMap. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#IndirectRecursiveMapIntermediate1: Found invalid shape recursion: ns.foo#IndirectRecursiveMapIntermediate1$value > ns.foo#IndirectRecursiveMapIntermediate2 > ns.foo#IndirectRecursiveMapIntermediate2$value > ns.foo#IndirectRecursiveMap > ns.foo#IndirectRecursiveMap$value > ns.foo#IndirectRecursiveMapIntermediate1. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#IndirectRecursiveMapIntermediate2: Found invalid shape recursion: ns.foo#IndirectRecursiveMapIntermediate2$value > ns.foo#IndirectRecursiveMap > ns.foo#IndirectRecursiveMap$value > ns.foo#IndirectRecursiveMapIntermediate1 > ns.foo#IndirectRecursiveMapIntermediate1$value > ns.foo#IndirectRecursiveMapIntermediate2. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#IndirectRecursiveSet: Found invalid shape recursion: ns.foo#IndirectRecursiveSet$member > ns.foo#IndirectRecursiveSetIntermediate1 > ns.foo#IndirectRecursiveSetIntermediate1$member > ns.foo#IndirectRecursiveSetIntermediate2 > ns.foo#IndirectRecursiveSetIntermediate2$member > ns.foo#IndirectRecursiveSet. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#IndirectRecursiveSetIntermediate1: Found invalid shape recursion: ns.foo#IndirectRecursiveSetIntermediate1$member > ns.foo#IndirectRecursiveSetIntermediate2 > ns.foo#IndirectRecursiveSetIntermediate2$member > ns.foo#IndirectRecursiveSet > ns.foo#IndirectRecursiveSet$member > ns.foo#IndirectRecursiveSetIntermediate1. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#IndirectRecursiveSetIntermediate2: Found invalid shape recursion: ns.foo#IndirectRecursiveSetIntermediate2$member > ns.foo#IndirectRecursiveSet > ns.foo#IndirectRecursiveSet$member > ns.foo#IndirectRecursiveSetIntermediate1 > ns.foo#IndirectRecursiveSetIntermediate1$member > ns.foo#IndirectRecursiveSetIntermediate2. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#InvalidDirectlyRecursiveList: Found invalid shape recursion: ns.foo#InvalidDirectlyRecursiveList$member > ns.foo#InvalidDirectlyRecursiveList. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#InvalidDirectlyRecursiveMap: Found invalid shape recursion: ns.foo#InvalidDirectlyRecursiveMap$value > ns.foo#InvalidDirectlyRecursiveMap. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion +[ERROR] ns.foo#InvalidDirectlyRecursiveSet: Found invalid shape recursion: ns.foo#InvalidDirectlyRecursiveSet$member > ns.foo#InvalidDirectlyRecursiveSet. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion.json b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion.json new file mode 100644 index 00000000000..6c55d8d3b41 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion.json @@ -0,0 +1,112 @@ +{ + "smithy": "0.4.0", + "ns.foo": { + "shapes": { + "InvalidDirectlyRecursiveList": { + "type": "list", + "member": { + "target": "InvalidDirectlyRecursiveList" + } + }, + "InvalidDirectlyRecursiveSet": { + "type": "set", + "member": { + "target": "InvalidDirectlyRecursiveSet" + } + }, + "InvalidDirectlyRecursiveMap": { + "type": "map", + "key": { + "target": "String" + }, + "value": { + "target": "InvalidDirectlyRecursiveMap" + } + }, + + "IndirectRecursiveList": { + "type": "list", + "member": { + "target": "IndirectRecursiveListIntermediate1" + } + }, + "IndirectRecursiveListIntermediate1": { + "type": "list", + "member": { + "target": "IndirectRecursiveListIntermediate2" + } + }, + "IndirectRecursiveListIntermediate2": { + "type": "list", + "member": { + "target": "IndirectRecursiveList" + } + }, + + "IndirectRecursiveSet": { + "type": "set", + "member": { + "target": "IndirectRecursiveSetIntermediate1" + } + }, + "IndirectRecursiveSetIntermediate1": { + "type": "set", + "member": { + "target": "IndirectRecursiveSetIntermediate2" + } + }, + "IndirectRecursiveSetIntermediate2": { + "type": "set", + "member": { + "target": "IndirectRecursiveSet" + } + }, + + "IndirectRecursiveMap": { + "type": "map", + "key": { + "target": "String" + }, + "value": { + "target": "IndirectRecursiveMapIntermediate1" + } + }, + "IndirectRecursiveMapIntermediate1": { + "type": "map", + "key": { + "target": "String" + }, + "value": { + "target": "IndirectRecursiveMapIntermediate2" + } + }, + "IndirectRecursiveMapIntermediate2": { + "type": "map", + "key": { + "target": "String" + }, + "value": { + "target": "IndirectRecursiveMap" + } + }, + + "ValidRecursiveShape": { + "type": "map", + "key": { + "target": "String" + }, + "value": { + "target": "ValidRecursiveShapeStruct" + } + }, + "ValidRecursiveShapeStruct": { + "type": "structure", + "members": { + "foo": { + "target": "ValidRecursiveShape" + } + } + } + } + } +} diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.errors index 4c5f4b53980..801a679c06f 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.errors @@ -8,7 +8,6 @@ [ERROR] ns.foo#InvalidListMemberReference$member: member shape targets an unresolved shape `ns.foo#NotFound` | Target [ERROR] ns.foo#InvalidListMemberResource$member: Members cannot target resource shapes, but found (resource: `ns.foo#MyResource`) | Target [ERROR] ns.foo#InvalidListMemberService$member: Members cannot target service shapes, but found (service: `ns.foo#MyService`) | Target -[ERROR] ns.foo#InvalidRecursiveList$member: list shape members cannot target their container | Target [ERROR] ns.foo#InvalidMapType: Map key member targets (structure: `ns.foo#ValidInput`), but is expected to target a string | Target [ERROR] ns.foo#InvalidOperationBadErrorTraits: Operation error targets an invalid structure, `ns.foo#ValidInput`, that is not marked with the `error` trait. | Target [ERROR] ns.foo#InvalidOperationBadErrorTraits: Operation input targets an invalid structure `ns.foo#ValidError` that is marked with the `error` trait. | Target diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.json b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.json index c3b1dd50d64..44250ce0f81 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.json @@ -32,12 +32,6 @@ "target": "MyService" } }, - "InvalidRecursiveList": { - "type": "list", - "member": { - "target": "InvalidRecursiveList" - } - }, "MyService": { "type": "service", "version": "2017-01-17",