You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Feature request: Support of custom serizialization/deserisialization (object -> dict) on idempotency decorator 'idempotent_function' #2886
I use Pydantic as a representation of my DTO. As such, I receive and return pydantic objects as part of my internal function calls.
Currently, the idempotency decorator supports returning dictionary only (this was also discussed here #2164 ).
I would like to be able bring my own serialization logic in order to support custom DTO implementations.
Solution/User Experience
User will be able to add a serailizer/deserializer in order to control how it is done using simple function parameters:
As there is currently an existing implementation of input serialization (object -> dict), not supplying any input_serializer will use the existing "best effort" implementation.
Alternative solutions
User will be able to add a serailizer/deserializer in order to control how it is done using the IdempotencyConfig:
Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link
heey @aradyaron firstly, big welcome and thank you for a nicely detailed feature request too!
If you have bandwidth, we'd love a PR on this so we can prioritize it (Observability Provider and Event Handler OpenAPI are taking most of our bandwidth now).
Two quick UX notes for now (might have more during implementation):
It'd be best to have it under IdempotencyConfig first - some customers have several functions annotated and would benefit it. We can add on a per function as an override so we can review the UX on the fly.
Start with output_serializer and output_deserializer
input_serializer could become error prone depending on the storage being used (we're adding Redis this year) or being switched to. It needs more feedback from customers: whether json_serializer makes more sense (common in other Powertools for AWS features), types of non-JSON data that isn't covered today (use cases), etc.
I'll add this configuration as part of IdempotencyConfig
Regarding input_serializer, as serialization in this case means from object to dictionary, is there any other option to add to it? For now the assumption of both input and output is json/dictionary compatible.
Awesome! I had a piece of paper to think through this design now for the last 10m and I was wrong in recommending the global config -- it's best in the decorator as you initially suggested.
I can't think of an use case where you'd want to use the same PaymentOutput.parse_obj globally, since every annotated function will likely have a different DTO or portions of it -- easy to make a mistake here.
Ah btw, I'm on Discord if you ever get stuck -- Idempotency is the most complex part of our codebase.
Idempotency has two major areas I wish we could refactor to make contributions easier: 1/ Decouple data transformation within IdempotencyHandler, 2/ Create a private testing utilities to make it easier to author functional tests (we went too far with Pytest fixtures here).
Until we manage to make these changes, any of us are more than happy to jump on a call and co-author PRs to lower the entry bar to improve this feature :)
@heitorlessa@aradyaron FYI, the current input serialiozation calls .json on the input class and for Pydantic V1 it works. But in Pydantic v2 its deperacred (though still works) but in v3 it will need to change to .model_dump.
This makes a lot of sense to me! I agree with all the observations, including that it should be "output" only for now. We should also make sure we don't break any existing behaviour when adding the new fields.
Note that the implementation should be 100% agnostic to the backend used for serde: in other words, we shouldn't need to import Pydantic to implement this.
@aradyaron if you want to submit a PR I would love to help you merge it! This would be a great addition.
This makes a lot of sense to me! I agree with all the observations, including that it should be "output" only for now. We should also make sure we don't break any existing behaviour when adding the new fields.
Note that the implementation should be 100% agnostic to the backend used for serde: in other words, we shouldn't need to import Pydantic to implement this.
@aradyaron if you want to submit a PR I would love to help you merge it! This would be a great addition.
Activity
boring-cyborg commentedon Jul 31, 2023
Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link
ran-isenberg commentedon Jul 31, 2023
@heitorlessa FYI, @aradyaron is a co-worker and a serverless ninja :)
+1 for this feature, it would really improve the utility.
heitorlessa commentedon Jul 31, 2023
heey @aradyaron firstly, big welcome and thank you for a nicely detailed feature request too!
If you have bandwidth, we'd love a PR on this so we can prioritize it (Observability Provider and Event Handler OpenAPI are taking most of our bandwidth now).
Two quick UX notes for now (might have more during implementation):
IdempotencyConfigfirst - some customers have several functions annotated and would benefit it. We can add on a per function as an override so we can review the UX on the fly.output_serializerandoutput_deserializerinput_serializercould become error prone depending on the storage being used (we're adding Redis this year) or being switched to. It needs more feedback from customers: whetherjson_serializermakes more sense (common in other Powertools for AWS features), types of non-JSON data that isn't covered today (use cases), etc.PS: You're lucky to work with @ran-isenberg ;)
aradyaron commentedon Jul 31, 2023
Hey @heitorlessa, thanks!
I'll open a pr on this.
IdempotencyConfiginput_serializer, as serialization in this case means from object to dictionary, is there any other option to add to it? For now the assumption of both input and output is json/dictionary compatible.heitorlessa commentedon Jul 31, 2023
Awesome! I had a piece of paper to think through this design now for the last 10m and I was wrong in recommending the global config -- it's best in the decorator as you initially suggested.
I can't think of an use case where you'd want to use the same
PaymentOutput.parse_objglobally, since every annotated function will likely have a different DTO or portions of it -- easy to make a mistake here.For
input_serializer, as of now we automatically serialize Pydantic models, Dataclasses, and Event Source Data Classes to Dictionary - are there any other use cases you're in need now? My worry now is mostly having more dials without hearing concrete use cases we're enabling.heitorlessa commentedon Jul 31, 2023
Ah btw, I'm on Discord if you ever get stuck -- Idempotency is the most complex part of our codebase.
Idempotency has two major areas I wish we could refactor to make contributions easier: 1/ Decouple data transformation within
IdempotencyHandler, 2/ Create a private testing utilities to make it easier to author functional tests (we went too far with Pytest fixtures here).Until we manage to make these changes, any of us are more than happy to jump on a call and co-author PRs to lower the entry bar to improve this feature :)
aradyaron commentedon Jul 31, 2023
Ok thanks for the tips @heitorlessa,
I'll take this into considerations in my Pr.
ran-isenberg commentedon Jul 31, 2023
@heitorlessa @aradyaron FYI, the current input serialiozation calls .json on the input class and for Pydantic V1 it works. But in Pydantic v2 its deperacred (though still works) but in v3 it will need to change to .model_dump.
heitorlessa commentedon Jul 31, 2023
yup, we'll take care of that on our V3 since we will drop support for Pydantic v1 :) I'm updating the roadmap page this week
1 remaining item
rubenfonseca commentedon Aug 16, 2023
This makes a lot of sense to me! I agree with all the observations, including that it should be "output" only for now. We should also make sure we don't break any existing behaviour when adding the new fields.
Note that the implementation should be 100% agnostic to the backend used for serde: in other words, we shouldn't need to import Pydantic to implement this.
@aradyaron if you want to submit a PR I would love to help you merge it! This would be a great addition.
ran-isenberg commentedon Aug 16, 2023
see #2951
rubenfonseca commentedon Aug 16, 2023
@ran-isenberg i need more coffee :)
github-actions commentedon Sep 5, 2023
This issue is now closed. Please be mindful that future comments are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.
github-actions commentedon Sep 8, 2023
This is now released under 2.24.0 version!