Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#11956 closed (fixed)

ReverseManyRelatedObjectsDescriptor wrongly assumes symmetry between inherited objects

Reported by: nidi Owned by: malcalypse
Component: Database layer (models, ORM) Version: 1.1
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Symmetrical many-to-many do only make sense between objects of the same model type, as described in the documentation. However, ReverseManyRelatedObjectDescriptors wrongly assume symmetricity when defining a ManyToManyField from an inherited model towards a base class, as in the following model definition:

class BaseModel(models.Model):
    pass

class ChildModel(BaseModel):
    many = models.ManyToManyField(BaseModel, related_name='many_children')

This results in the following integrity error in case Postgres is used (- for some reason, the problem does not occur using sqlite!):

> from symmetricbug.models import *
> base = BaseModel.objects.create()
> child = ChildModel.objects.create()
> child.many.add(base)

[...]

/usr/lib64/python2.5/site-packages/django/db/models/fields/related.pyc in add(self, *objs)
    432                 # If this is a symmetrical m2m relation to self, add the mirror entry in the m2m table
    433                 if self.symmetrical:
--> 434                     self._add_items(self.target_col_name, self.source_col_name, *objs)
    435             add.alters_data = True

[...]

IntegrityError: insert or update on table "symmetricbug_childmodel_many" violates foreign key constraint "symmetricbug_childmodel_many_childmodel_id_fkey"
DETAIL:  Key (childmodel_id)=(1) is not present in table "symmetricbug_childmodel".

This obviously does not make any sense, because there is no ChildModel object with id 1. The reason is that ReverseManyRelatedObjectsDescriptor wrongly assumes symmetricity:

django.db.models.fields.related:609 is

symmetrical=(self.field.rel.symmetrical and isinstance(instance, rel_model)),

and should be something alike

symmetrical=(self.field.rel.symmetrical and instance.__class__ == rel_model.__class__),

to be aware of subclasses.

Workaround: Use symmetrical=False in the model definition:

class ChildModel(BaseModel):
    many = models.ManyToManyField(BaseModel, related_name='many_children', symmetrical=False)

Attachments (0)

Change History (9)

comment:1 Changed 5 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

It should be noted that the code used to read exactly what you suggested, however that prevented you from adding subclasses of the ManyToMany model to the relationship.

comment:2 Changed 4 years ago by astoneman

I just ran into this, and it seems odd to me that the default value for symmetrical is decided on a per-instance basis, when the related manager is created, rather than just once, when the field is created. Would it make more sense to change django/db/models/fields/related:804 (in ManyToManyField.__init__) from:

            symmetrical=kwargs.pop('symmetrical', True),

to something like:

            symmetrical=kwargs.pop('symmetrical', to == RECURSIVE_RELATIONSHIP_CONSTANT),

and then assume it's set correctly farther in, and just remove the isinstance check on line 609?

comment:3 Changed 4 years ago by russellm

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 4 years ago by flyingfred0

  • Owner changed from nobody to flyingfred0
  • Status changed from new to assigned

comment:5 Changed 4 years ago by flyingfred0

  • Owner changed from flyingfred0 to nobody
  • Status changed from assigned to new

I had no luck reproducing this one with either psycopg or psycopg2 (against PostgreSQL 8.4.0) on OS X 10.5.

comment:6 Changed 4 years ago by malcalypse

  • Owner changed from nobody to malcalypse
  • Status changed from new to assigned

comment:7 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12908]) Fixed #11956 -- Modified the handling of m2m relationships between subclasses. Thanks to nidi for the report, and astoneman for the suggestion on how to fix the problem.

comment:8 Changed 4 years ago by russellm

(In [12909]) [1.1.X] Fixed #11956 -- Modified the handling of m2m relationships between subclasses. Thanks to nidi for the report, and astoneman for the suggestion on how to fix the problem.

Backport of r12908 from trunk.

comment:9 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.