Refs #21961 : Added support for database-level cascading options #16851
Conversation
Akash-Kumar-Sen
force-pushed
the
ticket_21961
branch
from
0d5642c to
cd94443
Compare
Akash-Kumar-Sen
changed the title
There was a problem hiding this comment.
@Akash-Kumar-Sen Thanks for this patch 👍 However, I found this proposition very confusing, you introduced a second parameter on_delete_db and a constant DB_CASCADE which does nothing but allow on_delete_db to be set, so DB_CASCADE has nothing to do with cascade deletion. Also, having two arguments creates sync issues. I don't see much value in the two arguments, especially since they are mutually exclusive.
Personally, I prefer Nick's proposition to introduce DB_* options and support them in on_delete.
|
Having it in a single |
Co-authored-by: Adam Johnson <me@adamj.eu>
Co-authored-by: Adam Johnson <me@adamj.eu>
|
Anything blocking this from getting approved? |
docs/ref/checks.txt
Outdated
| * **fields.E322**: The field specifies unsupported database level ``on_delete`` | ||
| on model declaring a ``GenericForeignKey``. | ||
| * **fields.E323**: Using python based on_delete with database level | ||
| ``on_delete`` referenced parent model is prohibited. | ||
| * **fields.E324**: The field specifies ``on_delete=DB_SET_DEFAULT``, but has no | ||
| ``db_default`` value. |
There was a problem hiding this comment.
Documented error messages should match the error messages raised.
| @@ -1579,6 +1582,12 @@ def _rename_index_sql(self, model, old_name, new_name): | |||
| new_name=self.quote_name(new_name), | |||
| ) | |||
|
|
|||
| def _create_on_delete_sql(self, model, field): | |||
| on_delete = getattr(field.remote_field, "on_delete", None) | |||
There was a problem hiding this comment.
on_delete is required, do we need to use getattr()?
There was a problem hiding this comment.
Sometimes field.remote_field can be None, getting the following error when tried without getattr
ERROR: test_remove_constraints_capital_letters (schema.tests.SchemaTests)
#23065 - Constraint names must be quoted if they contain capital letters.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python3.10/unittest/case.py", line 59, in testPartExecutor
yield
File "/usr/lib/python3.10/unittest/case.py", line 591, in run
self._callTestMethod(testMethod)
File "/usr/lib/python3.10/unittest/case.py", line 549, in _callTestMethod
method()
File "/home/akash/Djangocon/django/tests/schema/tests.py", line 4474, in test_remove_constraints_capital_letters
"on_delete_db": editor._create_on_delete_sql(model, field),
File "/home/akash/Djangocon/django/django/db/backends/base/schema.py", line 1590, in _create_on_delete_sql
on_delete = field.remote_field.on_delete
AttributeError: 'NoneType' object has no attribute 'on_delete'
|
I just arrived here from the original 10-year-old ticket and am blown away by how far this work has gotten now. I'm looking forward to the feature and am grateful for the work in this PR. 🙏 Good luck with what seems like minor requests from felixxm, @Akash-Kumar-Sen! |
Co-authored-by: Lily Foote <code@lilyf.org>
| @@ -241,3 +241,99 @@ class GenericDeleteBottomParent(models.Model): | |||
| generic_delete_bottom = models.ForeignKey( | |||
| GenericDeleteBottom, on_delete=models.CASCADE | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
Hi Akash 👋 thank you for being patient, I have started to test this PR.
In this file there is a model called A which has every on_delete possible except all of these new ones.
This is actually quite a similar approach to how I am testing in my local project.
It might make sense to review these tests with the model A and have each recreated with the DB delete counterparts when this is not already covered by existing tests.
For example, I think there might be an issue with the DB_RESTICT and DB_CASCADE relationship and we can add a test for very similar to delete.tests.OnDeleteTests.test_restrict_path_cascade_direct.
Here:
diff --git a/tests/delete/models.py b/tests/delete/models.py
index 5a3858d7ad..33f8094b6c 100644
--- a/tests/delete/models.py
+++ b/tests/delete/models.py
@@ -58,6 +58,7 @@ class A(models.Model):
related_name="setnull_nullable_set",
)
cascade = models.ForeignKey(R, models.CASCADE, related_name="cascade_set")
+ db_cascade = models.ForeignKey(R, models.DB_CASCADE, related_name="db_cascade_set")
cascade_nullable = models.ForeignKey(
R, models.CASCADE, null=True, related_name="cascade_nullable_set"
)
@@ -67,6 +68,9 @@ class A(models.Model):
restrict = models.ForeignKey(
R, models.RESTRICT, null=True, related_name="restrict_set"
)
+ db_restrict = models.ForeignKey(
+ R, models.DB_RESTRICT, null=True, related_name="db_restrict_set"
+ )
donothing = models.ForeignKey(
R, models.DO_NOTHING, null=True, related_name="donothing_set"
)
@@ -100,9 +104,11 @@ def create_a(name):
"setdefault",
"setdefault_none",
"cascade",
+ "db_cascade",
"cascade_nullable",
"protect",
"restrict",
+ "db_restrict",
"donothing",
"o2o_setnull",
):
diff --git a/tests/delete/tests.py b/tests/delete/tests.py
index e9a1c09240..bd6d671b2f 100644
--- a/tests/delete/tests.py
+++ b/tests/delete/tests.py
@@ -271,6 +271,26 @@ class OnDeleteTests(TestCase):
self.assertFalse(A.objects.filter(name="restrict").exists())
self.assertFalse(R.objects.filter(pk=a.restrict_id).exists())
+ def test_db_restrict_path_db_cascade_direct(self):
+ a = create_a("db_restrict")
+ a.db_restrict.p = P.objects.create()
+ a.db_restrict.save()
+ a.db_cascade_p = a.db_restrict.p
+ a.save()
+ a.db_restrict.p.delete()
+ self.assertFalse(A.objects.filter(name="db_restrict").exists())
+ self.assertFalse(R.objects.filter(pk=a.db_restrict_id).exists())
+
+ def test_db_restrict_path_cascade_direct(self):
+ a = create_a("db_restrict")
+ a.db_restrict.p = P.objects.create()
+ a.db_restrict.save()
+ a.cascade_p = a.db_restrict.p
+ a.save()
+ a.db_restrict.p.delete()
+ self.assertFalse(A.objects.filter(name="db_restrict").exists())
+ self.assertFalse(R.objects.filter(pk=a.db_restrict_id).exists())
+
def test_restrict_path_cascade_indirect_diamond(self):
delete_top = DeleteTop.objects.create()
b1 = B1.objects.create(delete_top=delete_top)The test test_db_restrict_path_db_cascade_direct fails with this error on SQLite:
ERROR: test_db_restrict_path_db_cascade_direct (delete.tests.OnDeleteTests.test_db_restrict_path_db_cascade_direct)
----------------------------------------------------------------------
Traceback (most recent call last):
File "C:\Users\boyce\OneDrive\Documents\GitHub\django\django\db\backends\utils.py", line 105, in _execute
return self.cursor.execute(sql, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\boyce\OneDrive\Documents\GitHub\django\django\db\backends\sqlite3\base.py", line 354, in execute
return super().execute(query, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sqlite3.IntegrityError: FOREIGN KEY constraint failed
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "<path_to_django>\tests\delete\tests.py", line 280, in test_db_restrict_path_cascade_direct
a.db_restrict.p.delete()
File "<path_to_django>\django\db\models\base.py", line 1249, in delete
return collector.delete()
^^^^^^^^^^^^^^^^^^
File "<path_to_django>\django\db\models\deletion.py", line 530, in delete
count = query.delete_batch(pk_list, self.using)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<path_to_django>\django\db\models\sql\subqueries.py", line 42, in delete_batch
num_deleted += self.do_query(
^^^^^^^^^^^^^^
File "<path_to_django>\django\db\models\sql\subqueries.py", line 20, in do_query
cursor = self.get_compiler(using).execute_sql(CURSOR)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<path_to_django>\django\db\models\sql\compiler.py", line 1570, in execute_sql
cursor.execute(sql, params)
File "<path_to_django>\django\db\backends\utils.py", line 79, in execute
return self._execute_with_wrappers(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<path_to_django>\django\db\backends\utils.py", line 92, in _execute_with_wrappers
return executor(sql, params, many, context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<path_to_django>\django\db\backends\utils.py", line 100, in _execute
with self.db.wrap_database_errors:
File "<path_to_django>\django\db\utils.py", line 91, in __exit__
raise dj_exc_value.with_traceback(traceback) from exc_value
File "<path_to_django>\django\db\backends\utils.py", line 105, in _execute
return self.cursor.execute(sql, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<path_to_django>\django\db\backends\sqlite3\base.py", line 354, in execute
return super().execute(query, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.IntegrityError: FOREIGN KEY constraint failed
----------------------------------------------------------------------
But test_db_restrict_path_cascade_direct passes. I think I would expect the behaviour to be the same and that they would both pass.
Can you confirm this?
There was a problem hiding this comment.
Thank you for the review @sarahboyce , I think you forgot to add the db_cascade_p field in the model A, Updated the code with suggested tests - https://github.com/django/django/pull/16851/files#diff-a842e4474a8c70b274fabec1b44e377a498181dfe6c925801b7dc930917c3e03R324-R343
There was a problem hiding this comment.
You're completely right thank you ⭐
There was a problem hiding this comment.
Looked into it again. I still think there's something going on and these tests (might) show it:
diff --git a/tests/delete/tests.py b/tests/delete/tests.py
index f84831dea7..5b08357f4d 100644
--- a/tests/delete/tests.py
+++ b/tests/delete/tests.py
@@ -331,6 +331,22 @@ class OnDeleteTests(TestCase):
self.assertFalse(A.objects.filter(name="db_restrict").exists())
self.assertFalse(R.objects.filter(pk=a.db_restrict_id).exists())
+ def test_db_restrict_db_cascade_direct(self):
+ a = create_a("db_restrict")
+ a.db_cascade = a.db_restrict
+ a.save()
+ a.db_restrict.delete()
+ self.assertFalse(A.objects.filter(name="db_restrict").exists())
+ self.assertFalse(R.objects.filter(pk=a.db_restrict_id).exists())
+
+ def test_db_restrict_cascade_direct(self):
+ a = create_a("db_restrict")
+ a.cascade = a.db_restrict
+ a.save()
+ a.db_restrict.delete()
+ self.assertFalse(A.objects.filter(name="db_restrict").exists())
+ self.assertFalse(R.objects.filter(pk=a.db_restrict_id).exists())
+
def test_db_restrict_path_cascade_direct(self):
a = create_a("db_restrict")
a.db_restrict.p = P.objects.create()I have the FK error with the test_db_restrict_db_cascade_direct test but not test_db_restrict_cascade_direct
In my local project I have:
models.py
from django.db import models
class Basic(models.Model):
name = models.CharField(max_length=30)
class EveryOnDelete(models.Model):
fk_cascade = models.ForeignKey(
Basic,
on_delete=models.CASCADE,
null=True,
related_name="fk_cascade",
)
fk_db_cascade = models.ForeignKey(
Basic,
on_delete=models.DB_CASCADE,
null=True,
related_name="fk_db_cascade",
)
fk_set_null = models.ForeignKey(
Basic,
on_delete=models.SET_NULL,
null=True,
related_name="fk_set_null",
)
fk_db_set_null = models.ForeignKey(
Basic,
on_delete=models.DB_SET_NULL,
null=True,
related_name="fk_db_set_null",
)
fk_set_default = models.ForeignKey(
Basic,
on_delete=models.SET_DEFAULT,
default="1",
null=True,
related_name="fk_set_default",
)
fk_db_set_default = models.ForeignKey(
Basic,
on_delete=models.DB_SET_DEFAULT,
null=True,
db_default="1",
related_name="fk_db_set_default",
)
fk_restrict = models.ForeignKey(
Basic,
on_delete=models.RESTRICT,
null=True,
related_name="fk_restrict",
)
fk_db_restrict = models.ForeignKey(
Basic,
on_delete=models.DB_RESTRICT,
null=True,
related_name="fk_db_restrict",
)
fk_protect = models.ForeignKey(
Basic,
on_delete=models.PROTECT,
null=True,
related_name="fk_protect",
)and some tests
from django.test import TestCase
from .models import Basic, EveryOnDelete
class DBDeleteTests(TestCase):
@classmethod
def setUpTestData(cls):
Basic.objects.create(name="id-1", id="1")
# Removed some to make this smaller
def test_db_fk_restrict_basic_deleted_allowed_cascade(self):
basic_1 = Basic.objects.create(name="not-protected")
EveryOnDelete.objects.create(
fk_db_restrict=basic_1,
fk_cascade=basic_1,
)
basic_1.delete()
self.assertEqual(Basic.objects.count(), 1)
self.assertEqual(EveryOnDelete.objects.count(), 0)
def test_db_fk_restrict_basic_deleted_allowed_db_cascade(self):
basic_1 = Basic.objects.create(name="not-protected")
EveryOnDelete.objects.create(
fk_db_cascade=basic_1,
fk_db_restrict=basic_1,
)
basic_1.delete()
self.assertEqual(Basic.objects.count(), 1)
self.assertEqual(EveryOnDelete.objects.count(), 0)Here test_db_fk_restrict_basic_deleted_allowed_db_cascade is failing with the FK error, I think it should pass rather than raise an IntegrityError?
There was a problem hiding this comment.
Got some time this week after a long rush, will look into this. Thank you for finding it out @sarahboyce
Akash-Kumar-Sen
force-pushed
the
ticket_21961
branch
from
914f3c1 to
ffdc28d
Compare
Akash-Kumar-Sen
force-pushed
the
ticket_21961
branch
from
3956da9 to
928b33a
Compare
|
Looking forward for this go out, any update on this? |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Excited to see this merged! We have a full-stack Next app using Supabase and Django for migrations/services and the fact that we can't enforce foreign key references at the database layer means that Django's migrations are lossy. All in all Django >>> every NodeJS migration system (so 100x thank yous to the team) but without things like this interop becomes really hard. |
Ticket : https://code.djangoproject.com/ticket/21961
Following PR : #14550