Inconsistencies in Storage API
Melvyn Sopacua
Hello,
the current Storage API has some inconsistencies and in short it's impossible to write anything that requires a directory to be made (if storage is FileSystemStorage) in an implementation agnostic way.
Aside from that, there's a listdir() method that only makes sense in a FileSystemStorage based implementation (DB storage could emulate it, but it isn't natural).
So, I had two ideas to fix it and would like to know developer preference before filing a PR:
- Remove FileSystemStorage API's from Storage (specifically listdir) and make save() do all the work to save the file, where the only acceptable failures are outside Django's control (read-only mounts, (table-)space issues, etc)
- Change listdir() to list_container and add methods that handle Storage containers: abstract things that hold the file. save() either deligates to container methods or requires that container exists before saving.
The use case that prompted this was that I've created an ImageField subclass that generates thumbnails in model-specified sizes and puts them in directories below the one of the current image (named widthxheight, f.e. 64x64). In doing so I had to go outside FieldFile.storage to have the intermediate directory created, so my code now isn't portable to different Storage subclasses.
Is there any interest in either of these (another?) approach and if so, what preference?
--
Melvyn Sopacua
Tim Graham
Melvyn Sopacua
On Monday 30 January 2017 08:16:20 Tim Graham wrote:
> Hi, could you point to where the problematic storage.listdir() call is
> in Django?
Sure.
https://docs.djangoproject.com/en/1.10/ref/files/storage/#django.core.files.storage.Storage.listdir
> On Monday, January 30, 2017 at 11:02:32 AM UTC-5, Melvyn Sopacua wrote:
> > Hello,
> >
> >
> >
> > the current Storage API has some inconsistencies and in short it's
> > impossible to write anything that requires a directory to be made
> > (if
> > storage is FileSystemStorage) in an implementation agnostic way.
> >
> >
> >
> > Aside from that, there's a listdir() method that only makes sense in
> > a FileSystemStorage based implementation (DB storage could emulate
> > it, but it isn't natural).
> >
> >
> >
> > So, I had two ideas to fix it and would like to know developer
> > preference>
> > before filing a PR:
> > 1. Remove FileSystemStorage API's from Storage (specifically
> > listdir)
> > and make save() do all the work to save the file, where the only
> > acceptable failures are outside Django's control (read-only
> > mounts, (table-)space issues, etc)
> > 2. Change listdir() to list_container and add methods that handle
> > Storage containers: abstract things that hold the file. save()
> > either
> > deligates to container methods or requires that container exists
> > before saving.
> >
> > The use case that prompted this was that I've created an ImageField
> > subclass that generates thumbnails in model-specified sizes and puts
> > them in directories below the one of the current image (named
> > widthxheight, f.e. 64x64). In doing so I had to go outside
> > FieldFile.storage to have the intermediate directory created, so my
> > code now isn't portable to different Storage subclasses.
> >
> >
> >
> > Is there any interest in either of these (another?) approach and if
> > so, what preference?
> >
> >
> > Melvyn Sopacua
--
Melvyn Sopacua
Tim Graham
Melvyn Sopacua
On Monday 30 January 2017 08:47:10 Tim Graham wrote:
> I meant: where does Django call that method that's causing a problem
> for your use case?
It doesn't. But both listdir() and now that I'm looking, also path() are FileSystemStorage (or better: Hierarchial storage) artifects, that don't belong in Storage. Either that, or mkdir, basename, join and dirname should be implemented in Storage in the same manner. Those four are missing for my use case:
def generate_thumbs(self, instance, force=False, *args, **kwargs) -> None:
"""
Generate thumbnails for each (widht, height) tuple in self.sizes.
Called from signal.post_save.
:param instance: The model instance being saved.
:type instance: ModelKernel
:param force: force saving
:type force: bool
"""
file_field = getattr(instance, self.name) # type: ImageFieldFile
file_field.open()
original = Image.open(file_field.file) # type: Image.Image
filename = os.path.basename(file_field.path)
base_path = os.path.dirname(file_field.path)
base_url = os.path.dirname(file_field.url)
changed = 0
thumb_store = self._get_thumbnail_store(
instance.get_model_info('model_name'), instance.pk
)
for dimensions in self.sizes:
combined = self.dimension_str(dimensions)
thumb_path = os.path.join(base_path, combined, filename)
if not file_field.storage.exists(thumb_path) or force:
self.make_container(thumb_path)
thumb = original.copy()
thumb.thumbnail(dimensions)
thumb.save(thumb_path)
thumb_url = os.path.join(base_url, combined, filename)
thumb_store.thumbs[combined] = {
'path': thumb_path,
'url': thumb_url
}
changed += 1
if changed:
thumb_store.save()
(ModelKernel is simply models.Model with a get_model_info() method that provides read-only access to some meta class properties and some other goodies tacked on for the project in question).
make_container:
def make_container(self, filename):
container = os.path.dirname(filename)
if not os.path.exists(container):
if not os.path.exists(os.path.dirname(container)):
self.make_container(container)
mkdir(container, settings.FILE_UPLOAD_DIRECTORY_PERMISSIONS)
thumb_store is:
class ThumbnailStore(models.Model):
image_model = models.CharField(max_length=200)
image_model_pk = models.CharField(max_length=63)
image_field_name = models.CharField(max_length=63)
thumbs = HStoreField()
class Meta:
unique_together = ('image_model', 'image_model_pk', 'image_field_name')
So my objective below is to either not have anything in Storage that deals with a hierarchy or support a hierarchy completely.