-
Notifications
You must be signed in to change notification settings - Fork 429
Feature request: Improve support for SQS-wrapped S3 event notifications #2078
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Thanks for opening your first issue here! We'll come back to you as soon as we can. |
Thank you Alan for following up on that comment and creating this feature — feel free to go ahead with a PR and we can get it released in our next release ;-)
<3
…On Mon, 3 Apr 2023 at 17:28, Thomas Von Heill ***@***.***> wrote:
Really love this suggestion. Even passing your own user defined
type/pydantic definition/interface to extend the sqs typing would be great
and help out the development process. Thank you for this suggestion
—
Reply to this email directly, view it on GitHub
<#2078 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBB6MLBHDN53H6VHBZTW7LUB7ANCNFSM6AAAAAAWQTEF5E>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***
com>
|
Hi @theipster! We are planning the next release and would like to know if you have the bandwidth to submit this PR! It would be amazing to have your contribution here. But don't worry if you can't (we know everyone is busy), we can do it and give you credit. |
Hi @leandrodamascena - no guarantees, but I can try to squeeze something in! 🙂 The class itself shouldn't be a problem, but I'm not too familiar with the existing test suite, so any high level guidance on what categories of tests is appropriate for this change would be greatly appreciated - thanks! |
I saw you submitted the PR and that's awesome! What do you think converting the PR from draft to work in progress? So we can work together to check the best user experience, to write the tests and documentation. We use pytest to write our tests; you can see examples of how we do this. Example: S3 Object Event You can copy these files and change the event and fields to test that the parser is parsing the fields as expected. Please reach out if you need additional guidance and we can work together to get this done. |
|
Uh oh!
There was an error while loading. Please reload this page.
Use case
Originally discussed in #1656 (comment).
S3 event notifications can be sometimes be ingested into Lambda via an intermediary such as an SQS queue (i.e. Lambda event source), for various architectural reasons - batching, retries, etc.. However, from the Lambda function's perspective, the intermediary might not be too important; what's important is the S3 event notification itself.
With the current Powertools built-ins, it is possible to parse the S3 event notification data structure out of the SQS message data structure, but this requires some awkward boilerplate code to chain the two data structures together (mostly due to the JSON-formatted wrapper in the middle):
Solution/User Experience
A simple quick-win to the user-experience would be to make a new data model available as a built-in:
This would be compatible with the existing framework and could be used as just another model, e.g.:
Note: see this comment about Mypy compatibility, although I believe it may no longer be a concern due to a recent typing improvement.
Alternative solutions
If there's a way to skip the intermediate handler function, that would be even nicer (but see trade-off below).
In other words, rather than needing to define a
def record_handler(record: SqsS3EventNotificationModel)
that iterates through therecord.body.Records
, it would be nice to just directly define adef record_handler(record: S3Model)
and let the processor unwrap everything instead.This would keep the business logic clean, and not having to care whether it came via SQS or EventBridge, etc..
However, the trade-off to this even simpler interface would be that any event metadata (from both the SQS event and the S3 event notification) would be unavailable.
Acknowledgment
The text was updated successfully, but these errors were encountered: