Opened 13 years ago
Last modified 9 years ago
#16920 new Bug
Models with GenericRelation are unnecessarily validated for clashes in reverse manager accessor
Reported by: | Ricky Rosario | Owned by: | nobody |
---|---|---|---|
Component: | contrib.contenttypes | Version: | 1.3 |
Severity: | Normal | Keywords: | |
Cc: | james@…, 4glitch@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
GenericRelation fields don't add a reverse manager to the related model. However, the model validation verifies that there are no collisions forcing the use of an unused related_name parameter (which then can cause issues such as #16913).
To reproduce:
Create a model with a GenericForeignKey and a related model with a GenericRelation. testapp1/models.py:
from django.contrib.contenttypes.models import ContentType from django.contrib.contenttypes import generic from django.db import models class Topic(models.Model): content_type = models.ForeignKey(ContentType) object_id = models.PositiveIntegerField() content_object = generic.GenericForeignKey('content_type', 'object_id') class Post(models.Model): topic = generic.GenericRelation(Topic)
In a different app, create another related model with the same name. testapp2/models.py:
from django.contrib.contenttypes import generic from django.db import models from testapp1.models import Topic class Post(models.Model): topic = generic.GenericRelation(Topic)
Now run syncdb:
$ ./manage.py syncdb Error: One or more models did not validate: testapp2.post: Accessor for m2m field 'topic' clashes with related m2m field 'Topic.post_set'. Add a related_name argument to the definition for 'topic'. testapp1.post: Accessor for m2m field 'topic' clashes with related m2m field 'Topic.post_set'. Add a related_name argument to the definition for 'topic'.
Change History (12)
comment:1 by , 13 years ago
Cc: | added |
---|
comment:2 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 13 years ago
Also, as part of this fix GenericRelation
should probably be modified so it doesn't silently accept the useless (and problematic) related_name
argument at all.
comment:5 by , 11 years ago
this was fixed by commit https://github.com/django/django/commit/97774429aeb54df4c09895c07cd1b09e70201f7d as part of #19385
comment:6 by , 11 years ago
Wrote two tests to cover this.
On django 1.5.1:
gabejackson@jax: tests# PYTHONPATH=..:$PYTHONPATH python ./runtests.py --settings=test_sqlite generic_relations Creating test database for alias 'default'... Creating test database for alias 'other'... ..F...F.. ====================================================================== FAIL: test_generic_relation_related_name_not_allowed (generic_relations.tests.GenericRelationsTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/gabejackson/Documents/pycharm/django-1.6.2/tests/generic_relations/tests.py", line 245, in test_generic_relation_related_name_not_allowed class InvalidGenericRelationModel(models.Model): AssertionError: TypeError not raised ====================================================================== FAIL: test_multiple_gen_rel_with_same_class_name (generic_relations.tests.GenericRelationsTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/gabejackson/Documents/pycharm/django-1.6.2/tests/generic_relations/tests.py", line 259, in test_multiple_gen_rel_with_same_class_name self.fail("validate() failed with: %s" % e) AssertionError: validate() failed with: One or more models did not validate: appone.post: Accessor for m2m field 'topic' clashes with related m2m field 'Topic.post_set'. Add a related_name argument to the definition for 'topic'. apptwo.post: Accessor for m2m field 'topic' clashes with related m2m field 'Topic.post_set'. Add a related_name argument to the definition for 'topic'. ---------------------------------------------------------------------- Ran 9 tests in 0.107s FAILED (failures=2) Destroying test database for alias 'default'... Destroying test database for alias 'other'...
on master:
Testing against Django installed in '/Users/gabejackson/Documents/pycharm/django-generic-rel-reverse/django' Creating test database for alias 'default'... Creating test database for alias 'other'... ......................... ---------------------------------------------------------------------- Ran 25 tests in 0.210s OK Destroying test database for alias 'default'... Destroying test database for alias 'other'...
Pull Request is here: https://github.com/django/django/pull/2407
comment:7 by , 11 years ago
after some more digging, this problem hasn't been solved – it only vanished. Apparently, GenericRelations currently don't get check()'ed. This may have gotten lost during implementation of the new checks framework. the PR now contains 3 tests:
- One to test that related_name will raise a TypeError when defined on GenericRelation
- One to test the OPs concern of defining the same model name in two different apps - this no longer leads to a clash since we do not check related_name clashes on GenericRelation anymore (changed this code)
- One to test that equal related_query_names still resolve in a clash when one or more GenericRelations define the same related_query_name to the same related object
PR is updated
comment:8 by , 10 years ago
Cc: | added |
---|
comment:9 by , 10 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
I left comments for improvement on the PR. Please uncheck "Patch needs improvement" when you update it, thanks.
comment:10 by , 10 years ago
Patch needs improvement: | unset |
---|
Clean up according to review. I'm not sure the refactor of _check_accessor_clashes and _check_reverse_query_clashes to _check_clashes(check_related_query_name=True/False) leads to more legible code, but at least code duplication is now gone.
Also rebased on current master.
comment:12 by , 9 years ago
Component: | Database layer (models, ORM) → contrib.contenttypes |
---|
This probably blocks on #16905.