Unusual use of trac.mimeview.Context

[pipern]

Thanks for writing this plugin! I've been reading through the code to see if we can build on it.

I've found the use of ​`trac.mimeview.Context` as a way of passing around the Request and data-dictionary a bit confusing - is there any interest in a patch which switches to a more traditional (IMHO, which may be wrong) Trac-like way of passing the req (and data) object rather than a Context instance? As the Context (when made with from_request does keep reference to the Request, maybe it's fine to avoid passing ALSO the Request around separately, but it doesn't feel very common - the Context (from the docstrings) is more about rendering. Grepping for context.req in discussionplugin causes many hits, but doing the same for other plugins or Trac code shows this is used only rarely.

For example, in `IMessageChangeListener`​ a context object is needed - but as this object often has extra attributes added, it's not clear what are actually available when implementing this interface. Also, after the call to message_created, it might/will have extra attributes on it :-) In fact, from reading now, it seems like no actual trac.mimeview.Context attributes are used in this call.

Some other uses of Context feel more awkward/unusual:

trac.mimeview.Context has no cursor attribute, but source:discussionplugin/0.11/tracdiscussion/api.py#L170​ places a cursor there. Later a lot of other things get put here too - source:discussionplugin/0.11/tracdiscussion/api.py#L301​. I understand it's handy to pass around some stafeul object, but feel some dictionary or discussionplugin specific object might be better? Grepping through a lot of plugins and Trac code, I don't see anywhere else that uses a dictionary at context.data when preparing what it's going to give to Genshi for rendering the template.

If we reorganise this a bit, is that interesting to you as a patch?

[Radek Bartoň]

Replying to pipern:

I've found the use of ​`trac.mimeview.Context` as a way of passing around the Request and data-dictionary a bit confusing - is there any interest in a patch which switches to a more traditional (IMHO, which may be wrong) Trac-like way of passing the req (and data) object rather than a Context instance? As the Context (when made with from_request does keep reference to the Request, maybe it's fine to avoid passing ALSO the Request around separately, but it doesn't feel very common - the Context (from the docstrings) is more about rendering. Grepping for context.req in discussionplugin causes many hits, but doing the same for other plugins or Trac code shows this is used only rarely.

Think of it as lower-level vs. higher-level abstraction of request processing. IRequestHandler.process_request(req) is a lower-level interface function that recognizes context of the request (core, wiki, timeline, etc.) and passes it to the higher-level discussion API interface IMessageChangeListener.message_created(context, message).

For example, in `IMessageChangeListener`​ a context object is needed - but as this object often has extra attributes added, it's not clear what are actually available when implementing this interface. Also, after the call to message_created, it might/will have extra attributes on it :-) In fact, from reading now, it seems like no actual trac.mimeview.Context attributes are used in this call.

Maybe to be correct, there should be a class DiscussionContext derived from Context that adds all that discussion specific attributes but I'm just exploiting the dynamicity of Python.

Some other uses of Context feel more awkward/unusual:

trac.mimeview.Context has no cursor attribute, but source:discussionplugin/0.11/tracdiscussion/api.py#L170​ places a cursor there. Later a lot of other things get put here too - source:discussionplugin/0.11/tracdiscussion/api.py#L301​. I understand it's handy to pass around some stafeul object, but feel some dictionary or discussionplugin specific object might be better? Grepping through a lot of plugins and Trac code, I don't see anywhere else that uses a dictionary at context.data when preparing what it's going to give to Genshi for rendering the template.

Cursor object is shared in the context object just for performance reasons and to save some code lines with constant repetition of its creation.

If we reorganise this a bit, is that interesting to you as a patch?

It depends on the type of changes. If this patch would simplify/clarify the thing and keep the philosophy as mentioned above untouched, I'd invite it.

Maybe it could help me to understand your objections if you'd give me the main reasoning of keeping the request object as dominant context information in the higher-level interfaces?

[anonymous]

Thanks for such a quick response!I understand a bit more where some of these things come from.

Replying to Blackhex:

Replying to pipern:

I've found the use of ​`trac.mimeview.Context` as a way of passing around the Request and data-dictionary a bit confusing - is there any interest in a patch which switches to a more traditional (IMHO, which may be wrong) Trac-like way of passing the req (and data) object rather than a Context instance? As the Context (when made with from_request does keep reference to the Request, maybe it's fine to avoid passing ALSO the Request around separately, but it doesn't feel very common - the Context (from the docstrings) is more about rendering. Grepping for context.req in discussionplugin causes many hits, but doing the same for other plugins or Trac code shows this is used only rarely.

Think of it as lower-level vs. higher-level abstraction of request processing. IRequestHandler.process_request(req) is a lower-level interface function that recognizes context of the request (core, wiki, timeline, etc.) and passes it to the higher-level discussion API interface IMessageChangeListener.message_created(context, message).

For example, in `IMessageChangeListener`​ a context object is needed - but as this object often has extra attributes added, it's not clear what are actually available when implementing this interface. Also, after the call to message_created, it might/will have extra attributes on it :-) In fact, from reading now, it seems like no actual trac.mimeview.Context attributes are used in this call.

Maybe to be correct, there should be a class DiscussionContext derived from Context that adds all that discussion specific attributes but I'm just exploiting the dynamicity of Python.

That would make it a lot clearer. Right now, I'm not at all sure what values are expect on a context, what are required, what are just 'for speed', and so on. It's to do with being new to the code, but also that it varies so much from other plugins/Trac code.

Some other uses of Context feel more awkward/unusual:

trac.mimeview.Context has no cursor attribute, but source:discussionplugin/0.11/tracdiscussion/api.py#L170​ places a cursor there. Later a lot of other things get put here too - source:discussionplugin/0.11/tracdiscussion/api.py#L301​. I understand it's handy to pass around some stafeul object, but feel some dictionary or discussionplugin specific object might be better? Grepping through a lot of plugins and Trac code, I don't see anywhere else that uses a dictionary at context.data when preparing what it's going to give to Genshi for rendering the template.

Cursor object is shared in the context object just for performance reasons and to save some code lines with constant repetition of its creation.

I think sometimes a context (with a cursor) is going into a function which then makes a new cursor anyway. Having a cursor attached to a mimeview.Context instance feels very strange.

If we reorganise this a bit, is that interesting to you as a patch?

It depends on the type of changes. If this patch would simplify/clarify the thing and keep the philosophy as mentioned above untouched, I'd invite it.

Maybe it could help me to understand your objections if you'd give me the main reasoning of keeping the request object as dominant context information in the higher-level interfaces?

It's more a confusion by having many extra things set on a mimeview.Context - such as a lot of rendering data that is going to be given to Genshi. I've read a lot of Trac code and plugins, and I found it hard to navigate some of the discussion code as it does irregular things :-)

Another example would be:

        # Determine template name.
        self._get_template(context, actions)

        # Return request template and data.
        self.log.debug('template: %s data: %s' % (context.template,
          context.data,))
        return context.template, {'discussion' : context.data}

it's not clear what _get_template() is doing to context, unless you know in advance.

       # Determine template name.
        template_name = self._get_template(context, actions)

        # Return request template and data.
        self.log.debug('template: %s data: %s' % (template_name,
          context.data,))
        return template_name, {'discussion' : context.data}

Probably when you learn when and where certain things are put on onto the 'context' object, it's easier to follow. But as someone new to this code, it feels like there is a huge amount of data being passed around so it's not (I would humbly suggest) clear what depends on what information, as it's all dynamically added to one huge object...

(I've just spotted some eval() usage - is pickling safer, in case some other plugin lets people put user-data into arbitrary session keys by mistake?:

        context.visited_forums = eval(context.req.session.get('visited-forums')
          or '{}')

)

[Radek Bartoň]

Replying to anonymous:

I think sometimes a context (with a cursor) is going into a function which then makes a new cursor anyway. Having a cursor attached to a mimeview.Context instance feels very strange.

That is in case when shared cursor has to be iterated while second cursor need to modify something in DB according to row data.

Probably when you learn when and where certain things are put on onto the 'context' object, it's easier to follow. But as someone new to this code, it feels like there is a huge amount of data being passed around so it's not (I would humbly suggest) clear what depends on what information, as it's all dynamically added to one huge object...

That's true.

(I've just spotted some eval() usage - is pickling safer, in case some other plugin lets people put user-data into arbitrary session keys by mistake?:

Maybe for some more complex structures. Here with just dictionary of integers it is stored in more readable form.

[pipern]

Replying to Blackhex:

Replying to anonymous:

(I've just spotted some eval() usage - is pickling safer, in case some other plugin lets people put user-data into arbitrary session keys by mistake?:

Maybe for some more complex structures. Here with just dictionary of integers it is stored in more readable form.

What if someone finds a way to put '__import__("os").system("rm -rf /")' into the visited-forums session entry?

Using a json parser, or pickle, is much safer. Or in Python 2.6, maybe this: ​http://docs.python.org/library/ast.html#ast.literal_eval but I don't have experience of that.

[anonymous]

What if someone finds a way to put '__import__("os").system("rm -rf /")' into the visited-forums session entry?

Ah, you mean that kind of safe :-). That wouldn't be good.

[Ryan J Ollos]

In 16492:

1.2dev: Refactor and use Trac 1.2 API

Refs #7990.

[Ryan J Ollos]

Many uses of context have been removed, and cursor is no longer attached to context. More could be done, but someone will probably have to provide a patch, otherwise we'll continue to progress there slowly.

I'm leaving this ticket open for fixing the use of eval.