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

Middleware, second edition #209

Merged
merged 7 commits into from
Dec 28, 2014
Merged

Middleware, second edition #209

merged 7 commits into from
Dec 28, 2014

Conversation

asvetlov
Copy link
Member

Application accepts middlewares keyword-only parameter, which should be sequence of middleware factories.

Every factory is a coroutine that accepts two parameters: app (Application instance) and handler (next handler in middleware chain. The last handler is web-handler selected by routing itself. Middleware should return new coroutine by wrapping handler parameter. Signature of returned handler should be the same as for web-handler: accept single request parameter, return response or raise exception. Factory is coroutine, thus it can do extra yield from calls on making new handler.

After constructing outermost handler by applying middleware chain to web-handler in reversed order RequestHandler executes that outermost handler as regular web-handler.

N.B. Middlewares are executed after routing, so it cannot process route exceptions.

Thoughts?

@asvetlov
Copy link
Member Author

BTW, middleware may change request passed to inner handler.

We have no API for that right now but it can be done easy by adding Request.clone(**kwargs) where kwargs key may be any request property.

@ludovic-gasc
Copy link
Contributor

Looks good to me.
Yocto detail: you have in typo in tests with middleware name.

Maybe, should be useful to define also middlewares for a specific route , to override application middlewares.
For example, a csrf middleware shouldn't be everywhere if you have also an API (it's a concrete issue I had with Django)

@asvetlov
Copy link
Member Author

I think adding enabling/disabling middlewares for routes makes a mess.

If you need to wrap some specific handlers -- do it explicitly, e.g.

app.router.add_route('POST', '/post', process_csrf(post_handler))

Can the way satisfy your requirements?

@ludovic-gasc
Copy link
Contributor

Yes, indeed.

@fafhrd91
Copy link
Member

lgtm.

i think we need Application.register_middleware() as well.

asvetlov added a commit that referenced this pull request Dec 28, 2014
@asvetlov asvetlov merged commit bd4b48a into master Dec 28, 2014
@asvetlov asvetlov deleted the middleware branch December 28, 2014 08:22
@asvetlov
Copy link
Member Author

Adding middleware to application should be done at configuration stage, current api allows to do that.
After making handler nobody should add middlewares or routes -- that's weird.

Also I doubt that we need a way to unregister middleware.

I guess to keep API as-is and add register_middleware method only after real use case.

@ludovic-gasc
Copy link
Contributor

+1 for @asvetlov please to keep it simple :-)

@lock
Copy link

lock bot commented Oct 30, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants