Wrapping of admin views does not preserve view attributes

[Matt Pryor]

Because of how the wrap function is defined in get_urls for both AdminSite and ModelAdmin, any properties added to the views by the admin_view decorator are not propagated to the actual view that is used.

This can be fixed by tweaking the implementation of the wrap function (note that a similar tweak would need to be made to the wrap function in ModelAdmin):

def custom_decorator(view):
    def wrapper(*args, **kwargs):
        return view(*args, **kwargs)
    wrapper.custom_attribute = 'SOMEVALUE'
    return update_wrapper(wrapper, view)


class CustomAdminSite(AdminSite):
    def admin_view(self, view, cacheable=False):
        # Apply custom decorator to all admin views
        return custom_decorator(super().admin_view(view, cacheable))

    def get_urls(self):
        # Current implementation of wrap
        def wrap(view, cacheable=False):
            def wrapper(*args, **kwargs):
                return self.admin_view(view, cacheable)(*args, **kwargs)
            wrapper.admin_site = self
            return update_wrapper(wrapper, view)

        # This will print False
        print(hasattr(wrap(self.index), 'custom_attribute')

        # Suggested implementation of wrap
        def wrap(view, cacheable=False):
            wrapped = self.admin_view(view, cacheable)
            wrapped.admin_site = self
            return wrapped

        # This will print True
        print(hasattr(wrap(self.index), 'custom_attribute')
        # This will print SOMEVALUE
        print(wrap(self.index).custom_attribute)

        return super().get_urls()

[Matt Pryor]

PR submitted with suggested changes: ​https://github.com/django/django/pull/14038

[Nick Pope]

It is unclear what your use case is for this change as you provide no example of why you'd need to assign a custom attribute to a view.

The current implementation, wrapping the function, is intended to prevent side effects of modifying the original function when assigning the .model_admin or .admin_site attributes.

[Tim McCurrach]

Replying to Nick Pope:

It is unclear what your use case is for this change as you provide no example of why you'd need to assign a custom attribute to a view.

The current implementation, wrapping the function, is intended to prevent side effects of modifying the original function when assigning the .model_admin or .admin_site attributes.

I feel like I may be missing something here.

Since admin_view is itself a decorator, the function that we avoid mutating is the function inner defined in admin_view. Why does it make a difference if we mutate inner (from admin_view) or wrapped from wrap? - neither are the original function!!

I agree, there needs to be some justification to add the proposed capability (and none come to mind immediately). Nonetheless it feels like the proposed PR is a simpler(better?) than what we have currently - and just happens to have the additional upside that you can add custom attributes to the view.

Looking back to the ​original commit where get_urls was added, wrap actually predates the .model_admin and .admin_site attributes:

def wrap(view):
    def wrapper(*args, **kwargs):
        return self.admin_view(view)(*args, **kwargs)
    return update_wrapper(wrapper, view)

I'm slightly lost as to why this was added at all, and why (as is suggested in the docstring) self.admin_view didn't wrap the relevant views directly).

I'm sure it was done this way for a good reason, I just can't see why. I also apologise if this isn't the appropriate place to ask this question, but I'm sure clarity as to why a ticket has been closed will be of use to anyone proposing a similar change in the future.