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

Blueprint middleware applied globally #37

Closed
narzeja opened this issue Oct 16, 2016 · 23 comments · Fixed by #1690
Closed

Blueprint middleware applied globally #37

narzeja opened this issue Oct 16, 2016 · 23 comments · Fixed by #1690

Comments

@narzeja
Copy link

narzeja commented Oct 16, 2016

I was a little too hasty updating the blueprints documentation for middleware/exceptions, and just realized that middlewares and exceptions registered through a blueprint decorator are applied to all routes.

Is this intended behaviour? If so, then the the blueprint documentation must be updated once more.

Alternative behaviour: middleware registered on a blueprint, are only applied to routes for that blueprint.
Some considerations:

  • should middleware applied to the app-object (instance of Sanic) also be applied to blueprint middleware?
  • if so, how will ordering be handled?
channelcat added a commit that referenced this issue Oct 16, 2016
@channelcat
Copy link
Contributor

Ah, I missed that in the pull request. I've updated the documentation for now. My thoughts on this are:

Applied locally:

  • Is more self-contained, prevents conflicting code.
  • Applying global middleware and exception handling requires registering them separately from the blueprint.

Applied globally:

  • Modularizes middleware and exception handling.
  • Blueprints can facilitate whole modules, allowing a single interface for sharing code between projects.
  • Can use decorators to achieve local middleware and exception handling:
def authenticated(func):
    def handler(request, *args, **kwargs):
        # Middleware goes here
        try:
            return func(request, *args, **kwargs)
        except MyCustomException:
            return text("son, i am dissapoint")
    return handler

@bp.route('/')
@authenticated
def index(request):
    return text("We did it fam")

...

In both scenarios you can achieve the same thing, but I think the latter offers more.

As for ordering, I'm thinking they should be applied in the order the blueprint was registered, and in the order they were registered in the blueprint. I think it would allow blueprints to require other blueprints in a way that's easy to discern in the code.

@narzeja
Copy link
Author

narzeja commented Oct 19, 2016

My concern with using a decorator for local middleware/exceptions, is that it requires a different way of adding them, compared to registering on the application object. My fear is, that it becomes too complicated or too involved to handle middleware/exceptions on blueprints.

I would prefer, that you register middleware and exceptions in the same manner (@<obj>.middleware), regardless of applying it on the app-object (affecting all routes) or the blueprint (affecting only those blueprint routes).

@eth3lbert
Copy link

What is the different between using @app.middleware and @bp.middleware? Are there any benefits to use @bp.middleware?

@Archelyst
Copy link
Contributor

To add my two cents: While it might be beneficial to apply middleware registered on a blueprint globally, that's definitely not what I'd expect to happen. I'd regard blueprints as pretty self-contained and separate units and find it kinda surprising to see them interact in such a way. What about adding a @bp.global_middleware?

@yoloseem
Copy link
Contributor

yoloseem commented Dec 26, 2016

It's defnly unexpected behavior. Well, for me with a long experience with Flask that has same concept the 'Blueprint', it surprised me a lot! I think that the blueprint is a modular sub-application. Unless it stands just for deleting all url prefixes in code, an application with blueprints would be like an 'united' application, not 'single' one. I can understand complexity fears but I believe that it can offer much more capability and convenience with blueprint's independence.

@eth3lbert) What is the different between using @app.middleware and @bp.middleware? Are there any benefits to use @bp.middleware?

@eth3lbert : You can apply various caching policy, control access to specific spaces, or inject something into your response only when it was processed in some bp, based on your blueprint separation.

@channelcat) As for ordering, I'm thinking they should be applied in the order the blueprint was registered, and in the order they were registered in the blueprint.

👍

@youknowone
Copy link
Contributor

youknowone commented Dec 28, 2016

I also think this is a counterintuitive behavior. Especially when you introduce this project as flask-like, I expected the blueprint-related features are totally separated and it doesn't affect the app out of blueprint.

I agree about the concolusion the global application offers more, still I also more agree to the other parts its name must not be Blueprint.middleware. Strongly voting to @Tim-Erwin's idea, you don't need to select only one idea of middleware behavior on the counterintuitive name. Please consider to let Blueprint has its own scoped middleware and allow them also to have modulized global middlewares like Blueprint.app_middleware or Blueprint.global_middleware.

@seemethere
Copy link
Member

Is this still an issue? Will reopen if necessary.

@seemethere
Copy link
Member

Reopening per request from @r0fls

@seemethere seemethere reopened this Feb 12, 2017
@gwthm-in
Copy link

Is it still there? any update on this?

@r0fls
Copy link
Contributor

r0fls commented Mar 25, 2017

still there

@stopspazzing
Copy link
Contributor

Does this still apply because of #715 being fixed?

@ak4nv
Copy link
Contributor

ak4nv commented Jun 15, 2017

Example from life: I have an application with admin control. In admin blueprint I have to use decorator for check admin rights for all views instead of one blueprint middleware handler.
And do not forget about this please

Explicit is better than implicit.

@rgacote
Copy link

rgacote commented Dec 20, 2017

Another real-world example.
Two URL patterns:

   /a/b/<cred1>/<cred2>
   /a/b?cred1=abc&cred2=xyz

Planned to have two middlewares that gathered the necessary parameters (one from the URL, another from arguments) and store them in the request object so the next layer of code would not need to worry about where the creds 'came from.'
(There's actually more than two, and some are significantly different from the examples.)

@FelixZY
Copy link

FelixZY commented Jul 19, 2018

Personally I would expect @app.middleware to apply globally and @bp.middleware to apply locally.

Another real life example (which led me here):

public = Blueprint("public", url_prefix="/public")
public.static("/", public_web_dir)

secure = Blueprint("secure", url_prefix="/secure")
secure.static("/", secure_web_dir)


@secure.middleware("request")
@authenticated
async def request_middleware(request):
    pass


app = Sanic()
app.blueprint(public)
app.blueprint(secure)
app.run(host="0.0.0.0", port=8080, debug=True)

In this case I expect to be able to access the static files served via public and be stopped from accessing the static files served via secure without authorization.

Due to the nature of bp.static I cannot add my decorator to the requests it manages, which makes using .middleware for all requests going to secure the logical solution. However, with the current globalness of .middleware I will need to come up with a more complicated solution.


While I definitely agree with @Tim-Erwin that it would be more logical to have a .global_middleware for the current behavior of .middleware for blueprints, maybe the introduction of .local_middleware could be an option to maintain compatibility?

@FelixZY
Copy link

FelixZY commented Jul 24, 2018

I have a workaround (although I still think this should be implemented in sanic). Using my last example:

public = Blueprint("public", url_prefix="/public")
public.static("/", public_web_dir)

secure = Blueprint("secure", url_prefix="/secure")
secure.static("/", secure_web_dir)


@secure.middleware("request")
def global_middleware(request):
    if request.path.startswith(secure.url_prefix):

        @authenticated
        def local_middleware(request):
            pass
        local_middleware(request)


app = Sanic()
app.blueprint(public)
app.blueprint(secure)
app.run(host="0.0.0.0", port=8080, debug=True)

@disarticulate
Copy link

I just wanted to agree that my expectation for a blueprint middleware is that it applies only to routes included in the blueprint.

I don't see why it would apply globally. My sample handler setup which seems to do what I would like:

from sanic import response
from sanic import Blueprint
from tempfile import TemporaryFile

blueprint = Blueprint('docx_server', url_prefix='/docx')

@blueprint.middleware('request')
async def type_conversion_dates(request):
  request.args['date'] = pendulum.now()

@blueprint.middleware('response')
async def set_model_response(request, response):
  response.content_type ='application/vnd.openxmlformats-officedocument.wordprocessingml.document'
  return response

@blueprint.route("/<template>", methods=['POST'], strict_slashes=False)
async def get_template_docx(request, template):
  """Returns X3D document
    context (json):
      employee_name (str): name
      employee_title (str): title
      employee_education (str[]): education
      employee_certification (str[]): certifications
      selected_experience (str): experience description
      extra_experience (ob[{title, description}]):
        title: of description
        description: experience description
      employee_organizations(str[]): organizations

    Args:
      template (str): render template name

    Returns:
      docx
  """
  async with request.app.db_pool.acquire() as con:
    template_name = request.args.get('template')
    doc = DocxTemplate(f'/app/templates/{template_name}')
    context = request.json
    doc.render(context)
    tf = TemporaryFile()
    doc.save(tf)

    dt_string = dt.format('YYYY-MM-DD')
    r.headers['Content-Disposition'] = f'filename="{dt_string}-{context.employee_name}-resume.docx"'
    return await response.file(tf)

This seems self contained and easy to manage. Except, now I have to add the middleware somewhere else, disconnecting it from an endpoint I expect to only take certain requests and only serve certain responses.

@matoous
Copy link

matoous commented Sep 7, 2019

Hi! I see there's no update for pass 3 years. Is there any plan to fix or somehow differently resolve this issue? The current behavior seems rather counterintuitive.

@Tronic
Copy link
Member

Tronic commented Sep 7, 2019

I believe that this should be fixed (make them local). However, currently routing is performed after request middlewares, so it would be a rather big and potentially breaking architectural change. In particular, one would need to consider whether existing middlewares rely on the current behaviour, e.g. to change request url or method prior to routing.

@matoous
Copy link

matoous commented Sep 7, 2019

I believe in that in general (among other languages) middleware should be applied after the routing and is what many people expect the middleware to do. You are right with that it is breaking change so I am not sure what the optimal solution would be. Maybe as @FelixZY suggested add local_middleware? But if this should be implemented, it would probably make more sense to make middleware function local by default.

@drock1
Copy link

drock1 commented Sep 25, 2019

I just wanted to pop into this thread to point out that the current documentation for blueprint group middleware implies (incorrectly) that blueprint group middleware only executes on the routes defined in that blueprint group (and not globally).

Using this middleware will ensure that you can apply a common middleware to all the blueprints that form the current blueprint group under consideration.

The example code also makes it appear that middleware applied to a blueprint only impacts routes under that blueprint. Specifically this code from the example:

@bp1.middleware('request')
async def bp1_only_middleware(request):
    print('applied on Blueprint : bp1 Only')

@github-sjacobs
Copy link

I'm wondering where this issue stands. I am using 19.6.0 (but tried 19.6.3) and I am seeing my blueprint middleware being applied globally. I tried wrapping it up and a blueprint group as well but the behavior is the same.

My understanding of the latest documentation is that middleware added to a blueprint will be global unless the blueprint is added to a blueprint group. In this case it should apply only to routes in that group.

While this distinction is a bit confusing, I can live with it but it doesn't seem to be working that way.

Any help would be appreciated.

@harshanarayana
Copy link
Contributor

Let me take a look. If it's an easy enough fix without major refactoring, let me see if I can open a quick PR to address this.

@harshanarayana
Copy link
Contributor

harshanarayana commented Oct 8, 2019

@huge-success/sanic-core-devs This is getting a bit interesting. I was able to get the blueprint based middleware to work the right way without changing much, But here is a curious case,

def test_bp_middleware(app):
    blueprint = Blueprint("test_middleware")

    @blueprint.middleware("response")
    async def process_response(request, response):
        return text("OK")

    @app.route("/")
    async def handler(request):
        return text("FAIL")

    app.blueprint(blueprint)

    request, response = app.test_client.get("/")

    assert response.status == 200
    assert response.text == "OK"

This is from one of the existing test cases. Now, if we make sure that the blueprint middleware gets applied only on the route registered with the middleware, then this expected output is invalid. Since the route was created using @app and not @blueprint.

What would be the best behavior in this case?

  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

With the above in mind, what is the expected precedence in which this can be applied ?

harshanarayana added a commit to harshanarayana/sanic that referenced this issue Oct 8, 2019
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 <harsha2k4@gmail.com>
harshanarayana added a commit to harshanarayana/sanic that referenced this issue Oct 26, 2019
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 <harsha2k4@gmail.com>
hyunjun added a commit to hyunjun/bookmarks that referenced this issue Dec 14, 2019
Sanic - a Flask-like Python 3.5+ web server that's written to go fast https://github.com/channelcat/sanic
한동안 top 10 contributor였던 개발자 Jeong YunWon http://youknowone.github.io님의 경고
  flask와 비슷하다 = 사기. 이제 그런 소개도 빼버린것 같은데, 개발자들은 호환성 맞출 생각 없음. url path도, blueprint도, 사실은 아무것도 안맞으므로, flask와 비슷해 보여서 쓰기로 결심했다면 사용하면 안됨. decorator 씌워서 route하는거 딱 하나만 비슷
  전반적으로 소프트웨어 디자인 자체에 대해 별 생각이 없음. 참고: sanic-org/sanic#37
  품질 관리보다 벤치마크에 집착. 벤치마크에 큰 향상이 있으면 쉽게 릴리즈. 심각한 소프트웨어 버그는 때로는 몇달씩 릴리즈되지 않을 수 있음. http 헤더 fragment를 잘못 파싱해 헤더가 누락되는 버그는 master에 머지된 후 릴리즈되기 까지 6개월
  프로파일링 기반으로 성능을 측정함에도 불구하고 합리적인 reasoning 없이 성능미신에 의해 최적화. 이 코드는 현재 사라졌으니 이 정도만 코멘트.
  위와 같은 문제와 여러 변주 이후로 토론에 지친 "정상적인 설계"를 원하는 초기 기여자들은 대부분 떠남. Contributors에 들어가서 기여자들이 얼마나 빨리 떠나는지 보고, 그들이 어떤 이슈에 참여했나 보면 어떤 일이 일어나는지 알 수 있음

Python과 Windows Program간의 데이터 공유 https://blog.naver.com/educoding/221737503305 memory mapped file, MFC <-> python
sjsadowski pushed a commit that referenced this issue Dec 20, 2019
* GIT-37: fix blueprint middleware application

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

Signed-off-by: Harsha Narayana <harsha2k4@gmail.com>

* GIT-37: add changelog

Signed-off-by: Harsha Narayana <harsha2k4@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.