Clear with a M2M field with a through model

[anonymous]

This ticket deals with similar issues as #15161.

When you have a M2M field with a through model where one of the foreign keys uses a to_field, the to_field is ignored by add, remove and clear. Only clear is available when using a normal through but it would be nice to fix for all of them.

To see the currently wrong behavior try something like this:

class Through(models.Model):
    ffrom = models.ForeignKey('Two', to_field='slug')
    to = models.ForeignKey('One')

class One(models.Model):
    name = models.CharField(max_length=100)

class Two(models.Model):
    slug = models.CharField(max_length=24, unique=True)
    ones = models.ManyToManyField(One, through=Through)

o = One(name='test')
t = Two(slug='test2')
Through(ffrom=t, to=o).save()
t.ones.clear()

You'll see the relationship not being cleared as the queries use the value for the pk of t instead of the value of 'slug' on t ('test2').

The fix could be as simple as changing ManyRelatedManager._pk_val to not be pk value but to lookup the correct value for the field. (line 534)

[anonymous]

Added a patch contains failing tests that show the problem. I can work on a fix for this but before starting I would like to hear from some of the core devs if they would accept a patch to also fix add and remove so they use the to_field. Assuming that someone is constructing the through table dynamically and setting auto_created. Or do you just want the clear method fixed and leave add and remove alone?

[anonymous]

Added m2m_through_field.patch which addresses the problem. Feedback would be appreciated.

[Russell Keith-Magee]

Via django-developers -- this bug has the potential for inadvertant data loss:

class Through(models.Model):
    two = models.ForeignKey('Model2', to_field='spot')
    one = models.ForeignKey('Model1')

class Model1(models.Model):
    name = models.CharField(max_length=100)

class Model2(models.Model):
    spot = models.IntegerField(unique=True)
    ones = models.ManyToManyField(Model1, through=Through)

# Create objects and relations
m = Model1(name='test')
m2 = Model2(spot=2, pk=1)
m3 = Model2(spot=1, pk=2)
m.save()
m2.save()
m3.save()
Through(two=m2, one=m).save()
Through(two=m3, one=m).save()

# Both have ones
m2.ones.all()
m3.ones.all()

m2.ones.clear()

# Still has ones, didn't get deleted when it should have
m2.ones.all()

# This got deleted instead
m3.ones.all()

[Russell Keith-Magee]

Since this is a potential data loss bug, it should be backported to 1.4, too.

[Anssi Kääriäinen]

I have a feeling we should not be testing models where the undocumented auto_created is used to mark a custom through model as created by Django. It is very much possible things will not work if you do that.

A bigger issue is that the patch assigns to _pk_val something that isn't necessarily a pk value.

I did the above changes, and the work is available from here: ​https://github.com/akaariai/django/tree/ticket_18823

The patch is work-in-progress. I don't have more time to dedicate to this issue just now, and thought it might be a good idea to post the work in any case. I don't know of any issue that needs to be fixed in the patch, but on the other hand I just stopped working at an arbitrary point when reviewing this, so it might be there is still something to do...

[anonymous]

Replying to akaariai:

A bigger issue is that the patch assigns to _pk_val something that isn't necessarily a pk value.

That's sort of the whole point of this it can't be a pk val as that might not be what the correct value. Would you prefer that I rename _pk_val to _fk_val?

I did the above changes, and the work is available from here: ​https://github.com/akaariai/django/tree/ticket_18823

By the 'above changes' do you mean just dropping the tests? Or is there something else there that i missed?

The patch is work-in-progress. I don't have more time to dedicate to this issue just now, and thought it might be a good idea to post the work in any case. I don't know of any issue that needs to be fixed in the patch, but on the other hand I just stopped working at an arbitrary point when reviewing this, so it might be there is still something to do...

[Anssi Kääriäinen]

Sorry, I uploaded the wrong version. Reuploaded to the same place now.

And yes, rename is what I was thinking about.

I would like to add lower level tests for .add/.remove so that we know they work correctly for possible future changes to m2mfield (to_field/from_field arguments come to mind here). The Meta: auto_created hack seems ugly, but maybe it could be acceptable for testing, as that was pretty easy way to test this. Lets see if some other way to test this surfaces. if not, maybe those tests could be resurrected...

[anonymous]

Thanks for taking the time to look at this. Renaming does make things clearer.

Seems to me that test_to_field_clear and test_to_field_clear_reverse are still acceptable tests as they don't depend on auto_created. I couldn't come up with another way to test add and remove. Maybe if the underlying private methods were always created and just the public add and remove methods were skipped for non-auto-created models then we could test those directly. But I didn't want to propose such a large change just for testing. I'll think about it a little more though as maybe there is a better solution that I'm missing.

[anonymous]

Not sure why I thought that the private methods weren't always created. I'll submit a new patch with tests to target those methods instead.

[anonymous]

Here's patch meant to be applied on top of ​https://github.com/akaariai/django/tree/ticket_18823. It tests _add_items and _remove_items instead of add and remove.

Hand coding the right fields in the right order seems a little fragile. I thought about using manager.source_field_name and manager.target_field_name in the tests, but didn't because that is undocumented api. Although so is _add_items and _remove_items. I guess i'm just really not sure when it is ok to use undocumented api in tests and when it isn't.

Let me know what you think

[Anssi Kääriäinen <akaariai@…>]

In 611c4d6f1c24763e5e6e331a5dcf9b610288aaa8:

Fixed #18823 -- Ensured m2m.clear() works when using through+to_field

There was a potential data-loss issue involved -- when clearing
instance's m2m assignments it was possible some other instance's
m2m data was deleted instead.

This commit also improved None handling for to_field cases.

[Anssi Kääriäinen <akaariai@…>]

In f105fbe52b21da206bfbaedf0e92326667d7b2d4:

[1.5.x] Fixed #18823 -- Ensured m2m.clear() works when using through+to_field

There was a potential data-loss issue involved -- when clearing
instance's m2m assignments it was possible some other instance's
m2m data was deleted instead.

This commit also improved None handling for to_field cases.

Backpatch of 611c4d6f1c24763e5e6e331a5dcf9b610288aaa8

[Anssi Kääriäinen <akaariai@…>]

In 37c87b785da934815f1f5ca863d88ac0685bff2a:

[1.4.x] Fixed #18823 -- Ensured m2m.clear() works when using through+to_field

There was a potential data-loss issue involved -- when clearing
instance's m2m assignments it was possible some other instance's
m2m data was deleted instead.

This commit also improved None handling for to_field cases.

Backpatch of 611c4d6f1c24763e5e6e331a5dcf9b610288aaa8

[Aymeric Augustin <aymeric.augustin@…>]

In 046300c43b44c3238e980f01c177170ed4efde34:

[1.4.x] Restored Python 2.5 compatibility in m2m_through_regress tests.

Refs #18823.

[Tim Graham <timograham@…>]

In 39a88438:

Used assertRaisesMessage() in m2m_through_regress tests.

The "needs to have a value for field" messages are incorrect and
reference nonexistent fields since the commit in which they were
introduced (refs #18823).

[Tim Graham <timograham@…>]

In b8741c00:

Refs #18823 -- Corrected field name in an m2m manager error message.