From fb55e42c8d42ff6624f279c4bdfaca5fa3215ae5 Mon Sep 17 00:00:00 2001 From: kstich Date: Thu, 20 Jul 2023 14:35:11 -0700 Subject: [PATCH] Improve validation for http binding protocols This commit updates validation of protocols that support the `http` trait to cover a case where no operations on a service with the protocol applied had the `http` trait was valid. --- .../spec/aws/aws-restjson1-protocol.rst | 5 +- .../spec/aws/aws-restxml-protocol.rst | 5 +- .../aws/protocols/aws-restjson1-protocol.rst | 5 +- .../aws/protocols/aws-restxml-protocol.rst | 5 +- .../HttpBindingsMissingValidator.java | 47 ++++++++++++------- .../http-bindings-missing-validator.errors | 4 +- 6 files changed, 39 insertions(+), 32 deletions(-) diff --git a/docs/source-1.0/spec/aws/aws-restjson1-protocol.rst b/docs/source-1.0/spec/aws/aws-restjson1-protocol.rst index eb65d62de25..0c47eb3bad8 100644 --- a/docs/source-1.0/spec/aws/aws-restjson1-protocol.rst +++ b/docs/source-1.0/spec/aws/aws-restjson1-protocol.rst @@ -154,9 +154,8 @@ that affect serialization: - Binds a top-level operation input structure member to a label in the hostPrefix of an endpoint trait. * - :ref:`http ` - - Configures the HTTP bindings of an operation. An operation that - does not define the ``http`` trait is ineligible for use with - this protocol. + - Configures the HTTP bindings of an operation. An operation bound to a + service with this protocol applied MUST have the ``http`` trait applied. * - :ref:`httpError ` - A ``client`` error has a default status code of ``400``, and a ``server`` error has a default status code of ``500``. The diff --git a/docs/source-1.0/spec/aws/aws-restxml-protocol.rst b/docs/source-1.0/spec/aws/aws-restxml-protocol.rst index ab971cad307..ecf98f6d87e 100644 --- a/docs/source-1.0/spec/aws/aws-restxml-protocol.rst +++ b/docs/source-1.0/spec/aws/aws-restxml-protocol.rst @@ -150,9 +150,8 @@ that affect serialization: - Binds a top-level operation input structure member to a label in the hostPrefix of an endpoint trait. * - :ref:`http ` - - Configures the HTTP bindings of an operation. An operation that - does not define the ``http`` trait is ineligible for use with - this protocol. + - Configures the HTTP bindings of an operation. An operation bound to a + service with this protocol applied MUST have the ``http`` trait applied. * - :ref:`httpError ` - A ``client`` error has a default status code of ``400``, and a ``server`` error has a default status code of ``500``. The diff --git a/docs/source-2.0/aws/protocols/aws-restjson1-protocol.rst b/docs/source-2.0/aws/protocols/aws-restjson1-protocol.rst index a383077c133..00ac52cfc55 100644 --- a/docs/source-2.0/aws/protocols/aws-restjson1-protocol.rst +++ b/docs/source-2.0/aws/protocols/aws-restjson1-protocol.rst @@ -146,9 +146,8 @@ that affect serialization: - Binds a top-level operation input structure member to a label in the hostPrefix of an endpoint trait. * - :ref:`http ` - - Configures the HTTP bindings of an operation. An operation that - does not define the ``http`` trait is ineligible for use with - this protocol. + - Configures the HTTP bindings of an operation. An operation bound to a + service with this protocol applied MUST have the ``http`` trait applied. * - :ref:`httpError ` - A ``client`` error has a default status code of ``400``, and a ``server`` error has a default status code of ``500``. The diff --git a/docs/source-2.0/aws/protocols/aws-restxml-protocol.rst b/docs/source-2.0/aws/protocols/aws-restxml-protocol.rst index cb32bee0a40..e266fc65311 100644 --- a/docs/source-2.0/aws/protocols/aws-restxml-protocol.rst +++ b/docs/source-2.0/aws/protocols/aws-restxml-protocol.rst @@ -141,9 +141,8 @@ that affect serialization: - Binds a top-level operation input structure member to a label in the hostPrefix of an endpoint trait. * - :ref:`http ` - - Configures the HTTP bindings of an operation. An operation that - does not define the ``http`` trait is ineligible for use with - this protocol. + - Configures the HTTP bindings of an operation. An operation bound to a + service with this protocol applied MUST have the ``http`` trait applied. * - :ref:`httpError ` - A ``client`` error has a default status code of ``400``, and a ``server`` error has a default status code of ``500``. The diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingsMissingValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingsMissingValidator.java index a51072c05c9..fabe8dcfc19 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingsMissingValidator.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingsMissingValidator.java @@ -24,12 +24,14 @@ import software.amazon.smithy.model.knowledge.TopDownIndex; import software.amazon.smithy.model.shapes.OperationShape; import software.amazon.smithy.model.shapes.ServiceShape; +import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.traits.HttpTrait; import software.amazon.smithy.model.traits.ProtocolDefinitionTrait; import software.amazon.smithy.model.validation.AbstractValidator; import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.model.validation.ValidationEvent; import software.amazon.smithy.utils.ListUtils; +import software.amazon.smithy.utils.Pair; /** * Validates that if any operation in a service uses the http trait, @@ -50,37 +52,46 @@ public List validate(Model model) { private List validateService(TopDownIndex topDownIndex, Model model, ServiceShape service) { Set operations = topDownIndex.getContainedOperations(service); + + ShapeId protocolTraitId = protocolWithBindings(service, model); + if (protocolTraitId != null) { + // Emit ERROR events if any of the protocols for the service support HTTP traits. + String reason = "The `" + protocolTraitId + "` protocol requires all"; + return validateOperations(service, operations, Severity.ERROR, reason); + } + // Stop early if there are no bindings at all in the model for any operation. if (operations.stream().noneMatch(this::hasBindings)) { return ListUtils.of(); } - Severity severity = determineSeverity(service, model); - - return operations.stream() - .filter(shape -> !shape.getTrait(HttpTrait.class).isPresent()) - .map(shape -> createEvent(severity, service, shape)) - .collect(Collectors.toList()); + return validateOperations(service, operations, Severity.WARNING, "One or more"); } - // Only emit ERROR events if any of the protocols for the service support HTTP traits - private Severity determineSeverity(ServiceShape service, Model model) { - if (ServiceIndex.of(model).getProtocols(service).values().stream() + private ShapeId protocolWithBindings(ServiceShape service, Model model) { + return ServiceIndex.of(model).getProtocols(service).values().stream() .map(t -> model.expectShape(t.toShapeId())) - .map(s -> s.expectTrait(ProtocolDefinitionTrait.class)) - .anyMatch(pdt -> pdt.getTraits().contains(HttpTrait.ID))) { - return Severity.ERROR; - } - return Severity.WARNING; + .map(s -> Pair.of(s.getId(), s.expectTrait(ProtocolDefinitionTrait.class))) + .filter(pair -> pair.getRight().getTraits().contains(HttpTrait.ID)) + .map(Pair::getLeft) + .findFirst().orElse(null); } private boolean hasBindings(OperationShape op) { return op.getTrait(HttpTrait.class).isPresent(); } - private ValidationEvent createEvent(Severity severity, ServiceShape service, OperationShape operation) { - return createEvent(severity, operation, operation.getSourceLocation(), String.format( - "One or more operations in the `%s` service define the `http` trait, but this " - + "operation is missing the `http` trait.", service.getId())); + private List validateOperations( + ServiceShape service, + Set operations, + Severity severity, + String reason + ) { + return operations.stream() + .filter(operation -> !operation.getTrait(HttpTrait.class).isPresent()) + .map(operation -> createEvent(severity, operation, operation.getSourceLocation(), + String.format("%s operations in the `%s` service define the `http` trait, but this " + + "operation is missing the `http` trait.", reason, service.getId()))) + .collect(Collectors.toList()); } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-bindings-missing-validator.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-bindings-missing-validator.errors index 1375a56da9c..d63edc77aa1 100644 --- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-bindings-missing-validator.errors +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-bindings-missing-validator.errors @@ -1,2 +1,2 @@ -[ERROR] ns.foo#MissingBindings1: One or more operations in the `ns.foo#MyService` service define the `http` trait, but this operation is missing the `http` trait. | HttpBindingsMissing -[ERROR] ns.foo#MissingBindings2: One or more operations in the `ns.foo#MyService` service define the `http` trait, but this operation is missing the `http` trait. | HttpBindingsMissing +[ERROR] ns.foo#MissingBindings1: The `ns.foo#restProtocol` protocol requires all operations in the `ns.foo#MyService` service define the `http` trait, but this operation is missing the `http` trait. | HttpBindingsMissing +[ERROR] ns.foo#MissingBindings2: The `ns.foo#restProtocol` protocol requires all operations in the `ns.foo#MyService` service define the `http` trait, but this operation is missing the `http` trait. | HttpBindingsMissing