Repository: smithy-lang/smithy (java)

Issue:
Protocols that support the `http` trait do not enforce that it must be attached to all operation shapes
As far as I'm aware, all protocols that support the `http` binding trait need to have it attached to all their `operation` shapes, which are routable using the required `http.uri` member.

(Aside: the above is not explicitly stated in the protocols' documentation pages)

However, the following model is accepted by Smithy (it's accepted with `restJson` too):

```smithy
$version: "2.0"

namespace com.amazonaws.simple

use aws.protocols#restXml

@restXml
service SimpleService {
    version: "2022-01-01",
    operations: [
        SimpleOperation,
    ],
}

//@http(uri: "/simple-operation", method: "GET")
operation SimpleOperation {
    output: SimpleOperationOutput,
}

structure SimpleOperationOutput {
    @required
    requiredString: String,
}
```


Pull request:
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.

Resolves #1438 


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Patch:
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 eb65d62de..0c47eb3ba 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 <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
index ab971cad3..ecf98f6d8 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 <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
index a383077c1..00ac52cfc 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 <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
index cb32bee0a..e266fc653 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 <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/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 a51072c05..fabe8dcfc 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.ServiceIndex;
 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 final class HttpBindingsMissingValidator extends AbstractValidator {
 
     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());
     }
 }


Test patch:
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 1375a56da..d63edc77a 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