Opened 2 years ago

Closed 2 years ago

#20486 closed Bug (fixed)

file_move_safe doesn't respect it's docstring

Reported by: kux Owned by: Kamu
Component: File uploads/storage Version: master
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_overwrite is False, this
function will throw an IOError.

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:

  1. update the docstring to match the actual behavior
  2. make the function raise IOError regardless of how the movement is being performed

Change History (7)

comment:1 Changed 2 years ago by kux

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.4 to master

comment:2 Changed 2 years ago by kux

This might be platform dependent.
I reproduced this on Linux 3.2.0-23-generic-pae #36-Ubuntu SMP

Last edited 2 years ago by kux (previous) (diff)

comment:3 Changed 2 years ago by Kamu

  • Easy pickings set
  • Triage Stage changed from Unreviewed to 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 Changed 2 years ago by Kamu

  • Has patch set
  • Owner changed from nobody to Kamu
  • Status changed from new to assigned
  • Triage Stage changed from Accepted to Ready for checkin

I had a stab at fixing it. Please see:

https://github.com/django/django/pull/1253

comment:5 Changed 2 years ago by Kamu

  • Triage Stage changed from Ready for checkin to Accepted

comment:6 Changed 2 years ago by Kamu

  • Cc Kamu added

comment:7 Changed 2 years ago by Russell Keith-Magee <russell@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 18e79f1425fa87f2f38df25c65203ec4d311f499:

Fixed #20486 -- Ensure that file_move_safe raises an error if the destination already exists.

Thanks to kux for the report, and Russ Webber for the patch.

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