Opened 7 years ago
Closed 2 weeks ago
#28999 closed Cleanup/optimization (fixed)
Document how to use class-based views if you want to reverse by them by instance
Reported by: | Andrew Standley | Owned by: | Clifford Gama |
---|---|---|---|
Component: | Documentation | Version: | 2.0 |
Severity: | Normal | Keywords: | url reverse cbv |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This is primarily an issue with CBV get_view()
method.
However this affects basically all url reverse resolution as urls.base reverse()
-> urls.resolvers get_resolver()
-> urls.resolvers RegexURLResolver -> utils.datastructures MultiValueDict -> dict() __getitem__()
which uses the key's hash value.
I discovered this while attempting to reverse a class based view. No matter what I tried I could not get a reverse url. Quite a bit of testing and digging later, the issue appears to be that {{{as_view()}} creates a new view function on every call, and every one of these functions has a unique hash. This means that using the result of one call to "as_view" as the key in MultiValueDict results in a value you can not retrieve with a subsequent call.
from testapp.views import MyCBView from django.utils.datastructures import MultiValueDict, MultiValueDictKeyError lookups = MultiValueDict() first_call = MyCBView.as_view() second_call = MyCBView.as_view() # Does Not Work lookups[first_call] = "Test Retrieval" try: test = lookups[second_call] except MultiValueDictKeyError: print("Lookup Failed {} != {}".format(first_call.__hash__(), second_call.__hash__())) # Works test = lookups[first_call] print("Lookup Succeeded test={}".format(test))
I am fairly certain that it is not intended (and certainly not documented) that you must store the return of "as_view", and use that stored value in your urlconf, if you ever want to be able to reverse using a CBV instance.
Change History (16)
comment:1 by , 7 years ago
follow-up: 4 comment:2 by , 7 years ago
I think it may be possible to fix by simply caching the view function when it is created and from then on returning the cache, but I haven't yet looked into how this would effect the initwargs.
Hmm, I'm not sure how to best describe my preference. It's part of a larger experimentation with the framework.
I'm trying to experiment with using get_absolute_url
on my models, and I have CBV DetailView classes for my models. Firstly it seems round-about to use a URL name that represents a url->view mapping when I know the view I want. The View Class for a given Model is a core relationship that should never change, if someone uses my Models they should want to use my Views for those model. The URL name "should" not change, but it is perfectly reasonable someone may want to use my models and my views without using my urlconf. The url schema (and thus URL name) could be considered a separate matter to the models/views so I would like to de-couple these things in the code for my Models/Views.
I hope that makes sense?
Secondly I would ideally like to find a way to create a 'reverse' method that finds the url regardless of the namespacing. (So a user of my app would not have to update my models.py if they wanted to include my urlconf under a namespace). Obviously this won't work with URL names, but since every function should have a unique hash, I believe I can make it work using view functions... But that is currently impossible for CBV because of this issue.
I'm probably trying to be far to fancy for my own good here, but that is the reason for my preference.
I admit, it is likely that for 99% of people, documenting that it doesn't work would probably be a satisfactory fix.
comment:3 by , 7 years ago
So I think I have a possible fix, but I'm not yet set up for contributing to Django so I haven't run the test suite on it. Preliminary testing appeared to work.
I would be interested in hearing others thoughts before investing more time into this.
@classonlymethod def as_view(cls, **initkwargs): """ Main entry point for a request-response process. """ for key in initkwargs: if key in cls.http_method_names: raise TypeError("You tried to pass in the %s method name as a " "keyword argument to %s(). Don't do that." % (key, cls.__name__)) if not hasattr(cls, key): raise TypeError("%s() received an invalid keyword %r. as_view " "only accepts arguments that are already " "attributes of the class." % (cls.__name__, key)) class callable_view(object): # Instance of callable_view is callable and acts as a view function def __call__(self, request, *args, **kwargs): self = cls(**initkwargs) if hasattr(self, 'get') and not hasattr(self, 'head'): self.head = self.get self.request = request self.args = args self.kwargs = kwargs return self.dispatch(request, *args, **kwargs) def __hash__(self): return hash(cls) # Class' 'view function' hash is defined by the Class def __eq__(self, other): if not hasattr(other, '__hash__'): return False return hash(self) == hash(other) def __repr__(self): return repr(self.__call__.__func__) view = callable_view() view.view_class = cls view.view_initkwargs = initkwargs # take name and docstring from class update_wrapper(view, cls, updated=()) update_wrapper(view.__call__.__func__, cls, updated=()) # and possible attributes set by decorators # like csrf_exempt from dispatch update_wrapper(view.__call__.__func__, cls.dispatch, assigned=()) return view
follow-up: 6 comment:4 by , 7 years ago
Replying to airstandley:
So I think I have a possible fix, but I'm not yet set up for contributing to Django so I haven't run the test suite on it. Preliminary testing appeared to work.
I would be interested in hearing others thoughts before investing more time into this.
<snip>
The callable_view
would compare equal regardless of its initkwargs
, I don't think that's the behaviour we'd want. We can compare the initkwargs as well, but that becomes kinda iffy when more complex arguments don't necessarily compare equal, in which case we get the same problem but in a less obvious way that's harder to debug.
I'd much rather document the pattern of calling .as_view()
once in views.py
and using the result as if it were a function-based view (if that's not yet documented), e.g.:
class MyDetailView(DetailView): ... my_detail_view = MyDetailView.as_view()
Then you can use my_detail_view
as if it were a function-based view, and reversing would work as expected. I think that should solve your usecase as well without requiring changes to the way .as_view()
and reverse()
work.
comment:5 by , 7 years ago
Component: | Generic views → Documentation |
---|---|
Summary: | URL Reverse does not work for CBV (Class Based Views) → Document how to use class-based views if you want to reverse by them by instance |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
I don't think the pattern that Marten described is documented.
comment:6 by , 7 years ago
Replying to Marten Kenbeek:
The
callable_view
would compare equal regardless of itsinitkwargs
, I don't think that's the behaviour we'd want.
<snip>
Alright, thanks for the insight. I was not entirely sure what the intention of the initkwargs
was. If the intent is that a CBV will be used more than once with different initkwargs
then I can not think of any approach with callable_view
that would work.
The latest docs suggest using CBV.as_view()
in the urlconf urlpatterns
. Would you change all of those references or add an addendum stating the downside of that approach and documenting the pattern that Marten described?
comment:7 by , 7 years ago
I would add a note and not change existing examples. I think reversing by instance isn't a common need.
comment:8 by , 3 weeks ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:10 by , 2 weeks ago
Patch needs improvement: | set |
---|
comment:11 by , 2 weeks ago
Patch needs improvement: | unset |
---|
comment:12 by , 2 weeks ago
Patch needs improvement: | set |
---|
comment:13 by , 2 weeks ago
Patch needs improvement: | unset |
---|
comment:14 by , 2 weeks ago
Triage Stage: | Accepted → Ready for checkin |
---|
I'm not sure if that's worth trying to fix (besides documenting that it doesn't work) -- is there a reason your prefer reversing that way as opposed to using a URL name?