Repository: smithy-lang/smithy
Pull request #2433 (2024-10-24 16:15:19)

Update `NullAndEmptyHeadersServer` tests for `restJson1` and `restXml`

#### Background
* What do these changes do? 
* Why are they important?

`NullAndEmptyHeaderClient` tests were [recently updated](https://github.com/smithy-lang/smithy/pull/2403) to expect empty headers to be serialized. Similar changes were not made for the server variants of those tests. This leads to an unexpected difference in serialization between servers and clients and prevents clients from receiving semantically meaningful headers from the server.

The test now align with the similar tests for clients. They expect empty headers to be serialized as "" and null headers to not be serialized

#### Testing
* How did you test these changes?
Confirmed that the same changes made for client header serialization in https://github.com/smithy-lang/smithy-rs/pull/3887 worked for the server tests as well.

#### Links
* Links to additional context, if necessary
* Issue #, if applicable (see [here](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword) for a list of keywords to use for linking issues)

---
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/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedMemberTarget.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedMemberTarget.java
index 73ce12033..f53cb1eb1 100644
--- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedMemberTarget.java
+++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedMemberTarget.java
@@ -25,9 +25,11 @@ import software.amazon.smithy.diff.ChangedShape;
 import software.amazon.smithy.diff.Differences;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.shapes.CollectionShape;
+import software.amazon.smithy.model.shapes.MapShape;
 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.ShapeType;
 import software.amazon.smithy.model.shapes.SimpleShape;
 import software.amazon.smithy.model.traits.EnumTrait;
 import software.amazon.smithy.model.traits.Trait;
@@ -99,7 +101,7 @@ public final class ChangedMemberTarget extends AbstractDiffEvaluator {
                                               oldShape.getType(), newShape.getType()));
         }
 
-        if (!(oldShape instanceof SimpleShape || oldShape instanceof CollectionShape)) {
+        if (!(oldShape instanceof SimpleShape || oldShape instanceof CollectionShape || oldShape instanceof MapShape)) {
             return ListUtils.of(String.format("The name of a %s is significant", oldShape.getType()));
         }
 
@@ -117,25 +119,42 @@ public final class ChangedMemberTarget extends AbstractDiffEvaluator {
         }
 
         if (oldShape instanceof CollectionShape) {
-            MemberShape oldMember = ((CollectionShape) oldShape).getMember();
-            MemberShape newMember = ((CollectionShape) newShape).getMember();
-            if (!oldMember.getTarget().equals(newMember.getTarget())) {
-                results.add(String.format("Both the old and new shapes are a %s, but the old shape targeted "
-                                          + "`%s` while the new shape targets `%s`",
-                                          oldShape.getType(),
-                                          oldMember.getTarget(),
-                                          newMember.getTarget()));
-            } else if (!oldMember.getAllTraits().equals(newMember.getAllTraits())) {
-                results.add(String.format("Both the old and new shapes are a %s, but their members have "
-                                          + "differing traits. %s",
-                                          oldShape.getType(),
-                                          createTraitDiffMessage(oldMember, newMember)));
-            }
+            evaluateMember(oldShape.getType(), results,
+                    ((CollectionShape) oldShape).getMember(),
+                    ((CollectionShape) newShape).getMember());
+        } else if (oldShape instanceof MapShape) {
+            MapShape oldMapShape = (MapShape) oldShape;
+            MapShape newMapShape = (MapShape) newShape;
+            // Both the key and value need to be evaluated for maps.
+            evaluateMember(oldShape.getType(), results,
+                    oldMapShape.getKey(),
+                    newMapShape.getKey());
+            evaluateMember(oldShape.getType(), results,
+                    oldMapShape.getValue(),
+                    newMapShape.getValue());
         }
 
         return results;
     }
 
+    private static void evaluateMember(
+            ShapeType oldShapeType,
+            List<String> results,
+            MemberShape oldMember,
+            MemberShape newMember
+    ) {
+        String memberSlug = oldShapeType == ShapeType.MAP ? oldMember.getMemberName() + " " : "";
+        if (!oldMember.getTarget().equals(newMember.getTarget())) {
+            results.add(String.format("Both the old and new shapes are a %s, but the old shape %stargeted "
+                            + "`%s` while the new shape targets `%s`",
+                    oldShapeType, memberSlug, oldMember.getTarget(), newMember.getTarget()));
+        } else if (!oldMember.getAllTraits().equals(newMember.getAllTraits())) {
+            results.add(String.format("Both the old and new shapes are a %s, but their %smembers have "
+                            + "differing traits. %s",
+                    oldShapeType, memberSlug, createTraitDiffMessage(oldMember, newMember)));
+        }
+    }
+
     private static String createSimpleMessage(ChangedShape<MemberShape> change, Shape oldTarget, Shape newTarget) {
         return String.format(
                 "The shape targeted by the member `%s` changed from `%s` (%s) to `%s` (%s). ",


Test patch:
diff --git a/smithy-aws-protocol-tests/model/restJson1/http-headers.smithy b/smithy-aws-protocol-tests/model/restJson1/http-headers.smithy
index 78fd9c909..2b5115004 100644
--- a/smithy-aws-protocol-tests/model/restJson1/http-headers.smithy
+++ b/smithy-aws-protocol-tests/model/restJson1/http-headers.smithy
@@ -412,7 +412,7 @@ structure InputAndOutputWithHeadersIO {
     headerIntegerEnumList: IntegerEnumList,
 }
 
-/// Null and empty headers are not sent over the wire.
+/// Null headers are not sent over the wire, empty headers are serialized to ""
 @readonly
 @http(uri: "/NullAndEmptyHeadersClient", method: "GET")
 @tags(["client-only"])
@@ -443,7 +443,7 @@ apply NullAndEmptyHeadersClient @httpRequestTests([
     },
 ])
 
-/// Null and empty headers are not sent over the wire.
+/// Null headers are not sent over the wire, empty headers are serialized to ""
 @readonly
 @http(uri: "/NullAndEmptyHeadersServer", method: "GET")
 @tags(["server-only"])
@@ -455,10 +455,14 @@ operation NullAndEmptyHeadersServer {
 apply NullAndEmptyHeadersServer @httpResponseTests([
     {
         id: "RestJsonNullAndEmptyHeaders",
-        documentation: "Do not send null or empty headers",
+        documentation: "Do not send null values, but do send empty strings and empty lists over the wire in headers",
         protocol: restJson1,
         code: 200,
-        forbidHeaders: ["X-A", "X-B", "X-C"],
+        forbidHeaders: ["X-A"],
+        headers: {
+            "X-B": ""
+            "X-C": ""
+        }
         params: {
             a: null,
             b: "",
diff --git a/smithy-aws-protocol-tests/model/restXml/http-headers.smithy b/smithy-aws-protocol-tests/model/restXml/http-headers.smithy
index 2bf49ffc9..6fc91964d 100644
--- a/smithy-aws-protocol-tests/model/restXml/http-headers.smithy
+++ b/smithy-aws-protocol-tests/model/restXml/http-headers.smithy
@@ -355,7 +355,7 @@ structure InputAndOutputWithHeadersIO {
     headerEnumList: FooEnumList,
 }
 
-/// Null and empty headers are not sent over the wire.
+/// Null headers are not sent over the wire, empty headers are serialized to ""
 @readonly
 @http(uri: "/NullAndEmptyHeadersClient", method: "GET")
 @tags(["client-only"])
@@ -386,7 +386,7 @@ apply NullAndEmptyHeadersClient @httpRequestTests([
     },
 ])
 
-/// Null and empty headers are not sent over the wire.
+/// Null headers are not sent over the wire, empty headers are serialized to ""
 @readonly
 @http(uri: "/NullAndEmptyHeadersServer", method: "GET")
 @tags(["server-only"])
@@ -398,10 +398,14 @@ operation NullAndEmptyHeadersServer {
 apply NullAndEmptyHeadersServer @httpResponseTests([
     {
         id: "NullAndEmptyHeaders",
-        documentation: "Do not send null or empty headers",
+        documentation: "Do not send null values, but do send empty strings and empty lists over the wire in headers",
         protocol: restXml,
         code: 200,
-        forbidHeaders: ["X-A", "X-B", "X-C"],
+        forbidHeaders: ["X-A"],
+        headers: {
+            "X-B": ""
+            "X-C": ""
+        }
         body: "",
         params: {
             a: null,
diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java
index a405893f1..c2613028f 100644
--- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java
+++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java
@@ -221,6 +221,26 @@ public class ChangedMemberTargetTest {
                            + "backward compatible."));
     }
 
+    @Test
+    public void detectsAcceptableMapMemberChangesInNestedTargets() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-valid-nested2-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-valid-nested2-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, Severity.WARNING).size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").get(0).getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). This was determined "
+                        + "backward compatible."));
+    }
+
     @Test
     public void detectsInvalidListMemberChangesInNestedTargets() {
         Model modelA = Model.assembler()
@@ -264,4 +284,92 @@ public class ChangedMemberTargetTest {
                            + "shapes are a list, but the old shape targeted `smithy.example#MyString` while "
                            + "the new shape targets `smithy.example#MyString2`."));
     }
+
+    @Test
+    public void detectsInvalidMapKeyChangesInNestedTargets() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey1-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey1-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
+        assertThat(event.getSeverity(), equalTo(Severity.ERROR));
+        assertThat(event.getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
+                        + "shapes are a map, but their key members have differing traits. The newly targeted "
+                        + "shape now has the following additional traits: [smithy.api#pattern]."));
+    }
+
+    @Test
+    public void detectsInvalidMapKeyTargetChange() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey2-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey2-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
+        assertThat(event.getSeverity(), equalTo(Severity.ERROR));
+        assertThat(event.getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
+                        + "shapes are a map, but the old shape key targeted `smithy.example#MyString` while "
+                        + "the new shape targets `smithy.example#MyString2`."));
+    }
+
+    @Test
+    public void detectsInvalidMapValueChangesInNestedTargets() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue1-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue1-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
+        assertThat(event.getSeverity(), equalTo(Severity.ERROR));
+        assertThat(event.getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
+                        + "shapes are a map, but their value members have differing traits. The newly targeted "
+                        + "shape now has the following additional traits: [smithy.api#pattern]."));
+    }
+
+    @Test
+    public void detectsInvalidMapValueTargetChange() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue2-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue2-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
+        assertThat(event.getSeverity(), equalTo(Severity.ERROR));
+        assertThat(event.getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
+                        + "shapes are a map, but the old shape value targeted `smithy.example#MyString` while "
+                        + "the new shape targets `smithy.example#MyString2`."));
+    }
 }
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-a.smithy
@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-b.smithy
new file mode 100644
index 000000000..100537818
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-b.smithy
@@ -0,0 +1,17 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    @pattern("^[a-z]+$")
+    key: MyString
+
+    value: MyString
+}
+
+string MyString
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-a.smithy
@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-b.smithy
new file mode 100644
index 000000000..fee26f65e
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-b.smithy
@@ -0,0 +1,17 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    key: MyString2
+    value: MyString
+}
+
+string MyString
+
+string MyString2
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-a.smithy
@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-b.smithy
new file mode 100644
index 000000000..97b5fbbcd
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-b.smithy
@@ -0,0 +1,17 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    key: MyString
+
+    @pattern("^[a-z]+$")
+    value: MyString
+}
+
+string MyString
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-a.smithy
@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-b.smithy
new file mode 100644
index 000000000..77a42f2aa
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-b.smithy
@@ -0,0 +1,17 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    key: MyString
+    value: MyString2
+}
+
+string MyString
+
+string MyString2
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-a.smithy
@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-b.smithy
new file mode 100644
index 000000000..0e7f9eb06
--- /dev/null
+++ b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-b.smithy
@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    key: MyString
+    value: MyString
+}
+
+string MyString