Enable Middleware Support for Blueprint Groups

[harshanarayana]

This commit will enable the users to implement a middleware at the blueprint group level whereby enforcing the middleware automatically to each of the available Blueprints that are part of the group.

This will enable a simple way in which a certain set of common features and criteria can be enforced on a Blueprint group. i.e. authentication and authorization

This commit will address the feature request raised as part of Issue #1386

[codecov-io]

Codecov Report

Merging #1399 into master will decrease coverage by 0.66%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1399      +/-   ##
==========================================
- Coverage   91.45%   90.78%   -0.67%     
==========================================
  Files          17       18       +1     
  Lines        1731     1900     +169     
  Branches      330      377      +47     
==========================================
+ Hits         1583     1725     +142     
- Misses        123      151      +28     
+ Partials       25       24       -1

Impacted Files	Coverage Δ
sanic/app.py	92.01% <100%> (+0.29%)	⬆️
sanic/blueprint_group.py	100% <100%> (ø)
sanic/reloader_helpers.py	15.57% <0%> (-0.34%)	⬇️
sanic/request.py	100% <0%> (+0.48%)	⬆️

---

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 52deeba...930ec49. Read the comment docs.

[sjsadowski]

Please update the documentation related to blueprints before we merge this.

[harshanarayana]

@sjsadowski I am working on adding the documents. I will update the PR with the changes shortly.

[vltr]

Can you please add a more comprehensive number of tests, with parameters in both the route and url_prefix? Also, add tests with other HTTP methods such as POST and OPTIONS, so everything can be checked successfully.

[harshanarayana]

@vltr Sure. Consider it done.

[harshanarayana]

@sjsadowski I added the basic documentation on usage and tested a local sphinx build to ensure the changes are working as expected.

@vltr I've added additional unit test cased with few more HTTP methods as well as url_prefix and some parameterized route

[sjsadowski]

@harshanarayana thanks. This won't go in to 18.12 but it will go in after that. Appreciate your help.

[harshanarayana]

@sjsadowski I'm OK with this getting merged into any version you see fit. No issues from my side. Just give me a heads-up if anything else needs to be addressed in this PR.

Documentation is added.

[ahopkins]

@yunstanford Are you good with this one?

[harshanarayana]

@huge-success/sanic-core-devs Can you please take a look at this and let me know if there are any other changes required to be done on this PR?

[ashleysommer]

@harshanarayana
Overall this looks pretty good.
When this gets merged, and users start to use it in their applications, tools like SanicPluginsFramework, and other plugins may no longer work as expected, because they tend to assume the only classes that can have middlewares on them are Sanic App and Blueprints. They may need to be modified to be BlueprintGroup aware, but that is inevitable fallout.

The only code issue I see is the use of group.blueprint = bp where you override the property setter to append bp to a list. That is a counter-intuitive pattern to me, as usually the python assignment operator = is used to replace the target, not append to it.

The way I've usually implemented this kind of functionality is to create a method called .append() on the class, and call .append(bp) as if the class itself is a list. This also matches the other list-like functionality on BlueprintGroup like the iteration behaviour.

[harshanarayana]

@ashleysommer Thank you very much for the review and suggestions.

tools like SanicPluginsFramework, and other plugins may no longer work as expected, because they tend to assume the only classes that can have middlewares on them are Sanic App and Blueprints

The idea was to keep almost all the existing behaviors of Blueprint.group intact so that the other plugins and frameworks don't run into any major breaking issues. I will take a look around the existing plugins and see if any of them will actually have a problem because of this change.

The way I've usually implemented this kind of functionality is to create a method called .append() on the class, and call .append(bp) as if the class itself is a list.

That sounds like a really good thing to do. Let me get that change sorted.

[ashleysommer]

The idea was to keep almost all the existing behaviors of Blueprint.group intact so that the other plugins and frameworks don't run into any major breaking issues.

Yeah, it's a good implementation. I didn't mean to suggest it might cause breaking changes. I don't think that. I just mean that some plugins will need to be BlueprintGroup aware, specifically if it is a middleware-based plugin and it wants to apply its plugin/middleware functionality to blueprint groups.

[harshanarayana]

I didn't mean to suggest it might cause breaking changes

Sorry if my message came across that way. I wasn't meaning that. After you mentioned, I realized that there might be some breaking changes wrt these plugins if they are doing an isinstance like check for something.

[ashleysommer]

@sjsadowski
Why was this closed?
I thought it should be merged.

[sjsadowski]

Ugh. It has to do with how projects work. I closed all the PRs that looked like they were merged from the project screen. Gotta go fix now...

Stale review, looks like we should be good to go.

[sjsadowski]

I'm good with this, even though it could break some projects (known)

[ahopkins]

I agree. I think this is a positive breaking change.