Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixed #18233: Raise IOError if destination file already exists, in django.core.files.move.file_move_safe. #18

Closed
wants to merge 2 commits into from

4 participants

Aviral Dasgupta Gabriel Falcão Anssi Kääriäinen Jannis Leidel
Aviral Dasgupta

On certain platforms like Linux, the existing code will simply ignore allow_overwrite and go ahead and overwrite the destination file anyway. This fixes it.

ps. This is also my first contribution to Django :)

Gabriel Falcão

Great work with the individual commit with tests !
:+1:

Jannis Leidel jezdez commented on the diff
django/core/files/move.py
@@ -51,6 +51,8 @@ def file_move_safe(old_file_name, new_file_name, chunk_size = 1024*64, allow_ove
return
try:
+ if not allow_overwrite and os.path.exists(new_file_name):
+ raise IOError('Destination file already exists: \'{0}\''.format(new_file_name))
Jannis Leidel Owner
jezdez added a note

It's best practice to use common string substitution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Anssi Kääriäinen
Collaborator

I am closing this pull request as the ticket discussions indicate this isn't 100% ready for commit.

Anssi Kääriäinen akaariai closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 1, 2012
  1. Aviral Dasgupta

    Fixed #18233: Raise IOError if destination file already exists, in

    aviraldg authored
    django.core.files.move.file_move_safe.
  2. Aviral Dasgupta
This page is out of date. Refresh to see the latest.
1  AUTHORS
View
@@ -563,6 +563,7 @@ answer newbie questions, and generally made Django that much better:
Gasper Zejn <zejn@kiberpipa.org>
Jarek Zgoda <jarek.zgoda@gmail.com>
Cheng Zhang
+ Aviral Dasgupta <aviraldg@gmail.com>
A big THANK YOU goes to:
2  django/core/files/move.py
View
@@ -51,6 +51,8 @@ def file_move_safe(old_file_name, new_file_name, chunk_size = 1024*64, allow_ove
return
try:
+ if not allow_overwrite and os.path.exists(new_file_name):
+ raise IOError('Destination file already exists: \'{0}\''.format(new_file_name))
Jannis Leidel Owner
jezdez added a note

It's best practice to use common string substitution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
os.rename(old_file_name, new_file_name)
return
except OSError:
22 tests/regressiontests/file_storage/tests.py
View
@@ -23,6 +23,7 @@
from django.core.files.images import get_image_dimensions
from django.core.files.storage import FileSystemStorage, get_storage_class
from django.core.files.uploadedfile import UploadedFile
+from django.core.files.move import file_move_safe
from django.test import SimpleTestCase
from django.utils import unittest
from ..servers.tests import LiveServerBase
@@ -586,3 +587,24 @@ def test_urllib2_urlopen(self):
remote_file = self.urlopen('/example_view/')
self.assertEqual(stored_file.read(), remote_file.read())
+
+class MoveTestCase(unittest.TestCase):
+ """
+ Test django.core.files.move.
+ """
+ def setUp(self):
+ self.file_a = tempfile.mkstemp()
+ self.file_b = tempfile.mkstemp()
+
+ def tearDown(self):
+ os.close(self.file_a[0])
+ os.close(self.file_b[0])
+ os.remove(self.file_a[1])
+ os.remove(self.file_b[1])
+
+ def test_overwrite_existing_file(self):
+ """
+ Test that file_move_safe raises an IOError if you try to overwrite an
+ existing file without passing in allow_overwrite=True.
+ """
+ self.assertRaises(IOError, file_move_safe, self.file_a[1], self.file_b[1])
Something went wrong with that request. Please try again.