#7539 closed Uncategorized (fixed)
Add ON DELETE and ON UPDATE support to Django
Reported by: | glassfordm | Owned by: | Carl Meyer |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | feature |
Cc: | miracle2k, mathijs@…, matthubb, diegobz, road, Sergey Belov, Marinho Brandão, me@…, findepi, gabor@…, dnordberg@…, ehs@…, Jani Tiainen, hanne.moa@…, aaron@…, groby@…, Ilya Semenov, ales.zoulek@…, plandry@…, lukasz.korzybski@…, preston@…, eschler@…, paluho@…, gorsky.exe@…, ego@…, Gonzalo Saavedra, l.dziedzia@…, cgieringer, t.django@…, goliath.mailinglist@…, jdunck@…, mlong@…, erikrose, paulc@…, jakub@…, david@…, jbauer@…, luc.saffre@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
The attached patch adds two levels of ON DELETE and ON UPDATE
support to Django.
- Just applying the patch changes nothing: Django should continue
to behave exactly as before, emulating ON DELETE CASCADE behavior
just as it has all along.
However:
1) Adding on_delete=RESTRICT or on_delete=SET_NULL to a ForeignKey
definition will cause Django to emulate the specified behavior
for that particular foreign key. Django will continue to emulate
CASCADE behavior for ForeignKeys which have no on_delete behavior
specified; it will also emulate CASCADE behavior for foreign keys
having on_delete=CASCADE defined, of course.
2) The new setting, settings.ON_DELETE_NONE_HANDLING = <option>,
specifies the behavior of foreign keys that have no on_delete
option specified. It defaults to CASCADE (because this is what
Django has done in the past), but can be set to RESTRICT (essentially
the default for SQL) or SET_NULL as well (although SET_NULL is not
recommended: it will cause errors for ForeignKeys that are not nullable).
3) Finally, the patch does not provide any Django emulation for
ON UPDATE behavior, so, while the on_update option can be added to
foreign key definitions, it does nothing (but see below).
- Defining settings.ON_DELETE_HANDLED_BY_DB = True changes Django's
ON DELETE behavior completely:
1) It disables Django emulation of ON DELETE behavior entirely.
2) It adds appropriate ON DELETE statements to the SQL generated
by Django (by "python manage.py sqlall", for instance).
- Defining settings.ON_UPDATE_HANDLED_BY_DB = True adds appropriate
ON UPDATE statements to the SQL generated by Django
(by "python manage.py sqlall", for instance).
Attachments (14)
Change History (80)
comment:1 by , 16 years ago
Component: | Uncategorized → Database wrapper |
---|---|
Keywords: | feature added |
milestone: | → post-1.0 |
Triage Stage: | Unreviewed → Design decision needed |
comment:2 by , 16 years ago
by , 16 years ago
Attachment: | on_delete_on_update.diff added |
---|
Updated patch for the latest version of Django (revision 9845 at the time the patch was created).
comment:3 by , 16 years ago
Cc: | added |
---|
comment:4 by , 16 years ago
Is there a relevant difference between ON DELETE RESTRICT and the default? I ask because Oracle supports ON DELETE CASCADE and ON DELETE SET NULL but not ON DELETE RESTRICT. For this reason, I think it would be preferable to omit the clause entirely rather than specify ON DELETE RESTRICT.
comment:5 by , 16 years ago
Currently, the default behavior of Django is to emulate ON DELETE CASCADE; for compatibility reasons, I think this needs to remain the same (unless explicitly changed by the developer of a Django site). The ON DELETE RESTRICT option is necessary to tell Django to deviate from its default behavior and prevent the delete from happening rather than cascading it. This is pretty much what ON DELETE RESTRICT means in MySQL (although, now that you mention it, I think I remember that MySQL's implementation is somewhat non-standard). Is there a better wording than "RESTRICT" that means the right thing for Oracle as well as other backends? Is ON DELETE NO ACTION better?
comment:6 by , 16 years ago
"ON DELETE NO ACTION" is also not supported. The only way to get the "RESTRICT" behavior with Oracle is to omit the clause entirely.
If I'm understanding correctly, the difference between "RESTRICT" and "NO ACTION" is that "NO ACTION" uses deferred constraint checking. With Oracle, this is specified by omitting the "ON DELETE" clause and adding "DEFERRABLE INITIALLY DEFERRED". This is actually independent of the "ON DELETE" clause and would also affect updates.
I guess the upshot of all this is just that the Oracle backend will need to override the on_delete_and_update_clauses method.
comment:8 by , 16 years ago
Cc: | added |
---|
comment:9 by , 16 years ago
Cc: | added |
---|
Has anyone been working on this bug? I would very much like to see a current patch for it.
comment:10 by , 16 years ago
@Dokter Bob: This isn't a bug, it's a feature request. It's a fairly big feature request, too, so it's going to require a bit of discussion before it gets anywhere near trunk. However, now is not the time for that discussion, as we're in the final stages of the v1.1 release.
comment:11 by , 16 years ago
Cc: | added |
---|
comment:12 by , 16 years ago
Cc: | added |
---|
by , 16 years ago
Attachment: | on_delete_on_update-r10558.diff added |
---|
Patch updated for r10558. Very fresh and barely tested, see more info in ticket.
comment:13 by , 16 years ago
Note: the patch I've just uploaded does not propagate the new cascading behaviour to the admin. I am not sure whether the 'old version' of this patch did but it is something someone needs to fix. ;)
comment:14 by , 16 years ago
Someone requested a discussion...
I just had an encounter with this, and what I lacked was *knowing* what's going on with regards to cascading deletion, because it's quite dangerous (you loose data!!) and steals lots of time. Preventing it is simple, though. Overriding the delete() method of a model and calling clear() on the related model's foreign keys is simple, and it's a manual implementation, that all programmers should be able to understand.
But I can think of another alternative: If null=True for the foreign key in question, why not automatically use SET NULL when the related instance is deleted? For me, this is even more "intuitive" than CASCADE. After all, null=True is something that the programmer specifies and has to deal with anywhere in the implementation, which is also why he doesn't need cascading deletion of such a relation.
Also, if on_delete is possible to set in a key field, it would have to comply with the null option.. and together with the "intuition argument" that creates a 1:1 correspondence between the two options.
And then the "logic argument": Django handles its logic in Python code, not in the Database, which is kept as a simple storage engine. The RESTRICT option is a validation issue, and will probably be handled this way in most cases, so having the database enforcing it, would be redundant. To enable it on model-level could pave the way for some nice new automatic validation in ModelForms, so I think it sounds like a nice feature.
If all this is implemented, I would suggest to remove the null option from key fields and have it set according to on_delete.
comment:15 by , 16 years ago
Discussion should happen on django-dev, so that everybody can be involved (not everybody monitors every single ticket). We ask that all design discussion happens there. As Russell mentioned, now is not a good time to restart the discussion, since the people with a large piece to contribute are much more focused on the next release and we know we can put this off for a few weeks or months without harm. So keep an eye on django-dev and when discussion opens up regarding 1.2 features, that's the time to pipe up, on the mailing list.
comment:16 by , 16 years ago
I know, but gotta serve the soup while it's still hot. I meant it as a kind of suspended discussion input, to be read in due time. Apologies if that isn't usual procedure.
comment:17 by , 16 years ago
Cc: | added |
---|
comment:18 by , 16 years ago
Cc: | added |
---|
comment:20 by , 15 years ago
Cc: | added |
---|
comment:22 by , 15 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:23 by , 15 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:24 by , 15 years ago
Cc: | added |
---|
comment:25 by , 15 years ago
Cc: | added |
---|
comment:26 by , 15 years ago
Cc: | added |
---|
by , 15 years ago
Attachment: | on_delete_on_update-r11620.diff.txt added |
---|
Very preliminary patch for 1.2 release
comment:27 by , 15 years ago
I've attached a new patch containing a very preliminary version of what I'm working on for possible inclusion in the 1.2 release. As discussed in the django developer mailing list some time ago, it has a much more limited scope than my original patch in these ways:
1) It removes all ON UPDATE type behavior.
2) It removes the feature that allows the backend to provide the ON DELETE support and to cause Django to generate SQL with ON DELETE clauses in it.
3) It removes the settings that could be added to the settings files to control these behaviors.
It is my hope that #2, and possibly #1, can be added in future versions of Django 1.2.
The patch I've attached is very preliminary in these ways:
1) It contains some debugging code (commented out) and some TODO comments indicating things that need to be improved.
2) Some areas could use efficiency improvements.
3) It contains several intentionally failing tests illustrating problems I haven't yet solved.
4) There are no docs yet.
5) I've made no attempt to support any of the alternate terminology suggestions from this discussion: http://groups.google.com/group/django-developers/browse_thread/thread/4ece22d6053f774b/8be0b9be9b6d0b5f. This doesn't mean I won't, just that I haven't. Ideally, I'd prefer comments from more people on the suggestion that were made before choosing one of them.
I've posted this patch to give an idea what the final patch is attempting to implement and to ask for help on the problems I haven't solved yet.
What's here: OneToOne and ForeignKey field types in models have a new attribute, on_delete. This can be set to CASCADE, PROTECT, SET_NULL, or SET_DEFAULT. If any of these attributes are present in a foreign key definition, Django implements the appropriate behavior when an attempt is made to delete an item referenced by that foreign key.
by , 15 years ago
Attachment: | on_delete_on_update-r11620.diff added |
---|
Same as previous patch, but with correct extension
by , 15 years ago
Attachment: | 7539.on_delete.diff added |
---|
comment:28 by , 15 years ago
@glassfordm: Is it possible that you forgot to svn add
your tests before making the diff? I can't find any on_delete
related tests, except a handful of low level tests for CollectedFields
.
The patch I just attached is based on on_delete_on_update-r11620.diff
. I made the following changes:
- Removed checks for
default
andnull
in_handle_sub_obj()
(as these are static properties of fields and already checked incontribute_to_class()
). These checks should perhaps be moved todjango.core.management.validation
.
- Moved instance-update and sql-update code inside
CollectedFields
so the actual data structure doesn't have to be known to code outside. Is there a reason field updates are not simply collected inCollectedObjects
, too?
- Changed how update queries are performed: instead of one per instance, it now uses one (batch) per field. Except for corner cases (more than GET_ITERATOR_CHUNK_SIZE foreign keys on a single model affected), this results in fewer queries.
- Added a pony of mine:
on_delete=DO_NOTHING
, which would help with #10829.
I didn't write any tests (bad), but all existing tests pass. Which hopefully means that at least I haven't broken CASCADE and SET_NULL.
Regarding stale related object caches, I'd do nothing special. Django currently doesn't refresh them, so any change here (for SET_NULL or CASCADE) would be backwards incompatible. And there is no guarantee the cache won't be stale again, when _handle_sub_obj()
is called anyway.
So this is a documentation issue with Model.delete()
at best, since QuerySet.delete()
always uses "fresh" data.
by , 15 years ago
Attachment: | 7539.on_delete.r11706.diff added |
---|
comment:29 by , 15 years ago
Changes between 7539.on_delete.diff and 7539.on_delete.r11706.diff:
- Added tests.
- Replaced class based constants with callables that do what
_handle_sub_object()
did before. That cleans up_collect_sub_objects()
and allowson_delete
behaviors to be added without touching any existing code (I addedon_delete=SET(value)
, because it's just a four line utility function).
- Merged
CollectedFields
intoCollectedObjects
- Made all operations that require knowledge of internals methods on
CollectedObjects
. This makesdjango.db.models.base.delete_objects
more readable.
- Couldn't resist and rewrote parts of
CollectedObjects
.
by , 15 years ago
Attachment: | 7539.on_delete.r11724.diff added |
---|
comment:30 by , 15 years ago
Changes between 7539.on_delete.r11706.diff and 7539.on_delete.r11724.diff:
- Updated to work with m2m-refactor
- Moved
delete_objects()
insideCollectedObjects
:CollectedObjects.delete()
- Got rid of
DeleteQuery.delete_batch_related()
: use the normal code path for m2m intermediary models. This is required to enableon_delete
for foreign keys on intermediary models. But it results in a dramatical increase ofSELECT
queries. Therefore .. - Moved
_collect_sub_objects()
toCollectedObjects
and changed its algorithm to collect related objects in batches. This reduces the number ofSELECT
queries for related objects. The delete call in the following example requires a total of 103 queries in trunk, while the patch only requires 4 queries: relatedC
instances will be looked up in one bulk query.class A(models.Model): pass class B(models.Model): a = models.ForeignKey(A) class C(models.Model): b = models.ForeignKey(B) a = A.objects.create() for i in xrange(100): B.objects.create(a=a) a.delete()
- Introduced a batch (don't select, just delete) option to get rid of the one remaining extra query for
auto_created
intermediary models. - Altered
collect()
to collect inheritance parents without additional queries. - Moved
CollectedObjects
into its own module, as it now encapsulates the whole deletion logic and depends ondb.models.sql
.
TODO:
- There is an ugly
contrib.contenttypes
import insubqueries.py
. Deleting generic relations should be handled by a customon_delete
handler instead.
by , 15 years ago
Attachment: | 7539.on_delete.r11724.2.diff added |
---|
comment:31 by , 15 years ago
Changes between 7539.on_delete.r11724.diff and 7539.on_delete.r11724.2.diff:
- Corrected the default behavior for nullable foreign keys (SET_NULL did not cause any test failures for those)
- Moved
CASCADE
,PROTECT
,SET
,SET_NULL
,SET_DEFAULT
, andDO_NOTHING
todb.models.deletion
- Renamed
CollectedObjects
toCollector
- Added a couple of docstrings
- Moved sanity checks to
core.management.validation
by , 15 years ago
Attachment: | on_delete_on_update-r11733.diff added |
---|
Same as my previous patch, but including unintentionally omitted unit tests
comment:32 by , 15 years ago
Cc: | added |
---|
comment:33 by , 15 years ago
Cc: | added |
---|
comment:34 by , 15 years ago
Cc: | added |
---|
by , 15 years ago
Attachment: | 7539.on_delete.r12009.diff added |
---|
comment:35 by , 15 years ago
Cc: | added |
---|
comment:36 by , 15 years ago
Cc: | added |
---|
by , 15 years ago
Attachment: | 7539.on_delete.r12288.diff added |
---|
comment:37 by , 15 years ago
Cc: | added |
---|
comment:38 by , 15 years ago
Cc: | added |
---|
by , 15 years ago
Attachment: | 7539.on_delete.r12380.diff added |
---|
comment:39 by , 15 years ago
Cc: | added |
---|
comment:40 by , 15 years ago
Cc: | added |
---|
related: #12382
Related to this issue is the way that the admin displays delete confirmations:
such as:
Contact: John Smith Phone number: 555-1212 (work) Contact: John Smith Phone number: 555-1212 (work) Contact: John Smith Phone number: 555-1212 (work) Contact: John Smith Phone number: 555-1212 (work) Contact: John Smith
comment:41 by , 15 years ago
Cc: | added |
---|
comment:42 by , 15 years ago
Cc: | added |
---|
comment:43 by , 15 years ago
Cc: | added |
---|
comment:44 by , 15 years ago
Cc: | added |
---|
comment:45 by , 15 years ago
Cc: | added |
---|
comment:46 by , 15 years ago
Cc: | added |
---|
comment:47 by , 15 years ago
Cc: | added |
---|
comment:49 by , 15 years ago
Cc: | added |
---|
comment:50 by , 15 years ago
Cc: | added |
---|
comment:51 by , 15 years ago
Cc: | added |
---|
comment:52 by , 14 years ago
Cc: | added |
---|
comment:53 by , 14 years ago
Cc: | added |
---|
follow-up: 55 comment:54 by , 14 years ago
Cc: | added |
---|
comment:56 by , 14 years ago
Cc: | added |
---|
follow-up: 58 comment:57 by , 14 years ago
Cc: | added |
---|
I tried to apply the patch to revision 14140, but there were quite some failures. Is it asked much to post a new patch against the latest version? Here are the error messages I got:
L:\snapshots\django>patch -p0 < 7539.on_delete.r12380.diff patching file tests/modeltests/invalid_models/models.py Hunk #1 FAILED at 181. Hunk #2 FAILED at 285. 2 out of 2 hunks FAILED -- saving rejects to file tests/modeltests/invalid_models/models.py.rej patching file tests/modeltests/delete/models.py Hunk #1 FAILED at 44. Hunk #2 FAILED at 79. Hunk #3 FAILED at 93. Hunk #4 FAILED at 107. Hunk #5 FAILED at 128. 5 out of 5 hunks FAILED -- saving rejects to file tests/modeltests/delete/models.py.rej patching file tests/modeltests/on_delete/__init__.py patching file tests/modeltests/on_delete/models.py patching file django/db/models/sql/subqueries.py Hunk #1 FAILED at 26. Hunk #2 succeeded at 45 (offset -43 lines). 1 out of 2 hunks FAILED -- saving rejects to file django/db/models/sql/subqueries.py.rej patching file django/db/models/base.py Hunk #1 FAILED at 7. Hunk #2 FAILED at 536. 2 out of 2 hunks FAILED -- saving rejects to file django/db/models/base.py.rej patching file django/db/models/options.py Hunk #1 succeeded at 370 (offset -6 lines). patching file django/db/models/fields/related.py Hunk #2 succeeded at 734 (offset 46 lines). Hunk #3 succeeded at 744 (offset 46 lines). Hunk #4 succeeded at 764 (offset 46 lines). Hunk #5 succeeded at 820 with fuzz 1 (offset 49 lines). patching file django/db/models/__init__.py patching file django/db/models/deletion.py patching file django/db/models/query.py
comment:58 by , 14 years ago
Patch needs improvement: | set |
---|
Replying to lsaffre:
I tried to apply the patch to revision 14140, but there were quite some failures. Is it asked much to post a new patch against the latest version?
I ported the patch to r14218. Most of the changes that broke the old code were introduced by the multi-db refactoring. It works now (as in: the tests pass), but I haven't added explicit tests for on_delete behavior in the presence of multiple databases.
And there's a whole new problem caused by a a piece of code with a TODO comment in contrib.admin. I haven't bothered to fix the test failures that are related to this dependency.
Feedback welcome.
by , 14 years ago
Attachment: | 7539.on_delete.r14218.diff added |
---|
comment:59 by , 14 years ago
comment:60 by , 14 years ago
Do those admin tests pass with the latest code (I'll be looking at reviewing this and I know carl meyer is as well)?
comment:61 by , 14 years ago
Triage Stage: | Design decision needed → Accepted |
---|
Looks like the admin code can be updated to use the new API without too much difficulty. We may also want to add an additional warning in the admin regarding objects that won't be deleted, but will be modified (have an FK nulled out or set to some other value).
by , 14 years ago
Attachment: | 7539.on_delete.r14291.diff added |
---|
comment:62 by , 14 years ago
Owner: | set to |
---|
Done some work on this patch, more to be done yet (see remaining FIXME comments in deletion.py). Work in progress here: http://github.com/carljm/django/compare/master...t7539
comment:63 by , 14 years ago
The github branch linked in my last comment passes all tests and, once I get some docs in it, could be RFC.
What it still lacks is support for GenericForeignKey / GenericRelation; these still have hardcoded cascade-delete behavior, as they did previously. I could be open to landing this without GFK support: there's no regression in GFK behavior, they just don't get the new cascade-delete configurability.
I do have some working proof-of-concept support for GFKs in a separate branch (http://github.com/carljm/django/compare/t7539...t7539-gfk), which also passes all tests (including a new one demonstrating on_delete=SET_NULL for a GFK), but I'm not entirely happy with the API. It requires GFK on-deletes to be selected from a separate set of functions (found in django.contrib.contenttypes.generic) from the ones used for regular FKs (found in django.db.models). This may be OK, but seems prone to user error (and would certainly need some added validation code to make the error at least intelligible).
I think it would be possible to allow GFK on_delete to use the same set of behavior-functions as regular FK on_delete, but it would require a significant refactor of the patch. The approach I envision would push some key chunks of logic (e.g. bulk-fetching related objects, what it means to update a field) into methods on RelatedObject/GenericRelation, so the stuff in models/deletion.py can be agnostic about what kind of relation it's dealing with.
Feedback welcome. One way or the other, I'm hoping to land this in the next week or two (with a wider call for feedback on django-developers first).
comment:64 by , 14 years ago
comment:65 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [14507]) Fixed #7539, #13067 -- Added on_delete argument to ForeignKey to control cascade behavior. Also refactored deletion for efficiency and code clarity. Many thanks to Johannes Dollinger and Michael Glassford for extensive work on the patch, and to Alex Gaynor, Russell Keith-Magee, and Jacob Kaplan-Moss for review.
comment:66 by , 13 years ago
Cc: | removed |
---|---|
Easy pickings: | unset |
Severity: | → Normal |
Type: | → Uncategorized |
UI/UX: | unset |
Can not apply the patch to latest django svn trunk (admin newforms merged).
I've manually modified the files, but get the following error when trying to run the application: