Re: [Django] #36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed
Component: Documentation | Version: 6.0
Severity: Normal | Resolution: needsinfo
Keywords: upload_to, | Triage Stage:
auto_now_add | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ran Benita):

Hi, thanks for looking.

> And testing both my view calling model.save() as well as the admin's add
form, gives me AssertionError for a missing created on all versions back
to 4.2.

Interesting, I just tried it now with a fresh project and I don't get an
assertion error on 5.2.10, but I do get it on 6.0.

I was able to bisect it to the following commit, which seems related:

{{{
commit 94680437a45a71c70ca8bd2e68b72aa1e2eff337
Author: Simon Charette <chare...@gmail.com>
Date: Wed Mar 19 01:39:19 2025 -0400

Fixed #27222 -- Refreshed model field values assigned expressions on
save().

Removed the can_return_columns_from_insert skip gates on existing
field_defaults tests to confirm the expected number of queries are
performed and that returning field overrides are respected.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed
Component: Documentation | Version: 6.0
Severity: Normal | Resolution: needsinfo
Keywords: upload_to, | Triage Stage:
auto_now_add | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Are you using MySQL?
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:6>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed
Component: Documentation | Version: 6.0
Severity: Normal | Resolution: needsinfo
Keywords: upload_to, | Triage Stage:
auto_now_add | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Ran Benita):

My real project (where I first encountered it) is using PostgreSQL. I did
the bisection using SQLite (default startproject configuration).
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:7>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed
Component: Documentation | Version: 6.0
Severity: Normal | Resolution: needsinfo
Keywords: upload_to, | Triage Stage:
auto_now_add | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Aha! I reversed the order of my fields such that the `DateTimeField`
precedes the `FileField`, like your model, and I reproduced. I'll dig a
little bit more.
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:8>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed

Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution: needsinfo
Keywords: upload_to, | Triage Stage: Accepted
auto_now_add, pre_save |

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

Changes (by Jacob Walls):

* cc: Simon Charette (added)
* component: Documentation => Database layer (models, ORM)
* keywords: upload_to, auto_now_add => upload_to, auto_now_add, pre_save
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted

Comment:

Wonderful, thanks for the bisect.

So I do think there is a regression in
94680437a45a71c70ca8bd2e68b72aa1e2eff337, in that we should call
`pre_save()` with `add=True` if we know we are doing an insert like this:

{{{#!diff
diff --git a/django/db/models/base.py b/django/db/models/base.py
index ad3f0c5e23..a1ac2ced98 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -1175,7 +1175,7 @@ class Model(AltersData, metaclass=ModelBase):
].features.can_return_columns_from_insert
for field in insert_fields:
value = (
- getattr(self, field.attname) if raw else
field.pre_save(self, False)
+ getattr(self, field.attname) if raw else
field.pre_save(self, add=True)
)
if hasattr(value, "resolve_expression"):
if field not in returning_fields:
}}}

... because before this code was added, this was accomplished with
`add=True` during `SQLInsertCompiler.as_sql()`...

{{{#!py
field_pre_save = partial(self.pre_save_val, field)
field_values = [
field_prepare(field_pre_save(obj)) for obj in
self.query.objs
]
}}}

... since `pre_save_val()` fathoms `add`:
{{{#!py
def pre_save_val(self, field, obj):
"""
Get the given field's value off the given obj. pre_save() is used
for
things like auto_now on DateTimeField. Skip it if this is a raw
query.
"""
if self.query.raw:
return getattr(obj, field.attname)
return field.pre_save(obj, add=True)
}}}

So by not fathoming the `add` argument at this earlier point, we introduce
a new vector for failures. I'm assuming we didn't bother with `add=True`
given that we're throwing away the returned value, but this report points
to a backward-compatibility issue. Tentatively accepting, since I'm
expecting we'll get a more clear idea of the impact here when drafting a
test case, and it's possible that there's some history about
`auto_now_add` I need to read up on w/r/t `pre_save()`.

Now, to the order-dependent thingy. It looks like you were relying on a
fragile behavior. Another good reason to move off of `auto_now_add`, as
you say. I think if we fix this regression then I'm not seeing as much of
a need for a docs update. Thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:9>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution: needsinfo
Keywords: upload_to, | Triage Stage: Accepted
auto_now_add, pre_save |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Thanks for the CC Jacob.

We should definitely pass `add=True` as you pointed out for correctness
reasons but given we discard the values if they are not expressions this
seems more like a way to avoid subtle undefined behavior breakages that we
can afford but it doesn't require too much gymnastics on our end.

In other words, I believe that landing a patch here should be seen as a
way to correctly invoke the saving machinery (we've historically called
`pre_save` with `add=True`) over a long term commitment over model field
definition order dictating attribute presence on model instances.

For example, something else that subtly changed is that we now call
`pre_save` twice (in `Model._save_table` and `SQLInsertCompiler.as_sql`)
and is something that would require move invasive changes that flipping
`add=True` to resolve.
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:10>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution: needsinfo
Keywords: upload_to, | Triage Stage: Accepted
auto_now_add, pre_save |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

> I believe that landing a patch here should be seen as a way to correctly
invoke the saving machinery (we've historically called pre_save with

add=True) over a long term commitment over model field definition order

dictating attribute presence on model instances.

I'm on board with that. Should we then add a caveat on `pre_save()` that
it should be idempotent? We advertise a similar caveat on
`AppConfig.ready()`:

> In the usual initialization process, the ready method is only called
once by Django. But in some corner cases, particularly in tests which are
fiddling with installed applications, ready might be called more than
once. In that case, either write idempotent methods, or put a flag on your
AppConfig classes to prevent rerunning code which should be executed
exactly one time.

Then again maybe this is less important to note if we assume most use
cases for `pre_save()` involve supplying missing values without side
effects.
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:11>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution: needsinfo
Keywords: upload_to, | Triage Stage: Accepted
auto_now_add, pre_save |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

> Should we then add a caveat on pre_save() that it should be idempotent?

I think we can't until we adjust `DateField.pre_save` and
`DateTimeFied.pre_save` to be.
[https://github.com/django/django/blob/7c1b3391d35fb10e912783fa6c87712e40de22dc/django/db/models/fields/__init__.py#L1508-L1512
As of now] they always generate a new value on call and assign it.

We could gate the `auto_now_add=True and add` generation by the presence
of the attribute on the provided model instances but we can't do that for
`auto_now=True` as the call of the function is the ''signal'' that a new
value should be generated.
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:12>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution: needsinfo
Keywords: upload_to, | Triage Stage: Accepted
auto_now_add, pre_save |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Nashrh Ashraf Khan):

Replying to [comment:3 Jacob Walls]:
> Hi, thanks for the report.
>
> I tried with a very similar model that I already had wired up with a
view:
>
> {{{#!py
> def jtw(instance, filename):
> assert instance.created
>
> class TempFile(models.Model):
> fileid = models.UUIDField(primary_key=True, default=uuid.uuid4)
> path = models.FileField(upload_to=jtw)
> created = models.DateTimeField(auto_now_add=True)

> }}}
>
> And testing both my view calling model.save() as well as the admin's add
form, gives me `AssertionError` for a missing `created` on all versions
back to 4.2.
>

> Were you using a custom form?
>
> > I will bisect to identify the commit where the behavior changed.
>
> Nashrh, did you get a bisect result?
>
> Happy to take another look when we have more info to advance the
investigation.

Hi,
Thanks Jacob. Ran Benita was able to bisect this to commit
94680437a45a71c70ca8bd2e68b72aa1e2eff337 which introduced the regression.

After reviewing the bisected commit and the discussion, I agree this is a
regression introduced by 94680437a45a71c70ca8bd2e68b72aa1e2eff337.

The insert path now calls field.pre_save(..., add=False) which prevents
auto_now_add from populating values before upload_to() runs.

Restoring add=True in Model._save_table appears consistent with historical
behavior and avoids order-dependent failures.

I’m preparing a patch with a regression test.
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:13>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan
Type: Bug | Status: closed
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution: needsinfo
Keywords: upload_to, | Triage Stage: Accepted
auto_now_add, pre_save |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Hi, thanks for your participation, but please avoid pasting repetitive
summaries here.
--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:14>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------
Reporter: Ran Benita | Owner: Nashrh
| Ashraf Khan

Type: Bug | Status: new

Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: upload_to, | Triage Stage: Accepted
auto_now_add, pre_save |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* resolution: needsinfo =>
* status: closed => new

--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:15>

[Django]

#36847: FileField(upload_to=...) callback no longer sees `auto_now_add ` field
-------------------------------------+-------------------------------------

Reporter: Ran Benita | Owner: Nilesh
| Pahari
Type: Bug | Status: assigned

Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: upload_to, | Triage Stage: Accepted
auto_now_add, pre_save |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Nilesh Pahari):

* owner: Nashrh Ashraf Khan => Nilesh Pahari
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36847#comment:16>