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

Allow DjangoObjectPermissions to use views that define get_queryset #2904

Closed

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented May 5, 2015

New version of #2864

The noticeable change compare to #2864 is that we do not catch anymore AttributeError

try:
    queryset = view.get_queryset()
except AttributeError:
    # the Attribute error will not catch what we want
    # because hasattr(view, 'get_queryset') will always return True
    # the only AttributeError we will catch is comming from the .get_queryset()
    # method itself during execution, which is really bad.
    queryset = getattr(view, 'queryset', None)
except AssertionError:
    # view.get_queryset() didn't find .queryset
     queryset = None

@@ -109,8 +110,6 @@ def get_required_permissions(self, method, model_cls):
def has_permission(self, request, view):
try:
queryset = view.get_queryset()
except AttributeError:
queryset = getattr(view, 'queryset', None)
Copy link
Member

Choose a reason for hiding this comment

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

@ticosax
Copy link
Contributor Author

ticosax commented May 5, 2015

APIRoot view doesn't have get_queryset() method.

@ticosax ticosax closed this May 5, 2015
@ticosax
Copy link
Contributor Author

ticosax commented May 5, 2015

@tomchristie Sorry I didn't saw your comments.
I closed it because I realized my assumption was wrong, not all views has .get_queryset() defined.
I will write a test with APIRoot support and take into account your comments.
thank you for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants