[Django] #31095: Django Related Manager set method

[Django]

#31095: Django Related Manager set method
-------------------------------------+-------------------------------------
Reporter: TZanke | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 1.11
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
To update a complete list of foreignkeys, we use set() method of
relatedmanager to get a performance gain and avoid remove and add keys not
touched by user.

But today i noticed our database removes all foreignkeys and adds them
again. After some debugging i found the issue in this condition:
https://github.com/django/django/blob/master/django/db/models/fields/related_descriptors.py#L1004

Our form returns all Foreignkeys as list of strings in cleaned_data, but
the strings do not match the pk (int). After converting the strings to
int, before calling set(), fixes the problem.

Question:
How to avoid this issue? Maybe all code usages of set() are using lists of
strings, maybe not. I dont know at the moment.
Is is possible Django fixes this issue? Should Django fix the issue? Maybe
strings should raise an exception?

--
Ticket URL: <https://code.djangoproject.com/ticket/31095>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#31095: Django Related Manager set method may not work with pk strings

-------------------------------------+-------------------------------------
Reporter: TZanke | Owner: nobody
Type: Bug | Status: new

Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

--
Ticket URL: <https://code.djangoproject.com/ticket/31095#comment:1>

[Django]

#31095: Related Manager set() should prepare values before checking for missing
elements.

-------------------------------------+-------------------------------------
Reporter: TZanke | Owner: nobody

Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master

(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by felixxm):

* version: 1.11 => master
* type: Bug => Cleanup/optimization
* easy: 0 => 1
* stage: Unreviewed => Accepted

Comment:

We cannot raise an exception on strings because `set()` accepts the field
the relation points, e.g. `CharField`. However we can optimize this with
preparing values, i.e.
{{{
diff --git a/django/db/models/fields/related_descriptors.py
b/django/db/models/fields/related_descriptors.py
index a9445d5d10..9f82ca4e8c 100644
--- a/django/db/models/fields/related_descriptors.py
+++ b/django/db/models/fields/related_descriptors.py
@@ -999,7 +999,7 @@ def create_forward_many_to_many_manager(superclass,
rel, reverse):
for obj in objs:
fk_val = (
self.target_field.get_foreign_related_value(obj)[0]
- if isinstance(obj, self.model) else obj
+ if isinstance(obj, self.model) else
self.target_field.get_prep_value(obj)
)
if fk_val in old_ids:
old_ids.remove(fk_val)
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/31095#comment:2>

[Django]

#31095: Related Manager set() should prepare values before checking for missing
elements.
-------------------------------------+-------------------------------------

Reporter: TZanke | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Hasan Ramezani):

* owner: nobody => Hasan Ramezani
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/31095#comment:3>

[Django]

#31095: Related Manager set() should prepare values before checking for missing
elements.
-------------------------------------+-------------------------------------
Reporter: TZanke | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Hasan Ramezani):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/31095#comment:4>

[Django]

#31095: Related Manager set() should prepare values before checking for missing
elements.
-------------------------------------+-------------------------------------
Reporter: TZanke | Owner: Hasan
Type: | Ramezani

Cleanup/optimization | 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: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

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

Comment:

In [changeset:"a3fc24f01484d4bdd106bc6d7c49049fc5352af6" a3fc24f0]:
{{{
#!CommitTicketReference repository=""
revision="a3fc24f01484d4bdd106bc6d7c49049fc5352af6"
Fixed #31095 -- Made RelatedManager.set() preserve existing m2m relations
with an invalid type.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/31095#comment:5>

[Django]

#31095: Related Manager set() should prepare values before checking for missing
elements.
-------------------------------------+-------------------------------------
Reporter: TZanke | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | 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: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by TZanke):

Thank you! Will this be back ported to any other Django version?

The following line looks very similar, should this also be fixed?
https://github.com/django/django/blob/master/django/db/models/fields/related_descriptors.py#L743

--
Ticket URL: <https://code.djangoproject.com/ticket/31095#comment:6>

[Django]

#31095: Related Manager set() should prepare values before checking for missing
elements.
-------------------------------------+-------------------------------------
Reporter: TZanke | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | 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: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by felixxm):

> Thank you! Will this be back ported to any other Django version?

No, this patch doesn't qualify for a backport, it will be included in
Django 3.1.

> The following line looks very similar, should this also be fixed?

No, in this case `set()` expects models instances, so it's not affected.

--
Ticket URL: <https://code.djangoproject.com/ticket/31095#comment:7>