Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#26382 closed Cleanup/optimization (duplicate)

Better decouple Storage and FileField

Reported by: Cristiano Coelho Owned by: nobody
Component: File uploads/storage Version: 1.9
Severity: Normal Keywords: storage filefield
Cc: Cristiano Coelho Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Hello, following this: https://groups.google.com/forum/#!topic/django-developers/CjoPZq8lZms

It seems like the Storage and FileField classes are doing too much work that should be actually be delegated to the storage implementation, making it currently a bit more complicated to implement custom storages since the base Storage and FileField classes perform some os dependent work and based on assumptions like files should be always be saved with forward slashes or os dependant semantics like folders and paths.

In particular, the methods get_directory_name, get_filename and generate_file_name from FileField (https://github.com/django/django/blob/master/django/db/models/fields/files.py#L297) and the method save from Storage (https://github.com/django/django/blob/master/django/core/files/storage.py#L57) should be changed a bit so they delegate their work to the actual storage system.

Finally for Storage.save, the below line should be moved to the ._save method of FileSystemStorage

# Store filenames with forward slashes, even on Windows
return force_text(name.replace('\\', '/'))

I would move get_directory_name, get_filename and generate_file_name to the Storage class, however those methods also rely on the actual instance and upload_to value or callable and having those parameters sent to the storage class might not be a good idea. I'm not sure what would be the best way to remove any 'os.path' usage of FileField into the actual storage class without breaking everything.

Change History (9)

comment:1 by Cristiano Coelho, 8 years ago

I have commited to my repository a first attempt to improve this which seems to be working very well. No tests failed, no tests required changes and only one possible backwards incompatible change which might affect only a very limited amount of users, basically the ones that are using a custom Storage class, running on windows and relying on the default .save method of Storage. All info at the commit notes.

https://github.com/cristianocca/django/commit/4be936ba3031f95d34101a919648f4e8f7d7856a

comment:2 by Cristiano Coelho, 8 years ago

Has patch: set

comment:3 by Tim Graham, 8 years ago

Patch needs improvement: set
Summary: Improve Storage and FileFieldBetter decouple Storage and FileField
Triage Stage: UnreviewedAccepted

Please send a pull request so you can use the automated checks from our CI setup. Any backwards-incompatible changes should be added to the release notes.

comment:4 by Cristiano Coelho, 8 years ago

Looks like I missed to add changes in the release notes as well, is it something I should do or the actual person that accepts the change?

comment:5 by Tim Graham, 8 years ago

Please draft something. See also our PatchReviewChecklist.

comment:6 by Cristiano Coelho, 8 years ago

I have looked into the docs usages of django.db.models.fields.files.FileField.get_filename and django.db.models.fields.files.FileField.get_directory_name and found none, seems like they were never supposed to be used, should I still add a deprecation documentation?

All this documentation is taking me more than expected, I will try to have something in a while.

comment:7 by Cristiano Coelho, 8 years ago

Well if everything was right, here's the pull request https://github.com/django/django/pull/6321
I have removed the related work to #26367 and once this is done I will get back to that again. Waiting on feedback.

comment:8 by Tim Graham, 8 years ago

Resolution: duplicate
Status: newclosed

Actually this looks like a duplicate of #26058

in reply to:  8 comment:9 by Korijn van Golen, 8 years ago

Replying to timgraham:

Actually this looks like a duplicate of #26058

Well, I'm glad someone finally noticed... ;) looks like my efforts were in vain now though.

Note: See TracTickets for help on using tickets.
Back to Top