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

Unrelated assert error message when there is an error in get_queryset #4585

Closed
Neraste opened this issue Oct 16, 2016 · 8 comments
Closed

Unrelated assert error message when there is an error in get_queryset #4585

Neraste opened this issue Oct 16, 2016 · 8 comments
Labels
Milestone

Comments

@Neraste
Copy link

Neraste commented Oct 16, 2016

In a view, I have define a get_queryset method in a APIView derived class. For some (not important) reasons, I have made a mistake in my code, but the error message I get is unrelated to the actual mistake: it is an AssertError, complaining there is no .queryset attribute or get_queryset method in the view. And the traceback does only imply files of the framework, not files I haver written and, obviously, neither the file I have made the mistake in.

Steps to reproduce

Create a class in views.py, deriving from APIView. Define a get_queryset method with a mistake in it, say an IndexError. the method do not return anything. Run it.

Txpected behavior

Get an error message involving an IndexError.
Get an error message about the empty return of get_queryset.

Actual behavior

Get an error message related to an AssertError.

@xordoquy
Copy link
Collaborator

xordoquy commented Oct 17, 2016

Hi there, this feels like a chat I had at pycon.fr. Was it with you ?

That put appart, you need to provide a queryset for the view to work with routers, whether or not you override get_queryset. Will send more informations once I can use my computer.

Meanwhile I'm keeping this opened as we may have a documentation issue here

@Neraste
Copy link
Author

Neraste commented Oct 17, 2016

Yep, that is me.

I am following the doc about this.

I will give you a MWE to locate the problem more precisely.

@xordoquy
Copy link
Collaborator

xordoquy commented Oct 17, 2016

Thanks for raising it :)
I managed to reproduce on the train back home.

We have an inconsistence here.

There's a word in the documentation about using permissions with views that don't have a queryset attribute but looking at the code, permission are using get_queryset several times

I don't see a reason why routers don't use get_queryset instead of queryset. So, we'll need a PR to fix the permission documentation and one to let routers use get_queryset like permissions do.

@xordoquy
Copy link
Collaborator

xordoquy commented Oct 17, 2016

I'm having hard time here and disgressing, sorry about that.
This issue is about getting rid of the assert message in favor of the initial exception within get_queryset right ?

@kevin-brown
Copy link
Member

I don't see a reason why routers don't use get_queryset instead of queryset.

There's a good chance that if someone is overriding get_queryset, they are relying on something being set (the view object, request, etc.) that cannot normally be set without going through the view lifecycle. By using the queryset argument, it allows us to quickly get a queryset (even if it's an empty queryset, like Model.objects.none()) without having to prepare the view every time.

@xordoquy
Copy link
Collaborator

@kevin-brown good catch, thanks a lot for your feedback. That point will be solved by adding some words in the documentation.

@Neraste
Copy link
Author

Neraste commented Oct 18, 2016

It seems that the problem I tried to describe is far less systematic than I thought... Raising an exception is not enough for poping an AssertError instead. And I didn't took note of what I was doing precisely when facing this problem.

However, I think I have isolated one form of it: if (for some dumb reasons), the get_queryset method did not return anything, I get:

AssertionError at /some/place

Cannot apply DjangoModelPermissions on a view that does not have `.queryset` property or overrides the `.get_queryset()` method.

It does not complain get_queryset did not return anything (which is the real problem), the error message is pretty confusing and the traceback does not involve files of my project.

@tomchristie
Copy link
Member

It does not complain get_queryset did not return anything (which is the real problem), the error message is pretty confusing.

Okay, that's a nicely isolated bit of behavior that'd be worth addressing. Let's constrain this issue to just that particular aspect, at least for now.

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

No branches or pull requests

5 participants