Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forbid recursion through all required members #1200

Merged
merged 2 commits into from
Apr 25, 2022
Merged

Forbid recursion through all required members #1200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15a7e2c
Forbid recursion through all required members
mtdowling Apr 22, 2022
89e12e9
Optimize PathFinder and fix recursion bug
mtdowling Apr 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions config/spotbugs/filter.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,10 @@
<Class name="software.amazon.smithy.model.knowledge.HttpBindingIndex"/>
<Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
</Match>

<!-- current is used to create an internal linked list's next value from a list -->
<Match>
<Class name="software.amazon.smithy.model.selector.PathFinder$Path"/>
<Bug pattern="UC_USELESS_OBJECT"/>
</Match>
</FindBugsFilter>
34 changes: 27 additions & 7 deletions docs/source/1.0/spec/core/model.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<List<List....``).
Smithy allows recursive shape definitions with the following limitations:

1. 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<List<List....``).
2. A structure cannot contain a cyclical set of members marked with the
:ref:`required-trait` that refers back to itself.

The following recursive shape definition is **valid**:

Expand Down Expand Up @@ -1031,6 +1034,23 @@ The following recursive shape definition is **invalid**:
}
}

The following recursive shape definition is **invalid** due to mutual
recursion and the :ref:`required-trait`.

.. code-block:: smithy

namespace smithy.example

structure RecursiveShape1 {
@required
recursiveMember: RecursiveShape2
}

structure RecursiveShape2 {
@required
recursiveMember: RecursiveShape1
}


.. _service-types:

Expand Down
171 changes: 119 additions & 52 deletions smithy-model/src/main/java/software/amazon/smithy/model/selector/PathFinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,17 @@
package software.amazon.smithy.model.selector;

import java.util.AbstractList;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.Iterator;
import java.util.List;
import java.util.NoSuchElementException;
import java.util.Optional;
import java.util.Queue;
import java.util.Set;
import java.util.function.Predicate;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.SourceException;
import software.amazon.smithy.model.knowledge.NeighborProviderIndex;
Expand All @@ -38,6 +40,7 @@
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.shapes.ToShapeId;
import software.amazon.smithy.utils.FunctionalUtils;
import software.amazon.smithy.utils.ListUtils;

/**
Expand All @@ -64,6 +67,7 @@ public final class PathFinder {

private final Model model;
private final NeighborProvider reverseProvider;
private Predicate<Relationship> filter = FunctionalUtils.alwaysTrue();

private PathFinder(Model model) {
this.model = model;
Expand All @@ -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<Relationship> 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.
Expand Down Expand Up @@ -118,7 +131,7 @@ private List<Path> searchFromShapeToSet(ToShapeId startingShape, Collection<Shap
if (shape == null || candidates.isEmpty()) {
return ListUtils.of();
} else {
return new Search(reverseProvider, shape, candidates).execute();
return new Search(reverseProvider, shape, candidates, filter).execute();
}
}

Expand Down Expand Up @@ -184,35 +197,82 @@ private Optional<Path> createPathTo(ToShapeId operationId, String memberName, Re
return Optional.empty();
}

List<Relationship> 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<Relationship> {
private final List<Relationship> relationships;
private final Relationship value;
private Path next;
private final int size;

public Path(List<Relationship> 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<Relationship> iterator() {
return new Iterator<Relationship>() {
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;
}
};
}

/**
Expand All @@ -225,14 +285,17 @@ public Relationship get(int index) {
* @return Returns the list of shapes.
*/
public List<Shape> getShapes() {
List<Shape> 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<Shape> results = new ArrayList<>(size());
Iterator<Relationship> 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;
}

Expand All @@ -242,7 +305,7 @@ public List<Shape> getShapes() {
* @return Returns the starting shape of the Path.
*/
public Shape getStartShape() {
return relationships.get(0).getShape();
return value.getShape();
}

/**
Expand All @@ -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.
*
Expand All @@ -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 {
Expand All @@ -291,48 +362,44 @@ private static final class Search {
private final NeighborProvider provider;
private final Collection<Shape> candidates;
private final List<Path> results = new ArrayList<>();

Search(NeighborProvider provider, Shape startingShape, Collection<Shape> candidates) {
private final Predicate<Relationship> filter;

Search(
NeighborProvider provider,
Shape startingShape,
Collection<Shape> candidates,
Predicate<Relationship> filter
) {
this.startingShape = startingShape;
this.candidates = candidates;
this.provider = provider;
this.filter = filter;
}

List<Path> execute() {
for (Shape candidate : candidates) {
traverseUp(candidate, new LinkedList<>(), new HashSet<>());
Queue<Path> 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<Relationship> path, Set<ShapeId> 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<ShapeId> newVisited = new HashSet<>(visited);
newVisited.add(current.getId());

private void addRelationships(Shape current, Queue<Path> queue, Path currentPath) {
for (Relationship relationship : provider.getNeighbors(current)) {
// Don't traverse up through containers.
if (relationship.getDirection() == RelationshipDirection.DIRECTED) {
List<Relationship> 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));
}
}
}
}
Expand Down
Loading