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

[django] Do not generate warning for (un)needed prefetch_related on get #10

Closed
twidi opened this issue Dec 10, 2015 · 7 comments
Closed

Comments

@twidi
Copy link
Contributor

twidi commented Dec 10, 2015

Hello.

I love the idea of this app. Wanted this/to create it for years. So, thank you.

One thing that annoy me a little is this: in the test test_many_to_one_reverse, nplusone generates a warning when selecting addresses for a user.

But I don't think it should, as doing a prefetch_related here won't change anything: we'll still have two queries, one to get the first user, and one to get the addresses (instantly if we use prefetch_related, and only when asking for addreses when not)

Sadly this prevents me to really use this lib because it can happens a lot as there is no optimization to do here, so nobody does a prefetch_related for a get (or .first() too?).
Think of edit forms, detail views...

The only way to change this would be to monkey patch the get method, to not take the current queryset into action for missing prefetch_related (but still do for missing select_related).

@twidi
Copy link
Contributor Author

twidi commented Dec 11, 2015

Checking in the code, I think that not only get, but generally __getitem__ if argument is not a slice but a single index. It will do the prefetch_related queries each time we do my_queryset[index]

I still don't know in your code how to ignore relations that are not FK or OneToOne for __getitem__

@jmcarp
Copy link
Owner

jmcarp commented Dec 11, 2015

Good point. One option might be to ignore queries that return a single result, like queryset.first() or queryset[2]. That's how bullet tries to handle this problem: https://github.com/flyerhzm/bullet/blob/master/lib/bullet/active_record42.rb. We could monkeypatch queryset.__iter__, and only warn on lazy loads when >1 objects were returned in the original load.

Does that make sense? I can try it out soon.

@twidi
Copy link
Contributor Author

twidi commented Dec 11, 2015

first and get both call queryset[0] so yes it's the same as queryset[2], it's why I talked about __getitem__. By the way we should not totally ignore queries, as FK (or One2One) that are not included with select_related should still emit a warning.

@twidi
Copy link
Contributor Author

twidi commented Dec 11, 2015

I really tried to do something but I didn't succeed to have a full vision of what the code is doing :)

@jmcarp
Copy link
Owner

jmcarp commented Dec 14, 2015

I started working on this in #11.

@twidi
Copy link
Contributor Author

twidi commented Dec 31, 2015

I just saw you merged your work. I'll try as soon as Tuesday ! Thanks a lot!

@twidi
Copy link
Contributor Author

twidi commented Jan 5, 2016

It seems ok, I close the issue :)

Fixed in #11

@twidi twidi closed this as completed Jan 5, 2016
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

No branches or pull requests

2 participants