Fixed #6450 -- Improved the checking of errors when creating the directories for saved files. Thanks to henry@precheur.org for the report and patch, and vung for the excellent test case.

git-svn-id: http://code.djangoproject.com/svn/django/trunk@8007 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/db/models/base.py

@@ -472,11 +472,12 @@ class Model(object):
         return os.path.getsize(self._get_FIELD_filename(field))
 
     def _save_FIELD_file(self, field, filename, raw_field, save=True):
-        directory = field.get_directory_name()
+        # Create the upload directory if it doesn't already exist
-        try: # Create the date-based directory if it doesn't exist.
+        directory = os.path.join(settings.MEDIA_ROOT, field.get_directory_name())
-            os.makedirs(os.path.join(settings.MEDIA_ROOT, directory))
+        if not os.path.exists(directory):
-        except OSError: # Directory probably already exists.
+            os.makedirs(directory)
-            pass
+        elif not os.path.isdir(directory):
+            raise IOError('%s exists and is not a directory' % directory)        
 
         # Check for old-style usage (files-as-dictionaries). Warn here first
         # since there are multiple locations where we need to support both new

tests/regressiontests/file_uploads/models.py

@@ -1,2 +1,9 @@
-# This file unintentionally left blank.
+import tempfile
-# Oops.
+import os
+from django.db import models
+
+UPLOAD_ROOT = tempfile.mkdtemp()
+UPLOAD_TO = os.path.join(UPLOAD_ROOT, 'test_upload')
+
+class FileModel(models.Model):
+    testfile = models.FileField(upload_to=UPLOAD_TO)

tests/regressiontests/file_uploads/tests.py

@@ -1,9 +1,16 @@
 import os
+import errno
 import sha
+import shutil
 import tempfile
+import unittest
+
+from django.core.files.uploadedfile import SimpleUploadedFile
 from django.test import TestCase, client
 from django.utils import simplejson
 
+from models import FileModel, UPLOAD_ROOT, UPLOAD_TO
+
 class FileUploadTests(TestCase):
     def test_simple_upload(self):
         post_data = {
@@ -179,3 +186,42 @@ class FileUploadTests(TestCase):
 
         self.assertEqual(got.get('file1'), 1)
         self.assertEqual(got.get('file2'), 2)
+
+class DirectoryCreationTests(unittest.TestCase):
+    """
+    Tests for error handling during directory creation 
+    via _save_FIELD_file (ticket #6450)
+    """
+    def setUp(self):
+        self.obj = FileModel()
+        if not os.path.isdir(UPLOAD_ROOT):
+            os.makedirs(UPLOAD_ROOT)
+
+    def tearDown(self):
+        os.chmod(UPLOAD_ROOT, 0700)
+        shutil.rmtree(UPLOAD_ROOT)
+
+    def test_readonly_root(self):
+        """Permission errors are not swallowed"""
+        os.chmod(UPLOAD_ROOT, 0500)
+        try:
+            self.obj.save_testfile_file('foo.txt', SimpleUploadedFile('foo.txt', 'x'))
+        except OSError, err:
+            self.assertEquals(err.errno, errno.EACCES)
+        except:
+            self.fail("OSError [Errno %s] not raised" % errno.EACCES)
+
+    def test_not_a_directory(self):
+        """The correct IOError is raised when the upload directory name exists but isn't a directory"""
+        # Create a file with the upload directory name
+        fd = open(UPLOAD_TO, 'w')
+        fd.close()
+        try:
+            self.obj.save_testfile_file('foo.txt', SimpleUploadedFile('foo.txt', 'x'))
+        except IOError, err:
+            # The test needs to be done on a specific string as IOError
+            # is raised even without the patch (just not early enough)
+            self.assertEquals(err.args[0], 
+                              "%s exists and is not a directory" % UPLOAD_TO)
+        except:
+            self.fail("IOError not raised")