Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #26817 -- Doc'd downsides and alternatives to Jinja2 context processors. #8063

Merged
merged 2 commits into from Mar 6, 2017

Conversation

timgraham
Copy link
Member

@timgraham
Copy link
Member Author

@carljm @aaugustin, this is based on your IRC discussion -- any input?

@carljm
Copy link
Member

carljm commented Feb 22, 2017

Looks reasonable to me.

Since context processors are run for every template that's rendered, think
carefully before putting expensive computations in them, especially if the
result isn't used in all or most of your templates.

Copy link
Member

@aaugustin aaugustin Feb 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This proposal looks technically correct to me but I think it could be misinterpreted.

The text starts by advising to use context processors while the general goal is to recommend against them. A critical condition is in parentheses.

Here's what I would write -- I know the quality of the wording isn't as good as the proposal above, I focused on the flow of ideas.


Using context processors with Jinja2 templates is discouraged.

Context processors are useful with Django templates because Django templates don't support calling functions with arguments. Jinja2 has no such limitation. Therefore it's possible and recommended to put the function that you would use as a context processor in the global variables available to the template (for example, using jinja2.Environment as described below) and then to call that function in the template::

{{ my_function(request) }}

Some built-in context processors for Django templates don't even depend on the request and return a fixed value. For Jinja2 templates, add this value to global variables directly instead of configuring a context processor to add it to the global variables.

Since context processors are run for every template that's rendered, think carefully before putting expensive computations in them, especially if the result isn't used in all or most of your templates.

The only known use case for context processors in Jinja2 templates requires the following conditions:

  • making an expensive computation that depends on the request
  • needing its results in every template
  • using its results multiple times in each template
  • not wanting to cache the result in an attribute of the request, for purity

If these conditions aren't met, passing a function to the template in global variables is simpler and more in line with the design of Jinja2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good but isn't the point about "Not wanting to cache the result in an attribute of the request, for purity." really more like "Avoiding the computation on requests that don't render templates." (assuming the alternative is caching the value on the request in a middleware's process_request()). I think that option should be described -- right now it looks like there are only two options: context processors and passing the function to the template.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While working on that comment I wrote something about middleware but eventuallly I erased that sentence because I was afraid to give bad advice.

Middleware can replace context processors because they can compute a value that depends on the request and store that value as an attribute of the request. Furthermore that attribute can be lazy (like request.user).

However, if I'm going to monkey-patch the result on the request to avoid computing it more than once in a request, I'd rather write a function that caches its result in a "private" attribute (request._something).

So here's why I left middleware out of this discussion:

  • they're even further from Jinja2 than Django's context processors, so less appropriate for calculating values to be used in templates
  • they still require monkey patching the request object, so they're no better than a function that caches its result in a private attribute in this regard
  • they require accessing directly the computed value on the request object, while both context processors and global functions create an abstraction that makes it easier to change their implementation
  • to avoid making the computation for every request, middleware require a lazy attribute, which is a bit complicated and fragile (like every attempt to proxy access to another object transparently in Python).

I prefer writing a function that caches its result on the request, as explained above.

Now, if memory serves, that's where Carl and I disagree. I'm tolerant of caching stuff on the request like this. He's more cautious and prefers to stay away from such hacks. That's what I meant by "for purity" but we need a better wording.

So, assuming the computed value is needed in every template and must not be computed more than once, the real trade off is:

  • use a global function — downside: requires monkey patching an attribute on the request to cache the result of the computation
  • use a context processor — downside: adds another level of abstraction that doesn't have a very strong reason for existing

At this point it's pretty much a matter of taste.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Aymeric's re-ordering to put the focus on the recommended approach.

For the cases where someone is reaching for a context processor (the context of these docs), I think middleware is never a preferable option (for the reasons @aaugustin lists), and best not mentioned here. The final comment about "not wanting to cache on a private attribute of the request" is more about the option that you could have a function which takes the request, and caches its result on the request to avoid repeat computation on repeat calls. I think this bullet point is confusing and should just be removed. Removing it does not imply that the three remaining bullet points can only be satisfied by using a context processor, just that a context processor is one reasonable choice if those conditions apply. We don't need to get too deep into the weeds.

In my use case that drove adding context processors, really the motivating factor was verbosity of templates more than purity about caching on the request. We had a me variable, injected by a context processor, that was a JSON-serialized version of request.user (needed because these templates were shared by server and client, so all data referenced in them needed to be JSON serialized in a consistent way, not server-only Python objects). This me variable was used very frequently in the templates (usually accessing elements of it), and neither I nor the front-end team maintaining the templates relished the idea of modifying every occurrence of e.g. {{ me.username }} to something like {{ me(request).username }}.

But I agree that this is well into "matter of taste" territory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Actually on second thought the concern in that case was more than aesthetic; there is no request on the client so the "function that operates on the request" option could not work for both server and client; the source of me was different in each case. But the aesthetic/verbosity issue alone would still have been sufficient, even without that additional factor.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

Tim: thanks for getting the ball rolling, feel free to take over from there if you'd like, else I can revisit my proposal to be less opinionated and better reflect the consensus.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I pushed an update.

@timgraham
Copy link
Member Author

Carl/Aymeric, want to check the latest wording?

@aaugustin
Copy link
Member

This looks very good. Thanks.

(This has been sitting in the leftmost tab of my browser for two weeks, I wanted to take a look this weekend but didn't get around to it.)

@carljm
Copy link
Member

carljm commented Mar 6, 2017

LGTM, thanks!

@timgraham timgraham merged commit dacdcec into django:master Mar 6, 2017
@timgraham timgraham deleted the 26817 branch March 6, 2017 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants