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.
alextwoods
left a comment
alextwoods
left a comment
There was a problem hiding this comment.
Overall looks good. Needs a changelog I think.
|
|
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
RanVaknin
added
the
perf-improvement
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
