#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 django-updates+unsubscr...@googlegroups.com.
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.
