More aggressively validate ambiguous file paths

docs/release_notes.rst

@@ -4,20 +4,38 @@ Release Notes
 v1.6.0
 ------
 
-API additions
+API Additions
 ^^^^^^^^^^^^^
 
 * Add `stor.extensions.swiftstack` module for translating swift paths to s3.
 * Add ``OBSPath.to_url()`` method to translate swift and s3 paths to HTTP paths.
 * Add ``stor.makedirs_p(path, mode=0o777)`` to cross-compatible API. This does
   nothing on OBS-paths (just there for convenience).
+* Add ``stor.test.TestCase`` that allows mocking out of both swift AND s3
+  functionality in a test case.
 
-CLI additions
+CLI Additions
 ^^^^^^^^^^^^^
 
 * ``stor url <path>`` to translate swift and s3 paths to HTTP paths.
 * ``stor convert-swiftstack [--bucket] <path>`` cli tool to convert s3 <-> swiftstack paths.
 
+Bug Fixes
+^^^^^^^^^
+
+* ``open()`` will raise with an OBS path that ends in trailing slash, rather
+  than generating an invalid/difficult OBS object.
+
+Deprecations
+^^^^^^^^^^^^
+
+* Add warning about calling ``open()`` or ``copy()`` with OBS or filesystem
+  paths that are ambiguous or end in a trailing slash. In the next version of
+  stor, this will be an error, to prevent differences between local and OBS
+  code. Set ``stor:raise_on_ambiguous_filesystem_path`` to get this behavior
+  now.
+* ``is_ambiguous()`` is now deprecated.
+
 v1.5.2
 ------
 

stor/base.py

@@ -268,6 +268,8 @@ def open(self, *args, **kwargs):
         Args:
             mode (str): first positional arg, mode of file descriptor
             encoding (str): text encoding to use (Python 3 only)
+        Raises:
+            ValueError: if ambiguous path - see ``stor.utils.validate_file_path`` for more.
         """
 
         raise NotImplementedError
@@ -371,14 +373,17 @@ class FileSystemPath(Path):
     """
     def open(self, *args, **kwargs):
         """
-        Opens a path and retains interface compatibility with
-        `SwiftPath` by popping the unused ``swift_upload_args`` keyword
-        argument.
+        Opens a path.
 
         Creates parent directory if it does not exist.
+
+        Raises:
+            ValueError: if ambiguous path - see ``stor.utils.validate_file_path`` for more.
         """
-        kwargs.pop('swift_upload_kwargs', None)
+        if kwargs.pop('swift_upload_kwargs', None):
+            warnings.warn('swift_upload_kwargs will be removed in stor 2.0')
         self.parent.makedirs_p()
+        utils.validate_file_path(self)
         return builtins.open(self, *args, **kwargs)
 
     def __enter__(self):

stor/default.cfg

@@ -1,5 +1,8 @@
 [stor]
 
+# set to raise if file paths are ambiguous - see stor.utils.validate_file_path for more.
+always_raise_on_ambiguous_path = False
+
 [s3]
 
 # See boto3 docs for more detail on these parameters - all passed directly to boto3.session.Session *if* set

stor/obs.py

@@ -1,6 +1,7 @@
 import locale
 import posixpath
 import sys
+import warnings
 
 from cached_property import cached_property
 import six
@@ -90,6 +91,7 @@ def is_ambiguous(self):
         """Returns true if it cannot be determined if the path is a
         file or directory
         """
+        warnings.warn('this function is deprecated and will be removed in stor 2.0')
         return not self.endswith('/') and not self.ext
 
     def _get_parts(self):
@@ -286,11 +288,17 @@ def __init__(self, pth, mode='r', encoding=None, **kwargs):
                 use binary mode OR explicitly set an encoding when reading/writing text (because
                 writers from different computers may store data on OBS in different ways).
                 Python 3 only.
+        Raises:
+            ValueError: if the mode is not valid or the path is ambiguous (see ``stor.copy``)
         """
         if mode not in self._VALID_MODES:
             raise ValueError('invalid mode for file: %r' % mode)
         if six.PY2 and encoding:  # pragma: no cover
             raise TypeError('encoding not supported in Python 2')
+        # TODO (jtratner): in stor 2.0 remove this explicit check and the warning
+        if pth.endswith('/'):
+            raise ValueError('file paths may not end with trailing slash')
+        utils.validate_file_path(pth, _warn=True)
         self._path = pth
         self.mode = mode
         self._kwargs = kwargs

stor/swift.py

@@ -688,18 +688,16 @@ def open(self, mode='r', encoding=None, swift_upload_options=None):
                 ("r" or "rb") and writing ("w", "wb")
             encoding (str): text encoding to use. Defaults to
                 ``locale.getpreferredencoding(False)`` (Python 3 only)
-            swift_upload_options (dict): DEPRECATED (use `stor.settings.use()`
-                instead). A dictionary of arguments that will be
-                passed as keyword args to `SwiftPath.upload` if any writes
-                occur on the opened resource.
-
+            swift_upload_options (dict): DEPRECATED (use `stor.settings.use()` instead).
         Returns:
             SwiftFile: The swift object.
 
         Raises:
             SwiftError: A swift client error occurred.
         """
         swift_upload_options = swift_upload_options or {}
+        if swift_upload_options:
+            warnings.warn('swift_upload_options will be removed in stor 2.0')
         return SwiftFile(self, mode=mode, encoding=encoding, **swift_upload_options)
 
     @_swift_retry(exceptions=(ConditionNotMetError, UnavailableError))

stor/test.py

@@ -207,3 +207,7 @@ def setUp(self):
             del s3._thread_local.s3_transfer_config
         except AttributeError:
             pass
+
+
+class StorTestCase(S3TestCase, SwiftTestCase):
+    """A TestCase class that mocks BOTH s3 and stor for tests"""

stor/tests/test_cli.py

@@ -87,7 +87,9 @@ def test_cli_error(self, mock_list):
     @mock.patch('stor.settings.USER_CONFIG_FILE', '')
     def test_cli_config(self, mock_copytree):
         expected_settings = {
-            'stor': {},
+            'stor': {
+                'always_raise_on_ambiguous_path': False
+            },
             's3': {
                 'aws_access_key_id': '',
                 'aws_secret_access_key': '',
@@ -478,14 +480,14 @@ def test_walkfiles_no_pattern(self, mock_walkfiles):
             './a/b.txt',
             './c.txt',
             './d.txt',
-            './file'
+            './file.ext'
         ]
         self.parse_args('stor walkfiles .')
         self.assertEquals(sys.stdout.getvalue(),
                           './a/b.txt\n'
                           './c.txt\n'
                           './d.txt\n'
-                          './file\n')
+                          './file.ext\n')
         mock_walkfiles.assert_called_once_with(PosixPath('.'))
 
 
@@ -503,16 +505,16 @@ class TestCat(BaseCliTest):
     @mock.patch.object(S3Path, 'read_object', autospec=True)
     def test_cat_s3(self, mock_read):
         mock_read.return_value = b'hello world\n'
-        self.parse_args('stor cat s3://test/file')
+        self.parse_args('stor cat s3://test/file.ext')
         self.assertEquals(sys.stdout.getvalue(), 'hello world\n')
-        mock_read.assert_called_once_with(S3Path('s3://test/file'))
+        mock_read.assert_called_once_with(S3Path('s3://test/file.ext'))
 
     @mock.patch.object(SwiftPath, 'read_object', autospec=True)
     def test_cat_swift(self, mock_read):
         mock_read.return_value = b'hello world'
-        self.parse_args('stor cat swift://some/test/file')
+        self.parse_args('stor cat swift://some/test/file.ext')
         self.assertEquals(sys.stdout.getvalue(), 'hello world\n')
-        mock_read.assert_called_once_with(SwiftPath('swift://some/test/file'))
+        mock_read.assert_called_once_with(SwiftPath('swift://some/test/file.ext'))
 
 
 class TestCd(BaseCliTest):

stor/tests/test_posix.py

@@ -92,25 +92,6 @@ def test_posix_file_destination(self):
             self.assertTrue(dest.exists())
             self.assertEquals(dest.open().read(), '1')
 
-    def test_ambigious_swift_resource_destination(self):
-        with stor.NamedTemporaryDirectory() as tmp_d:
-            source = tmp_d / '1'
-            with open(source, 'w') as tmp_file:
-                tmp_file.write('1')
-
-            dest = 'swift://tenant/container/ambiguous-resource'
-            with self.assertRaisesRegexp(ValueError, 'OBS destination'):
-                utils.copy(source, dest)
-
-    def test_ambigious_swift_container_destination(self):
-        with stor.NamedTemporaryDirectory() as tmp_d:
-            source = tmp_d / '1'
-            with open(source, 'w') as tmp_file:
-                tmp_file.write('1')
-
-            dest = 'swift://tenant/ambiguous-container'
-            with self.assertRaisesRegexp(ValueError, 'OBS destination'):
-                utils.copy(source, dest)
 
     def test_tenant_swift_destination(self):
         with stor.NamedTemporaryDirectory() as tmp_d:
@@ -119,7 +100,7 @@ def test_tenant_swift_destination(self):
             with open(source / '1', 'w') as tmp_file:
                 tmp_file.write('1')
 
-            dest = 'swift://tenant/'
+            dest = 'swift://tenant.with.dots'
             with self.assertRaisesRegexp(ValueError, 'copy to tenant'):
                 utils.copy(source / '1', dest)