PR#1197: [lib] ensuredir: normalize directory and don't throw error when dir exists - koji

koji

#1197 [lib] ensuredir: normalize directory and don't throw error when dir exists

Merged 4 years ago by tkopecek. Opened 5 years ago by julian8628.

julian8628/koji ensuredir into master

update docstr and do not swallow other errno

Yuming Zhu • 5 years ago

7b7fbd4

[lib] ensuredir: normalize directory and don't throw error when dir exists

Yu Ming Zhu • 5 years ago

f893bf6

koji/__init__.py

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`

tests/test_lib/test_file_ops.py

file added

+57

		`@@ -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')])`

julian8628 commented 5 years ago

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()'

ngompa commented 5 years ago

:thumbsup:

mikem commented 5 years ago

-        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?

mikem commented 5 years ago

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.

julian8628 commented 5 years ago

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

5 years ago

julian8628 commented 5 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