Skip to content
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

Bug: Parser's S3 event notifications schemas #2410

Closed
1 of 2 tasks
dreamorosi opened this issue Apr 22, 2024 · 5 comments · Fixed by #2429
Closed
1 of 2 tasks

Bug: Parser's S3 event notifications schemas #2410

dreamorosi opened this issue Apr 22, 2024 · 5 comments · Fixed by #2429
Assignees
Labels
bug Something isn't working completed This item is complete and has been merged/shipped parser This item relates to the Parser Utility

Comments

@dreamorosi
Copy link
Contributor

Summary

Based on the finding in aws-powertools/powertools-lambda-python#4156 it appears that when S3 sends a notification for an object deleted and triggers a Lambda function, the etag field is optional.

In looking at our implementation vs the types in @types/aws-lambda it seems that we have a single S3EventNotificationEventBridgeDetailSchema schema while DefinitelyTyped has multiple kinds, one for each notification type.

I'm not sure if we should add schemas for each event type like DefinitelyTyped does or have a single one, however having multiple might help with addressing the subtle differences in field presence (i.e. etag not present in delete notifications) & values (i.e. deletion-type equals Permanently Deleted).

For now I'm opening this as a feature request, but if we decide to just make etag optional and keep the schema as-is, we can convert this as maintenance type.

Why is this needed?

So that customers can use the schema to parse their events.

Which area does this relate to?

Parser

Solution

No response

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

@dreamorosi dreamorosi added discussing The issue needs to be discussed, elaborated, or refined parser This item relates to the Parser Utility labels Apr 22, 2024
@am29d
Copy link
Contributor

am29d commented Apr 22, 2024

Hmm that's odd because the @types/aws-lambda has a required field etag on the notification event payload.

It would help to separate them and also narrow down the some of the field values to constants. Let me run some test events to see, I am a bit uncertain why the it happens "sometime" as stated in the original issue in PT python.

@am29d am29d self-assigned this Apr 22, 2024
@am29d am29d added confirmed The scope is clear, ready for implementation and removed discussing The issue needs to be discussed, elaborated, or refined labels Apr 22, 2024
@dreamorosi
Copy link
Contributor Author

Agree, the sometime is throwing me off as well.

It would make more sense to me that the field is no longer there because the corresponding object has been removed, but the intermittent aspect is strange.

@am29d am29d added this to the Parser - GA Release milestone Apr 24, 2024
@am29d
Copy link
Contributor

am29d commented Apr 24, 2024

I have decided to set the eTag to optional for now. The @types/aws-lambda are not correct thus we can't rely on them. In addition, splitting the event types would introduce redundancy between the events for common fields, or additional complexity if we decide to split them based on base and specific fields. It'd be hard to read them.

I need to think of possible scenarios how we could reproduce real event structures, because there is no other way yet to get them correct.

@am29d am29d changed the title Feature request: Parser's S3 event notifications schemas Bug: Parser's S3 event notifications schemas Apr 24, 2024
@am29d am29d added the bug Something isn't working label Apr 24, 2024
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

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 github-actions bot added pending-release This item has been merged and will be released soon and removed confirmed The scope is clear, ready for implementation labels Apr 24, 2024
Copy link
Contributor

This is now released under v2.1.1 version!

@github-actions github-actions bot added completed This item is complete and has been merged/shipped and removed pending-release This item has been merged and will be released soon labels May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working completed This item is complete and has been merged/shipped parser This item relates to the Parser Utility
Projects
2 participants