Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  closed
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:  fixed
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Mariusz Felisiak ):

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


Comment:

 In [changeset:"379bf1a2d41494360d86bc3cf8adc482abca5d63" 379bf1a2]:
 {{{
 #!CommitTicketReference repository=""
 revision="379bf1a2d41494360d86bc3cf8adc482abca5d63"
 Fixed #8467 -- Prevented crash when adding existent m2m relation with an
 invalid type.

 This was an issue anymore on backends that allows conflicts to be
 ignored (Refs #19544) as long the provided values were coercible to the
 expected type. However on the remaining backends that don't support
 this feature, namely Oracle, this could still result in an
 IntegrityError.

 By attempting to coerce the provided values to the expected types in
 Python beforehand we allow the existing value set intersection in
 ManyRelatedManager._get_missing_target_ids to prevent the problematic
 insertion attempts.

 Thanks Baptiste Mispelon for triaging this old ticket against the
 current state of the master branch.
 }}}

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.1a0349f5a4119eccca96760cff93a7ba%40djangoproject.com.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak ):

 In [changeset:"8cc711999ab839c1a93cb228b35beac4a454fafc" 8cc7119]:
 {{{
 #!CommitTicketReference repository=""
 revision="8cc711999ab839c1a93cb228b35beac4a454fafc"
 Refs #8467 -- Added test for RelatedManager.add()/remove() with an invalid
 type.
 }}}

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.a9f71eeed338426503a4a21aa2df3786%40djangoproject.com.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Simon Charette):

 * needs_better_patch:  1 => 0
 * needs_tests:  1 => 0


Comment:

 https://github.com/django/django/pull/12123

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.c45b2dc59027c7462d6aaf4facc3e1f6%40djangoproject.com.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Simon Charette):

 Actually, this is more than an optimization because it will still crash
 after 28712d8acfffa9cdabb88cb610bae14913fa185d on backends that don't have
 the `supports_ignore_conflicts` feature flag enabled (only Oracle).

 I'll submit a PR for it.

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.5ead4e630b9f78a042dd2605b82f5828%40djangoproject.com.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Simon Charette):

 A regression test for the above optimization could be to
 `assertNumQueries(1)` on the `b.a.add(str(pk))` call when a `m2m_changed`
 signal is registered for the relationship. That is to disable the ''fast''
 insertion mechanism that is enabled on all backends except for Oracle.

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.37268d33b90f2a42b855d5fc258cbc3a%40djangoproject.com.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Simon Charette):

 I'm pretty sure we can consider this fixed by
 28712d8acfffa9cdabb88cb610bae14913fa185d.

 Even if we don't perform the conversion to the pk type in
 
[https://github.com/django/django/blob/master/django/db/models/fields/related_descriptors.py#L1066-L1067
 the branch handling non-model instances] of `_get_target_ids` we now
 ignore conflicts on bulk insertions so the collision on insertion is now
 ignored.

 I guess we could adjust the branch as an optimization if we wanted to by
 calling `target_field.to_python(obj)` by re-purposing this ticket as an
 optimization.

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.916f8a063eb161e596d7c58237b5de5c%40djangoproject.com.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Baptiste Mispelon):

 I was unable to reproduce this on a recent version (tag `3.0rc1` for
 example).

 I'm using the following test case (same models as in the original ticket):
 {{{
 from django.db import IntegrityError
 from django.test import TestCase

 from .models import A, B


 class Ticket8467TestCase(TestCase):
 def test_integrityerror(self):
 pk = A.objects.create().pk

 b = B.objects.create()
 b.a.add(str(pk))
 with self.assertRaises(IntegrityError):
 b.a.add(str(pk))
 }}}

 This test fails (meaning that an `IntegrityError` is not raised) on
 `3.0rc1` but passes for `2.2`.
 Using `git bisect`, I tracked it down to this commit:
 28712d8acfffa9cdabb88cb610bae14913fa185d.

 However it's not immediately clear to me why that commit would fix this
 ticket so I'm unsure whether I should close this ticket.

-- 
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 view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.9bc96ed050357fdf893fa164f9c126ad%40djangoproject.com.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Tim Graham):

 As noted in duplicate #27249, calling `Field.to_python()` on the input
 might work.

--
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/064.3d8a4775f6da2fbb0f612f36542cf96b%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by akaariai):

 I think we should fix this on the basis that using b.a.add('1') works
 partly, but the duplication check doesn't work. Either we should reject
 strings where ints are required, or handle type coercions fully.

 To fix this, we should coerce the input values to right type right at the
 beginning of add() and remove(). The same applies likely also to reverse
 foreign key sets. Still, we need to convert to the primary key type of the
 target model, that is, the code should work also when A had a custom
 primary key (for example, A.name was primary key).

 Tests for m2m.add can be found from
 
https://github.com/akaariai/django/commit/b1308c114f601db2f3f0c8d76c55acd966a14672.
 Similar tests should be added for .remove(), and for reverse foreign key
 .add() and .remove().

--
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/064.9af7cfcb311afdce3e69879d7124d702%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:  Duplicate entry, | Triage Stage:  Accepted
  add, remove, ManyToManyField   |
Has patch:  1|  Needs documentation:  0
  Needs tests:  1|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by timgraham):

 What's the use case for this functionality? Do we do similar type coercion
 elsewhere?

--
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/064.263cdd43c9961d12f484823526949bc4%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:  Duplicate entry, |  Needs documentation:  0
  add, remove, ManyToManyField   |  Patch needs improvement:  1
Has patch:  1|UI/UX:  0
  Needs tests:  1|
Easy pickings:  0|
-+-
Changes (by timo):

 * needs_better_patch:  0 => 1
 * needs_tests:  0 => 1


Comment:

 Patch needs improvement per @akaariai's comment as well as tests.

-- 
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/064.092766dc4fd23d3c796247d4b2ef320f%40djangoproject.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+-
 Reporter:  Wonlay   |Owner:  nobody
 Type:  New feature  |   Status:  new
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:  Duplicate entry, |  Needs documentation:  0
  add, remove, ManyToManyField   |  Patch needs improvement:  0
Has patch:  1|UI/UX:  0
  Needs tests:  0|
Easy pickings:  0|
-+-
Changes (by akaariai):

 * stage:  Design decision needed => Accepted


Comment:

 I think this fix makes sense. The patch should be made aware of
 from_field/to_field if possible, even it is not possible to create such
 ManyToManys currently this feature will very likely appear some day.

-- 
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.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [Django] #8467: For ManyToMany manager, we should convert objects being added or removed to the pk type if they are not.

[Django]

#8467: For ManyToMany manager, we should convert objects being added or removed 
to
the pk type if they are not.
-+--
  Reporter:  Wonlay  | Owner:  nobody   

Status:  new | Milestone:   

 Component:  Core framework  |   Version:  SVN  

Resolution:  |  Keywords:  Duplicate entry, 
add, remove, ManyToManyField
 Stage:  Design decision needed  | Has_patch:  1

Needs_docs:  0   |   Needs_tests:  0

Needs_better_patch:  0   |  
-+--
Changes (by jacob):

  * stage:  Unreviewed => Design decision needed

-- 
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 this group at 
http://groups.google.com/group/django-updates?hl=en
-~--~~~~--~~--~--~---