Repository: smithy-lang/smithy
Pull request #1969 (2023-09-06 06:22:21)

Fix ignored binding checks for mixins
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-2.0/aws/aws-auth.rst b/docs/source-2.0/aws/aws-auth.rst
index 4bfaf6db7..cad432300 100644
--- a/docs/source-2.0/aws/aws-auth.rst
+++ b/docs/source-2.0/aws/aws-auth.rst
@@ -6,12 +6,6 @@ AWS Authentication Traits
 
 This document defines AWS authentication schemes.
 
-If a request contains the ``Authorization`` header or a query string parameter
-with the name of ``X-Amz-Algorithm`` containing the value ``AWS4-HMAC-SHA256``,
-the request will undergo authentication and be rejected if it fails. Otherwise,
-if the :ref:`optionalAuth-trait` is applied, the service shall operate on the
-unauthenticated request.
-
 .. smithy-trait:: aws.auth#sigv4
 .. _aws.auth#sigv4-trait:
 
@@ -41,6 +35,12 @@ Trait value
             NOT be empty. This value SHOULD match the ``arnNamespace`` property
             of the :ref:`aws.api#service-trait`.
 
+If a request contains the ``Authorization`` header or a query string parameter
+with the name of ``X-Amz-Algorithm`` containing the value ``AWS4-HMAC-SHA256``,
+the request will undergo authentication and be rejected if it fails. Otherwise,
+if the :ref:`optionalAuth-trait` is applied, the service shall operate on the
+unauthenticated request.
+
 .. code-block:: smithy
 
     $version: "2"
diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java
index 0831aaae4..8d2963ee5 100644
--- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java
+++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedNullability.java
@@ -110,15 +110,15 @@ public class ChangedNullability extends AbstractDiffEvaluator {
             eventsToAdd.add(emit(Severity.DANGER, "AddedInputTrait", shape, message,
                             "The @input trait was added to " + newMember.getContainer()));
         } else if (!newHasInput) {
-            // Can't add nullable to a preexisting required member.
+            // Can't add clientOptional to a preexisting required member.
             if (change.isTraitAdded(ClientOptionalTrait.ID) && change.isTraitInBoth(RequiredTrait.ID)) {
-                eventsToAdd.add(emit(Severity.ERROR, "AddedNullableTrait", shape, message,
-                                "The @nullable trait was added to a @required member."));
+                eventsToAdd.add(emit(Severity.ERROR, "AddedClientOptionalTrait", shape, message,
+                                "The @clientOptional trait was added to a @required member."));
             }
-            // Can't add required to a member unless the member is marked as nullable.
+            // Can't add required to a member unless the member is marked as @clientOptional or part of @input.
             if (change.isTraitAdded(RequiredTrait.ID) && !newMember.hasTrait(ClientOptionalTrait.ID)) {
                 eventsToAdd.add(emit(Severity.ERROR, "AddedRequiredTrait", shape, message,
-                                "The @required trait was added to a member that is not marked as @nullable."));
+                                "The @required trait was added to a member."));
             }
             // Can't add the default trait to a member unless the member was previously required.
             if (change.isTraitAdded(DefaultTrait.ID) && !change.isTraitRemoved(RequiredTrait.ID)) {
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingTraitIgnoredValidator.java b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingTraitIgnoredValidator.java
index 78d2243ba..873cb35d7 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingTraitIgnoredValidator.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/HttpBindingTraitIgnoredValidator.java
@@ -22,6 +22,7 @@ import software.amazon.smithy.model.neighbor.NeighborProvider;
 import software.amazon.smithy.model.neighbor.Relationship;
 import software.amazon.smithy.model.neighbor.RelationshipType;
 import software.amazon.smithy.model.shapes.MemberShape;
+import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.shapes.StructureShape;
 import software.amazon.smithy.model.traits.HttpHeaderTrait;
@@ -31,6 +32,7 @@ import software.amazon.smithy.model.traits.HttpPrefixHeadersTrait;
 import software.amazon.smithy.model.traits.HttpQueryParamsTrait;
 import software.amazon.smithy.model.traits.HttpQueryTrait;
 import software.amazon.smithy.model.traits.HttpResponseCodeTrait;
+import software.amazon.smithy.model.traits.MixinTrait;
 import software.amazon.smithy.model.traits.Trait;
 import software.amazon.smithy.model.validation.AbstractValidator;
 import software.amazon.smithy.model.validation.ValidationEvent;
@@ -70,6 +72,12 @@ public class HttpBindingTraitIgnoredValidator extends AbstractValidator {
         List<ValidationEvent> events = new ArrayList<>();
         NeighborProvider reverse = NeighborProviderIndex.of(model).getReverseProvider();
         for (MemberShape memberShape : model.getMemberShapes()) {
+            Shape container = model.expectShape(memberShape.getContainer());
+            // Skip non-structures (invalid) and mixins (handled at mixed site).
+            if (!container.isStructureShape() || container.hasTrait(MixinTrait.class)) {
+                continue;
+            }
+
             // Gather all traits that are HTTP member binding.
             // Keep the trait instance around so that it can be used it later for source location.
             Map<ShapeId, Trait> traits = new HashMap<>();
@@ -82,11 +90,9 @@ public class HttpBindingTraitIgnoredValidator extends AbstractValidator {
             // The traits set is now the HTTP binding traits that are ignored outside
             // the top level of an operation's components.
             if (!traits.isEmpty()) {
-                StructureShape containerShape = model.expectShape(memberShape.getContainer(), StructureShape.class);
-
                 // All relationships and trait possibilities are checked at once to de-duplicate
                 // several parts of the iteration logic.
-                events.addAll(checkRelationships(containerShape, memberShape, traits, reverse));
+                events.addAll(checkRelationships(container.asStructureShape().get(), memberShape, traits, reverse));
             }
         }
         return events;
@@ -151,19 +157,16 @@ public class HttpBindingTraitIgnoredValidator extends AbstractValidator {
         // immediate grabbing of next traits is all that's necessary.
         if (!ignoredRelationships.isEmpty()) {
             if (!ignoredOutsideInputTraits.isEmpty()) {
-                ShapeId traitId = ignoredOutsideInputTraits.iterator().next();
-                events.add(emit("Input", memberShape, traits, traitId,
-                        checkedRelationshipCount, ignoredRelationships));
+                Trait trait = traits.get(ignoredOutsideInputTraits.iterator().next());
+                events.add(emit("Input", memberShape, trait, checkedRelationshipCount, ignoredRelationships));
 
             } else if (!ignoredOutsideOutputTraits.isEmpty()) {
-                ShapeId traitId = ignoredOutsideOutputTraits.iterator().next();
-                events.add(emit("Output", memberShape, traits, traitId,
-                        checkedRelationshipCount, ignoredRelationships));
+                Trait trait = traits.get(ignoredOutsideOutputTraits.iterator().next());
+                events.add(emit("Output", memberShape, trait, checkedRelationshipCount, ignoredRelationships));
             } else {
                 // The traits list is always non-empty here, so just grab the first.
-                ShapeId traitId = traits.keySet().iterator().next();
-                events.add(emit("TopLevel", memberShape, traits, traitId,
-                        checkedRelationshipCount, ignoredRelationships));
+                Trait trait = traits.get(traits.keySet().iterator().next());
+                events.add(emit("TopLevel", memberShape, trait, checkedRelationshipCount, ignoredRelationships));
             }
         }
 
@@ -180,18 +183,18 @@ public class HttpBindingTraitIgnoredValidator extends AbstractValidator {
     private ValidationEvent emit(
             String type,
             MemberShape memberShape,
-            Map<ShapeId, Trait> traits,
-            ShapeId traitId,
+            Trait trait,
             int checkedRelationshipCount,
             Map<RelationshipType, List<ShapeId>> ignoredRelationships
     ) {
-        String message = "The `%s` trait applied to this member is ";
+        String mixedIn = memberShape.getMixins().isEmpty() ? "" : " mixed in";
+        String message = "The `%s` trait applied to this%s member is ";
         if (checkedRelationshipCount == ignoredRelationships.size()) {
             message += "ignored in all contexts.";
         } else {
-            message += format("ignored in some contexts: %s", formatIgnoredRelationships(ignoredRelationships));
+            message += "ignored in some contexts: " + formatIgnoredRelationships(ignoredRelationships);
         }
-        return warning(memberShape, traits.get(traitId), format(message, Trait.getIdiomaticTraitName(traitId)), type);
+        return warning(memberShape, trait, format(message, Trait.getIdiomaticTraitName(trait), mixedIn), type);
     }
 
     private String formatIgnoredRelationships(Map<RelationshipType, List<ShapeId>> ignoredRelationships) {


Test patch:
diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java
index b18d07acc..0205a2990 100644
--- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java
+++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java
@@ -159,13 +159,12 @@ public class ChangedNullabilityTest {
         assertThat(events.stream()
                            .filter(event -> event.getSeverity() == Severity.ERROR)
                            .filter(event -> event.getId().equals("ChangedNullability.AddedRequiredTrait"))
-                           .filter(event -> event.getMessage().contains("The @required trait was added to a member "
-                                                                        + "that is not marked as @nullable"))
+                           .filter(event -> event.getMessage().contains("The @required trait was added to a member"))
                            .count(), equalTo(1L));
     }
 
     @Test
-    public void detectAdditionOfNullableTrait() {
+    public void detectAdditionOfClientOptionalTrait() {
         MemberShape member1 = MemberShape.builder()
                 .id("foo.baz#Baz$bam")
                 .target("foo.baz#String")
@@ -181,8 +180,8 @@ public class ChangedNullabilityTest {
 
         assertThat(events.stream()
                            .filter(event -> event.getSeverity() == Severity.ERROR)
-                           .filter(event -> event.getId().equals("ChangedNullability.AddedNullableTrait"))
-                           .filter(event -> event.getMessage().contains("The @nullable trait was added to a "
+                           .filter(event -> event.getId().equals("ChangedNullability.AddedClientOptionalTrait"))
+                           .filter(event -> event.getMessage().contains("The @clientOptional trait was added to a "
                                                                         + "@required member"))
                            .count(), equalTo(1L));
     }
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.errors b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.errors
index 104a2887e..6dee4e855 100644
--- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.errors
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.errors
@@ -15,3 +15,6 @@
 [WARNING] smithy.example#NestedBindings$stringMap2: The `httpPrefixHeaders` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings]} | HttpBindingTraitIgnored.TopLevel
 [WARNING] smithy.example#NestedBindings$code: The `httpResponseCode` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings], input: [smithy.example#NestedBindingOperation]} | HttpBindingTraitIgnored.Output
 [WARNING] smithy.example#NestedBindings$payload: The `httpPayload` trait applied to this member is ignored in some contexts: {member target: [smithy.example#IgnoredBindingOperationInput$nestedBindings, smithy.example#IgnoredBindingOperationOutput$nestedBindings]} | HttpBindingTraitIgnored.TopLevel
+[WARNING] smithy.example#MixedBindingOperationOutput$query: The `httpQuery` trait applied to this mixed in member is ignored in all contexts. | HttpBindingTraitIgnored.Input
+[WARNING] smithy.example#MixedBindingOperationInput$code: The `httpResponseCode` trait applied to this mixed in member is ignored in all contexts. | HttpBindingTraitIgnored.Output
+[WARNING] smithy.example#MixedBindingOperationOutput$code: The `httpResponseCode` trait applied to this mixed in member is ignored in some contexts: {member target: [smithy.example#MixedBindingOperationInput$content]} | HttpBindingTraitIgnored.Output
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.smithy
index 1200bad37..965c24fe6 100644
--- a/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.smithy
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/http-binding-trait-ignored.smithy
@@ -3,11 +3,12 @@ $version: "2"
 namespace smithy.example
 
 service InvalidExample {
-    version: "2020-12-29",
+    version: "2020-12-29"
     operations: [
         IgnoredBindingOperation
         NestedBindingOperation
-    ],
+        MixedBindingOperation
+    ]
 }
 
 @http(method: "POST", uri: "/ignored-binding")
@@ -77,3 +78,27 @@ map StringMap {
     key: String
     value: String
 }
+
+@http(method: "POST", uri: "/mixed-binding")
+operation MixedBindingOperation {
+    input: MixedBindingOperationInput
+    output: MixedBindingOperationOutput
+}
+
+structure MixedBindingOperationInput with [MixedQuery, MixedResponseCode] {
+    content: MixedBindingOperationOutput
+}
+
+structure MixedBindingOperationOutput with [MixedQuery, MixedResponseCode] {}
+
+@mixin
+structure MixedQuery {
+    @httpQuery("query")
+    query: String
+}
+
+@mixin
+structure MixedResponseCode {
+    @httpResponseCode
+    code: Integer
+}