From 44c468c9542e0e3f1a3d4185a4d468c6a79896e4 Mon Sep 17 00:00:00 2001 From: Addison Schiller Date: Fri, 22 Sep 2017 10:07:03 -0400 Subject: [PATCH 1/5] Reject complicated self-overwrites Fixing an issue where when a child component is hooked up To a project, and they share the same provider, it is possible to silently delete files --- tests/providers/box/test_provider.py | 29 ++++++++++--- tests/providers/dropbox/test_provider.py | 15 +++++++ tests/providers/googledrive/test_provider.py | 42 ++++++++++++------- tests/providers/owncloud/test_provider.py | 1 - waterbutler/providers/box/provider.py | 10 +++++ waterbutler/providers/dropbox/provider.py | 10 +++++ waterbutler/providers/googledrive/provider.py | 10 +++++ waterbutler/providers/owncloud/provider.py | 5 +++ 8 files changed, 102 insertions(+), 20 deletions(-) diff --git a/tests/providers/box/test_provider.py b/tests/providers/box/test_provider.py index 47e8d55ab..9a8a0c756 100644 --- a/tests/providers/box/test_provider.py +++ b/tests/providers/box/test_provider.py @@ -583,12 +583,22 @@ async def test_intra_copy_file(self, provider, root_provider_fixtures): assert result == expected + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_intra_copy_overwrite_error(self, provider, root_provider_fixtures): + item = root_provider_fixtures['file_metadata']['entries'][0] + src_path = WaterButlerPath('/name.txt', _ids=(provider, item['id'])) + with pytest.raises(exceptions.IntraCopyError) as e: + await provider.intra_copy(provider, src_path, src_path) + + assert e.value.code == 409 + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_intra_copy_file_replace(self, provider, root_provider_fixtures): item = root_provider_fixtures['file_metadata']['entries'][0] src_path = WaterButlerPath('/name.txt', _ids=(provider, item['id'])) - dest_path = WaterButlerPath('/charmander/name.txt', _ids=(provider, item['id'], item['id'])) + dest_path = WaterButlerPath('/charmander/name.txt', _ids=(provider, item['id'], 'cats77831')) file_url = provider.build_url('files', src_path.identifier, 'copy') delete_url = provider.build_url('files', dest_path.identifier) @@ -636,7 +646,7 @@ async def test_intra_copy_folder_replace(self, provider, intra_fixtures, root_pr list_metadata = root_provider_fixtures['folder_list_metadata'] src_path = WaterButlerPath('/name/', _ids=(provider, item['id'])) - dest_path = WaterButlerPath('/charmander/name/', _ids=(provider, item['id'], item['id'])) + dest_path = WaterButlerPath('/charmander/name/', _ids=(provider, item['id'], '4jkmrm4zzerj')) file_url = provider.build_url('folders', src_path.identifier, 'copy') delete_url = provider.build_url('folders', dest_path.identifier, recursive=True) @@ -679,12 +689,22 @@ async def test_intra_move_file(self, provider, root_provider_fixtures): assert result == expected + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_intra_move_overwrite_error(self, provider, root_provider_fixtures): + item = root_provider_fixtures['file_metadata']['entries'][0] + src_path = WaterButlerPath('/name.txt', _ids=(provider, item['id'])) + with pytest.raises(exceptions.IntraCopyError) as e: + await provider.intra_move(provider, src_path, src_path) + + assert e.value.code == 409 + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_intra_move_file_replace(self, provider, root_provider_fixtures): item = root_provider_fixtures['file_metadata']['entries'][0] src_path = WaterButlerPath('/name.txt', _ids=(provider, item['id'])) - dest_path = WaterButlerPath('/charmander/name.txt', _ids=(provider, item['id'], item['id'])) + dest_path = WaterButlerPath('/charmander/name.txt', _ids=(provider, item['id'], 'YgzZejrj834j')) file_url = provider.build_url('files', src_path.identifier) delete_url = provider.build_url('files', dest_path.identifier) @@ -725,7 +745,6 @@ async def test_intra_move_folder(self, provider, intra_fixtures, root_provider_f assert result == expected - @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_intra_move_folder_replace(self, provider, intra_fixtures, root_provider_fixtures): @@ -733,7 +752,7 @@ async def test_intra_move_folder_replace(self, provider, intra_fixtures, root_pr list_metadata = root_provider_fixtures['folder_list_metadata'] src_path = WaterButlerPath('/name/', _ids=(provider, item['id'])) - dest_path = WaterButlerPath('/charmander/name/', _ids=(provider, item['id'], item['id'])) + dest_path = WaterButlerPath('/charmander/name/', _ids=(provider, item['id'], '7759994812')) file_url = provider.build_url('folders', src_path.identifier) delete_url = provider.build_url('folders', dest_path.identifier, recursive=True) diff --git a/tests/providers/dropbox/test_provider.py b/tests/providers/dropbox/test_provider.py index 01f1b6c00..582f80d11 100644 --- a/tests/providers/dropbox/test_provider.py +++ b/tests/providers/dropbox/test_provider.py @@ -719,6 +719,21 @@ async def test_intra_move_casing_change(self, provider): assert e.value.code == 400 + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_intra_overwrite_error(self, provider): + src_path = WaterButlerPath('/pfile.txt', prepend=provider.folder) + + with pytest.raises(exceptions.IntraCopyError) as e: + await provider.intra_move(provider, src_path, src_path) + + assert e.value.code == 409 + + with pytest.raises(exceptions.IntraCopyError) as e: + await provider.intra_copy(provider, src_path, src_path) + + assert e.value.code == 409 + class TestOperations: diff --git a/tests/providers/googledrive/test_provider.py b/tests/providers/googledrive/test_provider.py index 81624f5a5..061d11b6e 100644 --- a/tests/providers/googledrive/test_provider.py +++ b/tests/providers/googledrive/test_provider.py @@ -1488,9 +1488,9 @@ class TestIntraFunctions: @pytest.mark.aiohttpretty async def test_intra_move_file(self, provider, root_provider_fixtures): item = root_provider_fixtures['docs_file_metadata'] - src_path = WaterButlerPath('/unsure.txt', _ids=(provider.folder['id'], item['id'])) - dest_path = WaterButlerPath('/really/unsure.txt', _ids=(provider.folder['id'], - item['id'], item['id'])) + src_path = WaterButlerPath('/unsure.txt', _ids=('0', item['id'])) + dest_path = WaterButlerPath('/really/unsure.txt', _ids=('0', + 'yy42kcj', 'rrjk42k')) url = provider.build_url('files', src_path.identifier) data = json.dumps({ @@ -1501,7 +1501,7 @@ async def test_intra_move_file(self, provider, root_provider_fixtures): }), aiohttpretty.register_json_uri('PATCH', url, data=data, body=item) - delete_url = provider.build_url('files', item['id']) + delete_url = provider.build_url('files', dest_path.identifier) del_url_body = json.dumps({'labels': {'trashed': 'true'}}) aiohttpretty.register_uri('PUT', delete_url, body=del_url_body, status=200) @@ -1515,9 +1515,9 @@ async def test_intra_move_file(self, provider, root_provider_fixtures): @pytest.mark.aiohttpretty async def test_intra_move_folder(self, provider, root_provider_fixtures): item = root_provider_fixtures['folder_metadata'] - src_path = WaterButlerPath('/unsure/', _ids=(provider.folder['id'], item['id'])) - dest_path = WaterButlerPath('/really/unsure/', _ids=(provider.folder['id'], - item['id'], item['id'])) + src_path = WaterButlerPath('/unsure/', _ids=('0', item['id'])) + dest_path = WaterButlerPath('/really/unsure/', _ids=('0', + '42jdkerf', '7ejGjeajr')) url = provider.build_url('files', src_path.identifier) data = json.dumps({ @@ -1528,11 +1528,11 @@ async def test_intra_move_folder(self, provider, root_provider_fixtures): }), aiohttpretty.register_json_uri('PATCH', url, data=data, body=item) - delete_url = provider.build_url('files', item['id']) + delete_url = provider.build_url('files', dest_path.identifier) del_url_body = json.dumps({'labels': {'trashed': 'true'}}) aiohttpretty.register_uri('PUT', delete_url, body=del_url_body, status=200) - children_query = provider._build_query(dest_path.identifier) + children_query = provider._build_query(src_path.identifier) children_url = provider.build_url('files', q=children_query, alt='json', maxResults=1000) children_list = generate_list(3, **root_provider_fixtures['folder_metadata']) aiohttpretty.register_json_uri('GET', children_url, body=children_list) @@ -1551,10 +1551,9 @@ async def test_intra_move_folder(self, provider, root_provider_fixtures): @pytest.mark.aiohttpretty async def test_intra_copy_file(self, provider, root_provider_fixtures): item = root_provider_fixtures['docs_file_metadata'] - src_path = WaterButlerPath('/unsure.txt', _ids=(provider.folder['id'], item['id'])) - dest_path = WaterButlerPath('/really/unsure.txt', _ids=(provider.folder['id'], - item['id'], item['id'])) - + src_path = WaterButlerPath('/unsure.txt', _ids=('0', item['id'])) + dest_path = WaterButlerPath('/really/unsure.txt', _ids=('0', + '312kjfe', '4ckk2lkl3')) url = provider.build_url('files', src_path.identifier, 'copy') data = json.dumps({ 'parents': [{ @@ -1564,7 +1563,7 @@ async def test_intra_copy_file(self, provider, root_provider_fixtures): }), aiohttpretty.register_json_uri('POST', url, data=data, body=item) - delete_url = provider.build_url('files', item['id']) + delete_url = provider.build_url('files', dest_path.identifier) del_url_body = json.dumps({'labels': {'trashed': 'true'}}) aiohttpretty.register_uri('PUT', delete_url, body=del_url_body, status=200) @@ -1574,6 +1573,21 @@ async def test_intra_copy_file(self, provider, root_provider_fixtures): assert result == expected assert aiohttpretty.has_call(method='PUT', uri=delete_url) + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_intra_copy_move_overwrite_error(self, provider, root_provider_fixtures): + item = root_provider_fixtures['docs_file_metadata'] + src_path = WaterButlerPath('/unsure.txt', _ids=('0', item['id'])) + + with pytest.raises(exceptions.IntraCopyError) as e: + await provider.intra_copy(provider, src_path, src_path) + + assert e.value.code == 409 + with pytest.raises(exceptions.IntraCopyError) as e: + await provider.intra_move(provider, src_path, src_path) + + assert e.value.code == 409 + class TestOperationsOrMisc: diff --git a/tests/providers/owncloud/test_provider.py b/tests/providers/owncloud/test_provider.py index 3ef3cb5d9..c4e8ac521 100644 --- a/tests/providers/owncloud/test_provider.py +++ b/tests/providers/owncloud/test_provider.py @@ -321,7 +321,6 @@ async def test_intra_copy_file(self, provider, file_metadata): assert metadata.name == 'dissertation.aux' assert metadata.kind == 'file' - class TestMetadata: @pytest.mark.asyncio diff --git a/waterbutler/providers/box/provider.py b/waterbutler/providers/box/provider.py index a8b8f8096..b55aa4779 100644 --- a/waterbutler/providers/box/provider.py +++ b/waterbutler/providers/box/provider.py @@ -195,6 +195,11 @@ async def intra_copy(self, # type: ignore src_path: wb_path.WaterButlerPath, dest_path: wb_path.WaterButlerPath) \ -> typing.Tuple[typing.Union[BoxFileMetadata, BoxFolderMetadata], bool]: + + if src_path.identifier == dest_path.identifier: + raise exceptions.IntraCopyError("Cannot overwrite a file with itself", + code=HTTPStatus.CONFLICT) + if dest_path.identifier is not None: await dest_provider.delete(dest_path) @@ -223,6 +228,11 @@ async def intra_move(self, # type: ignore dest_provider: provider.BaseProvider, src_path: wb_path.WaterButlerPath, dest_path: wb_path.WaterButlerPath) -> typing.Tuple[BaseBoxMetadata, bool]: + + if src_path.identifier == dest_path.identifier: + raise exceptions.IntraCopyError("Cannot overwrite a file with itself", + code=HTTPStatus.CONFLICT) + if dest_path.identifier is not None and str(dest_path).lower() != str(src_path).lower(): await dest_provider.delete(dest_path) diff --git a/waterbutler/providers/dropbox/provider.py b/waterbutler/providers/dropbox/provider.py index 3353770f5..034f73774 100644 --- a/waterbutler/providers/dropbox/provider.py +++ b/waterbutler/providers/dropbox/provider.py @@ -151,6 +151,11 @@ async def intra_copy(self, # type: ignore dest_path: WaterButlerPath) \ -> typing.Tuple[typing.Union[DropboxFileMetadata, DropboxFolderMetadata], bool]: dest_folder = dest_provider.folder + + if dest_path.full_path == src_path.full_path: + raise exceptions.IntraCopyError("Cannot overwrite a file with itself", + code=HTTPStatus.CONFLICT) + try: if self == dest_provider: data = await self.dropbox_request( @@ -192,6 +197,11 @@ async def intra_move(self, # type: ignore dest_provider: 'DropboxProvider', src_path: WaterButlerPath, dest_path: WaterButlerPath) -> typing.Tuple[BaseDropboxMetadata, bool]: + + if dest_path.full_path == src_path.full_path: + raise exceptions.IntraCopyError("Cannot overwrite a file with itself", + code=HTTPStatus.CONFLICT) + if dest_path.full_path.lower() == src_path.full_path.lower(): # Dropbox does not support changing the casing in a file name raise exceptions.InvalidPathError( diff --git a/waterbutler/providers/googledrive/provider.py b/waterbutler/providers/googledrive/provider.py index 73712d601..607dc5e07 100644 --- a/waterbutler/providers/googledrive/provider.py +++ b/waterbutler/providers/googledrive/provider.py @@ -151,6 +151,11 @@ async def intra_move(self, # type: ignore src_path: wb_path.WaterButlerPath, dest_path: wb_path.WaterButlerPath) \ -> typing.Tuple[BaseGoogleDriveMetadata, bool]: + + if src_path.identifier == dest_path.identifier: + raise exceptions.IntraCopyError("Cannot overwrite a file with itself", + code=HTTPStatus.CONFLICT) + self.metrics.add('intra_move.destination_exists', dest_path.identifier is not None) if dest_path.identifier: await dest_provider.delete(dest_path) @@ -187,6 +192,11 @@ async def intra_copy(self, src_path: wb_path.WaterButlerPath, dest_path: wb_path.WaterButlerPath) \ -> typing.Tuple[GoogleDriveFileMetadata, bool]: + + if src_path.identifier == dest_path.identifier: + raise exceptions.IntraCopyError("Cannot overwrite a file with itself", + code=HTTPStatus.CONFLICT) + self.metrics.add('intra_copy.destination_exists', dest_path.identifier is not None) if dest_path.identifier: await dest_provider.delete(dest_path) diff --git a/waterbutler/providers/owncloud/provider.py b/waterbutler/providers/owncloud/provider.py index 29cc2b10e..81947dcf9 100644 --- a/waterbutler/providers/owncloud/provider.py +++ b/waterbutler/providers/owncloud/provider.py @@ -1,4 +1,5 @@ import aiohttp +from http import HTTPStatus from waterbutler.core import streams from waterbutler.core import provider @@ -291,6 +292,10 @@ async def _do_dav_move_copy(self, src_path, dest_path, operation): if operation != 'MOVE' and operation != 'COPY': raise NotImplementedError("ownCloud move/copy only supports MOVE and COPY endpoints") + if src_path.full_path == dest_path.full_path: + raise exceptions.IntraCopyError("Cannot overwrite a file with itself", + code=HTTPStatus.CONFLICT) + resp = await self.make_request( operation, self._webdav_url_ + src_path.full_path, From 049c593c29f195bb64de05b12acd63f7806f6519 Mon Sep 17 00:00:00 2001 From: Addison Schiller Date: Wed, 22 Nov 2017 09:48:31 -0500 Subject: [PATCH 2/5] Reworking and refactoring --- tests/core/test_provider.py | 18 ++++ tests/providers/box/test_provider.py | 92 ++++++++++--------- tests/providers/dropbox/test_provider.py | 61 +++++++----- tests/providers/googledrive/test_provider.py | 43 +++------ tests/providers/owncloud/test_provider.py | 14 +++ waterbutler/core/provider.py | 55 +++++++++++ waterbutler/providers/box/provider.py | 11 +-- waterbutler/providers/dropbox/provider.py | 11 +-- waterbutler/providers/googledrive/provider.py | 11 +-- waterbutler/providers/owncloud/provider.py | 8 +- 10 files changed, 200 insertions(+), 124 deletions(-) diff --git a/tests/core/test_provider.py b/tests/core/test_provider.py index 123888ca0..d06386560 100644 --- a/tests/core/test_provider.py +++ b/tests/core/test_provider.py @@ -276,6 +276,15 @@ async def test_passes_on_rename(self, provider1): conflict='replace', ) + @pytest.mark.asyncio + async def test_copy_will_self_overwrite(self, provider1): + src_path = await provider1.validate_path('/source/path') + dest_path = await provider1.validate_path('/destination/') + provider1.will_self_overwrite = utils.MockCoroutine() + + with pytest.raises(exceptions.OverwriteSelfError): + await provider1.copy(provider1, src_path, dest_path) + @pytest.mark.asyncio async def test_checks_can_intra_copy(self, provider1): provider1.can_intra_copy = mock.Mock(return_value=False) @@ -393,6 +402,15 @@ async def test_passes_on_rename(self, provider1): conflict='replace', ) + @pytest.mark.asyncio + async def test_move_will_self_overwrite(self, provider1): + src_path = await provider1.validate_path('/source/path') + dest_path = await provider1.validate_path('/destination/') + provider1.will_self_overwrite = utils.MockCoroutine() + + with pytest.raises(exceptions.OverwriteSelfError): + await provider1.move(provider1, src_path, dest_path) + @pytest.mark.asyncio async def test_checks_can_intra_move(self, provider1): provider1.can_intra_move = mock.Mock(return_value=False) diff --git a/tests/providers/box/test_provider.py b/tests/providers/box/test_provider.py index 9a8a0c756..32bd3690a 100644 --- a/tests/providers/box/test_provider.py +++ b/tests/providers/box/test_provider.py @@ -165,7 +165,9 @@ async def test_validate_path(self, provider, root_provider_fixtures): provider.folder = '0' folder_id = '0' - good_url = provider.build_url('folders', folder_id, 'items', fields='id,name,type', limit=1000) + good_url = provider.build_url('folders', folder_id, 'items', + fields='id,name,type', limit=1000) + aiohttpretty.register_json_uri('GET', good_url, body=root_provider_fixtures['revalidate_metadata'], status=200) @@ -295,7 +297,7 @@ async def test_upload_checksum_mismatch(self, provider, root_provider_fixtures, aiohttpretty.register_json_uri('POST', upload_url, status=201, body=root_provider_fixtures['checksum_mismatch_metadata']) - with pytest.raises(exceptions.UploadChecksumMismatchError) as exc: + with pytest.raises(exceptions.UploadChecksumMismatchError): await provider.upload(file_stream, path) assert aiohttpretty.has_call(method='POST', uri=upload_url) @@ -360,8 +362,11 @@ async def test_delete_root(self, provider, root_provider_fixtures): url = provider.build_url('folders', root_path.identifier, 'items', fields='id,name,size,modified_at,etag,total_count', offset=(0), limit=1000) - aiohttpretty.register_json_uri('GET', url, - body=root_provider_fixtures['one_entry_folder_list_metadata']) + aiohttpretty.register_json_uri( + 'GET', + url, + body=root_provider_fixtures['one_entry_folder_list_metadata'] + ) url = provider.build_url('files', item['id'], fields='id,name,path_collection') delete_url = provider.build_url('files', path.identifier) @@ -568,6 +573,7 @@ async def test_get_revisions_free_account(self, provider, root_provider_fixtures class TestIntraCopy: + @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_intra_copy_file(self, provider, root_provider_fixtures): @@ -583,16 +589,6 @@ async def test_intra_copy_file(self, provider, root_provider_fixtures): assert result == expected - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_intra_copy_overwrite_error(self, provider, root_provider_fixtures): - item = root_provider_fixtures['file_metadata']['entries'][0] - src_path = WaterButlerPath('/name.txt', _ids=(provider, item['id'])) - with pytest.raises(exceptions.IntraCopyError) as e: - await provider.intra_copy(provider, src_path, src_path) - - assert e.value.code == 409 - @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_intra_copy_file_replace(self, provider, root_provider_fixtures): @@ -630,7 +626,9 @@ async def test_intra_copy_folder(self, provider, intra_fixtures, root_provider_f expected_folder = BoxFolderMetadata(item, dest_path) expected_folder._children = [] for child_item in list_metadata['entries']: - child_path = dest_path.child(child_item['name'], folder=(child_item['type'] == 'folder')) + child_path = dest_path.child(child_item['name'], + folder=(child_item['type'] == 'folder')) + serialized_child = provider._serialize_item(child_item, child_path) expected_folder._children.append(serialized_child) expected = (expected_folder, True) @@ -641,7 +639,10 @@ async def test_intra_copy_folder(self, provider, intra_fixtures, root_provider_f @pytest.mark.asyncio @pytest.mark.aiohttpretty - async def test_intra_copy_folder_replace(self, provider, intra_fixtures, root_provider_fixtures): + async def test_intra_copy_folder_replace(self, + provider, + intra_fixtures, + root_provider_fixtures): item = intra_fixtures['intra_folder_metadata'] list_metadata = root_provider_fixtures['folder_list_metadata'] @@ -661,7 +662,9 @@ async def test_intra_copy_folder_replace(self, provider, intra_fixtures, root_pr expected_folder = BoxFolderMetadata(item, dest_path) expected_folder._children = [] for child_item in list_metadata['entries']: - child_path = dest_path.child(child_item['name'], folder=(child_item['type'] == 'folder')) + child_path = dest_path.child(child_item['name'], + folder=(child_item['type'] == 'folder')) + serialized_child = provider._serialize_item(child_item, child_path) expected_folder._children.append(serialized_child) expected = (expected_folder, False) @@ -689,22 +692,13 @@ async def test_intra_move_file(self, provider, root_provider_fixtures): assert result == expected - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_intra_move_overwrite_error(self, provider, root_provider_fixtures): - item = root_provider_fixtures['file_metadata']['entries'][0] - src_path = WaterButlerPath('/name.txt', _ids=(provider, item['id'])) - with pytest.raises(exceptions.IntraCopyError) as e: - await provider.intra_move(provider, src_path, src_path) - - assert e.value.code == 409 - @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_intra_move_file_replace(self, provider, root_provider_fixtures): item = root_provider_fixtures['file_metadata']['entries'][0] src_path = WaterButlerPath('/name.txt', _ids=(provider, item['id'])) - dest_path = WaterButlerPath('/charmander/name.txt', _ids=(provider, item['id'], 'YgzZejrj834j')) + dest_path = WaterButlerPath('/charmander/name.txt', + _ids=(provider, item['id'], 'YgzZejrj834j')) file_url = provider.build_url('files', src_path.identifier) delete_url = provider.build_url('files', dest_path.identifier) @@ -736,7 +730,10 @@ async def test_intra_move_folder(self, provider, intra_fixtures, root_provider_f expected_folder = BoxFolderMetadata(item, dest_path) expected_folder._children = [] for child_item in list_metadata['entries']: - child_path = dest_path.child(child_item['name'], folder=(child_item['type'] == 'folder')) + child_path = dest_path.child( + child_item['name'], + folder=(child_item['type'] == 'folder') + ) serialized_child = provider._serialize_item(child_item, child_path) expected_folder._children.append(serialized_child) expected = (expected_folder, True) @@ -747,7 +744,10 @@ async def test_intra_move_folder(self, provider, intra_fixtures, root_provider_f @pytest.mark.asyncio @pytest.mark.aiohttpretty - async def test_intra_move_folder_replace(self, provider, intra_fixtures, root_provider_fixtures): + async def test_intra_move_folder_replace(self, + provider, + intra_fixtures, + root_provider_fixtures): item = intra_fixtures['intra_folder_metadata'] list_metadata = root_provider_fixtures['folder_list_metadata'] @@ -767,7 +767,9 @@ async def test_intra_move_folder_replace(self, provider, intra_fixtures, root_pr expected_folder = BoxFolderMetadata(item, dest_path) expected_folder._children = [] for child_item in list_metadata['entries']: - child_path = dest_path.child(child_item['name'], folder=(child_item['type'] == 'folder')) + child_path = dest_path.child(child_item['name'], + folder=(child_item['type'] == 'folder')) + serialized_child = provider._serialize_item(child_item, child_path) expected_folder._children.append(serialized_child) expected = (expected_folder, False) @@ -851,25 +853,29 @@ async def test_returns_metadata(self, provider, root_provider_fixtures): class TestOperations: - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_can_duplicate_names(self, provider): + def test_will_self_overwrite(self, provider, other_provider): + src_path = WaterButlerPath('/50 shades of nope.txt', + _ids=(provider.folder, '12231')) + dest_path = WaterButlerPath('/50 shades of nope2223.txt', + _ids=(provider.folder, '2342sdfsd')) + + result = provider.will_self_overwrite(other_provider, src_path, dest_path) + assert result is False + + result = provider.will_self_overwrite(other_provider, src_path, src_path) + assert result is True + + def test_can_duplicate_names(self, provider): assert provider.can_duplicate_names() is False - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_shares_storage_root(self, provider, other_provider): + def test_shares_storage_root(self, provider, other_provider): assert provider.shares_storage_root(other_provider) is False assert provider.shares_storage_root(provider) is True - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_can_intra_move(self, provider, other_provider): + def test_can_intra_move(self, provider, other_provider): assert provider.can_intra_move(other_provider) is False assert provider.can_intra_move(provider) is True - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_can_intra_copy(self, provider, other_provider): + def test_can_intra_copy(self, provider, other_provider): assert provider.can_intra_copy(other_provider) is False assert provider.can_intra_copy(provider) is True diff --git a/tests/providers/dropbox/test_provider.py b/tests/providers/dropbox/test_provider.py index 582f80d11..649853eda 100644 --- a/tests/providers/dropbox/test_provider.py +++ b/tests/providers/dropbox/test_provider.py @@ -290,8 +290,12 @@ async def test_folder_with_subdirectory_metadata(self, provider, root_provider_f path = await provider.validate_path('/') url = provider.build_url('files', 'list_folder') data = {'path': path.full_path} - aiohttpretty.register_json_uri('POST', url, data=data, - body=root_provider_fixtures['folder_with_subdirectory_metadata']) + aiohttpretty.register_json_uri( + 'POST', + url, + data=data, + body=root_provider_fixtures['folder_with_subdirectory_metadata'] + ) result = await provider.metadata(path) assert isinstance(result, list) @@ -308,8 +312,12 @@ async def test_folder_with_hasmore_metadata(self, provider, root_provider_fixtur data = {'path': path.full_path} aiohttpretty.register_json_uri('POST', url, data=data, body=root_provider_fixtures['folder_with_hasmore_metadata']) - aiohttpretty.register_json_uri('POST', url + '/continue', data=data, - body=root_provider_fixtures['folder_with_subdirectory_metadata']) + aiohttpretty.register_json_uri( + 'POST', + url + '/continue', + data=data, + body=root_provider_fixtures['folder_with_subdirectory_metadata'] + ) result = await provider.metadata(path) @@ -544,7 +552,9 @@ async def test_intra_copy_replace_file(self, provider, root_provider_fixtures, e { 'headers': {'Content-Type': 'application/json'}, 'data': data, - 'body': json.dumps(error_fixtures['rename_conflict_folder_metadata']).encode('utf-8'), + 'body': json.dumps( + error_fixtures['rename_conflict_folder_metadata'] + ).encode('utf-8'), 'status': 409 }, { @@ -574,8 +584,12 @@ async def test_intra_copy_file_different_provider(self, provider, other_provider url1 = provider.build_url('files', 'copy_reference', 'save') data1 = {'copy_reference': 'test', 'path': dest_path.full_path.rstrip('/')} - aiohttpretty.register_json_uri('POST', url1, data=data1, - body=intra_copy_fixtures['intra_copy_other_provider_file_metadata']) + aiohttpretty.register_json_uri( + 'POST', + url1, + data=data1, + body=intra_copy_fixtures['intra_copy_other_provider_file_metadata'] + ) result = await provider.intra_copy(other_provider, src_path, dest_path) expected = (DropboxFileMetadata( @@ -648,7 +662,9 @@ async def test_intra_move_replace_file(self, provider, root_provider_fixtures, e { 'headers': {'Content-Type': 'application/json'}, 'data': data, - 'body': json.dumps(error_fixtures['rename_conflict_file_metadata']).encode('utf-8'), + 'body': json.dumps( + error_fixtures['rename_conflict_file_metadata'] + ).encode('utf-8'), 'status': 409 }, { @@ -689,7 +705,9 @@ async def test_intra_move_replace_folder(self, provider, root_provider_fixtures, { 'headers': {'Content-Type': 'application/json'}, 'data': data, - 'body': json.dumps(error_fixtures['rename_conflict_folder_metadata']).encode('utf-8'), + 'body': json.dumps( + error_fixtures['rename_conflict_folder_metadata'] + ).encode('utf-8'), 'status': 409 }, { @@ -719,23 +737,20 @@ async def test_intra_move_casing_change(self, provider): assert e.value.code == 400 - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_intra_overwrite_error(self, provider): - src_path = WaterButlerPath('/pfile.txt', prepend=provider.folder) - with pytest.raises(exceptions.IntraCopyError) as e: - await provider.intra_move(provider, src_path, src_path) - - assert e.value.code == 409 - - with pytest.raises(exceptions.IntraCopyError) as e: - await provider.intra_copy(provider, src_path, src_path) +class TestOperations: - assert e.value.code == 409 + def test_will_self_overwrite(self, provider, other_provider): + src_path = WaterButlerPath('/50 shades of nope.txt', + _ids=(provider.folder, '12231')) + dest_path = WaterButlerPath('/50 shades of nope2223.txt', + _ids=(provider.folder, '2342sdfsd')) + result = provider.will_self_overwrite(other_provider, src_path, dest_path) + assert result is False -class TestOperations: + result = provider.will_self_overwrite(other_provider, src_path, src_path) + assert result is True def test_can_intra_copy(self, provider): assert provider.can_intra_copy(provider) @@ -747,7 +762,7 @@ def test_can_intra_move(self, provider): assert provider.can_intra_move(provider) def test_cannot_intra_move_other(self, provider, other_provider): - assert provider.can_intra_move(other_provider) == False + assert provider.can_intra_move(other_provider) is False def test_conflict_error_handler_not_found(self, provider, error_fixtures): error_path = '/Photos/folder/file' diff --git a/tests/providers/googledrive/test_provider.py b/tests/providers/googledrive/test_provider.py index 061d11b6e..eeee379c1 100644 --- a/tests/providers/googledrive/test_provider.py +++ b/tests/providers/googledrive/test_provider.py @@ -1573,51 +1573,36 @@ async def test_intra_copy_file(self, provider, root_provider_fixtures): assert result == expected assert aiohttpretty.has_call(method='PUT', uri=delete_url) - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_intra_copy_move_overwrite_error(self, provider, root_provider_fixtures): - item = root_provider_fixtures['docs_file_metadata'] - src_path = WaterButlerPath('/unsure.txt', _ids=('0', item['id'])) - - with pytest.raises(exceptions.IntraCopyError) as e: - await provider.intra_copy(provider, src_path, src_path) - assert e.value.code == 409 - with pytest.raises(exceptions.IntraCopyError) as e: - await provider.intra_move(provider, src_path, src_path) +class TestOperationsOrMisc: - assert e.value.code == 409 + def test_will_self_overwrite(self, provider, other_provider): + src_path = GoogleDrivePath('/root/Gear1.stl', _ids=['0', '10', '11']) + dest_path = GoogleDrivePath('/root/Gear23123.stl', _ids=['0', '10', '12']) + result = provider.will_self_overwrite(other_provider, src_path, dest_path) + assert result is False -class TestOperationsOrMisc: + result = provider.will_self_overwrite(other_provider, src_path, src_path) + assert result is True - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_can_duplicate_names(self, provider): + def test_can_duplicate_names(self, provider): assert provider.can_duplicate_names() is True - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_shares_storage_root(self, provider, other_provider): + def test_shares_storage_root(self, provider, other_provider): assert provider.shares_storage_root(other_provider) is True assert provider.shares_storage_root(provider) is True - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_can_intra_move(self, provider, other_provider): + def test_can_intra_move(self, provider, other_provider): assert provider.can_intra_move(other_provider) is False assert provider.can_intra_move(provider) is True - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test__serialize_item_raw(self, provider, root_provider_fixtures): + def test__serialize_item_raw(self, provider, root_provider_fixtures): item = root_provider_fixtures['docs_file_metadata'] assert provider._serialize_item(None, item, True) == item - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_can_intra_copy(self, provider, other_provider, root_provider_fixtures): + def test_can_intra_copy(self, provider, other_provider, root_provider_fixtures): item = root_provider_fixtures['list_file']['items'][0] path = WaterButlerPath('/birdie.jpg', _ids=(provider.folder['id'], item['id'])) @@ -1654,7 +1639,7 @@ async def test_revalidate_path_file_error(self, provider, root_provider_fixtures body=error_fixtures['parts_file_missing_metadata']) with pytest.raises(exceptions.MetadataError) as e: - result = await provider._resolve_path_to_ids(file_name) + await provider._resolve_path_to_ids(file_name) assert e.value.message == '{} not found'.format(str(path)) assert e.value.code == 404 diff --git a/tests/providers/owncloud/test_provider.py b/tests/providers/owncloud/test_provider.py index c4e8ac521..e9f3ffce4 100644 --- a/tests/providers/owncloud/test_provider.py +++ b/tests/providers/owncloud/test_provider.py @@ -36,6 +36,7 @@ def file_like(file_content): return io.BytesIO(file_content) + @pytest.fixture def file_stream(file_like): return streams.FileStreamReader(file_like) @@ -321,6 +322,7 @@ async def test_intra_copy_file(self, provider, file_metadata): assert metadata.name == 'dissertation.aux' assert metadata.kind == 'file' + class TestMetadata: @pytest.mark.asyncio @@ -359,6 +361,18 @@ async def test_revisions(self, provider, file_metadata): class TestOperations: + def test_will_self_overwrite(self, provider): + src_path = WaterButlerPath('/50 shades of nope.txt', + _ids=(provider.folder, '12231')) + dest_path = WaterButlerPath('/50 shades of nope2223.txt', + _ids=(provider.folder, '2342sdfsd')) + + result = provider.will_self_overwrite(provider, src_path, dest_path) + assert result is False + + result = provider.will_self_overwrite(provider, src_path, src_path) + assert result is True + def test_can_intra_copy(self, provider, provider_different_credentials): assert provider.can_intra_copy(provider) assert not provider.can_intra_copy(provider_different_credentials) diff --git a/waterbutler/core/provider.py b/waterbutler/core/provider.py index ba443e33f..789349e44 100644 --- a/waterbutler/core/provider.py +++ b/waterbutler/core/provider.py @@ -224,6 +224,18 @@ async def move(self, }) if handle_naming: + # In handle_naming so we don't run it multiple times + # This does not mean the `dest_path` is a folder, at this point it could just be the parent + # of the actual dest_path, or have a blank name + if not dest_path.is_file: + temp_path = await self.revalidate_path( + dest_path, + rename or src_path.name, + folder=src_path.is_dir + ) + if self.will_self_overwrite(dest_provider, src_path, temp_path): + raise exceptions.OverwriteSelfError(src_path) + dest_path = await dest_provider.handle_naming( src_path, dest_path, @@ -269,7 +281,29 @@ async def copy(self, 'conflict': conflict, 'got_rename': rename is not None, }) + + if not dest_path.is_file: + temp_path = await self.revalidate_path( + dest_path, + rename or src_path.name, + folder=src_path.is_dir + ) + if self.will_self_overwrite(dest_provider, src_path, temp_path): + raise exceptions.OverwriteSelfError(src_path) + if handle_naming: + # In handle_naming so we don't run it multiple times + # This does not mean the `dest_path` is a folder, at this point it could just be the parent + # of the actual dest_path, or have a blank name + if not dest_path.is_file: + temp_path = await self.revalidate_path( + dest_path, + rename or src_path.name, + folder=src_path.is_dir + ) + if self.will_self_overwrite(dest_provider, src_path, temp_path): + raise exceptions.OverwriteSelfError(src_path) + dest_path = await dest_provider.handle_naming( src_path, dest_path, @@ -393,10 +427,15 @@ async def handle_naming(self, :rtype: :class:`.WaterButlerPath` """ + + # This function can either take a file path, or a parent path to make a file path out of. + if src_path.is_dir and dest_path.is_file: # Cant copy a directory to a file raise ValueError('Destination must be a directory if the source is') + # This is confusing. `dest_path` at this point can refer to the parent or root of + # the file we want to move/copy. So even if moving/copying a file, this code will run if not dest_path.is_file: # Directories always are going to be copied into # cp /folder1/ /folder2/ -> /folder1/folder2/ @@ -410,6 +449,22 @@ async def handle_naming(self, return dest_path + def will_self_overwrite(self, + dest_provider: 'BaseProvider', + src_path: wb_path.WaterButlerPath, + dest_path: wb_path.WaterButlerPath) -> bool: + """ Return wether a move or copy operation will result in a self-overwrite. + + .. note:: + Defaults to False + + :param dest_provider: ( :class:`.BaseProvider` ) The provider to check against + :param src_path: ( :class:`.WaterButlerPath` ) The move/copy source path + :param dest_path: ( :class:`.WaterButlerPath` ) The move/copy destination path + :rtype: :class:`bool` + """ + return False + def can_intra_copy(self, other: 'BaseProvider', path: wb_path.WaterButlerPath=None) -> bool: diff --git a/waterbutler/providers/box/provider.py b/waterbutler/providers/box/provider.py index b55aa4779..a8096661b 100644 --- a/waterbutler/providers/box/provider.py +++ b/waterbutler/providers/box/provider.py @@ -182,6 +182,9 @@ def shares_storage_root(self, other: provider.BaseProvider) -> bool: Add a comparison of credentials to avoid this.""" return super().shares_storage_root(other) and self.credentials == other.credentials + def will_self_overwrite(self, dest_provider, src_path, dest_path): + return self.NAME == dest_provider.NAME and src_path.identifier == dest_path.identifier + def can_intra_move(self, other: provider.BaseProvider, path: wb_path.WaterButlerPath=None) -> bool: return self == other @@ -196,10 +199,6 @@ async def intra_copy(self, # type: ignore dest_path: wb_path.WaterButlerPath) \ -> typing.Tuple[typing.Union[BoxFileMetadata, BoxFolderMetadata], bool]: - if src_path.identifier == dest_path.identifier: - raise exceptions.IntraCopyError("Cannot overwrite a file with itself", - code=HTTPStatus.CONFLICT) - if dest_path.identifier is not None: await dest_provider.delete(dest_path) @@ -229,10 +228,6 @@ async def intra_move(self, # type: ignore src_path: wb_path.WaterButlerPath, dest_path: wb_path.WaterButlerPath) -> typing.Tuple[BaseBoxMetadata, bool]: - if src_path.identifier == dest_path.identifier: - raise exceptions.IntraCopyError("Cannot overwrite a file with itself", - code=HTTPStatus.CONFLICT) - if dest_path.identifier is not None and str(dest_path).lower() != str(src_path).lower(): await dest_provider.delete(dest_path) diff --git a/waterbutler/providers/dropbox/provider.py b/waterbutler/providers/dropbox/provider.py index 034f73774..40cdb1435 100644 --- a/waterbutler/providers/dropbox/provider.py +++ b/waterbutler/providers/dropbox/provider.py @@ -152,10 +152,6 @@ async def intra_copy(self, # type: ignore -> typing.Tuple[typing.Union[DropboxFileMetadata, DropboxFolderMetadata], bool]: dest_folder = dest_provider.folder - if dest_path.full_path == src_path.full_path: - raise exceptions.IntraCopyError("Cannot overwrite a file with itself", - code=HTTPStatus.CONFLICT) - try: if self == dest_provider: data = await self.dropbox_request( @@ -198,10 +194,6 @@ async def intra_move(self, # type: ignore src_path: WaterButlerPath, dest_path: WaterButlerPath) -> typing.Tuple[BaseDropboxMetadata, bool]: - if dest_path.full_path == src_path.full_path: - raise exceptions.IntraCopyError("Cannot overwrite a file with itself", - code=HTTPStatus.CONFLICT) - if dest_path.full_path.lower() == src_path.full_path.lower(): # Dropbox does not support changing the casing in a file name raise exceptions.InvalidPathError( @@ -380,6 +372,9 @@ async def create_folder(self, path: WaterButlerPath, **kwargs) -> DropboxFolderM ) return DropboxFolderMetadata(data, self.folder) + def will_self_overwrite(self, dest_provider, src_path, dest_path): + return self.NAME == dest_provider.NAME and dest_path.full_path == src_path.full_path + def can_intra_copy(self, dest_provider: provider.BaseProvider, path: WaterButlerPath=None) -> bool: return type(self) == type(dest_provider) diff --git a/waterbutler/providers/googledrive/provider.py b/waterbutler/providers/googledrive/provider.py index 607dc5e07..92d3b56c2 100644 --- a/waterbutler/providers/googledrive/provider.py +++ b/waterbutler/providers/googledrive/provider.py @@ -135,6 +135,9 @@ def can_duplicate_names(self) -> bool: def default_headers(self) -> dict: return {'authorization': 'Bearer {}'.format(self.token)} + def will_self_overwrite(self, dest_provider, src_path, dest_path): + return self.NAME == dest_provider.NAME and src_path.identifier == dest_path.identifier + def can_intra_move(self, other: provider.BaseProvider, path=None) -> bool: @@ -152,10 +155,6 @@ async def intra_move(self, # type: ignore dest_path: wb_path.WaterButlerPath) \ -> typing.Tuple[BaseGoogleDriveMetadata, bool]: - if src_path.identifier == dest_path.identifier: - raise exceptions.IntraCopyError("Cannot overwrite a file with itself", - code=HTTPStatus.CONFLICT) - self.metrics.add('intra_move.destination_exists', dest_path.identifier is not None) if dest_path.identifier: await dest_provider.delete(dest_path) @@ -193,10 +192,6 @@ async def intra_copy(self, dest_path: wb_path.WaterButlerPath) \ -> typing.Tuple[GoogleDriveFileMetadata, bool]: - if src_path.identifier == dest_path.identifier: - raise exceptions.IntraCopyError("Cannot overwrite a file with itself", - code=HTTPStatus.CONFLICT) - self.metrics.add('intra_copy.destination_exists', dest_path.identifier is not None) if dest_path.identifier: await dest_provider.delete(dest_path) diff --git a/waterbutler/providers/owncloud/provider.py b/waterbutler/providers/owncloud/provider.py index 81947dcf9..d183129c1 100644 --- a/waterbutler/providers/owncloud/provider.py +++ b/waterbutler/providers/owncloud/provider.py @@ -1,5 +1,4 @@ import aiohttp -from http import HTTPStatus from waterbutler.core import streams from waterbutler.core import provider @@ -267,6 +266,9 @@ async def create_folder(self, path, **kwargs): def can_duplicate_names(self): return True + def will_self_overwrite(self, dest_provider, src_path, dest_path): + return self.NAME == dest_provider.NAME and src_path.identifier == dest_path.identifier + def can_intra_copy(self, dest_provider, path=None): return self == dest_provider @@ -292,10 +294,6 @@ async def _do_dav_move_copy(self, src_path, dest_path, operation): if operation != 'MOVE' and operation != 'COPY': raise NotImplementedError("ownCloud move/copy only supports MOVE and COPY endpoints") - if src_path.full_path == dest_path.full_path: - raise exceptions.IntraCopyError("Cannot overwrite a file with itself", - code=HTTPStatus.CONFLICT) - resp = await self.make_request( operation, self._webdav_url_ + src_path.full_path, From c0fc043f3738d0de1bed838fa31141a434c68f59 Mon Sep 17 00:00:00 2001 From: Addison Schiller Date: Wed, 29 Nov 2017 11:22:39 -0500 Subject: [PATCH 3/5] Responding to CR --- waterbutler/core/provider.py | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/waterbutler/core/provider.py b/waterbutler/core/provider.py index 789349e44..b2cbe7dea 100644 --- a/waterbutler/core/provider.py +++ b/waterbutler/core/provider.py @@ -223,18 +223,18 @@ async def move(self, 'got_rename': rename is not None, }) + # This does not mean the `dest_path` is a folder, at this point it could just be the parent + # of the actual dest_path, or have a blank name + if not dest_path.is_file: + temp_path = await self.revalidate_path( + dest_path, + rename or src_path.name, + folder=src_path.is_dir + ) + if self.will_self_overwrite(dest_provider, src_path, temp_path): + raise exceptions.OverwriteSelfError(src_path) + if handle_naming: - # In handle_naming so we don't run it multiple times - # This does not mean the `dest_path` is a folder, at this point it could just be the parent - # of the actual dest_path, or have a blank name - if not dest_path.is_file: - temp_path = await self.revalidate_path( - dest_path, - rename or src_path.name, - folder=src_path.is_dir - ) - if self.will_self_overwrite(dest_provider, src_path, temp_path): - raise exceptions.OverwriteSelfError(src_path) dest_path = await dest_provider.handle_naming( src_path, @@ -282,6 +282,8 @@ async def copy(self, 'got_rename': rename is not None, }) + # This does not mean the `dest_path` is a folder, at this point it could just be the parent + # of the actual dest_path, or have a blank name if not dest_path.is_file: temp_path = await self.revalidate_path( dest_path, @@ -292,17 +294,6 @@ async def copy(self, raise exceptions.OverwriteSelfError(src_path) if handle_naming: - # In handle_naming so we don't run it multiple times - # This does not mean the `dest_path` is a folder, at this point it could just be the parent - # of the actual dest_path, or have a blank name - if not dest_path.is_file: - temp_path = await self.revalidate_path( - dest_path, - rename or src_path.name, - folder=src_path.is_dir - ) - if self.will_self_overwrite(dest_provider, src_path, temp_path): - raise exceptions.OverwriteSelfError(src_path) dest_path = await dest_provider.handle_naming( src_path, @@ -457,6 +448,7 @@ def will_self_overwrite(self, .. note:: Defaults to False + Overridden by providers that need to run this check :param dest_provider: ( :class:`.BaseProvider` ) The provider to check against :param src_path: ( :class:`.WaterButlerPath` ) The move/copy source path From 0162781af66a705b1e61592f3a9fe1d967ee2f5d Mon Sep 17 00:00:00 2001 From: TomBaxter Date: Mon, 25 Sep 2017 15:54:07 -0400 Subject: [PATCH 4/5] Raise exception for copy/move replace folder that orphans itself. [#SVCS-479] Replace folder from inside folder deletes both Add replace_will_orphan(src_path,dest_path) Checks if dest_path overwrite will orphan src_path Call replace_will_orphan in copy/move defs in waterbutler.core.provider --- tests/core/test_provider.py | 57 +++++++++++++++++++++ tests/providers/osfstorage/test_provider.py | 1 - waterbutler/core/exceptions.py | 9 ++++ waterbutler/core/provider.py | 40 ++++++++++++++- 4 files changed, 104 insertions(+), 3 deletions(-) diff --git a/tests/core/test_provider.py b/tests/core/test_provider.py index d06386560..a41bea98a 100644 --- a/tests/core/test_provider.py +++ b/tests/core/test_provider.py @@ -4,6 +4,7 @@ from unittest import mock from waterbutler.core import metadata from waterbutler.core import exceptions +from waterbutler.core.path import WaterButlerPath @pytest.fixture @@ -37,6 +38,36 @@ def test_cant_intra_move(self, provider1): def test_can_intra_move(self, provider2): assert provider2.can_intra_move(provider2) is True + @pytest.mark.asyncio + async def test_will_orphan_dest_is_file(self, provider1): + src_path = WaterButlerPath('/folder1/folder1/', + _ids=['root', 'folder', 'Folder'], + folder=True) + dest_path = WaterButlerPath('/folder1/', + _ids=['root','folder'], + folder=False) + assert provider1.replace_will_orphan(src_path, dest_path) == False + + @pytest.mark.asyncio + async def test_will_orphan_dest_different_names(self, provider1): + src_path = WaterButlerPath('/folder1/folder1/', + _ids=['root', 'folder', 'Folder'], + folder=True) + dest_path = WaterButlerPath('/folder2/', + _ids=['root','folder'], + folder=True) + assert provider1.replace_will_orphan(src_path, dest_path) == False + + @pytest.mark.asyncio + async def test_will_orphan_dest_different_branch(self, provider1): + src_path = WaterButlerPath('/other_folder/folder1/', + _ids=['root', 'other_folder', 'Folder'], + folder=True) + dest_path = WaterButlerPath('/folder1/', + _ids=['root','folder'], + folder=True) + assert provider1.replace_will_orphan(src_path, dest_path) == False + @pytest.mark.asyncio async def test_exists(self, provider1): ret = await provider1.exists('somepath') @@ -309,6 +340,19 @@ async def test_calls_intra_copy(self, provider1): provider1.can_intra_copy.assert_called_once_with(provider1, src_path) provider1.intra_copy.assert_called_once_with(provider1, src_path, dest_path) + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_intra_copy_folder_orphan(self, provider1): + src_path = await provider1.validate_path('/folder1/folder1/') + dest_path = await provider1.validate_path('/') + + provider1.can_intra_copy = mock.Mock(return_value=True) + + with pytest.raises(exceptions.OrphanSelfError) as exc: + await provider1.copy(provider1, src_path, dest_path) + assert exc.value.code == 400 + assert exc.typename == 'OrphanSelfError' + @pytest.mark.asyncio async def test_calls_folder_op_on_dir(self, provider1): src_path = await provider1.validate_path('/source/path/') @@ -435,6 +479,19 @@ async def test_calls_intra_move(self, provider1): provider1.can_intra_move.assert_called_once_with(provider1, src_path) provider1.intra_move.assert_called_once_with(provider1, src_path, dest_path) + @pytest.mark.asyncio + @pytest.mark.aiohttpretty + async def test_intra_move_folder_orphan(self, provider1): + src_path = await provider1.validate_path('/folder1/folder1/') + dest_path = await provider1.validate_path('/') + + provider1.can_intra_move = mock.Mock(return_value=True) + + with pytest.raises(exceptions.OrphanSelfError) as exc: + await provider1.move(provider1, src_path, dest_path) + assert exc.value.code == 400 + assert exc.typename == 'OrphanSelfError' + @pytest.mark.asyncio async def test_calls_folder_op_on_dir_and_delete(self, provider1): src_path = await provider1.validate_path('/source/path/') diff --git a/tests/providers/osfstorage/test_provider.py b/tests/providers/osfstorage/test_provider.py index e0e6d52da..5fa100beb 100644 --- a/tests/providers/osfstorage/test_provider.py +++ b/tests/providers/osfstorage/test_provider.py @@ -341,7 +341,6 @@ async def test_intra_copy_folder(self, provider_and_mock, provider_and_mock2, src_mock._children_metadata.assert_not_called() src_mock.validate_v1_path.assert_not_called() - @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_intra_copy_file(self, provider_and_mock, provider_and_mock2, diff --git a/waterbutler/core/exceptions.py b/waterbutler/core/exceptions.py index 24c5f76c6..339b44053 100644 --- a/waterbutler/core/exceptions.py +++ b/waterbutler/core/exceptions.py @@ -208,6 +208,15 @@ def __init__(self, path): 'folder onto itself is not supported.'.format(path)) +class OrphanSelfError(InvalidParameters): + """Error for conflict=replace sent to methods copy or move, when action would result + in removal of a directory containing the file or folder to be copied or moved. + """ + def __init__(self, path): + super().__init__('Unable to move or copy \'{}\'. Moving or copying a folder onto parent ' + 'folder of same name with "replace" is not supported.'.format(path)) + + class UnsupportedOperationError(ProviderError): def __init__(self, message, code=HTTPStatus.FORBIDDEN, is_user_error=True): if not message: diff --git a/waterbutler/core/provider.py b/waterbutler/core/provider.py index b2cbe7dea..c1d23e49c 100644 --- a/waterbutler/core/provider.py +++ b/waterbutler/core/provider.py @@ -207,12 +207,12 @@ async def move(self, Performs a copy and then a delete. Calls :func:`BaseProvider.intra_move` if possible. - :param dest_provider: ( :class:`.BaseProvider` ) The provider to move to + :param dest_provider: ( :class:`BaseProvider` ) The provider to move to :param src_path: ( :class:`.WaterButlerPath` ) Path to where the resource can be found :param dest_path: ( :class:`.WaterButlerPath` ) Path to where the resource will be moved :param rename: ( :class:`str` ) The desired name of the resulting path, may be incremented :param conflict: ( :class:`str` ) What to do in the event of a name conflict, ``replace`` or ``keep`` - :param handle_naming: ( :class:`bool` ) If a naming conflict is detected, should it be automatically handled? + :param handle_naming: ( :class:`bool` ) If true will be run through handle_naming method, and handled in accordance with conflict parameter. """ args = (dest_provider, src_path, dest_path) kwargs = {'rename': rename, 'conflict': conflict} @@ -252,6 +252,13 @@ async def move(self, ): raise exceptions.OverwriteSelfError(src_path) + if ( + self == dest_provider and + conflict == 'replace' and + dest_provider.replace_will_orphan(src_path, dest_path) + ): + raise exceptions.OrphanSelfError(src_path) + self.provider_metrics.add('move.can_intra_move', False) if self.can_intra_move(dest_provider, src_path): self.provider_metrics.add('move.can_intra_move', True) @@ -311,6 +318,13 @@ async def copy(self, ): raise exceptions.OverwriteSelfError(src_path) + if ( + self == dest_provider and + conflict == 'replace' and + dest_provider.replace_will_orphan(src_path, dest_path) + ): + raise exceptions.OrphanSelfError(src_path) + self.provider_metrics.add('copy.can_intra_copy', False) if self.can_intra_copy(dest_provider, src_path): self.provider_metrics.add('copy.can_intra_copy', True) @@ -523,6 +537,28 @@ async def intra_move(self, await self.delete(src_path) return data, created + def replace_will_orphan(self, + src_path: wb_path.WaterButlerPath, + dest_path: wb_path.WaterButlerPath) -> bool: + """Check if copy/move conflict=replace will orphan src_path. + + Assumes src_provider == dest_provider and conflict == 'replace' have already been checked. + This can be an expensive operation. Should be used as last resort. Should be overridden if + provider has a cheaper option. + """ + if not dest_path.kind == 'folder': + return False + if src_path.name != dest_path.name: + return False + + incr_path = src_path + while incr_path.materialized_path != '/': + incr_path = incr_path.parent + if incr_path.materialized_path == dest_path.materialized_path: + return True + + return False + async def exists(self, path: wb_path.WaterButlerPath, **kwargs) \ -> typing.Union[bool, wb_metadata.BaseMetadata, typing.List[wb_metadata.BaseMetadata]]: """Check for existence of WaterButlerPath From 4f985e39e7470d15f67cdce9cf59efae2977cc7e Mon Sep 17 00:00:00 2001 From: TomBaxter Date: Tue, 19 Dec 2017 13:57:52 -0500 Subject: [PATCH 5/5] Refactor overwrite conflict logic SVCS-479 Move three pieces of overwrite conflict logic from move/copy into handle_naming and rename handle_naming to handle_conflict. Improved logic and reduced code. --- tests/core/test_provider.py | 65 +++++++++---------- waterbutler/core/provider.py | 121 +++++++++++++++-------------------- 2 files changed, 79 insertions(+), 107 deletions(-) diff --git a/tests/core/test_provider.py b/tests/core/test_provider.py index a41bea98a..7717d1e84 100644 --- a/tests/core/test_provider.py +++ b/tests/core/test_provider.py @@ -203,7 +203,7 @@ async def test_no_problem(self, provider1): dest_path = await provider1.validate_path('/test/path/') provider1.exists = utils.MockCoroutine(return_value=False) - handled = await provider1.handle_naming(src_path, dest_path) + handled = await provider1.handle_conflicts(provider1, src_path, dest_path) assert handled == src_path.child('path', folder=True) assert handled.is_dir is True @@ -216,7 +216,7 @@ async def test_rename_via_path(self, provider1): dest_path = await provider1.validate_path('/test/name2') provider1.exists = utils.MockCoroutine(return_value=False) - handled = await provider1.handle_naming(src_path, dest_path) + handled = await provider1.handle_conflicts(provider1, src_path, dest_path) assert handled.name == 'name2' assert handled.is_file is True @@ -227,24 +227,11 @@ async def test_rename_explicit(self, provider1): src_path = await provider1.validate_path('/test/name1') provider1.exists = utils.MockCoroutine(return_value=False) - handled = await provider1.handle_naming(src_path, dest_path, rename='name2') + handled = await provider1.handle_conflicts(provider1, src_path, dest_path, rename='name2') assert handled.name == 'name2' assert handled.is_file is True - @pytest.mark.asyncio - async def test_no_problem_file(self, provider1): - src_path = await provider1.validate_path('/test/path') - dest_path = await provider1.validate_path('/test/path') - provider1.exists = utils.MockCoroutine(return_value=False) - - handled = await provider1.handle_naming(src_path, dest_path) - - assert handled == dest_path # == not is - assert handled.is_file is True - assert len(handled.parts) == 3 # Includes root - assert handled.name == 'path' - class TestCopy: @@ -253,22 +240,23 @@ async def test_handles_naming_false(self, provider1): src_path = await provider1.validate_path('/source/path') dest_path = await provider1.validate_path('/destination/path') - provider1.handle_naming = utils.MockCoroutine() + provider1.handle_conflicts = utils.MockCoroutine() - await provider1.copy(provider1, src_path, dest_path, handle_naming=False) + await provider1.copy(provider1, src_path, dest_path, handle_conflicts=False) - assert provider1.handle_naming.called is False + assert provider1.handle_conflicts.called is False @pytest.mark.asyncio async def test_handles_naming(self, provider1): src_path = await provider1.validate_path('/source/path') dest_path = await provider1.validate_path('/destination/path') - provider1.handle_naming = utils.MockCoroutine() + provider1.handle_conflicts = utils.MockCoroutine() await provider1.copy(provider1, src_path, dest_path) - provider1.handle_naming.assert_called_once_with( + provider1.handle_conflicts.assert_called_once_with( + provider1, src_path, dest_path, rename=None, @@ -280,11 +268,12 @@ async def test_passes_on_conflict(self, provider1): src_path = await provider1.validate_path('/source/path') dest_path = await provider1.validate_path('/destination/path') - provider1.handle_naming = utils.MockCoroutine() + provider1.handle_conflicts = utils.MockCoroutine() await provider1.copy(provider1, src_path, dest_path, conflict='keep') - provider1.handle_naming.assert_called_once_with( + provider1.handle_conflicts.assert_called_once_with( + provider1, src_path, dest_path, rename=None, @@ -296,11 +285,12 @@ async def test_passes_on_rename(self, provider1): src_path = await provider1.validate_path('/source/path') dest_path = await provider1.validate_path('/destination/path') - provider1.handle_naming = utils.MockCoroutine() + provider1.handle_conflicts = utils.MockCoroutine() await provider1.copy(provider1, src_path, dest_path, rename='Baz') - provider1.handle_naming.assert_called_once_with( + provider1.handle_conflicts.assert_called_once_with( + provider1, src_path, dest_path, rename='Baz', @@ -392,22 +382,23 @@ async def test_handles_naming_false(self, provider1): src_path = await provider1.validate_path('/source/path') dest_path = await provider1.validate_path('/destination/path') - provider1.handle_naming = utils.MockCoroutine() + provider1.handle_conflicts = utils.MockCoroutine() - await provider1.move(provider1, src_path, dest_path, handle_naming=False) + await provider1.move(provider1, src_path, dest_path, handle_conflicts=False) - assert provider1.handle_naming.called is False + assert provider1.handle_conflicts.called is False @pytest.mark.asyncio async def test_handles_naming(self, provider1): src_path = await provider1.validate_path('/source/path') dest_path = await provider1.validate_path('/destination/path') - provider1.handle_naming = utils.MockCoroutine() + provider1.handle_conflicts = utils.MockCoroutine() await provider1.move(provider1, src_path, dest_path) - provider1.handle_naming.assert_called_once_with( + provider1.handle_conflicts.assert_called_once_with( + provider1, src_path, dest_path, rename=None, @@ -419,11 +410,12 @@ async def test_passes_on_conflict(self, provider1): src_path = await provider1.validate_path('/source/path') dest_path = await provider1.validate_path('/destination/path') - provider1.handle_naming = utils.MockCoroutine() + provider1.handle_conflicts = utils.MockCoroutine() await provider1.move(provider1, src_path, dest_path, conflict='keep') - provider1.handle_naming.assert_called_once_with( + provider1.handle_conflicts.assert_called_once_with( + provider1, src_path, dest_path, rename=None, @@ -435,11 +427,12 @@ async def test_passes_on_rename(self, provider1): src_path = await provider1.validate_path('/source/path') dest_path = await provider1.validate_path('/destination/path') - provider1.handle_naming = utils.MockCoroutine() + provider1.handle_conflicts = utils.MockCoroutine() await provider1.move(provider1, src_path, dest_path, rename='Baz') - provider1.handle_naming.assert_called_once_with( + provider1.handle_conflicts.assert_called_once_with( + provider1, src_path, dest_path, rename='Baz', @@ -528,7 +521,7 @@ async def test_calls_copy_and_delete(self, provider1): provider1, src_path, dest_path, - handle_naming=False + handle_conflicts=False ) @pytest.mark.asyncio @@ -548,7 +541,7 @@ async def test_no_delete_on_copy_error(self, provider1): provider1, src_path, dest_path, - handle_naming=False + handle_conflicts=False ) def test_build_range_header(self, provider1): diff --git a/waterbutler/core/provider.py b/waterbutler/core/provider.py index c1d23e49c..62f7b9953 100644 --- a/waterbutler/core/provider.py +++ b/waterbutler/core/provider.py @@ -202,7 +202,7 @@ async def move(self, dest_path: wb_path.WaterButlerPath, rename: str=None, conflict: str='replace', - handle_naming: bool=True) -> typing.Tuple[wb_metadata.BaseMetadata, bool]: + handle_conflicts: bool=True) -> typing.Tuple[wb_metadata.BaseMetadata, bool]: """Moves a file or folder from the current provider to the specified one Performs a copy and then a delete. Calls :func:`BaseProvider.intra_move` if possible. @@ -212,31 +212,21 @@ async def move(self, :param dest_path: ( :class:`.WaterButlerPath` ) Path to where the resource will be moved :param rename: ( :class:`str` ) The desired name of the resulting path, may be incremented :param conflict: ( :class:`str` ) What to do in the event of a name conflict, ``replace`` or ``keep`` - :param handle_naming: ( :class:`bool` ) If true will be run through handle_naming method, and handled in accordance with conflict parameter. + :param handle_conflicts: ( :class:`bool` ) If true will be run through handle_conflicts method. """ args = (dest_provider, src_path, dest_path) kwargs = {'rename': rename, 'conflict': conflict} self.provider_metrics.add('move', { - 'got_handle_naming': handle_naming, + 'got_handle_conflicts': handle_conflicts, 'conflict': conflict, 'got_rename': rename is not None, }) - # This does not mean the `dest_path` is a folder, at this point it could just be the parent - # of the actual dest_path, or have a blank name - if not dest_path.is_file: - temp_path = await self.revalidate_path( - dest_path, - rename or src_path.name, - folder=src_path.is_dir - ) - if self.will_self_overwrite(dest_provider, src_path, temp_path): - raise exceptions.OverwriteSelfError(src_path) - - if handle_naming: + if handle_conflicts: - dest_path = await dest_provider.handle_naming( + dest_path = await dest_provider.handle_conflicts( + self, src_path, dest_path, rename=rename, @@ -245,20 +235,6 @@ async def move(self, args = (dest_provider, src_path, dest_path) kwargs = {} - # files and folders shouldn't overwrite themselves - if ( - self.shares_storage_root(dest_provider) and - src_path.materialized_path == dest_path.materialized_path - ): - raise exceptions.OverwriteSelfError(src_path) - - if ( - self == dest_provider and - conflict == 'replace' and - dest_provider.replace_will_orphan(src_path, dest_path) - ): - raise exceptions.OrphanSelfError(src_path) - self.provider_metrics.add('move.can_intra_move', False) if self.can_intra_move(dest_provider, src_path): self.provider_metrics.add('move.can_intra_move', True) @@ -267,7 +243,8 @@ async def move(self, if src_path.is_dir: meta_data, created = await self._folder_file_op(self.move, *args, **kwargs) # type: ignore else: - meta_data, created = await self.copy(*args, handle_naming=False, **kwargs) # type: ignore + # handle_conflicts false because we've already been through handle_conflicts once. + meta_data, created = await self.copy(*args, handle_conflicts=False, **kwargs) # type: ignore await self.delete(src_path) @@ -278,31 +255,21 @@ async def copy(self, src_path: wb_path.WaterButlerPath, dest_path: wb_path.WaterButlerPath, rename: str=None, conflict: str='replace', - handle_naming: bool=True) \ + handle_conflicts: bool=True) \ -> typing.Tuple[wb_metadata.BaseMetadata, bool]: args = (dest_provider, src_path, dest_path) - kwargs = {'rename': rename, 'conflict': conflict, 'handle_naming': handle_naming} + kwargs = {'rename': rename, 'conflict': conflict, 'handle_conflicts': handle_conflicts} self.provider_metrics.add('copy', { - 'got_handle_naming': handle_naming, + 'got_handle_conflicts': handle_conflicts, 'conflict': conflict, 'got_rename': rename is not None, }) - # This does not mean the `dest_path` is a folder, at this point it could just be the parent - # of the actual dest_path, or have a blank name - if not dest_path.is_file: - temp_path = await self.revalidate_path( - dest_path, - rename or src_path.name, - folder=src_path.is_dir - ) - if self.will_self_overwrite(dest_provider, src_path, temp_path): - raise exceptions.OverwriteSelfError(src_path) - - if handle_naming: + if handle_conflicts: - dest_path = await dest_provider.handle_naming( + dest_path = await dest_provider.handle_conflicts( + self, src_path, dest_path, rename=rename, @@ -311,20 +278,6 @@ async def copy(self, args = (dest_provider, src_path, dest_path) kwargs = {} - # files and folders shouldn't overwrite themselves - if ( - self.shares_storage_root(dest_provider) and - src_path.materialized_path == dest_path.materialized_path - ): - raise exceptions.OverwriteSelfError(src_path) - - if ( - self == dest_provider and - conflict == 'replace' and - dest_provider.replace_will_orphan(src_path, dest_path) - ): - raise exceptions.OrphanSelfError(src_path) - self.provider_metrics.add('copy.can_intra_copy', False) if self.can_intra_copy(dest_provider, src_path): self.provider_metrics.add('copy.can_intra_copy', True) @@ -389,8 +342,13 @@ async def _folder_file_op(self, dest_provider, # TODO figure out a way to cut down on all the requests made here (await self.revalidate_path(src_path, item.name, folder=item.is_folder)), + # I don't think dest_provider needs to be revalidated at this point. + # 'def move' and 'def copy are the only methods calling _folder_file_op + # both move and copy already call handle_conflicts + # which is why the next line down sets handle_conflicts=False + # which in turn already calls dest_provider.revalidate_path (await dest_provider.revalidate_path(dest_path, item.name, folder=item.is_folder)), - handle_naming=False, + handle_conflicts=False, ) )) @@ -407,11 +365,12 @@ async def _folder_file_op(self, return folder, created - async def handle_naming(self, - src_path: wb_path.WaterButlerPath, - dest_path: wb_path.WaterButlerPath, - rename: str=None, - conflict: str='replace') -> wb_path.WaterButlerPath: + async def handle_conflicts(self, + src_provider, + src_path: wb_path.WaterButlerPath, + dest_path: wb_path.WaterButlerPath, + rename: str=None, + conflict: str='replace') -> wb_path.WaterButlerPath: """Given a :class:`.WaterButlerPath` and the desired name, handle any potential naming issues. i.e.: @@ -425,22 +384,24 @@ async def handle_naming(self, cp /file.txt /folder/doc.txt -> /folder/doc.txt + :param src_provider: ( :class:`.BaseProvider` ) The source provider :param src_path: ( :class:`.WaterButlerPath` ) The object that is being copied :param dest_path: ( :class:`.WaterButlerPath` ) The path that is being copied to or into :param rename: ( :class:`str` ) The desired name of the resulting path, may be incremented :param conflict: ( :class:`str` ) The conflict resolution strategy, ``replace`` or ``keep`` :rtype: :class:`.WaterButlerPath` - """ - # This function can either take a file path, or a parent path to make a file path out of. + self is the destination provider + dest_path may be 1. The folder path into which the src will be moved/copied/uploaded + 2. The file path that will be overwritten with new content + 3. None in the case of unvalidated path for ID based provider + """ if src_path.is_dir and dest_path.is_file: # Cant copy a directory to a file raise ValueError('Destination must be a directory if the source is') - # This is confusing. `dest_path` at this point can refer to the parent or root of - # the file we want to move/copy. So even if moving/copying a file, this code will run if not dest_path.is_file: # Directories always are going to be copied into # cp /folder1/ /folder2/ -> /folder1/folder2/ @@ -449,6 +410,24 @@ async def handle_naming(self, rename or src_path.name, folder=src_path.is_dir ) + if conflict != 'keep': + if self.will_self_overwrite(src_provider, src_path, dest_path): + raise exceptions.OverwriteSelfError(src_path) + + # files and folders shouldn't overwrite themselves + # TODO: how is this different from above will_self_overwrite call? + if ( + self.shares_storage_root(src_provider) and + src_path.materialized_path == dest_path.materialized_path + ): + raise exceptions.OverwriteSelfError(src_path) + + if ( + self == src_provider and + conflict == 'replace' and + self.replace_will_orphan(src_path, dest_path) + ): + raise exceptions.OrphanSelfError(src_path) dest_path, _ = await self.handle_name_conflict(dest_path, conflict=conflict)