Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Worked around method_decorator problem with custom getattr #1820

Merged
merged 1 commit into from Dec 4, 2013
Merged

Worked around method_decorator problem with custom getattr #1820

merged 1 commit into from Dec 4, 2013

Conversation

dpwrussell
Copy link
Member

When using django's method_decorator on a Class Based View, decorators with arguments will fail because the method_decorator blindly tries to copy an attribute that an instance does not have, __name__. This is really something that django need to sort out (https://code.djangoproject.com/ticket/13879), so we can probably undo it this the future.

Resolves https://trac.openmicroscopy.org.uk/ome/ticket/11732

Also, switch to using python's functools instead of legacy django.utils.functional.

@atarkowska
Copy link
Member

I don't see problems with merging this code. @manics do you have any comments?

@manics
Copy link
Member

manics commented Nov 25, 2013

@aleksandra-tarkowska I'm not familiar enough with Django.

@joshmoore
Copy link
Member

@knabar / @cneves / @chris-allan : any opinions?

@knabar
Copy link
Member

knabar commented Nov 26, 2013

@joshmoore Looks good as a temporary patch

@chris-allan
Copy link
Member

For reference, the Python bug (only "fixed" in 3.3+), how the Django team fixed this issue with cache_page() and the resultant method in django.util.decorators which handles missing attributes:

The amount of hackery here is pretty significant. These commits need to be annotated better. At a minimum I would like to see these commits annotated with the ticket numbers. If re-writing history is a problem then the comments should be extended to include them.

@dpwrussell
Copy link
Member Author

I will unify this into a single commit with the issues in the commits, I'll put them in the code as well. It would also be possible to instead write our own version of method_decorator. Something like this:

https://gist.github.com/dpwrussell/7573003#file-test3-py

@chris-allan
Copy link
Member

Sounds good.

@knabar and I were also discussing our own version of method_decorator() but it's kind of six of one, half a dozen of another as far as hackery goes. It also has the added requirement of needing to have explicit instructions in our documentation to use our version. Not to mention the the subsequent frustration for plugin developers that miss that.

What would obviously be ideal is if the Django project would expand method_decorator() to use available_attrs() as their make_middleware_decorator() already does. Maybe we should PR to that effect as the status of ticket13093 on the Django side of things is basically dead? It's not going to help us in the short term but would show that at least we're willing to be good citizens in that community.

shrugs Up to you.

@dpwrussell
Copy link
Member Author

https://code.djangoproject.com/ticket/21513#ticket

I created a new one rather than tack it onto https://code.djangoproject.com/ticket/13879 as in that ticket they are now talking about adding method_decorator_with_args which is not what my PR addresses.

@chris-allan
Copy link
Member

Annotated as discussed and while ugly is rather innocuous. Assuming we have no issues over the next day or so this should be ready to merge.

@@ -75,6 +76,15 @@ def __init__(self, useragent='OMERO.web', isAdmin=False,
self.omero_group = omero_group
self.allowPublic = allowPublic

# To make django's method_deocrator work, this is required until
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/deocrator/decorator/

This addresses the fact that django's method_decorator does not cope
with argumented (or instantitated at all) decorators.

This can and should be removed if upstream changes make it possible to
do so.

References:

The python bug (fixed in 3.3+ only, not backporting) http://bugs.python.org/issue3445
django cache_page fix: https://code.djangoproject.com/ticket/13093
django handling missing attributes: https://github.com/django/django/blob/c3aa2948c6c14862407501290571f858ccf45b07/django/utils/decorators.py#L73
django bugs: https://code.djangoproject.com/ticket/21513, https://code.djangoproject.com/ticket/13879
@dpwrussell
Copy link
Member Author

@chris-allan Fixed and squashed 'deocrator' typo.

@joshmoore
Copy link
Member

Things have been relatively quite over the past 2 days. Merging as discussed.

joshmoore added a commit that referenced this pull request Dec 4, 2013
Worked around method_decorator problem with custom getattr
@joshmoore joshmoore merged commit 67598f5 into ome:develop Dec 4, 2013
@dpwrussell
Copy link
Member Author

Can I rebase this onto 4_4? It shouldn't change any behaviour for any existing (well, excluding 3rd parties who would have to have worked around this in some other way in the unlikely event they encountered it) code.

@dpwrussell
Copy link
Member Author

--rebased-to #1928

@dpwrussell dpwrussell deleted the decorator_fix branch February 12, 2014 16:54
@dpwrussell
Copy link
Member Author

FYI: My change has now been merged upstream.

django/django#1997

@joshmoore
Copy link
Member

Is there any cleanup that should be planned based on 1997?

@dpwrussell
Copy link
Member Author

I will keep an eye on the django releases and when this commit makes it into a stable django release then we could bump and cleanup, but until then it's still necessary.

@chris-allan chris-allan mentioned this pull request Oct 25, 2016
9 tasks
will-moore added a commit to will-moore/openmicroscopy that referenced this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants