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.

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

Check references via idRef in UnreferencedShapes #2105

Merged
merged 6 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
[ERROR] smithy.example#AdditionalSchemasConflictResource: The `bar` property of the generated `AdditionalSchemasConflictResource` CloudFormation resource targets multiple shapes: [smithy.api#Boolean, smithy.api#String]. Reusing member names that target different shapes can cause confusion for users of the API. This target discrepancy must either be resolved in the model or one of the members must be excluded from the conversion. | CfnResourceProperty
[NOTE] smithy.example#AdditionalSchemasConflictProperties: The structure shape is not connected to from any service shape. | UnreferencedShape
[WARNING] smithy.example#AdditionalSchemasConflictResource: This shape applies a trait that is unstable: aws.cloudformation#cfnResource | UnstableTrait
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
[NOTE] smithy.example#AdditionalSchemasDeconflictedProperties: The structure shape is not connected to from any service shape. | UnreferencedShape
[WARNING] smithy.example#CreateAdditionalSchemasDeconflictedResourceRequest$bar: This shape applies a trait that is unstable: aws.cloudformation#cfnExcludeProperty | UnstableTrait
[WARNING] smithy.example#AdditionalSchemasDeconflictedResource: This shape applies a trait that is unstable: aws.cloudformation#cfnResource | UnstableTrait
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
[ERROR] smithy.example#InvalidAdditionalSchemasShapeResource: Error validating trait `aws.cloudformation#cfnResource`.additionalSchemas.0: Shape ID `smithy.example#ListShape` does not match selector `structure` | TraitValue
[NOTE] smithy.example#ListShape: The list shape is not connected to from any service shape. | UnreferencedShape
[WARNING] smithy.example#InvalidAdditionalSchemasShapeResource: This shape applies a trait that is unstable: aws.cloudformation#cfnResource | UnstableTrait
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/*
* Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package software.amazon.smithy.model.neighbor;

import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.node.Node;
import software.amazon.smithy.model.node.StringNode;
import software.amazon.smithy.model.selector.PathFinder;
import software.amazon.smithy.model.selector.Selector;
import software.amazon.smithy.model.shapes.MemberShape;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.traits.IdRefTrait;
import software.amazon.smithy.model.traits.Trait;
import software.amazon.smithy.model.traits.TraitDefinition;

/**
* Finds all {@link RelationshipType#ID_REF} relationships in a model.
*
* <p>This works by finding all paths from {@link TraitDefinition} shapes
* to shapes with {@link IdRefTrait}, then using those paths to search
* the node values of each application of the trait to extract the {@link ShapeId}
* value. Because we don't have a fixed set of traits known to potentially have
* idRef members, this has to be done dynamically.
*/
final class IdRefShapeRelationships {
private static final Selector WITH_ID_REF = Selector.parse("[trait|idRef]");

private final Model model;
private final Map<ShapeId, Set<Relationship>> relationships = new HashMap<>();

IdRefShapeRelationships(Model model) {
this.model = model;
}

Map<ShapeId, Set<Relationship>> getRelationships() {
PathFinder finder = PathFinder.create(model);
for (Shape traitDef : model.getShapesWithTrait(TraitDefinition.class)) {
if (traitDef.hasTrait(IdRefTrait.class)) {
// PathFinder doesn't handle the case where the trait def has the idRef
NodeQuery query = new NodeQuery().self();
addRelationships(traitDef, query);
continue;
}
List<PathFinder.Path> paths = finder.search(traitDef, WITH_ID_REF);
if (!paths.isEmpty()) {
for (PathFinder.Path path : paths) {
NodeQuery query = buildNodeQuery(path);
addRelationships(traitDef, query);
}
}
}
return relationships;
}

private void addRelationships(Shape traitDef, NodeQuery query) {
model.getShapesWithTrait(traitDef.getId()).forEach(shape -> {
Trait trait = shape.findTrait(traitDef.getId()).get(); // We already know the shape has the trait.
Node node = trait.toNode();
// Invalid shape ids are handled by the idRef trait validator, so ignore them here.
query.execute(node).forEach(n -> n.asStringNode()
.flatMap(StringNode::asShapeId)
.flatMap(model::getShape)
.map(referenced -> Relationship.create(shape, RelationshipType.ID_REF, referenced))
.ifPresent(rel -> relationships
.computeIfAbsent(rel.getShape().getId(), id -> new HashSet<>()).add(rel)));
});
}

private static NodeQuery buildNodeQuery(PathFinder.Path path) {
NodeQuery query = new NodeQuery();
// The path goes from trait definition -> shape with idRef
for (Relationship relationship : path) {
if (!relationship.getNeighborShape().isPresent()) {
break;
}
switch (relationship.getRelationshipType()) {
case MAP_KEY:
query.anyMemberName();
break;
case MAP_VALUE:
query.anyMember();
break;
case LIST_MEMBER:
case SET_MEMBER:
query.anyElement();
break;
case UNION_MEMBER:
case STRUCTURE_MEMBER:
MemberShape member = (MemberShape) relationship.getNeighborShape().get();
mtdowling marked this conversation as resolved.
Show resolved Hide resolved
query.member(member.getMemberName());
break;
default:
// Other relationship types don't produce meaningful edges to search the node.
break;
}
}
return query;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.shapes.Shape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.traits.IdRefTrait;
import software.amazon.smithy.utils.ListUtils;

/**
Expand Down Expand Up @@ -91,6 +92,29 @@ static NeighborProvider withTraitRelationships(Model model, NeighborProvider nei
};
}

/**
* Creates a NeighborProvider that includes {@link RelationshipType#ID_REF}
* relationships.
*
* @param model Model to use to look up shapes referenced by {@link IdRefTrait}.
* @param neighborProvider Provider to wrap.
* @return Returns the created neighbor provider.
*/
static NeighborProvider withIdRefRelationships(Model model, NeighborProvider neighborProvider) {
Map<ShapeId, Set<Relationship>> idRefRelationships = new IdRefShapeRelationships(model).getRelationships();
return shape -> {
List<Relationship> relationships = neighborProvider.getNeighbors(shape);

if (!idRefRelationships.containsKey(shape.getId())) {
return relationships;
}

relationships = new ArrayList<>(relationships);
relationships.addAll(idRefRelationships.get(shape.getId()));
return relationships;
};
}

/**
* Creates a NeighborProvider that precomputes the neighbors of a model.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package software.amazon.smithy.model.neighbor;

import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Queue;
import software.amazon.smithy.model.node.Node;

/**
* Searches {@link Node}s to find matching children. Each search
* condition is executed on the result of the previous search,
* and the results are aggregated.
*/
final class NodeQuery {
private static final Query SELF = (node, result) -> result.add(node);

private static final Query ANY_MEMBER = (node, result) -> {
if (node == null || !node.isObjectNode()) {
return;
}
result.addAll(node.expectObjectNode().getMembers().values());
};

private static final Query ANY_ELEMENT = (node, result) -> {
if (node == null || !node.isArrayNode()) {
return;
}
result.addAll(node.expectArrayNode().getElements());
};

private static final Query ANY_MEMBER_NAME = (node, result) -> {
if (node == null || !node.isObjectNode()) {
return;
}
result.addAll(node.expectObjectNode().getMembers().keySet());
};

private final List<Query> queries = new ArrayList<>();

NodeQuery() {
}

NodeQuery self() {
queries.add(SELF);
return this;
}

NodeQuery member(String name) {
queries.add((node, result) -> {
if (node == null || !node.isObjectNode()) {
return;
}
node.expectObjectNode().getMember(name).ifPresent(result::add);
});
return this;
}

NodeQuery anyMember() {
mtdowling marked this conversation as resolved.
Show resolved Hide resolved
queries.add(ANY_MEMBER);
return this;
}

NodeQuery anyElement() {
queries.add(ANY_ELEMENT);
return this;
}

NodeQuery anyMemberName() {
queries.add(ANY_MEMBER_NAME);
return this;
}

Collection<Node> execute(Node node) {
Queue<Node> previousResult = new ArrayDeque<>();

if (queries.isEmpty()) {
return previousResult;
}

previousResult.add(node);
for (Query query : queries) {
// Each time a query runs, it adds to the queue, but we only want it to
// run on the nodes added by the previous query.
for (int i = previousResult.size(); i > 0; i--) {
query.run(previousResult.poll(), previousResult);
}
}

return previousResult;
}

@FunctionalInterface
interface Query {
void run(Node node, Queue<Node> result);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@
import software.amazon.smithy.model.shapes.OperationShape;
import software.amazon.smithy.model.shapes.ResourceShape;
import software.amazon.smithy.model.shapes.SetShape;
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.shapes.UnionShape;
import software.amazon.smithy.model.traits.IdRefTrait;
import software.amazon.smithy.model.traits.TraitDefinition;
import software.amazon.smithy.utils.SmithyInternalApi;

Expand Down Expand Up @@ -214,7 +216,37 @@ public enum RelationshipType {
* Relationship that exists between a structure or union and a mixin applied
* to the shape.
*/
MIXIN("mixin", RelationshipDirection.DIRECTED);
MIXIN("mixin", RelationshipDirection.DIRECTED),

/**
* Relationships that exist between a shape and another shape referenced by an
* {@link IdRefTrait}.
*
* <p>This relationship is formed by applying a trait with a value containing a
* reference to another {@link ShapeId}. For
* example:
* <pre>
* {@code
* @trait
* structure myRef {
* @idRef
* shape: String
* }
*
* // @myRef trait applied, and the value references the shape `Referenced`
* @myRef(shape: Referenced)
* structure WithMyRef {}
*
* string Referenced
* }
* </pre>
*
* <p>This kind of relationship is not returned by default from a
* {@link NeighborProvider}. You must explicitly wrap a {@link NeighborProvider}
* with {@link NeighborProvider#withIdRefRelationships(Model, NeighborProvider)}
* in order to yield idRef relationships.
*/
ID_REF(null, RelationshipDirection.DIRECTED);

private String selectorLabel;
private RelationshipDirection direction;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@

/**
* Finds shapes that are not connected to a service shape, are not trait
* definitions, and are not referenced by trait definitions.
* definitions, are not referenced by trait definitions, and are not
* referenced in trait values through
* {@link software.amazon.smithy.model.traits.IdRefTrait}.
*
* <p>Prelude shapes are never considered unreferenced.
*/
Expand All @@ -53,7 +55,9 @@ public UnreferencedShapes(Predicate<Shape> keepFilter) {
* @return Returns the unreferenced shapes.
*/
public Set<Shape> compute(Model model) {
Walker shapeWalker = new Walker(NeighborProviderIndex.of(model).getProvider());
NeighborProvider baseProvider = NeighborProviderIndex.of(model).getProvider();
NeighborProvider providerWithIdRefRelationships = NeighborProvider.withIdRefRelationships(model, baseProvider);
Walker shapeWalker = new Walker(providerWithIdRefRelationships);

// Find all shapes connected to any service shape.
Set<Shape> connected = new HashSet<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,19 @@ public ShapeId expectShapeId() {
}
}

/**
* Gets the value of the string as a ShapeId if it is a valid Shape ID.
*
* @return Returns the optional Shape ID.
*/
public Optional<ShapeId> asShapeId() {
try {
return Optional.of(ShapeId.from(getValue()));
} catch (ShapeIdSyntaxException e) {
return Optional.empty();
}
}

@Override
public boolean equals(Object other) {
return other instanceof StringNode && value.equals(((StringNode) other).getValue());
Expand Down
Loading