Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#31095 closed Cleanup/optimization (fixed)

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

Reported by: TZanke Owned by: Hasan Ramezani
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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?

Change History (7)

comment:1 Changed 3 years ago by TZanke

Summary: Django Related Manager set methodDjango Related Manager set method may not work with pk strings

comment:2 Changed 3 years ago by Mariusz Felisiak

Easy pickings: set
Summary: Django Related Manager set method may not work with pk stringsRelated Manager set() should prepare values before checking for missing elements.
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 1.11master

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)

comment:3 Changed 3 years ago by Hasan Ramezani

Owner: changed from nobody to Hasan Ramezani
Status: newassigned

comment:4 Changed 3 years ago by Hasan Ramezani

Has patch: set

comment:5 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In a3fc24f0:

Fixed #31095 -- Made RelatedManager.set() preserve existing m2m relations with an invalid type.

comment:6 Changed 3 years ago 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

comment:7 Changed 3 years ago by Mariusz Felisiak

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.

Note: See TracTickets for help on using tickets.
Back to Top