Issue #559 resolved

Allow configuration of WSGI middleware aspects

michele
created an issue

This patch implements what I've described here [1] some times ago, it turns out to be pretty simple thanks to CP3 design.

It allows the configuration of various middleware aspects by introducing a new wsgi namespace inside cherrypy.Application.

That patch is just an initial attempt, it also introduces a Middleware class (not finished) that resembles the way you work with Tool and allows you to register various middleware to be used and configured.

For example:

{{{ app_conf = { '/': { 'wsgi.middleware_pipeline': ['changecase'], 'wsgi.changecase.to': 'lower', } }

register our middleware like you do with a tool

cherrypy.wsgi.changecase = Middleware(ChangeCase)

cherrypy.config.update(global_conf) cherrypy.quickstart(Root(), script_name='/', conf=app_conf) }}}

Root() will be wrapped inside the ChangeCase middleware that will be instantiated using the to parameter.

Ideally Middleware should be tweaked to set its instance attributes to middleware arg (like Tool) and allow for easy introspection of available options, the Middleware class is simple so it will not work for more esoteric middleware that for example are not expecting app as a first parameter, but I think that we should keep it simple for simple well done middleware, if you need to register an strange middleware you can subclass it or use a different solution, the only requirement is that you should register a callable that accepts the middleware config dict and returns a closure for calling that middleware to wrap your application.

Other issues are related to the wrapping mechanism, ATM it's being done inside mount but Application is managing the wsgi namespace, we should or move it inside Application of to Tree, I think that Tree is where it really belongs since the Application itself shouldn't be aware of any middleware wrapping it.

Finally I think WSGIBox should be moved to _cpwsgi.py and made more useful probably, I've cooked it up very quickly.

Opinions?

[1] http://tinyurl.com/kgww7

Comments (10)

  1. Robert Brewer

    This is a great start. I think you've correctly identified the "ugly bits" in your own comments. ;) The only thing I'd push for still is that these changes happen without needing to alter the Application base class: there should be a way to decorate it (when requested by the developer), rather than alter it fundamentally. Now, if that's not possible because of the design of especially tree.mount, then we should take this opportunity to fix tree.mount to support a decoration approach (maybe just an isinstance(root, Application) check is enough?).

  2. michele reporter

    Ok, as discussed yesterday on IRC I've moved all the ugly bits inside Application itself, I think it looks very clean now and the changes are pretty minimal.

    Note that I've added a _setup_wsgiapp method so that we don't need to re-wrap the application with middlewares every time app.__call__ is invoked, this makes sense since middleware shouldn't be dynamically applied but only once at initialization time, this also avoids any performance hit I guess.

    I also moved WSGIMiddleware and WSGIMiddlewareCollection inside _cpwsgi.py, you can look at test.py for an example application that makes use of them.

    WSGIMiddleware (Middleware previously) has also been simplified in that its __call__ method shouldn't return a closure wrapping the middleware and that accepts an app argument but just receives the app to wrap plus keyword arguments for configuring the middleware it manages and return the wrapped app, I removed the closure since I don't need it anymore and it looks simpler.

    The only outstanding issues I can see:

    Making WSGIMiddleware easily introspect-able like Tool, this means that for a middleware like this:

    class ChangeCase(object):
        def __init__(self, app, option_1, option_2="hello"):
            pass
    

    we should be able to do something like this:

    >>> changecase = WSGIMiddleware(ChangeCase)
    >>> changecase.self
    ...
    AttributeError
    >>> changecase.app
    ...
    AttributeError
    >>> changecase.option_1
    None
    >>> changecase.option_2
    "hello"
    

    as I said I don't think we need something more fancy (but I may be wrong), if you need to wrap other middleware that are making funny things you can just use a custom class, a subclass or a callable, what matters is that it should just follow the same way of working:

    1) called with app as first positional argument, and with the middleware config as keyword arguments

    2) return the app wrapped inside the middleware as configured

    An other thing that you may want to look at is the way we manage the configuration namespace, for example is wsgi.* good? ATM we have a standalone setting wsgi.middleware_pipeline and then in the same namespace we have wsgi.[middleware_name].* this doesn't look so good probably but using a subnamespace makes things more difficult.

    Also when you declare the middleware_pipeline you don't need to prefix stuff inside the list with the wsgi namespace, does this look right?

    Another possible solution is this:

    wsgi.evalexception.on = True
    wsgi.changecase.on = True
    wsgi.changecase.option1 = value
    

    but without declaring the pipeline explicitly this makes things not so intuitive IMHO and you need a way to order the pipeline (an order parameter?), the list seems the best way to go.

    '''The best solution''' IMHO is this one:

    wsgi.middleware_pipeline = [wsgi.middlewares.evalexception, wsgi.middlewares.changecase]
    wsgi.middlewares.changecase.option = 40
    

    Here we keep the wsgi.* namespace uncluttered by moving the middleware collection to a middlewares namespace (just like tools), this leaves the door open for future builtin wsgi.* settings that otherwise will be impossible since registered middleware could cause name conflicts, also note that you need to declare the whole namespace inside the list, it's a little more verbose but it makes more sense, coherent and will avoid any confusion IMHO.

    To register a middleware you will need to do something like this:

    wsgi.middlewares.mymiddleware = WSGIMiddleware(MyMiddleware)
    

    '''I strongly vote for this solution personally.'''

    As you can see the changes left to do are pretty minimal and mostly stylistic.

    I hope you can complete this thing as I think is a simple but very useful feature to have in CP3.

    Personally I'm not sure I can hack on this anymore (even if I really would like to) and within a short time since I need to start preparing an exam (I should be doing it already :D).

  3. Robert Brewer

    I've attached an example of how this might work in a more separated manner (so that CP users who don't use WSGI don't have all the extra cruft in their App objects). I'd like this namespace to be in the standard distro, but be the "poster child" for adding your own namespace to an App without having to patch the _cptree module in any way.

  4. michele reporter

    I've attached a new patch based on your latest one.

    Improvements:

    - it seems as you were always overriding script_name inside Pipeline when invoking the base class, I've fixed this and used super also

    - instead of applying the same middlewares at every invocation of the application we can just do it once for all after the config stuffs have been merged, so I override merge instead of call and after the base class one has done its stuffs we can safely setup self.wsgiapp that will later on be used by __call__ when responding (so there is no need to override __call__)

    Here's the beautiful code:

        def merge(self, conf):
            """Merge the given config into self.config."""
            super(Pipeline, self).merge(conf)
            if self._head is None:
                pipe = self.pipeline[:]
                pipe.reverse()
                for name, callable in pipe:
                    conf = self.pipeconfig.get(name, {})
                    self.wsgiapp = callable(self.wsgiapp, **conf)
            else:
                self.wsgiapp = self._head
    

    Just wondering, what's the use case for self._head?

    Anyway, I think that thing is now ready!

    Thanks fumanchu, remember to rename app.conf to app.config while you're at it. ;-)

  5. Robert Brewer
    it seems as you were always overriding script_name
    inside Pipeline when invoking the base class
    

    Thanks! I'll make that change.

    instead of applying the same middlewares at every
    invocation of the application we can just do it once
    for all after the config stuffs have been merged
    ...
    Just wondering, what's the use case for self._head?
    

    Instead of applying the same middlewares at every invocation of the application, self._head allows us to just do it once for all. ;) Your solution overwrites self.wsgiapp; using _head avoids that, so if you want to modify the pipeline at some point (even from a page handler!) you can set _head = None and the next call will be able to compute the new pipeline and stick *that* in _head.

    remember to rename app.conf to app.config while you're at it
    

    Will do. :)

  6. michele reporter

    Great work, looks wonderful fumanchu.

    The bits that allows one to add a not removable (from config) set of middleware using the code directly just rocks. ;-)

    Note that you checked in test_wsgi_ns.py with DOS line endings, running a dos2unix on the whole CP3 codebase it seems as it is the only offender. ;-)

    Regarding head, damn I hadn't noticed that you were using it in that way (it was 2AM here ;-)), one thing you may want to change is the direct check of head being None:

    >>> head = None
    >>> if not head:
    ...     print "hello"
    ...
    hello
    >>> head = False
    >>> if not head:
    ...     print "hello"
    ...
    hello
    >>>
    

    but I'm sure I'm missing something again!

    Thanks great work!

  7. Log in to comment