[Django] #26058: Custom storage backend's not entirely decoupled from FileField
Django
-------------------------------------+-------------------------------------
Reporter: Korijn | Owner: nobody
Type: Bug | Status: new
Component: File | Version: 1.9
uploads/storage | Keywords: custom storage
Severity: Normal | filefield
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Let me start by saying that I've implemented custom FileFields that can
handle Numpy objects, to provide some convenience methods to access the
typed objects.
Later, when implementing a custom storage backend for Azure Storage, I
encountered some issues when handling filenames. Cloud storage doesn't
behave like local file system storage does. Most of Django's code is
agnostic of this problem, except for a small section of FileField:
In Azure, blobs are stored in containers, and are optionally stored in
subfolders (known as prefixes in Azure-speak). To be able to use the
upload_to property, I need to be able to pass the full path, e.g.:
container/blob
container/sub/blob
container/sub/sub/blob
Unfortunately, FileField strips the directory part and only passes the
blob name to my custom storage backend's get_valid_name implementation,
where I really need to validate the whole string. I ended up subclassing
FileField:
{{{
class CloudStorageFileField(FileField):
"""
Provides some overrides to enable cloud storage backends
"""
def get_directory_name(self):
return
force_text(datetime.datetime.now().strftime(force_str(self.upload_to)))
def get_filename(self, filename):
return filename
def generate_filename(self, instance, filename):
if callable(self.upload_to):
filename = self.upload_to(instance, filename)
filename = self.storage.get_valid_name(filename)
return filename
return self.get_directory_name() +
self.storage.get_valid_name(filename)
}}}
As you can see, all I did was remove the local file system related logic.
Unfortunately, my custom file fields need to inherit from this class to
work with Azure storage, and when I switch to local storage in a different
environment, they have to inherit from the regular FileField! This is
obviously an undesirable situation, imposed by the otherwise-great tight
coupling to local file system logic in the three methods of FileField.
In order to truly decouple this, FielField would need to be a little bit
more agnostic about the storage backend. This could be done by moving the
generate_filename method to the backend entirely, for example.
I classified this as a bug, as this case shows that you're not "entirely"
able to do anything you want when implementing custom storage backends.
You can see the full implementation of both the backend and the
numpyfilefield here: https://gist.github.com/Korijn/e0bcbdcedb494509973a
Your thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/26058>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Korijn | Owner: nobody
Type: Bug | Status: new
Component: File | Version: 1.9
uploads/storage |
Keywords: custom storage | Triage Stage:
filefield | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
container/blob
container/sub/blob
container/sub/sub/blob
def get_directory_name(self):
return
force_text(datetime.datetime.now().strftime(force_str(self.upload_to)))
return self.get_directory_name() +
self.storage.get_valid_name(filename)
}}}
obviously an undesirable situation, imposed by the tight coupling to local
file system logic in the three methods of FileField.
In order to truly decouple this, FileField would need to be a little bit
more agnostic about the storage backend. This could be done by moving the
generate_filename method to the backend entirely, for example.
I classified this as a bug, as this case shows that you're not
''entirely'' able to do anything you want when implementing custom storage
backends.
You can see the implementation of both the custom storage backend and the
NumpyFileField here: https://gist.github.com/Korijn/e0bcbdcedb494509973a
Your thoughts?
--
--
Ticket URL: <https://code.djangoproject.com/ticket/26058#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Korijn | Owner: nobody
Cleanup/optimization |
Component: File | Version: 1.9
uploads/storage |
Keywords: custom storage | Triage Stage: Accepted
filefield |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
The main question is whether or not the changes can be made in a backwards
compatible fashion.
--
Ticket URL: <https://code.djangoproject.com/ticket/26058#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Korijn | Owner: nobody
Cleanup/optimization |
Component: File | Version: 1.9
uploads/storage |
Keywords: custom storage | Triage Stage: Accepted
filefield |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Korijn):
How's this?
{{{
def generate_filename(self, instance, filename):
# allow for customized filename generation
if hasattr(self.storage, 'generate_filename'):
if callable(self.upload_to):
filename = self.upload_to(instance, filename)
return self.storage.generate_filename(filename)
filename =
force_text(datetime.datetime.now().strftime(force_str(self.upload_to)))
return self.storage.generate_filename(filename)
# If upload_to is a callable, make sure that the path it returns
is
# passed through get_valid_name() of the underlying storage.
if callable(self.upload_to):
directory_name, filename =
os.path.split(self.upload_to(instance, filename))
filename = self.storage.get_valid_name(filename)
return os.path.normpath(os.path.join(directory_name,
filename))
return os.path.join(self.get_directory_name(),
self.get_filename(filename))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26058#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Korijn | Owner: nobody
Cleanup/optimization |
Component: File | Version: 1.9
uploads/storage |
Keywords: custom storage | Triage Stage: Accepted
filefield |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Korijn):
Not sure if my edit generated a notification, so, bump.
--
Ticket URL: <https://code.djangoproject.com/ticket/26058#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Korijn | Owner: nobody
Cleanup/optimization |
Component: File | Version: 1.9
uploads/storage |
Keywords: custom storage | Triage Stage: Accepted
filefield |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
#26382 is a duplicate with a patch proposed:
[https://github.com/django/django/pull/6321 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/26058#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Korijn | Owner: nobody
Cleanup/optimization |
Component: File | Version: 1.9
uploads/storage |
Keywords: custom storage | Triage Stage: Accepted
filefield |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26058#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Korijn | Owner: nobody
Cleanup/optimization |
Component: File | Version: 1.9
uploads/storage |
Keywords: custom storage | Triage Stage: Accepted
filefield |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/6538 Updated PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/26058#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Korijn | Owner: nobody
Cleanup/optimization |
Component: File | Version: 1.9
uploads/storage |
Keywords: custom storage | Triage Stage: Accepted
filefield |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"914c72be2abb1c6dd860cb9279beaa66409ae1b2" 914c72b]:
{{{
#!CommitTicketReference repository=""
revision="914c72be2abb1c6dd860cb9279beaa66409ae1b2"
Fixed #26058 -- Delegated os.path bits of FileField's filename generation
to the Storage.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26058#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: File | Version: 1.9
uploads/storage |
Keywords: custom storage | Triage Stage: Accepted
filefield |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"0dfc5479a8e50215866bbf43604bed8416a1b504" 0dfc5479]:
{{{
#!CommitTicketReference repository=""
revision="0dfc5479a8e50215866bbf43604bed8416a1b504"
Refs #26058 -- Removed deprecated
FileField.get_directory_name()/get_filename().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26058#comment:9>