Code

Opened 6 years ago

Closed 3 years ago

Last modified 2 years ago

#7539 closed Uncategorized (fixed)

Add ON DELETE and ON UPDATE support to Django

Reported by: glassfordm Owned by: carljm
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: feature
Cc: miracle2k, mathijs@…, matthubb, diegobz, road, arikon, marinho, me@…, findepi, gabor@…, dnordberg@…, ehs@…, jtiai, hanne.moa@…, aaron@…, groby@…, semenov, ales.zoulek@…, plandry@…, lukasz.korzybski@…, preston@…, eschler@…, paluho@…, gorsky.exe@…, ego@…, gonz, 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.

  1. 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).

  1. 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).

  1. 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)

on_delete_on_update.diff (39.2 KB) - added by glassfordm 5 years ago.
Updated patch for the latest version of Django (revision 9845 at the time the patch was created).
on_delete_on_update-r10558.diff (20.9 KB) - added by dokterbob 5 years ago.
Patch updated for r10558. Very fresh and barely tested, see more info in ticket.
on_delete_on_update-r11620.diff.txt (21.0 KB) - added by glassfordm 5 years ago.
Very preliminary patch for 1.2 release
on_delete_on_update-r11620.diff (21.0 KB) - added by glassfordm 5 years ago.
Same as previous patch, but with correct extension
7539.on_delete.diff (18.2 KB) - added by emulbreh 4 years ago.
7539.on_delete.r11706.diff (26.3 KB) - added by emulbreh 4 years ago.
7539.on_delete.r11724.diff (42.0 KB) - added by emulbreh 4 years ago.
7539.on_delete.r11724.2.diff (45.8 KB) - added by emulbreh 4 years ago.
on_delete_on_update-r11733.diff (45.7 KB) - added by glassfordm 4 years ago.
Same as my previous patch, but including unintentionally omitted unit tests
7539.on_delete.r12009.diff (45.8 KB) - added by emulbreh 4 years ago.
7539.on_delete.r12288.diff (45.8 KB) - added by emulbreh 4 years ago.
7539.on_delete.r12380.diff (46.1 KB) - added by emulbreh 4 years ago.
7539.on_delete.r14218.diff (47.6 KB) - added by emulbreh 4 years ago.
7539.on_delete.r14291.diff (50.7 KB) - added by emulbreh 3 years ago.

Download all attachments as: .zip

Change History (80)

comment:1 Changed 6 years ago by edgarsj

  • Component changed from Uncategorized to Database wrapper
  • Keywords feature added
  • milestone set to post-1.0
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 6 years ago by anonymous

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:

 File "/usr/lib/python2.5/site-packages/django/db/models/fields/related.py", line 596, in __init__
    self.on_delete = on_delete
NameError: global name 'on_delete' is not defined

Changed 5 years ago by glassfordm

Updated patch for the latest version of Django (revision 9845 at the time the patch was created).

comment:3 Changed 5 years ago by miracle2k

  • Cc miracle2k added

comment:4 Changed 5 years ago by ikelly

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 Changed 5 years ago by glassfordm

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 Changed 5 years ago by ikelly

"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:7 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:8 Changed 5 years ago by guettli

  • Cc hv@… added

comment:9 Changed 5 years ago by Dokter Bob

  • Cc mathijs@… added

Has anyone been working on this bug? I would very much like to see a current patch for it.

comment:10 Changed 5 years ago by russellm

@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 Changed 5 years ago by anonymous

  • Cc matthubb added

comment:12 Changed 5 years ago by diegobz

  • Cc diegobz added

Changed 5 years ago by dokterbob

Patch updated for r10558. Very fresh and barely tested, see more info in ticket.

comment:13 Changed 5 years ago by dokterbob

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 Changed 5 years ago by benjaoming

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 Changed 5 years ago by mtredinnick

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 Changed 5 years ago by benjaoming

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 Changed 5 years ago by road

  • Cc road added

comment:18 Changed 5 years ago by arikon

  • Cc arikon added

comment:19 Changed 5 years ago by guettli

Related: #6108

comment:20 Changed 5 years ago by marinho

  • Cc marinho added

comment:21 Changed 5 years ago by daybreaker

  • Cc me@… added

I'm also interested on this issue.

comment:22 Changed 5 years ago by findepi

  • Cc findepi added
  • Owner changed from nobody to findepi
  • Status changed from new to assigned

comment:23 Changed 5 years ago by findepi

  • Owner findepi deleted
  • Status changed from assigned to new

comment:24 Changed 5 years ago by gabor

  • Cc gabor@… added

comment:25 Changed 5 years ago by anonymous

  • Cc dnordberg@… added

comment:26 Changed 5 years ago by jtiai

  • Cc jtiai added

Changed 5 years ago by glassfordm

Very preliminary patch for 1.2 release

comment:27 Changed 5 years ago by glassfordm

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.

Changed 5 years ago by glassfordm

Same as previous patch, but with correct extension

Changed 4 years ago by emulbreh

comment:28 Changed 4 years ago by anonymous

@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 and null in _handle_sub_obj() (as these are static properties of fields and already checked in contribute_to_class()). These checks should perhaps be moved to django.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 in CollectedObjects, 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.

Changed 4 years ago by emulbreh

comment:29 Changed 4 years ago by emulbreh

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 allows on_delete behaviors to be added without touching any existing code (I added on_delete=SET(value), because it's just a four line utility function).


  • Merged CollectedFields into CollectedObjects
  • Made all operations that require knowledge of internals methods on CollectedObjects. This makes django.db.models.base.delete_objects more readable.
  • Couldn't resist and rewrote parts of CollectedObjects.

Changed 4 years ago by emulbreh

comment:30 Changed 4 years ago by emulbreh

Changes between 7539.on_delete.r11706.diff and 7539.on_delete.r11724.diff:

  • Updated to work with m2m-refactor
  • Moved delete_objects() inside CollectedObjects: CollectedObjects.delete()
  • Got rid of DeleteQuery.delete_batch_related(): use the normal code path for m2m intermediary models. This is required to enable on_delete for foreign keys on intermediary models. But it results in a dramatical increase of SELECT queries. Therefore ..
  • Moved _collect_sub_objects() to CollectedObjects and changed its algorithm to collect related objects in batches. This reduces the number of SELECT 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: related C 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 on db.models.sql.

TODO:

  • There is an ugly contrib.contenttypes import in subqueries.py. Deleting generic relations should be handled by a custom on_delete handler instead.

Changed 4 years ago by emulbreh

comment:31 Changed 4 years ago by emulbreh

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, and DO_NOTHING to db.models.deletion
  • Renamed CollectedObjects to Collector
  • Added a couple of docstrings
  • Moved sanity checks to core.management.validation

Changed 4 years ago by glassfordm

Same as my previous patch, but including unintentionally omitted unit tests

comment:32 Changed 4 years ago by edsu

  • Cc ehs@… added

comment:33 Changed 4 years ago by HM

  • Cc hanne.moa@… added

comment:34 Changed 4 years ago by darkpixel

  • Cc aaron@… added

Changed 4 years ago by emulbreh

comment:35 Changed 4 years ago by groby

  • Cc groby@… added

comment:36 Changed 4 years ago by semenov

  • Cc semenov added

Changed 4 years ago by emulbreh

comment:37 Changed 4 years ago by anonymous

  • Cc ales.zoulek@… added

comment:38 Changed 4 years ago by anonymous

  • Cc plandry@… added

Changed 4 years ago by emulbreh

comment:39 Changed 4 years ago by naos

  • Cc lukasz.korzybski@… added

comment:40 Changed 4 years ago by anonymous

  • Cc preston@… 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 Changed 4 years ago by anonymous

  • Cc eschler@… added

comment:42 Changed 4 years ago by paluh

  • Cc paluho@… added

comment:43 Changed 4 years ago by gorsky

  • Cc gorsky.exe@… added

comment:44 Changed 4 years ago by anentropic

  • Cc ego@… added

comment:45 Changed 4 years ago by gonz

  • Cc gonz added

comment:46 Changed 4 years ago by dzida

  • Cc l.dziedzia@… added

comment:47 Changed 4 years ago by cgieringer

  • Cc cgieringer added

comment:48 Changed 4 years ago by cgieringer

Related: #6870

comment:49 Changed 4 years ago by Vlada Macek <macek@…>

  • Cc t.django@… added

comment:50 Changed 4 years ago by David Danier <david.danier@…>

  • Cc goliath.mailinglist@… added

comment:51 Changed 4 years ago by jdunck

  • Cc jdunck@… added

comment:52 Changed 4 years ago by mattlong

  • Cc mlong@… added

comment:53 Changed 4 years ago by erikrose

  • Cc erikrose, paulc@… added

comment:54 follow-up: Changed 4 years ago by jakub

  • Cc jakub@… added

comment:55 in reply to: ↑ 54 Changed 4 years ago by DavidPratten

  • Cc david@… added

Replying to jakub:

comment:56 Changed 4 years ago by rubic

  • Cc jbauer@… added

comment:57 follow-up: Changed 4 years ago by lsaffre

  • Cc luc.saffre@… 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 in reply to: ↑ 57 Changed 4 years ago by emulbreh

  • 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.

Changed 4 years ago by emulbreh

comment:59 Changed 4 years ago by emulbreh

For reference: r12598 introduced the offending contrib.admin code to fix #6191 and #11296.

comment:60 Changed 4 years ago by Alex

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 Changed 4 years ago by carljm

  • Triage Stage changed from Design decision needed to 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).

Changed 3 years ago by emulbreh

comment:62 Changed 3 years ago by carljm

  • Owner set to carljm

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 Changed 3 years ago by carljm

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 Changed 3 years ago by carljm

The current patch here also fixes #13067 and #14599. Just so I don't forget.

comment:65 Changed 3 years ago by carljm

  • Resolution set to fixed
  • Status changed from new to 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 Changed 2 years ago by guettli

  • Cc hv@… removed
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.