Ticket #559 (defect)

patch

example application (ATM Middleware class is here)

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?).

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).

New improved patch

New test application

small modification, use a named function instead of a lambda (looks clearer)

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.

Separated example of wsgi namespace

slight improvement to wsgins.patch

some improvements for latest fumanchu's patch

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. ;-)

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. :)

WSGI-namespace example with no subclassing

Aaaaahhhh, much better now (see the new pure_wsgins.patch). No subclassing, which means:

1. a cleaner App base class,
2. plays better with other concurrent namespaces,
3. can use an arbitrary namespace key, and
4. is a great demo of the namespacing concept.

I'm ready to check it in if there are no outstanding issues...the only other thing I might do is lowercase the name "Pipeline".

Implemented in [1310] as cherrypy.wsgi.pipeline, including an Application config namespace.

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!

Refactored a bit in [1430].