Skip null check in doMarshal JSON serializer#6807
Conversation
RanVaknin
changed the title
RanVaknin
changed the title
| import software.amazon.awssdk.protocols.json.AwsJsonProtocolMetadata; | ||
| import software.amazon.awssdk.protocols.json.internal.AwsStructuredPlainJsonFactory; | ||
|
|
||
| class JsonProtocolMarshallerTest { |
There was a problem hiding this comment.
Almost all of the codepaths that this file tests are covered by the protocol test package. nullPayloadField_required_throwsIllegalArgumentException is the only test that is not covered by the end to end tests because its technically not possible to get there. This codepath branch was likely added as a defense in depth.
| } else if (field.containsTrait(RequiredTrait.class, TraitType.REQUIRED_TRAIT)) { | ||
| throw new IllegalArgumentException( | ||
| String.format("Parameter '%s' must not be null", field.locationName())); |
There was a problem hiding this comment.
Do we validate required client side? I thought we generally didn't do client side validation of required and instead just sent these requests to the service (since the api may have evolved and removed the required over time).
There was a problem hiding this comment.
I agree, but this is not a new validation. I had to port this logic out from the null marshaller:
since we are handling nulls now at the doMarshal level.
There was a problem hiding this comment.
Ah I see - yeah, agree this makes sense here then.
| } else if (field.location() != MarshallLocation.PAYLOAD) { | ||
| marshallField(field, val); |
There was a problem hiding this comment.
Is there any reason we added field.location() != MarshallLocation.PAYLOAD check here?
There was a problem hiding this comment.
Yes, this optimization only applies to PAYLOAD fields. Non payload locations (path, header, query) have their own null marshallers with their own behavior. If we removed the != MarshallLocation.PAYLOAD check and skipped all null fields, null path params would be silently dropped instead of throwing, which broke RestJsonExceptionTests.nullPathParam_ThrowsSdkClientException in an earlier iteration of this PR.
There was a problem hiding this comment.
I see, it's not very obvious from looking at the code, can we add a comment in code explaining this?
alextwoods
left a comment
alextwoods
left a comment
There was a problem hiding this comment.
Overall looks good. Needs a changelog I think.
|
3 participants
Background
During JSON serialization,
JsonProtocolMarshaller.doMarshall()iterates everySdkFieldon a POJO and callsmarshallField()for each one, including fields that are null. marshallField uses a registry to dispatch to a type specific marshaller (string, int, map, null, etc ) based on the field's value.When the value is null, it dispatches to the NULL marshaller, that checks if the field is required (throw) or not (no-op). For non-required null fields, this entire chain (registry lookup, marshaller resolution, trait check) is wasted work.
The proposed change is to short circuit null handling for payload fields directly on the generic
doMarshall()layer, and avoid the dispatch. Non payload fields still delegate tomarshallFieldsince their null marshallers have different validation logic.Boto3's RestJson serializer handles it very similarly.
Testing
Our protocol tests already cover the main code paths of doMarshal (null headers, null query params, partial payload serialization). I added one unit test to specific JsonProtocolMarshaller directly to cover the case where a null required payload field reaches the marshaller. This codepath is caught earlier by the SDK's generated validators, but the this is just make sure the marshaller handles it correctly if that layer is ever bypassed.
Results:
Benchmarks written in #6776
Before:
After
