[Django] #23759: Storage.get_available_name should preserve all file extensions, not just the first one
Django
the first one
--------------------------------------+--------------------
Reporter: thenewguy | Owner: nobody
Type: Uncategorized | Status: new
Component: File uploads/storage | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+--------------------
`get_available_name` only preserves the first file extension. If the file
is `foo.tar.gz` then `get_available_name` will return `foo.tar_RANDOM.gz`
instead of `foo_RANDOM.tar.gz`
https://github.com/django/django/blob/master/django/core/files/storage.py#L71
{{{
def get_available_name(self, name):
"""
Returns a filename that's free on the target storage system, and
available for new content to be written to.
"""
dir_name, file_name = os.path.split(name)
file_root, file_ext = os.path.splitext(file_name)
# If the filename already exists, add an underscore and a random 7
# character alphanumeric string (before the file extension, if one
# exists) to the filename until the generated filename doesn't
exist.
while self.exists(name):
# file_ext includes the dot.
name = os.path.join(dir_name, "%s_%s%s" % (file_root,
get_random_string(7), file_ext))
return name
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23759>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
the first one
Reporter: thenewguy | Owner: nobody
Type: Uncategorized | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
In general, I think a period is a valid character in a filename, so we
cannot simply assume everything to the right of the first period is the
file extension. Some different strategies for the issue are detailed on
[http://stackoverflow.com/questions/6525334/getting-file-extension-using-
pattern-matching-in-python stackoverflow]. I am not sure its up to Django
to pick one, but we could at least document the caveat and link how to
write your own storage subclass so someone can implement the behavior they
desire. Thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:1>
Django
the first one
Reporter: thenewguy | Owner: nobody
Type: Uncategorized | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by thenewguy):
Sorry for the delay. I just found out status updates on tickets I am
watching have been going to spam. I guess that is why I never get
updates.
In this case, I do not think it is an issue if a valid part of the
filename is assumed to be an extension because `get_available_name` does
not promise to return the same filename, and it doesn't matter where the
random string is in the name to be "available".
However, the mimetypes module could be used to determine if an extension
was actually an extension. If mimetypes returns a type or an encoding,
add the extension to the extensions list, otherwise break and assume there
are no more extensions.
Also, this only happens if the submitted filename already exists.
I think it would be very nice to be able to maintain reliable file
extensions out of the box. This is important for many cases, including
when a user downloads the file.
Here is a sample mixin I've used in the past (keep in mind it does not
query mimetypes, but that is trivial)
{{{
class GetUUIDAvailableNameStorageMixin(object):
def get_available_name(self, name):
"""
Returns a hex encoded uuid filename, using the extensions provided
by the
input filename, that's suitable for use in the target storage
system.
"""
exts = []
root = ext = name
while ext:
root, ext = splitext(root)
exts.insert(0, ext)
name = "%s%s" % (uuid4().hex, "".join(exts))
return super(GetUUIDAvailableNameStorageMixin,
self).get_available_name(name)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:2>
Django
the first one
Reporter: thenewguy | Owner: nobody
Type: Uncategorized | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by claudep):
I think that if we can do it without introducing too much complexity, we
should do it. Could you provide a patch with tests?
--
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:3>
Django
the first one
Reporter: thenewguy | Owner: thenewguy
Type: Uncategorized | Status: assigned
Component: File | Version: 1.7
uploads/storage | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by thenewguy):
* owner: nobody => thenewguy
* status: new => assigned
* stage: Unreviewed => Accepted
Comment:
assigned and marked accepted per
https://code.djangoproject.com/ticket/23759#comment:3
--
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:4>
Django
the first one
Reporter: thenewguy | Owner: thenewguy
Component: File | Version: master
uploads/storage | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* version: 1.7 => master
* type: Uncategorized => New feature
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:5>
Django
the first one
Reporter: thenewguy | Owner: (none)
Type: New feature | Status: new
Component: File uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by felixxm):
* owner: thenewguy => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:6>
Django
the first one
Reporter: thenewguy | Owner: Adam
| Zapletal
uploads/storage |
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 Adam Zapletal):
* owner: (none) => Adam Zapletal
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:7>
Django
the first one
Reporter: thenewguy | Owner: Adam
| Zapletal
Type: New feature | Status: assigned
Component: File | Version: dev
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Adam Zapletal):
--
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:8>
Django
the first one
Reporter: thenewguy | Owner: Adam
| Zapletal
Type: New feature | Status: assigned
Component: File | Version: dev
uploads/storage |
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/17881 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:9>
Django
the first one
Reporter: thenewguy | Owner: Adam
| Zapletal
uploads/storage |
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"eb2d49b73408f1e60b8e1fa7fee822816945e1cd" eb2d49b]:
{{{#!CommitTicketReference repository=""
revision="eb2d49b73408f1e60b8e1fa7fee822816945e1cd"
Fixed #23759 -- Preserved all file extensions in
Storage.get_available_name().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23759#comment:10>