From 21a816d69be214f908d9c02b34b44fd292967412 Mon Sep 17 00:00:00 2001 From: Kevin Stich Date: Wed, 9 Oct 2024 22:42:53 -0700 Subject: [PATCH] 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. --- .gitignore | 3 + ...emberShouldReferenceResourceValidator.java | 135 ++++++++++++++++++ ...e.amazon.smithy.model.validation.Validator | 1 + .../missing-member-references.errors | 1 + .../missing-member-references.smithy | 39 +++++ .../references/valid-implicit-references.json | 6 +- .../detects-misbound-operations.errors | 1 + ...esource-operation-binding-validator.errors | 2 +- .../resource-operation-binding-validator.json | 4 +- 9 files changed, 186 insertions(+), 6 deletions(-) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/MemberShouldReferenceResourceValidator.java create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/missing-member-references.errors create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/missing-member-references.smithy diff --git a/.gitignore b/.gitignore index ed1c10be16a..f39ff7525a1 100644 --- a/.gitignore +++ b/.gitignore @@ -14,6 +14,9 @@ docs/src *.iml *.iws +# Fleet +.fleet/ + # VSCode bin/ diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/MemberShouldReferenceResourceValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/MemberShouldReferenceResourceValidator.java new file mode 100644 index 00000000000..a4cf57bc5e4 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/MemberShouldReferenceResourceValidator.java @@ -0,0 +1,135 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.model.validation.validators; + +import static java.lang.String.format; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.selector.PathFinder; +import software.amazon.smithy.model.shapes.MemberShape; +import software.amazon.smithy.model.shapes.ResourceShape; +import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.traits.ReferencesTrait; +import software.amazon.smithy.model.validation.AbstractValidator; +import software.amazon.smithy.model.validation.ValidationEvent; +import software.amazon.smithy.model.validation.ValidationUtils; +import software.amazon.smithy.utils.ListUtils; + +/** + * Validates if a member matches a resource identifier without the + * proper configuration of a `@references` trait. + */ +public final class MemberShouldReferenceResourceValidator extends AbstractValidator { + @Override + public List validate(Model model) { + // There are usually far fewer resources than members, precompute the identifiers + // so various short circuits can be added. + List identifierNames = getAllIdentifierNames(model); + // Short circuit validating all the members if we don't have any resources to test. + if (identifierNames.isEmpty()) { + return ListUtils.of(); + } + + // Check every member to see if it's a potential reference. + List events = new ArrayList<>(); + for (MemberShape member : model.getMemberShapes()) { + // Only the known identifier names can match for this, skip names that we don't know. + if (!identifierNames.contains(member.getMemberName())) { + continue; + } + // Only strings can be identifiers, so skip non-String targets. + if (!model.expectShape(member.getTarget()).isStringShape()) { + continue; + } + + List potentialReferences = computePotentialReferences(model, member); + if (!potentialReferences.isEmpty()) { + events.add(warning(member, format("This member appears to reference the following resources without " + + "being included in a `@references` trait: [%s]", + ValidationUtils.tickedList(potentialReferences)))); + } + } + + return events; + } + + private List getAllIdentifierNames(Model model) { + List identifierNames = new ArrayList<>(); + for (ResourceShape resource : model.getResourceShapes()) { + identifierNames.addAll(resource.getIdentifiers().keySet()); + } + return identifierNames; + } + + private List computePotentialReferences(Model model, MemberShape member) { + // Exclude any resources already in `@references` on the member or container structure. + List resourcesToIgnore = new ArrayList<>(); + ignoreReferencedResources(member, resourcesToIgnore); + ignoreReferencedResources(model.expectShape(member.getContainer()), resourcesToIgnore); + + // Check each resource in the model for something missed. + List potentialResources = new ArrayList<>(); + for (ResourceShape resource : model.getResourceShapes()) { + // We'll want to ignore some resources based on the member -> resource path. + computeResourcesToIgnore(model, member, resource, resourcesToIgnore); + + // Exclude members bound to resource hierarchies from generating events, + // including for resources that are within the same hierarchy. + if (resourcesToIgnore.contains(resource.getId())) { + continue; + } + + // This member matches the identifier for the resource we're checking, add it to a list. + if (isIdentifierMatch(resource, member)) { + potentialResources.add(resource.getId()); + } + } + + // Clean up any resources added through other paths that should be ignored. + potentialResources.removeAll(resourcesToIgnore); + return potentialResources; + } + + private void computeResourcesToIgnore(Model model, MemberShape member, ResourceShape resource, + List resourcesToIgnore) { + // Exclude actually bound members via searching with a PathFinder. + List resourceMemberPaths = PathFinder.create(model) + .search(resource, ListUtils.of(member)); + if (!resourceMemberPaths.isEmpty()) { + // This member is already bound to a resource, so we don't need a references trait for it. + // In addition, we should not tell users to add a references trait for other resources that + // are children in that hierarchy - any parent resources or other children of those parents. + for (PathFinder.Path path : resourceMemberPaths) { + for (Shape pathShape : path.getShapes()) { + if (pathShape.isResourceShape()) { + ResourceShape resourceShape = (ResourceShape) pathShape; + resourcesToIgnore.add(resourceShape.getId()); + resourcesToIgnore.addAll(resourceShape.getResources()); + } + } + } + } + } + + private void ignoreReferencedResources(Shape shape, List resourcesToIgnore) { + if (shape.hasTrait(ReferencesTrait.class)) { + for (ReferencesTrait.Reference reference : shape.expectTrait(ReferencesTrait.class) + .getReferences()) { + resourcesToIgnore.add(reference.getResource()); + } + } + } + + private boolean isIdentifierMatch(ResourceShape resource, MemberShape member) { + Map identifiers = resource.getIdentifiers(); + return identifiers.containsKey(member.getMemberName()) + && identifiers.get(member.getMemberName()).equals(member.getTarget()); + } +} diff --git a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator index b3072519fac..a43408752c1 100644 --- a/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator +++ b/smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator @@ -26,6 +26,7 @@ software.amazon.smithy.model.validation.validators.IdempotencyTokenIgnoredValida software.amazon.smithy.model.validation.validators.JsonNameValidator software.amazon.smithy.model.validation.validators.LengthTraitValidator software.amazon.smithy.model.validation.validators.MediaTypeValidator +software.amazon.smithy.model.validation.validators.MemberShouldReferenceResourceValidator software.amazon.smithy.model.validation.validators.NoInlineDocumentSupportValidator software.amazon.smithy.model.validation.validators.OperationValidator software.amazon.smithy.model.validation.validators.PaginatedTraitValidator diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/missing-member-references.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/missing-member-references.errors new file mode 100644 index 00000000000..6b086ce8cfd --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/missing-member-references.errors @@ -0,0 +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 diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/missing-member-references.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/missing-member-references.smithy new file mode 100644 index 00000000000..4da9d4a65cd --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/missing-member-references.smithy @@ -0,0 +1,39 @@ +$version: "2.0" + +namespace ns.foo + +resource FooResource { + identifiers: { + fooId: String + } +} + +operation Operation1 { + input := { + fooId: String + } +} + +operation Operation2 { + input := @references([{ + resource: FooResource + }]) { + fooId: String + } +} + +resource BarResource { + identifiers: { + barId: BarId + } +} + +operation Operation3 { + input := @references([{ + resource: BarResource + }]) { + barId: BarId + } +} + +string BarId diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/valid-implicit-references.json b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/valid-implicit-references.json index 2811433602a..31dbfd79290 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/valid-implicit-references.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/references/valid-implicit-references.json @@ -19,9 +19,6 @@ }, "traits": { "smithy.api#references": [ - { - "resource": "ns.foo#MyResource" - }, { "resource": "ns.foo#MyResource" }, @@ -48,6 +45,9 @@ "smithy.api#references": [ { "resource": "ns.foo#MyResource" + }, + { + "resource": "ns.foo#MyResource2" } ] } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.errors index e649b0549ef..8a60535dd8e 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.errors @@ -1 +1,2 @@ [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 +[WARNING] smithy.example#GetFooInput$fooId: This member appears to reference the following resources without being included in a `@references` trait: [`smithy.example#Foo`] | MemberShouldReferenceResource diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-operation-binding-validator.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-operation-binding-validator.errors index 8a249199fb8..705ea9cbdce 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-operation-binding-validator.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-operation-binding-validator.errors @@ -1,2 +1,2 @@ -[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 +[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 [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 diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-operation-binding-validator.json b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-operation-binding-validator.json index 9bb2b0f1cd9..9508ffe816a 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-operation-binding-validator.json +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-operation-binding-validator.json @@ -83,7 +83,7 @@ "ns.foo#InvalidResource": { "type": "resource", "identifiers": { - "foo": { + "bar": { "target": "smithy.api#String" } }, @@ -129,7 +129,7 @@ "ns.foo#InvalidResourceOperation2Input": { "type": "structure", "members": { - "foo": { + "bar": { "target": "smithy.api#String", "traits": { "smithy.api#required": {}