From 7813acbfee4e90b589996ffcfa02fbe73785f654 Mon Sep 17 00:00:00 2001 From: Kevin Stich Date: Wed, 18 Sep 2024 15:40:17 -0700 Subject: [PATCH] Add validator for service bound resoruce operations This commit introduces a new validator that emits warnings when it detects operations that are bound directly to a service when a resource bound to that service may be a better match. --- ...erviceBoundResourceOperationValidator.java | 83 +++++++++++++++++++ ...e.amazon.smithy.model.validation.Validator | 1 + .../detects-misbound-operations.errors | 1 + .../detects-misbound-operations.smithy | 53 ++++++++++++ 4 files changed, 138 insertions(+) create mode 100644 smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ServiceBoundResourceOperationValidator.java create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.errors create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.smithy diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ServiceBoundResourceOperationValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ServiceBoundResourceOperationValidator.java new file mode 100644 index 00000000000..f2cc8e44e96 --- /dev/null +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ServiceBoundResourceOperationValidator.java @@ -0,0 +1,83 @@ +/* + * 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.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.knowledge.OperationIndex; +import software.amazon.smithy.model.knowledge.TopDownIndex; +import software.amazon.smithy.model.shapes.MemberShape; +import software.amazon.smithy.model.shapes.OperationShape; +import software.amazon.smithy.model.shapes.ResourceShape; +import software.amazon.smithy.model.shapes.ServiceShape; +import software.amazon.smithy.model.shapes.ShapeId; +import software.amazon.smithy.model.traits.RequiredTrait; +import software.amazon.smithy.model.validation.AbstractValidator; +import software.amazon.smithy.model.validation.ValidationEvent; +import software.amazon.smithy.model.validation.ValidationUtils; + +/** + * Validates if operations that are bound directly to a service may + * be more accurately bound to a resource bound to the same service. + */ +public final class ServiceBoundResourceOperationValidator extends AbstractValidator { + @Override + public List validate(Model model) { + List events = new ArrayList<>(); + OperationIndex operationIndex = OperationIndex.of(model); + TopDownIndex topDownIndex = TopDownIndex.of(model); + + // Check every service operation to see if it should be bound to a resource instead. + for (ServiceShape service : model.getServiceShapes()) { + // Store potential targets to emit one event per operation. + Map> potentiallyBetterBindings = new HashMap<>(); + for (ShapeId operationId : service.getOperations()) { + OperationShape operation = model.expectShape(operationId, OperationShape.class); + // Check the resources of the containing service to test for input/output attachment. + for (ResourceShape resource : topDownIndex.getContainedResources(service)) { + // Check the operation members to see if they are implicit matches for resource identifiers. + for (MemberShape member : operationIndex.getInputMembers(operation).values()) { + if (isImplicitIdentifierBinding(member, resource)) { + potentiallyBetterBindings.computeIfAbsent(operation, k -> new HashSet<>()) + .add(resource.getId()); + } + } + for (MemberShape member : operationIndex.getOutputMembers(operation).values()) { + if (isImplicitIdentifierBinding(member, resource)) { + potentiallyBetterBindings.computeIfAbsent(operation, k -> new HashSet<>()) + .add(resource.getId()); + } + } + } + } + + // Emit events per service that's present with a potentially bad binding. + for (Map.Entry> entry : potentiallyBetterBindings.entrySet()) { + events.add(warning(entry.getKey(), service, format( + "The `%s` operation is bound to the `%s` service but has members that match identifiers " + + "of the following resource shapes: [%s]. It may be more accurately bound to one " + + "of them than directly to the service.", + entry.getKey().getId(), service.getId(), ValidationUtils.tickedList(entry.getValue())), + service.getId().toString(), entry.getKey().getId().getName())); + } + } + + return events; + } + + private boolean isImplicitIdentifierBinding(MemberShape member, ResourceShape resource) { + return resource.getIdentifiers().containsKey(member.getMemberName()) + && member.getTrait(RequiredTrait.class).isPresent() + && member.getTarget().equals(resource.getIdentifiers().get(member.getMemberName())); + } +} 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 2c4af42885c..b3072519fac 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 @@ -40,6 +40,7 @@ software.amazon.smithy.model.validation.validators.ResourceIdentifierValidator software.amazon.smithy.model.validation.validators.ResourceLifecycleValidator software.amazon.smithy.model.validation.validators.ResourceOperationInputOutputValidator software.amazon.smithy.model.validation.validators.ServiceAuthDefinitionsValidator +software.amazon.smithy.model.validation.validators.ServiceBoundResourceOperationValidator software.amazon.smithy.model.validation.validators.ServiceValidator software.amazon.smithy.model.validation.validators.SetValidator software.amazon.smithy.model.validation.validators.ShapeIdConflictValidator 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 new file mode 100644 index 00000000000..e649b0549ef --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.errors @@ -0,0 +1 @@ +[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 diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.smithy new file mode 100644 index 00000000000..196a457c128 --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/resource-identifiers/detects-misbound-operations.smithy @@ -0,0 +1,53 @@ +$version: "2.0" + +namespace smithy.example + +service FooService { + version: "2020-07-02" + operations: [GetFoo] + resources: [Foo, BoundNoMatch] +} + +resource Foo { + identifiers: { + fooId: String + } + create: CreateFoo +} + +operation CreateFoo { + input := { + @required + something: String + } + output := { + @required + fooId: String + + @required + something: String + } +} + +operation GetFoo { + input := { + @required + fooId: String + } + output := { + @required + something: String + } +} + +resource Unbound { + identifiers: { + id: String + } +} + +resource BoundNoMatch { + identifiers: { + id: String + } +}