Opened 12 years ago
Closed 12 years ago
#20486 closed Bug (fixed)
file_move_safe doesn't respect it's docstring
| Reported by: | Ioan Alexandru Cucu | Owned by: | Kamu |
|---|---|---|---|
| Component: | File uploads/storage | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Kamu | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | yes | UI/UX: | no |
Description
django.core.files.move.file_move_safe's docstring states the following:
If the destination file exists and
allow_overwriteisFalse, this
function will throw anIOError.
However, if the destination file exists and movement is performed using os.rename (which is the case most of the times) then no exception is being thrown. The exception is only being thrown if streaming manually from one file to another.
This could be fixed by doing one of the following:
- update the docstring to match the actual behavior
- make the function raise IOError regardless of how the movement is being performed
Change History (7)
comment:1 by , 12 years ago
| Version: | 1.4 → master |
|---|
comment:3 by , 12 years ago
| Easy pickings: | set |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
Looking over the code and http://docs.python.org/2/library/os.html#os.rename it looks like this is quite true.
An interesting note is os.rename will not replace the file on Windows: "On Windows, if dst already exists, OSError will be raised even if it is a file; there may be no way to implement an atomic rename when dst names an existing file." So this seems to be unix systems only.
comment:4 by , 12 years ago
| Has patch: | set |
|---|---|
| Owner: | changed from to |
| Status: | new → assigned |
| Triage Stage: | Accepted → Ready for checkin |
I had a stab at fixing it. Please see:
comment:5 by , 12 years ago
| Triage Stage: | Ready for checkin → Accepted |
|---|
comment:6 by , 12 years ago
| Cc: | added |
|---|
comment:7 by , 12 years ago
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
Note. This might be platform dependent.
I reproduced this on Linux 3.2.0-23-generic-pae #36-Ubuntu SMP