From 2116238d5f2ce19571becb5620ef16ce9048db9f Mon Sep 17 00:00:00 2001 From: Bernd Wechner Date: Wed, 22 Sep 2021 15:53:10 +1000 Subject: [PATCH] Fixed #33131 -- Improved error messages for clashing reverse accessor names. --- django/db/models/fields/related.py | 8 +- docs/ref/checks.txt | 10 +- .../test_relative_fields.py | 109 +++++++++--------- .../test_abstract_inheritance.py | 5 +- 4 files changed, 72 insertions(+), 60 deletions(-) diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 8070457088..2e8c87c1b1 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -239,7 +239,9 @@ class RelatedField(FieldCacheMixin, Field): if not rel_is_hidden and clash_field.name == rel_name: errors.append( checks.Error( - "Reverse accessor for '%s' clashes with field name '%s'." % (field_name, clash_name), + f"Reverse accessor '{rel_opts.object_name}.{rel_name}' " + f"for '{field_name}' clashes with field name " + f"'{clash_name}'.", hint=("Rename field '%s', or add/change a related_name " "argument to the definition for field '%s'.") % (clash_name, field_name), obj=self, @@ -271,7 +273,9 @@ class RelatedField(FieldCacheMixin, Field): if not rel_is_hidden and clash_field.get_accessor_name() == rel_name: errors.append( checks.Error( - "Reverse accessor for '%s' clashes with reverse accessor for '%s'." % (field_name, clash_name), + f"Reverse accessor '{rel_opts.object_name}.{rel_name}' " + f"for '{field_name}' clashes with reverse accessor for " + f"'{clash_name}'.", hint=("Add or change a related_name argument " "to the definition for '%s' or '%s'.") % (field_name, clash_name), obj=self, diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index fe8ed6bc45..727d16b772 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -247,12 +247,14 @@ Related fields either not installed, or is abstract. * **fields.E301**: Field defines a relation with the model ``.`` which has been swapped out. -* **fields.E302**: Reverse accessor for ``..`` - clashes with field name ``..``. +* **fields.E302**: Reverse accessor ``.`` for + ``..`` clashes with field name + ``..``. * **fields.E303**: Reverse query name for ``..`` clashes with field name ``..``. -* **fields.E304**: Reverse accessor for ``..`` - clashes with reverse accessor for ``..``. +* **fields.E304**: Reverse accessor ``.`` for + ``..`` clashes with reverse accessor for + ``..``. * **fields.E305**: Reverse query name for ``..`` clashes with reverse query name for ``..``. * **fields.E306**: The name ```` is invalid ``related_name`` for field diff --git a/tests/invalid_models_tests/test_relative_fields.py b/tests/invalid_models_tests/test_relative_fields.py index 6e885f1705..438d1b2a45 100644 --- a/tests/invalid_models_tests/test_relative_fields.py +++ b/tests/invalid_models_tests/test_relative_fields.py @@ -862,8 +862,8 @@ class AccessorClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.rel' " - "clashes with field name " + "Reverse accessor 'Target.model_set' for " + "'invalid_models_tests.Model.rel' clashes with field name " "'invalid_models_tests.Target.model_set'.", hint=( "Rename field 'invalid_models_tests.Target.model_set', or " @@ -885,9 +885,9 @@ class AccessorClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.foreign' " - "clashes with reverse accessor for " - "'invalid_models_tests.Model.m2m'.", + "Reverse accessor 'Target.model_set' for " + "'invalid_models_tests.Model.foreign' clashes with reverse " + "accessor for 'invalid_models_tests.Model.m2m'.", hint=( "Add or change a related_name argument to the definition " "for 'invalid_models_tests.Model.foreign' or " @@ -897,9 +897,9 @@ class AccessorClashTests(SimpleTestCase): id='fields.E304', ), Error( - "Reverse accessor for 'invalid_models_tests.Model.m2m' " - "clashes with reverse accessor for " - "'invalid_models_tests.Model.foreign'.", + "Reverse accessor 'Target.model_set' for " + "'invalid_models_tests.Model.m2m' clashes with reverse " + "accessor for 'invalid_models_tests.Model.foreign'.", hint=( "Add or change a related_name argument to the definition " "for 'invalid_models_tests.Model.m2m' or " @@ -927,9 +927,9 @@ class AccessorClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.children' " - "clashes with field name " - "'invalid_models_tests.Child.m2m_clash'.", + "Reverse accessor 'Child.m2m_clash' for " + "'invalid_models_tests.Model.children' clashes with field " + "name 'invalid_models_tests.Child.m2m_clash'.", hint=( "Rename field 'invalid_models_tests.Child.m2m_clash', or " "add/change a related_name argument to the definition for " @@ -1085,8 +1085,9 @@ class ExplicitRelatedNameClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.rel' " - "clashes with field name 'invalid_models_tests.Target.clash'.", + "Reverse accessor 'Target.clash' for " + "'invalid_models_tests.Model.rel' clashes with field name " + "'invalid_models_tests.Target.clash'.", hint=( "Rename field 'invalid_models_tests.Target.clash', or " "add/change a related_name argument to the definition for " @@ -1218,9 +1219,9 @@ class SelfReferentialM2MClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.first_m2m' " - "clashes with reverse accessor for " - "'invalid_models_tests.Model.second_m2m'.", + "Reverse accessor 'Model.model_set' for " + "'invalid_models_tests.Model.first_m2m' clashes with reverse " + "accessor for 'invalid_models_tests.Model.second_m2m'.", hint=( "Add or change a related_name argument to the definition " "for 'invalid_models_tests.Model.first_m2m' or " @@ -1230,9 +1231,9 @@ class SelfReferentialM2MClashTests(SimpleTestCase): id='fields.E304', ), Error( - "Reverse accessor for 'invalid_models_tests.Model.second_m2m' " - "clashes with reverse accessor for " - "'invalid_models_tests.Model.first_m2m'.", + "Reverse accessor 'Model.model_set' for " + "'invalid_models_tests.Model.second_m2m' clashes with reverse " + "accessor for 'invalid_models_tests.Model.first_m2m'.", hint=( "Add or change a related_name argument to the definition " "for 'invalid_models_tests.Model.second_m2m' or " @@ -1249,9 +1250,9 @@ class SelfReferentialM2MClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.model_set' " - "clashes with field name " - "'invalid_models_tests.Model.model_set'.", + "Reverse accessor 'Model.model_set' for " + "'invalid_models_tests.Model.model_set' clashes with field " + "name 'invalid_models_tests.Model.model_set'.", hint=( "Rename field 'invalid_models_tests.Model.model_set', or " "add/change a related_name argument to the definition for " @@ -1287,8 +1288,9 @@ class SelfReferentialM2MClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.m2m' " - "clashes with field name 'invalid_models_tests.Model.clash'.", + "Reverse accessor 'Model.clash' for " + "'invalid_models_tests.Model.m2m' clashes with field name " + "'invalid_models_tests.Model.clash'.", hint=( "Rename field 'invalid_models_tests.Model.clash', or " "add/change a related_name argument to the definition for " @@ -1327,9 +1329,9 @@ class SelfReferentialFKClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.model_set' " - "clashes with field name " - "'invalid_models_tests.Model.model_set'.", + "Reverse accessor 'Model.model_set' for " + "'invalid_models_tests.Model.model_set' clashes with field " + "name 'invalid_models_tests.Model.model_set'.", hint=( "Rename field 'invalid_models_tests.Model.model_set', or " "add/change a related_name argument to the definition for " @@ -1365,8 +1367,9 @@ class SelfReferentialFKClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.foreign' " - "clashes with field name 'invalid_models_tests.Model.clash'.", + "Reverse accessor 'Model.clash' for " + "'invalid_models_tests.Model.foreign' clashes with field name " + "'invalid_models_tests.Model.clash'.", hint=( "Rename field 'invalid_models_tests.Model.clash', or " "add/change a related_name argument to the definition for " @@ -1413,8 +1416,9 @@ class ComplexClashTests(SimpleTestCase): self.assertEqual(Model.check(), [ Error( - "Reverse accessor for 'invalid_models_tests.Model.foreign_1' " - "clashes with field name 'invalid_models_tests.Target.id'.", + "Reverse accessor 'Target.id' for " + "'invalid_models_tests.Model.foreign_1' clashes with field " + "name 'invalid_models_tests.Target.id'.", hint=( "Rename field 'invalid_models_tests.Target.id', or " "add/change a related_name argument to the definition for " @@ -1435,9 +1439,9 @@ class ComplexClashTests(SimpleTestCase): id='fields.E303', ), Error( - "Reverse accessor for 'invalid_models_tests.Model.foreign_1' " - "clashes with reverse accessor for " - "'invalid_models_tests.Model.m2m_1'.", + "Reverse accessor 'Target.id' for " + "'invalid_models_tests.Model.foreign_1' clashes with reverse " + "accessor for 'invalid_models_tests.Model.m2m_1'.", hint=( "Add or change a related_name argument to the definition " "for 'invalid_models_tests.Model.foreign_1' or " @@ -1460,9 +1464,9 @@ class ComplexClashTests(SimpleTestCase): ), Error( - "Reverse accessor for 'invalid_models_tests.Model.foreign_2' " - "clashes with reverse accessor for " - "'invalid_models_tests.Model.m2m_2'.", + "Reverse accessor 'Target.src_safe' for " + "'invalid_models_tests.Model.foreign_2' clashes with reverse " + "accessor for 'invalid_models_tests.Model.m2m_2'.", hint=( "Add or change a related_name argument to the definition " "for 'invalid_models_tests.Model.foreign_2' or " @@ -1485,8 +1489,9 @@ class ComplexClashTests(SimpleTestCase): ), Error( - "Reverse accessor for 'invalid_models_tests.Model.m2m_1' " - "clashes with field name 'invalid_models_tests.Target.id'.", + "Reverse accessor 'Target.id' for " + "'invalid_models_tests.Model.m2m_1' clashes with field name " + "'invalid_models_tests.Target.id'.", hint=( "Rename field 'invalid_models_tests.Target.id', or " "add/change a related_name argument to the definition for " @@ -1507,9 +1512,9 @@ class ComplexClashTests(SimpleTestCase): id='fields.E303', ), Error( - "Reverse accessor for 'invalid_models_tests.Model.m2m_1' " - "clashes with reverse accessor for " - "'invalid_models_tests.Model.foreign_1'.", + "Reverse accessor 'Target.id' for " + "'invalid_models_tests.Model.m2m_1' clashes with reverse " + "accessor for 'invalid_models_tests.Model.foreign_1'.", hint=( "Add or change a related_name argument to the definition " "for 'invalid_models_tests.Model.m2m_1' or " @@ -1531,9 +1536,9 @@ class ComplexClashTests(SimpleTestCase): id='fields.E305', ), Error( - "Reverse accessor for 'invalid_models_tests.Model.m2m_2' " - "clashes with reverse accessor for " - "'invalid_models_tests.Model.foreign_2'.", + "Reverse accessor 'Target.src_safe' for " + "'invalid_models_tests.Model.m2m_2' clashes with reverse " + "accessor for 'invalid_models_tests.Model.foreign_2'.", hint=( "Add or change a related_name argument to the definition " "for 'invalid_models_tests.Model.m2m_2' or " @@ -1564,16 +1569,16 @@ class ComplexClashTests(SimpleTestCase): other_parent = models.OneToOneField(Parent, models.CASCADE) errors = [ - ('fields.E304', 'accessor', 'parent_ptr', 'other_parent'), - ('fields.E305', 'query name', 'parent_ptr', 'other_parent'), - ('fields.E304', 'accessor', 'other_parent', 'parent_ptr'), - ('fields.E305', 'query name', 'other_parent', 'parent_ptr'), + ('fields.E304', 'accessor', " 'Parent.child'", 'parent_ptr', 'other_parent'), + ('fields.E305', 'query name', '', 'parent_ptr', 'other_parent'), + ('fields.E304', 'accessor', " 'Parent.child'", 'other_parent', 'parent_ptr'), + ('fields.E305', 'query name', '', 'other_parent', 'parent_ptr'), ] self.assertEqual(Child.check(), [ Error( - "Reverse %s for 'invalid_models_tests.Child.%s' clashes with " + "Reverse %s%s for 'invalid_models_tests.Child.%s' clashes with " "reverse %s for 'invalid_models_tests.Child.%s'." - % (attr, field_name, attr, clash_name), + % (attr, rel_name, field_name, attr, clash_name), hint=( "Add or change a related_name argument to the definition " "for 'invalid_models_tests.Child.%s' or " @@ -1582,7 +1587,7 @@ class ComplexClashTests(SimpleTestCase): obj=Child._meta.get_field(field_name), id=error_id, ) - for error_id, attr, field_name, clash_name in errors + for error_id, attr, rel_name, field_name, clash_name in errors ]) diff --git a/tests/model_inheritance/test_abstract_inheritance.py b/tests/model_inheritance/test_abstract_inheritance.py index a78111b5e3..80da06aeff 100644 --- a/tests/model_inheritance/test_abstract_inheritance.py +++ b/tests/model_inheritance/test_abstract_inheritance.py @@ -292,8 +292,9 @@ class AbstractInheritanceTests(SimpleTestCase): Foo._meta.get_field('foo').check(), [ Error( - "Reverse accessor for 'model_inheritance.Foo.foo' clashes " - "with field name 'model_inheritance.Descendant.foo'.", + "Reverse accessor 'Descendant.foo' for " + "'model_inheritance.Foo.foo' clashes with field name " + "'model_inheritance.Descendant.foo'.", hint=( "Rename field 'model_inheritance.Descendant.foo', or " "add/change a related_name argument to the definition "