#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 <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 [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019b997c6d7b-02512eb2-eeac-4df6-8b09-b6089d552fe0-000000%40eu-central-1.amazonses.com.
