Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
-+-
 Reporter:  carljm   |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Claude Paroz ):

 In [changeset:"7ce9644d9347455ae6f9bd383788a65e4bcadda3"]:
 {{{
 #!CommitTicketReference repository=""
 revision="7ce9644d9347455ae6f9bd383788a65e4bcadda3"
 Added a test to ensure bad assignation to M2M doesn't clear data

 Refs #14394.
 }}}

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
-+-
 Reporter:  carljm   |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  master
  (models, ORM)  |
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by claudep):

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


Comment:

 I think this issue has been addressed in current code, notably by
 bc9be72bdc9bb4dfc7f967ac3856115f0a6166b8. I'll still commit a test for
 confirmation.

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
-+-
 Reporter:  carljm   |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  SVN
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by akaariai):

 The "lets iterate through the objects and fetch them one at a time" is a
 clear no-no. However, you can't assume a primary key is an int.

 I really don't know what there is to do here. You are at a point where
 data validation should have prevented the errors. If you get an error,
 rollback your transaction. If you don't want to do that, use a savepoint.

 This is not a unique situation in Django. For example, if you do a .save()
 on multitable inherited model, you can get it half saved if there is a
 data type mismatch in the topmost object in the inheritance chain.
 Something like:
 {{{
 class A:
 pass
 class B(A):
 f = models.IntegerField()
 B(f='not an integer').save()
 }}}
 do that and you got a save into A's table, but not into B's table.

 Now, this is not a reason to avoid all sanity checks. But, aiming for
 anything like perfection (guaranteeing that the insert will succeed after
 the delete) isn't worth the effort in my opinion. Even the query for all
 the objects one by one will not be guaranteed to work under normal
 transactional rules. You would need something like select for update. Lets
 not go there.

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
-+-
 Reporter:  carljm   |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  SVN
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  1|  Patch needs improvement:  1
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by krzysiumed):

 * cc: krzysiumed@… (added)
 * needs_better_patch:  0 => 1
 * ui_ux:   => 0
 * easy:   => 0


Comment:

 The last patch seems to be almost ready-for-checkin, but I think we should
 not check if primary keys are valid (lines 743-746 in last patch), because
 it requires large amount of sql queries and it could be slow. Instead, we
 should check if primary key is `int`. Maybe we should add note in
 documentation that django does not check it?

 In my opinion tests need small improvements:
 * In `test_assigning_any_iterable_with_valid_models_to_m2m_works` we don't
 need to check all possible iterables: sets, lists, tuples.
 * As I wrote, we should not validate primary keys, so we don't need
 
`test_assigning_iterable_with_invalid_pks_to_m2m_doesnt_clear_existing_relations`.
 * We can join `test_assigning_any_iterable_with_valid_models_to_m2m_works`
 and `test_assigning_any_iterable_with_valid_primary_keys_to_m2m_works` by
 testing `c1.tags = [t1, t2.pk]`.
 * There are redundant lines of code (for example `t1 =
 Tag.objects.create(name='t1')`). We can move them to `setUp`.

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
---+
  Reporter:  carljm| Owner:  nobody
Status:  new   | Milestone:  1.3   
 Component:  Database layer (models, ORM)  |   Version:  SVN   
Resolution:|  Keywords:
 Stage:  Accepted  | Has_patch:  1 
Needs_docs:  0 |   Needs_tests:  0 
Needs_better_patch:  0 |  
---+
Comment (by Alex):

 [14602] removed ManyToManyField.isValidIDList which was unused and did
 something a little different from what's proposed here, but I'm not it for
 posterity (and because Carl asked me to).

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
---+
  Reporter:  carljm| Owner:  nobody
Status:  new   | Milestone:  1.3   
 Component:  Database layer (models, ORM)  |   Version:  SVN   
Resolution:|  Keywords:
 Stage:  Accepted  | Has_patch:  1 
Needs_docs:  0 |   Needs_tests:  0 
Needs_better_patch:  0 |  
---+
Comment (by john_scott):

 That is certainly much more strict validation. I will leave it to people
 much smarter than I to decide whether it is worth the queries to test if
 each int is a valid pk. There is validation much further down the stack
 that tests this before saving to the db, just not before calling
 `clear()`.

 Since the `add()` method can handle an iterable of pks, perhaps just
 provide this rather than model instances since `_add_items()` just grabs
 these anyway:

 {{{
 try:
 instance = manager.model.objects.get(pk=obj)
 except (manager.model.DoesNotExist, ValueError):
 raise TypeError(u"Cannot set value to this ManyToManyField. Make sure
 you pass valid models or primary keys.")
 else:
 model_instances.append(instance.pk)
 }}}

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
---+
  Reporter:  carljm| Owner:  nobody
Status:  new   | Milestone:  1.3   
 Component:  Database layer (models, ORM)  |   Version:  SVN   
Resolution:|  Keywords:
 Stage:  Accepted  | Has_patch:  1 
Needs_docs:  0 |   Needs_tests:  0 
Needs_better_patch:  0 |  
---+
Comment (by igors):

 Following jonh_scott's suggestion, I've attached a patch that validates
 the value more carefully. It has to be an iterable with valid models
 instances or primary keys.

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
---+
  Reporter:  carljm| Owner:  nobody
Status:  new   | Milestone:  1.3   
 Component:  Database layer (models, ORM)  |   Version:  SVN   
Resolution:|  Keywords:
 Stage:  Accepted  | Has_patch:  1 
Needs_docs:  0 |   Needs_tests:  0 
Needs_better_patch:  0 |  
---+
Comment (by john_scott):

 The `if not isinstance(value, (list, tuple))` check in the patch is not
 quite right. Any iterable (`QuerySet`, `set`, etc.) is also valid so long
 as it yields valid related model instances or primary keys.

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
---+
  Reporter:  carljm| Owner:  nobody
Status:  new   | Milestone:  1.3   
 Component:  Database layer (models, ORM)  |   Version:  SVN   
Resolution:|  Keywords:
 Stage:  Accepted  | Has_patch:  1 
Needs_docs:  0 |   Needs_tests:  0 
Needs_better_patch:  0 |  
---+
Changes (by PaulM):

  * stage:  Unreviewed => Accepted

Comment:

 Please don't change accepted tickets to unreviewed. These statuses have
 nothing to do with your patch.

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
---+
  Reporter:  carljm| Owner:  nobody
Status:  new   | Milestone:  1.3   
 Component:  Database layer (models, ORM)  |   Version:  SVN   
Resolution:|  Keywords:
 Stage:  Unreviewed| Has_patch:  1 
Needs_docs:  0 |   Needs_tests:  0 
Needs_better_patch:  0 |  
---+
Changes (by igors):

  * stage:  Accepted => Unreviewed

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
---+
  Reporter:  carljm| Owner:  nobody
Status:  new   | Milestone:  1.3   
 Component:  Database layer (models, ORM)  |   Version:  SVN   
Resolution:|  Keywords:
 Stage:  Accepted  | Has_patch:  1 
Needs_docs:  0 |   Needs_tests:  0 
Needs_better_patch:  0 |  
---+
Changes (by igors):

  * has_patch:  0 => 1

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

Re: [Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
---+
  Reporter:  carljm| Owner:  nobody
Status:  new   | Milestone:  1.3   
 Component:  Database layer (models, ORM)  |   Version:  SVN   
Resolution:|  Keywords:
 Stage:  Accepted  | Has_patch:  0 
Needs_docs:  0 |   Needs_tests:  0 
Needs_better_patch:  0 |  
---+
Changes (by PaulM):

  * needs_better_patch:  => 0
  * needs_docs:  => 0
  * stage:  Unreviewed => Accepted
  * needs_tests:  => 0
  * milestone:  => 1.3

Comment:

 This seems like reasonable behavior.

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

[Django] #14394: Assigning bad data to an m2m attribute should not clear existing data

[Django]

#14394: Assigning bad data to an m2m attribute should not clear existing data
--+-
 Reporter:  carljm|   Owner:  nobody
   Status:  new   |   Milestone:
Component:  Database layer (models, ORM)  | Version:  SVN   
 Keywords:|   Stage:  Unreviewed
Has_patch:  0 |  
--+-
 If you assign something nonsensical to a many-to-many attribute on a model
 instance, you'll get a `TypeError`; but not until after Django has cleared
 all the existing relationship data. Ideally Django would verify that
 you've passed in something reasonable before it clears the existing data.

 (This issue was originally half of #14373).

 Attached patch with test demonstrating the issue.

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