[Django] #26722: Django silently discarding the user-provided on_delete with GenericRelation
Django
--------------------------------------+--------------------
Reporter: delizin | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
In the [https://docs.djangoproject.com/en/1.9/ref/contrib/contenttypes
/#generic-relations contenttypes framework documentation] it shows the
content_type foreignkey field with the on_delete option set to
models.CASCADE.
In it states that
[https://docs.djangoproject.com/en/1.9/ref/contrib/contenttypes/#reverse-
generic-relations Reverse generic relations] section it states "Unlike
ForeignKey, GenericForeignKey does not accept an on_delete argument to
customize this behavior".
Before realizing that it was not possible to change the on_delete method,
I attempted to change it to models.SET_NULL using three different methods
with only one resulting in an error or warning.
The first method I attempted was setting the content_type on_delete to
models.SET_NULL, because it was the only value with an on_delete argument
shown in the documentation (though I now assume that was perhaps shown
because it will be required with Django 2.0). It is logical for this to
not provide any warning or errors, but perhaps the documentation could be
made clearer on this point.
{{{
#!div style="font-size: 80%"
Model:
{{{#!python
class foo(models.Model):
content_type = models.ForeignKey(ContentType, null=True, blank=True,
related_name='logentry_content_type', on_delete=models.SET_NULL)
object_id = models.IntegerField(null=True, blank=True)
content_object = GenericForeignKey('content_type', 'object_id')
}}}
}}}
The second method was to try adding the on_delete argument to the
GenericForeignKey. This was rejected immediately as an unexpected keyword.
That behavior is expected based on the documentation.
Next I tried adding an on_delete argument to the GenericRelation itself.
The argument was silently accepted and then discarded.
{{{
#!div style="font-size: 80%"
Model:
{{{#!python
class bar(models.Model):
relation = GenericRelation(foo, on_delete=models.SET_NULL)
}}}
}}}
The issue appears to be that in the GenericRelation __init__ it is forcing
the kwargs['on_delete'] to models.CASCADE and discarding any user input.
{{{
#!div style="font-size: 80%"
[https://github.com/django/django/blob/master/django/contrib/contenttypes/fields.py#L300
GenericRelation model definition]
{{{#!python
kwargs['on_delete'] = models.CASCADE
}}}
}}}
My preference for a fix would be to allow GenericRelation to respect the
user defined wishes for on_delete, but if that is not feasible then
supplying an on_delete argument to GenericRelation should generate a
warning or an error to prevent users from assuming that GenericRelation
will respect the setting.
This ticket is related to a question I originally asked in #django-dev on
June 7th, 2015 at 15:15 UTC. I have attached the discussion below for
completeness.
{{{
#!div style="font-size: 70%"
{{{#!irc
[15:15] <delizin> In the django contenttypes framework docs for Generic
Relations it shows the content_type foreignkey with the
on_delete=models.CASCADE. However in use, if that foreign key is set to
nullable and on_delete is set to models.SET_NULL, when the generic object
is deleted, it still cascades. Adding on_delete=models.SET_NULL to the
GenericRelation does not change this behavior either. Is this a bug,
feature or just incorrect usage?
[15:16] <delizin> Relevant documentation:
https://docs.djangoproject.com/en/1.9/ref/contrib/contenttypes/#generic-
relations and the model I was testing with: https://dpaste.de/sYYJ
[15:22] <+bmispelon> delizin: I'm not very familiar with the contenttypes
framework but the documentation you linked says: "Unlike ForeignKey,
GenericForeignKey does not accept an on_delete argument to customize this
behavior"
[15:22] <+bmispelon> I have a sneaking suspicion that Django is happily
accepting your `on_delete` and silently discarding it
[15:22] <+bmispelon> which would be a bug in my opinion
[15:24] <+bmispelon>
https://github.com/django/django/blob/master/django/contrib/contenttypes/fields.py#L300
[15:27] <delizin> bmispelon: Interesting. I missed that part of the
documentation, though I agree with you that it shouldn't just ignore the
on_delete statement and should instead throw an error if you attempt to
change it.
[15:28] <+bmispelon> this anti-pattern is sadly quite common in Django's
codebase
[15:29] <+bmispelon> could you open an issue about it (Django silently
discarding the user-provided on_delete)?
[15:29] <delizin> bmispelon: Certainly
[15:30] <+bmispelon> I think Django should also warn you when you define a
generic relation based on a contenttype FK that has on_delete=SET_NULL (or
anything other than CASCADE)
[15:30] <+bmispelon> because from what you're saying, it seems that Django
will always cascade, no matter what
}}}
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26722>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: delizin | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26722#comment:1>
Django
Reporter: Delizin | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Humberto Rocha):
Hi, I have some models that rely at GenericRelations and recently
discovered that some data was disappearing from the database so i
discovered that issue.
There's any explanation on why GenericRelations can not have a SET_NULL or
even DO_NOTING implemented as a possible option of on_delete parameter?
The docs only states that "Unlike ForeignKey, GenericForeignKey does not
accept an on_delete argument to customize this behavior; if desired, you
can avoid the cascade-deletion simply by not using GenericRelation, and
alternate behavior can be provided via the pre_delete signal." which is
kind of vague.
Currently I am trying to tweak a little the GenericRelation code to see if
I can add at least the DO_NOTING behavior to it but even if I change the
this hardcoded line
[https://github.com/django/django/blob/c02d473781dc2e8699db8edd37cc77f7d43993fc/django/contrib/contenttypes/fields.py#L297]
from CASCADE to DO_NOTHING it seems to be ignored.
If anyone could help me out by pointing the way to follow on that would be
very helpful
--
Ticket URL: <https://code.djangoproject.com/ticket/26722#comment:2>