Re: [Django] #11027: Storage backends should know about the max_length attribute of the FileFields
Django
FileFields
------------------------------------+--------------------------------------
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Milestone: 1.3 | Component: File uploads/storage
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
------------------------------------+--------------------------------------
Changes (by carljm):
* type: New feature => Bug
* easy: => 0
* stage: Design decision needed => Accepted
Comment:
Marking this as a bug, since #15247 makes it clear that's what it is.
Discussed briefly with jezdez on IRC and concluded that the fix proposed
here probably is the right one, adding a max_length argument to the
get_valid_name method of storage classes. Unfortunately, this is backwards
incompatible, and it won't just break code that was relying on buggy
behavior, it will break all custom storage backends. So it requires a
deprecation path, and during the deprecation period that will require
introspecting the get_valid_name method to see whether it accepts the
valid_name argument (or trying with, catching the error, and then trying
without).
Either one of those is kind of ugly, but the alternative is to introduce a
new method, like "get_valid_name_with_max_length". That makes the
deprecation-period code a bit nicer, but then we're stuck with the longer
method name for good. Probably better to take the short-term hit.
--
Ticket URL: <https://code.djangoproject.com/ticket/11027#comment:11>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
FileFields
------------------------------------+--------------------------------------
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Milestone: 1.3 | Component: File uploads/storage
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
------------------------------------+--------------------------------------
Changes (by hvdklauw):
* cc: hvdklauw (removed)
* ui_ux: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/11027#comment:12>
Django
FileFields
------------------------------------+--------------------------------------
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Milestone: 1.3 | Component: File uploads/storage
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------+--------------------------------------
Changes (by hvdklauw):
* cc: hvdklauw@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/11027#comment:13>
Django
FileFields
------------------------------------+--------------------------------------
Reporter: apollo13 | Owner: nobody
Type: Bug | Status: new
Milestone: 1.3 | Component: File uploads/storage
Version: SVN | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------+--------------------------------------
Comment (by ishalyapin):
Hey, it is not very difficult problem. I wrote a solution and use it in my
projects.
It is not perfect solution, but it is much better then unhandled
exception.
I hope you take it in django.
{{{#!python
def filefield_maxlength_validator(value):
""""
Check if absolute file path can fit in database table
and filename length can fit in filesystem.
"""
FILESYSTEM_FILENAME_MAXLENGTH = 255 # ext4, for example
RESERVE = 50 # We need reserve for generating thumbnails and for
suffix if filename already exists, example - filename_85x85_1.jpg
# For the initial file saving value.path contains not the same path,
which will be used to save file
filename = value.name
filepath = os.path.join(
settings.MEDIA_ROOT,
value.field.generate_filename(value.instance, filename)
)
bytes_filename = len(filename.encode('utf-8')) # filename length in
bytes
bytes_filepath = len(filepath.encode('utf-8')) # path length in bytes
# File path length should fit in table cell and filename lenth should
fit in in filesystem
if bytes_filepath > value.field.max_length or bytes_filename >
FILESYSTEM_FILENAME_MAXLENGTH - RESERVE:
if os.path.isfile(value.path):
os.remove(value.path)
raise exceptions.ValidationError(_(u"File name too long."))
return value
FileField.default_validators = FileField.default_validators[:] +
[filefield_maxlength_validator]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/11027#comment:14>
Django
FileFields
Reporter: apollo13 | Owner: melinath
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by melinath):
* owner: nobody => melinath
--
Ticket URL: <https://code.djangoproject.com/ticket/11027#comment:16>
Django
FileFields
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: melinath
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by melinath):
* status: new => assigned
Comment:
Overriding get_valid_name is probably not a workable solution. Currently,
it claims only to return a filename "that's suitable for use in the target
storage system." This just means a short cleaning of whitespace
characters, hyphens, etc. So making it actually return a valid filename
(in terms of length) would be a change in mission. Additionally, we
actually need to validate the length of the entire storage path (which
get_valid_name doesn't have access to). And finally, even if we got a
valid name, get_available_name can increase the length of that name during
the saving process, leaving us exactly where we started.
get_available_name needs to take a max_length argument, or it needs to be
able to guarantee that it won't increase the length of the filename. Since
the latter isn't possible as far as I can see, I expect to do the former.
If get_available_name *can't* make a name within the given max length, it
will raise a !ValueError. !FileField validation would then need to call
get_available_name *after* generating a filename and instead of (or after)
a simple check of the length of the filename. Any !ValueError raised can
be re-raised as a !ValidationError.
As an alternative, we could add a new storage method called (say)
get_valid_storage_path, which would handle max_length and the
available_name checks, then deprecate get_available_name.
--
Ticket URL: <https://code.djangoproject.com/ticket/11027#comment:17>
Django
FileFields
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: melinath
Type: Bug | Status: assigned
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by akaariai):
I kind of like the idea that get_valid_name() just converts the name to be
something valid for the storage backend. Then, get_available_name has the
final control of how to truncate the name, and to make sure the name is
within the name length limit. The contract is altered from "gimme an
available file name as close to the requested name" to "gimme an available
file name as close to the requested name, max length N".
It seems the final name is determined by .save() - if the file name
clashes due to race conditions, then .save() can alter the file name. This
brings another point to the discussion: if final file name is determined
by .save(), then shouldn't .save() also know the file name length
restriction?
As for implementation, I like the try-except approach most.
--
Ticket URL: <https://code.djangoproject.com/ticket/11027#comment:18>
Django
FileFields
--------------------------------------+------------------------------------
Reporter: apollo13 | Owner: melinath
Type: Bug | Status: assigned
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by melinath):
Yes, .save() (and ._save()) would also need to know about the max_length
restriction.
--
Ticket URL: <https://code.djangoproject.com/ticket/11027#comment:19>
Django
FileFields
Reporter: apollo13 | Owner: melinath
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* status: assigned => closed
* cc: cmawebsite@… (added)
* resolution: => duplicate
Comment:
I think this might be a 5 year long duplicate of #9893 which is now fixed
:) Feel free to reopen if it's different.
--
Ticket URL: <https://code.djangoproject.com/ticket/11027#comment:20>