Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#3655 closed defect (fixed)

IRequestFilter pre-processor - either implementation or documentation is wrong

Reported by: simon-code@… Owned by: Christian Boos
Priority: normal Milestone: 0.10
Component: general Version: devel
Severity: normal Keywords:
Cc:
Release Notes:
API Changes:

Description

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.

Attachments (4)

irequestfilter_patch_3689.diff (1.9 KB ) - added by simon-code@… 11 years ago.
Patch for trac.web.main and trac.web.api that adds a 'prepare_request()' method to the IRequestFilter interface.
irequestfilter_patch_3689.2.diff (2.1 KB ) - added by Christian Boos 11 years ago.
Slightly modified version: trap errors raised by calls to prepare_request()
irequestfilter_patch_3689.3.diff (2.9 KB ) - added by Christian Boos 11 years ago.
Also post-process a request in case of early errors or errors while dispatching the request
irequestfilter-r3717.diff (3.9 KB ) - added by Christian Boos 11 years ago.
Updated patch, integrating feedback from cmlenz

Download all attachments as: .zip

Change History (21)

Changed 11 years ago by simon-code@…

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

comment:1 Changed 11 years ago by 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?

Changed 11 years ago by Christian Boos

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

comment:2 Changed 11 years ago by Christian Boos

Owner: changed from Jonas Borgström to 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?

comment:3 Changed 11 years ago by 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)

comment:4 Changed 11 years ago by 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.

comment:5 in reply to:  3 Changed 11 years ago by 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!

comment:6 Changed 11 years ago by 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 :-)

comment:7 Changed 11 years ago by 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.

comment:8 in reply to:  7 Changed 11 years ago by 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.

comment:9 in reply to:  7 ; Changed 11 years ago by 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.

comment:10 in reply to:  9 Changed 11 years ago by 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

Changed 11 years ago by Christian Boos

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

comment:11 Changed 11 years ago by 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.

comment:12 Changed 11 years ago by 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…

Changed 11 years ago by Christian Boos

Attachment: irequestfilter-r3717.diff added

Updated patch, integrating feedback from cmlenz

comment:13 Changed 11 years ago by Christian Boos

Keywords: review added
Milestone: 0.10
Status: newassigned

comment:14 in reply to:  13 Changed 11 years ago by 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.

comment:15 in reply to:  13 Changed 11 years ago by Christian Boos

Keywords: review removed
Resolution: fixed
Status: assignedclosed

Replying to cboos:

Please try out attachment:irequestfilter-r3717.diff

Patch applied in r3720.

comment:16 Changed 11 years ago by Christian Boos

Keywords: review added
Resolution: fixed
Status: closedreopened

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

  • main.py

     
    228228                        req.display(template, content_type or 'text/html')
    229229                    else:
    230230                        self._post_process_request(req)
     231                except RequestDone:
     232                    raise
    231233                except:
    232234                    err = sys.exc_info()
    233235                    try:

comment:17 Changed 11 years ago by Christian Boos

Keywords: review removed
Resolution: fixed
Status: reopenedclosed

Patch applied in r3755.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .

 
Note: See TracTickets for help on using tickets.