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(event_handler): Router prefix mismatch regression after Middleware feat #3302

Merged

Conversation

leandrodamascena
Copy link
Contributor

Issue number: #3255

Summary

Changes

When using a route with a prefix the Middleware execution was failing.

User experience

There are no changes to the user experience other than fixing the bug.

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.

@leandrodamascena leandrodamascena requested a review from a team as a code owner November 6, 2023 21:58
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ad9e40f) 95.26% compared to head (a9c49ef) 95.27%.
Report is 5 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3302      +/-   ##
===========================================
+ Coverage    95.26%   95.27%   +0.01%     
===========================================
  Files          207      207              
  Lines         9581     9581              
  Branches      1756      798     -958     
===========================================
+ Hits          9127     9128       +1     
+ Misses         337      335       -2     
- Partials       117      118       +1     
Files Coverage Δ
aws_lambda_powertools/event_handler/api_gateway.py 98.12% <100.00%> (+0.46%) ⬆️

... and 1 file with indirect coverage changes

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

@heitorlessa heitorlessa changed the title fix(event_handler): fixing Middlewares execution when using route prefix fix(event_handler): route prefix mismatch when Middlewares is used Nov 7, 2023
@heitorlessa heitorlessa changed the title fix(event_handler): route prefix mismatch when Middlewares is used fix(event_handler): Router prefix mismatch when Middlewares is used Nov 7, 2023
@heitorlessa heitorlessa changed the title fix(event_handler): Router prefix mismatch when Middlewares is used fix(event_handler): Router prefix mismatch when Middleware is used Nov 7, 2023
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

GREAT catch! Made suggestions to clean up the test. We ran out of time back then to get rid of parametrize, so we can do better now.

One additional suggestion I couldn't due to the UI limiting it... is to add a comment in the for loop block so we don't lose that much time figuring out that logic..

https://github.com/aws-powertools/powertools-lambda-python/pull/3302/files#diff-bdb4b43087f89fdf398381f3d88048f6182562ca67e8907259d5c56d658465c1R2012

It isn't immediately clear this block: https://github.com/aws-powertools/powertools-lambda-python/pull/3302/files#diff-bdb4b43087f89fdf398381f3d88048f6182562ca67e8907259d5c56d658465c1R2012

tests/functional/event_handler/test_api_middlewares.py Outdated Show resolved Hide resolved
tests/functional/event_handler/test_api_middlewares.py Outdated Show resolved Hide resolved
tests/functional/event_handler/test_api_middlewares.py Outdated Show resolved Hide resolved
tests/functional/event_handler/test_api_middlewares.py Outdated Show resolved Hide resolved
tests/functional/event_handler/test_api_middlewares.py Outdated Show resolved Hide resolved
tests/functional/event_handler/test_api_middlewares.py Outdated Show resolved Hide resolved
tests/functional/event_handler/test_api_middlewares.py Outdated Show resolved Hide resolved
@heitorlessa
Copy link
Contributor

There's another test we're missing to catch a regression like this --- we don't have a test where we have both strip_prefix + router prefix.

Could you add that one in too, please?

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

tiny change in the latest test to make it easier to go back in the future.

Can be merged after that 🎉 !!!!!!

tests/functional/event_handler/test_api_gateway.py Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Nov 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
No Duplication information No Duplication information

@heitorlessa heitorlessa changed the title fix(event_handler): Router prefix mismatch when Middleware is used fix(event_handler): Router prefix mismatch regression after Middleware feat Nov 7, 2023
@heitorlessa
Copy link
Contributor

GREEEEEAAAAT JOB.

That was a pesky regression to detect

@leandrodamascena leandrodamascena merged commit 45e6555 into aws-powertools:develop Nov 7, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event_handlers 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: Extended middleware not being called after assigned to route.
3 participants