[Django] #28856: GenericForeignKey attributes create new instances on every access
Django
------------------------------------------------+------------------------
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
Given these models:
{{{
class OtherSuper(models.Model):
pass
class OtherSub(OtherSuper):
pass
class Ref(models.Model):
obj_type = models.ForeignKey(ContentType, on_delete=models.PROTECT)
obj_id = models.CharField(max_length=255)
obj = GenericForeignKey('obj_type', 'obj_id')
}}}
I get this behavior:
{{{
In [1]: ref = Ref.objects.create(obj=OtherSub.objects.create())
In [2]: id(ref.obj) == id(ref.obj)
Out[2]: True
In [3]: ref.refresh_from_db()
In [4]: id(ref.obj) == id(ref.obj)
Out[4]: False
}}}
Each time `ref.obj` is accessed, a new instance is created for its value.
This is a problem, since doing something like `ref.obj.field = 1;
ref.obj.save()` won't actually update the field in the database. This only
happens when the referenced object is an instance of a model that
subclasses another model. (So, it wouldn't happen if referencing
`OtherSuper` in the models above.)
I've written a regression test against stable/1.10.x . I'll attach a
patch.
I discovered this because I have code that does the above (changes a field
on the related model and calls save). I call this a regression because it
works correctly on 1.9.
I'm not sure what the underly bug is; I looked at the diff in
`contenttypes` between 1.9 and 1.10, and there are more than a few
changes. Hopefully someone who understands the `GenericForeignKey`
implementation can figure this out.
--
Ticket URL: <https://code.djangoproject.com/ticket/28856>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
contrib.contenttypes |
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 Morgan Wahl):
* Attachment "1.10-regression-test.patch" added.
patch with regression test
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
contrib.contenttypes |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Morgan Wahl:
Old description:
> class OtherSub(OtherSuper):
> pass
>
New description:
Given these models:
{{{
class OtherSuper(models.Model):
pass
class OtherSub(OtherSuper):
pass
class Ref(models.Model):
obj_type = models.ForeignKey(ContentType, on_delete=models.PROTECT)
obj_id = models.CharField(max_length=255)
obj = GenericForeignKey('obj_type', 'obj_id')
}}}
I get this behavior:
{{{
In [1]: ref = Ref.objects.create(obj=OtherSub.objects.create())
In [2]: id(ref.obj) == id(ref.obj)
Out[2]: True
In [3]: ref.refresh_from_db()
In [4]: id(ref.obj) == id(ref.obj)
Out[4]: False
}}}
Each time `ref.obj` is accessed, a new instance is created for its value.
This is a problem, since doing something like `ref.obj.field = 1;
ref.obj.save()` won't actually update the field in the database. This only
happens when the referenced object is an instance of a model that
subclasses another model. (So, it wouldn't happen if referencing
`OtherSuper` in the models above.) The `refresh_from_db()` call is also
necessary to reproduce this in a simple test like the above; it happens
with any instance created from an existing DB record.
I've written a regression test against stable/1.10.x . I'll attach a
patch.
I discovered this because I have code that does the above (changes a field
on the related model and calls save). I call this a regression because it
works correctly on 1.9.
I'm not sure what the underly bug is; I looked at the diff in
`contenttypes` between 1.9 and 1.10, and there are more than a few
changes. Hopefully someone who understands the `GenericForeignKey`
implementation can figure this out.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:1>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
contrib.contenttypes |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Bisected to b9f8635f58ad743995cad2081b3dc395e55761e5. Do you spot a bug in
that commit?
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:2>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
contrib.contenttypes |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I'm pretty sure this is the expected behavior from `refresh_from_db()`,
refreshing any relationship should create a new instance as
`ref.refresh_from_db()` should roughly translate to `ref =
Ref.objects.get(pk=ref.pk)` while preserving `ref` identity.
e.g. foreign key relationships have always worked this way
{{{#!python
>> obj_type = ref.obj_type
>> ref.refresh_from_db()
>> ref.obj_type is obj_type
False
}}}
b9f8635f58ad743995cad2081b3dc395e55761e5 just made this consistent for
generic foreign key which wouldn't be refreshed before it landed. This was
a problem because if either `ref.obj_type` or `ref.obj_id` had changed
when `refresh_from_db()` was called then `ref.obj` would point to a
different object that the one referenced by `ref.obj_type` and
`ref.obj_id`.
Django's ORM doesn't maintain an identity mapping of object, as opposed to
SQL Alchemy for example, so you shouldn't expect refreshed instances to
share their identity.
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:3>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
contrib.contenttypes |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Morgan Wahl):
Thanks for looking at this Simon.
I should've been a bit clearer about the problem: on the instance return
from `refresh_from_db`, *every access of the GFK attribute* produces a
*different instance* of the related object. (I tried to show that with the
`id(ref.obj) == id(ref.obj)` line).
I called `refresh_from_db` just to simulate a real-world situation where
the row backing the `ref` instance here is created at some point, and at
some later point a new model instance is instantiated for that row.
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:4>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Thanks Morgan, I missed that! This is quite a complex scenario: GFK, PK
type mismatch, and MTI so I'm not surprised it broke given the small
number of tests for PK type coercion.
The culprit seems to be that related fields don't delegate `to_python` to
the field they reference so the `OtherSub` primary key, a `OneToOneField`,
uses the default `Field.to_python` which is an identity function. This
causes
[https://github.com/django/django/blob/b9f8635f58ad743995cad2081b3dc395e55761e5/django/contrib/contenttypes/fields.py#L234
this line] to be falsey because `rel_obj._meta.pk.to_python('1')` returns
`'1'` instead of `1`.
I feel like the correct way to solve that would to implement
`ForeignKey.to_python` (which `OneToOneField` subclasses) to use
`self.foreign_related_fields[0].to_python` but even if this causes a
single failure in the test suite I feel like this would be to intrusive
for a backport. What I'd favor doing instead is implementing this approach
in master and adapt `GenericForeignKey.__get__` to work around the issue
if a backport is deemed eligible.
Here's a PR for master: https://github.com/django/django/pull/9395
And one for 1.11.x if you judge it should be backported Tim:
https://github.com/django/django/pull/9396
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:5>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Morgan Wahl):
Could the backport fix be applied to 1.10 as well? Is **is** still
supported, and this is a potential data loss scenario.
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:6>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
I don't think this merits another Django 1.10 release considering that
supports ends December 1. If they care about security, most users should
have upgraded to Django 1.11 by now.
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:7>
Django
Reporter: Morgan Wahl | Owner: nobody
Component: contrib.contenttypes | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"e50add6ca1605dcc06c8c5a5770342779a4d5124" e50add6c]:
{{{
#!CommitTicketReference repository=""
revision="e50add6ca1605dcc06c8c5a5770342779a4d5124"
Fixed #28856 -- Fixed a regression in caching of a GenericForeignKey
pointing to a MTI model.
Regression in b9f8635f58ad743995cad2081b3dc395e55761e5.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:8>
Django
Reporter: Morgan Wahl | Owner: nobody
Component: contrib.contenttypes | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d31424fec1a3de9d281535c0503644a9d7b93c63" d31424fe]:
{{{
#!CommitTicketReference repository=""
revision="d31424fec1a3de9d281535c0503644a9d7b93c63"
[2.0.x] Fixed #28856 -- Fixed a regression in caching of a
GenericForeignKey pointing to a MTI model.
Regression in b9f8635f58ad743995cad2081b3dc395e55761e5.
Modified backport of e50add6ca1605dcc06c8c5a5770342779a4d5124 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:9>
Django
Reporter: Morgan Wahl | Owner: nobody
Component: contrib.contenttypes | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f319e7abad145ddcb1017293b5cdb7e09a92ee85" f319e7ab]:
{{{
#!CommitTicketReference repository=""
revision="f319e7abad145ddcb1017293b5cdb7e09a92ee85"
[1.11.x] Fixed #28856 -- Fixed a regression in caching of a
GenericForeignKey pointing to a MTI model.
Regression in b9f8635f58ad743995cad2081b3dc395e55761e5.
Backport of d31424fec1a3de9d281535c0503644a9d7b93c63 from stable/2.0.x
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:10>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Morgan Wahl):
* status: closed => new
* resolution: fixed =>
Comment:
This fix applied to 1.11 fails when the referenced object is a subclass of
a subclass.
I have regression test and fix for this situation here:
https://github.com/django/django/compare/stable/1.11.x...addgene:mw/gfk-
recursive-fix
Should I open a PR?
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:11>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Morgan Wahl):
I went ahead and made the PR so I could see test results.
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:12>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a2aea4521d5e3cf8c76ef17e6edafee1c87bbf0a" a2aea452]:
{{{
#!CommitTicketReference repository=""
revision="a2aea4521d5e3cf8c76ef17e6edafee1c87bbf0a"
[1.11.x] Refs #28856 -- Fixed caching of a GenericForeignKey pointing to a
model that uses more than one level of MTI.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:13>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a06828cd2e3e34188466ed4f9a76d7838d0bf4f5" a06828cd]:
{{{
#!CommitTicketReference repository=""
revision="a06828cd2e3e34188466ed4f9a76d7838d0bf4f5"
[2.0.x] Reverted "[1.11.x] Refs #28856 -- Fixed caching of a
GenericForeignKey pointing to a model that uses more than one level of
MTI."
This reverts commit a2aea4521d5e3cf8c76ef17e6edafee1c87bbf0a as it was
committed by mistake.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:14>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Morgan Wahl <morgan@…>):
In [changeset:"35222035029863f95769e2e59beeeb953d125689" 35222035]:
{{{
#!CommitTicketReference repository=""
revision="35222035029863f95769e2e59beeeb953d125689"
[1.11.x] Refs #28856 -- Fixed caching of a GenericForeignKey pointing to a
model that uses more than one level of MTI.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:15>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5ca9cf47a7adcb990bd7e4886d5b8c066b5ca171" 5ca9cf4]:
{{{
#!CommitTicketReference repository=""
revision="5ca9cf47a7adcb990bd7e4886d5b8c066b5ca171"
[2.0.x] Refs #28856 -- Fixed caching of a GenericForeignKey pointing to a
model that uses more than one level of MTI.
Forwardport of 35222035029863f95769e2e59beeeb953d125689 from stable/1.11.x
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:16>
Django
Reporter: Morgan Wahl | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a9e5ac823df8ba8b786b6450c967ca378c008d0e" a9e5ac8]:
{{{
#!CommitTicketReference repository=""
revision="a9e5ac823df8ba8b786b6450c967ca378c008d0e"
Refs #28856 -- Added test for caching of a GenericForeignKey pointing to a
model that uses more than one level of MTI.
Forwardport of test and release notes of
35222035029863f95769e2e59beeeb953d125689 from stable/1.11.x
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:17>
Django
Reporter: Morgan Wahl | Owner: nobody
Component: contrib.contenttypes | Version: 1.10
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/28856#comment:18>