Opened 12 years ago

Closed 11 years ago

Last modified 7 years ago

#18823 closed Bug (fixed)

Clear with a M2M field with a through model

Reported by: anonymous Owned by: nobody
Component: Database layer (models, ORM) Version: 1.4
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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)

Attachments (3)

failing_tests.patch (749 bytes ) - added by anonymous 12 years ago.
m2m_through_field.patch (5.5 KB ) - added by anonymous 12 years ago.
tests-patch.diff (2.1 KB ) - added by anonymous 12 years ago.

Download all attachments as: .zip

Change History (19)

by anonymous, 12 years ago

Attachment: failing_tests.patch added

comment:1 by anonymous, 12 years ago

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?

by anonymous, 12 years ago

Attachment: m2m_through_field.patch added

comment:2 by anonymous, 12 years ago

Has patch: set

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

comment:3 by Russell Keith-Magee, 12 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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()

comment:4 by Russell Keith-Magee, 12 years ago

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

comment:5 by Anssi Kääriäinen, 12 years ago

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...

in reply to:  5 comment:6 by anonymous, 12 years ago

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...

comment:7 by Anssi Kääriäinen, 12 years ago

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...

comment:8 by anonymous, 12 years ago

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.

comment:9 by anonymous, 12 years ago

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.

by anonymous, 12 years ago

Attachment: tests-patch.diff added

comment:10 by anonymous, 12 years ago

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

comment:11 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: newclosed

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.

comment:12 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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

comment:13 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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

comment:14 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 046300c43b44c3238e980f01c177170ed4efde34:

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

Refs #18823.

comment:15 by Tim Graham <timograham@…>, 7 years ago

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).

comment:16 by Tim Graham <timograham@…>, 7 years ago

In b8741c00:

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

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