feat(parser): support for S3 Event Notifications via EventBridge

[ivica-k]

Issue number: #1656

Summary

Changes

adds support for S3 event notifications through EventBridge

User experience

from aws_lambda_powertools.utilities.parser import parse, envelopes
from aws_lambda_powertools.utilities.parser.models import S3EventNotificationEventBridgeDetailModel

from aws_lambda_powertools.utilities.data_classes import S3EventBridgeEvent

def lambda_handler(event: dict, context):
    dataclass_event = S3EventBridgeEvent(event)
    parsed_event: S3EventNotificationEventBridgeDetailModel = parse(
        model=S3EventNotificationEventBridgeDetailModel,
        envelope=envelopes.EventBridgeEnvelope,
        event=event
    )

    print(f"From dataclass: {dataclass_event.account}")
    print(f"From dataclass: {dataclass_event.detail.object.etag}")
    print(f"From dataclass: {dataclass_event.detail.bucket.name}")

    print(f"From pydantic model: {parsed_event.object.etag}")
    print(f"From pydantic model: {parsed_event.bucket.name}")


if __name__ == "__main__":
    event = {
        "version": "0",
        "id": "17793124-05d4-b198-2fde-7ededc63b103",
        "detail-type": "Object Created",
        "source": "aws.s3",
        "account": "111122223333",
        "time": "2021-11-12T00:00:00Z",
        "region": "ca-central-1",
        "resources": [
            "arn:aws:s3:::DOC-EXAMPLE-BUCKET1"
        ],
        "detail": {
            "version": "0",
            "bucket": {
                "name": "DOC-EXAMPLE-BUCKET1"
            },
            "object": {
                "key": "example-key",
                "size": 5,
                "etag": "b1946ac92492d2347c6235b4d2611184",
                "version-id": "IYV3p45BT0ac8hjHg1houSdS1a.Mro8e",
                "sequencer": "617f08299329d189"
            },
            "request-id": "N4N7GDK58NMKJ12R",
            "requester": "123456789012",
            "source-ip-address": "1.2.3.4",
            "reason": "PutObject"
        }
    }

    lambda_handler(event=event, context=None)

Prints:

From dataclass: 111122223333
From dataclass: b1946ac92492d2347c6235b4d2611184
From dataclass: DOC-EXAMPLE-BUCKET1
From pydantic model: b1946ac92492d2347c6235b4d2611184
From pydantic model: DOC-EXAMPLE-BUCKET1

Notes

I made this PR while code changes are half done to make the discussion about further changes easier. Tests and documentation will be added later, once these functional changes are deemed good.

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)

Acknowledgement

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.

[boring-cyborg]

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

[heitorlessa]

hey @ivica-k - THANK YOU, excited to review your first PR shortly. I've just added python to your code snippets to enable syntax highlighting.

[heitorlessa]

hey @ivica-k - reviewing it now... let's break this into two PRs: one for Parser and one for Event Source Data Classes.

[heitorlessa]

Looks great! The only changes we'll need to ensure this works as expected are:

• A real event generated under tests/events (replace accountID/resource name with dummy ones) - S3 Object Notification Example
• Test for Parser to guarantee mandatory vs optional field work as expected and don't fail at runtime with validation error: S3 Model test example

Lemme know if you need any help with it!

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (4b0914b) 97.44% compared to head (fd4d3e2) 97.45%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1982   +/-   ##
========================================
  Coverage    97.44%   97.45%           
========================================
  Files          146      146           
  Lines         6660     6685   +25     
  Branches       478      478           
========================================
+ Hits          6490     6515   +25     
  Misses         134      134           
  Partials        36       36           

Impacted Files	Coverage Δ
...bda_powertools/utilities/parser/models/__init__.py	100.00% <100.00%> (ø)
...powertools/utilities/parser/models/event_bridge.py	100.00% <100.00%> (ø)
...ws_lambda_powertools/utilities/parser/models/s3.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[heitorlessa]

There's only one medium change to make and I'm doing that now to speed up the merge - S3EventNotificationEventBridgeModel is the parent and preferred piece we document and test , because the envelope will throw away EventBridge metadata a customer might want to have.

S3EventNotificationEventBridgeDetailModel would be similar to SqsRecordModel, where you want to dig in where the payload is only, discarding everything else (e.g., SqsModel).

With that in mind (if it's any clearer), we first test the parent model S3EventNotificationEventBridgeModel, and then do a specific one for its associated envelope.

I've changed the exports and the docs, I'll work on the tests.

PS: Parser testing suite is something I'd love to redo entirely

[heitorlessa]

What I've changed

• Added tests _no_envelope and replaced hardcoded data from event with the actual raw event - this is something I wish we've done before in the Parser's test suite, as it's brittle if we ever generate events to test contracts on a regular basis
• Fixed field type in S3EventNotificationEventBridgeModel - I caught this when doing the tests without envelope, since detail = S3EventNotificationEventBridgeModel doesn't allow for Pydantic to recursively instantiate and validate nested models. It's now detail: S3EventNotificationEventBridgeDetailModel.
• Removed None tests from new no_envelope tests - Pydantic will raise a validation error if a field is typed as required but it's missing in the payload, so we don't need to test as any discrepancy will result in a validation error. That is, fields with Optional[] don't need a specific assertion.

Last items to review on Monday

• I'm almost sure we don't need those tests that use an Envelope, but I'm dizzy from meetings and can't make that call today.
  • Why: The reason for Envelope is for customers to bring their own custom model, so we'd apply their model in a particular field (detail) to which we'd discard all parent metadata (e.g., event.id) and return their Model already initialized and validated. Customers won't be able to bring their own models in this particular event per se as it's controlled by S3. If they could, they could still use the EventBridgeEnvelope which is already tested elsewhere.
• Fix mypy typing on Type[Model].

If we don't speak again, have a fantastic weekend @ivica-k !!

[ivica-k]

Damn, it would have been easier for you to make the whole PR from scratch than to fix my things...

Thank you for the great explanation of changes and have a great weekend :)

If it makes any difference, I agree with your point about testing the envelope; as you said, EventBridgeEnvelope is already test and working.

[heitorlessa]

Not at all @ivica-k - getting the events, the structure, and kick starting everything is paramount... we can't thank you enough ;-)

We'll finish on Monday and move to the data classes to see if we can fix that super odd GH Actions checkout issue.

🙏

[ivica-k]

Hey @heitorlessa I feel bad that you're doing all of these commits; is there a chance I could participate at this point?

[heitorlessa]

Let me ping you on Discord and we can do it together - I didn't really need do this a50116c, but I figured this is the best opportunity to start paying some tech debt I always wanted to (I don't like the way tests are done in Parser).

There's one last thing I'm doing now which is making sure Mypy works.

[heitorlessa]

Okay finished, mind having a review to see if I missed anything?

What have I done?

• Replaced the original tests with _no_envelope tests I added
• Paid an old typing tech debt with EventBridgeModel which mypy detected in this PR - S3EventNotificationEventBridgeModel overrides detail with a model, which we should've addressed ages ago. A lesson I learned here was there's a surprising number of customers using Pydantic but not Mypy, otherwise there would've been an issue created related to EventBridgeModel.
• Paid one of the oldest tech debt in migrating functional tests to unit tests. Ideally, I should've created multiple issues for this, but I had 10 spare minutes and I thought this would be the perfect opportunity to do now.
  • Tech debt background: Parser's functional test use event_parser and standalone functions to do assertions. This should be decoupled to ease writing tests + perf - unit tests to test model validation, functional tests to test parse, event_parser with/without envelope for any model. HELP: This is an area I'd love to get help as there's plenty to decouple here in the codebase. Similarly to event source data classes that you're also working on.

Special note. Never feel bad about this. We wouldn't be this far without your help. There's also so many competing priorities that if you hadn't started this PR we wouldn't even touch this feature until later in the year, even more so that I'm so happy we paid two tech debts that will pave the road for a better code base. Typically, if there's an issue related I'd ask any contributor to kindly factor these in, but we haven't created an issue to cover these tech debts hence why I did it.

[heitorlessa]

Explanation on the typing (in case you aren't familiar with mypy)

https://github.com/ivica-k/aws-lambda-powertools-python/blob/feat(event-source)-1656-s3-event-eventbridge/aws_lambda_powertools/utilities/parser/types.py#L17

AnyInheritedModel = Union[Type[BaseModel], BaseModel]
RawDictOrModel = Union[Dict[str, Any], AnyInheritedModel]

Type[BaseModel] allows us to accept any model that inherits a generic model, while BaseModel allows any unbounded model (those not initialized).

class EventBridgeModel(BaseModel):
    ...
    detail: RawDictOrModel

If a customer now wants to bring their own model, this won't trigger a Mypy issue anymore:

class Order(BaseModel):
    payment_id: str


class OrderEventBridge(EventBridgeModel):
    detail: Order

What's even better... a customer can even use the superset Json type from Pydantic to deserialize JSON string into a Dict and then to their Model without any mypy complaints too

class Order(BaseModel):
    payment_id: str


class OrderEventBridge(EventBridgeModel):
    detail: Json[Order]

[heitorlessa]

@ivica-k thanks for jumping on a call with me - as we've just agreed, I'll merge this now as soon as CI checks pass, and we can move on to the data class PR one.

[boring-cyborg]

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