Opened 13 hours ago
Last modified 5 hours ago
#36855 new Bug
Field.pre_save() called twice during save() in Django 6.0 when inserting new records
| Reported by: | Nino Walker | Owned by: | |
|---|---|---|---|
| Component: | Documentation | Version: | 6.0 |
| Severity: | Release blocker | Keywords: | pre_save |
| Cc: | Nino Walker, Simon Charette | Triage Stage: | Accepted |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
Commit 94680437 introduced a change where Field.pre_save() can be called twice during Model.save() for new records:
Diff: https://github.com/django/django/commit/94680437a45a71c70ca8bd2e68b72aa1e2eff337
Full method: https://github.com/django/django/blob/94680437a45a71c70ca8bd2e68b72aa1e2eff337/django/db/models/base.py#L1085-L1164
Previously, pre_save() was only called once in the values comprehension.
Impact:
This is a breaking change for custom model fields that:
- Have side effects in pre_save() (e.g., registering transaction.on_commit callbacks)
- Transform the value in a way that's not idempotent
- Maintain internal state based on pre_save() being called once per save
Example:
A custom field that registers an on_commit callback in pre_save() will now register it twice, causing the callback to execute twice.
Expected behavior:
Field.pre_save() should be called exactly once per Model.save() operation, regardless of whether the save results in an INSERT or UPDATE.
Change History (5)
comment:1 by , 12 hours ago
| Component: | Database layer (models, ORM) → Documentation |
|---|---|
| Keywords: | pre_save added |
| Severity: | Normal → Release blocker |
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 12 hours ago
| Cc: | added |
|---|
comment:3 by , 12 hours ago
Thank you for the prompt reply. The specific context was with the django-gcp library, which I was testing for compatibility with 6.0. I haven't been in the Django internals since 1.2, so I won't argue the virtues of your position. Below is my exact context workaround:
https://github.com/ninowalker/django-gcp/pull/2/changes
Specific workaround: https://github.com/ninowalker/django-gcp/pull/2/changes#diff-d92dddc7dd622b9917b9a6f5b0a4dee29f5c8d5c1870831953399c681626d9ac
Thanks!
Here's the implementation of the storage field: https://github.com/octue/django-gcp/blob/main/django_gcp/storage/fields.py (from the upstream repository)
Thanks for the report. This was discussed just a couple days ago, but I'm happy to have a new ticket for clarity.
I suggested documenting that
pre_save()functions should be idempotent in ticket:36847#comment:11, but Simon pointed out we should update our ownDateField.pre_saveimplementations first. (They aren't technically idempotent, but they're pretty close. They're certainly side-effect free.)Some of the use cases you mention seem a better fit for the
pre_savesignal than thepre_save()field method, but I'd have to look closer. Committing to support for non-idempotentpre_save()methods is a new feature that would merit some discussion elsewhere on the forum or django/new-features. Take the example of another field method:ArrayField.get_db_prep_valuecallsget_db_prep_value()on each of its base fields. For that reason ifget_db_prep_value()had side effects, that would be a problem.We likely should have documented this as a minor incompatible change in 6.0, so I'm reframing this as a documentation fix for the release notes for now. Django has a grain, and one way to follow the grain is ensure that
pre_save()only preprocesses single values without side effects. Side effects can be orchestrated in many other ways.