Skip to content

Commit

Permalink
Add validator for service bound resoruce operations
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
kstich committed Sep 18, 2024
1 parent e334862 commit 0be0afb
Show file tree
Hide file tree
Showing 4 changed files with 138 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -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<ValidationEvent> validate(Model model) {
List<ValidationEvent> 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<OperationShape, Set<ShapeId>> 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<OperationShape, Set<ShapeId>> 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()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
}
}

0 comments on commit 0be0afb

Please sign in to comment.