Opened 8 years ago
Closed 6 years ago
#28144 closed New feature (fixed)
Add allow_overwrite kwarg to FileSystemStorage._save
Reported by: | Jon Prindiville | Owned by: | Jon Prindiville |
---|---|---|---|
Component: | File uploads/storage | Version: | 1.11 |
Severity: | Normal | Keywords: | file storage overwrite |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
I want to be able to construct a filesystem storage backend that will let me overwrite files while still leveraging FileSystemStorage
.
This set of changes (https://github.com/django/django/compare/master...jonprindiville:ticket_28144) would enable that without changing any of the default behaviour of FileSystemStorage
. In addition to the new kwarg, this adds a test for the existing no-overwriting behaviour, and a test for the newly enabled overwriting behaviour.
I'm not sure about what I should enter here for "Version" (could go in anywhere, really) and "Has patch" (I don't have a patch, but I do have a PR-able branch, linked above. I am assuming that I should wait for review here before actually submitting a PR, though)
HOW?
Just add an allow_overwrite
kwarg to FileSystemStorage._save
that a subclasser can optionally take advantage of.
WHY?
You can't do this in a good way today.
A piece of advice I've seen in several places (see Refs) for creating an overwriting storage backend is to create a subclass of FileSystemStorage
where the get_available_name
deletes an existing file of the same name. This, in my opinion, is a sneaky use of this function, based on the name. I guess it works, but that function is not called get_available_name_and_maybe_delete_some_stuff_also
A less sneaky (but more race-conditiony) implementation I have seen is to have _save
delete the existing file before calling FileSystemStorage._save
.
One of these is deceptive and one loops forever if you're unlucky.
Refs:
Various incarnations of this "ovewriting" implementation:
- django-storages, marked as deprecated (https://github.com/jschneier/django-storages/blob/1.5.2/storages/backends/overwrite.py)
- djangosnippets (https://djangosnippets.org/snippets/976/)
- someone's gist (https://gist.github.com/fabiomontefuscolo/1584462)
- django-overwrite-storage (https://github.com/ckot/django-overwrite-storage/blob/master/overwrite_storage/storage.py)
Change History (6)
comment:1 by , 8 years ago
Description: | modified (diff) |
---|
comment:2 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 8 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
I'm not too sure about adding to the release notes or AUTHORS file, so I'm leaving that alone for the time being.
follow-up: 5 comment:4 by , 7 years ago
Patch needs improvement: | set |
---|
comment:5 by , 7 years ago
Replying to Tim Martin:
I'm open to picking up this issue and working on this patch to get it accepted.
You set the patch to 'Needs improvement'.
Can you elaborate on what kind of improvements are needed?
It looks reasonable.