#25363: Model fields pre_save not called if force_insert=True
-------------------------------------+-------------------------------------
Reporter: davidfischer-ch | Owner:
| davidfischer-ch
Type: Bug | Status: closed
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: pre_save,save | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by davidfischer-ch):
Hi akaariai,
Your mix-in is interesting but it has some issues, hopefully easy to fix.
1. This is actually broken by the `pre_save` logic for a simple reason :
The `pre_save` methods are called inside the save, a.k.a inside the
`super().save(...)` so your mix-in and my mix-in [https://github.com
/davidfischer-
ch/pytoolbox/blob/master/pytoolbox/django/models/mixins.py#L115
AutoUpdateFieldsMixin] will not detect those. Its a reason why I
implemented [https://github.com/davidfischer-
ch/pytoolbox/blob/master/pytoolbox/django/models/mixins.py#L153
CallFieldsPreSaveMixin].
I also implemented another mix-in (sometimes I feel like a DJ, doing many
mixes),[https://github.com/davidfischer-
ch/pytoolbox/blob/master/pytoolbox/django/models/mixins.py#L89
AutoRemovePKFromUpdateFieldsMixin] that will check if the primary key is
set to a new value, then the intention of the developer is probably to
duplicate the model by saving it with a new primary key. So the mix-in let
Django save with its own default options for save.
2. `__dict__.copy()` is not sufficient, if one of the fields, is, for
example, a `JSONField` then the content is a mutable type (`dict`). This
container may contains nested mutable types, and you have to `deepcopy` to
ensure your copy will reflect the unchanged data from the database and
will not be modified.
3. your overload of `save()` isn't generic enough to support all possible
arguments.
Interesting you saved the data in `_state`.
That's cleaner than my mix-in.
I am thinking loudly, but is it more performant that any field set by the
code should be updated regardless the value, something that will not
happen often, or is it better to check for diff. Checking for diff will
also double the memory usage for the data (is it any copy-on-write mix-in
out here?).
We also need to think about concurrent updates when designing a save-the-
changes mix-in. For that reason I implemented another mix-in
[https://github.com/davidfischer-
ch/pytoolbox/blob/master/pytoolbox/django/models/mixins.py#L243
UpdatePreconditionsMixin]. I don't know what is the best option, but maybe
its more explicit and intuitive that if the program sets the fields, that
they will be saved regardless the value.
Yes, many questions and thoughts.
I am eager to heard about you guys.
--
Ticket URL: <https://code.djangoproject.com/ticket/25363#comment:8>
Django <https://code.djangoproject.com/>
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/073.914a7fc2580f1fa3e489797dcd367a0c%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
