Opened 7 years ago

Closed 3 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 Tim Graham, 7 years ago

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?

comment:2 by Andrew Standley, 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 (an thus URL name) could be considered a separate matter to the models/views so I would like to de-couple these things in code for my Models/Views.

I hope that makes sense?

Secondly I would ideally like to find a way to create a 'reverse' that find 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.

Version 0, edited 7 years ago by Andrew Standley (next)

comment:3 by Andrew Standley, 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
Last edited 7 years ago by Andrew Standley (previous) (diff)

in reply to:  2 ; comment:4 by Marten Kenbeek, 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 Tim Graham, 7 years ago

Component: Generic viewsDocumentation
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: UnreviewedAccepted
Type: BugCleanup/optimization

I don't think the pattern that Marten described is documented.

in reply to:  4 comment:6 by Andrew Standley, 7 years ago

Replying to Marten Kenbeek:

The callable_view would compare equal regardless of its initkwargs, 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 Tim Graham, 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 Clifford Gama, 5 weeks ago

Owner: changed from nobody to Clifford Gama
Status: newassigned

comment:9 by Clifford Gama, 4 weeks ago

Has patch: set

comment:10 by Sarah Boyce, 3 weeks ago

Patch needs improvement: set

comment:11 by Clifford Gama, 3 weeks ago

Patch needs improvement: unset

comment:12 by Sarah Boyce, 3 weeks ago

Patch needs improvement: set

comment:13 by Clifford Gama, 3 weeks ago

Patch needs improvement: unset

comment:14 by Sarah Boyce, 3 weeks ago

Triage Stage: AcceptedReady for checkin

comment:15 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

In be138f32:

Refs #28999 -- Added tests for reversing a class-based view by instance.

comment:16 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In 4d11ea1e:

Fixed #28999 -- Documented how to reverse a class-based view by instance.

Co-authored-by: Sarah Boyce <42296566+sarahboyce@…>

Note: See TracTickets for help on using tickets.
Back to Top