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

fix(parser): remove AttributeError validation from event_parser function #5742

Merged

Conversation

anafalcao
Copy link
Collaborator

@anafalcao anafalcao commented Dec 16, 2024

Issue number: #5705

Summary

Changes

Please provide a summary of what's being changed

Refine exception handling in the event_parser decorator and parser function. Replace the broad AttributeError catch with specific handling for ValidationError, InvalidModelTypeError, TypeError, and ValueError. This change ensures that Pydantic validation errors and invalid model type errors are raised as-is, while other type and value errors during model instantiation are wrapped in an InvalidModelTypeError. The AttributeError catch has been removed to prevent masking unrelated exceptions.

User experience

Please share what the user experience looks like before and after this change

Users encounter confusing InvalidModelTypeError exceptions, even when the event parsing succeeds. Unrelated exceptions within the handler function are incorrectly attributed to event parsing issues. The PR wants to provide the users more accurate error reporting. Exceptions unrelated to event parsing are no longer masked by parser, allowing for easier debugging and a clearer understanding of where errors originate in their code.

Checklist

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

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.

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 16, 2024
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.18%. Comparing base (1f3fd8d) to head (d3c63ca).
Report is 4 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #5742   +/-   ##
========================================
  Coverage    96.17%   96.18%           
========================================
  Files          231      231           
  Lines        10922    10919    -3     
  Branches      2019     2019           
========================================
- Hits         10504    10502    -2     
+ Misses         329      328    -1     
  Partials        89       89           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@boring-cyborg boring-cyborg bot added the tests label Dec 17, 2024
@pull-request-size 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 Dec 17, 2024
@pull-request-size 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 Dec 17, 2024
@anafalcao anafalcao marked this pull request as ready for review December 19, 2024 12:53
@anafalcao anafalcao requested a review from a team as a code owner December 19, 2024 12:53
@github-actions github-actions bot added the bug Something isn't working label Dec 20, 2024
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @anafalcao! Thanks a lot for working on this. We need to make some small changes before we merge!

tests/functional/parser/test_parser.py Outdated Show resolved Hide resolved
aws_lambda_powertools/utilities/parser/parser.py Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 20, 2024
@leandrodamascena leandrodamascena changed the title fix(parser): refine exception handling in function and decorator fix(parser): remove AttributeError validation from event_parser function Dec 20, 2024
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work @anafalcao! APPROVED!

@leandrodamascena leandrodamascena merged commit a44bd6d into aws-powertools:develop Dec 20, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: event_parser decorator catches an irrelevant AttributeError and raises InvalidModelTypeError exception.
2 participants