> 3) Add the decorators when you call 'instantiator' (which, by the
> way, I would rather call 'make_view' or something more specific).
> You would then have, *in views.py*:
>
> 3a) my_view = some_decorator(make_view(MyViewClass))
>
> or possibly (though I don't think this adds much):
>
> 3b) my_view = make_view(MyViewClass,
> decorators=[some_decorator])
>
I had some more thoughts about this. One problem with applying the
decorators after is if someone is re-using the class (e.g.
subclassing), but forgets to apply some *necessary* decorators when
creating the view from it. One work-around is that 'make_view' checks
the class itself for a 'decorators' attribute, and applies them:
def make_view(viewclass):
def view(request, *args, **kwargs):
obj = viewclass()
return obj(request, *args, **kwargs)
for f in getattr(viewclass, 'decorators', []):
view = f(view)
return view
This would make it harder to forget, but would still allow subclasses
to adjust the decorators that are used by default. It won't help in
the case where people use the class directly in the URLconf, but I
don't think that should be encouraged anyway.
Luke
--
"Oh, look. I appear to be lying at the bottom of a very deep, dark
hole. That seems a familiar concept. What does it remind me of? Ah,
I remember. Life." (Marvin the paranoid android)
Luke Plant || http://lukeplant.me.uk/
We're working on this with Ben during djangocon.eu sprint [1]. Any comment appreciated, still a work in progress though.
Regards,
David
[1] http://github.com/bfirsh/django-class-based-views
Looks cool. I think Jari's idea of a class method to instantiate the
classes would be a great pattern to establish - something like an
'as_view' classmethod on the base class which returns a view function
that uses the class:
@classmethod
def as_view(cls, *initargs, **initkwargs):
def view(*args, **kwargs):
obj = cls(*initargs, **initkwargs)
return obj(*args, **kwargs)
return view
The developer can choose whether to write 'MyClassView.as_view()'
directly into the URLconf, or put a line into views.py like 'my_view =
MyClassView.as_view()'
This will solve the thread safety issue, and doesn't require another
import for a decorator, as Jari pointed out.
We could decide whether to apply decorators inside as_view() or
__call__(). The argument for putting it in 'as_view' is that __call__
does some processing that you might want to skip entirely (e.g. the
get_object() call - if we applied a 'cache_page' decorator, we
wouldn't want the get_object() call to be made at all if there was a
cache hit).
On that point, I think 'GenericView' should be split into 'ClassView'
and 'GenericView'. The get_object() call is probably not a good thing
to have on a base class.
Regards,
We discussed that point with Jacob and it looks like it's easier to modify urlresolvers to create a new instance of the class when we detect it's a class, this should solve the thread safety issue. Do you see any drawback to this approach?
That way we can declare decorators via a decorators' list [1] or directly in the class [2]. I'm not fond of the second way, decorating a decorator, but we can create decorators dedicated to class based views easily that way.
>
> On that point, I think 'GenericView' should be split into 'ClassView'
> and 'GenericView'. The get_object() call is probably not a good thing
> to have on a base class.
Agreed, I'll make that change.
Thanks,
David
[1] http://github.com/bfirsh/django-class-based-views/blob/master/class_based_views/tests/views.py#L24
[2] http://github.com/bfirsh/django-class-based-views/blob/master/class_based_views/tests/views.py#L43
> > This will solve the thread safety issue, and doesn't require
> > another import for a decorator, as Jari pointed out.
> >
> > We could decide whether to apply decorators inside as_view() or
> > __call__(). The argument for putting it in 'as_view' is that
> > __call__ does some processing that you might want to skip
> > entirely (e.g. the get_object() call - if we applied a
> > 'cache_page' decorator, we wouldn't want the get_object() call
> > to be made at all if there was a cache hit).
>
> We discussed that point with Jacob and it looks like it's easier to
> modify urlresolvers to create a new instance of the class when we
> detect it's a class, this should solve the thread safety issue. Do
> you see any drawback to this approach?
I'm sure that will work. My main reluctance is using isinstance()
when it isn't necessary, and having to change the definition of what a
'view' is when it isn't strictly necessary. (A view is no longer
simply "a callable that takes a request and returns a response"). I
guess this is partly about aesthetics, and I know practicality beats
purity, but:
There might also be the following practical disadvantages when it
comes to the API we encourage.
If the recommended usage is to do:
urlpatterns = ...
(r'something/', MyClassView)
...
then it would be easy to accidentally do `MyClassView()`, and it would
appear to work fine, until you later get thread safety issues. Also,
if you want to pass arguments to the constructor, you would most
naturally do `MyClassView(somearg=something)`, and again it would
appear to work.
If, however, we encouraged `MyClassView.as_view()` from the start, we
can cope with constructor arguments more easily - it changes to
`MyClassView.as_view(somearg=something)`.
(Either way, it would still be possible to pass in MyClassView() and
get thread safety problems, but it's about making mistakes less
natural).
Of course, if we want to use dotted path syntax in URLconfs -
"path.to.MyClassView" - rather than imports, then we would *need* to
have the isinstance() check to be able to cope with it at all. My
response to that is: my preference would be that the use of class
based views should remain an implementation detail, and as far as the
URLconf is concerned, views.py should be exporting a ready-to-use
callable not the class. (The class is exported for the purpose of
other people subclassing, or passing their own constructor arguments,
not for direct use in the URLconf).
There is also the issue of what to do when you need to add a decorator
to someone else's view function. As a developer, I shouldn't have to
know that class based views even *exist*, let alone how to use them,
if I'm just a client - I should *always* be able to say:
from someapp.views import some_view_function
my_view_function = login_required(some_view_function)
But if someapp.views has only exported SomeClassView and not
some_view_function, I now have to do something different.
Given that, for many Django apps, the view functions are part of the
public API that needs to be exported (i.e. hooked into people's
URLconfs or wrapped for re-use), I think it would be good to encourage
practices which will lead to stable and consistent APIs, and so *not*
allow or encourage classes to be used directly in the URLconf.
We're currently working towards URLpatterns of the form::
('...', SomeView)
i.e. passing a class directly, which would be instantiated implicitly
then called.
The reason we're trying it this way is thread safety: if a view is a
class, then people *will* store state on self. So if we have single
instances in views like we do now, then we're giving folks enough rope
to hang themselves. By having each request get a fresh instance we
avoid that problem.
The other alternative we discussed was having the base view class do
something with threadlocals to make sure state saved on self is
threadsafe. I vetoed that because I hate threadlocals :)
On Fri, May 28, 2010 at 3:41 PM, Luke Plant <L.Pla...@cantab.net> wrote:
> I'm sure that will work. My main reluctance is using isinstance()
> when it isn't necessary, and having to change the definition of what a
> 'view' is when it isn't strictly necessary. (A view is no longer
> simply "a callable that takes a request and returns a response").
I *am* a bit worried by this -- I'm not sure redefining the definition
of "view" is such a good idea. "A view is a callable or a class with
callable instances" isn't nearly as simple and nice a recommendation.
> If, however, we encouraged `MyClassView.as_view()` from the start, we
> can cope with constructor arguments more easily - it changes to
> `MyClassView.as_view(somearg=something)`.
My only real objection here is that `as_view` is just a bunch of boilerplate::
urlpatterns = patterns('',
('one/', SomeView.as_view()),
('two/', SomeOtherView.as_view()),
('three', YourView.as_view())
)
I just really don't like the way that looks.
As for accepting extra __init__ arguments: I specifically *don't* want
to allow this (at least easily) because I want to encourage
subclassing as the "correct" way of extending views. Yes, this means a
slight loss of flexibility, but it also means that we're forcing
people into a more extensible and fuure-proof way of coding, and
that's a good thing.
> (Either way, it would still be possible to pass in MyClassView() and
> get thread safety problems, but it's about making mistakes less
> natural).
... and either way it's pretty easy to issue a warning if you've
subclassed ``django.views.View`` (my proposed name for the view base
class) and re-used it for multiple requests.
> There is also the issue of what to do when you need to add a decorator
> to someone else's view function.
Again, I want to encourge "subclass it" as the correct answer here.
> Given that, for many Django apps, the view functions are part of the
> public API that needs to be exported (i.e. hooked into people's
> URLconfs or wrapped for re-use), I think it would be good to encourage
> practices which will lead to stable and consistent APIs, and so *not*
> allow or encourage classes to be used directly in the URLconf.
I think we're in agreement here -- we're both trying to avoid view-ish
stuff in the URLconfs (a la the old way we used to do generic views
with dicts in urls.py).
Having played with it for a few years now, though, I'm finding
subclassing is really the best way to make this happen.
So reusable apps instead of exporting functional views export
class-based ones; users who want to re-use them import, subclass, and
extend.
Jacob
Storing state on self makes things a heck of a lot easier. We are going to create a "View" and a "ResourceView". A View just renders a template, but a ResourceView has some kind of resource attached to it (model instance, QuerySet, whatever). On View, this would be the get_context() signature (where args and kwargs are from the URL):
def get_context(self, request, *args, **kwargs):
....
When we create ResourceView, we need to pass the resource in here somehow, interfering with args and kwargs:
def get_context(self, request, resource, *args, **kwargs):
....
We would have to redefine all the methods which require the resource. It seems neater to store it on self.
We have solved accidentally instantiating the view by raising an exception if a View instance is called twice. Of course, people who understand the threading issues are free to create their own class-based views.
I like this method because of its transparency. I fear having to explain as_view() in the documentation. Thread problems are hard to understand.
Ben
> My only real objection here is that `as_view` is just a bunch of
> boilerplate::
>
> urlpatterns = patterns('',
> ('one/', SomeView.as_view()),
> ('two/', SomeOtherView.as_view()),
> ('three', YourView.as_view())
> )
>
> I just really don't like the way that looks.
Agreed. I also think that if you have a mixture of normal views and
class based view, this is ugly:
urlpatterns = patterns('app.views',
('one/', 'some_view_function',
('two/', SomeOtherView),
('three/', YourView)
)
and it would be nicer to spell it:
urlpatterns = patterns('app.views',
('one/', 'some_view_function'),
('two/', 'some_other_view'),
('three/', 'your_view')
)
and have these additional lines in 'app.views':
some_other_view = SomeOtherView.as_view()
your_view = YourView.as_view()
I know that is just moving the boilerplate around, but it is giving a
consistent interface. If some code in a re-usable app moves from
normal views to class based views, they will have to do something like
this *anyway* for backwards compatibility.
But I can see that if subclassing is the default way of re-using, then
exporting these functions gives multiple options about how they should
be re-used, which you are wanting to avoid.
> > There is also the issue of what to do when you need to add a
> > decorator to someone else's view function.
>
> Again, I want to encourge "subclass it" as the correct answer here.
In that case, I guess it would be good to make this hard to do wrong,
by providing helpers that add the decorator to the right end of the
list of decorators.
But don't worry, we've changed our minds again! If __call__() creates a copy of self and calls methods on the copy, as far as I can see it solves all our problems. No boilerplate, and it's really hard to make it unsafe thread-wise.
An example of how it could work:
http://github.com/bfirsh/django-class-based-views/blob/master/class_based_views/base.py
Thanks
Ben
You mean:
class View(HttpResponse):
...
One reason against this is it makes it harder to re-use other views
from within the View. You are forced to mutate the 'self' instance of
HttpResponse (or else use some nasty hack), rather than being able to
simply return the HttpResponse that might be returned from a utility
function or a sub-view.
This is an interesting approach. The only downside I can think of at
the moment is that implementing __new__() like that really violates
expectations - Python programmers have a fairly strong expectation
that if you do 'x = SomeClass()', where SomeClass is defined as a
class, then x will be an instance of SomeClass.
> On Tuesday 01 June 2010 11:43:30 henning....@gmail.com wrote:
>> On May 30, 7:24 am, Waldemar Kornewald <wkornew...@gmail.com> wrote:
>>> Maybe I missed something, but why don't you use __new__ instead
>>> of copying the instance?
>>
>> Here is an example where I used __new__ for class based views in my
>> project:
>> http://djangosnippets.org/snippets/2046/
>> No __call__ or as_view is needed with this approach.
>> I can easily replace a function with a class without changing the
>> url configuration.
>
> This is an interesting approach. The only downside I can think of at
> the moment is that implementing __new__() like that really violates
> expectations - Python programmers have a fairly strong expectation
> that if you do 'x = SomeClass()', where SomeClass is defined as a
> class, then x will be an instance of SomeClass.
Yeah, this idea came up at the sprints, but it's a little too magic for my tastes.
Ben
Hi :-)
I'm not really sure about that approach. As with __new__, __call__
comes (at least for me) with a bunch of expectations. For me it
executes and operates on one single instance of an object. Sure, this
could be interpreted as using the instance as a factory for usually
you'd use class methods for something like that, but still.
On the other hand I can definitely see the merit in it since you could
then easily programmatically modify the "view" factory by just
modifying "yet another object".
-- Horst
One advantage of using __call__ over __new__ is passing arguments to __init__. That's handy for lazily configuring views:
(r'', DetailView(queryset=Thing.objects.all())
Ben
I thought we said we really wanted to encourage using subclassing to
solve these problems (providing many types of args in urls.py can
cause problems).
Alex
--
"I disapprove of what you say, but I will defend to the death your
right to say it." -- Voltaire
"The people's good is the highest law." -- Cicero
"Code can always be simpler than you think, but never as simple as you
want" -- Me
Why can't you use this instead:
(r'', 'views.DetailView', {'queryset': Thing.object.all()})
Bye,
Waldemar Kornewald
> > One reason against this is it makes it harder to re-use other
> > views from within the View.
>
> I don't really understand what you mean. Do you mean re-using
> 'good' old function-views?
Yes, or new ones, or any callable that returns an HttpResponse that I
want to return. It is quite a common pattern to have one view wrap
another or *several* others, and also to use standard response objects
in some utility function or 'sub view'. e.g.
def my_view(request, *args):
if some_condition():
return some_view(request, *args)
elif other_condition():
return some_standard_response(request.user)
else:
...
return render_to_response(...)
def some_standard_response(user)
return HttpResponseRedirect(...)
This would be massively uglified, at best, if we make View a subclass
of HttpResponse.
Regards,
Uhm... guys,
Maybe something simpler?
---- 8< ----
from threading import local, Thread
class View(object):
_child = local()
def __new__(cls, *args, **kwargs):
existing = hasattr(cls._child, 'instance')
if not existing:
print 'first time in this thread'
cls._child.instance = super(View, cls).__new__(cls, *args, **kwargs)
return cls._child.instance
def __call__(self, foo):
print 'call', id(self), foo
return 'bar'
test = View()
test(1)
test = View()
test(2)
test = View()
test(3)
class TestThread(Thread):
def run(self):
test = View()
test(4)
t = TestThread()
t.setDaemon(True)
t.start()
t.join()
test = View()
test(5)
---- 8< ----
Try it now, tests itself.
--
Patryk Zawadzki
Actually you might want to have a couple of instances with different
__init__ params:
---- 8< ----
from threading import local, Thread
class View(object):
_child = local()
def __new__(cls, *args, **kwargs):
if not hasattr(cls._child, 'dict'):
cls._child.dict = {}
param_hash = (args, tuple(kwargs.items()))
existing = cls._child.dict.get(param_hash, None)
if not existing:
print 'first time in this thread with args', param_hash
cls._child.dict[param_hash] = super(View, cls).__new__(cls)
return cls._child.dict[param_hash]
def __call__(self, foo):
print 'call', id(self), foo
return 'bar'
--
Patryk Zawadzki
The supposed View class is a pure utility class that is close to a
module. It does not represent a state in an application, it takes
input and returns output, none of it should outlive a "__call__" call.
Therefore I see no reason for a proper implementation to not be
thread-safe. Just treat "self" as immutable past "__init__". In other
words, don't treat "self" as a junkyard for temporary variables.
This is WRONG:
class View(object):
def greet(self):
return HttpResponse('Hi %s' % self.request.POST.get('hello', ''))
def __call__(self, request):
self.request = request
return greet(self)
This is CORRECT and not any harder to design/implement:
class View(object):
def greet(self, request):
return HttpResponse('Hi %s' % request.POST.get('hello', ''))
def __call__(self, request):
return greet(self, request)
I really don't think we should add anti-moron filters at the framework
level past documenting "taking care to ensire immutability of view
instances".
A true moron will always outsmart us with creativity. It's perfectly
possible to write thread-unsafe code without using classes:
req = None
def greet(self, request):
global req
req = request
# ...
return HttpResponse('Hi %s' % req.POST.get('hello', ''))
or even:
def greet(self, request):
greet.foo = request
# ...
return HttpResponse('Hi %s' % greet.foo.POST.get('hello', ''))
:)
--
Patryk Zawadzki
On Tue, Jun 15, 2010 at 3:38 PM, Patryk Zawadzki <pat...@pld-linux.org> wrote:
> I really don't think we should add anti-moron filters at the framework
> level past documenting "taking care to ensire immutability of view
> instances".
This came out a bit harsh. What I mean is: teach users how to write
thread-safe views (classes or not) and ignore those who refuse to do
it. They either know what they're doing or there is really no point in
forcing the help upon them.
--
Patryk Zawadzki
--
Patryk Zawadzki
I consider using self to store temporary variables a bad design
practice. You wouldn't do that in C++ or Java. Having said that I
might be biased.
If you really want to do such hacks, use threading.local instead of
writing to self directly:
class View(object):
storage = threading.local()
def __call__(self, request, id):
self.storage.request = request
# ...
Cloning objects on each call to a method is a really really really
really bad idea and puts unnecessary stress on the garbage collector
(actually if any of the methods are careless enough to pass self to an
external function, it might result in a steady leak of non-freeable
memory).
--
Patryk Zawadzki
The request is passed round so methods look like views to decorators. Magically dropping it for decorators seems a bit scary. :/
Ben
Is there any reason why all of you think not passing request around is a gain?
Would you recommend anyone to do:
---- 8< ----
request = None
def my_view():
global request
return HttpResponse('Hi, %s' % (request.user.get_full_name(), ))
def my_other_view():
globl request
return HttpResponse('Hello again, %s' % (request.user.get_full_name(), ))
# ...
---- 8< ----
As I seriously see no difference between this and pushing stuff to self.request.
Request objects are esoteric. They are only meaningful during that
short period of time you spend in the __call__ method. Once you return
a HttpResponse, request is ready to retire. Storing them at the level
objects grants them potentially eternal life for no reason.
Consider this:
---- 8< ----
class AwesomeAdd(object):
def __call__(self, number1, nubmer2):
self.number1 = number1
self.number2 = number2
return self.do_the_math()
def do_the_math(self):
# OMG BBQ, we cleverly avoided passing params around!!1!one
return self.number1 + self.number2
aa = AwesomeAdd()
foo = aa(3, 5)
---- 8< ----
Doesn't that scream "WTF"? :)
--
Patryk Zawadzki
Would you seriously recommend people to extend "int" when they want to
implement multiplication? :)
--
Patryk Zawadzki
The __new__-based approach is just lovely. :) Really, it's much more
elegant than copying self and it's very convenient to be able to
access self.request and the configuration options from anywhere. This
solves a lot of problems when you want to customize a view's behavior.
We really missed this when we used Django's class-based Feed view.
Some minor issues:
The __init__ function isn't called with *args, **kwargs (at least, I
think this was an oversight).
FormView.POST should probably use self.get_content like View.GET does
by default.
Should there be different methods for each request.method, at all?
Often form POST handling code can reuse the GET part if form
validation fails. By splitting everything up in two methods you blow
up the code unnecessarily. BTW, I think the current code would
unnecessarily instantiate the form two times if the form doesn't
validate.
Also, _load_config_values should guarantee that you don't pass
unsupported arguments. This should also work with inheritance.
Bye,
Waldemar Kornewald
Please take a deeper look at his code. He doesn't use __init__. He
uses __new__, so each request gets his own View instance.
Instead of
aa = AwesomeAdd()
foo = aa(3, 5)
the __new__-based approach allows to do this:
foo = AwesomeAdd(3, 5)
IOW, the "constructor" directly returns an HttpResponse (foo) instead
of an AwesomeAdd instance.
Bye,
Waldemar Kornewald
There is no difference between using __init__ and __new__ as long as
you treat the instance as a junkyard for temporary variables. If done
properly, class-based views do _not_ need a separate instance for each
request.
Here's an example of a thread-safe view that works happily with just
one instance:
---- 8< ----
class MyView(object):
def __init__(self, queryset, template='bar.html'):
self.qs = queryset
self.template = template
def __call__(self, request, id):
instance = get_object_or_404(self.qs, id=id)
return direct_to_template(request, self.template, {'object': instance})
# ...
url(r'^foo/(?P<id>\d+)/', MyView(queryset=Foo.objects.all(),
template='foo.html')),
---- 8< ----
--
Patryk Zawadzki