#34816: GenericForeignKey crashes if content_type_id is changed and object_id is
type incompatible with old object
-------------------------------------+-------------------------------------
Reporter: Richard Laager | Owner: nobody
Type: Uncategorized | Status: new
Component: | Version: 4.2
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 Richard Laager):
* status: closed => new
* resolution: invalid =>
Old description:
> Steps to reproduce:
>
> 1. Create a model ("A") with a GenericForeignKey.
> 2. Create an instance of A ("a") referencing an instance of model "B"
> with a PK that is an integer type.
> 3. Change the instance of "A" to reference an instance of model "C" with
> a PK that is incompatible with int(). But make this change using
> `content_type_id` and `object_id` properties, not `content_object`, i.e.:
> {{{#!python
> a.content_type_id = ContentType.objects.get_for_model(B)
> a.object_id = "foo"
> }}}
> 4. Try to access `a.content_object`.
>
> Expected result:
>
> This looks up and returns an instance of "b" (the B with a PK of "foo").
>
> Actual result:
>
> This crashes (I'm using 3.2, but the code is unchanged in master):
> {{{#!python
> File "django/db/models/fields/__init__.py", line 1836, in to_python
> return int(value)
>
> ValueError: invalid literal for int() with base 10: 'foo'
> }}}
>
> This happens because it tries to `to_python()` the new key on the old
> model's PK field.
>
> One possible fix would be to make the logic short-circuit, like this
> (also attached):
> {{{#!diff
> diff --git a/django/contrib/contenttypes/fields.py
> b/django/contrib/contenttypes/fields.py
> index 35fcd0d908..e984fb5375 100644
> --- a/django/contrib/contenttypes/fields.py
> +++ b/django/contrib/contenttypes/fields.py
> @@ -242,11 +242,11 @@ class GenericForeignKey(FieldCacheMixin):
> ct_match = (
> ct_id == self.get_content_type(obj=rel_obj,
> using=instance._state.db).id
> )
> - pk_match = rel_obj._meta.pk.to_python(pk_val) == rel_obj.pk
> - if ct_match and pk_match:
> - return rel_obj
> - else:
> - rel_obj = None
> + if ct_match:
> + pk_match = rel_obj._meta.pk.to_python(pk_val) ==
> rel_obj.pk
> + if pk_match:
> + return rel_obj
> + rel_obj = None
> if ct_id is not None:
> ct = self.get_content_type(id=ct_id,
> using=instance._state.db)
> try:
> }}}
New description:
Steps to reproduce:
1. Create a model ("A") with a GenericForeignKey.
2. Create an instance of A ("a") referencing an instance of model "B" with
a PK that is an integer type.
3. Change the instance of "A" to reference an instance of model "C" with a
PK that is incompatible with int(). But make this change using
`content_type_id` and `object_id` properties, not `content_object`, i.e.:
{{{#!python
a.content_type_id = ContentType.objects.get_for_model(B).pk
a.object_id = "foo"
}}}
4. Try to access `a.content_object`.
Expected result:
This looks up and returns an instance of "b" (the B with a PK of "foo").
Actual result:
This crashes (I'm using 3.2, but the code is unchanged in master):
{{{#!python
File "django/db/models/fields/__init__.py", line 1836, in to_python
return int(value)
ValueError: invalid literal for int() with base 10: 'foo'
}}}
This happens because it tries to `to_python()` the new key on the old
model's PK field.
One possible fix would be to make the logic short-circuit, like this (also
attached):
{{{#!diff
diff --git a/django/contrib/contenttypes/fields.py
b/django/contrib/contenttypes/fields.py
index 35fcd0d908..e984fb5375 100644
--- a/django/contrib/contenttypes/fields.py
+++ b/django/contrib/contenttypes/fields.py
@@ -242,11 +242,11 @@ class GenericForeignKey(FieldCacheMixin):
ct_match = (
ct_id == self.get_content_type(obj=rel_obj,
using=instance._state.db).id
)
- pk_match = rel_obj._meta.pk.to_python(pk_val) == rel_obj.pk
- if ct_match and pk_match:
- return rel_obj
- else:
- rel_obj = None
+ if ct_match:
+ pk_match = rel_obj._meta.pk.to_python(pk_val) ==
rel_obj.pk
+ if pk_match:
+ return rel_obj
+ rel_obj = None
if ct_id is not None:
ct = self.get_content_type(id=ct_id,
using=instance._state.db)
try:
}}}
--
Comment:
Sorry, that's just a red herring from writing up the bug report. Here is a
real example from right now:
{{{
In [1]: from case.models import Case
In [2]: case = Case.objects.get(id=REDACTED)
In [3]: case.content_object
Out[3]: <REDACTED: REDACTED>
In [4]: case.content_type_id = 175
In [5]: case.object_id = '218-555-1212'
In [6]: case.content_object
---------------------------------------------------------------------------
ValueError Traceback (most recent call
last)
File /srv/www/testing/lib/.venv/lib/python3.10/site-
packages/django/db/models/fields/__init__.py:1836, in
IntegerField.to_python(self, value)
1835 try:
-> 1836 return int(value)
1837 except (TypeError, ValueError):
ValueError: invalid literal for int() with base 10: '218-555-1212'
During handling of the above exception, another exception occurred:
ValidationError Traceback (most recent call
last)
Cell In[6], line 1
----> 1 case.content_object
File /srv/www/testing/lib/.venv/lib/python3.10/site-
packages/django/contrib/contenttypes/fields.py:233, in
GenericForeignKey.__get__(self, instance, cls)
231 if rel_obj is not None:
232 ct_match = ct_id == self.get_content_type(obj=rel_obj,
using=instance._state.db).id
--> 233 pk_match = rel_obj._meta.pk.to_python(pk_val) == rel_obj.pk
234 if ct_match and pk_match:
235 return rel_obj
File /srv/www/testing/lib/.venv/lib/python3.10/site-
packages/django/db/models/fields/__init__.py:1838, in
IntegerField.to_python(self, value)
1836 return int(value)
1837 except (TypeError, ValueError):
-> 1838 raise exceptions.ValidationError(
1839 self.error_messages['invalid'],
1840 code='invalid',
1841 params={'value': value},
1842 )
ValidationError: ['“218-555-1212” value must be an integer.']
}}}
If step 3 (accessing `case.content_object`) is omitted, the problem does
not occur because there is no cached content_object. Alternatively, if
between 3 and 4 one does `case.content_object = None` to clear the cache,
the problem does not occur.
--
Ticket URL: <https://code.djangoproject.com/ticket/34816#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/0107018a6e1a6f00-ae65d49e-95e5-4ef5-b2b4-0fb3df518df3-000000%40eu-central-1.amazonses.com.
