#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)
Change History (19)
by , 12 years ago
Attachment: | failing_tests.patch added |
---|
comment:1 by , 12 years ago
by , 12 years ago
Attachment: | m2m_through_field.patch added |
---|
comment:2 by , 12 years ago
Has patch: | set |
---|
Added m2m_through_field.patch which addresses the problem. Feedback would be appreciated.
comment:3 by , 12 years ago
Severity: | Normal → Release blocker |
---|---|
Triage Stage: | Unreviewed → 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 by , 12 years ago
Since this is a potential data loss bug, it should be backported to 1.4, too.
follow-up: 6 comment:5 by , 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...
comment:6 by , 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 , 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 , 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 , 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 , 12 years ago
Attachment: | tests-patch.diff added |
---|
comment:10 by , 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 , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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?