diff --git a/config/spotbugs/filter.xml b/config/spotbugs/filter.xml index 9aa362726ab..083a10dd25c 100644 --- a/config/spotbugs/filter.xml +++ b/config/spotbugs/filter.xml @@ -111,4 +111,10 @@ + + + + + + diff --git a/docs/source/1.0/spec/core/model.rst b/docs/source/1.0/spec/core/model.rst index d04adaa1ecb..7d568ec1972 100644 --- a/docs/source/1.0/spec/core/model.rst +++ b/docs/source/1.0/spec/core/model.rst @@ -959,13 +959,16 @@ model that can be marked with the ``smithy.api#unitType`` trait. 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 ``List filter = FunctionalUtils.alwaysTrue(); private PathFinder(Model model) { this.model = model; @@ -80,6 +84,15 @@ public static PathFinder create(Model model) { return new PathFinder(model); } + /** + * Sets a predicate function to prevents traversing specific relationships. + * + * @param predicate Predicate that must return true in order to continue traversing relationships. + */ + public void relationshipFilter(Predicate predicate) { + this.filter = predicate; + } + /** * Finds all of the possible paths from the starting shape to all shapes * connected to the starting shape that match the given selector. @@ -118,7 +131,7 @@ private List searchFromShapeToSet(ToShapeId startingShape, Collection createPathTo(ToShapeId operationId, String memberName, Re return Optional.empty(); } - List relationships = new ArrayList<>(); - relationships.add(Relationship.create(operation, rel, struct)); - relationships.add(Relationship.create(struct, RelationshipType.STRUCTURE_MEMBER, member)); - relationships.add(Relationship.create(member, RelationshipType.MEMBER_TARGET, target)); - return Optional.of(new Path(relationships)); + Path path = new Path(Relationship.create(member, RelationshipType.MEMBER_TARGET, target), null); + path = new Path(Relationship.create(struct, RelationshipType.STRUCTURE_MEMBER, member), path); + path = new Path(Relationship.create(operation, rel, struct), path); + return Optional.of(path); } /** * An immutable {@code Relationship} path from a starting shape to an end shape. */ public static final class Path extends AbstractList { - private final List relationships; + private final Relationship value; + private Path next; + private final int size; public Path(List relationships) { if (relationships.isEmpty()) { throw new IllegalArgumentException("Relationships cannot be empty!"); } - this.relationships = relationships; + this.size = relationships.size(); + this.value = relationships.get(0); + + if (relationships.size() == 1) { + next = null; + } else { + Path current = this; + for (int i = 1; i < relationships.size(); i++) { + current.next = new Path(relationships.get(i), null); + current = current.next; + } + } + } + + private Path(Relationship value, Path next) { + this.value = value; + this.next = next; + this.size = 1 + ((next == null) ? 0 : next.size); } @Override public int size() { - return relationships.size(); + return size; } @Override public Relationship get(int index) { - return relationships.get(index); + Path current = this; + for (int i = 0; i < index; i++) { + current = current.next; + if (current == null) { + throw new IndexOutOfBoundsException("Invalid index " + index + "; size " + size()); + } + } + return current.value; + } + + @Override + public Iterator iterator() { + return new Iterator() { + private Path current = Path.this; + + @Override + public boolean hasNext() { + return current != null; + } + + @Override + public Relationship next() { + if (current == null) { + throw new NoSuchElementException(); + } + Relationship result = current.value; + current = current.next; + return result; + } + }; } /** @@ -225,14 +285,17 @@ public Relationship get(int index) { * @return Returns the list of shapes. */ public List getShapes() { - List results = relationships.stream() - .map(Relationship::getShape) - .collect(Collectors.toList()); - Relationship last = relationships.get(relationships.size() - 1); - if (last.getNeighborShape().isPresent()) { - results.add(last.getNeighborShape().get()); + List results = new ArrayList<>(size()); + Iterator iterator = iterator(); + for (int i = 0; i < size(); i++) { + Relationship rel = iterator.next(); + results.add(rel.getShape()); + // Add the shape pointed to by the tail to the result set if present + // without need to get the tail after iterating (an O(N) operation). + if (i == size() - 1) { + rel.getNeighborShape().ifPresent(results::add); + } } - return results; } @@ -242,7 +305,7 @@ public List getShapes() { * @return Returns the starting shape of the Path. */ public Shape getStartShape() { - return relationships.get(0).getShape(); + return value.getShape(); } /** @@ -254,12 +317,20 @@ public Shape getStartShape() { * @throws SourceException if the last relationship is invalid. */ public Shape getEndShape() { - Relationship last = relationships.get(relationships.size() - 1); + Relationship last = tail(); return last.getNeighborShape().orElseThrow(() -> new SourceException( "Relationship points to a shape that is invalid: " + last, last.getShape())); } + private Relationship tail() { + Path current = this; + while (current.next != null) { + current = current.next; + } + return current.value; + } + /** * Converts the path to valid {@link Selector} syntax. * @@ -270,7 +341,7 @@ public String toString() { StringBuilder result = new StringBuilder(); result.append("[id|").append(getStartShape().getId()).append("]"); - for (Relationship rel : relationships) { + for (Relationship rel : this) { if (rel.getRelationshipType() == RelationshipType.MEMBER_TARGET) { result.append(" > "); } else { @@ -291,48 +362,44 @@ private static final class Search { private final NeighborProvider provider; private final Collection candidates; private final List results = new ArrayList<>(); - - Search(NeighborProvider provider, Shape startingShape, Collection candidates) { + private final Predicate filter; + + Search( + NeighborProvider provider, + Shape startingShape, + Collection candidates, + Predicate filter + ) { this.startingShape = startingShape; this.candidates = candidates; this.provider = provider; + this.filter = filter; } List execute() { - for (Shape candidate : candidates) { - traverseUp(candidate, new LinkedList<>(), new HashSet<>()); + Queue queue = new ArrayDeque<>(); + for (Shape current : candidates) { + addRelationships(current, queue, null); + while (!queue.isEmpty()) { + Path path = queue.poll(); + Shape shape = path.value.getShape(); + if (shape.getId().equals(startingShape.getId())) { + results.add(path); + } else { + addRelationships(shape, queue, path); + } + } } return results; } - private void traverseUp(Shape current, List path, Set visited) { - if (!path.isEmpty() && current.getId().equals(startingShape.getId())) { - // Add the path to the result set if the target shape was reached. - // But, don't add the path if no nodes have been traversed. - results.add(new Path(path)); - return; - } - - // Short circuit any possible recursion. While it's not possible - // to enter a recursive path with the built-in NeighborProvider - // implementations and the bottom-up traversal, it is possible - // that a custom neighbor provider has a different behavior, so - // this check remains just in case. - if (visited.contains(current.getId())) { - return; - } - - Set newVisited = new HashSet<>(visited); - newVisited.add(current.getId()); - + private void addRelationships(Shape current, Queue queue, Path currentPath) { for (Relationship relationship : provider.getNeighbors(current)) { - // Don't traverse up through containers. if (relationship.getDirection() == RelationshipDirection.DIRECTED) { - List newPath = new ArrayList<>(path.size() + 1); - newPath.add(relationship); - newPath.addAll(path); - traverseUp(relationship.getShape(), newPath, newVisited); + if (filter.test(relationship)) { + queue.add(new Path(relationship, currentPath)); + } } } } 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 4f75f32ac8d..d125667aa21 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 @@ -1,5 +1,5 @@ /* - * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved. + * Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved. * * Licensed under the Apache License, Version 2.0 (the "License"). * You may not use this file except in compliance with the License. @@ -15,29 +15,28 @@ package software.amazon.smithy.model.validation.validators; -import java.util.ArrayDeque; -import java.util.Deque; -import java.util.HashSet; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; -import java.util.Objects; -import java.util.Set; -import java.util.stream.Collectors; +import java.util.StringJoiner; import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.selector.PathFinder; 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.SetShape; import software.amazon.smithy.model.shapes.Shape; -import software.amazon.smithy.model.shapes.ShapeId; -import software.amazon.smithy.model.shapes.ShapeVisitor; +import software.amazon.smithy.model.shapes.StructureShape; +import software.amazon.smithy.model.traits.RequiredTrait; import software.amazon.smithy.model.validation.AbstractValidator; import software.amazon.smithy.model.validation.ValidationEvent; +import software.amazon.smithy.utils.FunctionalUtils; /** * Ensures that list, set, and map shapes are not directly recursive, * meaning that if they do have a recursive reference to themselves, * one or more references that form the recursive path travels through - * a structure or union shape. + * a structure or union shape. And ensures that structure members are + * not infinitely mutually recursive using the required trait. * *

This check removes an entire class of problems from things like * code generators where a list of itself or a list of maps of itself @@ -47,82 +46,66 @@ public final class ShapeRecursionValidator extends AbstractValidator { @Override public List validate(Model model) { - return model.shapes() - .map(shape -> validateShape(model, shape)) - .filter(Objects::nonNull) - .collect(Collectors.toList()); + PathFinder finder = PathFinder.create(model); + List events = new ArrayList<>(); + validateListMapSetShapes(finder, model, events); + validateStructurePaths(finder, model, events); + return events; } - private ValidationEvent validateShape(Model model, Shape shape) { - return new RecursiveNeighborVisitor(model, shape).visit(shape); - } - - private final class RecursiveNeighborVisitor extends ShapeVisitor.Default { + private void validateListMapSetShapes(PathFinder finder, Model model, List events) { + finder.relationshipFilter(rel -> !(rel.getShape().isStructureShape() || rel.getShape().isUnionShape())); - private final Model model; - private final Shape root; - private final Set visited = new HashSet<>(); - private final Deque context = new ArrayDeque<>(); - - RecursiveNeighborVisitor(Model model, Shape root) { - this.root = root; - this.model = model; + for (ListShape shape : model.getListShapes()) { + validateListMapSetShapes(shape, finder, events); } - ValidationEvent visit(Shape shape) { - ValidationEvent event = hasShapeBeenVisited(shape); - return event != null ? event : shape.accept(this); + for (SetShape shape : model.getSetShapes()) { + validateListMapSetShapes(shape, finder, events); } - 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))); + for (MapShape shape : model.getMapShapes()) { + validateListMapSetShapes(shape, finder, events); } - @Override - protected ValidationEvent getDefault(Shape shape) { - return null; - } + finder.relationshipFilter(FunctionalUtils.alwaysTrue()); + } - @Override - public ValidationEvent listShape(ListShape shape) { - return validateMember(shape, shape.getMember()); + private void validateListMapSetShapes(Shape shape, PathFinder finder, List events) { + for (PathFinder.Path path : finder.search(shape, Collections.singletonList(shape))) { + events.add(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.", formatPath(path)))); } + } - @Override - public ValidationEvent setShape(SetShape shape) { - return validateMember(shape, shape.getMember()); - } + private void validateStructurePaths(PathFinder finder, Model model, List events) { + finder.relationshipFilter(rel -> { + if (rel.getShape().isStructureShape()) { + return rel.getNeighborShape().get().hasTrait(RequiredTrait.class); + } else { + return rel.getShape().isMemberShape(); + } + }); - @Override - public ValidationEvent mapShape(MapShape shape) { - return validateMember(shape, shape.getValue()); + for (StructureShape shape : model.getStructureShapes()) { + for (PathFinder.Path path : finder.search(shape, Collections.singletonList(shape))) { + events.add(error(shape, String.format( + "Found invalid shape recursion: %s. A structure cannot be mutually recursive through all " + + "required members.", formatPath(path)))); + } } + } - private ValidationEvent validateMember(Shape container, MemberShape member) { - ValidationEvent event = null; - Shape target = model.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()); + private String formatPath(PathFinder.Path path) { + StringJoiner joiner = new StringJoiner(" > "); + List shapes = path.getShapes(); + for (int i = 0; i < shapes.size(); i++) { + // Skip the first shape (the subject) to shorten the error message. + if (i > 0) { + joiner.add(shapes.get(i).getId().toString()); } - - return event; } + return joiner.toString(); } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-required-trait.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-required-trait.errors new file mode 100644 index 00000000000..8b37bb9ab7a --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-required-trait.errors @@ -0,0 +1,2 @@ +[ERROR] smithy.example#RecursiveShape1: Found invalid shape recursion: smithy.example#RecursiveShape1$recursiveMember > smithy.example#RecursiveShape2 > smithy.example#RecursiveShape2$recursiveMember > smithy.example#RecursiveShape1. A structure cannot be mutually recursive through all required members. | ShapeRecursion +[ERROR] smithy.example#RecursiveShape2: Found invalid shape recursion: smithy.example#RecursiveShape2$recursiveMember > smithy.example#RecursiveShape1 > smithy.example#RecursiveShape1$recursiveMember > smithy.example#RecursiveShape2. A structure cannot be mutually recursive through all required members. | ShapeRecursion diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-required-trait.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-required-trait.smithy new file mode 100644 index 00000000000..4173fca238d --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-required-trait.smithy @@ -0,0 +1,15 @@ +namespace smithy.example + +structure RecursiveShape1 { + @required + recursiveMember: RecursiveShape2 +} + +structure RecursiveShape2 { + // Bad + @required + recursiveMember: RecursiveShape1, + + // Ok + recursiveMember2: RecursiveShape1 +}