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.
smithy-lang · Jul 21, 2023 · fb55e42 · fb55e42
1 parent f5d9df9
commit fb55e42
Show file tree

Hide file tree

Showing 6 changed files with 39 additions and 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
@@ -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 <http-trait>`
-      - 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 <httpError-trait>`
       - 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
@@ -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 <http-trait>`
-      - 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 <httpError-trait>`
       - 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
@@ -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 <http-trait>`
-      - 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 <httpError-trait>`
       - 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
@@ -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 <http-trait>`
-      - 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 <httpError-trait>`
       - 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/...java/software/amazon/smithy/model/validation/validators/HttpBindingsMissingValidator.java b/...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<ValidationEvent> validate(Model model) {
 
     private List<ValidationEvent> validateService(TopDownIndex topDownIndex, Model model, ServiceShape service) {
         Set<OperationShape> 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<ValidationEvent> validateOperations(
+            ServiceShape service,
+            Set<OperationShape> 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/...software/amazon/smithy/model/errorfiles/validators/http-bindings-missing-validator.errors b/...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