[Django] #25693: Data loss if a ManyToManyField is shadowed by Prefetch
Django
----------------------------------------------+--------------------
Reporter: Ian-Foote | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
With two models:
{{{#!python
class Book(Model):
name = CharField(max_length=100)
class Author(Model):
books = ManyToManyField(Book)
}}}
it is possible to create a prefetch query that loses data from
Author.books:
{{{#!python
poems = Book.objects.filter(name='Poems')
Author.objects.prefetch_related(
Prefetch('books', queryset=poems, to_attr='books'),
)
}}}
When this queryset is evaluated, each Author's books is overridden by the
poems queryset.
--
Ticket URL: <https://code.djangoproject.com/ticket/25693>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ian-Foote):
* owner: nobody => Ian-Foote
* needs_docs: => 0
* status: new => assigned
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:1>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Ian-Foote:
Old description:
> With two models:
>
> {{{#!python
> class Book(Model):
> name = CharField(max_length=100)
>
> class Author(Model):
> books = ManyToManyField(Book)
> }}}
>
> it is possible to create a prefetch query that loses data from
> Author.books:
>
> {{{#!python
> poems = Book.objects.filter(name='Poems')
> Author.objects.prefetch_related(
> Prefetch('books', queryset=poems, to_attr='books'),
> )
> }}}
>
> When this queryset is evaluated, each Author's books is overridden by the
> poems queryset.
New description:
With two models:
{{{#!python
class Book(Model):
name = CharField(max_length=100)
class Author(Model):
books = ManyToManyField(Book)
}}}
it is possible to create a prefetch query that loses data from
{{{Author.books}}}:
{{{#!python
poems = Book.objects.filter(name='Poems')
Author.objects.prefetch_related(
Prefetch('books', queryset=poems, to_attr='books'),
)
}}}
When this queryset is evaluated, each Author's {{{books}}} is overridden
by the {{{poems}}} queryset.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:2>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tomchristie):
* cc: tom@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:3>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tomchristie):
Realities downstream issue: https://github.com/tomchristie/django-rest-
framework/issues/2442
I assume this ticket is a duplicate of the (closed, wontfix) ticket here:
https://code.djangoproject.com/ticket/21584
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:4>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
I think these are different issues.
----
#21584 demonstrates getting an object, prefetching related objects,
creating an additional related object: the list of prefetched related
objects isn't refreshed automatically. (The description makes it look more
complicated than it actually is.)
That's because parent.children_set is just a queryset. It's consistent
with how querysets have always been working. There is exactly the same
behavior without `prefetech_related`, with any queryset for which the
results have been fetched.
The DRF issue Tom mentions is probably a consequence of this. I believe
DRF should refetch data that may have been invalidated by the updates
before serializing its response.
----
Now, this bug appears to be much worse becase incorrect data gets written
back to the database (if I understand correctly).
Ian, do you know since which version of Django it occurs? I'm wondering if
we can but controls in place to avoid assigning prefetched querysets to
writable descriptors or preventing the write from occurring...
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:5>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by knbk):
This seems related to #25550. Would a backport fix this bug?
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:6>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ian-Foote):
I've verified it on master and on 1.8. I assume it also applies to 1.9.
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:7>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* cc: charettes (added)
Comment:
This issue also recently surfaced on the
[https://groups.google.com/d/topic/django-users/CDe4McxxCsI/discussion
django-users mailing list].
> Now, this bug appears to be much worse becase incorrect data gets
written back to the database (if I understand correctly).
You do.
> Ian, do you know since which version of Django it occurs? I'm wondering
if we can but controls in place to avoid assigning prefetched querysets to
writable descriptors or preventing the write from occurring...
This bug exists since the introduction of the `Prefetch` object (Django
1.7).
> This seems related to #25550. Would a backport fix this bug?
The ticket is related but the issue will remain until the assignment is
actually removed. It's currently only pending deprecation on `master`.
Since we'll have to live with this assignment issue until 2.0 the only
viable solution I can think of at the moment would be to raise a
`ValueError` if `getattr(queryset.model, to_attr)` is an instance of one
of the problematic descriptors.
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:8>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
Note that the issue is even worse with `GenericRelation` on Django < 1.9
which used to issue a `manager.clear()` instead of relying on the revised
logic added by #21169.
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:9>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ian-Foote):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:10>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 0 => 1
* version: master => 1.7
* needs_docs: 0 => 1
Comment:
The patch is looking good. It's only missing a release note for the 1.8
and 1.7 series.
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:11>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: assigned
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ian-Foote):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:12>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Component: Database layer | Version: 1.7
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4608573788c04fc047da42b4b7b48fdee8136ad3" 46085737]:
{{{
#!CommitTicketReference repository=""
revision="4608573788c04fc047da42b4b7b48fdee8136ad3"
Fixed #25693 -- Prevented data loss with Prefetch and ManyToManyField.
Thanks to Jamie Matthews for finding and explaining the bug.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:13>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f9a08eb8975d8a49bee8cc42ae207100adb6ce75" f9a08eb]:
{{{
#!CommitTicketReference repository=""
revision="f9a08eb8975d8a49bee8cc42ae207100adb6ce75"
[1.9.x] Fixed #25693 -- Prevented data loss with Prefetch and
ManyToManyField.
Thanks to Jamie Matthews for finding and explaining the bug.
Backport of 4608573788c04fc047da42b4b7b48fdee8136ad3 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:14>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5fc9a1b8bd3eb13b7de86ed82ad281d34fa02e8c" 5fc9a1b8]:
{{{
#!CommitTicketReference repository=""
revision="5fc9a1b8bd3eb13b7de86ed82ad281d34fa02e8c"
[1.8.x] Fixed #25693 -- Prevented data loss with Prefetch and
ManyToManyField.
Thanks to Jamie Matthews for finding and explaining the bug.
Backport of 4608573788c04fc047da42b4b7b48fdee8136ad3 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:15>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"6184cb9baa8426ed8b3e1fb5d96afac809dd2a0c" 6184cb9b]:
{{{
#!CommitTicketReference repository=""
revision="6184cb9baa8426ed8b3e1fb5d96afac809dd2a0c"
[1.7.x] Fixed #25693 -- Prevented data loss with Prefetch and
ManyToManyField.
Thanks to Jamie Matthews for finding and explaining the bug.
Backport of 4608573788c04fc047da42b4b7b48fdee8136ad3 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:16>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"cc8c02fa0fa2119704d1c39ca8509850aef84acc" cc8c02fa]:
{{{
#!CommitTicketReference repository=""
revision="cc8c02fa0fa2119704d1c39ca8509850aef84acc"
Refs #25693 -- Added a regression test for `to_attr` validation on forward
m2m.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:18>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"4a9c32f5eece9030c2b568e930cec0c1ba8f1da0" 4a9c32f]:
{{{
#!CommitTicketReference repository=""
revision="4a9c32f5eece9030c2b568e930cec0c1ba8f1da0"
Refs #25693 -- Avoided redundant calls to get_fields() in `to_attr`
validation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:17>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"164cbdac7d29a6a79df3fd5155d44df3589b057c" 164cbdac]:
{{{
#!CommitTicketReference repository=""
revision="164cbdac7d29a6a79df3fd5155d44df3589b057c"
[1.9.x] Refs #25693 -- Added a regression test for `to_attr` validation on
forward m2m.
Backport of cc8c02fa0fa2119704d1c39ca8509850aef84acc from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:20>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"0d1d30752aeb734e78b81f467cc8edd9154385c9" 0d1d307]:
{{{
#!CommitTicketReference repository=""
revision="0d1d30752aeb734e78b81f467cc8edd9154385c9"
[1.9.x] Refs #25693 -- Avoided redundant calls to get_fields() in
`to_attr` validation.
Backport of 4a9c32f5eece9030c2b568e930cec0c1ba8f1da0 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:19>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"ae4613803cb344a26b158934e785a82147515db0" ae461380]:
{{{
#!CommitTicketReference repository=""
revision="ae4613803cb344a26b158934e785a82147515db0"
[1.8.x] Refs #25693 -- Added a regression test for `to_attr` validation on
forward m2m.
Backport of cc8c02fa0fa2119704d1c39ca8509850aef84acc from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:22>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"a3baee2f6244f7c4b9ff253791b8972f684f0225" a3baee2]:
{{{
#!CommitTicketReference repository=""
revision="a3baee2f6244f7c4b9ff253791b8972f684f0225"
[1.8.x] Refs #25693 -- Avoided redundant calls to get_fields() in
`to_attr` validation.
Backport of 4a9c32f5eece9030c2b568e930cec0c1ba8f1da0 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:21>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"3d037b9f689d287567d5961939817d96907a1459" 3d037b9]:
{{{
#!CommitTicketReference repository=""
revision="3d037b9f689d287567d5961939817d96907a1459"
[1.7.x] Refs #25693 -- Avoided redundant calls to get_fields() in
`to_attr` validation.
Conflicts:
django/db/models/query.py
Backport of cc8c02fa0fa2119704d1c39ca8509850aef84acc from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:23>
Django
Reporter: Ian-Foote | Owner: Ian-Foote
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"fd1426570e7601128745d0e524704412c4431749" fd14265]:
{{{
#!CommitTicketReference repository=""
revision="fd1426570e7601128745d0e524704412c4431749"
[1.7.x] Refs #25693 -- Added a regression test for `to_attr` validation on
forward m2m.
Backport of cc8c02fa0fa2119704d1c39ca8509850aef84acc from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25693#comment:24>