It seems like you have a pretty good grasp on the problems and possible solutions. I'm not familiar enough with storage backends and FileFields to provide much guidance so hopefully someone else can comment. Regarding the linked line in the save method which forces forward slashes, it seems that the FileSystemStorage should be doing that, rather than the base Storage class. It looks like Storage tries to do too much, so pushing some behaviour back down to FileSystemStorage seems correct. Again though, we have to be wary of backwards compatibility. You could offer opt-in to more correct behaviour while deprecating the status quo. Something like a `normalize_slashes = True` that can be flipped to False for brand new code.
On Thursday, 17 March 2016 14:30:43 UTC+11, Cristiano Coelho wrote: > > I agree with you, generate_filename should just call the field upload_to > and then delegate the whole name generation to the storage. > > There's another thing about file storage that is troubling me: > https://github.com/django/django/blob/master/django/core/files/storage.py#L57 > > The docs state you are not suposed to override the save method and just > implement _save, however, doing a complete random rename at the send pretty > much forces you to always override save instead. That name replacement is > probably to save the file name in a way you can easily create the URL, but > again, that should be delegated to the storage.url method shouldn't it? > > There's one more thing I noticed (gonna open a ticket about this one) is > that the proxy class for FileField (FieldFile, what a name!) states in the > docs that in order to call its save method you need a django File object > rather than a simple file-like object, I can understand that's because the > save method uses the .size property ( > https://github.com/django/django/blob/master/django/db/models/fields/files.py#L92) > > to save the file size into a _size variable. It doesn't seem anywhere in > the code the _size is being used as size is a property that gets the file > size either from the storage class or the actual file. So removing that > line, could also allow us to use normal files in the save method. > > El miércoles, 16 de marzo de 2016, 23:41:58 (UTC-3), Josh Smeaton escribió: >> >> It seems like FileField should delegate some of these methods to an >> underlying Storage backend, no? I don't know what the implications to >> back-compat would be, but the idea seems like a sensible one to start with. >> The storage backend API may need to grow some additional methods to >> verify/validate paths and filenames or it might already have the correct >> methods needed for FileField to work. Fields should do all of their >> path/storage IO via their storage object though. >> >> >> On Thursday, 17 March 2016 12:16:00 UTC+11, Cristiano Coelho wrote: >>> >>> To add a bit more about this, it seems that FileField is really meant to >>> be working with an OS file system, making it harder to use a custom Storage >>> that sends data to somewhere like AWS S3 where basically everything is a >>> file (there are no real folders, just key prefixes) >>> >>> These 3 functions inside FileField are the culprits: >>> >>> def get_directory_name(self): >>> return >>> os.path.normpath(force_text(datetime.datetime.now().strftime(force_str(self.upload_to)))) >>> >>> def get_filename(self, filename): >>> return >>> os.path.normpath(self.storage.get_valid_name(os.path.basename(filename))) >>> >>> def generate_filename(self, instance, 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)) >>> >>> >>> >>> They basically destroy any file name you give to it even with upload_to. >>> This is not an issue on a storage that uses the underlying file system, but >>> it might be quite an issue on different systems, in particular if file >>> names are using slashes as prefixes. >>> >>> So what I did was to override it a bit: >>> >>> class S3FileField(FileField): >>> >>> def generate_filename(self, instance, 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): >>> filename = self.upload_to(instance, filename) >>> filename = self.storage.get_valid_name(filename) >>> return filename >>> >>> return self.storage.get_valid_name(filename) >>> >>> >>> And all S3 issues gone! I wonder if this is the best way to do it. It >>> would be great to have an additional keyword argument or something on the >>> File (and image) fields to let the above functions know that they should >>> not perform any OS operation on paths but seems like it would cause a lot >>> of trouble. >>> >> -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/95ac5a1a-5443-4adf-9fb0-0570f9d8cdb6%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
