feat(parser): add support for SQS-wrapped S3 event notifications #2108

theipster · 2023-04-11T11:56:21Z

Issue number: #2078

Summary

Changes

Exposes a new data model for SQS-wrapped S3 event notifications.

User experience

Please see #2078 description.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Meet tenets criteria
I have performed a self-review of this change
Changes have been tested
Changes are documented
PR title follows conventional commit semantics

Is this a breaking change?

RFC issue number:

Checklist:

Migration process documented
Implement warnings (if it can live side by side)

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.

Signed-off-by: Alan Ip <[email protected]>

boring-cyborg · 2023-04-11T11:56:24Z

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our AWS Lambda Powertools Discord: Invite link

codecov-commenter · 2023-04-11T12:02:29Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (32666e4) 97.47% compared to head (9c759d7) 97.47%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2108   +/-   ##
========================================
  Coverage    97.47%   97.47%           
========================================
  Files          148      149    +1     
  Lines         6882     6891    +9     
  Branches       505      505           
========================================
+ Hits          6708     6717    +9     
  Misses         137      137           
  Partials        37       37

Impacted Files	Coverage Δ
...bda_powertools/utilities/parser/models/__init__.py	`100.00% <100.00%> (ø)`
...s/utilities/parser/models/s3_event_notification.py	`100.00% <100.00%> (ø)`
...s_lambda_powertools/utilities/parser/models/sqs.py	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

The latter represents an event payload, i.e. a list of records.

Since the SqsS3EventNotificationModel is only responsible for chaining (rather than redefining) the existing SqsModel and S3Model models, let's re-use both the existing stubs and their corresponding assertions.

theipster · 2023-04-14T02:32:02Z

@leandrodamascena Thanks for your guidance. Do these latest changes look roughly sane to you? 🙂

I've cheekily borrowed the S3Model assertions, seeing as the primary concern of SqsS3EventNotificationModel is more about chaining SqsRecordModel and S3Model together, rather than the properties themselves.

Signed-off-by: Alan Ip <[email protected]>

leandrodamascena · 2023-04-14T14:04:43Z

@leandrodamascena Thanks for your guidance. Do these latest changes look roughly sane to you? slightly_smiling_face

I've cheekily borrowed the S3Model assertions, seeing as the primary concern of SqsS3EventNotificationModel is more about chaining SqsRecordModel and S3Model together, rather than the properties themselves.

Hi @theipster! Surely these things make sense. I didn't get a chance to see why the CI is crashing, it looks like mypy complaining about types. But don't worry, I always suffer from mypy errors also 😢 .

I will look into this today and we can work together to complete it and release in the next Powertools version. So far, very good job 👏!

leandrodamascena · 2023-04-19T21:56:42Z

Hi @theipster! I'm trying to send commits to this PullRequest and I can't because it looks like you didn't allow the maintainers to do that.

There is an option in this pull request that must be checked to allow this. Can you enable it, please?

leandrodamascena · 2023-04-24T22:34:39Z

@theipster hi!! When you have time, could you pls check my last comment?

Here's the patch I'm looking to apply to this PR to test things out: https://gist.github.com/leandrodamascena/69cad79e01fcff4697c3672f5db14f67

theipster · 2023-04-25T02:03:16Z

@leandrodamascena Apologies for the late reply. I've now enabled this setting; thanks for your patch.

Is there any reason why mypy requires Sequence[] here, whereas it's fine with using List[] elsewhere, e.g. for S3Model? Is there some kind of exception in place for S3Model?

…ation-model

leandrodamascena · 2023-04-25T20:57:40Z

@leandrodamascena Apologies for the late reply. I've now enabled this setting; thanks for your patch.

Is there any reason why mypy requires Sequence[] here, whereas it's fine with using List[] elsewhere, e.g. for S3Model? Is there some kind of exception in place for S3Model?

Hi @theipster no worries! Thanks for enabling this.

The problem is not in the S3Model and yes, the code works as expected. But to avoid issues with different data types, we use mypy, a library for static type checking. This helps us identify potential bugs during the development process and make the code more reliable. Because of this, the code seems to work but when mypy checks the types it encounters some issues.

The problem we're encountering is due to the Records field in the parent class, SqsModel, which is set as a List. Since List is an invariant data type, we're running into issues when trying to override this field with a str type Records field in the SqsS3EventNotificationModel class. As a result, mypy is complaining about this.

I changed the field type to Sequence in SqsModel and now the errors have stopped. Sequence is a more generic data type and List is a subclass of it.

I don't know if I made myself very clear about this, but I leave some references here:

https://blog.daftcode.pl/covariance-contravariance-and-invariance-the-ultimate-python-guide-8fabc0c24278

https://mypy.readthedocs.io/en/stable/common_issues.html#variance

leandrodamascena · 2023-04-25T20:59:55Z

tests/functional/parser/test_sqs_s3_event_notification.py

+def test_handle_sqs_json_body_containing_s3_notifications():
+    sqs_event_dict = load_event("sqsEvent.json")
+    s3_event_notification_dict = load_event("s3Event.json")
+    for record in sqs_event_dict["Records"]:
+        record["body"] = json_serialize(s3_event_notification_dict)
+
+    parsed_event: SqsS3EventNotificationModel = handle_sqs_json_body_containing_s3_notifications(
+        sqs_event_dict, LambdaContext()
+    )
+
+    assert len(parsed_event.Records) == 2
+    for parsed_sqs_record in parsed_event.Records:
+        assert_s3(parsed_sqs_record.body)


I would like an opinion here @rubenfonseca @heitorlessa. I think this test is ok, do you see another way to optimize this?

Looks ok to me

This is a good opportunity to move to the new unit tests since all we should care here is whether the new Model can be instantiated correctly (along with its ramifications, JSON deserialization etc.).

Following this, all you'd need to do here is:

Create a new S3->SQS JSON event file under tests/events

Create a new test at tests/unit/parser/test_s3.py

Test whether the model can be initialized without any problem (we already test both SQS and S3 Models)

Test whether an invalid JSON in the S3 data will lead to a model validation failure

Previously, we were shoving everything under tests/functional where we have a lot of boilerplate like @event_parser and these functions, when in fact we're merely testing the Model since we already have separate tests for the @event_parser... which in this case is a proper functional test.

Hope that makes sense ;)

It makes perfect sense @heitorlessa! I changed the files as per this feedback

docs/utilities/parser.md

aws_lambda_powertools/utilities/parser/models/__init__.py

leandrodamascena · 2023-04-26T16:41:55Z

docs/utilities/parser.md

+| Model name                              | Description                                                                           |
+| --------------------------------------- | ------------------------------------------------------------------------------------- |
+| **AlbModel**                            | Lambda Event Source payload for Amazon Application Load Balancer                      |
+| **APIGatewayProxyEventModel**           | Lambda Event Source payload for Amazon API Gateway                                    |
+| **APIGatewayProxyEventV2Model**         | Lambda Event Source payload for Amazon API Gateway v2 payload                         |
+| **CloudwatchLogsModel**                 | Lambda Event Source payload for Amazon CloudWatch Logs                                |
+| **DynamoDBStreamModel**                 | Lambda Event Source payload for Amazon DynamoDB Streams                               |
+| **EventBridgeModel**                    | Lambda Event Source payload for Amazon EventBridge                                    |
+| **KafkaMskEventModel**                  | Lambda Event Source payload for AWS MSK payload                                       |
+| **KafkaSelfManagedEventModel**          | Lambda Event Source payload for self managed Kafka payload                            |
+| **KinesisDataStreamModel**              | Lambda Event Source payload for Amazon Kinesis Data Streams                           |
+| **KinesisFirehoseModel**                | Lambda Event Source payload for Amazon Kinesis Firehose                               |
+| **LambdaFunctionUrlModel**              | Lambda Event Source payload for Lambda Function URL payload                           |
+| **S3EventNotificationEventBridgeModel** | Lambda Event Source payload for Amazon S3 Event Notification to EventBridge.          |
+| **S3Model**                             | Lambda Event Source payload for Amazon S3                                             |
+| **S3ObjectLambdaEvent**                 | Lambda Event Source payload for Amazon S3 Object Lambda                               |
+| **S3SqsEventNotificationModel**         | Lambda Event Source payload for S3 event notifications wrapped in SQS event (S3->SQS) |
+| **SesModel**                            | Lambda Event Source payload for Amazon Simple Email Service                           |
+| **SnsModel**                            | Lambda Event Source payload for Amazon Simple Notification Service                    |
+| **SqsModel**                            | Lambda Event Source payload for Amazon SQS                                            |


I took the opportunity to organize the names of the models in alphabetical order.

leandrodamascena · 2023-04-26T16:45:37Z

Hi @theipster, congrats on this amazing job, you've done most of the work.
I just made some small changes to bring closer the developer experience we have and the project architecture.

Thank you very much for your time dedicated to this PR. Do you have any considerations before we make the last review and merge this PR?

theipster · 2023-04-26T16:54:46Z

@leandrodamascena Thank you for your support in making this go much more smoothly!

Nothing to add from my side, LGTM ✅

rubenfonseca · 2023-05-01T08:31:57Z

tests/events/sqsS3Event.json

Should we rename this file to s3SqsEvent.json for consistency?

rubenfonseca · 2023-05-01T08:32:28Z

tests/unit/parser/test_s3.py

+
+
+def test_s3_sqs_event_notification():
+    raw_event = load_event("sqsS3Event.json")


Change this line too

rubenfonseca · 2023-05-01T08:32:34Z

tests/unit/parser/test_s3.py

+
+
+def test_s3_sqs_event_notification_body_invalid_json():
+    raw_event = load_event("s3Event.json")


Change this line too

This test was wrong. I was using a S3 JSON file to simulate an invalid body, but it is supposed to use an invalid json body (like a string).

Fixed.

If you compare against my original code in commit 251d8ba, I used sqsEvent.json, which is another way to solve this problem.

This is also why I originally named the classes SqsS3... (because the SQS payload literally wraps the S3 payload) rather than S3Sqs... (the direction of traffic). They're simply different ways of conceptualising the same scenario, neither is particularly right or wrong I guess!

You're right @theipster, I used the wrong words to explain why I changed the test. It's not a question of being right or wrong, it's just different ways of solving the same question.

Thank you very much for this feedback, it will help me to avoid mistakes like this in the future.

rubenfonseca

LGTM

boring-cyborg · 2023-05-02T10:07:53Z

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

theipster added 4 commits January 17, 2023 00:03

Introduce model for SQS-wrapped S3 event notifications

502ce6d

Signed-off-by: Alan Ip <[email protected]>

Export SqsS3EventNotificationModel

72b127d

Signed-off-by: Alan Ip <[email protected]>

Merge branch 'develop' into s3-event-notification-model

a13e190

Signed-off-by: Alan Ip <[email protected]>

Re-order exports into (slightly more) alphabetical order

665a99d

Signed-off-by: Alan Ip <[email protected]>

pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 11, 2023

github-actions bot added the feature New feature or functionality label Apr 11, 2023

theipster marked this pull request as ready for review April 13, 2023 23:24

theipster requested a review from a team as a code owner April 13, 2023 23:24

theipster requested review from rubenfonseca and removed request for a team April 13, 2023 23:24

Differentiate between *RecordModel vs *Model

6c426dd

The latter represents an event payload, i.e. a list of records.

pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 14, 2023

Add positive functional tests for SqsS3EventNotificationModel

be69e4c

Since the SqsS3EventNotificationModel is only responsible for chaining (rather than redefining) the existing SqsModel and S3Model models, let's re-use both the existing stubs and their corresponding assertions.

boring-cyborg bot added the tests label Apr 14, 2023

pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 14, 2023

theipster added 3 commits April 14, 2023 03:02

Silly me, I may have overcomplicated this...

ad2ba2c

Fix formatting

70e7b1c

Add negative functional tests for SqsS3EventNotificationModel

251d8ba

leandrodamascena linked an issue Apr 14, 2023 that may be closed by this pull request

Feature request: Improve support for SQS-wrapped S3 event notifications #2078

Closed

2 tasks

Explicitly import local method

1116552

Signed-off-by: Alan Ip <[email protected]>

Merge remote-tracking branch 'upstream/develop' into s3-event-notific…

598b90d

…ation-model

parser(sqs3): fixing mypy errors and adding documentation

31eaa2e

boring-cyborg bot added the documentation Improvements or additions to documentation label Apr 25, 2023

leandrodamascena reviewed Apr 25, 2023

View reviewed changes

heitorlessa reviewed Apr 26, 2023

View reviewed changes

docs/utilities/parser.md Outdated Show resolved Hide resolved

parser(sqs3): improving documentation, refactoring classname and tests

f118583

pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 26, 2023

leandrodamascena changed the title ~~feat(parser): improve support for SQS-wrapped S3 event notifications~~ feat(parser): add support for SQS-wrapped S3 event notifications Apr 26, 2023

leandrodamascena reviewed Apr 26, 2023

View reviewed changes

aws_lambda_powertools/utilities/parser/models/__init__.py Show resolved Hide resolved

leandrodamascena reviewed Apr 26, 2023

View reviewed changes

leandrodamascena requested review from heitorlessa and removed request for rubenfonseca April 26, 2023 16:45

leandrodamascena requested a review from rubenfonseca April 27, 2023 08:11

rubenfonseca suggested changes May 1, 2023

View reviewed changes

leandrodamascena added 2 commits May 1, 2023 18:55

tests: addressing Ruben's feedback

26cf2b7

Merge branch 'develop' into s3-event-notification-model

9c759d7

rubenfonseca self-requested a review May 2, 2023 10:07

leandrodamascena self-requested a review May 2, 2023 10:07

rubenfonseca approved these changes May 2, 2023

View reviewed changes

leandrodamascena merged commit ea91e5c into aws-powertools:develop May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(parser): add support for SQS-wrapped S3 event notifications #2108

feat(parser): add support for SQS-wrapped S3 event notifications #2108

theipster commented Apr 11, 2023

boring-cyborg bot commented Apr 11, 2023

codecov-commenter commented Apr 11, 2023 •

edited

Loading

theipster commented Apr 14, 2023

leandrodamascena commented Apr 14, 2023

leandrodamascena commented Apr 19, 2023

leandrodamascena commented Apr 24, 2023

theipster commented Apr 25, 2023

leandrodamascena commented Apr 25, 2023

leandrodamascena Apr 25, 2023

rubenfonseca Apr 26, 2023

heitorlessa Apr 26, 2023

leandrodamascena Apr 26, 2023

leandrodamascena Apr 26, 2023

leandrodamascena commented Apr 26, 2023

theipster commented Apr 26, 2023

rubenfonseca May 1, 2023

leandrodamascena May 1, 2023

rubenfonseca May 1, 2023

leandrodamascena May 1, 2023

rubenfonseca May 1, 2023

leandrodamascena May 1, 2023

theipster May 2, 2023

leandrodamascena May 2, 2023

rubenfonseca left a comment

boring-cyborg bot commented May 2, 2023



		def test_s3_sqs_event_notification():
		raw_event = load_event("sqsS3Event.json")



		def test_s3_sqs_event_notification_body_invalid_json():
		raw_event = load_event("s3Event.json")

feat(parser): add support for SQS-wrapped S3 event notifications #2108

feat(parser): add support for SQS-wrapped S3 event notifications #2108

Conversation

theipster commented Apr 11, 2023

Summary

Changes

User experience

Checklist

Acknowledgment

boring-cyborg bot commented Apr 11, 2023

codecov-commenter commented Apr 11, 2023 • edited Loading

Codecov Report

theipster commented Apr 14, 2023

leandrodamascena commented Apr 14, 2023

leandrodamascena commented Apr 19, 2023

leandrodamascena commented Apr 24, 2023

theipster commented Apr 25, 2023

leandrodamascena commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandrodamascena commented Apr 26, 2023

theipster commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenfonseca left a comment

Choose a reason for hiding this comment

boring-cyborg bot commented May 2, 2023

codecov-commenter commented Apr 11, 2023 •

edited

Loading