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 James Socol, 13 years ago

Cc: james@… added

comment:2 by Carl Meyer, 13 years ago

Triage Stage: UnreviewedAccepted

This probably blocks on #16905.

comment:3 by Carl Meyer, 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:4 by Gabe Jackson, 11 years ago

#22207 is pre-cursor to fixing this.

Last edited 11 years ago by Gabe Jackson (previous) (diff)

comment:5 by Gabe Jackson, 11 years ago

Version 0, edited 11 years ago by Gabe Jackson (next)

comment:6 by Gabe Jackson, 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 Gabe Jackson, 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 anonymous, 10 years ago

Cc: 4glitch@… added

comment:9 by Tim Graham, 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 Gabe Jackson, 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:11 by Tim Graham, 10 years ago

Patch needs improvement: set

Tests are not passing.

comment:12 by Tim Graham, 9 years ago

Component: Database layer (models, ORM)contrib.contenttypes
Note: See TracTickets for help on using tickets.
Back to Top