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

GIT-37: fix blueprint middleware application #1690

Conversation

harshanarayana
Copy link
Contributor

  1. If you register a middleware via @blueprint.middleware then it will apply only to the routes defined by the blueprint.
  2. If you register a middleware via @blueprint_group.middleware then it will apply to all blueprint based routes that are part of the group.
  3. If you define a middleware via @app.middleware then it will be applied on all available routes

Fixes #37

@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #1690 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1690      +/-   ##
==========================================
- Coverage   92.09%   92.09%   -0.01%     
==========================================
  Files          22       22              
  Lines        2215     2238      +23     
  Branches      411      419       +8     
==========================================
+ Hits         2040     2061      +21     
  Misses        137      137              
- Partials       38       40       +2
Impacted Files Coverage Δ
sanic/app.py 92.17% <100%> (-0.1%) ⬇️
sanic/router.py 95.49% <100%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e506c89...008e9e9. Read the comment docs.

@harshanarayana
Copy link
Contributor Author

@huge-success/sanic-core-devs Can you please take a look at this PR and see if this looks good? We can close a pretty old issue with this one.

@sjsadowski
Copy link
Contributor

@harshanarayana do we need to add test to prevent reversions for this behavior?

@harshanarayana
Copy link
Contributor Author

@sjsadowski

do we need to add tests to prevent reversions for this behavior?

Existing tests have been modified to assert new behavior. So any change in this behavior again will cause those tests to fail. Do you happen to have any specific scenario in mind that I may have missed ?

1. If you register a middleware via `@blueprint.middleware` then it will apply only to the routes defined by the blueprint.
2. If you register a middleware via `@blueprint_group.middleware` then it will apply to all blueprint based routes that are part of the group.
3. If you define a middleware via `@app.middleware` then it will be applied on all available routes

Fixes sanic-org#37

Signed-off-by: Harsha Narayana <[email protected]>
Signed-off-by: Harsha Narayana <[email protected]>
@harshanarayana harshanarayana force-pushed the fix/GIT-37-Blueprint-Middleware-Application branch from 9122019 to 008e9e9 Compare October 26, 2019 06:04
@harshanarayana harshanarayana changed the title WIP: GIT-37: fix blueprint middleware application GIT-37: fix blueprint middleware application Nov 2, 2019
@harshanarayana
Copy link
Contributor Author

@huge-success/sanic-core-devs Can we get this one into the next release ? This is sort of a breaking change but in the right direction

@harshanarayana
Copy link
Contributor Author

cc @huge-success/sanic-core-devs

@sjsadowski
Copy link
Contributor

I've approved it. I'm going to add a note on sanicframework.org and figure out how to call it out in the generated release notes before merging.

@harshanarayana
Copy link
Contributor Author

figure out how to call it out in the generated release notes before merging.

@sjsadowski This can be done easily. I've added a command make release that will generate the changelog based on the added changelogs snippet.

@sjsadowski
Copy link
Contributor

@harshanarayana awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blueprint middleware applied globally
2 participants