Re: [Django] #9893: Filename + path length greater than 100 truncated on database insertion in Core.Storage
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner:
Type: Bug | streetcleaner
Component: Core (Other) | Status: assigned
Severity: Normal | Version: SVN
Keywords: storage, filename | Resolution:
Has patch: 1 | Triage Stage: Accepted
Needs tests: 1 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by dekkers):
* cc: jeroen@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:24>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Core.Storage
Reporter: Refefer | Owner: aaugustin
Type: Bug | Status: new
Component: Core (Other) | Version: SVN
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+-------------------------------------
Changes (by aaugustin):
* owner: streetcleaner => aaugustin
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:25>
Django
Core.Storage
-----------------------------------+-------------------------------------
Reporter: Refefer | Owner: aaugustin
Component: Core (Other) | Version: master
Severity: Normal | Resolution: fixed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [dcd4383107d96c18bcb53312ca4de070374b334c]:
{{{
#!CommitTicketReference repository=""
revision="dcd4383107d96c18bcb53312ca4de070374b334c"
Fixed #9893 -- Validated the length of file names
after the full file name is generated by the storage class.
Thanks Refefer for the report, carsongee for the patch, and
everyone else involved in the discussion.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:26>
Django
Core.Storage
-----------------------------------+-------------------------------------
Reporter: Refefer | Owner: aaugustin
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Severity: Normal | Resolution: fixed
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+-------------------------------------
Comment (by Claude Paroz <claude@…>):
In [05d333ba3bb16af024c11966d2072de38fe9f82f]:
{{{
#!CommitTicketReference repository=""
revision="05d333ba3bb16af024c11966d2072de38fe9f82f"
Fixed #18515 -- Conditionally regenerated filename in FileField validation
When a FileField value has been saved, a new validation should not
regenerate a new filename when checking the length. Refs #9893.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:27>
Django
Core.Storage
-----------------------------------+-------------------------------------
Reporter: Refefer | Owner: aaugustin
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
I'm going to revert that commit because of the regression reported in
#19525.
Here are the relevant bits of that ticket:
-----
As of dcd43831 (fixing #9893), a FileField will call `generate_filename()`
as part of the validation step for a FileField on a form. This was then
updated in 05d333ba to address #18515.
Unfortunately, this means that the filename function provided as an
argument to upload_to can no longer reference any field with a pre-save
behavior.
The common use case for this is to organize files on disk according to
upload date. For example:
{{{
def user_filename(instance, filename):
return os.path.join('user_files',
instance.uploaded_timestamp.strftime('%Y-%m-%d'), filename)
class UserFile(models.Model):
uploaded_timestamp = models.DateTimeField(auto_now_add=True)
data = models.FileField(upload_to=user_filename)
}}}
Under Django 1.5, attempting to call is_valid() on a Modelform for this
model will raise a "'NoneType' object has no attribute 'strftime'"
exception, because instance.uploaded_timestamp hasn't been instantiated
yet. This is despite the fact that the uploaded data has been provided,
the generated filename would be valid, and the upload timestamp can be
computed.
In Django 1.4 and earlier, this works because no validation was performed
for FileFields filenames; the uploaded_timestamp was evaluated as part of
the model pre-save, and the persistence of the file to disk occurred after
the model was saved.
To my reading,
[https://docs.djangoproject.com/en/1.4/ref/models/fields/#django.db.models.FileField.upload_to
the documentation is ambiguous] on whether this is expected behavior or
not. It says that the model may not be saved to the database yet, but
points at AutoField as the cause for problems. However, it also explicitly
talks about using strftime as part of file paths. A file datetimes of
'now' would seem to be an obvious usage of this feature.
----
For the record, I discovered this by upgrading a commercial project to
Django 1.5, so there is at least one project in the wild that will be
affected by this change. Although I've discovered it with a auto_now_add
FileField, it's not hard to see that this change also affects any field
with a pre_save behaviour.
It also has the potential to lead to incorrect validation. Consider the
case of a field with a pre_save behavior that updates the field (auto_now
is one example, but any denormalization/summary field would be an
example). The call to validate occurs *before* the call to pre_save is
made, which means that you're going to get the pre_save value used as part
of your validation. If you then save the model, the pre_save() will be
called, and the actual filename that is used for saving the file will be
different to the one used for validation.
Some initial thoughts about possible solutions:
* Document that you can't use a field with pre-save behaviour. Not ideal
IMHO, since it rules out an obvious use case for upload_to.
* Roll back the fix. Also less than ideal; #9893 is an edge case bug, but
it's something that has been seen in the wild, and isn't *too* hard to
generate.
* Invoke pre_save on all model fields prior to validation. Given that
most validation doesn't need this, this approach seems a little excessive.
----
I've just checked with my production code, and yes, `default=timezone.now`
works for the `auto_now_add case`. However, it won't address `auto_now`,
or the `pre_save` conflict problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:28>
Django
Core.Storage
-----------------------------------+-------------------------------------
Reporter: Refefer | Owner: aaugustin
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"db278c3bf9177043c42a9ed8b529a5c117938460"]:
{{{
#!CommitTicketReference repository=""
revision="db278c3bf9177043c42a9ed8b529a5c117938460"
Fixed #19525 -- Reverted dcd4383107 and 05d333ba3b.
Refs #9893, #18515.
Thanks Russell for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:29>
Django
Core.Storage
-----------------------------------+-------------------------------------
Reporter: Refefer | Owner: aaugustin
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+-------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"3cb87ec605fb9e7785aa0580bd8220797b622e0c"]:
{{{
#!CommitTicketReference repository=""
revision="3cb87ec605fb9e7785aa0580bd8220797b622e0c"
[1.5.x] Fixed #19525 -- Reverted dcd4383107 and 05d333ba3b.
Refs #9893, #18515.
Thanks Russell for the report.
Backport of db278c3 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:30>
Django
Core.Storage
-----------------------------------+-------------------------------------
Reporter: Refefer | Owner: aaugustin
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+-------------------------------------
Comment (by Preston Holmes <preston@…>):
In [changeset:"0e431e5dd7ed19aa2119ceba9ebed050c2988844"]:
{{{
#!CommitTicketReference repository=""
revision="0e431e5dd7ed19aa2119ceba9ebed050c2988844"
[1.5.x] Fixed #19525 -- Reverted dcd4383107 and 05d333ba3b.
Refs #9893, #18515.
Thanks Russell for the report.
Backport of db278c3 from master.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:31>
Django
Core.Storage
-----------------------------------+-------------------------------------
Reporter: Refefer | Owner: aaugustin
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+-------------------------------------
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:32>
Django
Core.Storage
-----------------------------------+------------------------------------
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------
* owner: aaugustin =>
* status: assigned => new
Comment:
I don't know how to solve this properly.
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:33>
Django
Core.Storage
Reporter: Refefer | Owner:
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* component: Core (Other) => File uploads/storage
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:34>
Django
Core.Storage
--------------------------------------+------------------------------------
Reporter: Refefer | Owner:
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* cc: walter+django@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:35>
Django
Core.Storage
--------------------------------------+------------------------------------
Reporter: Refefer | Owner:
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by wdoekes):
So, I ran into bug #13314. These are all similar but not quite the same.
#9893 -- Filename + path length greater than 100 (when making filenames
unique)[[br]]
#10410 -- FileField saves one filename on disk and another on DB[[br]]
#13314 -- validation does not account for "upload_to" when counting
characters
This ticket #9893 is about the changed filenames when making them unique.
Ticket #13314 is just about the upload_to parameter. For that problem the
fix could be done like this:
{{{
--- django/db/models/fields/files.py.orig 2013-04-01
17:03:59.752332630 +0200
+++ django/db/models/fields/files.py 2013-04-01 17:08:15.833870239
+0200
@@ -290,6 +290,8 @@
if 'initial' in kwargs:
defaults['required'] = False
defaults.update(kwargs)
+ # And deduct the upload_to length from the max_length.
+ defaults['max_length'] -= len(self.upload_to)
return super(FileField, self).formfield(**defaults)
class ImageFileDescriptor(FileDescriptor):
}}}
With this environment:
* ImageField with max_length default 100
* An upload_to value of "profile/" (8 characters)
Without the fix, I get a DatabaseError for a 99-byte length
filename.[[br]]
With the fix, I get a nice ValidationError until I reduce the filename to
92 bytes.
That should be at least be one less problem, right?
This was tested with Django 1.3.7.
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:36>
Django
Core.Storage
Reporter: Refefer | Owner:
Type: Bug | pavel_shpilev
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by pavel_shpilev):
* status: new => assigned
* owner: => pavel_shpilev
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:37>
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner:
Type: Bug | pavel_shpilev
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by pavel_shpilev):
Hey guys
Long time no activity on the ticket...
I have recently run into this bug on production myself. We needed a
solution ASAP, so I ended up with an overwrite for get_available_name(),
providing max_filename_length as an argument. Pretty much what @akaihola
first suggested in [https://code.djangoproject.com/ticket/9893#comment:5].
Seem to be working fine for us. I can try implement this as a general
solution now, if you think this would be a proper fix.
Cheers.
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:38>
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner:
Type: Bug | pavel_shpilev
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: charettes (added)
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
pavel_shpilev put together [https://github.com/django/django/pull/3369 a
PR] based on ticket:9893#comment:1 that still requires some adjustments.
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:39>
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner:
Type: Bug | pavel_shpilev
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
Pavel Shpilev wants another review.
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:40>
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner:
Type: Bug | pavel_shpilev
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by berkerpeksag):
* needs_docs: 1 => 0
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/3369 PR #3369] LGTM.
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:41>
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner:
Type: Bug | pavel_shpilev
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Comment for improvement is on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:42>
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner:
Type: Bug | Status: assigned
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
Keywords: storage, filename | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:43>
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner:
| pavel_shpilev
Component: File | Version: master
uploads/storage |
Keywords: storage, filename | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a7c256cb5491bf2a77abdff01638239db5bfd9d5"]:
{{{
#!CommitTicketReference repository=""
revision="a7c256cb5491bf2a77abdff01638239db5bfd9d5"
Fixed #9893 -- Allowed using a field's max_length in the Storage.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:44>
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner:
| pavel_shpilev
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: storage, filename | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"1bb6ecf6d3b31bd606754ddbd1398550f605d3e5" 1bb6ecf]:
{{{
#!CommitTicketReference repository=""
revision="1bb6ecf6d3b31bd606754ddbd1398550f605d3e5"
Refs #9893 -- Removed shims for lack of max_length support in file storage
per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:45>
Django
Core.Storage
-------------------------------------+-------------------------------------
| Shpilev
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: storage, filename | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jan Geboers):
Hello,
could it be that this bug still exists?
Both on Django 1.8.18 and 1.11.2 I see this behavior isn't corrected yet.
The validation of max_length takes only file_name into account and ignores
upload_to, causing truncation on the database level, exactly as described
in https://code.djangoproject.com/ticket/13314
When I check out the relevant code I strongly suspect this bug was never
fixed:
https://github.com/django/django/blob/master/django/forms/fields.py#L553
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:46>
Django
Core.Storage
-------------------------------------+-------------------------------------
Reporter: Refefer | Owner: Pavel
| Shpilev
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution: fixed
Keywords: storage, filename | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
#13314 is reopened per the last comment.
--
Ticket URL: <https://code.djangoproject.com/ticket/9893#comment:47>