[Django] #30436: ForeignKey on_delete parameter should be validated
Django
-----------------------------------------+------------------------
Reporter: Rémy Hubscher | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.2
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 |
-----------------------------------------+------------------------
If you set `on_delete=None` as a ForeignKey field parameter you might get
the following error:
{{{
File "django/contrib/admin/options.py", line 1823, in
get_deleted_objects
return get_deleted_objects(objs, request, self.admin_site)
File "django/contrib/admin/utils.py", line 134, in get_deleted_objects
collector.collect(objs)
File "django/contrib/admin/utils.py", line 197, in collect
return super().collect(objs, source_attr=source_attr, **kwargs)
File "django/db/models/deletion.py", line 221, in collect
field.remote_field.on_delete(self, field, sub_objs, self.using)
TypeError: 'NoneType' object is not callable
}}}
I believe that we could validate the on_delete value to prevent such
behaviour. Or at least tell that None is not a valid on_delete value.
Refs
https://docs.djangoproject.com/fr/2.2/ref/models/fields/#django.db.models.ForeignKey.on_delete
--
Ticket URL: <https://code.djangoproject.com/ticket/30436>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Rémy Hubscher | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 2.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by zeynel):
IMO it would be unnecessary to add an extra check for `None` since it
covers all options for `on_delete` explicitly in documentation:
The possible values for on_delete are found in django.db.models:
...
https://docs.djangoproject.com/en/2.2/ref/models/fields/#django.db.models.ForeignKey.on_delete
--
Ticket URL: <https://code.djangoproject.com/ticket/30436#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Rémy Hubscher | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
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 felixxm):
* component: Uncategorized => Database layer (models, ORM)
* version: 2.2 => master
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Thanks for the report. I can imagine that someone may provide custom
`on_delete` behavior, hence I don't think we should validate this
attribute with a static list of `on_delete` behaviors defined by Django.
IMO we can raise
{{{
raise ValueError('on_delete must be callable.')
}}}
in `__init__()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30436#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by robinh00d):
* status: new => assigned
* owner: nobody => robinh00d
--
Ticket URL: <https://code.djangoproject.com/ticket/30436#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Rémy Hubscher | Owner: robinh00d
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by robinh00d):
I agree with adding a check in `__init__()` and raising
`ValueError('on_delete must be callable.')` . I'm going to submit a patch
soon, do you have any suggestion as to where I should write my tests?
--
Ticket URL: <https://code.djangoproject.com/ticket/30436#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Rémy Hubscher | Owner: robinh00d
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
`tests/delete` looks like a good place for tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/30436#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Rémy Hubscher | Owner: robinh00d
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by robinh00d):
* has_patch: 0 => 1
Comment:
I have submitted a PR:
[https://github.com/django/django/pull/11356 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30436#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Rémy Hubscher | Owner: robinh00d
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
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 Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c231a75112d8a06e1a776ef97f28a3be1b343047" c231a751]:
{{{
#!CommitTicketReference repository=""
revision="c231a75112d8a06e1a776ef97f28a3be1b343047"
Fixed #30436 -- Added check that on_delete is callable in ForeignKey and
OneToOneField.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30436#comment:7>