Skip to content

Commit a78331f

Browse files
committed
Add validator for identifiers missing references
This commit introduces a new validator that emits warnings when it detects a member that matches a resource identifier without the relevant `@references` trait configuration.
1 parent 0f10790 commit a78331f

File tree

8 files changed

+185
-6
lines changed

8 files changed

+185
-6
lines changed
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package software.amazon.smithy.model.validation.validators;
7+
8+
import static java.lang.String.format;
9+
10+
import java.util.ArrayList;
11+
import java.util.HashSet;
12+
import java.util.List;
13+
import java.util.Map;
14+
import java.util.Set;
15+
import software.amazon.smithy.model.Model;
16+
import software.amazon.smithy.model.selector.PathFinder;
17+
import software.amazon.smithy.model.shapes.MemberShape;
18+
import software.amazon.smithy.model.shapes.ResourceShape;
19+
import software.amazon.smithy.model.shapes.Shape;
20+
import software.amazon.smithy.model.shapes.ShapeId;
21+
import software.amazon.smithy.model.traits.ReferencesTrait;
22+
import software.amazon.smithy.model.validation.AbstractValidator;
23+
import software.amazon.smithy.model.validation.ValidationEvent;
24+
import software.amazon.smithy.model.validation.ValidationUtils;
25+
import software.amazon.smithy.utils.ListUtils;
26+
27+
/**
28+
* Validates if a member matches a resource identifier without the
29+
* proper configuration of a `@references` trait.
30+
*/
31+
public final class MemberShouldReferenceResourceValidator extends AbstractValidator {
32+
@Override
33+
public List<ValidationEvent> validate(Model model) {
34+
// There are usually far fewer resources than members, precompute the identifiers
35+
// so various short circuits can be added.
36+
Set<String> identifierNames = getAllIdentifierNames(model);
37+
// Short circuit validating all the members if we don't have any resources to test.
38+
if (identifierNames.isEmpty()) {
39+
return ListUtils.of();
40+
}
41+
42+
// Check every member to see if it's a potential reference.
43+
List<ValidationEvent> events = new ArrayList<>();
44+
for (MemberShape member : model.getMemberShapes()) {
45+
// Only the known identifier names can match for this, skip names that we don't know.
46+
if (!identifierNames.contains(member.getMemberName())) {
47+
continue;
48+
}
49+
// Only strings can be identifiers, so skip non-String targets.
50+
if (!model.expectShape(member.getTarget()).isStringShape()) {
51+
continue;
52+
}
53+
54+
Set<ShapeId> potentialReferences = computePotentialReferences(model, member);
55+
if (!potentialReferences.isEmpty()) {
56+
events.add(warning(member, format("This member appears to reference the following resources without "
57+
+ "being included in a `@references` trait: [%s]",
58+
ValidationUtils.tickedList(potentialReferences))));
59+
}
60+
}
61+
62+
return events;
63+
}
64+
65+
private Set<String> getAllIdentifierNames(Model model) {
66+
Set<String> identifierNames = new HashSet<>();
67+
for (ResourceShape resource : model.getResourceShapes()) {
68+
identifierNames.addAll(resource.getIdentifiers().keySet());
69+
}
70+
return identifierNames;
71+
}
72+
73+
private Set<ShapeId> computePotentialReferences(Model model, MemberShape member) {
74+
// Exclude any resources already in `@references` on the member or container structure.
75+
Set<ShapeId> resourcesToIgnore = new HashSet<>();
76+
ignoreReferencedResources(member, resourcesToIgnore);
77+
ignoreReferencedResources(model.expectShape(member.getContainer()), resourcesToIgnore);
78+
79+
// Check each resource in the model for something missed.
80+
Set<ShapeId> potentialResources = new HashSet<>();
81+
for (ResourceShape resource : model.getResourceShapes()) {
82+
// We'll want to ignore some resources based on the member -> resource path.
83+
computeResourcesToIgnore(model, member, resource, resourcesToIgnore);
84+
85+
// Exclude members bound to resource hierarchies from generating events,
86+
// including for resources that are within the same hierarchy.
87+
if (resourcesToIgnore.contains(resource.getId())) {
88+
continue;
89+
}
90+
91+
// This member matches the identifier for the resource we're checking, add it to a list.
92+
if (isIdentifierMatch(resource, member)) {
93+
potentialResources.add(resource.getId());
94+
}
95+
}
96+
97+
// Clean up any resources added through other paths that should be ignored.
98+
potentialResources.removeAll(resourcesToIgnore);
99+
return potentialResources;
100+
}
101+
102+
private void computeResourcesToIgnore(Model model, MemberShape member, ResourceShape resource,
103+
Set<ShapeId> resourcesToIgnore) {
104+
// Exclude actually bound members via searching with a PathFinder.
105+
List<PathFinder.Path> resourceMemberPaths = PathFinder.create(model)
106+
.search(resource, ListUtils.of(member));
107+
if (!resourceMemberPaths.isEmpty()) {
108+
// This member is already bound to a resource, so we don't need a references trait for it.
109+
// In addition, we should not tell users to add a references trait for other resources that
110+
// are children in that hierarchy - any parent resources or other children of those parents.
111+
for (PathFinder.Path path : resourceMemberPaths) {
112+
for (Shape pathShape : path.getShapes()) {
113+
if (pathShape.isResourceShape()) {
114+
ResourceShape resourceShape = (ResourceShape) pathShape;
115+
resourcesToIgnore.add(resourceShape.getId());
116+
resourcesToIgnore.addAll(resourceShape.getResources());
117+
}
118+
}
119+
}
120+
}
121+
}
122+
123+
private void ignoreReferencedResources(Shape shape, Set<ShapeId> resourcesToIgnore) {
124+
if (shape.hasTrait(ReferencesTrait.class)) {
125+
for (ReferencesTrait.Reference reference : shape.expectTrait(ReferencesTrait.class)
126+
.getReferences()) {
127+
resourcesToIgnore.add(reference.getResource());
128+
}
129+
}
130+
}
131+
132+
private boolean isIdentifierMatch(ResourceShape resource, MemberShape member) {
133+
Map<String, ShapeId> identifiers = resource.getIdentifiers();
134+
return identifiers.containsKey(member.getMemberName())
135+
&& identifiers.get(member.getMemberName()).equals(member.getTarget());
136+
}
137+
}

smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ software.amazon.smithy.model.validation.validators.IdempotencyTokenIgnoredValida
2626
software.amazon.smithy.model.validation.validators.JsonNameValidator
2727
software.amazon.smithy.model.validation.validators.LengthTraitValidator
2828
software.amazon.smithy.model.validation.validators.MediaTypeValidator
29+
software.amazon.smithy.model.validation.validators.MemberShouldReferenceResourceValidator
2930
software.amazon.smithy.model.validation.validators.NoInlineDocumentSupportValidator
3031
software.amazon.smithy.model.validation.validators.OperationValidator
3132
software.amazon.smithy.model.validation.validators.PaginatedTraitValidator
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
[WARNING] ns.foo#Operation1Input$fooId: This member appears to reference the following resources without being included in a `@references` trait: [`ns.foo#FooResource`] | MemberShouldReferenceResource
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
$version: "2.0"
2+
3+
namespace ns.foo
4+
5+
resource FooResource {
6+
identifiers: {
7+
fooId: String
8+
}
9+
}
10+
11+
operation Operation1 {
12+
input := {
13+
fooId: String
14+
}
15+
}
16+
17+
operation Operation2 {
18+
input := @references([{
19+
resource: FooResource
20+
}]) {
21+
fooId: String
22+
}
23+
}
24+
25+
resource BarResource {
26+
identifiers: {
27+
barId: BarId
28+
}
29+
}
30+
31+
operation Operation3 {
32+
input := @references([{
33+
resource: BarResource
34+
}]) {
35+
barId: BarId
36+
}
37+
}
38+
39+
string BarId

smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/valid-implicit-references.json

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,6 @@
1919
},
2020
"traits": {
2121
"smithy.api#references": [
22-
{
23-
"resource": "ns.foo#MyResource"
24-
},
2522
{
2623
"resource": "ns.foo#MyResource"
2724
},
@@ -48,6 +45,9 @@
4845
"smithy.api#references": [
4946
{
5047
"resource": "ns.foo#MyResource"
48+
},
49+
{
50+
"resource": "ns.foo#MyResource2"
5151
}
5252
]
5353
}
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
[WARNING] smithy.example#GetFoo: The `smithy.example#GetFoo` operation is bound to the `smithy.example#FooService` service but has members that match identifiers of the following resource shapes: [`smithy.example#Foo`]. It may be more accurately bound to one of them than directly to the service. | ServiceBoundResourceOperation.smithy.example#FooService.GetFoo
2+
[WARNING] smithy.example#GetFooInput$fooId: This member appears to reference the following resources without being included in a `@references` trait: [`smithy.example#Foo`] | MemberShouldReferenceResource
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
[ERROR] ns.foo#InvalidResourceOperation1: This operation does not form a valid instance operation when bound to resource `ns.foo#InvalidResource`. All of the identifiers of the resource were not implicitly or explicitly bound to the input of the operation. Expected the following identifier bindings: [required member named `foo` that targets `smithy.api#String`]. Found the following identifier bindings: [] | ResourceIdentifierBinding
1+
[ERROR] ns.foo#InvalidResourceOperation1: This operation does not form a valid instance operation when bound to resource `ns.foo#InvalidResource`. All of the identifiers of the resource were not implicitly or explicitly bound to the input of the operation. Expected the following identifier bindings: [required member named `bar` that targets `smithy.api#String`]. Found the following identifier bindings: [] | ResourceIdentifierBinding
22
[ERROR] ns.foo#ValidResourceOperation2: This operation does not form a valid instance operation when bound to resource `ns.foo#ValidResource`. All of the identifiers of the resource were not implicitly or explicitly bound to the input of the operation. Expected the following identifier bindings: [required member named `foo` that targets `smithy.api#String`]. Found the following identifier bindings: [] | ResourceIdentifierBinding

smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-operation-binding-validator.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@
8383
"ns.foo#InvalidResource": {
8484
"type": "resource",
8585
"identifiers": {
86-
"foo": {
86+
"bar": {
8787
"target": "smithy.api#String"
8888
}
8989
},
@@ -129,7 +129,7 @@
129129
"ns.foo#InvalidResourceOperation2Input": {
130130
"type": "structure",
131131
"members": {
132-
"foo": {
132+
"bar": {
133133
"target": "smithy.api#String",
134134
"traits": {
135135
"smithy.api#required": {}

0 commit comments

Comments
 (0)