Gentle Proposal: add 'render' shortcut in 1.3
Mikhail Korobov
I'm talking about this ticket: http://code.djangoproject.com/ticket/12816
With class-based views landed and the deprecation of
django.views.generic.simple (and direct_to_template in particular)
there won't be simple and undeprecated shortcut for
render_to_response.
So please add the 'render' shortcut in 1.3. direct_to_template was
always a hack (when used as render_to_response replacement) because it
'unrolls' callables and 1.3 seems to be a perfect place to replace
small hacks for proper solutions.
Please take my proposal as a gentle reminder :) If there are reasons
for this not to be in 1.3 (developer time, unresolved questions, etc.)
then fine, but it'll be pity if this feature won't be in next release
just because we forget about it.
Jacob Kaplan-Moss
> So please add the 'render' shortcut in 1.3.
It's one of the things on my list. If you'd like to make it happen
faster, a patch + tests would make it a no-brainer for me.
Jacob
Mikhail Korobov
There is an unresolved question in the ticket: "The only hesitation is
the relationship with #12815; we should resolve that decision before
committing anything for this ticket."
#12815 is about introducing TemplateResponse. Is the patch with
'render' shortcut returning just HttpResponse acceptable? I think that
TemplateResponse is less useful after class-based views make their way
into trunk so 'render' shortcut shouldn't bother returning
TemplateResponse. There are ways to reuse view logic (and change view
context in particular) now and TemplateResponse (which was a good
addition to django 1.2/1.1/1.0) seems to only complicate things in
django 1.3.
On 21 окт, 01:02, Jacob Kaplan-Moss <ja...@jacobian.org> wrote:
Jacob Kaplan-Moss
> There is an unresolved question in the ticket: "The only hesitation is
> the relationship with #12815; we should resolve that decision before
> committing anything for this ticket."
>
> #12815 is about introducing TemplateResponse. Is the patch with
> 'render' shortcut returning just HttpResponse acceptable? I think that
> TemplateResponse is less useful after class-based views make their way
> into trunk so 'render' shortcut shouldn't bother returning
> TemplateResponse. There are ways to reuse view logic (and change view
> context in particular) now and TemplateResponse (which was a good
> addition to django 1.2/1.1/1.0) seems to only complicate things in
> django 1.3.
I agree completely with this reasoning - just render returning an
HttpResponse is fine, I think.
Jacob
Łukasz Rekucki
Both render_to_response() and direct_to_template() have one very
annoying flaw: http://code.djangoproject.com/ticket/12669. Please add
a "response_class" keyword to your render() function ;). Thanks!
--
Łukasz Rekucki
Mikhail Korobov
and it is in 'almost-wontfix' state now. Yes, it's a great time to
either move it to wontfix or mark as accepted and implement alongside
with the render shortcut.
On 21 окт, 02:05, Łukasz Rekucki <lreku...@gmail.com> wrote:
> On 20 October 2010 21:57, Jacob Kaplan-Moss <ja...@jacobian.org> wrote:
>
>
>
>
>
Ivan Sagalaev
> #12815 is about introducing TemplateResponse. Is the patch with
> 'render' shortcut returning just HttpResponse acceptable? I think that
> TemplateResponse is less useful after class-based views make their way
> into trunk so 'render' shortcut shouldn't bother returning
> TemplateResponse. There are ways to reuse view logic (and change view
> context in particular) now and TemplateResponse (which was a good
> addition to django 1.2/1.1/1.0) seems to only complicate things in
> django 1.3.
Wait!!!
Sorry… Hello everyone :-)
If I remember correctly TemplateResponse was solving a problem of some
middleware wanting to mess with a view context before it's baked into
final string representation. This would solve in a good way what now is
accomplished by things like @render_to decorator.
What I don't understand from your reasoning is how class-based views are
going to help here? From what I see only Django-supplied generic views
are now class-based and we didn't deprecate simple user view functions.
Which means that "render" won't be as useful for them if it wouldn't
provide any means to post-process the context and if the context won't
be aware of request: these are two main points why people are not happy
with render_to_response right now.
Mikhail, do you have any actual objections against TemplateResponse or
you just don't want to complicate your implementation? If it is the
latter then TemplateResponse has been already implemented[1] by Simon
Willison. You might just use it.
[1]:
http://github.com/simonw/django-openid/blob/master/django_openid/response.py
Mikhail Korobov
On 21 окт, 03:00, Ivan Sagalaev <man...@softwaremaniacs.org> wrote:
>
> Wait!!!
>
> Sorry… Hello everyone :-)
>
> If I remember correctly TemplateResponse was solving a problem of some
> middleware wanting to mess with a view context before it's baked into
> final string representation. This would solve in a good way what now is
> accomplished by things like @render_to decorator.
>
> What I don't understand from your reasoning is how class-based views are
> going to help here? From what I see only Django-supplied generic views
> are now class-based and we didn't deprecate simple user view functions.
> Which means that "render" won't be as useful for them if it wouldn't
> provide any means to post-process the context and if the context won't
> be aware of request: these are two main points why people are not happy
> with render_to_response right now.
>
1. Developer writes a view and want to reuse it (e.g. change the
context).
My assumptions was:
a) Class based views are now the recommended way to write reusable
views
b) The main benefit from TemplateResponse is the ability to reuse view
responses. I made this assumption because of the example in #12815,
the 'this pattern would be particularly valuable for customising the
admin' statement and the original example in your proposal (
http://groups.google.com/group/django-developers/msg/d5df254f01800ee2
).
c) b) can now be achieved by writing a class-based view
2. Developer wants to write a middleware that messes with view
context. Class-based views are not going to help here. But I thought
it is not nearly as demanded as the first use case. This is useful but
not mandatory. That's where my reasoning came from.
> Mikhail, do you have any actual objections against TemplateResponse or
> you just don't want to complicate your implementation? If it is the
> latter then TemplateResponse has been already implemented[1] by Simon
> Willison. You might just use it.
>
Thanks for pointing out the implemented TemplateResponse. No, I
haven't actual objections against it and just don't want to complicate
the implementation. There was an issue with TemplateResponse approach
for Ben Firshman ( http://groups.google.com/group/django-developers/msg/fc9e0f8810d3e784
) and the ticket is in DDN so it is not as simple as just replace
HttpResponse with TemplateResponse.
Ivan Sagalaev
> 1. Developer writes a view and want to reuse it (e.g. change the
> context).
>
> My assumptions was:
> a) Class based views are now the recommended way to write reusable
> views
This is where our views differ. I have to admit that I didn't follow
closely the process of designing class-based generic views but I never
had a feeling that the class-based approach was now somehow recommended
way to create views in general. If it's the case it would be the most
significant change in overall Django design since introducing meta-class
based models (and a really bad change if you ask me).
Can someone of core committers clarify this: is it now recommended to
write all views as classes?
> b) The main benefit from TemplateResponse is the ability to reuse view
> responses. I made this assumption because of the example in #12815,
> the 'this pattern would be particularly valuable for customising the
> admin' statement and the original example in your proposal (
> http://groups.google.com/group/django-developers/msg/d5df254f01800ee2
> ).
Yes this is the main benefit. The example with middleware from my
previous email was misleading, sorry.
> Thanks for pointing out the implemented TemplateResponse. No, I
> haven't actual objections against it and just don't want to complicate
> the implementation. There was an issue with TemplateResponse approach
> for Ben Firshman ( http://groups.google.com/group/django-developers/msg/fc9e0f8810d3e784
> ) and the ticket is in DDN so it is not as simple as just replace
> HttpResponse with TemplateResponse.
I can't test it right now myself but from what I remember exceptions in
the response middleware were always handled. And in the 1.2 cycle we've
fixed it for request middleware too. So may be the problem doesn't even
exist anymore. Can you point me to that ticket which is in DDN so I
could test it?
James Bennett
<man...@softwaremaniacs.org> wrote:
> Can someone of core committers clarify this: is it now recommended to write
> all views as classes?
Django cares about whether your views meet the following criteria:
1. Is a callable object.
2. When called, accepts an instance of HttpRequest as its first
positional argument.
3. When called, returns an instance of HttpResponse or raises an exception.
That's it. Write them as functions, if that makes sense for your use
case. Write them as classes with callable instances, if that makes
sense for your use case. Generic views are now class-based because
that's what seems to work best *for the case of generic views*;
subclassing and overriding is, for the type of flexibility the generic
views need, simpler and cleaner than supporting long lists of keyword
arguments. But the generic views are not all views, and not all views
have to do what the generic views do. So my position is that you
should think about what you need from your view, and choose
class-based or function-based as appropriate.
--
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."
Ivan Sagalaev
> Django cares about whether your views meet the following criteria:
>
> 1. Is a callable object.
> 2. When called, accepts an instance of HttpRequest as its first
> positional argument.
> 3. When called, returns an instance of HttpResponse or raises an exception.
>
> That's it. Write them as functions, if that makes sense for your use
> case. Write them as classes with callable instances, if that makes
> sense for your use case. Generic views are now class-based because
> that's what seems to work best *for the case of generic views*;
> subclassing and overriding is, for the type of flexibility the generic
> views need, simpler and cleaner than supporting long lists of keyword
> arguments. But the generic views are not all views, and not all views
> have to do what the generic views do. So my position is that you
> should think about what you need from your view, and choose
> class-based or function-based as appropriate.
Thanks, that what I thought.
Going back to the argument about TemplateResponse I can say that it's a
good way for a view-as-function to keep its response hackable.
Mikhail Korobov
I was talking about reusable views and most views don't have to be
reusable (though it would be nice). Django now can help developer in
writing reusable views and it was not the case when TemplateResponse
was invented. That's what I mean with "TemplateResponse is less useful
now". TemplateResponse and class-based views are targeting the same
problem and they are different ways to solve it. I don't know what is
better, the argument was that we already have one way to reuse view
logic.
My statement about "only complicate things in django 1.3" was
premature, sorry. TemplateResponse is indeed a nice way to reuse view
logic that don't require writing a class-based view.
The ticket in DDN is http://code.djangoproject.com/ticket/12815.
So now the questions are:
1. Do we need 2 nice ways for reusing view logic (with and without
classes)? I don't have an opinion on this.
2. Does TemplateResponse allow pretty exception pages or not? Is Ben's
issue resolved?
Ivan Sagalaev
> 2. Does TemplateResponse allow pretty exception pages or not? Is Ben's
> issue resolved?
I'll look into it this evening (MSD).
Ivan Sagalaev
So I did.
There are actually two problems:
- Exceptions in response middleware are indeed happen outside of the
request's `try .. except` block. This is a problem by itself[1] and I'd
be happy to fix it after ticket 9886[2] is committed. It's another
refactoring of core request code so I don't want to mess things up doing
one patch on top of another :-).
- Non-pretty plain text tracebacks can be caused not only be middleware
but also by any error occurring during template rendering. Because all
this happen *after* request was returned to the web server handler and
is being iterated over.
This second problem can be easily fixed by introducing a method for
explicit evaluation of the content: `evaluate()` or `force_content()`
that will be a noop for any HttpRespone class except the
TemplateResponse. The method will be called by the request handler right
before returning the response.
Sounds good?
P.S. BTW looking at the TemplateResponse implementation I see that Simon
had actually intended it to be effectively *the* render shortcut[3]. I
find it quite beautiful really :-)
[1]: http://code.djangoproject.com/ticket/14523
[2]: http://code.djangoproject.com/ticket/9886
[3]:
http://github.com/simonw/django-openid/blob/master/django_openid/response.py#L85
Mikhail Korobov
refactoring ;) Ivan, thank you for the research.
I'll provide a draft patch for 'render == TemplateResponse' soon.
By the way, Łukasz Rekucki's suggestion to add the 'response_class' to
render shortcut is complicated much by TemplateResponse because
SimpleTemplateResponse is inherited from HttpResponse.
Mikhail Korobov
This is the TempleteResponse by Simon Willison with tests and minor
tweaks.
Notes:
- TemplateResponse and SimpleTemplateResponse reside in
django.template.response
- django/shortcuts/__init__.py used to have extra spaces. They are
removed.
- *args and **kwargs in TemplateResponse and SimpleTemplateResponse
are gone in order to provide more help for IDEs
- 'content' argument is not allowed for TemplateResponse and
SimpleTemplateResponse now
- context can be omitted now, the only required argument for
SimpleTemplateResponse is the template; request and template are
required for TemplateResponse
- 'response_class' is not implemented but the status code can be
passed as 'status' parameter.
- _set_content doesn't return a value now
The result:
from django.shortcuts import render
def my_view(request):
render(request, 'foo.json', {'foo': 'bar'}, 'application/json',
504)
def extended_view(request)
response = my_view(request)
response.template_context.update({'foo': 'spam', 'baz': 'egg'})
response.template_name = 'spam.json'
response.status_code = 400
return response
Russell Keith-Magee
<man...@softwaremaniacs.org> wrote:
> On 10/21/2010 03:22 PM, Ivan Sagalaev wrote:
>>
>> On 10/21/2010 11:49 AM, Mikhail Korobov wrote:
>>>
>>> 2. Does TemplateResponse allow pretty exception pages or not? Is Ben's
>>> issue resolved?
>>
>> I'll look into it this evening (MSD).
>
> So I did.
>
> There are actually two problems:
>
> - Exceptions in response middleware are indeed happen outside of the
> request's `try .. except` block. This is a problem by itself[1] and I'd be
> happy to fix it after ticket 9886[2] is committed. It's another refactoring
> of core request code so I don't want to mess things up doing one patch on
> top of another :-).
>
> - Non-pretty plain text tracebacks can be caused not only be middleware but
> also by any error occurring during template rendering. Because all this
> happen *after* request was returned to the web server handler and is being
> iterated over.
>
> This second problem can be easily fixed by introducing a method for explicit
> evaluation of the content: `evaluate()` or `force_content()` that will be a
> noop for any HttpRespone class except the TemplateResponse. The method will
> be called by the request handler right before returning the response.
>
> Sounds good?
Hi Ivan and Mikhail,
Just so you don't get the feeling that you're just discussing this
between yourselves -- I'm historically on record of being in favor of
the render() shortcut and the TemplateResponse(), and my position
hasn't changed recently. This is exactly the sort of problem that I
want to target for 1.3 (especially since we're now tracking 6 closely
related tickets -- #9081, #9886, #12815, #12816, #12669 and #14523 --
making this a sweet spot for attention).
Jacob has already marked #9886 RFC, and on first inspection, the patch
looks good to me too; I want to have a closer look before I commit,
though. If you want to proceed assuming that #9886 will be committed
(i.e., make the fix for #14523 have #9866 as a prerequisite), I doubt
it would be wasted effort.
Regarding the second problem you describe -- unless I'm mistaken,
Simon's TemplateResponse addresses this with his "baking" concept; a
response is baked the first time it is iterated or otherwise
evaluated.
I have a couple of non-Django things to attend to over the next week
or so; but once those are sorted out, I should be able to take a
detailed look at any patches you have prepared.
Yours,
Russ Magee %-)
Russell Keith-Magee
> Patch is ready for review: http://bitbucket.org/kmike/django/changeset/37d977574923
>
> This is the TempleteResponse by Simon Willison with tests and minor
> tweaks.
>
> Notes:
>
> - TemplateResponse and SimpleTemplateResponse reside in
> django.template.response
> - django/shortcuts/__init__.py used to have extra spaces. They are
> removed.
> - *args and **kwargs in TemplateResponse and SimpleTemplateResponse
> are gone in order to provide more help for IDEs
> - 'content' argument is not allowed for TemplateResponse and
> SimpleTemplateResponse now
> - context can be omitted now, the only required argument for
> SimpleTemplateResponse is the template; request and template are
> required for TemplateResponse
> - 'response_class' is not implemented but the status code can be
> passed as 'status' parameter.
> - _set_content doesn't return a value now
This is starting to look quite good. Some quick review comments:
* Integration with generic views. I would have thought that
TemplateResponseMixin would be a natural place to be using a
TemplateResponse.
* Tests can't rely on assumed context processors, as the comment in
the TemplateResponse tests states. See the flatpages views tests for
an example of how to work around this sort of problem.
* MOAR TESTS!1!! :-) There are a couple of API points that aren't
tested, such as set_content(), force_bake() and iteration over content
as a baking trigger. There might be a couple of others; these are just
the ones that jumped out at me.
* Docs! I appreciate that writing docs is premature when we're still
fiddling with details, but I think the core API is getting close, so
now would be a good time to at least put in some stub documentation
indicating what needs to be filled out later.
Yours,
Russ Magee %-)
Yours,
Russ Magee %-)
Russell Keith-Magee
Is this addressed by the status_code argument to TemplateResponse? i.e.,
return TemplateResponse(request, template, context, status_code=403)
We could also include some constants to make things a little prettier:
return TemplateResponse(request, template, context, status_code=FORBIDDEN)
Would that satisfy your use case for #12669?
Yours,
Russ Magee %-)
Ivan Sagalaev
On 10/22/2010 05:20 AM, Russell Keith-Magee wrote:
> Jacob has already marked #9886 RFC, and on first inspection, the patch
> looks good to me too; I want to have a closer look before I commit,
> though. If you want to proceed assuming that #9886 will be committed
> (i.e., make the fix for #14523 have #9866 as a prerequisite), I doubt
> it would be wasted effort.
I didn't feel it would be wasted :-). I was going to start doing the new
patch on top of my local bzr branch with #9886 applied (arent' DVCSes
awesome?). What I meant is that I didn't want to actually attach a diff
file to the ticket yet because it has chances of not being applicable
against trunk.
> Regarding the second problem you describe -- unless I'm mistaken,
> Simon's TemplateResponse addresses this with his "baking" concept; a
> response is baked the first time it is iterated or otherwise
> evaluated.
The problem is that this iteration may (and does) happen after response
is returned from the handler � and hence outside the `try .. except`.
The point is to call baking explicitly right before the return.
Mikhail Korobov
_set_content behaves differently for baked and non-baked responses:
response = render(request, Template('foo'))
response.content = 'bar'
print response.content # 'foo'
response.content = 'baz'
print response.content # 'baz'
This is confusing so I think responses should be marked as baked in
_set_content, not in force_bake.
The patch that should resolve this concern and Russell's concerns
regarding the tests: http://bitbucket.org/kmike/django/changeset/00f8be464749
I'll take a look at docs and generic views integration later.
Should new generic views return TemplateResponse by default?
Łukasz Rekucki
Yes, it does, but constants are a must have, imho.
Also, excuse my ignorance, but even after reading this thread, code
and tests I don't understand what are the use-cases for
TemplateResponse and why I would want to use a one in a CBV. Some
examples would be helpful.
--
Łukasz Rekucki
Russell Keith-Magee
The use case is to be able to modify the way that content will be
rendered *after* a view has finished processing, instead of the view
producing a block of rendered HTML (or whatever) that response
middlewares and decorators must accept without question. By deferring
the rendering process, you can make changes such as adding items or
changing items in the context, or changing the template that is used
to render the result.
Yes, you could get the same result by introducing an additional mixin
to a generic view. However, a TemplateResponse allows you to implement
the functionality as a decorator, rather than subclassing; and it can
be used on all views, not just class-based views.
Yours,
Russ Magee %-)
Russell Keith-Magee
I would have thought so. Is there a compelling reason why CBV's
shouldn't return a TemplateResponse?
Yours,
Russ Magee %-)
SmileyChris
gotchas that I can think of off the top of my head:
1. You need to explicitly bake the response if you are testing using
assertContains
2. You need to explicitly bake the response before the
contrib.messages middleware
On Oct 23, 1:32 am, Russell Keith-Magee <russ...@keith-magee.com>
wrote:
Mikhail Korobov
I don't see anything harmful neither in
django.contrib.messages.middleware.MessageMiddleware nor in
django.test.testcases.assertContains.
Messages middleware passes response as-is and assertContains reads
'content' attribute and thus forces the baking.
at lest the following test case forks fine for me:
class AssertTestCase(TestCase):
def test_assert_contains(self):
request = RequestFactory().get('/')
template = Template('foo')
response = TemplateResponse(request, template)
self.assertContains(response, 'oo')
Can you please provide more details?
Mikhail Korobov
> On 22 October 2010 03:59, Russell Keith-Magee <russ...@keith-magee.com> wrote:
> > 2010/10/21 Łukasz Rekucki <lreku...@gmail.com>:
> >> annoying flaw:http://code.djangoproject.com/ticket/12669. Please add
> >> a "response_class" keyword to your render() function ;). Thanks!
>
> > Is this addressed by the status_code argument to TemplateResponse? i.e.,
>
> > return TemplateResponse(request, template, context, status_code=403)
>
> > We could also include some constants to make things a little prettier:
>
> > return TemplateResponse(request, template, context, status_code=FORBIDDEN)
>
> > Would that satisfy your use case for #12669?
>
> Yes, it does, but constants are a must have, imho.
>
are already available:
>>> import httplib
>>> httplib.FORBIDDEN
403
Mikhail Korobov
I've done some research on generic views integration and think that
TemplateResponseMixin should be refactored in order to use
TemplateResponse (SimpleTemplateResponse actually) because it
currently assumes that template must be rendered before the response
is returned.
'render_template' hook doesn't fit lazy-rendered responses and
'get_response' == 'render_to_response' if there is no
'render_template' hook. I propose to eliminate 'get_response' and
'render_template' hooks. These hooks are not used by django itself.
get_response's purpose was to make it easy to override response class.
Because render_to_response now returns response itself it is natural
to override render_to_response instead of get_response.
render_template's purpose was to change a way template is rendered. I
don't know what is the exact use case. If one wants to use e.g. jinja2
template engine then he should override get_template method so it will
return jinja.Template instead of django.Template (both have a 'render'
method).
Am I understand their purposes properly?
Anyway, one can override render_to_response hook and return any
HttpResponse subclass using any template rendering logic.
Here is a chart to make things clear:
http://www.lucidchart.com/publicSegments/view/4cc313b1-9478-4985-9870-2a190afcbe04
Some other gotchas:
- TemplateResponseMixin.render_to_response should possibly be renamed
to 'TemplateResponseMixin.render' for consistency.
- django.test.TestCase.assertTemplateUsed should possibly bake the
response explicitly because TemplateResponses are lazy.
I need core developers' blessing.
SmileyChris
back to work I'll have a look to see what the actual cases are.
Regarding the messages middleware, I *know* there's a problem. A
message won't be marked as "read", since the template hasn't iterated
the messages object by the time the middleware is triggered
Mikhail Korobov
return response as-is. I'll take a look.
As for tests, response.context and response.templates are not
available for TemplateResponse instances before they are baked so test
client should be patched to explicitly bake the response. There is
response.template_context and response.template_name but their
semantics differ.
Mikhail Korobov
middleware fixes): http://bitbucket.org/kmike/django/overview
Yet another gotchas:
- response.template_name for generic views will contain Template
instance, not template names, so response.template_name is quite
misleading. The better name ('template') is already taken by test
client (but it is deprecated in 1.3). I can't find a good solution so
leave the 'template_name' for now.
- hasattr(response, 'bake') and callable(response.bake) checks are
ugly. The alternative is to provide HttpResponse.bake method but this
way HttpResponse will be aware of TemplateResponse and it doesn't seem
clean for me.
Mikhail Korobov
I came up with even more naive implementation of
TemplateResponseMixin: http://bitbucket.org/kmike/django/src/a3e242ca7b4b/django/views/generic/base.py#cl-87
response.template_name will contain a list of names and there is
(almost) no code duplication between TemplateResponse and
TemplateResponseMixin with this implementation. Custom template
loading and context instantiation go to TemplateResponse subclasses.
Russell Keith-Magee
> Sorry for massive email spam on this list :)
>
> I came up with even more naive implementation of
> TemplateResponseMixin: http://bitbucket.org/kmike/django/src/a3e242ca7b4b/django/views/generic/base.py#cl-87
>
> response.template_name will contain a list of names and there is
> (almost) no code duplication between TemplateResponse and
> TemplateResponseMixin with this implementation. Custom template
> loading and context instantiation go to TemplateResponse subclasses.
This is starting to look good to me; here are some comments, going
back a couple of messages:
* Yes, you've got the right idea with regards to the role played by
the various TemplateResponseMixin methods
* It seems reasonable to me that assertTemplateUsed would require
some baking, and yes, that should be happening at the test client
before template rendering signals are disconnected.
* The problem with messages is a big one -- probably even a
show-stopper if we can't find a way to reconcile the general use case
that it represents (i.e., we don't just need a fix for
contrib.messages -- we need to explain how/why the problem exists,
and provide a consistent approach for analogous problems)
* Following the convention of the rest of the API, the call to
get_template_names() should be internal to get_response(), not passed
in as an argument.
* I'm not entirely convinced that get_response() is needed now. As
your implementation currently stands, render_to_response() is just a
call to get_response() -- which suggests that the extra level of
indirection isn't needed.
Backing up this position -- most of the flexibility that
TemplateResponseMixin has is to enable the easy integration of
different rendering engines and contexts; those API points are now
provided by TemplateResponse, so there isn't any need to preserve them
in the mixin. If you want to use a different loader, template
renderer, context instance, etc, you subclass TemplateResponse.
So - revised source code:
class TemplateResponseMixin(object):
"""
A mixin that can be used to render a template.
"""
template_name = None
template_response_class = TemplateResponse
def render_to_response(self, context):
"""
Returns a response with a template rendered with the given context.
"""
return self.template_response_class(
request=self.request,
template=self.get_template_names(),
context=context,
**response_kwargs
)
def get_template_names(self):
"""
Returns a list of template names to be used for the request. Must return
a list. May not be called if render_to_response is overridden.
"""
if self.template_name is None:
return []
else:
return [self.template_name]
However, all this is a moot point if we can't find a fix for the
contrib.messages problem.
Yours,
Russ Magee %-)
Mikhail Korobov
that should happen on template rendering (iteration over the messages
in this case) and don't access response content directly.
I was about to introduce 'BakingMiddleware' - the middleware that
bakes the response explicitly. It can be a border between middlewares
with rendered templates as requirement and middlewares without this
requirement. But we can't inject this middleware in backwards-
compatible way to user's code so I just fixed the contrib.messages
middleware. I still like the idea but don't know if we can afford 'add
BakingMiddleware before the MessagesMiddleware' note in upgrade docs.
This is all quite similar to problems django have with streaming http
responses (see
http://groups.google.com/group/django-developers/browse_thread/thread/9dc1bb93eed77987/c61c1b8d5426c1cb?lnk=gst&q=http+content#c61c1b8d5426c1cb)
and maybe there are some ideas from that thread which may be useful.
On 25 окт, 19:33, Russell Keith-Magee <russ...@keith-magee.com> wrote:
> 2010/10/25 Mikhail Korobov <kmik...@googlemail.com>:
> > Sorry for massive email spam on this list :)
>
> > I came up with even more naive implementation of
Mikhail Korobov
1. Introduce the BakingMiddleware
(django.template.response.BakingMiddleware or
django.template.middleware.BakingMiddleware?). This middleware bakes
the response using .bake() method.
2. Put this middleware as last middleware in default settings.py (with
a blank line above).
3. The ugly bit: if there is no BakingMiddleware in
settings.MIDDLEWARE_CLASSES then assume that it is the last
middleware.
django.core.handlers.base.BaseHandler should be patched for that.
Warning may also be emitted if there is no explicit BakingMiddleware
in settings.py so that this ugly bit can be removed in future.
4. raise ImproperlyConfigured exception if unbaked response is
received by messages middleware
This way changes will be backwards-compatible on middleware level
because middlewares will only receive baked responses by default. No
existing middlewares make use of TemplateResponse and things will work
as usual with default setup.
Cool new response middlewares that makes use of TemplateResponse (e.g.
do some caching or paginating on context objects) can be put after the
BakingMiddleware so they will be able mess with an unbaked response if
it is available.
Implementation (no tests and docs): http://bitbucket.org/kmike/django/changeset/68571aa0e5a3
On 25 окт, 20:36, Mikhail Korobov <kmik...@googlemail.com> wrote:
> contrib.messages middleware was broken because it relies on something
> that should happen on template rendering (iteration over the messages
> in this case) and don't access response content directly.
>
> I was about to introduce 'BakingMiddleware' - the middleware that
> bakes the response explicitly. It can be a border between middlewares
> with rendered templates as requirement and middlewares without this
> requirement. But we can't inject this middleware in backwards-
> compatible way to user's code so I just fixed the contrib.messages
> middleware. I still like the idea but don't know if we can afford 'add
> BakingMiddleware before the MessagesMiddleware' note in upgrade docs.
>
> This is all quite similar to problems django have with streaming http
Ivan Sagalaev
> * The problem with messages is a big one -- probably even a
> show-stopper if we can't find a way to reconcile the general use case
> that it represents (i.e., we don't just need a fix for
> contrib.messages -- we need to explain how/why the problem exists,
> and provide a consistent approach for analogous problems)
I apologize in advance if I missed some important bits of the
conversation � I'm on vacation right now :-).
Why can't we teach messages middleware to just explicitly bake a
response and call it a proper solution? By adding `force_bake` to the
HttpResponse class itself we effectively declare that *any* HttpResponse
can be lazy and middleware that expects some side-effects from a
response has to bake it.
Sure it must be documented that middleware forcing response baking
should work after the ones that don't.
Mikhail Korobov
Let me explain why I prefer 'border' middleware way (that is
implemented) over explicit baking in messages middleware (that was
implemented but then replaced with 'border' middleware).
1. 'Border' middleware is a backwards-compatible change, the
requirement to bake response in middleware isn't.
2. With BakingMiddleware it is clear what middlewares expect responses
to be baked and what expect responses to be unbaked. This provides
practical benefits. If there is no BakingMiddleware and some
middleware wants to change template or context it must call
'force_bake' (not just 'bake') to make sure the changes will apply. If
there is an another middleware that want to change something (template
or context) then template will be rendered several times. With
BakingMiddleware the contract is a bit different. Template will be
rendered exactly 1 time in this case because middleware can assume
that the response was not baked.
3. The code behind BakingMiddleware should be executed anyway in order
to prevent template rendering outside try-catch statements. The
difference is only that you propose to execute 'bake' in the end of
response cycle and I propose to execute it at the beginning of the
response cycle but to make this customizable (by changing the position
of the BakingMiddleware).
On 27 окт, 13:33, Ivan Sagalaev <man...@softwaremaniacs.org> wrote:
> On 10/25/2010 04:33 PM, Russell Keith-Magee wrote:
>
> > * The problem with messages is a big one -- probably even a
> > show-stopper if we can't find a way to reconcile the general use case
> > that it represents (i.e., we don't just need a fix for
> > contrib.messages -- we need to explain how/why the problem exists,
> > and provide a consistent approach for analogous problems)
>
> I apologize in advance if I missed some important bits of the
Mikhail Korobov
even public 'bake' method) method if BakingMiddleware is implemented.
With BakingMiddleware there is exactly one place where response should
be baked and user's code shouldn't be calling 'force_bake' and even
'bake' on responses.
Ivan Sagalaev
> 1. 'Border' middleware is a backwards-compatible change, the
> requirement to bake response in middleware isn't.
>
> difference is only that you propose to execute 'bake' in the end of
> response cycle and I propose to execute it at the beginning of the
> response cycle but to make this customizable (by changing the position
> of the BakingMiddleware).
I understand your points now, thanks. Two things bother me about
'border' middleware:
- its semantics is a bit different than that of others middleware in the
list in settings and this difference is not explicitly clear when
looking at the list
- it's bad to have a boilerplate code that people just have to put somewhere
I've spent a night with a thought and now I think I can propose even
better solution.
We can introduce a new kind of middleware — "template response
middleware" for lack of a better name. A user who wants to do something
with a template response *before* it is baked has to write a middleware
like this:
class ContextInjectionMiddleware:
def process_template_response(self, request, response):
# do something with response
Request handling code would look like this then:
response = get_response(...)
if hastattr(response, 'force_bake'):
# apply template response middleware
response.force_bake()
# apply normal response middleware
This way we:
- are getting rid of force_bake in HttpResponse where it's a noop
- maintain backward compatibility since response is baked before all
currently written middleware
- require explicitly named method to deal with a new concept
What's not to like? :-)
Mikhail Korobov
The request handling code have to be put into WSGIHandler and into
ModPythonHandler so I'll wait until the patch for http://code.djangoproject.com/ticket/9886
will be landed.
Ivan Sagalaev
> The request handling code have to be put into WSGIHandler and into
> ModPythonHandler so I'll wait until the patch for http://code.djangoproject.com/ticket/9886
> will be landed.
I'd say it's even worth to wait for
http://code.djangoproject.com/ticket/14523 that moves response
middleware application into the base code.
Russell Keith-Magee
I like this idea -- it's is an elegant solution to the problem, and
avoids all the backwards compatibility issues I can think of.
I have two comments:
Firstly, there needs to be a shortcut for non-template responses. If
your response isn't a template response, there's no point putting it
through Template Reponse Middleware.
Secondly, it seems to me like there may be some need for baking
protection here. If any template-response-middleware were to bake the
response, subsequent template-reposnse-middlewares could potentially
have problems, as any changes they make to context etc will be
ignored. Wouldn't it make sense to put a flag on the TemplateResponse
that prohibits accidental baking? That way the force_bake() that
happens between the template-response-middleware and the normal
response-middleware would be the guaranteed point at which the
template is writ large as content.
Regarding #9886 and #14523 -- they're both RFC, and they're on my todo
list of things to commit in the near future. I just need to find a few
spare moments to give the patches a final review and commit.
Yours,
Russ Magee %-)
Mikhail Korobov
Thank you for your reviews and moving things on!
On 29 окт, 07:35, Russell Keith-Magee <russ...@keith-magee.com> wrote:
>
> I like this idea -- it's is an elegant solution to the problem, and
> avoids all the backwards compatibility issues I can think of.
>
> I have two comments:
>
> Firstly, there needs to be a shortcut for non-template responses. If
> your response isn't a template response, there's no point putting it
> through Template Reponse Middleware.
>
response middleware. Whenever the response is a template response is
determined by the presence of 'force_bake' method.
> Secondly, it seems to me like there may be some need for baking
> protection here. If any template-response-middleware were to bake the
> response, subsequent template-reposnse-middlewares could potentially
> have problems, as any changes they make to context etc will be
> ignored. Wouldn't it make sense to put a flag on the TemplateResponse
> that prohibits accidental baking? That way the force_bake() that
> happens between the template-response-middleware and the normal
> response-middleware would be the guaranteed point at which the
> template is writ large as content.
>
They shouldn't bake responses anywhere in their code.
The original TemplateResponse idea was not the same.
Original TemplateResponse was baked on first content access. Now
TemplateResponse should be baked exactly in one place: right after
template response middlewares. Other baking points seems to be error-
prone with this solution.
So maybe it will be better not to make bake/force_bake public so that
users won't be able to shoot themselves in the foot? And maybe it'll
be better even not to bake response magically on first content
access?
> Regarding #9886 and #14523 -- they're both RFC, and they're on my todo
> list of things to commit in the near future. I just need to find a few
> spare moments to give the patches a final review and commit.
>
> Yours,
> Russ Magee %-)
Russell Keith-Magee
> Hi Russel,
>
> Thank you for your reviews and moving things on!
>
> On 29 окт, 07:35, Russell Keith-Magee <russ...@keith-magee.com> wrote:
>>
>> I like this idea -- it's is an elegant solution to the problem, and
>> avoids all the backwards compatibility issues I can think of.
>>
>> I have two comments:
>>
>> Firstly, there needs to be a shortcut for non-template responses. If
>> your response isn't a template response, there's no point putting it
>> through Template Reponse Middleware.
>>
>
> Ivan's code doesn't put non-template responses through template
> response middleware. Whenever the response is a template response is
> determined by the presence of 'force_bake' method.
Ah - I wasn't aware there was a working implementation of this idea --
did I miss a link somewhere?
>> Secondly, it seems to me like there may be some need for baking
>> protection here. If any template-response-middleware were to bake the
>> response, subsequent template-reposnse-middlewares could potentially
>> have problems, as any changes they make to context etc will be
>> ignored. Wouldn't it make sense to put a flag on the TemplateResponse
>> that prohibits accidental baking? That way the force_bake() that
>> happens between the template-response-middleware and the normal
>> response-middleware would be the guaranteed point at which the
>> template is writ large as content.
>>
>
> As I can see, users shouldn't bake responses not only in middlewares.
> They shouldn't bake responses anywhere in their code.
>
> The original TemplateResponse idea was not the same.
Agreed. This is a change from the original completely-lazy-evaluated
TemplateResponse idea, but I think it makes sense in terms of being
explicit and avoiding a wealth of potential bugs in implicit
evaluation.
> So maybe it will be better not to make bake/force_bake public so that
> users won't be able to shoot themselves in the foot? And maybe it'll
> be better even not to bake response magically on first content
> access?
That's essentially what I was suggesting -- that if a middleware or
decorator accidental accessed response.content before the end of
template-context-processor handling (when the explicit baking occurs),
it should raise an exception rather than implicitly baking the
response.
Yours,
Russ Magee %-)
Ivan Sagalaev
>> Wouldn't it make sense to put a flag on the TemplateResponse
>> that prohibits accidental baking?
Mikhail:
> So maybe it will be better not to make bake/force_bake public so that
> users won't be able to shoot themselves in the foot?
I don't think it's doable at all. People still can call any method in as
well as they can ignore or alter any protection flag. I believe it's
sufficient to abide to the Python way here, write a proper documentation
and treat people as grown-ups trusting them not to do bad things in
their process_template_response.
Russell Keith-Magee
I agree that it's important to treat people as grown ups. However,
this is something that is trivial to do by accident -- for example,
printing response.content would be an obvious debug step -- and it
will be a non-trivial thing to identify that this is the cause of your
problems.
Maybe an unconditional exception is a little strong, but some sort of
safety catch seems called for.
Yours,
Russ Magee %-)
Mikhail Korobov
>
> Ah - I wasn't aware there was a working implementation of this idea --
> did I miss a link somewhere?
>
code snippet:
response = get_response(...)
if hastattr(response, 'force_bake'):
# apply template response middleware
response.force_bake()
# apply normal response middleware
> If any template-response-middleware were to bake the
> response, subsequent template-reposnse-middlewares could potentially
> have problems, as any changes they make to context etc will be
> ignored.
the template will be rendered twice.
Ivan Sagalaev
> I agree that it's important to treat people as grown ups. However,
> this is something that is trivial to do by accident -- for example,
> printing response.content would be an obvious debug step -- and it
> will be a non-trivial thing to identify that this is the cause of your
> problems.
Aha, I see the point now. On a second thought I think we can avoid this
problem altogether by not passing actual response object into
middleware. Instead we could pass just those bits that a middleware
should care about: a template name and a context instance. The
middleware then may (or even must) return new values for those that
would be placed back into the response by the request handler.
Something still bothers me about it, but I can't invent any real
objections myself.
Carl Meyer
> Aha, I see the point now. On a second thought I think we can avoid this
> problem altogether by not passing actual response object into
> middleware. Instead we could pass just those bits that a middleware
> should care about: a template name and a context instance. The
> middleware then may (or even must) return new values for those that
> would be placed back into the response by the request handler.
look at some other response data (headers? or simply extra annotation
attributes?) in order to make decisions about what to do.
Carl
Ivan Sagalaev
> Seems that a template-response-middleware might reasonably want to
> look at some other response data (headers? or simply extra annotation
> attributes?) in order to make decisions about what to do.
You right. And it's not feasible (or beautiful) to just rip everything
out of response and pass as local arguments. Back to thinking :-).
Russel and others: do you think the issue of accidental baking is a
show-stopper here?
SmileyChris
On Oct 31, 8:26 am, Ivan Sagalaev <man...@softwaremaniacs.org> wrote:
> Russel and others: do you think the issue of accidental baking is a
> show-stopper here?
property would raise an error until it is explicitly baked. Problem
solved.
Ivan Sagalaev
> It seems a simple enough proposal that trying to access the content
> property would raise an error until it is explicitly baked. Problem
> solved.
True. I seem to somehow missed it between the lines. Thanks! I'm +1 on
it by the way.
Mikhail Korobov
The new patch is attached to ticket (see
http://code.djangoproject.com/attachment/ticket/12816/render_shortcut.6.diff?format=raw
). Docs are cumbersome (and incomplete), and a couple of middleware
tests are missing.
1) Template response middleware is introduced. It is applied only for
response instances that can be baked.
2) Exception is raised on content access or iteration for unbaked
response.
3) 'force_bake' method is removed. 'bake' now behaves like old
'force_bake'.
4) Test client fixes are no longer needed and they are removed.
5) Some docs are added.
Am we in hurry to get 'render' shortcut into 1.3 alpha 1 or it can be
added later?
Mikhail Korobov
ready by 1.3 alpha 1.
> The new patch is attached to ticket (seehttp://code.djangoproject.com/attachment/ticket/12816/render_shortcut...
Ivan Sagalaev
> I'm quite busy now and don't think I'll be able to make the patch
> ready by 1.3 alpha 1.
Full feature freeze is expected only by the time of beta so I don't
think it's absolutely necessary to push it before alpha 1.
Anyway since I care very much for the patch I can pick it up if you
couldn't find a time to maintain it. Just drop me a line off-list in
this case.
Russell Keith-Magee
> Hi all.
>
> The new patch is attached to ticket (see
> http://code.djangoproject.com/attachment/ticket/12816/render_shortcut.6.diff?format=raw
> ). Docs are cumbersome (and incomplete), and a couple of middleware
> tests are missing.
>
> 1) Template response middleware is introduced. It is applied only for
> response instances that can be baked.
> 2) Exception is raised on content access or iteration for unbaked
> response.
> 3) 'force_bake' method is removed. 'bake' now behaves like old
> 'force_bake'.
> 4) Test client fixes are no longer needed and they are removed.
> 5) Some docs are added.
>
> Am we in hurry to get 'render' shortcut into 1.3 alpha 1 or it can be
> added later?
No - this isn't urgent for alpha 1; I'm happy to see it land in time
for the beta.
I've had a quick look at the patch, and it looks pretty good. Thanks
for your work on this, Mikhail.
Yours,
Russ Magee %-)