Opened 3 years ago

Closed 2 years ago

Last modified 2 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 3 years ago.
m2m_through_field.patch (5.5 KB) - added by anonymous 3 years ago.
tests-patch.diff (2.1 KB) - added by anonymous 3 years ago.

Download all attachments as: .zip

Change History (17)

Changed 3 years ago by anonymous

comment:1 Changed 3 years ago by anonymous

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

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?

Changed 3 years ago by anonymous

comment:2 Changed 3 years ago by anonymous

  • Has patch set

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

comment:3 Changed 3 years ago by russellm

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 3 years ago by russellm

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

comment:5 follow-up: Changed 3 years ago by akaariai

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

comment:6 in reply to: ↑ 5 Changed 3 years ago by 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...

comment:7 Changed 3 years ago by akaariai

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 Changed 3 years ago by 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.

comment:9 Changed 3 years ago by 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.

Changed 3 years ago by anonymous

comment:10 Changed 3 years ago by 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

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

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

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 Changed 2 years ago by 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

comment:13 Changed 2 years ago by 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

comment:14 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

In 046300c43b44c3238e980f01c177170ed4efde34:

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

Refs #18823.

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