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)
follow-up: 2 comment:1 by , 9 years ago
comment:2 by , 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 , 9 years ago
Component: | Database layer (models, ORM) → Documentation |
---|---|
Summary: | prefetch_related() and add() on related field → Document that prefetch_related() cache doesn't change unless it's refetched from the database |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/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 , 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 , 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:7 by , 8 years ago
Discussion opened here: https://groups.google.com/forum/#!topic/django-developers/ejOpJ_r7tv8
comment:8 by , 8 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
The behavior change in #26706 invalidates this ticket.
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.