[Django] #16920: Models with GenericRelation are unnecessarily validated for clashes in reverse manager accessor
Django
reverse manager accessor
-------------------+----------------------------------------------
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Database layer (models, ORM)
Version: 1.3 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------+----------------------------------------------
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'.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16920>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
reverse manager accessor
Type: Bug | Status: new
Milestone: | Component: Database layer
Resolution: | Severity: Normal
Triage Stage: | Keywords:
Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by jsocol):
* cc: james@… (added)
* needs_docs: => 0
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:1>
Django
reverse manager accessor
Type: Bug | Status: new
Milestone: | Component: Database layer
Resolution: | Severity: Normal
Needs documentation: 0 | Has patch: 0
Patch needs improvement: 0 | Needs tests: 0
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by carljm):
* stage: Unreviewed => Accepted
Comment:
This probably blocks on #16905.
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:2>
Django
reverse manager accessor
Type: Bug | Status: new
Milestone: | Component: Database layer
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords:
Needs documentation: 0 | Has patch: 0
Patch needs improvement: 0 | Needs tests: 0
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:3>
Django
reverse manager accessor
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by gabejackson):
this will be fixed if #22207 lands.
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:4>
Django
reverse manager accessor
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by gabejackson):
this was fixed by commit
[https://github.com/django/django/commit/97774429aeb54df4c09895c07cd1b09e70201f7d]
as part of #19385
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:5>
Django
reverse manager accessor
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by gabejackson):
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]
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:6>
Django
reverse manager accessor
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by gabejackson):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:7>
Django
reverse manager accessor
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: 4glitch@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:8>
Django
reverse manager accessor
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
I left comments for improvement on the PR. Please uncheck "Patch needs
improvement" when you update it, thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:9>
Django
reverse manager accessor
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:10>
Django
reverse manager accessor
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Tests are not passing.
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:11>
Django
reverse manager accessor
Reporter: r1cky | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* component: Database layer (models, ORM) => contrib.contenttypes
--
Ticket URL: <https://code.djangoproject.com/ticket/16920#comment:12>