Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#26162 closed Bug (fixed)

Data loss when ManyToManyField refers to the wrong table

Reported by: Ian Foote Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 1.8
Severity: Release blocker Keywords:
Cc: Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If I have two identically named models in two different apps, but these models each have a ManyToManyField to the same model, then one of these ManyToManyFields will use the other's database table.

App 1:

class Bar(models.Model):
    name = models.CharField(max_length=255)

class Foo(models.Model):
    name = models.CharField(max_length=255)
    bars = models.ManyToManyField(Bar, related_name='+')

App 2:

from app1.models import Bar

class Foo(models.Model):
   name = models.CharField(max_length=255)
   bars = models.ManyToManyField(Bar, related_name='+')

Tests:

from app1.models import Bar, Foo

def test_add_bar_to_foo():
    bar = Bar.objects.create()
    foo = Foo.objects.create()

    foo.bars.add(bar)

    assert bar in foo.bars.all()

I expect the test to pass, but instead it fails.

A working example of this code can be found at: https://github.com/Ian-Foote/django-bug-sandbox/pull/2

Change History (7)

comment:1 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

Django 1.6 gave a validation error for this case:

CommandError: One or more models did not validate:
foo.foo: Accessor for m2m field 'bars' clashes with related m2m field 'Bar.+'. Add a related_name argument to the definition for 'bars'.
foo.foo: Reverse query name for m2m field 'bars' clashes with related m2m field 'Bar.+'. Add a related_name argument to the definition for 'bars'.
foo2.foo: Accessor for m2m field 'bars' clashes with related m2m field 'Bar.+'. Add a related_name argument to the definition for 'bars'.
foo2.foo: Reverse query name for m2m field 'bars' clashes with related m2m field 'Bar.+'. Add a related_name argument to the definition for 'bars'.

but this ceased after 9f76ea1eaad0db0183fa3b5bade16392db0cafbd.

This might be related to #24505 and its duplicate, #25486.

comment:2 by Simon Charette, 9 years ago

Cc: Simon Charette added

I suspect this is caused by colliding related_query_names.

When you hide a related manager by setting related_name to a value ending with '+' the related_query_name is still set to its default value (based on the model name).

If we explicit the model definitions created by Django here's what we have:

class Bar(models.Model):
    class Meta:
        app_label = 'app1'

class Foo(models.Model):
    bars = models.ManyToManyField(Bar, related_name='+', through='app1.FooBar')

    class Meta:
        app_label = 'app1'

class FooBar(models.Model):
    bar = models.ForeignKey(
        Bar, related_name='+', related_query_name='foobar'
    )
    foo = models.ForeignKey(
        'app1.Foo', related_name='+', related_query_name='foobar'
    )

    class Meta:
        app_label = 'app1'

class Foo(models.Model):
    bars = models.ManyToManyField(Bar, related_name='+', through='app2.FooBar')

    class Meta:
        app_label = 'app2'

class FooBar(models.Model):
    bar = models.ForeignKey(
        Bar, related_name='+', related_query_name='foobar'
    )
    foo = models.ForeignKey(
        'app2.Foo', related_name='+', related_query_name='foobar'
    )

    class Meta:
        app_label = 'app2'

As you can see there's a related_query_name='foobar' conflict on Bar that makes foo.bars.all() (resolving to Bar.object.filter(foobar__foo=foo) ambiguous. This clash is not detected by the check framework because the relationships are hidden.

In order to fix this I suggest we start by fixing the clash checks to run even if the relationship is hidden, we just have to skip E302 and E304 in this case.

Then we should consider making implicit intermediary model creation more smart about this possible conflict by namespacing related_query_name by application label. This is a backward incompatible change in some way since users might have relied on the automatically generated queryname even if unlikely. The backward incompatibility could be lessened by only namespacing the queryname if the referenced model is from a different application.

comment:3 by Simon Charette, 9 years ago

Has patch: set
Owner: changed from nobody to Simon Charette
Status: newassigned

Here's a PR with the checks adjustments.

comment:4 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:5 by Simon Charette <charette.s@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In a325fb1f:

Fixed #26162 -- Checked query name clashes of hidden relationships.

Although reverse accessor clashes should be skipped query name can't be hidden.

Thanks to Ian Foote and Tim Graham for the review.

comment:6 by Simon Charette <charette.s@…>, 9 years ago

In 5872372:

Fixed #26162 -- Checked query name clashes of hidden relationships.

Although reverse accessor clashes should be skipped query name can't be hidden.

Thanks to Ian Foote and Tim Graham for the review.

comment:7 by Simon Charette <charette.s@…>, 9 years ago

In edff550:

[1.8.x] Fixed #26162 -- Checked query name clashes of hidden relationships.

Although reverse accessor clashes should be skipped query name can't be hidden.

Thanks to Ian Foote and Tim Graham for the review.

Backport of a325fb1f9b14b46288d0e1342407be4a6db2bdb1 from master

Note: See TracTickets for help on using tickets.
Back to Top