feat(event_source): support custom json_deserializer; add json_body in SQSEvent #2200
Conversation
pull-request-size
bot
added
the
size/M
| @property | ||
| def json_body(self) -> Dict: | ||
| """Parses the submitted body as json""" | ||
| try: | ||
| if self._json_data is None: | ||
| self._json_data = self._json_deserializer(self["body"]) | ||
| return self._json_data | ||
| except Exception: | ||
| return self["body"] |
There was a problem hiding this comment.
The reason I add a try/except block here is because it could be a simple string that cannot be deserialized as json.
Should we consider this for the other json_body fields in Kinesis/Kafka/APIGW? Should we return the plain body if the deserialization fails?
I would like to hear from you.
There was a problem hiding this comment.
We should fail hard here to give the caller a chance to handle this error correctly.
Reasoning in a mental model fashion:
- Do we offer a non-JSON body property e.g.,
body?- If so,
json_bodyshould fail if it can't deserialize it
- If so,
- If the customer received a poison pill record, a non-JSON string, will they be given the chance to handle a JSON deserialization error?
- If we return
self["body"], it can result to a totally different error we can't even anticipate, e.g.,json_body["blah"]
- If we return
As a rule of thumb, you'd want to propagate the error as close to the actual problem so they instinctively know how to handle it. If we catch all, we're masking the actual error and leading them to an unknown error later on.
tiny note: until Python 3.11, try/catch has a performance overhead even if it's tiny, so we can also improve here by letting them do it (not accidentally twice).
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #2200 +/- ##
===========================================
- Coverage 97.47% 97.46% -0.01%
===========================================
Files 148 149 +1
Lines 6882 6895 +13
Branches 505 506 +1
===========================================
+ Hits 6708 6720 +12
Misses 137 137
- Partials 37 38 +1
☔ View full report in Codecov by Sentry. |
|
|
||
| from aws_lambda_powertools.shared.headers_serializer import BaseHeadersSerializer | ||
|
|
||
|
|
||
| class DictWrapper(Mapping): | ||
| """Provides a single read only access to a wrapper dict""" | ||
|
|
||
| def __init__(self, data: Dict[str, Any]): | ||
| def __init__(self, data: Dict[str, Any], json_deserializer: Optional[Callable] = None): |
There was a problem hiding this comment.
docstring ;) we apparently missed back in the days for DictWrapper, but given the json_deserializer will be added to all data classes, we can propagate the docstring to everyone benefits from it.
pushing that change
pull-request-size
bot
added
size/L
heitorlessa
changed the title
Issue number: #2199
Summary
Changes
This PR aims to add support for a custom
jsondeserialization method in the event source classes.User experience
BEFORE
AFTER
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.