IRequestFilter pre-processor - either implementation or documentation is wrong

[simon-code@…]

Documentation for pre-processor in trac.web.api.IRequestFilter says …typically adding values to req.hdf…

But, looking at trac.web.main init and dispatching code, the HDF object isn't initialised when the pre-processor is called. That makes it impossible to manipulate req.hdf - see trunk/trac/web/main.py@3602#L175​

I need to set values to the HDF that is always available for my various custom templates, styles and plugins. Using the post processor works fine, except when there is any kind of error - and the req processing is halted immediately…

I made an early request for this interface in #2724, and having used it now I still think it makes sense to have 2 pre-processors - one as the current (before most init) that can influence handler choice and even allow for 'application embedding', and one that is used for setting/adjusting new and Trac generated information for all handlers before sending to processing.

However, sticking with one pre-processor, I think that the intent of the documentation is correct and most useful for now - and that the filters call should be moved down to line ±214.

[simon-code@…]

Patch for trac.web.main and trac.web.api that adds a 'prepare_request()' method to the IRequestFilter interface.

[simon-code@…]

The attached patch solves the issue by adding another method to the IRequestFilter interface.

I tried numerous variations on getting something useful to work, and finding a balance in the code on when to call the filter and what status the request should have then. Conclusion: There is no practical way of combining handler choice with request HDF modifications, and 2 methods are required.

The attached patch keeps the current logic (and naming) as-is, and should not break any Trac code or plugins - but plugins that implement the interface (if they exist yet) need to add the method and just return the req object. A simple solution to this would be to make the API include the default return statement(s) - not done in the attached patch for 3689.

Including default return statements in the API definition also makes it easier to add to the API (interfaces) without breaking any existing code - at least when there is such an obvious default return for the method. I highly recommended it, but I'll leave it to core developers to decide on that.

Any chance of including the patch for 0.10? Please?

[Christian Boos]

Slightly modified version: trap errors raised by calls to prepare_request()

[Christian Boos]

The proposed interface change looks useful, but I think TracError and PermissionError exceptions raised by the prepare_request methods should be trapped. Can you try attachment:irequestfilter_patch_3689.2.diff​ ?

Also, I don't exactly understand what you mean by Including default return statements in the API definition, can you provide an example of what you'd like to have?

[simon-code@…]

About 'default return statements', I mean simply add to the interface definition - so that every method does not need to be implemented in a plugin. Example - removed documentation just for clarity:

class IRequestFilter(Interface):

    def pre_process_request(req, handler):
        return handler
    
    def prepare_request(req, handler):
        return req

    def post_process_request(req, template, content_type):
        return (template, content_type)

[anonymous]

Looked at revision 2 of the patch, and for my purposes at least the call in trac-web.main cannot be put that late…

I needed something for 'prepare' that I know will be available to all templates. The second version is too late, as for instance a 404 error will return a template before values are prepared.

That means that my custom template information, javascript navigation and styles are not added - and the error page can not look like all other pages.

[Christian Boos]

Replying to simon-code@bvnetwork.no:

About 'default return statements', I mean simply add to the interface definition - so that every method does not need to be implemented in a plugin.

Ah right, I should have understood, as I often wanted that myself. I experimented a bit, coming up with this:

Index: trac/core.py
===================================================================
--- trac/core.py        (revision 3686)
+++ trac/core.py        (working copy)
@@ -127,6 +127,10 @@
            'implements() can only be used once in a class definition'

     locals['_implements'] = interfaces
+    for intf in interfaces:
+        for meth in dir(intf):
+            if not meth.startswith('_'):
+                locals[meth] = lambda self,*args: getattr(intf(), meth)(*args)


 class Component(object):

With the above patch, the Interface methods would need to be proper methods, i.e. have a self argument.

That works fine for simple test cases on the command line, though Trac won't behave properly with the above patch, probably because some Interface methods previously undefined are now being called and they don't have the self argument.

… back to the topic: ok, for the use case you described, version 1 of the patch is better. Now the question remains as whether to schedule the patch for 0.10 or not. Opinion of other developers welcomed!

[simon-code@…]

Tried your interface patch, but that does not work as I figured. If I remove a method from an interface implementation (only implement 2 of the 3 IRequestFilter methods), a traceback is gently produced…

...
    locals[meth] = lambda self,*args: getattr(intf(), meth)(*args)
TypeError: prepare_request() takes exactly 2 arguments (3 given)

Strange thing is that it is pre_process_request() that I removed… Adding it backs means all works fine.

Looking at you code (glancing actually, as any lambda means my understanding approaches zero), I wonder if the logic would be to add the method from the base Interface class to the instance if it doesn't exit in the instance? For all I know that is what your code tries to do… :-)

However, I was merely thinking 'inheritance' when I mentioned it - and forgot to consider how the component archtecture is actually implemented… I suppose such a discussion is above and beyond this ticket.

About where to call prepare_request(): As an error page effectively is handling a request, then hopefully you will use my location (last chance before any processing and template rendering is about to start).

A case for 0.10 inclusion: The IRequestHandler was added in trunk, and has never been part of a release. And, if anyone had tried using pre_process_request() for the purposes it states, there likely would have been a ticket sooner. The change is low (no) impact, and makes the interface usable as it was intended. Please include :-)

[Christopher Lenz]

About providing default implementations of interface methods: interfaces are (intentionally) orthogonal to inheritance… if interfaces exist where default implementations make sense, we should provide abstract base classes in addition to the interface, such as WikiMacroBase. For IRequestFilter it might make sense too, especially because you can't just use pass to implement the methods.

About the actual issue raised in this ticket: I think that for 0.10 we should just fix the docstring. We'll be migrating from ClearSilver to ​Markup after 0.10 anyway, and will provide an extension point that is explicitly designed for adding data to the template context (not implemented on the branch yet). IRequestFilter should IMHO retain its current purity of having symmetrical pre-/post-processing hooks.

[Matthew Good]

Replying to cmlenz:

About providing default implementations of interface methods: interfaces are (intentionally) orthogonal to inheritance… if interfaces exist where default implementations make sense, we should provide abstract base classes in addition to the interface, such as WikiMacroBase. For IRequestFilter it might make sense too, especially because you can't just use pass to implement the methods.

Bah, I got a midair collision trying to post basically the same thing.

[simon-code@…]

Replying to cmlenz:

I think that for 0.10 we should just fix the docstring. We'll be migrating from ClearSilver to ​Markup after 0.10 anyway, and will provide an extension point that is explicitly designed for adding data to the template context (not implemented on the branch yet). IRequestFilter should IMHO retain its current purity of having symmetrical pre-/post-processing hooks.

Oki… Seems like I have to revert to using INavigationProvider.get_navigation_items() without yielding items as my opportunity to modify req.hdf and add my styles and javascripts… It works just like suggested prepare_request, but feels like a hack :-)

Clearly I am in no position to evaluate long-term plans or purity of interface implementations. I just feel that the interface that will be introduced with 0.10 is incomplete. My 3 step filter is better, but maybe not ideal either for the common needs of adding stuff to hdf, and adding styles and scripts that are to be available for all 'visuals' regardless of handler.

For a styles (themes) plugin especially, it is kind of a catch-22:

1. I tried using the post-processor, but as the error handler does not pipe through the post-processor on its way to rendering, I had to move forward in the call chain.
2. Moving to a pre-processor (like prepare_request()) means that my stylesheet is added first, and instead of me overriding styles in handlers, the handlers would revert (part of) my themes. I find that I need to add styles twice (pre AND post) to make sure they also override anything introduced in handlers. As there is no method to remove items from a HDF dataset..

Maybe a more sensible approach is to make sure the error handling call the post-processor as well? For anything visual, the post-processor is the most correct place - if we can guarantee that all template-based information actually call it, including 'normal' error messages.

[Christian Boos]

Replying to simon-code@bvnetwork.no:

Replying to cmlenz:

… IRequestFilter should IMHO retain its current purity of having symmetrical pre-/post-processing hooks.

…

1. I tried using the post-processor, but as the error handler does not pipe through the post-processor on its way to rendering, I had to move forward in the call chain.

Maybe we could address the above point, and:

• be sure to keep the original error around after the post-processing is done
• in case of an error during the post-processing, only show that error if there's not already a previous error to show

[Christian Boos]

Also post-process a request in case of early errors or errors while dispatching the request

[simon-code@…]

The third patch seems to work fine. Have tried early error like 404 + 'normal' errors in use, and in all cirtumstances my single post_process_request() adds the required information to visual processing - req.hdf entries, styles and javascript.

[simon-code@…]

Have been thinking more about this, and would it be useful to trap all the 'Error subsystem' in a more general way? So that the 'symmetry' of the filter looked like this:

def pre_process_request()
   """ Like today """

def process_request_error(error, req, handler, template, content_type)
   """ Called for all errors 'on the way out'. Use to add or change req.hdf,
   call post_process_request, or redirect to other handler, resource or
   relevant information. """
   return (req, handler, template, content_type)

def post_process_request()
   """ Like today """

An exception inside a process_request_error(), would just mean that the original error was passed to rendering and display (default error handling).

It should work the same regardless of whether it is early error (like handler being None), or a Permission or Trac error occuring inside a handler.

Just thinking…

[Christian Boos]

Updated patch, integrating feedback from cmlenz

[Christian Boos]

Plese try out attachment:irequestfilter-r3717.diff​

[simon-code@…]

Replying to cboos:

Plese try out attachment:irequestfilter-r3717.diff​

Works as expected - a 404 (no handler) error runs through the post-processor like other errors.

[Christian Boos]

Replying to cboos:

Please try out attachment:irequestfilter-r3717.diff​

Patch applied in r3720.

[Christian Boos]

Just an afterthought, should _post_process_request(req) get to see the finished requests too, or should we rather do:

main.py

@@ -228 +228 @@
                         req.display(template, content_type or 'text/html')
                     else:
                         self._post_process_request(req)
+                except RequestDone:
+                    raise
                 except:
                     err = sys.exc_info()
                     try:

[Christian Boos]

Patch applied in r3755.