From 5502d45d4d90e3dc3dec303af3aa4c2e5fc50163 Mon Sep 17 00:00:00 2001 From: Anuj Kumar Date: Sat, 1 Dec 2018 15:01:58 -0800 Subject: [PATCH 1/7] preparing the packages to (finally) be separated --- docs/testing.rst | 15 ++++++++++++-- stor/__init__.py | 2 -- stor/base.py | 11 +++++----- stor/cli.py | 36 ++++++++++++++++++++------------ stor/test.py | 0 stor/utils.py | 50 ++++----------------------------------------- stor_dx/dx.py | 2 +- stor_swift/swift.py | 5 +++-- 8 files changed, 50 insertions(+), 71 deletions(-) delete mode 100644 stor/test.py diff --git a/docs/testing.rst b/docs/testing.rst index 5abf9c91..be1f4797 100644 --- a/docs/testing.rst +++ b/docs/testing.rst @@ -2,12 +2,23 @@ Testing ======= -Stor provides a test case to use when testing against Swift storage. It also +Stor provides a test case to use when testing against OBS storage. It also provides a mixin class that can be used when creating other base test classes -.. automodule:: stor.test .. autoclass:: stor_swift.test.SwiftTestMixin :members: .. autoclass:: stor_swift.test.SwiftTestCase :members: + +.. autoclass:: stor_s3.test.S3TestMixin + :members: + +.. autoclass:: stor_s3.test.S3TestCase + :members: + +.. autoclass:: stor_dx.test.DXTestMixin + :members: + +.. autoclass:: stor_dx.test.DXTestCase + :members: diff --git a/stor/__init__.py b/stor/__init__.py index 71d9edd6..6054840e 100644 --- a/stor/__init__.py +++ b/stor/__init__.py @@ -23,7 +23,6 @@ import pkg_resources from stor.utils import is_filesystem_path -from stor.utils import is_swift_path from stor.utils import is_obs_path from stor.utils import NamedTemporaryDirectory from stor.base import Path @@ -118,7 +117,6 @@ def glob(pth, pattern): # pragma: no cover 'rmtree', 'walkfiles', 'is_filesystem_path', - 'is_swift_path', 'is_obs_path', 'NamedTemporaryDirectory', ] diff --git a/stor/base.py b/stor/base.py index 19ca805d..b9e394d3 100644 --- a/stor/base.py +++ b/stor/base.py @@ -4,7 +4,6 @@ import os import ntpath import posixpath -import shlex import shutil import sys import warnings @@ -42,17 +41,19 @@ class Path(text_type): """ def __new__(cls, path): + from stor_swift import utils as swift_utils + from stor_s3 import utils as s3_utils + from stor_dx import utils as dx_utils if cls is Path: if not hasattr(path, 'startswith'): raise TypeError('must be a string like') - if utils.is_dx_path(path): - from stor_dx import utils as dx_utils + if dx_utils.is_dx_path(path): cls = dx_utils.find_dx_class(path) - elif utils.is_swift_path(path): + elif swift_utils.is_swift_path(path): from stor_swift.swift import SwiftPath cls = SwiftPath - elif utils.is_s3_path(path): + elif s3_utils.is_s3_path(path): from stor_s3.s3 import S3Path cls = S3Path diff --git a/stor/cli.py b/stor/cli.py index 3f6c9ff0..39e1b623 100644 --- a/stor/cli.py +++ b/stor/cli.py @@ -277,10 +277,13 @@ def get_path(pth, mode=None): def _wrapped_list(path, **kwargs): """Use iterative walkfiles for DX paths, rather than trying to generate full list first""" - if utils.is_dx_path(path): - func = stor.walkfiles - else: - func = stor.list + func = stor.list + try: + from stor_dx import utils as dx_utils + if dx_utils.is_dx_path(path): + func = stor.walkfiles + except ImportError: # pragma: no cover + pass return func(path, **kwargs) @@ -292,15 +295,22 @@ def _to_url(path): def _convert_swiftstack(path, bucket=None): path = stor.Path(path) - if utils.is_swift_path(path): - if not bucket: - # TODO (jtratner): print help here - raise ValueError('--bucket is required for swift paths') - return swiftstack.swift_to_s3(path, bucket=bucket) - elif utils.is_s3_path(path): - return swiftstack.s3_to_swift(path) - else: - raise ValueError("invalid path for conversion: '%s'" % path) + try: + from stor_swift import utils as swift_utils + if swift_utils.is_swift_path(path): + if not bucket: + # TODO (jtratner): print help here + raise ValueError('--bucket is required for swift paths') + return swiftstack.swift_to_s3(path, bucket=bucket) + except ImportError: # pragma: no cover + pass + try: + from stor_s3 import utils as s3_utils + if s3_utils.is_s3_path(path): + return swiftstack.s3_to_swift(path) + except ImportError: # pragma: no cover + pass + raise ValueError("invalid path for conversion: '%s'" % path) def create_parser(): diff --git a/stor/test.py b/stor/test.py deleted file mode 100644 index e69de29b..00000000 diff --git a/stor/utils.py b/stor/utils.py index c2e7b72c..5322253f 100644 --- a/stor/utils.py +++ b/stor/utils.py @@ -173,21 +173,6 @@ def validate_manifest_list(expected_objs, list_results): return set(expected_objs).issubset(listed_objs) -def is_swift_path(p): - """Determines if the path is a Swift path. - - All Swift paths start with swift:// - - Args: - p (str): The path string - - Returns: - bool: True if p is a Swift path, False otherwise. - """ - from stor_swift.swift import SwiftPath - return p.startswith(SwiftPath.drive) - - def is_filesystem_path(p): """Determines if the path is posix or windows filesystem. @@ -197,24 +182,12 @@ def is_filesystem_path(p): Returns: bool: True if p is a Windows path, False otherwise. """ + from stor_swift.utils import is_swift_path + from stor_s3.utils import is_s3_path + from stor_dx.utils import is_dx_path return not (is_swift_path(p) or is_s3_path(p) or is_dx_path(p)) -def is_s3_path(p): - """Determines if the path is a S3 path. - - All S3 paths start with ``s3://`` - - Args: - p (str): The path string - - Returns - bool: True if p is a S3 path, False otherwise. - """ - from stor_s3.s3 import S3Path - return p.startswith(S3Path.drive) - - def is_obs_path(p): """Determines if the path is an OBS path (an S3 or Swift path). @@ -224,22 +197,7 @@ def is_obs_path(p): Returns bool: True if p is an OBS path, False otherwise. """ - return is_s3_path(p) or is_swift_path(p) or is_dx_path(p) - - -def is_dx_path(p): - """Determines if the path is a DX path. - - All DX paths start with ``dx://`` - - Args: - p (str): The path string - - Returns - bool: True if p is a DX path, False otherwise. - """ - from stor_dx.dx import DXPath - return p.startswith(DXPath.drive) + return not is_filesystem_path(p) def is_writeable(path, swift_retry_options=None): diff --git a/stor_dx/dx.py b/stor_dx/dx.py index ab18cc3b..06b5a796 100644 --- a/stor_dx/dx.py +++ b/stor_dx/dx.py @@ -1302,4 +1302,4 @@ def _wait_on_close(self): # ignore timeout because we want client code to deal with it except dxpy.DXError as e: # pragma: no cover if 'timeout' not in str(e): - raise \ No newline at end of file + raise diff --git a/stor_swift/swift.py b/stor_swift/swift.py index afc129e0..0bcee451 100644 --- a/stor_swift/swift.py +++ b/stor_swift/swift.py @@ -50,7 +50,6 @@ from swiftclient.utils import generate_temp_url from stor import exceptions as stor_exceptions -from stor import is_swift_path from stor import settings from stor import utils from stor.base import Path @@ -58,6 +57,7 @@ from stor.obs import OBSUploadObject from stor.posix import PosixPath from stor.third_party.backoff import with_backoff +from stor_swift.utils import is_swift_path logger = logging.getLogger(__name__) @@ -1052,7 +1052,8 @@ def upload(self, to_upload, condition=None, use_manifest=False, - headers=None): + headers=None, + **kwargs): """Uploads a list of files and directories to swift. This method retries ``num_retries`` times if swift is unavailable or if From afb4d6a75b425496132692b04734c527746c82a8 Mon Sep 17 00:00:00 2001 From: Anuj Kumar Date: Mon, 3 Dec 2018 14:57:00 -0800 Subject: [PATCH 2/7] reverting changes to is_writeable but leaving the tests in resp modules --- stor/utils.py | 47 ++++++++++++++++- stor_s3/tests/test_utils_s3.py | 4 +- stor_s3/utils.py | 68 ------------------------ stor_swift/tests/test_utils_swift.py | 16 +++--- stor_swift/utils.py | 77 ---------------------------- 5 files changed, 56 insertions(+), 156 deletions(-) diff --git a/stor/utils.py b/stor/utils.py index 5322253f..2892ded6 100644 --- a/stor/utils.py +++ b/stor/utils.py @@ -231,10 +231,55 @@ def is_writeable(path, swift_retry_options=None): Returns: bool: Whether ``path`` is writeable or not. """ + from stor import basename + from stor import join from stor import Path + from stor import remove + from stor_swift.swift import ConflictError + from stor_swift.swift import SwiftPath + from stor_swift.swift import UnauthorizedError + from stor_swift.swift import UnavailableError + path = with_trailing_slash(Path(path)) - return os.access(path, os.W_OK) + if is_filesystem_path(path): + return os.access(path, os.W_OK) + + container_path = None + container_existed = None + if is_swift_path(path): + # We want this function to behave as a no-op with regards to the underlying + # container structure. Therefore we need to remove any containers created by this + # function that were not present when it was called. The `container_existed` + # defined below will store whether the container that we're checking existed when + # calling this function, so that we know if it should be removed at the end. + container_path = Path('{}{}/{}/'.format( + SwiftPath.drive, + path.tenant, + path.container + )) + container_existed = container_path.exists() + + with tempfile.NamedTemporaryFile() as tmpfile: + try: + # Attempt to create a file in the `path`. + stor.copy(tmpfile.name, path, swift_retry_options=swift_retry_options) + # Remove the file that was created. + remove(join(path, basename(tmpfile.name))) + answer = True + except (UnauthorizedError, UnavailableError, IOError, OSError, exceptions.FailedUploadError): # nopep8 + answer = False + + # Remove the Swift container if it didn't exist when calling this function, but exists + # now. This way this function remains a no-op with regards to container structure. + if container_existed is False and container_path.exists(): + try: + container_path.remove_container() + except ConflictError: + # Ignore if some other thread/user created the container in the meantime. + pass + + return answer def _safe_get_size(name): diff --git a/stor_s3/tests/test_utils_s3.py b/stor_s3/tests/test_utils_s3.py index d0417731..30877418 100644 --- a/stor_s3/tests/test_utils_s3.py +++ b/stor_s3/tests/test_utils_s3.py @@ -42,11 +42,11 @@ def setUp(self): def test_success(self): path = 's3://stor-test/foo/bar' - self.assertTrue(utils.is_writeable(path)) + self.assertTrue(stor.utils.is_writeable(path)) self.mock_remove.assert_called_with( S3Path('{}/{}'.format(path, self.filename))) def test_path_no_perms(self): self.mock_copy.side_effect = stor.exceptions.FailedUploadError('foo') - self.assertFalse(utils.is_writeable('s3://stor-test/foo/bar')) + self.assertFalse(stor.utils.is_writeable('s3://stor-test/foo/bar')) self.assertFalse(self.mock_remove.called) diff --git a/stor_s3/utils.py b/stor_s3/utils.py index 187aca32..88ea7598 100644 --- a/stor_s3/utils.py +++ b/stor_s3/utils.py @@ -16,71 +16,3 @@ def is_s3_path(p): """ from stor_s3.s3 import S3Path return p.startswith(S3Path.drive) - - -def is_writeable(path, swift_retry_options=None): - """ - Determine whether we have permission to write to path. - - Behavior of this method is slightly different for different storage types when the - directory doesn't exist: - 1. For local file systems, this function will return True if the target directory - exists and a file written to it. - 2. For AWS S3, this function will return True only if the target bucket is already - present and we have write access to the bucket. - 3. For Swift, this function will return True, only if the target tenant is already - present and we have write access to the tenant and container. The container doesn't - have to be present. - - This is function is useful, because `stor.stat()` will succeed if we have read-only - permissions to `path`, but the eventual attempt to upload will fail. - - Secondly, `path` might not exist yet. If the intent of the caller is to create it, , - stor.stat() will fail, however the eventual upload attempt would succeed. - - Args: - path (stor.Path|str): The path to check. - swift_retry_options (dict): Optional retry arguments to use for swift - upload or download. View the - `swift module-level documentation ` for more - information on retry arguments. If the goal is to not use - exponential backoff, pass ``{'num_retries': 0}`` here. - - Returns: - bool: Whether ``path`` is writeable or not. - """ - from stor import basename - from stor import join - from stor import Path - from stor import remove - from stor.exceptions import UnauthorizedError - from stor.exceptions import UnavailableError - from stor.exceptions import ConflictError - from stor.exceptions import FailedUploadError - import stor - - path = stor_utils.with_trailing_slash(Path(path)) - - container_path = None - container_existed = None - - with tempfile.NamedTemporaryFile() as tmpfile: - try: - # Attempt to create a file in the `path`. - stor.copy(tmpfile.name, path, swift_retry_options=swift_retry_options) - # Remove the file that was created. - remove(join(path, basename(tmpfile.name))) - answer = True - except (UnauthorizedError, UnavailableError, IOError, OSError, FailedUploadError): # nopep8 - answer = False - - # Remove the Swift container if it didn't exist when calling this function, but exists - # now. This way this function remains a no-op with regards to container structure. - if container_existed is False and container_path.exists(): # pragma: no cover - try: - container_path.remove_container() - except ConflictError: - # Ignore if some other thread/user created the container in the meantime. - pass - - return answer diff --git a/stor_swift/tests/test_utils_swift.py b/stor_swift/tests/test_utils_swift.py index b137ce53..9110c995 100644 --- a/stor_swift/tests/test_utils_swift.py +++ b/stor_swift/tests/test_utils_swift.py @@ -53,39 +53,39 @@ def setUp(self): def test_existing_path(self): self.mock_exists.return_value = True path = SwiftPath('swift://AUTH_stor_test/container/test/') - self.assertTrue(utils.is_writeable(path)) + self.assertTrue(stor_utils.is_writeable(path)) self.mock_remove.assert_called_with(path / self.filename) def test_non_existing_path(self): self.mock_exists.return_value = False - self.assertTrue(utils.is_writeable('swift://AUTH_stor_test/container/test/')) + self.assertTrue(stor_utils.is_writeable('swift://AUTH_stor_test/container/test/')) def test_path_unchanged(self): # Make the first call to exists() return False and the second return True. self.mock_exists.side_effect = [False, True] - utils.is_writeable('swift://AUTH_stor_test/container/test/') + stor_utils.is_writeable('swift://AUTH_stor_test/container/test/') self.mock_remove_container.assert_called_once_with( SwiftPath('swift://AUTH_stor_test/container/')) def test_existing_path_not_removed(self): self.mock_exists.return_value = True - utils.is_writeable('swift://AUTH_stor_test/container/test/') + stor_utils.is_writeable('swift://AUTH_stor_test/container/test/') self.assertFalse(self.mock_remove_container.called) def test_path_no_perms(self): self.mock_copy.side_effect = stor_swift.swift.UnauthorizedError('foo') - self.assertFalse(utils.is_writeable('swift://AUTH_stor_test/container/test/')) + self.assertFalse(stor_utils.is_writeable('swift://AUTH_stor_test/container/test/')) def test_disable_backoff(self): path = Path('swift://AUTH_stor_test/container/test/') swift_opts = {'num_retries': 0} - utils.is_writeable(path, swift_opts) + stor_utils.is_writeable(path, swift_opts) self.mock_copy.assert_called_with( self.filename, path, swift_retry_options=swift_opts) def test_no_trailing_slash(self): path = SwiftPath('swift://AUTH_stor_test/container') - utils.is_writeable(path) # no trailing slash + stor_utils.is_writeable(path) # no trailing slash self.mock_copy.assert_called_with( self.filename, stor_utils.with_trailing_slash(path), @@ -97,4 +97,4 @@ def test_container_created_in_another_client(self): # is_writeable is called. self.mock_exists.side_effect = [False, True] self.mock_remove_container.side_effect = stor_swift.swift.ConflictError('foo') - self.assertTrue(utils.is_writeable('swift://AUTH_stor_test/container/')) + self.assertTrue(stor_utils.is_writeable('swift://AUTH_stor_test/container/')) diff --git a/stor_swift/utils.py b/stor_swift/utils.py index a290a852..b293d81b 100644 --- a/stor_swift/utils.py +++ b/stor_swift/utils.py @@ -17,80 +17,3 @@ def is_swift_path(p): """ from stor_swift.swift import SwiftPath return p.startswith(SwiftPath.drive) - - -def is_writeable(path, swift_retry_options=None): - """ - Determine whether we have permission to write to path. - - Behavior of this method is slightly different for different storage types when the - directory doesn't exist: - 1. For local file systems, this function will return True if the target directory - exists and a file written to it. - 2. For AWS S3, this function will return True only if the target bucket is already - present and we have write access to the bucket. - 3. For Swift, this function will return True, only if the target tenant is already - present and we have write access to the tenant and container. The container doesn't - have to be present. - - This is function is useful, because `stor.stat()` will succeed if we have read-only - permissions to `path`, but the eventual attempt to upload will fail. - - Secondly, `path` might not exist yet. If the intent of the caller is to create it, , - stor.stat() will fail, however the eventual upload attempt would succeed. - - Args: - path (stor.Path|str): The path to check. - swift_retry_options (dict): Optional retry arguments to use for swift - upload or download. View the - `swift module-level documentation ` for more - information on retry arguments. If the goal is to not use - exponential backoff, pass ``{'num_retries': 0}`` here. - - Returns: - bool: Whether ``path`` is writeable or not. - """ - from stor import basename - from stor import join - from stor import Path - from stor import remove - from stor_swift.swift import ConflictError - from stor_swift.swift import SwiftPath - from stor_swift.swift import UnauthorizedError - from stor_swift.swift import UnavailableError - import stor - - path = stor_utils.with_trailing_slash(Path(path)) - - # We want this function to behave as a no-op with regards to the underlying - # container structure. Therefore we need to remove any containers created by this - # function that were not present when it was called. The `container_existed` - # defined below will store whether the container that we're checking existed when - # calling this function, so that we know if it should be removed at the end. - container_path = Path('{}{}/{}/'.format( - SwiftPath.drive, - path.tenant, - path.container - )) - container_existed = container_path.exists() - - with tempfile.NamedTemporaryFile() as tmpfile: - try: - # Attempt to create a file in the `path`. - stor.copy(tmpfile.name, path, swift_retry_options=swift_retry_options) - # Remove the file that was created. - remove(join(path, basename(tmpfile.name))) - answer = True - except (UnauthorizedError, UnavailableError, IOError, OSError, exceptions.FailedUploadError): # nopep8 - answer = False - - # Remove the Swift container if it didn't exist when calling this function, but exists - # now. This way this function remains a no-op with regards to container structure. - if container_existed is False and container_path.exists(): - try: - container_path.remove_container() - except ConflictError: - # Ignore if some other thread/user created the container in the meantime. - pass - - return answer From a12ef5f9957b763aebc21644ff9b241e594cf678 Mon Sep 17 00:00:00 2001 From: Anuj Kumar Date: Mon, 3 Dec 2018 15:17:53 -0800 Subject: [PATCH 3/7] bug fix --- stor/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/stor/utils.py b/stor/utils.py index 2892ded6..2c55b4a9 100644 --- a/stor/utils.py +++ b/stor/utils.py @@ -239,6 +239,7 @@ def is_writeable(path, swift_retry_options=None): from stor_swift.swift import SwiftPath from stor_swift.swift import UnauthorizedError from stor_swift.swift import UnavailableError + import stor path = with_trailing_slash(Path(path)) From 0aabfc92684f351c7a9150efae47cebb936510b3 Mon Sep 17 00:00:00 2001 From: Anuj Kumar Date: Mon, 3 Dec 2018 15:28:30 -0800 Subject: [PATCH 4/7] making is_writeable ready for package split --- stor/utils.py | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/stor/utils.py b/stor/utils.py index 2c55b4a9..5674b40c 100644 --- a/stor/utils.py +++ b/stor/utils.py @@ -235,10 +235,9 @@ def is_writeable(path, swift_retry_options=None): from stor import join from stor import Path from stor import remove - from stor_swift.swift import ConflictError - from stor_swift.swift import SwiftPath - from stor_swift.swift import UnauthorizedError - from stor_swift.swift import UnavailableError + from stor.exceptions import ConflictError + from stor.exceptions import UnauthorizedError + from stor.exceptions import UnavailableError import stor path = with_trailing_slash(Path(path)) @@ -248,18 +247,24 @@ def is_writeable(path, swift_retry_options=None): container_path = None container_existed = None - if is_swift_path(path): - # We want this function to behave as a no-op with regards to the underlying - # container structure. Therefore we need to remove any containers created by this - # function that were not present when it was called. The `container_existed` - # defined below will store whether the container that we're checking existed when - # calling this function, so that we know if it should be removed at the end. - container_path = Path('{}{}/{}/'.format( - SwiftPath.drive, - path.tenant, - path.container - )) - container_existed = container_path.exists() + try: + from stor_swift.swift import SwiftPath + from stor_swift.utils import is_swift_path + except ImportError as e: # pragma: no cover + logger.warn('Cannot import Swift module: {}'.format(e)) + else: + if is_swift_path(path): + # We want this function to behave as a no-op with regards to the underlying + # container structure. Therefore we need to remove any containers created by this + # function that were not present when it was called. The `container_existed` + # defined below will store whether the container that we're checking existed when + # calling this function, so that we know if it should be removed at the end. + container_path = Path('{}{}/{}/'.format( + SwiftPath.drive, + path.tenant, + path.container + )) + container_existed = container_path.exists() with tempfile.NamedTemporaryFile() as tmpfile: try: From df8892e40bd6466f95418d68ba8ce815ef4366b0 Mon Sep 17 00:00:00 2001 From: Anuj Kumar Date: Mon, 3 Dec 2018 16:15:15 -0800 Subject: [PATCH 5/7] Revert "making is_writeable ready for package split" This reverts commit 0aabfc92684f351c7a9150efae47cebb936510b3. --- stor/utils.py | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/stor/utils.py b/stor/utils.py index 5674b40c..2c55b4a9 100644 --- a/stor/utils.py +++ b/stor/utils.py @@ -235,9 +235,10 @@ def is_writeable(path, swift_retry_options=None): from stor import join from stor import Path from stor import remove - from stor.exceptions import ConflictError - from stor.exceptions import UnauthorizedError - from stor.exceptions import UnavailableError + from stor_swift.swift import ConflictError + from stor_swift.swift import SwiftPath + from stor_swift.swift import UnauthorizedError + from stor_swift.swift import UnavailableError import stor path = with_trailing_slash(Path(path)) @@ -247,24 +248,18 @@ def is_writeable(path, swift_retry_options=None): container_path = None container_existed = None - try: - from stor_swift.swift import SwiftPath - from stor_swift.utils import is_swift_path - except ImportError as e: # pragma: no cover - logger.warn('Cannot import Swift module: {}'.format(e)) - else: - if is_swift_path(path): - # We want this function to behave as a no-op with regards to the underlying - # container structure. Therefore we need to remove any containers created by this - # function that were not present when it was called. The `container_existed` - # defined below will store whether the container that we're checking existed when - # calling this function, so that we know if it should be removed at the end. - container_path = Path('{}{}/{}/'.format( - SwiftPath.drive, - path.tenant, - path.container - )) - container_existed = container_path.exists() + if is_swift_path(path): + # We want this function to behave as a no-op with regards to the underlying + # container structure. Therefore we need to remove any containers created by this + # function that were not present when it was called. The `container_existed` + # defined below will store whether the container that we're checking existed when + # calling this function, so that we know if it should be removed at the end. + container_path = Path('{}{}/{}/'.format( + SwiftPath.drive, + path.tenant, + path.container + )) + container_existed = container_path.exists() with tempfile.NamedTemporaryFile() as tmpfile: try: From 66e62cea79a315468499cca0da2a52b5f7288847 Mon Sep 17 00:00:00 2001 From: Anuj Kumar Date: Mon, 3 Dec 2018 16:15:20 -0800 Subject: [PATCH 6/7] Revert "bug fix" This reverts commit a12ef5f9957b763aebc21644ff9b241e594cf678. --- stor/utils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/stor/utils.py b/stor/utils.py index 2c55b4a9..2892ded6 100644 --- a/stor/utils.py +++ b/stor/utils.py @@ -239,7 +239,6 @@ def is_writeable(path, swift_retry_options=None): from stor_swift.swift import SwiftPath from stor_swift.swift import UnauthorizedError from stor_swift.swift import UnavailableError - import stor path = with_trailing_slash(Path(path)) From 13adfcb0842605a7206ab8a5a85c709c98feae94 Mon Sep 17 00:00:00 2001 From: Anuj Kumar Date: Mon, 3 Dec 2018 16:15:22 -0800 Subject: [PATCH 7/7] Revert "reverting changes to is_writeable but leaving the tests in resp modules" This reverts commit afb4d6a75b425496132692b04734c527746c82a8. --- stor/utils.py | 47 +---------------- stor_s3/tests/test_utils_s3.py | 4 +- stor_s3/utils.py | 68 ++++++++++++++++++++++++ stor_swift/tests/test_utils_swift.py | 16 +++--- stor_swift/utils.py | 77 ++++++++++++++++++++++++++++ 5 files changed, 156 insertions(+), 56 deletions(-) diff --git a/stor/utils.py b/stor/utils.py index 2892ded6..5322253f 100644 --- a/stor/utils.py +++ b/stor/utils.py @@ -231,55 +231,10 @@ def is_writeable(path, swift_retry_options=None): Returns: bool: Whether ``path`` is writeable or not. """ - from stor import basename - from stor import join from stor import Path - from stor import remove - from stor_swift.swift import ConflictError - from stor_swift.swift import SwiftPath - from stor_swift.swift import UnauthorizedError - from stor_swift.swift import UnavailableError - path = with_trailing_slash(Path(path)) - if is_filesystem_path(path): - return os.access(path, os.W_OK) - - container_path = None - container_existed = None - if is_swift_path(path): - # We want this function to behave as a no-op with regards to the underlying - # container structure. Therefore we need to remove any containers created by this - # function that were not present when it was called. The `container_existed` - # defined below will store whether the container that we're checking existed when - # calling this function, so that we know if it should be removed at the end. - container_path = Path('{}{}/{}/'.format( - SwiftPath.drive, - path.tenant, - path.container - )) - container_existed = container_path.exists() - - with tempfile.NamedTemporaryFile() as tmpfile: - try: - # Attempt to create a file in the `path`. - stor.copy(tmpfile.name, path, swift_retry_options=swift_retry_options) - # Remove the file that was created. - remove(join(path, basename(tmpfile.name))) - answer = True - except (UnauthorizedError, UnavailableError, IOError, OSError, exceptions.FailedUploadError): # nopep8 - answer = False - - # Remove the Swift container if it didn't exist when calling this function, but exists - # now. This way this function remains a no-op with regards to container structure. - if container_existed is False and container_path.exists(): - try: - container_path.remove_container() - except ConflictError: - # Ignore if some other thread/user created the container in the meantime. - pass - - return answer + return os.access(path, os.W_OK) def _safe_get_size(name): diff --git a/stor_s3/tests/test_utils_s3.py b/stor_s3/tests/test_utils_s3.py index 30877418..d0417731 100644 --- a/stor_s3/tests/test_utils_s3.py +++ b/stor_s3/tests/test_utils_s3.py @@ -42,11 +42,11 @@ def setUp(self): def test_success(self): path = 's3://stor-test/foo/bar' - self.assertTrue(stor.utils.is_writeable(path)) + self.assertTrue(utils.is_writeable(path)) self.mock_remove.assert_called_with( S3Path('{}/{}'.format(path, self.filename))) def test_path_no_perms(self): self.mock_copy.side_effect = stor.exceptions.FailedUploadError('foo') - self.assertFalse(stor.utils.is_writeable('s3://stor-test/foo/bar')) + self.assertFalse(utils.is_writeable('s3://stor-test/foo/bar')) self.assertFalse(self.mock_remove.called) diff --git a/stor_s3/utils.py b/stor_s3/utils.py index 88ea7598..187aca32 100644 --- a/stor_s3/utils.py +++ b/stor_s3/utils.py @@ -16,3 +16,71 @@ def is_s3_path(p): """ from stor_s3.s3 import S3Path return p.startswith(S3Path.drive) + + +def is_writeable(path, swift_retry_options=None): + """ + Determine whether we have permission to write to path. + + Behavior of this method is slightly different for different storage types when the + directory doesn't exist: + 1. For local file systems, this function will return True if the target directory + exists and a file written to it. + 2. For AWS S3, this function will return True only if the target bucket is already + present and we have write access to the bucket. + 3. For Swift, this function will return True, only if the target tenant is already + present and we have write access to the tenant and container. The container doesn't + have to be present. + + This is function is useful, because `stor.stat()` will succeed if we have read-only + permissions to `path`, but the eventual attempt to upload will fail. + + Secondly, `path` might not exist yet. If the intent of the caller is to create it, , + stor.stat() will fail, however the eventual upload attempt would succeed. + + Args: + path (stor.Path|str): The path to check. + swift_retry_options (dict): Optional retry arguments to use for swift + upload or download. View the + `swift module-level documentation ` for more + information on retry arguments. If the goal is to not use + exponential backoff, pass ``{'num_retries': 0}`` here. + + Returns: + bool: Whether ``path`` is writeable or not. + """ + from stor import basename + from stor import join + from stor import Path + from stor import remove + from stor.exceptions import UnauthorizedError + from stor.exceptions import UnavailableError + from stor.exceptions import ConflictError + from stor.exceptions import FailedUploadError + import stor + + path = stor_utils.with_trailing_slash(Path(path)) + + container_path = None + container_existed = None + + with tempfile.NamedTemporaryFile() as tmpfile: + try: + # Attempt to create a file in the `path`. + stor.copy(tmpfile.name, path, swift_retry_options=swift_retry_options) + # Remove the file that was created. + remove(join(path, basename(tmpfile.name))) + answer = True + except (UnauthorizedError, UnavailableError, IOError, OSError, FailedUploadError): # nopep8 + answer = False + + # Remove the Swift container if it didn't exist when calling this function, but exists + # now. This way this function remains a no-op with regards to container structure. + if container_existed is False and container_path.exists(): # pragma: no cover + try: + container_path.remove_container() + except ConflictError: + # Ignore if some other thread/user created the container in the meantime. + pass + + return answer diff --git a/stor_swift/tests/test_utils_swift.py b/stor_swift/tests/test_utils_swift.py index 9110c995..b137ce53 100644 --- a/stor_swift/tests/test_utils_swift.py +++ b/stor_swift/tests/test_utils_swift.py @@ -53,39 +53,39 @@ def setUp(self): def test_existing_path(self): self.mock_exists.return_value = True path = SwiftPath('swift://AUTH_stor_test/container/test/') - self.assertTrue(stor_utils.is_writeable(path)) + self.assertTrue(utils.is_writeable(path)) self.mock_remove.assert_called_with(path / self.filename) def test_non_existing_path(self): self.mock_exists.return_value = False - self.assertTrue(stor_utils.is_writeable('swift://AUTH_stor_test/container/test/')) + self.assertTrue(utils.is_writeable('swift://AUTH_stor_test/container/test/')) def test_path_unchanged(self): # Make the first call to exists() return False and the second return True. self.mock_exists.side_effect = [False, True] - stor_utils.is_writeable('swift://AUTH_stor_test/container/test/') + utils.is_writeable('swift://AUTH_stor_test/container/test/') self.mock_remove_container.assert_called_once_with( SwiftPath('swift://AUTH_stor_test/container/')) def test_existing_path_not_removed(self): self.mock_exists.return_value = True - stor_utils.is_writeable('swift://AUTH_stor_test/container/test/') + utils.is_writeable('swift://AUTH_stor_test/container/test/') self.assertFalse(self.mock_remove_container.called) def test_path_no_perms(self): self.mock_copy.side_effect = stor_swift.swift.UnauthorizedError('foo') - self.assertFalse(stor_utils.is_writeable('swift://AUTH_stor_test/container/test/')) + self.assertFalse(utils.is_writeable('swift://AUTH_stor_test/container/test/')) def test_disable_backoff(self): path = Path('swift://AUTH_stor_test/container/test/') swift_opts = {'num_retries': 0} - stor_utils.is_writeable(path, swift_opts) + utils.is_writeable(path, swift_opts) self.mock_copy.assert_called_with( self.filename, path, swift_retry_options=swift_opts) def test_no_trailing_slash(self): path = SwiftPath('swift://AUTH_stor_test/container') - stor_utils.is_writeable(path) # no trailing slash + utils.is_writeable(path) # no trailing slash self.mock_copy.assert_called_with( self.filename, stor_utils.with_trailing_slash(path), @@ -97,4 +97,4 @@ def test_container_created_in_another_client(self): # is_writeable is called. self.mock_exists.side_effect = [False, True] self.mock_remove_container.side_effect = stor_swift.swift.ConflictError('foo') - self.assertTrue(stor_utils.is_writeable('swift://AUTH_stor_test/container/')) + self.assertTrue(utils.is_writeable('swift://AUTH_stor_test/container/')) diff --git a/stor_swift/utils.py b/stor_swift/utils.py index b293d81b..a290a852 100644 --- a/stor_swift/utils.py +++ b/stor_swift/utils.py @@ -17,3 +17,80 @@ def is_swift_path(p): """ from stor_swift.swift import SwiftPath return p.startswith(SwiftPath.drive) + + +def is_writeable(path, swift_retry_options=None): + """ + Determine whether we have permission to write to path. + + Behavior of this method is slightly different for different storage types when the + directory doesn't exist: + 1. For local file systems, this function will return True if the target directory + exists and a file written to it. + 2. For AWS S3, this function will return True only if the target bucket is already + present and we have write access to the bucket. + 3. For Swift, this function will return True, only if the target tenant is already + present and we have write access to the tenant and container. The container doesn't + have to be present. + + This is function is useful, because `stor.stat()` will succeed if we have read-only + permissions to `path`, but the eventual attempt to upload will fail. + + Secondly, `path` might not exist yet. If the intent of the caller is to create it, , + stor.stat() will fail, however the eventual upload attempt would succeed. + + Args: + path (stor.Path|str): The path to check. + swift_retry_options (dict): Optional retry arguments to use for swift + upload or download. View the + `swift module-level documentation ` for more + information on retry arguments. If the goal is to not use + exponential backoff, pass ``{'num_retries': 0}`` here. + + Returns: + bool: Whether ``path`` is writeable or not. + """ + from stor import basename + from stor import join + from stor import Path + from stor import remove + from stor_swift.swift import ConflictError + from stor_swift.swift import SwiftPath + from stor_swift.swift import UnauthorizedError + from stor_swift.swift import UnavailableError + import stor + + path = stor_utils.with_trailing_slash(Path(path)) + + # We want this function to behave as a no-op with regards to the underlying + # container structure. Therefore we need to remove any containers created by this + # function that were not present when it was called. The `container_existed` + # defined below will store whether the container that we're checking existed when + # calling this function, so that we know if it should be removed at the end. + container_path = Path('{}{}/{}/'.format( + SwiftPath.drive, + path.tenant, + path.container + )) + container_existed = container_path.exists() + + with tempfile.NamedTemporaryFile() as tmpfile: + try: + # Attempt to create a file in the `path`. + stor.copy(tmpfile.name, path, swift_retry_options=swift_retry_options) + # Remove the file that was created. + remove(join(path, basename(tmpfile.name))) + answer = True + except (UnauthorizedError, UnavailableError, IOError, OSError, exceptions.FailedUploadError): # nopep8 + answer = False + + # Remove the Swift container if it didn't exist when calling this function, but exists + # now. This way this function remains a no-op with regards to container structure. + if container_existed is False and container_path.exists(): + try: + container_path.remove_container() + except ConflictError: + # Ignore if some other thread/user created the container in the meantime. + pass + + return answer