[SVCS-479] Raise exception for copy/move replace folder that orphans itself. #274

TomBaxter · 2017-09-25T20:10:06Z

Purpose:

SVCS-479
Replace folder from inside folder deletes both.

Changes:

Updated waterbutler/core/provider.py
-- Add def replace_will_orphan
-- Update def move calls replace_will_orphan
-- Update def copy calls replace_will_orphan
Update waterbutler/core/exceptions.py
-- Add class OrphanSelfError
Update tests/providers/osfstorage/test_provider.py
-- Add test test_intra_copy_folder_orphan

Side effects

Folder move/copies that use conflict=replace may now be more costly

coveralls · 2017-09-25T20:31:33Z

Coverage increased (+0.03%) to 82.969% when pulling baf91c6 on TomBaxter:feature/svcs-479 into 6058fd0 on CenterForOpenScience:develop.

coveralls · 2017-09-26T13:16:02Z

Coverage increased (+0.03%) to 82.969% when pulling 78e2273 on TomBaxter:feature/svcs-479 into 6058fd0 on CenterForOpenScience:develop.

coveralls · 2017-09-26T13:16:02Z

Coverage increased (+0.03%) to 82.969% when pulling 78e2273 on TomBaxter:feature/svcs-479 into 6058fd0 on CenterForOpenScience:develop.

AddisonSchiller · 2017-10-30T19:34:35Z

waterbutler/core/provider.py

@@ -476,6 +485,25 @@ def can_intra_move(self,
        await self.delete(src_path)
        return data, created

+    async def replace_will_orphan(self,


This function makes no awaits so it shouldn't need to be async

AddisonSchiller · 2017-10-30T20:40:11Z

waterbutler/core/provider.py

+
+        This can be an expensive operation. Should be used as last resort. Should be overridden if
+        provider has a cheaper option.
+        """


Since this can be expensive, is there a possible check you can do here that will quickly tell you if you need to go down the whole chain, ie if

src_path.name != dest_path.name: return False

Unsure if this works or if this check is already done.

There is separate logic/exception, from a different ticket for checking for self overwrite. Perhaps they should be combined.

AddisonSchiller · 2017-10-30T20:42:51Z

tests/providers/osfstorage/test_provider.py

@@ -381,6 +381,22 @@ def file_lineage():

 @pytest.mark.asyncio
 @pytest.mark.aiohttpretty
+async def test_intra_copy_folder_orphan(provider_and_mock, provider_and_mock2, children_response):


I know I already asked but putting this here for thought anyway.
Any possibility of moving this to core/test_provider? (feels more complete that way)
If it is too large of a change, go ahead and ignore this and let Longze or Fitz decide.

Spoke with Fitz. For now going with this for brevity.

AddisonSchiller · 2017-10-30T20:46:31Z

waterbutler/core/exceptions.py

@@ -193,6 +193,12 @@ def __init__(self, path):
                         'folder onto itself is not supported.'.format(path))


+class OrphanSelfError(InvalidParameters):
+    def __init__(self, path):
+        super().__init__('Unable to move or copy \'{}\'. Moving or copying a folder onto parent '


Nitpick: The error message is a bit hard for a user to understand. Not a deal breaker though.

Agreed it is a lot to parse, but not sure how to phrase it in a more clear way that still provides all needed information.

I tried to rephrase this but failed. 👍

AddisonSchiller · 2017-10-30T20:48:33Z

Forgot to add message to review....

Local testing went fine (just with osfstorage however)
1 change for sure (look at use of async/await)
some rewording and possible moving of test?
1 possible change on orphan function.
Otherwise looks good

coveralls · 2017-11-13T16:01:23Z

Coverage decreased (-0.0006%) to 89.103% when pulling 37c8756 on TomBaxter:feature/svcs-479 into 473191c on CenterForOpenScience:develop.

AddisonSchiller

You have 2 very tiny lines not hit in test coverage.
#L499 and #L248. Not a deal breaker, can be done in next phase of review

Ready for next phase now.

AddisonSchiller · 2017-11-13T16:55:50Z

waterbutler/core/provider.py

+        if not dest_path.kind == 'folder':
+            return False
+        if src_path.name != dest_path.name:
+            return False


This line isn't hit by test coverage

AddisonSchiller · 2017-11-13T16:56:29Z

waterbutler/core/provider.py

+            conflict == 'replace' and
+            dest_provider.replace_will_orphan(dest_path, src_path)
+        ):
+            raise exceptions.OrphanSelfError(src_path)


this line isn't hit by test coverage

coveralls · 2017-11-22T16:21:35Z

Coverage decreased (-0.003%) to 89.939% when pulling 71e2c08 on TomBaxter:feature/svcs-479 into 26bf209 on CenterForOpenScience:develop.

cslzchen

Looks good and works as expected locally! Please add more tests and add/update the doc str for new methods. Thanks! 🔥 🔥

cslzchen · 2017-11-29T15:18:08Z

waterbutler/core/exceptions.py

@@ -208,6 +208,12 @@ def __init__(self, path):
                         'folder onto itself is not supported.'.format(path))


+class OrphanSelfError(InvalidParameters):


Can you add a doc str for this exception?

cslzchen · 2017-11-29T15:38:10Z

waterbutler/core/provider.py

+                            src_path: wb_path.WaterButlerPath) -> bool:
+        """Check if copy/move conflict=replace will orphan src_path.
+
+        This can be an expensive operation. Should be used as last resort. Should be overridden if


Did a test, this is not as bad as it seems. Only when there is a nested structure like the following will trigger iteration on each level (for both valid and invalid move and copy).

cslzchen · 2017-11-29T15:42:00Z

tests/providers/osfstorage/test_provider.py

@@ -342,6 +342,24 @@ class TestIntraCopy:
        src_mock.validate_v1_path.assert_not_called()


+    @pytest.mark.asyncio
+    @pytest.mark.aiohttpretty
+    async def test_intra_copy_folder_orphan(self, provider_and_mock, provider_and_mock2,


As mentioned by @AddisonSchiller , this test only covers one situation when it is an invalid copy. Can you add tests to cover the following for both copy and move? So in total 4 * 2 = 8.

invalid one

return false if is not folder

return false if name is not the same

loop breaks if materialized path is not the same

Moved tests to core/provider and added move test for invalid.
Added single tests for 'not folder', 'not same name', and 'loop break' since they aren't running through copy/move.
Completed.

cslzchen · 2017-11-29T15:42:45Z

waterbutler/core/exceptions.py

@@ -193,6 +193,12 @@ def __init__(self, path):
                         'folder onto itself is not supported.'.format(path))


+class OrphanSelfError(InvalidParameters):
+    def __init__(self, path):
+        super().__init__('Unable to move or copy \'{}\'. Moving or copying a folder onto parent '


I tried to rephrase this but failed. 👍

coveralls · 2017-12-11T19:27:33Z

Coverage increased (+0.03%) to 89.806% when pulling 9b3b8b7 on TomBaxter:feature/svcs-479 into 6249a7a on CenterForOpenScience:develop.

cslzchen

LGTM and move to PCR 🎆 🎆 !

felliott · 2017-12-12T15:21:15Z

There seems to be a lot of overlap between this and #270 so they may end up getting merged into one super-PR. Leaving both open for now for reference until a final PR is merged.

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

[#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

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.

coveralls · 2017-12-19T21:00:12Z

Coverage increased (+0.06%) to 89.829% when pulling 4f985e3 on TomBaxter:feature/svcs-479 into 3ec764a on CenterForOpenScience:develop.

NyanHelsing · 2018-02-09T21:50:31Z

waterbutler/core/provider.py

+
+        if (
+            self == src_provider and
+            conflict == 'replace' and


Boolean operators should be at the start of new lines

actually no

NyanHelsing · 2018-02-12T14:41:10Z

waterbutler/core/provider.py

+        if not dest_path.kind == 'folder':
+            return False
+        if src_path.name != dest_path.name:
+            return False


I think it would be more readable x != y rather than not x == y; It'll save a few bytes, (I know we're pressed for disk), but we should probably be a little more consistent about this.

And we have a 4th one, which is used here ... Instead of doing str comparison, we should use one of the other three which is of type bool.

@property def kind(self) -> str: """ Returns `folder` if the path represents a folder, otherwise returns `file`. """ return 'folder' if self._is_folder else 'file'

That looks like its a private method because it's prefixed with an underscore. Should this be private, or can consuming code rely on self._is_folder?

Let's ignore this for now. It's everywhere in the code base, not only for WB but also for OSF and MFR ...

NyanHelsing · 2018-02-12T14:48:30Z

tests/core/test_provider.py

+        dest_path = WaterButlerPath('/folder1/',
+                                    _ids=['root','folder'],
+                                    folder=True)
+        assert provider1.replace_will_orphan(src_path, dest_path) == False


Should there be a test that covers the case the replace will orphan; i.e assert provider1.replace_will_orphan(src_path, dest_path) == True?

NyanHelsing · 2018-02-12T14:59:24Z

waterbutler/core/provider.py

+            self.shares_storage_root(src_provider) and
+            src_path.materialized_path == dest_path.materialized_path
+        ):
+            raise exceptions.OverwriteSelfError(src_path)


It seems that OverwriteSelf is a degenerate case of OrphanSelf, why do we need to check twice and/or can these checks be combined?

src_path has an is_dir attribute. Does dest_path too? If there isn't there anything other than path or dir that it can be it should be possible for line 405 to just be if dest_path.is_dir:

Both dest_path and src_path are instances of WaterButlerPath, which has is_dir, is_folder and is_file. I don't know why we have three when we only need one. Let's choose one and be consistent at least within each provider. For new providers, I use is_folder.

@property def is_dir(self) -> bool: """ Returns `True` if the path represents a folder. """ return self._is_folder @property def is_folder(self) -> bool: return self._is_folder @property def is_file(self) -> bool: """ Returns `True` if the path represents a file. """ return not self._is_folder

For the two exception, my understanding is that they handle different cases.

OverwriteSelfError handles the case when a move/copy's dest_path is exactly the same as the src_path.

OrphanSelfError handles the case when src_path and dest_path are different but dest_path happens to be one of the parent folders of the src_path up in the tree.

My suggestion is that we keep the two separate. If necessary, please update the docstr for both class and maybe a few comments on what each checks does if necessary.

NyanHelsing · 2018-02-12T16:41:12Z

waterbutler/core/provider.py

+                    raise exceptions.OverwriteSelfError(src_path)
+
+        # files and folders shouldn't overwrite themselves
+        # TODO: how is this different from above will_self_overwrite call?


Remove the todo? the former deals specifically with the case the destination is a dir, and the original will be kept?

Yes, please!

NyanHelsing · 2018-02-12T16:42:29Z

waterbutler/core/provider.py

+
+        if handle_conflicts:
+
+            dest_path = await dest_provider.handle_conflicts(


Extra whitespace?

NyanHelsing · 2018-02-12T16:42:47Z

waterbutler/core/provider.py

-            dest_path = await dest_provider.handle_naming(
+        if handle_conflicts:
+
+            dest_path = await dest_provider.handle_conflicts(


Extra whitespace

cslzchen

@birdbrained Thanks for the initial review. If you are taking this PR over, here are a few things:

Improve the code style, control/condition statement and consistency the way you prefer. I am always 👍 x 💯 for it.
For OverwriteSelfError and OrphanSelfError, I suggest keep them separate based on my understanding. However, feel free to change it if you have a good reason that I am not aware of.

cslzchen · 2018-04-04T19:14:18Z

waterbutler/core/provider.py

+            self.shares_storage_root(src_provider) and
+            src_path.materialized_path == dest_path.materialized_path
+        ):
+            raise exceptions.OverwriteSelfError(src_path)


Both dest_path and src_path are instances of WaterButlerPath, which has is_dir, is_folder and is_file. I don't know why we have three when we only need one. Let's choose one and be consistent at least within each provider. For new providers, I use is_folder.

@property def is_dir(self) -> bool: """ Returns `True` if the path represents a folder. """ return self._is_folder @property def is_folder(self) -> bool: return self._is_folder @property def is_file(self) -> bool: """ Returns `True` if the path represents a file. """ return not self._is_folder

cslzchen · 2018-04-04T19:53:56Z

waterbutler/core/provider.py

+        if not dest_path.kind == 'folder':
+            return False
+        if src_path.name != dest_path.name:
+            return False


And we have a 4th one, which is used here ... Instead of doing str comparison, we should use one of the other three which is of type bool.

@property def kind(self) -> str: """ Returns `folder` if the path represents a folder, otherwise returns `file`. """ return 'folder' if self._is_folder else 'file'

cslzchen · 2018-04-04T20:10:32Z

waterbutler/core/provider.py

+            self.shares_storage_root(src_provider) and
+            src_path.materialized_path == dest_path.materialized_path
+        ):
+            raise exceptions.OverwriteSelfError(src_path)


For the two exception, my understanding is that they handle different cases.

OverwriteSelfError handles the case when a move/copy's dest_path is exactly the same as the src_path.

OrphanSelfError handles the case when src_path and dest_path are different but dest_path happens to be one of the parent folders of the src_path up in the tree.

My suggestion is that we keep the two separate. If necessary, please update the docstr for both class and maybe a few comments on what each checks does if necessary.

cslzchen · 2018-04-04T20:12:35Z

waterbutler/core/provider.py

+                    raise exceptions.OverwriteSelfError(src_path)
+
+        # files and folders shouldn't overwrite themselves
+        # TODO: how is this different from above will_self_overwrite call?


Yes, please!

NyanHelsing · 2018-04-10T15:44:25Z

New PR #333

cslzchen · 2018-04-10T16:59:13Z

This PR is replaced and closed.

TomBaxter changed the title ~~[RFR] [SVCS-479] Raise exception for copy/move replace folder that orphans itself.~~ [SVCS-479] Raise exception for copy/move replace folder that orphans itself. Oct 5, 2017

AddisonSchiller suggested changes Oct 30, 2017

View reviewed changes

cslzchen added the To Be Replaced label Oct 30, 2017

AddisonSchiller approved these changes Nov 13, 2017

View reviewed changes

cslzchen added Code Review and removed To Be Replaced labels Nov 13, 2017

cslzchen requested changes Nov 29, 2017

View reviewed changes

cslzchen added the Add'l Dev label Dec 11, 2017

cslzchen approved these changes Dec 11, 2017

View reviewed changes

cslzchen added Final Review and removed Add'l Dev Code Review labels Dec 11, 2017

felliott mentioned this pull request Dec 12, 2017

[SVCS-133] Reject complicated self-overwrites #270

Closed

felliott added the Add'l Dev label Dec 12, 2017

AddisonSchiller and others added 5 commits December 19, 2017 14:01

Reject complicated self-overwrites

44c468c

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

Reworking and refactoring

049c593

Responding to CR

c0fc043

Refactor overwrite conflict logic

4f985e3

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.

NyanHelsing reviewed Feb 12, 2018

View reviewed changes

cslzchen removed the Add'l Dev label Apr 4, 2018

cslzchen requested changes Apr 4, 2018

View reviewed changes

cslzchen added Add'l Dev and removed Final Review labels Apr 4, 2018

NyanHelsing mentioned this pull request Apr 10, 2018

[SVCS-479] Raise exception for copy/move replace folder that orphans itself. #333

Open

cslzchen closed this Apr 10, 2018

		@@ -208,6 +208,12 @@ def __init__(self, path):
		'folder onto itself is not supported.'.format(path))


		class OrphanSelfError(InvalidParameters):


		if handle_conflicts:

		dest_path = await dest_provider.handle_conflicts(

[SVCS-479] Raise exception for copy/move replace folder that orphans itself. #274

[SVCS-479] Raise exception for copy/move replace folder that orphans itself. #274

Conversation

TomBaxter commented Sep 25, 2017 • edited Loading

Purpose:

Changes:

Side effects

coveralls commented Sep 25, 2017 • edited Loading

coveralls commented Sep 26, 2017

coveralls commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AddisonSchiller commented Oct 30, 2017 • edited Loading

coveralls commented Nov 13, 2017 • edited Loading

AddisonSchiller left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 22, 2017 • edited Loading

cslzchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 11, 2017 • edited Loading

cslzchen left a comment

Choose a reason for hiding this comment

felliott commented Dec 12, 2017

coveralls commented Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cslzchen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NyanHelsing commented Apr 10, 2018

cslzchen commented Apr 10, 2018

TomBaxter commented Sep 25, 2017 •

edited

Loading

coveralls commented Sep 25, 2017 •

edited

Loading

AddisonSchiller commented Oct 30, 2017 •

edited

Loading

coveralls commented Nov 13, 2017 •

edited

Loading

AddisonSchiller left a comment •

edited

Loading

coveralls commented Nov 22, 2017 •

edited

Loading

coveralls commented Dec 11, 2017 •

edited

Loading

coveralls commented Dec 19, 2017 •

edited

Loading

cslzchen left a comment •

edited

Loading