Skip to content

Commit

Permalink
More aggressively validate ambiguous file paths
Browse files Browse the repository at this point in the history
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
  • Loading branch information
jtratner committed Jan 24, 2018
1 parent ecd9bad commit 4a498bf
Show file tree
Hide file tree
Showing 13 changed files with 287 additions and 168 deletions.
22 changes: 20 additions & 2 deletions docs/release_notes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
------

Expand Down
13 changes: 9 additions & 4 deletions stor/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
3 changes: 3 additions & 0 deletions stor/default.cfg
Original file line number Diff line number Diff line change
@@ -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
Expand Down
8 changes: 8 additions & 0 deletions stor/obs.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import locale
import posixpath
import sys
import warnings

from cached_property import cached_property
import six
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down
8 changes: 3 additions & 5 deletions stor/swift.py
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
4 changes: 4 additions & 0 deletions stor/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"""
16 changes: 9 additions & 7 deletions stor/tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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': '',
Expand Down Expand Up @@ -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('.'))


Expand All @@ -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):
Expand Down
21 changes: 1 addition & 20 deletions stor/tests/test_posix.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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)

Expand Down
Loading

0 comments on commit 4a498bf

Please sign in to comment.