Skip to content

Commit 54a01b7

Browse files
RanVakninRanVaknin
authored
Skip null check in doMarshal JSON serializer (#6807)
* Skip null non required fields in JsonProtocolMarshaller.doMarshall * Fix domarshall logic * Fix logic, add null suppression * Add test coverage * Add changelog * Add comment for better readability
1 parent 6ae0046 commit 54a01b7

File tree

4 files changed

+222
-1
lines changed

4 files changed

+222
-1
lines changed

‎.changes/next-release/bugfix-AWSSDKforJavav2-1a353a6.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "AWS SDK for Java v2",
4+
"contributor": "",
5+
"description": "Optimized JSON serialization by skipping null field marshalling for payload fields"
6+
}

‎build-tools/src/main/resources/software/amazon/awssdk/spotbugs-suppressions.xml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,4 +523,12 @@
523523
<Class name="software.amazon.awssdk.http.urlconnection.UrlConnectionSdkHttpService"/>
524524
<Bug pattern="CT_CONSTRUCTOR_THROW"/>
525525
</Match>
526+
527+
<!-- Intentionally passing null to marshallField for non-PAYLOAD fields
528+
whose NULL marshallers handle null validation. -->
529+
<Match>
530+
<Class name="software.amazon.awssdk.protocols.json.internal.marshall.JsonProtocolMarshaller"/>
531+
<Method name="doMarshall"/>
532+
<Bug pattern="NP_LOAD_OF_KNOWN_NULL_VALUE"/>
533+
</Match>
526534
</FindBugsFilter>

‎core/protocols/aws-json-protocol/src/main/java/software/amazon/awssdk/protocols/json/internal/marshall/JsonProtocolMarshaller.java‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import software.amazon.awssdk.core.protocol.MarshallLocation;
3636
import software.amazon.awssdk.core.protocol.MarshallingType;
3737
import software.amazon.awssdk.core.traits.PayloadTrait;
38+
import software.amazon.awssdk.core.traits.RequiredTrait;
3839
import software.amazon.awssdk.core.traits.TimestampFormatTrait;
3940
import software.amazon.awssdk.core.traits.TraitType;
4041
import software.amazon.awssdk.http.SdkHttpFullRequest;
@@ -212,8 +213,17 @@ void doMarshall(SdkPojo pojo) {
212213
}
213214
} else if (isExplicitPayloadMember(field)) {
214215
marshallExplicitJsonPayload(field, val);
215-
} else {
216+
} else if (val != null) {
217+
marshallField(field, val);
218+
} else if (field.location() != MarshallLocation.PAYLOAD) {
219+
// Null payload fields that aren't required are no-op in the marshaller registry.
220+
// We short circuit to avoid the registry lookup and dispatch overhead.
221+
// Non payload locations (path, header, query) have null marshallers with
222+
// different behavior, so they must still go through marshallField.
216223
marshallField(field, val);
224+
} else if (field.containsTrait(RequiredTrait.class, TraitType.REQUIRED_TRAIT)) {
225+
throw new IllegalArgumentException(
226+
String.format("Parameter '%s' must not be null", field.locationName()));
217227
}
218228
}
219229
}

‎core/protocols/aws-json-protocol/src/test/java/software/amazon/awssdk/protocols/json/internal/marshall/JsonProtocolMarshallerTest.java‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.awssdk.protocols.json.internal.marshall;
17+
18+
import static org.assertj.core.api.Assertions.assertThat;
19+
import static org.assertj.core.api.Assertions.assertThatNoException;
20+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
21+
22+
import java.net.URI;
23+
import java.util.Arrays;
24+
import java.util.Collections;
25+
import java.util.List;
26+
import java.util.Map;
27+
import org.junit.jupiter.api.Test;
28+
import software.amazon.awssdk.core.SdkField;
29+
import software.amazon.awssdk.core.SdkPojo;
30+
import software.amazon.awssdk.core.protocol.MarshallLocation;
31+
import software.amazon.awssdk.core.protocol.MarshallingType;
32+
import software.amazon.awssdk.core.traits.LocationTrait;
33+
import software.amazon.awssdk.core.traits.RequiredTrait;
34+
import software.amazon.awssdk.http.SdkHttpFullRequest;
35+
import software.amazon.awssdk.http.SdkHttpMethod;
36+
import software.amazon.awssdk.protocols.core.OperationInfo;
37+
import software.amazon.awssdk.protocols.core.ProtocolMarshaller;
38+
import software.amazon.awssdk.protocols.json.AwsJsonProtocol;
39+
import software.amazon.awssdk.protocols.json.AwsJsonProtocolMetadata;
40+
import software.amazon.awssdk.protocols.json.internal.AwsStructuredPlainJsonFactory;
41+
42+
class JsonProtocolMarshallerTest {
43+
44+
private static final URI ENDPOINT = URI.create("http://localhost");
45+
private static final String CONTENT_TYPE = "application/x-amz-json-1.0";
46+
private static final OperationInfo OP_INFO = OperationInfo.builder()
47+
.httpMethod(SdkHttpMethod.POST)
48+
.hasImplicitPayloadMembers(true)
49+
.build();
50+
private static final AwsJsonProtocolMetadata METADATA =
51+
AwsJsonProtocolMetadata.builder()
52+
.protocol(AwsJsonProtocol.AWS_JSON)
53+
.contentType(CONTENT_TYPE)
54+
.build();
55+
56+
@Test
57+
void nullPayloadField_notRequired_isSkipped() {
58+
SdkField<String> field = payloadField("OptionalField", obj -> null);
59+
SdkPojo pojo = new SimplePojo(field);
60+
61+
SdkHttpFullRequest result = createMarshaller().marshall(pojo);
62+
63+
String body = bodyAsString(result);
64+
assertThat(body).doesNotContain("OptionalField");
65+
}
66+
67+
@Test
68+
void nullPayloadField_required_throwsIllegalArgumentException() {
69+
SdkField<String> field = SdkField.<String>builder(MarshallingType.STRING)
70+
.memberName("RequiredField")
71+
.getter(obj -> null)
72+
.setter((obj, val) -> { })
73+
.traits(LocationTrait.builder()
74+
.location(MarshallLocation.PAYLOAD)
75+
.locationName("RequiredField")
76+
.build(),
77+
RequiredTrait.create())
78+
.build();
79+
80+
SdkPojo pojo = new SimplePojo(field);
81+
82+
assertThatThrownBy(() -> createMarshaller().marshall(pojo))
83+
.isInstanceOf(IllegalArgumentException.class)
84+
.hasMessageContaining("RequiredField")
85+
.hasMessageContaining("must not be null");
86+
}
87+
88+
@Test
89+
void nonNullPayloadField_isSerialized() {
90+
SdkField<String> field = payloadField("Name", obj -> "hello");
91+
SdkPojo pojo = new SimplePojo(field);
92+
93+
SdkHttpFullRequest result = createMarshaller().marshall(pojo);
94+
95+
String body = bodyAsString(result);
96+
assertThat(body).contains("\"Name\"");
97+
assertThat(body).contains("\"hello\"");
98+
}
99+
100+
@Test
101+
void nullNonPayloadField_stillGoesToMarshallField() {
102+
SdkField<String> field = SdkField.<String>builder(MarshallingType.STRING)
103+
.memberName("HeaderField")
104+
.getter(obj -> null)
105+
.setter((obj, val) -> { })
106+
.traits(LocationTrait.builder()
107+
.location(MarshallLocation.HEADER)
108+
.locationName("x-custom-header")
109+
.build())
110+
.build();
111+
112+
SdkPojo pojo = new SimplePojo(field);
113+
114+
assertThatNoException().isThrownBy(
115+
() -> createMarshaller().marshall(pojo));
116+
}
117+
118+
@Test
119+
void nullPathField_notRequired_stillThrows() {
120+
SdkField<String> field = SdkField.<String>builder(MarshallingType.STRING)
121+
.memberName("PathParam")
122+
.getter(obj -> null)
123+
.setter((obj, val) -> { })
124+
.traits(LocationTrait.builder()
125+
.location(MarshallLocation.PATH)
126+
.locationName("PathParam")
127+
.build())
128+
.build();
129+
130+
SdkPojo pojo = new SimplePojo(field);
131+
132+
assertThatThrownBy(() -> createMarshaller().marshall(pojo))
133+
.isInstanceOf(IllegalArgumentException.class)
134+
.hasMessageContaining("PathParam")
135+
.hasMessageContaining("must not be null");
136+
}
137+
138+
private static SdkField<String> payloadField(String name,
139+
java.util.function.Function<Object, String> getter) {
140+
return SdkField.<String>builder(MarshallingType.STRING)
141+
.memberName(name)
142+
.getter(getter)
143+
.setter((obj, val) -> { })
144+
.traits(LocationTrait.builder()
145+
.location(MarshallLocation.PAYLOAD)
146+
.locationName(name)
147+
.build())
148+
.build();
149+
}
150+
151+
private static ProtocolMarshaller<SdkHttpFullRequest> createMarshaller() {
152+
return JsonProtocolMarshallerBuilder.create()
153+
.endpoint(ENDPOINT)
154+
.jsonGenerator(AwsStructuredPlainJsonFactory
155+
.SDK_JSON_FACTORY.createWriter(CONTENT_TYPE))
156+
.contentType(CONTENT_TYPE)
157+
.operationInfo(OP_INFO)
158+
.sendExplicitNullForPayload(false)
159+
.protocolMetadata(METADATA)
160+
.build();
161+
}
162+
163+
private static String bodyAsString(SdkHttpFullRequest request) {
164+
return request.contentStreamProvider()
165+
.map(p -> {
166+
try {
167+
return software.amazon.awssdk.utils.IoUtils.toUtf8String(p.newStream());
168+
} catch (Exception e) {
169+
throw new RuntimeException(e);
170+
}
171+
})
172+
.orElse("");
173+
}
174+
175+
private static final class SimplePojo implements SdkPojo {
176+
private final List<SdkField<?>> fields;
177+
178+
SimplePojo(SdkField<?>... fields) {
179+
this.fields = Arrays.asList(fields);
180+
}
181+
182+
@Override
183+
public List<SdkField<?>> sdkFields() {
184+
return fields;
185+
}
186+
187+
@Override
188+
public boolean equalsBySdkFields(Object other) {
189+
return other instanceof SimplePojo;
190+
}
191+
192+
@Override
193+
public Map<String, SdkField<?>> sdkFieldNameToField() {
194+
return Collections.emptyMap();
195+
}
196+
}
197+
}

0 commit comments

Comments
 (0)