Opened 9 years ago

Closed 8 years ago

#25344 closed Cleanup/optimization (invalid)

Document that prefetch_related() cache doesn't change unless it's refetched from the database

Reported by: Stefan Schindler Owned by: nobody
Component: Documentation Version: 1.8
Severity: Normal Keywords: prefetch_related
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When calling add() on a related field (ForeignKey) that has been prefetched, albeit the added object is processed at database level, it's not added to the prefetched collection. Following calls to field.all() therefore do not contain the added object.

Example:

from example.models import *

Article.objects.all().delete()
Author.objects.all().delete()

Author().save()

author = Author.objects.all().prefetch_related("articles").first()
author.articles.add(Article())

print(list(author.articles.all()))
print(list(Article.objects.all()))

Prints:

[]
[<Article: Article object>]

I could not find an info in the documentation about that behavior, and it can be very hard to spot in bigger code bases. This should not fail silently in my opinion, and disallowing adding to prefetched related fields might even be an option.

Here's the link to a complete minimal project setup; the code from above can just be pasted in a manage.py shell session: https://github.com/TankOs/django_prefetch_add

Change History (8)

comment:1 by Tim Graham, 9 years ago

I don't know that it's a problem worth the possibility of breaking existing code for (either by disallowing or by modifying the prefetch cache), but other opinions welcome.

in reply to:  1 comment:2 by Stefan Schindler, 9 years ago

Replying to timgraham:

I don't know that it's a problem worth the possibility of breaking existing code for (either by disallowing or by modifying the prefetch cache), but other opinions welcome.

The question is what the expected behavior is. From a logical standpoint, it's for sure returning all possible objects when all() is called, because the name implies all possible related objects. From a technical point this might differ, as I probably don't want new objects to be included within a cache that has already been filled (by the actual prefetch_related call).

Whatever it's going to be, at least some documentation is required. I might be the first one to run into this, but I can tell you that I had a really long debugging session before I found the cause of my issue. ;-)

(By the way, I'd be happy to provide a patch once this has been decided.)

comment:3 by Tim Graham, 9 years ago

Component: Database layer (models, ORM)Documentation
Summary: prefetch_related() and add() on related fieldDocument that prefetch_related() cache doesn't change unless it's refetched from the database
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I don't think it's a good idea to change the behavior, so I'll accept as a documentation patch. Especially for custom Prefetch's it's probably impossible to implement correct behavior "in-memory" (e.g. if Prefetch has a custom queryset, we'd have to determine if the new object matches it). Feel free to raise the issue on the DevelopersMailingList if you feel I'm wrong.

comment:4 by Stefan Schindler, 9 years ago

I agree that it's very difficult (if not impossible) to implement correct behavior for all prefetch scenarios. What do you think of disallowing adding entities to prefetched fields? Because if adding an entity does not reflect that entity in the collection, then it's what causes the most confusion, because naturally, when you add things to a container, you expect to find it there.

Personally, I consider prefetched fields as being immutable, so adding items to them is not what I'd expect anyway. I'm thinking of an exception if the user tries to add entities to a prefetched collection.

Feel free to raise the issue on the DevelopersMailingList if you feel I'm wrong.

Can this ticket still be used for discussions?

comment:5 by Tim Graham, 9 years ago

I'm not sure about disabling manager methods if the objects are prefetched. That could be awfully backwards incompatible for questionable gain in my opinion. As no one else offered any opinions in a week after the ticket was opened, I suggested to write to the mailing list to reach a wider audience. Proposals for backwards incompatible changes are always best raised there too.

comment:6 by Stefan Schindler, 9 years ago

Okay thanks, I'll post to the mailing list then. Thank you! :-)

comment:8 by Tim Graham, 8 years ago

Resolution: invalid
Status: newclosed

The behavior change in #26706 invalidates this ticket.

Note: See TracTickets for help on using tickets.
Back to Top