#1197 [lib] ensuredir: normalize directory and don't throw error when dir exists
Merged 2 years ago by tkopecek. Opened 3 years ago by julian8628.
julian8628/koji ensuredir  into  master

file modified
+14 -4
@@ -482,8 +482,18 @@ 

  

  ## BEGIN kojikamid dup

  

+ 

  def ensuredir(directory):

-     """Create directory, if necessary."""

+     """Create directory, if necessary.

+ 

+     :param str directory: path of the directory

+ 

+     :returns: str: normalized directory path

+ 

+     :raises OSError: If argument already exists and is not a directory, or

+                      error occurs from underlying `os.mkdir`.

+     """

+     directory = os.path.normpath(directory)

      if os.path.exists(directory):

          if not os.path.isdir(directory):

              raise OSError("Not a directory: %s" % directory)
@@ -498,10 +508,10 @@ 

          # note: if head is blank, then we've reached the top of a relative path

          try:

              os.mkdir(directory)

-         except OSError:

-             #thrown when dir already exists (could happen in a race)

+         except OSError as e:

+             # do not thrown when dir already exists (could happen in a race)

              if not os.path.isdir(directory):

-                 #something else must have gone wrong

+                 # something else must have gone wrong

                  raise

      return directory

  

@@ -0,0 +1,57 @@ 

+ from __future__ import absolute_import

+ 

+ try:

+     import unittest2 as unittest

+ except ImportError:

+     import unittest

+ 

+ import mock

+ import errno

+ 

+ from koji import ensuredir

+ 

+ 

+ class TestEnsureDir(unittest.TestCase):

+     @mock.patch('os.mkdir')

+     @mock.patch('os.path.exists')

+     @mock.patch('os.path.isdir')

+     def test_ensuredir_errors(self, mock_isdir, mock_exists, mock_mkdir):

+         mock_exists.return_value = False

+         with self.assertRaises(OSError) as cm:

+             ensuredir('/')

+         self.assertEqual(cm.exception.args[0], 'root directory missing? /')

+         mock_mkdir.assert_not_called()

+ 

+         mock_exists.return_value = True

+         mock_isdir.return_value = False

+         with self.assertRaises(OSError) as cm:

+             ensuredir('/path/foo/bar')

+         self.assertEqual(cm.exception.args[0],

+                          'Not a directory: /path/foo/bar')

+         mock_mkdir.assert_not_called()

+ 

+         mock_exists.return_value = False

+         mock_isdir.return_value = False

+         mock_mkdir.side_effect = OSError(errno.EEXIST, 'error msg')

+         with self.assertRaises(OSError) as cm:

+             ensuredir('path')

+         self.assertEqual(cm.exception.args[0], errno.EEXIST)

+         mock_mkdir.assert_called_once_with('path')

+ 

+         mock_mkdir.reset_mock()

+         mock_mkdir.side_effect = OSError(errno.EEXIST, 'error msg')

+         mock_isdir.return_value = True

+         ensuredir('path')

+         mock_mkdir.assert_called_once_with('path')

+ 

+     @mock.patch('os.mkdir')

+     @mock.patch('os.path.exists')

+     @mock.patch('os.path.isdir')

+     def test_ensuredir(self, mock_isdir, mock_exists, mock_mkdir):

+         mock_exists.side_effect = [False, False, True]

+         mock_isdir.return_value = True

+         ensuredir('/path/foo/bar/')

+         self.assertEqual(mock_exists.call_count, 3)

+         self.assertEqual(mock_isdir.call_count, 1)

+         mock_mkdir.assert_has_calls([mock.call('/path/foo'),

+                                      mock.call('/path/foo/bar')])

fixes: #1196

  • call 'os.path.normpath(directory)' to make sure the path doesn't contain ".", ".." and is not end with "/"
  • don't raise OSError if directory exists when executing 'os.mkdir()'
-        except OSError:
-            #thrown when dir already exists (could happen in a race)
-            if not os.path.isdir(directory):
+        except OSError as e:
+            # do not thrown when dir already exists (could happen in a race)
+            if e.errno == errno.EEXIST and not os.path.isdir(directory):

I feel like we're masking a problem here. In what reasonable case can mkdir throw EEXIST and os.path.isdir return false at the same time?

I have no objection to the normpath.

The raises part in the docstring doesn't quite seem right. First case should be "argument already exists and is not a directory", and the second case isn't exactly our exception, it comes from mkdir. If we're going to document exceptions that the underlying mkdir might raise, then there are more possibilities that stated.

I feel like we're masking a problem here. In what reasonable case can mkdir throw EEXIST and os.path.isdir return false at the same time?

It's still possible that other program creates this file which is not a dir, after the first os.path.exists(directory) is called.
hmm.. I masked all other errnos. will roll it back.

rebased onto f893bf6

3 years ago

The raises part in the docstring doesn't quite seem right. First case should be "argument already exists and is not a directory", and the second case isn't exactly our exception, it comes from mkdir. If we're going to document exceptions that the underlying mkdir might raise, then there are more possibilities that stated.

Updated with the previous problem

Commit bff03c2 fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago