Changes between Version 1 and Version 11 of Ticket #24694


Ignore:
Timestamp:
Feb 14, 2017, 7:55:17 PM (8 years ago)
Author:
Tim Graham
Comment:

(edited IRC conversation for readability)

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #24694

    • Property Triage Stage Unreviewed → Accepted
    • Property Cc Farhan Ahmad berker.peksag@… added
    • Property Has patch set
    • Property Owner changed from nobody to Berker Peksag
    • Property Status new → closed
    • Property Resolution → fixed

  • Ticket #24694 – Description

    v1 v11  
    66
    77{{{
    8 18:35 <+carljm> mYk: I'm upgrading a project to Django 1.8, and I am wondering if we were wrong to not provide context processors for Jinja.
    9 18:35 <+carljm> IIRC the rationale was something like "you don't need them in Jinja because you can just put stuff into the env globals
    10                 directly", but this isn't really true - the point of context processors is that they provide request-specific values, and
    11                 there's no obvious way to do that.
    12 18:37 <+carljm> You can emulate by putting a function into the context that returns some value, but then you have inefficiency if that function
    13                 does something non-trivial and its value is used in multiple places.
    14 18:37 <+carljm> For a concrete example, this project wants a serialized (i.e. to primitive values), JSON-serializable representation of
    15                 request.user to be available in all templates.
    16 18:38 <+carljm> In 1.7 this was done with a context processor that provided that representation in a `me` variable.
    17 18:39 <+carljm> In 1.8, without some context-processor equivalent, that would have to become `me(request)` or `request|me` or something, and to
    18                 avoid serializing multiple times needlessly, it would often turn into `{% set me = me(request) %}`
    19 18:40 <+carljm> In looking into this, I also noticed that Flask does provide context processors (just like Django's) atop Jinja2.
    20 18:41 <+carljm> At the moment, I'm implementing context processors for the Django Jinja2 backend. It's not hard, but unfortunately requires
    21                 copying most of the implementation of the Jinja2 backend and its Template wrapper (they aren't really factored for subclass
    22                 customization)
    23 18:41 <+carljm> I'd be very interested to hear if you think that I'm Doing it Wrong and there's a better option available for this type of use
    24                 case.
    25 18:43 <+carljm> Or if I'm not wrong, perhaps we should consider adding support for OPTIONS['context_processors'] to the Jinja2 backend for 1.9.
    26 18:46 <+carljm> Or at least make it easier to subclass the backend and add that support. Specifically, this would mean factoring out an
    27                 "instantiate_template" (name to-be-bikeshedded) internal method on the backend class, which would be called by both from_string
    28                 and get_template, and which would allow a subclass to easily swap out both the Template wrapper class used and the arguments
    29                 passed to it.
    30 18:50 <+carljm> (Actually with @jinja2.contextfunction you could remove the explicit naming of the request in the above examples, but the other
    31                 issues still apply.)
    32 18:51 <+jezdez> carljm: doesn’t django-jinja support ctx processors natively?
    33 18:52 <+jezdez> not that it answers your questions above, just something that may buy you quicker results
    34 18:54 <+carljm> jezdez: So does jingo (which is what we were using on 1.7)
    35 18:55 <+carljm> Unfortunately jingo isn't 1.8-compatible (due to its use of template internals that changed in 1.8); do you know if
    36                 django-jinja is?
    37 18:55 <+carljm> In any case, part of the goal of this upgrade was to switch from an external Jinja support package to the native support, so
    38                 I'll probably continue on this path. Already done with adding context proc support, anyway :-)
    39 18:55 <+jezdez> carljm: right, I think I’d like to move away from jingo in general since it does a few things oddly
    40 18:55 <+carljm> Thanks though!
    41 18:56 <+jezdez> yeah, I heard django-jinja is 1.8 compatible
    42 18:56 <+MarkusH> I'd probably look into what they are doing to provide context processors and may adapt some of the monkey patches
    43 18:56 <+carljm> jezdez: What are you thinking of specifically? I was pretty happy with jingo as a pre-1.8 solution.
    44 18:56 < pmachine> carljm: django-jinja provides it's own backend class for 1.8
    45 18:56 <+carljm> Ah, nice!
    46 18:56 <+jezdez> carljm: the helpers api is clunky and the monkey patches just smell like tech debt
    47 18:56 <+jezdez> I don’t see why we can’t have one and only one implementation going forward now that 1.8 is out
    48 18:57 <+jezdez> carljm: but yeah, I get what you’re saying, it’d be neat to just move away from any library
    49 18:57 <+jezdez> maybe in 2.0
    50 18:57 <+jezdez> (not kidding)
    51 18:57 <+carljm> Yeah; with 1.8 having a Jinja2 backend, I'd like to see the "one and only implementation" be that one.
    52 18:58 <+carljm> as in, if using the built-in backend is painful enough that most people are using an external library on top of it still, we
    53                 should fix those pain points.
    54 18:58 <+jezdez> so maybe we should spend more time on implementing translation support and ctx processors?
    55 18:58 <+carljm> yep :-)
    56 [snip]
    57 19:05 < pmachine> btw, I'm +1 on enabling context processors and making the backend easier to subclass
    58 19:07 <+carljm> pmachine: cool. I'll file an issue and PR for it, but I'd like to see if mYk has any comments first.
    59 19:08 <+carljm> (or more likely, two issues and two PRs)
    60 19:14 <+mYk> am I being summoned?
    61 19:14 <+mYk> I'm reading the backlog
    62 19:15 <+mYk> carljm: part of the decision was: "it's easier not to provide context processors at first and add them later than the opposite"
    63 19:16 <+mYk> if Flask provides them, that's a strong argument
    64 19:16 <+carljm> yes, definitely better to start minimal
    65 19:16 <+carljm> Flask, and it seems every third-party Django-Jinja adapter, too.
    66 [snip discussion of Jinja backend subclassibility]
    67 19:21 <+mYk> it seems more straightforward to me to just add support for context processors
    68 19:22 <+mYk> the question is -- should it be a "standard" option (like DIRS and APP_DIRS) or a "backend-specific" option (which goes inside
    69              OPTIONS)?
    70 19:23 <+mYk> since 1.8 was released already and context_processors is a "backend-specific" option of the Django backend, the latter option
    71              seems less disruptive
    72 19:23 <+carljm> yes, that was my feeling too
    73 19:23 <+mYk> to be clear -- this is mostly a convenience & performance thing, right?
    74 19:24 <+mYk> you want to write {{ context_processor_output }} instead of {{ context_processor(request) }} in templates
    75 19:24 <+carljm> precisely, yes. it doesn't enable anything that is impossible otherwise.
    76 19:24 <+carljm> it's really the same thing you're doing with csrf_input and csrf_token in the current backend
    77 19:25 <+carljm> only there you've used yet another approach (Lazy), which is one I'd dearly love to avoid :-)
    78 19:25 <+mYk> well I had to in that case
    79 19:25 <+mYk> coming back to context processors
    80 19:25 <+mYk> I really like the explicitness of {{ context_processor(request) }}
    81 19:26 <+carljm> yes, in that case it does have to be lazy so CSRF token isn't triggered unnecessarily - but that approach does solve the same
    82                 cases
    83 19:26 <+mYk> I think it fits well with Django's insistence on passing the request to each view function
    84 19:26 <+carljm> that in itself doesn't bother me much
    85 19:26 <+carljm> but for a value which may be used many places, the efficiency concern does.
    86 19:27 <+mYk> it also clarifies whether you can render a given template without a request (ie outside of the request-response cycle)
    87 19:27 <+carljm> you can do {% set somevar = context_processor(request) %}
    88 19:27 <+carljm> but for ubiqitous values (the only kind I would ever use a context proc for anyway), that's ugly boilerplate to have in all
    89                 templates
    90 19:28 <+mYk> do you often have computationally expensive things that you use multiple times in a given template?
    91 19:28 <+mYk> you could also cache the value in a private attribute of the request, but that's ugly
    92 19:28 <+carljm> yes, I considered that, too
    93 19:29 <+mYk> if we step back for a minute, my general position is:
    94 19:30 <+mYk> * real-life usage of context processors (starting with django's built-in processors) leads me to think that they aren't a very
    95              good API
    96 19:30 <+carljm> in the current project, I have four context processors I would use in Jinja. Only one of them (serialized user) is expensive
    97                 and widely-used enough that I'd be concerned about calling it at every use site.
    98 19:31 <+mYk> * they implicitly depend on the request, which is a net negative for me, because I'm a strong proponent of dumb code
    99 19:31 <+mYk> * if we come up with use cases for which they're clearly the best answer, I'm happy to add them, but I'll still be afraid that
    100              they will be misused
    101 19:33 <+mYk> based on what you've described, I'm wondering if some sort of caching isn't a better answer to your goal: optimizing performance
    102              by avoiding repeated calls to an expensive function
    103 19:34 <+carljm> ...thinking...
    104 19:34 <+mYk> (and that will avoid paying the cost of calling that function on pages that don't use it, if any)
    105 19:35 <+mYk> one last idea: context processors look to me like response middleware in disguise
    106 19:35 <+mYk> if you have something complicated (or expensive) to do in each response, perhaps doing it explicitly in a middleware would be a
    107              good design
    108 19:36 <+carljm> Yes, that's a variant of "cache the value in a private attribute of the request", where the value instead becomes a public
    109                 attribute of the request, attached in a middleware.
    110 19:37 <+carljm> I agree that solves the use case, and preserves the explicit dependence on the request.
    111 19:38 <+mYk> the various ideas I've suggested are merely shifting the executing of this function and the caching of its results at various
    112              points in the request/response cycle
    113 19:38 <+carljm> It means the value is computed for all requests, even when a template is not rendered.
    114 19:38 <+mYk> I'm trying to optimize for "there should be one obvious way to do it" and "explicit is better than implicit"
    115 19:39 <+carljm> Yes. The question is whether "compute once when a template is rendered, and make globally available in the template context"
    116                 has enough good use cases to provide API for.
    117 19:39 <+mYk> it means the value is computed for all requests => maybe -- off the top of my head, I'm not sure how to tell if a given response
    118              is going to render a template
    119 19:39 <+carljm> My meta-concern is that it doesn't really matter whether context processors are good API; Django is stuck with them. Unless we
    120                 deprecate them for DTL, the result of not providing them for Jinja may be that people just use external Jinja backends instead
    121                 which provide them.
    122 19:40 <+mYk> yes...
    123 19:41 <+mYk> being technically correct serves us well if everyone keeps using django-jinja...
    124 19:41 <+mYk> that reminds me of mistakes I made in the time zone APIs, which were technically correct at first, and are slowly becoming usable
    125              by mortals
    126 19:42 <+carljm> I think for my use case (which I think is the prototypical best use case for a context processor), the best alternative would
    127                 be a cached_property on the request.
    128 19:42 <+mYk> carljm: a realistic solution may be to provide context processors for users who are used to them and don't want to think about it,
    129              and discussthe alternatives in the docs
    130 19:42 <+carljm> Thus not computed unless accessed, but computed only once when accessed.
    131 19:42 <+carljm> Unfortunately Django makes that hard to do - trying to do the equivalent in a middleware really requires use of Lazy
    132 19:43 <+carljm> because you don't get to subclass Request, you just get to monkeypatch it.
    133 19:43 <+mYk> yup, AuthMiddleware has horrific code to set up request.user
    134 19:44 <+carljm> yes, horrific code which has been rewritten at least once because the previous version was both horrific and broken.
    135 19:45 <+mYk> I'm not taking bets on whether the current version is also horrific and broken :-)
    136 19:45 <+carljm> Well, my conclusion at this point is that while I see the arguments for explicit dependence on the request, I don't yet see a
    137                 practical alternative to a context processor that I would be happy with for this use case.
    138 19:45 <+mYk> okay
    139 19:46 <+carljm> And given that, plus the precedent for context-processors in Django and the likelihood that people will find an implementation
    140                 of them one way or another, I think I'd remain in favor of adding them.
    141 19:46  -!- f3lp <~f@179.218.60.168> has joined #django-dev
    142 19:46 <+carljm> (Perhaps with discussion of downsides and alternatives in the docs.)
    143 19:46 <+mYk> that works for me
    144 19:47 <+mYk> keeping them as a "backend-specific" option will also de-emphasize them a bit
    145 19:47 <+mYk> (even though we're just doing this because the ship has sailed with 1.8)
    146 19:47 <+carljm> A cached function is the closest thing to an alternative that I'd want to use, but then you have to decide between caching in a
    147                 private request attr (ick) vs some other caching approach (which feels wrong because the value is logically request-scoped)
    148 19:48 <+carljm> sure
    149 19:48 <+carljm> And in practice unless we provide a seamless convenience API for the request-scoped-cached-function approach, I don't think
    150                 anyone will choose to use it over context processors.
    151 19:50 <+carljm> Ok, I will submit an issue (now) and PR (later).
    152 19:51 <+carljm> Thanks for the thought-provoking discussion!
     8<carljm> mYk: I am wondering if we were wrong to not provide context processors for Jinja. IIRC the rationale was something like "you don't need them in Jinja because you can just put stuff into the env globals directly", but this isn't really true - the point of context processors is that they provide request-specific values, and there's no obvious way to do that.
     9
     10You can emulate by putting a function into the context that returns some value, but then you have inefficiency if that function does something non-trivial and its value is used in multiple places.
     11
     12For a concrete example, this project wants a serialized (i.e. to primitive values), JSON-serializable representation of  request.user to be available in all templates.
     13
     14In 1.7 this was done with a context processor that provided that representation in a `me` variable. In 1.8, without some context-processor equivalent, that would have to become `me(request)` or `request|me` or something, and to avoid serializing multiple times needlessly, it would often turn into `{% set me = me(request) %}`
     15
     16In looking into this, I also noticed that Flask does provide context processors (just like Django's) atop Jinja2.
     17At the moment, I'm implementing context processors for the Django Jinja2 backend. It's not hard, but unfortunately requires copying most of the implementation of the Jinja2 backend and its Template wrapper (they aren't really factored for subclass customization).
     18
     19I'd be very interested to hear if you think that I'm Doing it Wrong and there's a better option available for this type of use case. Or if I'm not wrong, perhaps we should consider adding support for OPTIONS['context_processors'] to the Jinja2 backend for 1.9. Or at least make it easier to subclass the backend and add that support. Specifically, this would mean factoring out an "instantiate_template" (name to-be-bikeshedded) internal method on the backend class, which would be called by both from_string and get_template, and which would allow a subclass to easily swap out both the Template wrapper class used and the arguments passed to it. (Actually with @jinja2.contextfunction you could remove the explicit naming of the request in the above examples, but the other issues still apply.)
     20
     21<jezdez> doesn’t django-jinja support ctx processors natively? not that it answers your questions above, just something that may buy you quicker results.
     22<carljm> So does jingo (which is what we were using on 1.7). Unfortunately jingo isn't 1.8-compatible (due to its use of template internals that changed in 1.8); do you know if django-jinja is? In any case, part of the goal of this upgrade was to switch from an external Jinja support package to the native support, so I'll probably continue on this path. Already done with adding context proc support, anyway :-)
     23<jezdez> right, I think I’d like to move away from jingo in general since it does a few things oddly
     24<carljm> Thanks though!
     25<jezdez> yeah, I heard django-jinja is 1.8 compatible
     26<carljm> What are you thinking of specifically? I was pretty happy with jingo as a pre-1.8 solution.
     27<pmachine> django-jinja provides it's own backend class for 1.8
     28<carljm> Ah, nice!
     29<jezdez> The helpers api is clunky and the monkey patches just smell like tech debt. I don’t see why we can’t have one and only one implementation going forward now that 1.8 is out, but yeah, I get what you’re saying, it’d be neat to just move away from any library.
     30<carljm> Yeah; with 1.8 having a Jinja2 backend, I'd like to see the "one and only implementation" be that one. As in, if using the built-in backend is painful enough that most people are using an external library on top of it still, we should fix those pain points.
     31<jezdez> so maybe we should spend more time on implementing translation support and ctx processors?
     32<carljm> yep :-)
     33<pmachine> btw, I'm +1 on enabling context processors and making the backend easier to subclass
     34<carljm> I'll file an issue and PR for it, but I'd like to see if mYk has any comments first.
     35<mYk> carljm: part of the decision was: "it's easier not to provide context processors at first and add them later than the opposite". If Flask provides them, that's a strong argument.
     36<carljm> yes, definitely better to start minimal
     37<carljm> Flask, and it seems every third-party Django-Jinja adapter, too.
     38<mYk> it seems more straightforward to me to just add support for context processors. The question is -- should it be a "standard" option (like DIRS and APP_DIRS) or a "backend-specific" option (which goes inside OPTIONS)? Since 1.8 was released already and context_processors is a "backend-specific" option of the Django backend, the latter option seems less disruptive
     39<carljm> yes, that was my feeling too
     40<mYk> to be clear -- this is mostly a convenience & performance thing, right? you want to write {{ context_processor_output }} instead of {{ context_processor(request) }} in templates
     41<carljm> precisely, yes. it doesn't enable anything that is impossible otherwise. It's really the same thing you're doing with csrf_input and csrf_token in the current backend, only there you've used yet another approach (Lazy), which is one I'd dearly love to avoid :-)
     42<mYk> well I had to in that case. coming back to context processors, I really like the explicitness of {{ context_processor(request) }}
     43<carljm> yes, in that case it does have to be lazy so CSRF token isn't triggered unnecessarily - but that approach does solve the same cases
     44<mYk> I think it fits well with Django's insistence on passing the request to each view function
     45<carljm> that in itself doesn't bother me much, but for a value which may be used many places, the efficiency concern does.
     46<mYk> it also clarifies whether you can render a given template without a request (ie outside of the request-response cycle)
     47<carljm> you can do {% set somevar = context_processor(request) %}, but for ubiquitous values (the only kind I would ever use a context proc for anyway), that's ugly boilerplate to have in all templates
     48<mYk> do you often have computationally expensive things that you use multiple times in a given template?. You could also cache the value in a private attribute of the request, but that's ugly
     49<carljm> yes, I considered that, too. In the current project, I have four context processors I would use in Jinja. Only one of them (serialized user) is expensive and widely-used enough that I'd be concerned about calling it at every use site.
     50<mYk> if we step back for a minute, my general position is:
     51* real-life usage of context processors (starting with django's built-in processors) leads me to think that they aren't a very good API
     52* they implicitly depend on the request, which is a net negative for me, because I'm a strong proponent of dumb code
     53* if we come up with use cases for which they're clearly the best answer, I'm happy to add them, but I'll still be afraid that they will be misused based on what you've described, I'm wondering if some sort of caching isn't a better answer to your goal: optimizing performance by avoiding repeated calls to an expensive function
     54<carljm> ...thinking...
     55<mYk> (and that will avoid paying the cost of calling that function on pages that don't use it, if any)
     56one last idea: context processors look to me like response middleware in disguise. If you have something complicated (or expensive) to do in each response, perhaps doing it explicitly in a middleware would be a good design.
     57<carljm> Yes, that's a variant of "cache the value in a private attribute of the request", where the value instead becomes a public attribute of the request, attached in a middleware. I agree that solves the use case, and preserves the explicit dependence on the request.
     58<mYk> the various ideas I've suggested are merely shifting the executing of this function and the caching of its results at various points in the request/response cycle
     59<carljm> It means the value is computed for all requests, even when a template is not rendered.
     60<mYk> I'm trying to optimize for "there should be one obvious way to do it" and "explicit is better than implicit"
     61<carljm> Yes. The question is whether "compute once when a template is rendered, and make globally available in the template context" has enough good use cases to provide API for.
     62<mYk> it means the value is computed for all requests => maybe -- off the top of my head, I'm not sure how to tell if a given response is going to render a template
     63<carljm> My meta-concern is that it doesn't really matter whether context processors are good API; Django is stuck with them. Unless we deprecate them for DTL, the result of not providing them for Jinja may be that people just use external Jinja backends instead which provide them.
     64<mYk> yes... being technically correct serves us well if everyone keeps using django-jinja... that reminds me of mistakes I made in the time zone APIs, which were technically correct at first, and are slowly becoming usable by mortals
     65<carljm> I think for my use case (which I think is the prototypical best use case for a context processor), the best alternative would be a cached_property on the request.
     66<mYk> carljm: a realistic solution may be to provide context processors for users who are used to them and don't want to think about it, and discuss the alternatives in the docs
     67<carljm> Thus not computed unless accessed, but computed only once when accessed. Unfortunately Django makes that hard to do - trying to do the equivalent in a middleware really requires use of Lazy because you don't get to subclass Request, you just get to monkeypatch it.
     68<mYk> yup, AuthMiddleware has horrific code to set up request.user
     69<carljm> yes, horrific code which has been rewritten at least once because the previous version was both horrific and broken.
     70<mYk> I'm not taking bets on whether the current version is also horrific and broken :-)
     71<carljm> Well, my conclusion at this point is that while I see the arguments for explicit dependence on the request, I don't yet see a practical alternative to a context processor that I would be happy with for this use case.
     72<mYk> okay
     73<carljm> And given that, plus the precedent for context-processors in Django and the likelihood that people will find an implementation of them one way or another, I think I'd remain in favor of adding them. (Perhaps with discussion of downsides and alternatives in the docs.)
     74<mYk> that works for me. keeping them as a "backend-specific" option will also de-emphasize them a bit (even though we're just doing this because the ship has sailed with 1.8)
     75<carljm> A cached function is the closest thing to an alternative that I'd want to use, but then you have to decide between caching in a private request attr (ick) vs some other caching approach (which feels wrong because the value is logically request-scoped)
     76<carljm> sure, and in practice unless we provide a seamless convenience API for the request-scoped-cached-function approach, I don't think anyone will choose to use it over context processors.
    15377}}}
Back to Top