ASF: fix empty exception member serialization when required #13186
Conversation
bentsku
added
area: asf
semver: patch
| continue | ||
|
|
||
| if value: | ||
| if value or (member in shape.required_members and value is not None): |
There was a problem hiding this comment.
Just chiming in here, even though the PR is not ready for review yet: As far as I can see, this would nearly be semantically equal to:
if value is not None:
The only case that would slip though here (and wouldn't in your solution) is that a falsy non-required value would also be serialized.
My question is: Is that really the differentiation? That looks a bit weird to me 😅 If so, please add a comment to this section 🤣
There was a problem hiding this comment.
This is exactly what it means 😅 any non-truthy values will not be serialized (this might be problematic with numbers values, but we haven't encountered it yet in an exception structure). I added more unit tests to illustrate this.
For example, if you instantiate a ServiceException-derived exception, the exception message field is set to "", so it illustrated the falsy value thing.
There was a problem hiding this comment.
Okay, right 😅 Might be good to explain this in a comment, as this is a bit specific and hard to read. 😛
There was a problem hiding this comment.
I think I'll update the logic to be in 2 steps to be clearer, and add comments 😄:
if value is None:
continue
if value or member in shape.required_members:
remaining_params[member] = valueThere was a problem hiding this comment.
As a side-note, while working on the backup service which is a rest-json service, I noticed that it returns null values exception members in the raw body 😅 (but Botocore does not deserialize them) so I think the logic is different there, and is not entirely derived from our JSONResponseSerializer. I'll investigate a bit later! For boto3, this is not an issue as it does not deserialize null fields back to the user.
bentsku
force-pushed
the
asf-fix-required-exception-members
branch
from
aa9383f to
18d1256
Compare
alexrashed
left a comment
alexrashed
left a comment
There was a problem hiding this comment.
Really good catch and great tests! 🐛🔨
| continue | ||
|
|
||
| if value: | ||
| if value or (member in shape.required_members and value is not None): |
There was a problem hiding this comment.
Okay, right 😅 Might be good to explain this in a comment, as this is a bit specific and hard to read. 😛
alexrashed
added
the
notes: skip
Motivation
Follow up from #13180
When working on multi-protocol support, it came to light that we shouldn't serialize empty members on exception structures. However, one test started failing for the
verifiedpermissionsservice after we merged this.This is because the shape of that exception explicitly declares
requiredmembers on the exception structure shape:{ "ResourceNotFoundException":{ "type":"structure", "required":[ "message", "resourceId", "resourceType" ], "members":{ "message":{"shape":"String"}, "resourceId":{ "shape":"String", "documentation":"<p>The unique ID of the resource referenced in the failed request.</p>" }, "resourceType":{ "shape":"ResourceType", "documentation":"<p>The resource type of the resource referenced in the failed request.</p>" } }, "documentation":"<p>The request failed because it references a resource that doesn't exist.</p>", "exception":true } }I've added unit tests to validate the fix and confirm the behavior, and validated that it fixed the test of the dependency.
Changes
Nonevalue is still not serializedsmithy-rpc-v2-cboreven though we don't have a service supporting required members yet