Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Allow DjangoObjectPermissions to use views that define get_queryset#2905
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 ourterms of service andprivacy 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#2905
Uh oh!
There was an error while loading.Please reload this page.
Conversation
rest_framework/permissions.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd probably prefer for us to just leave the logic in the method itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fair enough
63c84c3 toea2bc91Compareticosax commentedMay 5, 2015
@tomchristie I addressed your comment. |
ea2bc91 tofbd8156Compareticosax commentedMay 13, 2015
@xordoquy Is there is a chance to have it included in 3.1.2 ? |
xordoquy commentedMay 13, 2015
Will try to find some time to review it before the release |
ticosax commentedMay 13, 2015
Thank you I appreciate. |
rest_framework/permissions.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This block looks confusing as it currently stands.
Can't it just be...
try: queryset = view.get_queryset()except AttributeError: queryset = getattr(view, 'queryset', None)Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The problem is that yo can have.get_queryset() defined but its implementation might raiseAttributeError during execution. We should prevent catching thisAttributeError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
but its implementation might raise AttributeError during execution.
That's such a crazy edge case that I don't really care about it. I'd prefer clarity of this code, and we'd still be using sensible enough behavior in that case. (Plus we'd expect the error to be raised elsewhere such as when initially using the queryset in the view)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I understand. I'm going to make the change
lovelydinosaur commentedMay 13, 2015
lovelydinosaur commentedMay 13, 2015
I'll put a 3.1.2 label on it speculatively just to keep it surfaced for the moment, but up to@xordoquy if it stays or not. |
fbd8156 toeb2be3aCompareticosax commentedMay 13, 2015
I made the changes. |
rest_framework/permissions.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure why we have anAssertionError block here, but it looks wrong. Just the first four lines are sufficient:
try: queryset = view.get_queryset()except AttributeError: queryset = getattr(view, 'queryset', None)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Mainly for two reasons:
1 - To honourgetattr(view, '_ignore_model_permissions', False) behaviour. Since some views without .queryset are tolerated.
2 - Because the absence of queryset is raised few lines below with an adhoc error message.
assertquerysetisnotNone, ('Cannot apply DjangoModelPermissions on a view that ')
I thought it would be nice to keep it that way, to be consistent withDjangoModelPermissions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We def don't need theAssertionError catching in either case, but it might also be nicer to moveif getattr(view, '_ignore_model_permissions', False): return True behavior to instead beabove the queryset check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Very nice. I like it.
lovelydinosaur commentedMay 13, 2015
@ticosax Looks nearly there. |
eb2be3a to031ac2aCompareticosax commentedMay 13, 2015
I pushed a new version. |
Allow DjangoObjectPermissions to use views that define get_queryset
lovelydinosaur commentedMay 13, 2015
✨ |
ticosax commentedMay 13, 2015
😅 Thank you |
xordoquy commentedMay 13, 2015
ace ! |
I'm sorry for the noise. I hope this time it is the right one.
It is a refactoring of#2904, I added explicit support for
APIRootwith a test.