Opened 8 years ago
Last modified 6 years ago
#26722 new Bug
Django silently discarding the user-provided on_delete with GenericRelation
| Reported by: | Delizin | Owned by: | nobody |
|---|---|---|---|
| Component: | contrib.contenttypes | Version: | 1.9 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
In the contenttypes framework documentation it shows the content_type foreignkey field with the on_delete option set to models.CASCADE.
In it states that 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.
Model:
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.
Model:
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 kwargson_delete to models.CASCADE and discarding any user input.
GenericRelation model definition
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.
[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
Change History (2)
comment:1 by , 8 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
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