-
Notifications
You must be signed in to change notification settings - Fork 218
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Prevent bad list, set, map recursion
We currently don't allow a directly recursive list or set shape. However, we still allow for list, set, and map shapes to be transitively recursive unconditionally (e.g., listA -> listA is invalid, but listA -> set -> listA is valid). This is problematic for code generators in various languages that want to use parameteric types and standard libraries to codegen lists, sets, and maps. For example, in Java, a list in Smithy should be generated as a `List<X>` where X is the code generated member of the list. However, if the list is recursive onto itself, then it's impossible to generate valid code (e.g., `List<List<List...`). This change requires that recursive list, set, and map shapes must have one or more structure or union members in their recursive path in order to define a valid shape. This will result in shapes that are far easier to generate and provides constraints that are very helpful when converting to other formats (for example, this might be useful in JSON schema conversions if we want to always inline list and set shape definitions). This change should not have a practical impact on models since no current AWS service defines an unconditionally recursive list, set, or map shape.
- Loading branch information
Showing
8 changed files
with
333 additions
and
16 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
130 changes: 130 additions & 0 deletions
130
...main/java/software/amazon/smithy/model/validation/validators/ShapeRecursionValidator.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| /* | ||
| * Copyright 2019 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. | ||
| * A copy of the License is located at | ||
| * | ||
| * http://aws.amazon.com/apache2.0 | ||
| * | ||
| * or in the "license" file accompanying this file. This file is distributed | ||
| * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
| * express or implied. See the License for the specific language governing | ||
| * permissions and limitations under the License. | ||
| */ | ||
|
|
||
| package software.amazon.smithy.model.validation.validators; | ||
|
|
||
| import java.util.ArrayDeque; | ||
| import java.util.Deque; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
| import java.util.stream.Collectors; | ||
| import software.amazon.smithy.model.Model; | ||
| 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.ShapeIndex; | ||
| import software.amazon.smithy.model.shapes.ShapeVisitor; | ||
| import software.amazon.smithy.model.validation.AbstractValidator; | ||
| import software.amazon.smithy.model.validation.ValidationEvent; | ||
|
|
||
| /** | ||
| * 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. | ||
| * | ||
| * <p>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 | ||
| * is impossible to define. | ||
| */ | ||
| public class ShapeRecursionValidator extends AbstractValidator { | ||
|
|
||
| @Override | ||
| public List<ValidationEvent> 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<ValidationEvent> { | ||
|
|
||
| private final ShapeIndex index; | ||
| private final Shape root; | ||
| private final Set<ShapeId> visited = new HashSet<>(); | ||
| private final Deque<String> 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; | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
12 changes: 12 additions & 0 deletions
12
.../test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion.errors
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
112 changes: 112 additions & 0 deletions
112
...rc/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.