Skip to content

Standardize the way SdkBytes are unmarshalled when they're part of a payload and the service returns no content. #3021

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/next-release/bugfix-AWSSDKforJavav2-98d2e80.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"category": "AWS SDK for Java v2",
"contributor": "",
"type": "bugfix",
"description": "Always return an empty SDK bytes when services model their response payload as a blob. Previously, it would either return null, empty bytes or throw an exception depending on the protocol, HTTP client and whether the service was using chunked encoding for their responses."
}
9 changes: 8 additions & 1 deletion bom-internal/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,17 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<version>${wiremock.version}</version>
<scope>test</scope>
</dependency>
<!-- Only use wiremock instead of wiremock-jre8 when working around bugs in the newer version -->
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<version>2.18.0</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
Expand Down
2 changes: 1 addition & 1 deletion core/auth-crt/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
2 changes: 1 addition & 1 deletion core/auth/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
2 changes: 1 addition & 1 deletion core/aws-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
2 changes: 1 addition & 1 deletion core/metrics-spi/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.annotations.ThreadSafe;
Expand All @@ -36,6 +37,7 @@
import software.amazon.awssdk.core.traits.MapTrait;
import software.amazon.awssdk.core.traits.PayloadTrait;
import software.amazon.awssdk.core.traits.TimestampFormatTrait;
import software.amazon.awssdk.http.AbortableInputStream;
import software.amazon.awssdk.http.SdkHttpFullResponse;
import software.amazon.awssdk.protocols.core.StringToInstant;
import software.amazon.awssdk.protocols.core.StringToValueConverter;
Expand Down Expand Up @@ -225,9 +227,13 @@ private static <TypeT extends SdkPojo> TypeT unmarshallStructured(SdkPojo sdkPoj
JsonNode jsonContent,
JsonUnmarshallerContext context) {
for (SdkField<?> field : sdkPojo.sdkFields()) {
if (isExplicitPayloadMember(field) && field.marshallingType() == MarshallingType.SDK_BYTES &&
context.response().content().isPresent()) {
field.set(sdkPojo, SdkBytes.fromInputStream(context.response().content().get()));
if (isExplicitPayloadMember(field) && field.marshallingType() == MarshallingType.SDK_BYTES) {
Optional<AbortableInputStream> responseContent = context.response().content();
if (responseContent.isPresent()) {
field.set(sdkPojo, SdkBytes.fromInputStream(responseContent.get()));
} else {
field.set(sdkPojo, SdkBytes.fromByteArrayUnsafe(new byte[0]));
}
} else {
JsonNode jsonFieldContent = getJsonNode(jsonContent, field);
JsonUnmarshaller<Object> unmarshaller = context.getUnmarshaller(field.location(), field.marshallingType());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,25 @@

import static software.amazon.awssdk.awscore.util.AwsHeader.AWS_REQUEST_ID;
import static software.amazon.awssdk.protocols.query.internal.marshall.SimpleTypeQueryMarshaller.defaultTimestampFormats;
import static software.amazon.awssdk.utils.FunctionalUtils.invokeSafely;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.core.SdkBytes;
import software.amazon.awssdk.core.SdkField;
import software.amazon.awssdk.core.SdkPojo;
import software.amazon.awssdk.core.protocol.MarshallingType;
import software.amazon.awssdk.core.traits.PayloadTrait;
import software.amazon.awssdk.http.SdkHttpFullResponse;
import software.amazon.awssdk.protocols.core.StringToInstant;
import software.amazon.awssdk.protocols.core.StringToValueConverter;
import software.amazon.awssdk.protocols.query.unmarshall.XmlDomParser;
import software.amazon.awssdk.protocols.query.unmarshall.XmlElement;
import software.amazon.awssdk.protocols.query.unmarshall.XmlErrorUnmarshaller;
import software.amazon.awssdk.utils.CollectionUtils;
import software.amazon.awssdk.utils.IoUtils;
import software.amazon.awssdk.utils.Pair;
import software.amazon.awssdk.utils.builder.Buildable;

Expand Down Expand Up @@ -69,11 +73,26 @@ private QueryProtocolUnmarshaller(Builder builder) {

public <TypeT extends SdkPojo> Pair<TypeT, Map<String, String>> unmarshall(SdkPojo sdkPojo,
SdkHttpFullResponse response) {
if (responsePayloadIsBlob(sdkPojo)) {
XmlElement document = XmlElement.builder()
.textContent(response.content()
.map(s -> invokeSafely(() -> IoUtils.toUtf8String(s)))
.orElse(""))
.build();
return Pair.of(unmarshall(sdkPojo, document, response), new HashMap<>());
}

XmlElement document = response.content().map(XmlDomParser::parse).orElseGet(XmlElement::empty);
XmlElement resultRoot = hasResultWrapper ? document.getFirstChild() : document;
return Pair.of(unmarshall(sdkPojo, resultRoot, response), parseMetadata(document));
}

private boolean responsePayloadIsBlob(SdkPojo sdkPojo) {
return sdkPojo.sdkFields().stream()
.anyMatch(field -> field.marshallingType() == MarshallingType.SDK_BYTES &&
field.containsTrait(PayloadTrait.class));
}

/**
* This method is also used to unmarshall exceptions. We use this since we've already parsed the XML
* and the result root is in a different location depending on the protocol/service.
Expand Down Expand Up @@ -109,6 +128,10 @@ private String metadataKeyName(XmlElement c) {
private SdkPojo unmarshall(QueryUnmarshallerContext context, SdkPojo sdkPojo, XmlElement root) {
if (root != null) {
for (SdkField<?> field : sdkPojo.sdkFields()) {
if (field.containsTrait(PayloadTrait.class) && field.marshallingType() == MarshallingType.SDK_BYTES) {
field.set(sdkPojo, SdkBytes.fromUtf8String(root.textContent()));
}

List<XmlElement> element = root.getElementsByName(field.unmarshallLocationName());
if (!CollectionUtils.isNullOrEmpty(element)) {
QueryUnmarshaller<Object> unmarshaller =
Expand All @@ -118,6 +141,7 @@ private SdkPojo unmarshall(QueryUnmarshallerContext context, SdkPojo sdkPojo, Xm
}
}
}

return (SdkPojo) ((Buildable) sdkPojo).build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

package software.amazon.awssdk.protocols.query.unmarshall;

import java.io.IOException;
import java.io.InputStream;
import java.util.HashMap;
import java.util.Iterator;
Expand All @@ -27,6 +28,7 @@
import javax.xml.stream.events.XMLEvent;
import software.amazon.awssdk.annotations.SdkProtectedApi;
import software.amazon.awssdk.core.exception.SdkClientException;
import software.amazon.awssdk.utils.LookaheadInputStream;

/**
* Parses an XML document into a simple DOM like structure, {@link XmlElement}.
Expand All @@ -40,15 +42,20 @@ private XmlDomParser() {
}

public static XmlElement parse(InputStream inputStream) {
LookaheadInputStream stream = new LookaheadInputStream(inputStream);
try {
XMLEventReader reader = FACTORY.get().createXMLEventReader(inputStream);
if (stream.peek() == -1) {
return XmlElement.empty();
}

XMLEventReader reader = FACTORY.get().createXMLEventReader(stream);
XMLEvent nextEvent;
// Skip ahead to the first start element
do {
nextEvent = reader.nextEvent();
} while (reader.hasNext() && !nextEvent.isStartElement());
return parseElement(nextEvent.asStartElement(), reader);
} catch (XMLStreamException e) {
} catch (IOException | XMLStreamException e) {
throw SdkClientException.create("Could not parse XML response.", e);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,13 @@ SdkPojo unmarshall(XmlUnmarshallerContext context, SdkPojo sdkPojo, XmlElement r

if (root != null && field.location() == MarshallLocation.PAYLOAD) {
if (!context.response().content().isPresent()) {
// This is a payload field, but the service sent no content. Do not populate this field (leave it null).
// This is a payload field, but the service sent no content. Do not populate this field (leave it null or
// empty).
if (field.marshallingType() == MarshallingType.SDK_BYTES && field.containsTrait(PayloadTrait.class)) {
// SDK bytes bound directly to the payload field should never be left empty
field.set(sdkPojo, SdkBytes.fromByteArrayUnsafe(new byte[0]));
}

continue;
}

Expand All @@ -98,9 +104,8 @@ SdkPojo unmarshall(XmlUnmarshallerContext context, SdkPojo sdkPojo, XmlElement r
root.getElementsByName(field.unmarshallLocationName());

if (!CollectionUtils.isNullOrEmpty(element)) {
boolean isFieldBlobTypePayload = payloadMemberAsBlobType.isPresent()
&& payloadMemberAsBlobType.get().equals(field);

boolean isFieldBlobTypePayload = payloadMemberAsBlobType.isPresent() &&
payloadMemberAsBlobType.get().equals(field);
if (isFieldBlobTypePayload) {
field.set(sdkPojo, SdkBytes.fromInputStream(context.response().content().get()));
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@

package software.amazon.awssdk.protocols.xml.internal.unmarshall;

import java.io.BufferedInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.util.Optional;
import software.amazon.awssdk.annotations.SdkInternalApi;
Expand All @@ -30,6 +28,7 @@
import software.amazon.awssdk.http.SdkHttpFullResponse;
import software.amazon.awssdk.protocols.query.unmarshall.XmlDomParser;
import software.amazon.awssdk.protocols.query.unmarshall.XmlElement;
import software.amazon.awssdk.utils.LookaheadInputStream;

/**
* Static methods to assist with parsing the response of AWS XML requests.
Expand Down Expand Up @@ -57,12 +56,10 @@ public static XmlElement parse(SdkPojo sdkPojo, SdkHttpFullResponse response) {
}

// Make sure there is content in the stream before passing it to the parser.
InputStream content = ensureMarkSupported(responseContent.get());
content.mark(2);
if (content.read() == -1) {
LookaheadInputStream content = new LookaheadInputStream(responseContent.get());
if (content.peek() == -1) {
return XmlElement.empty();
}
content.reset();

return XmlDomParser.parse(content);
} catch (IOException e) {
Expand All @@ -75,14 +72,6 @@ public static XmlElement parse(SdkPojo sdkPojo, SdkHttpFullResponse response) {
}
}

private static InputStream ensureMarkSupported(AbortableInputStream content) {
if (content.markSupported()) {
return content;
}

return new BufferedInputStream(content);
}

/**
* Gets the Member which is a Payload and which is of Blob Type.
* @param sdkPojo
Expand Down
2 changes: 1 addition & 1 deletion core/regions/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
2 changes: 1 addition & 1 deletion core/sdk-core/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
1 change: 1 addition & 0 deletions http-clients/apache-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
<artifactId>hamcrest-all</artifactId>
<scope>test</scope>
</dependency>
<!-- Use wiremock instead of wiremock-jre8 because HEAD request handling doesn't work in the newer versions. -->
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
Expand Down
1 change: 1 addition & 0 deletions http-clients/aws-crt-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
</dependency>

<!--Test Dependencies-->
<!-- Use wiremock instead of wiremock-jre8 because HEAD request handling doesn't work in the newer versions. -->
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
Expand Down
1 change: 1 addition & 0 deletions http-clients/netty-nio-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
<version>${awsjavasdk.version}</version>
<scope>test</scope>
</dependency>
<!-- Use wiremock instead of wiremock-jre8 because HEAD request handling doesn't work in the newer versions. -->
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
Expand Down
2 changes: 1 addition & 1 deletion metric-publishers/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@
<scope>test</scope>
</dependency>
<dependency>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<groupId>com.github.tomakehurst</groupId>
<scope>test</scope>
</dependency>
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
<jacksonjr.version>2.13.1</jacksonjr.version>
<eventstream.version>1.0.1</eventstream.version>
<commons.lang.version>3.12.0</commons.lang.version>
<wiremock.version>2.18.0</wiremock.version>
<wiremock.version>2.32.0</wiremock.version>
<slf4j.version>1.7.30</slf4j.version>
<log4j.version>2.17.1</log4j.version>
<commons.io.version>2.5</commons.io.version>
Expand Down
2 changes: 1 addition & 1 deletion services-custom/dynamodb-enhanced/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
2 changes: 1 addition & 1 deletion services/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@
<scope>test</scope>
</dependency>
<dependency>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<groupId>com.github.tomakehurst</groupId>
<scope>test</scope>
</dependency>
Expand Down
2 changes: 1 addition & 1 deletion services/s3/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
7 changes: 6 additions & 1 deletion test/auth-sts-testing/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,12 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
2 changes: 1 addition & 1 deletion test/codegen-generated-classes-test/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@
</dependency>
<dependency>
<groupId>com.github.tomakehurst</groupId>
<artifactId>wiremock</artifactId>
<artifactId>wiremock-jre8</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Expand Down
Loading