Opened 30 hours ago

Last modified 23 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 Jacob Walls, 30 hours ago

Component: Database layer (models, ORM)Documentation
Keywords: pre_save added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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 own DateField.pre_save implementations 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_save signal than the pre_save() field method, but I'd have to look closer. Committing to support for non-idempotent pre_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_value calls get_db_prep_value() on each of its base fields. For that reason if get_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.

comment:2 by Jacob Walls, 30 hours ago

Cc: Simon Charette added

comment:3 by Nino Walker, 30 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!

Version 0, edited 30 hours ago by Nino Walker (next)

comment:4 by HarshAlreja, 23 hours ago

Has patch: set

comment:5 by Jacob Walls, 23 hours ago

Has patch: unset

PR didn't target release notes.

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