[Django] #32477: Wrapping of admin views does not preserve view attributes
Django
-----------------------------------------+------------------------
Reporter: Matt Pryor | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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`):
{{{
#!python
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()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32477>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Matt Pryor | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matt Pryor):
PR submitted with suggested changes:
https://github.com/django/django/pull/14038
--
Ticket URL: <https://code.djangoproject.com/ticket/32477#comment:1>
Django
Reporter: Matt Pryor | Owner: nobody
Component: contrib.admin | Version: 3.1
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nick Pope):
* status: new => closed
* resolution: => needsinfo
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32477#comment:2>
Django
Reporter: Matt Pryor | Owner: nobody
Component: contrib.admin | Version: 3.1
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim McCurrach):
Replying to [comment:2 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 [https://github.com/django/django/commit/1f84630c87
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32477#comment:3>