Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |
 Severity:  Release blocker  |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Tim Graham ):

 In [changeset:"39a884380226e3ae372cdc93b7227ad2c8e7b086" 39a88438]:
 {{{
 #!CommitTicketReference repository=""
 revision="39a884380226e3ae372cdc93b7227ad2c8e7b086"
 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).
 }}}

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.e67a84d80732a7118b5d621c684f5463%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |
 Severity:  Release blocker  |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Tim Graham ):

 In [changeset:"b8741c005873396de24a085fe9ab13d2348a306b" b8741c00]:
 {{{
 #!CommitTicketReference repository=""
 revision="b8741c005873396de24a085fe9ab13d2348a306b"
 Refs #18823 -- Corrected field name in an m2m manager error message.
 }}}

--
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.2aa7547f4b1f93e5be8898d70967b0df%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:  fixed
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by Aymeric Augustin ):

 In [changeset:"046300c43b44c3238e980f01c177170ed4efde34"]:
 {{{
 #!CommitTicketReference repository=""
 revision="046300c43b44c3238e980f01c177170ed4efde34"
 [1.4.x] Restored Python 2.5 compatibility in m2m_through_regress tests.

 Refs #18823.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:  fixed
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by Anssi Kääriäinen ):

 In [changeset:"37c87b785da934815f1f5ca863d88ac0685bff2a"]:
 {{{
 #!CommitTicketReference repository=""
 revision="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
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:  fixed
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by Anssi Kääriäinen ):

 In [changeset:"f105fbe52b21da206bfbaedf0e92326667d7b2d4"]:
 {{{
 #!CommitTicketReference repository=""
 revision="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
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:  fixed
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by Anssi Kääriäinen ):

 * status:  new => closed
 * resolution:   => fixed


Comment:

 In [changeset:"611c4d6f1c24763e5e6e331a5dcf9b610288aaa8"]:
 {{{
 #!CommitTicketReference repository=""
 revision="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.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by anonymous):

 Replying to [comment:5 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...

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by russellm):

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Release blocker  | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by russellm):

 * severity:  Normal => Release blocker
 * stage:  Unreviewed => Accepted


Comment:

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by anonymous):

 * has_patch:  0 => 1


Comment:

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
-+-
 Reporter:  anonymous|Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  1.4
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by anonymous):

 * needs_better_patch:   => 0
 * needs_tests:   => 0
 * needs_docs:   => 0


Comment:

 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?

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Django] #18823: Clear with a M2M field with a through model

[Django]

#18823: Clear with a M2M field with a through model
--+
 Reporter:  anonymous |  Owner:  nobody
 Type:  Bug   | Status:  new
Component:  Database layer (models, ORM)  |Version:  1.4
 Severity:  Normal|   Keywords:
 Triage Stage:  Unreviewed|  Has patch:  0
Easy pickings:  0 |  UI/UX:  0
--+
 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)

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.