Add support for marshalling lists of strings in HTTP headers... (aws#2588)

Bennett-Lynch · web-flow · commit 3d38a0825b83 · 2021-07-14T14:32:21.000-07:00
* Add support for marshalling lists of strings in HTTP headers... ...for JSON/XML protocols. ## Motivation and Context We currently lack support for marshalling lists of strings (and enums) in HTTP headers. Other IDLs and modeling languages, e.g., Smithy, do support such bindings and services may come to expect such support: https://awslabs.github.io/smithy/1.0/spec/core/http-traits.html#httpheader-trait ## Description * Add support for marshalling a list of strings to relevant JSON and XML marshalling classes * Consistent with the existing XML HeaderUnmarshaller map implementation, unmarshalling only supports string value types (this can be extended in the future if needed) * Marshalling implementations are changed to use appendHeader rather than putHeader * Add convenience method SdkField#getRequiredTrait(..) to support the common use case of ensuring a trait is present; update other existing use cases to utilize this method * Update relevant test classes to correctly recognize headers as a Map<String, List<String>> * Only wrap marshalling assertion exceptions as runtime when needed, minimizing stack trace noise ## Testing New tests added to: * rest-json-input.json * rest-json-output.json * rest-xml-input.json * rest-xml-output.json
diff --git a/.changes/next-release/feature-AWSSDKforJavav2-641dd1e.json b/.changes/next-release/feature-AWSSDKforJavav2-641dd1e.json
@@ -0,0 +1,6 @@
+{
+    "category": "AWS SDK for Java v2", 
+    "contributor": "", 
+    "type": "feature", 
+    "description": "Add support for marshalling lists of strings in HTTP headers"
+}
diff --git a/core/protocols/aws-json-protocol/src/main/java/software/amazon/awssdk/protocols/json/internal/marshall/HeaderMarshaller.java b/core/protocols/aws-json-protocol/src/main/java/software/amazon/awssdk/protocols/json/internal/marshall/HeaderMarshaller.java
@@ -15,11 +15,16 @@
 
 package software.amazon.awssdk.protocols.json.internal.marshall;
 
+import static software.amazon.awssdk.utils.CollectionUtils.isNullOrEmpty;
+
 import java.nio.charset.StandardCharsets;
 import java.time.Instant;
+import java.util.List;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 import software.amazon.awssdk.core.SdkField;
+import software.amazon.awssdk.core.protocol.MarshallLocation;
 import software.amazon.awssdk.core.traits.JsonValueTrait;
+import software.amazon.awssdk.core.traits.ListTrait;
 import software.amazon.awssdk.protocols.core.ValueToStringConverter;
 import software.amazon.awssdk.utils.BinaryUtils;
 
@@ -45,6 +50,19 @@ public final class HeaderMarshaller {
     public static final JsonMarshaller<Instant> INSTANT
         = new SimpleHeaderMarshaller<>(JsonProtocolMarshaller.INSTANT_VALUE_TO_STRING);
 
+    public static final JsonMarshaller<List<?>> LIST = (list, context, paramName, sdkField) -> {
+        // Null or empty lists cannot be meaningfully (or safely) represented in an HTTP header message since header-fields must 
+        // typically have a non-empty field-value. https://datatracker.ietf.org/doc/html/rfc7230#section-3.2
+        if (isNullOrEmpty(list)) {
+            return;
+        }
+        SdkField memberFieldInfo = sdkField.getRequiredTrait(ListTrait.class).memberFieldInfo();
+        for (Object listValue : list) {
+            JsonMarshaller marshaller = context.marshallerRegistry().getMarshaller(MarshallLocation.HEADER, listValue);
+            marshaller.marshall(listValue, context, paramName, memberFieldInfo);
+        }
+    };
+
     private HeaderMarshaller() {
     }
 
@@ -58,8 +76,7 @@ private SimpleHeaderMarshaller(ValueToStringConverter.ValueToString<T> converter
 
         @Override
         public void marshall(T val, JsonMarshallerContext context, String paramName, SdkField<T> sdkField) {
-            context.request().putHeader(paramName, converter.convert(val, sdkField));
+            context.request().appendHeader(paramName, converter.convert(val, sdkField));
         }
     }
-
 }
diff --git a/core/protocols/aws-json-protocol/src/main/java/software/amazon/awssdk/protocols/json/internal/marshall/JsonProtocolMarshaller.java b/core/protocols/aws-json-protocol/src/main/java/software/amazon/awssdk/protocols/json/internal/marshall/JsonProtocolMarshaller.java
@@ -110,6 +110,7 @@ private static JsonMarshallerRegistry createMarshallerRegistry() {
             .headerMarshaller(MarshallingType.FLOAT, HeaderMarshaller.FLOAT)
             .headerMarshaller(MarshallingType.BOOLEAN, HeaderMarshaller.BOOLEAN)
             .headerMarshaller(MarshallingType.INSTANT, HeaderMarshaller.INSTANT)
+            .headerMarshaller(MarshallingType.LIST, HeaderMarshaller.LIST)
             .headerMarshaller(MarshallingType.NULL, JsonMarshaller.NULL)
 
             .queryParamMarshaller(MarshallingType.STRING, QueryParamMarshaller.STRING)
diff --git a/core/protocols/aws-json-protocol/src/main/java/software/amazon/awssdk/protocols/json/internal/unmarshall/HeaderUnmarshaller.java b/core/protocols/aws-json-protocol/src/main/java/software/amazon/awssdk/protocols/json/internal/unmarshall/HeaderUnmarshaller.java
@@ -15,14 +15,18 @@
 
 package software.amazon.awssdk.protocols.json.internal.unmarshall;
 
+import static java.util.stream.Collectors.toList;
+
 import java.nio.charset.StandardCharsets;
 import java.time.Instant;
+import java.util.List;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 import software.amazon.awssdk.core.SdkField;
 import software.amazon.awssdk.core.traits.JsonValueTrait;
 import software.amazon.awssdk.protocols.core.StringToValueConverter;
 import software.amazon.awssdk.protocols.json.internal.dom.SdkJsonNode;
 import software.amazon.awssdk.utils.BinaryUtils;
+import software.amazon.awssdk.utils.http.SdkHttpUtils;
 
 /**
  * Header unmarshallers for all the simple types we support.
@@ -39,6 +43,11 @@ final class HeaderUnmarshaller {
     public static final JsonUnmarshaller<Boolean> BOOLEAN = new SimpleHeaderUnmarshaller<>(StringToValueConverter.TO_BOOLEAN);
     public static final JsonUnmarshaller<Float> FLOAT = new SimpleHeaderUnmarshaller<>(StringToValueConverter.TO_FLOAT);
 
+    // Only supports string value type
+    public static final JsonUnmarshaller<List<?>> LIST = (context, jsonContent, field) -> {
+        return SdkHttpUtils.allMatchingHeaders(context.response().headers(), field.locationName()).collect(toList());
+    };
+
     private HeaderUnmarshaller() {
     }
 
diff --git a/core/protocols/aws-json-protocol/src/main/java/software/amazon/awssdk/protocols/json/internal/unmarshall/JsonProtocolUnmarshaller.java b/core/protocols/aws-json-protocol/src/main/java/software/amazon/awssdk/protocols/json/internal/unmarshall/JsonProtocolUnmarshaller.java
@@ -82,6 +82,7 @@ private static JsonUnmarshallerRegistry createUnmarshallerRegistry(
             .headerUnmarshaller(MarshallingType.BOOLEAN, HeaderUnmarshaller.BOOLEAN)
             .headerUnmarshaller(MarshallingType.INSTANT, HeaderUnmarshaller.createInstantHeaderUnmarshaller(instantStringToValue))
             .headerUnmarshaller(MarshallingType.FLOAT, HeaderUnmarshaller.FLOAT)
+            .headerUnmarshaller(MarshallingType.LIST, HeaderUnmarshaller.LIST)
 
             .payloadUnmarshaller(MarshallingType.STRING, new SimpleTypeJsonUnmarshaller<>(StringToValueConverter.TO_STRING))
             .payloadUnmarshaller(MarshallingType.INTEGER, new SimpleTypeJsonUnmarshaller<>(StringToValueConverter.TO_INTEGER))
diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/HeaderMarshaller.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/HeaderMarshaller.java
@@ -15,11 +15,15 @@
 
 package software.amazon.awssdk.protocols.xml.internal.marshall;
 
+import static software.amazon.awssdk.utils.CollectionUtils.isNullOrEmpty;
+
 import java.time.Instant;
+import java.util.List;
 import java.util.Map;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 import software.amazon.awssdk.core.SdkField;
 import software.amazon.awssdk.core.protocol.MarshallLocation;
+import software.amazon.awssdk.core.traits.ListTrait;
 import software.amazon.awssdk.protocols.core.ValueToStringConverter;
 
 @SdkInternalApi
@@ -62,10 +66,30 @@ public void marshall(Map<String, ?> map, XmlMarshallerContext context, String pa
 
         @Override
         protected boolean shouldEmit(Map map) {
-            return map != null && !map.isEmpty();
+            return !isNullOrEmpty(map);
         }
     };
 
+    public static final XmlMarshaller<List<?>> LIST = new SimpleHeaderMarshaller<List<?>>(null) {
+        @Override
+        public void marshall(List<?> list, XmlMarshallerContext context, String paramName, SdkField<List<?>> sdkField) {
+            if (!shouldEmit(list)) {
+                return;
+            }
+            SdkField memberFieldInfo = sdkField.getRequiredTrait(ListTrait.class).memberFieldInfo();
+            for (Object listValue : list) {
+                XmlMarshaller marshaller = context.marshallerRegistry().getMarshaller(MarshallLocation.HEADER, listValue);
+                marshaller.marshall(listValue, context, paramName, memberFieldInfo);
+            }
+        }
+
+        @Override
+        protected boolean shouldEmit(List list) {
+            // Null or empty lists cannot be meaningfully (or safely) represented in an HTTP header message since header-fields
+            // must typically have a non-empty field-value. https://datatracker.ietf.org/doc/html/rfc7230#section-3.2
+            return !isNullOrEmpty(list);
+        }
+    };
 
     private HeaderMarshaller() {
     }
@@ -83,7 +107,7 @@ public void marshall(T val, XmlMarshallerContext context, String paramName, SdkF
                 return;
             }
 
-            context.request().putHeader(paramName, converter.convert(val, sdkField));
+            context.request().appendHeader(paramName, converter.convert(val, sdkField));
         }
 
         protected boolean shouldEmit(T val) {
diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/QueryParamMarshaller.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/QueryParamMarshaller.java
@@ -60,8 +60,7 @@ public final class QueryParamMarshaller {
             return;
         }
 
-        MapTrait mapTrait = sdkField.getOptionalTrait(MapTrait.class)
-                                    .orElseThrow(() -> new IllegalStateException("SdkField of list type is missing List trait"));
+        MapTrait mapTrait = sdkField.getRequiredTrait(MapTrait.class);
         SdkField valueField = mapTrait.valueFieldInfo();
 
         for (Map.Entry<String, ?> entry : map.entrySet()) {
diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlPayloadMarshaller.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlPayloadMarshaller.java
@@ -81,9 +81,7 @@ public void marshall(List<?> val, XmlMarshallerContext context, String paramName
         @Override
         public void marshall(List<?> list, XmlMarshallerContext context, String paramName,
                              SdkField<List<?>> sdkField, ValueToStringConverter.ValueToString<List<?>> converter) {
-            ListTrait listTrait = sdkField
-                .getOptionalTrait(ListTrait.class)
-                .orElseThrow(() -> new IllegalStateException(paramName + " member is missing ListTrait"));
+            ListTrait listTrait = sdkField.getRequiredTrait(ListTrait.class);
 
             if (!listTrait.isFlattened()) {
                 context.xmlGenerator().startElement(paramName);
@@ -125,8 +123,7 @@ protected boolean shouldEmit(List list, String paramName) {
         public void marshall(Map<String, ?> map, XmlMarshallerContext context, String paramName,
                              SdkField<Map<String, ?>> sdkField, ValueToStringConverter.ValueToString<Map<String, ?>> converter) {
 
-            MapTrait mapTrait = sdkField.getOptionalTrait(MapTrait.class)
-                                        .orElseThrow(() -> new IllegalStateException(paramName + " member is missing MapTrait"));
+            MapTrait mapTrait = sdkField.getRequiredTrait(MapTrait.class);
 
             for (Map.Entry<String, ?> entry : map.entrySet()) {
                 context.xmlGenerator().startElement("entry");
diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlProtocolMarshaller.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/marshall/XmlProtocolMarshaller.java
@@ -176,6 +176,7 @@ private static XmlMarshallerRegistry createMarshallerRegistry() {
             .headerMarshaller(MarshallingType.BOOLEAN, HeaderMarshaller.BOOLEAN)
             .headerMarshaller(MarshallingType.INSTANT, HeaderMarshaller.INSTANT)
             .headerMarshaller(MarshallingType.MAP, HeaderMarshaller.MAP)
+            .headerMarshaller(MarshallingType.LIST, HeaderMarshaller.LIST)
             .headerMarshaller(MarshallingType.NULL, XmlMarshaller.NULL)
 
             .queryParamMarshaller(MarshallingType.STRING, QueryParamMarshaller.STRING)
diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/unmarshall/HeaderUnmarshaller.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/unmarshall/HeaderUnmarshaller.java
@@ -15,6 +15,7 @@
 
 package software.amazon.awssdk.protocols.xml.internal.unmarshall;
 
+import static java.util.stream.Collectors.toList;
 import static software.amazon.awssdk.utils.StringUtils.replacePrefixIgnoreCase;
 import static software.amazon.awssdk.utils.StringUtils.startsWithIgnoreCase;
 
@@ -26,6 +27,7 @@
 import software.amazon.awssdk.core.SdkField;
 import software.amazon.awssdk.protocols.core.StringToValueConverter;
 import software.amazon.awssdk.protocols.query.unmarshall.XmlElement;
+import software.amazon.awssdk.utils.http.SdkHttpUtils;
 
 @SdkInternalApi
 public final class HeaderUnmarshaller {
@@ -39,6 +41,7 @@ public final class HeaderUnmarshaller {
     public static final XmlUnmarshaller<Instant> INSTANT =
         new SimpleHeaderUnmarshaller<>(XmlProtocolUnmarshaller.INSTANT_STRING_TO_VALUE);
 
+    // Only supports string value type
     public static final XmlUnmarshaller<Map<String, ?>> MAP = ((context, content, field) -> {
         Map<String, String> result = new HashMap<>();
         context.response().headers().entrySet().stream()
@@ -48,6 +51,11 @@ public final class HeaderUnmarshaller {
         return result;
     });
 
+    // Only supports string value type
+    public static final XmlUnmarshaller<List<?>> LIST = (context, content, field) -> {
+        return SdkHttpUtils.allMatchingHeaders(context.response().headers(), field.locationName()).collect(toList());
+    };
+
     private HeaderUnmarshaller() {
     }
 
diff --git a/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/unmarshall/XmlProtocolUnmarshaller.java b/core/protocols/aws-xml-protocol/src/main/java/software/amazon/awssdk/protocols/xml/internal/unmarshall/XmlProtocolUnmarshaller.java
@@ -150,6 +150,7 @@ private static XmlUnmarshallerRegistry createUnmarshallerRegistry() {
             .headerUnmarshaller(MarshallingType.INSTANT, HeaderUnmarshaller.INSTANT)
             .headerUnmarshaller(MarshallingType.FLOAT, HeaderUnmarshaller.FLOAT)
             .headerUnmarshaller(MarshallingType.MAP, HeaderUnmarshaller.MAP)
+            .headerUnmarshaller(MarshallingType.LIST, HeaderUnmarshaller.LIST)
 
             .payloadUnmarshaller(MarshallingType.STRING, XmlPayloadUnmarshaller.STRING)
             .payloadUnmarshaller(MarshallingType.INTEGER, XmlPayloadUnmarshaller.INTEGER)
diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/SdkField.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/SdkField.java
@@ -122,6 +122,23 @@ public <T extends Trait> Optional<T> getOptionalTrait(Class<T> clzz) {
         return Optional.ofNullable((T) traits.get(clzz));
     }
 
+    /**
+     * Gets the trait of the specified class, or throw {@link IllegalStateException} if not available.
+     *
+     * @param clzz Trait class to get.
+     * @param <T> Type of trait.
+     * @return Trait instance.
+     * @throws IllegalStateException if trait is not present.
+     */
+    @SuppressWarnings("unchecked")
+    public <T extends Trait> T getRequiredTrait(Class<T> clzz) throws IllegalStateException {
+        T trait = (T) traits.get(clzz);
+        if (trait == null) {
+            throw new IllegalStateException(memberName + " member is missing " + clzz.getSimpleName());
+        }
+        return trait;
+    }
+
     /**
      * Checks if a given {@link Trait} is present on the field.
      *
diff --git a/test/protocol-tests-core/src/main/java/software/amazon/awssdk/protocol/asserts/marshalling/HeadersAssertion.java b/test/protocol-tests-core/src/main/java/software/amazon/awssdk/protocol/asserts/marshalling/HeadersAssertion.java
@@ -17,6 +17,7 @@
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 import com.github.tomakehurst.wiremock.http.HttpHeaders;
 import com.github.tomakehurst.wiremock.verification.LoggedRequest;
@@ -28,11 +29,11 @@
  */
 public class HeadersAssertion extends MarshallingAssertion {
 
-    private Map<String, String> contains;
+    private Map<String, List<String>> contains;
 
     private List<String> doesNotContain;
 
-    public void setContains(Map<String, String> contains) {
+    public void setContains(Map<String, List<String>> contains) {
         this.contains = contains;
     }
 
@@ -51,8 +52,11 @@ protected void doAssert(LoggedRequest actual) throws Exception {
     }
 
     private void assertHeadersContains(HttpHeaders actual) {
-        contains.entrySet().forEach(e -> {
-            assertEquals(e.getValue(), actual.getHeader(e.getKey()).firstValue());
+        contains.forEach((expectedKey, expectedValues) -> {
+            assertTrue(String.format("Header '%s' was expected to be present. Actual headers: %s", expectedKey, actual),
+                       actual.getHeader(expectedKey).isPresent());
+            List<String> actualValues = actual.getHeader(expectedKey).values();
+            assertEquals(expectedValues, actualValues);
         });
     }
 
diff --git a/test/protocol-tests-core/src/main/java/software/amazon/awssdk/protocol/asserts/marshalling/MarshallingAssertion.java b/test/protocol-tests-core/src/main/java/software/amazon/awssdk/protocol/asserts/marshalling/MarshallingAssertion.java
@@ -29,9 +29,11 @@ public abstract class MarshallingAssertion {
      * @throws AssertionError If any assertions fail
      */
     public final void assertMatches(LoggedRequest actual) throws AssertionError {
-        // Catches the exception to play nicer with lambda's
+        // Wrap checked exceptions to play nicer with lambda's
         try {
             doAssert(actual);
+        } catch (Error | RuntimeException e) {
+            throw e;
         } catch (Exception e) {
             throw new RuntimeException(e);
         }
diff --git a/test/protocol-tests-core/src/main/java/software/amazon/awssdk/protocol/model/GivenResponse.java b/test/protocol-tests-core/src/main/java/software/amazon/awssdk/protocol/model/GivenResponse.java
@@ -16,13 +16,14 @@
 package software.amazon.awssdk.protocol.model;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.List;
 import java.util.Map;
 
 public class GivenResponse {
 
     @JsonProperty(value = "status_code")
     private Integer statusCode;
-    private Map<String, String> headers;
+    private Map<String, List<String>> headers;
     private String body;
 
     public Integer getStatusCode() {
@@ -33,11 +34,11 @@ public void setStatusCode(Integer statusCode) {
         this.statusCode = statusCode;
     }
 
-    public Map<String, String> getHeaders() {
+    public Map<String, List<String>> getHeaders() {
         return headers;
     }
 
-    public void setHeaders(Map<String, String> headers) {
+    public void setHeaders(Map<String, List<String>> headers) {
         this.headers = headers;
     }
 
diff --git a/test/protocol-tests-core/src/main/java/software/amazon/awssdk/protocol/runners/UnmarshallingTestRunner.java b/test/protocol-tests-core/src/main/java/software/amazon/awssdk/protocol/runners/UnmarshallingTestRunner.java
@@ -99,7 +99,9 @@ private ResponseDefinitionBuilder toResponseBuilder(GivenResponse givenResponse)
 
         ResponseDefinitionBuilder responseBuilder = aResponse().withStatus(200);
         if (givenResponse.getHeaders() != null) {
-            givenResponse.getHeaders().forEach(responseBuilder::withHeader);
+            givenResponse.getHeaders().forEach((key, values) -> {
+                responseBuilder.withHeader(key, values.toArray(new String[0]));
+            });
         }
         if (givenResponse.getStatusCode() != null) {
             responseBuilder.withStatus(givenResponse.getStatusCode());
diff --git a/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-input.json b/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-input.json
diff --git a/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-output.json b/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-json-output.json
diff --git a/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-xml-input.json b/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-xml-input.json
diff --git a/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-xml-output.json b/test/protocol-tests-core/src/main/resources/software/amazon/awssdk/protocol/suites/cases/rest-xml-output.json
diff --git a/test/protocol-tests/src/main/resources/codegen-resources/restjson/service-2.json b/test/protocol-tests/src/main/resources/codegen-resources/restjson/service-2.json
diff --git a/test/protocol-tests/src/main/resources/codegen-resources/restxml/service-2.json b/test/protocol-tests/src/main/resources/codegen-resources/restxml/service-2.json

Original file line number	Diff line number	Diff line change
`@@ -60,8 +60,7 @@ public final class QueryParamMarshaller {`
`60`	`60`	`return;`
`61`	`61`	`}`
`62`	`62`
`63`		`- MapTrait mapTrait = sdkField.getOptionalTrait(MapTrait.class)`
`64`		`- .orElseThrow(() -> new IllegalStateException("SdkField of list type is missing List trait"));`
	`63`	`+ MapTrait mapTrait = sdkField.getRequiredTrait(MapTrait.class);`
`65`	`64`	`SdkField valueField = mapTrait.valueFieldInfo();`
`66`	`65`
`67`	`66`	`for (Map.Entry<String, ?> entry : map.entrySet()) {`