Move Files and Directories by dmichalowicz · Pull Request #64 · quantopian/pgcontents

dmichalowicz · 2019-07-16T13:08:24Z

Add support for moving files and directories.

dmichalowicz · 2019-07-16T15:46:54Z

+        try:
+            super(PostgresTestCase, self).test_rename_file()
+        finally:
+            del HybridContentsManager.get_file_id


This setup isn't great but I didn't have any other ideas at the moment other than doing this mocking, or removing this check: https://github.com/quantopian/pgcontents/pull/64/files#diff-ecde3e655b312f914854e679ceacaeccR195

I think the test_rename_file test in PostgresContentsManagerTestCase could be re-written to not need file id?

dmichalowicz · 2019-07-16T15:59:04Z

@ssanderson The build is still failing but this should still be ready for a first look.

dmichalowicz · 2019-07-18T15:31:42Z

I'm pretty sure I did something wrong with managing connections to the database, so the tests are hitting the limit of the maximum number of allowed connections.

dmichalowicz · 2019-07-18T17:21:32Z

+
+        # Move the populated directory over to "biz".
+        renamed = cm.rename_file('foo/bar/populated_dir', 'biz/populated_dir')
+        assert renamed == 1


This is probably misleading now that I think about it...

ssanderson

@dmichalowicz left some comments for you.

ssanderson · 2019-07-18T17:30:17Z


    save = path_dispatch2('save', 'model', True)
    rename = path_dispatch_old_new('rename', False)
+    rename_file = path_dispatch_old_new('rename_file', False)


What's up with this addition? Is there some new codepath now that needs this?

This is needed due to the fact that one of the hybrid manager tests subclasses PostgresContentsManagerTestCase, which now call rename_file directly.

ssanderson · 2019-07-18T17:47:19Z

            model['message'] = validation_message
        return model

+    @outside_root_to_404


What's up with this refactor? Are there cases where we now need the ability to rename multiple files?

Discussed in slack, going to remove this in favor of renaming multiple files by making multiple http requests to rename_file.

ssanderson · 2019-07-18T17:47:59Z

+        Returns
+        -------
+        renamed : int
+            The count of paths that were successfully renamed.


I think this has to be len(old_paths) right? When would a user need this return value?

They wouldn't, it's mainly for testing purposes. I'm going to either change rename_file to return nothing, or to return a simple string saying whether we "renamed_file" or "renamed_directory".

ssanderson · 2019-07-18T17:49:00Z

+
+        Parameters
+        ----------
+        old_paths : list


Seems like a dict mapping old -> new might be a simpler API here? Do we need order here?

ssanderson · 2019-07-18T19:40:17Z

+        assert nb_model['name'] == 'Untitled.ipynb'
+
+        # A simple rename of the file within the same directory.
+        file_id = cm.get_file_id('Untitled.ipynb')


Rather than depend on get_file_id for this test (which isn't part of the contents spec; we added it to make notebook sharing work internally at Quantopian I believe), could we just set the content of the file when we create it and make assertions about that? Or do we want to assert specifically that this operation doesn't change the file id?

Is there a reason to use Untitled.ipynb here vs. nb_name, which is already in scope?

Or do we want to assert specifically that this operation doesn't change the file id?

Nope, it was more just an easy way to check we're dealing with the right file. I think it's fine to simply remove.

I'll change this to nb_name

ssanderson · 2019-07-18T20:15:18Z

+        try:
+            super(PostgresTestCase, self).test_rename_file()
+        finally:
+            del HybridContentsManager.get_file_id


I think the test_rename_file test in PostgresContentsManagerTestCase could be re-written to not need file id?

ssanderson · 2019-07-18T20:16:42Z

+    def test_move_empty_directory(self):
+        cm = self.contents_manager
+
+        parent_folder_model = cm.new_untitled(type='directory')


Same notes as above here re: actually naming these.

ssanderson · 2019-07-18T20:22:56Z

+
+        # A simple rename of the file within the same directory.
+        file_id = cm.get_file_id('Untitled.ipynb')
+        renamed = cm.rename_file(nb_path, 'new_name.ipynb')


This test should probably be using rename, not rename_file. rename is the API called by the server; it's responsible for both moving files and moving checkpoints. rename_file is just the file-moving portion of that responsibility. We generally test with rename elsewhere in this suite.

I noticed that but actually thought calling rename_file was better/more explicit considering it's the function we are overriding. I can change this to rename, but if that's always the preferred method I kind of wish rename_file was made private 😕

In a language like C++ or Java, rename_file would probably have been "protected" (i.e., visible to subclasses, but not to code outside the class hierarchy); it's publicly-named on the base class because it's designed for subclasses to override, but it's not really public for external consumers.

ssanderson · 2019-07-18T20:47:14Z

+        for old_path in old_api_paths:
+            with assertRaisesHTTPError(self, 404):
+                cm.get(old_path)
+


One thing I think we're missing here is a set of tests asserting that checkpoints get moved correctly when files move. The best model for that would probably be to add a test similar to test_checkpoints_follow_file (defined in test_contents_api in the upstream jupyter notebook test suite) and add it to our _APITestBase that defines a suite of tests that should be passed by all of our implementations.

ssanderson · 2019-07-18T20:49:49Z

+    # manager that implements it.
+    def test_move_multiple_objects(self):
+        pass
+


Do we need a test asserting that HybridContentsManager doesn't allow file renames between sub-managers? I think that behavior is still preserved here, but it doesn't look like we have any coverage for it, which we probably should.

(This would probably want to go in MultiRootTestCase).

Ya I can add that

dmichalowicz · 2019-07-19T18:36:18Z

        cls.td.cleanup()

+    def test_checkpoints_move_with_file(self):
+        pass


So this test fails for this case because the FileCheckpoints class is not recognizing any checkpoints when renaming a file. See https://github.com/jupyter/notebook/blob/master/notebook/services/contents/filecheckpoints.py#L98-L99. I'm not sure what to do about this without making upstream changes, but I can imagine that method wanting to list out all checkpoints for the given path if the path is a directory.

Hmm, yeah, that looks like it's a bug upstream. I opened #68 to track this. Can you add a comment pointing to that issue? I'll push an issue and/or bugfix upstream.

Ya I'll add a comment.

dmichalowicz · 2019-07-19T18:42:45Z

Thinking more about the database connection issues, I suppose it's also possible there's no actual problem with my changes but rather the tests I added simply pushed the total number of connections over the maximum? Even if that's the case I'm still not so sure how to debug.

dmichalowicz · 2019-07-22T18:22:58Z

@ssanderson I rebased this and the build is fixed, and I removed the "renaming multiple files at once" stuff, so it's ready for another look when you get a chance.

ssanderson

@dmichalowicz couple small questions. Otherwise LGTM.

ssanderson · 2019-07-23T14:50:09Z

                self.do_409(str(e))
+            except (web.HTTPError, PathOutsideRoot):
+                raise
+            except Exception as e:


Do uncaught exceptions not get logged by default by tornado? I thought they did.

Looks like they do (I didn't know this), I'm just going to remove this except clause entirely.

Actually, despite this fact, I think the manual logging here is clearer/more explicit about there being an issue in our code and about what exact rename call caused the problem. So I don't think it hurts to keep this. I also noticed this is what save does above.

ssanderson · 2019-07-23T14:51:46Z


+    def test_checkpoints_move_with_file(self):
+        # Read initial file state.
+        self.api.read('foo/a.ipynb')


Is this call doing anything?

Nope I had copied one of the notebook tests to model off of but this isn't needed.

ssanderson · 2019-07-23T15:03:06Z

        cls.td.cleanup()

+    def test_checkpoints_move_with_file(self):
+        pass


Hmm, yeah, that looks like it's a bug upstream. I opened #68 to track this. Can you add a comment pointing to that issue? I'll push an issue and/or bugfix upstream.

dmichalowicz added the enhancement label Jul 16, 2019

dmichalowicz assigned ssanderson Jul 16, 2019

dmichalowicz mentioned this pull request Jul 16, 2019

Move Files and Directories #63

Closed

dmichalowicz commented Jul 16, 2019

View reviewed changes

dmichalowicz commented Jul 18, 2019

View reviewed changes

ssanderson reviewed Jul 18, 2019

View reviewed changes

dmichalowicz commented Jul 19, 2019

View reviewed changes

dmichalowicz force-pushed the move-files-rebased branch from 0c5a18e to 68db4e2 Compare July 22, 2019 18:07

ssanderson force-pushed the move-files-rebased branch from 68db4e2 to aad97b2 Compare July 23, 2019 14:10

quantopian deleted a comment from dmichalowicz Jul 23, 2019

ssanderson approved these changes Jul 23, 2019

View reviewed changes

PrestonYadegar and others added 4 commits July 23, 2019 12:07

ENH: Support for moving files and directories

2de2bd3

MAINT: Remove function for renaming multiple files at once

e82e744

TEST: Test that renames cannot happen across roots

3c563be

BUG: Checkpoints were not being moved for directory renames

4fb66e1

dmichalowicz force-pushed the move-files-rebased branch from aad97b2 to 4fb66e1 Compare July 23, 2019 16:07

dmichalowicz merged commit cf7bc6d into master Jul 23, 2019

dmichalowicz deleted the move-files-rebased branch July 23, 2019 17:39

Uh oh!

Conversation

dmichalowicz commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmichalowicz commented Jul 16, 2019

Uh oh!

dmichalowicz commented Jul 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssanderson Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmichalowicz commented Jul 19, 2019

Uh oh!

dmichalowicz commented Jul 22, 2019

Uh oh!

ssanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dmichalowicz commented Jul 16, 2019 •

edited

Loading

ssanderson Jul 18, 2019 •

edited

Loading