More aggressively validate ambiguous file paths

Closes #52

* Error on filepaths with trailing slash because they do not have a name
  and thus appear to be directories.
* Add validation for bad paths to ``OBSFile``.
* Warn if filesystem paths would not pass this validation (otherwise you
  have the possibility of something that works locally and not on OBS).
  This can be set to raise instead if you set
  ``stor.raise_on_ambiguous_filesystem_path`` to be True
* Deprecate ``is_ambiguous()`` which didn't 100% serve the purpose we
  wanted anyways (since you can always tell when user means directory
  because of the user of ``copytree()`` and/or ``-r``)

Sem-Ver: feature

docs/release_notes.rst

@@ -1,6 +1,31 @@
 Release Notes
 =============
 
+v1.6.0
+------
+
+API Additions
+^^^^^^^^^^^^^
+
+* Add ``stor.test.TestCase`` that allows mocking out of both swift AND s3
+  functionality in a test case.
+
+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.4
 ------
 

stor/base.py

@@ -267,6 +267,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
@@ -370,14 +372,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.
+raise_on_ambiguous_filesystem_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):
@@ -282,11 +284,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

@@ -40,7 +40,9 @@ def test_cli_error(self, mock_list, mock_stderr):
     @mock.patch('stor.settings.USER_CONFIG_FILE', '')
     def test_cli_config(self, mock_copytree):
         expected_settings = {
-            'stor': {},
+            'stor': {
+                'raise_on_ambiguous_filesystem_path': False
+            },
             's3': {
                 'aws_access_key_id': '',
                 'aws_secret_access_key': '',
@@ -444,31 +446,31 @@ 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('.'))
 
 
 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):
@@ -507,14 +509,14 @@ def test_cd_swift(self, mock_isdir):
     @mock.patch('sys.stderr', new=six.StringIO())
     def test_cd_not_dir_s3(self, mock_isdir):
         with self.assertRaisesRegexp(SystemExit, '1'):
-            self.parse_args('stor cd s3://test/file')
+            self.parse_args('stor cd s3://test/file.ext')
         self.assertIn('not a directory', sys.stderr.getvalue())
 
     @mock.patch.object(SwiftPath, 'isdir', return_value=False, autospec=True)
     @mock.patch('sys.stderr', new=six.StringIO())
     def test_cd_not_dir_swift(self, mock_isdir):
         with self.assertRaisesRegexp(SystemExit, '1'):
-            self.parse_args('stor cd swift://test/container/file')
+            self.parse_args('stor cd swift://test/container/file.ext')
         self.assertIn('not a directory', sys.stderr.getvalue())
 
     @mock.patch('sys.stderr', new=six.StringIO())

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)