From 2de2bd3be5657152f67954d1b1a344fb0ab6fbdf Mon Sep 17 00:00:00 2001
From: Preston Yadegar <pyadegar@quantopian.com>
Date: Mon, 20 Aug 2018 11:52:51 -0400
Subject: [PATCH 1/4] ENH: Support for moving files and directories

---
 .gitignore                              |   1 +
 pgcontents/hybridmanager.py             |   1 +
 pgcontents/pgmanager.py                 |  95 ++++++++--
 pgcontents/query.py                     |  42 ++---
 pgcontents/tests/test_hybrid_manager.py |  17 ++
 pgcontents/tests/test_pgmanager.py      | 225 ++++++++++++++++++++++++
 6 files changed, 340 insertions(+), 41 deletions(-)

diff --git a/.gitignore b/.gitignore
index aee4476..0b20999 100644
--- a/.gitignore
+++ b/.gitignore
@@ -40,6 +40,7 @@ htmlcov/
 .noseids
 nosetests.xml
 coverage.xml
+/.pytest_cache/*
 
 # Translations
 *.mo
diff --git a/pgcontents/hybridmanager.py b/pgcontents/hybridmanager.py
index 9c2862c..c9a2241 100644
--- a/pgcontents/hybridmanager.py
+++ b/pgcontents/hybridmanager.py
@@ -221,6 +221,7 @@ def _extra_root_dirs(self):
 
     save = path_dispatch2('save', 'model', True)
     rename = path_dispatch_old_new('rename', False)
+    rename_file = path_dispatch_old_new('rename_file', False)
 
     __get = path_dispatch1('get', True)
     __delete = path_dispatch1('delete', False)
diff --git a/pgcontents/pgmanager.py b/pgcontents/pgmanager.py
index 5aa418a..3e5037a 100644
--- a/pgcontents/pgmanager.py
+++ b/pgcontents/pgmanager.py
@@ -18,10 +18,11 @@
 from __future__ import unicode_literals
 from itertools import chain
 from tornado import web
+from traitlets import default
 
 from .api_utils import (
-    base_model,
     base_directory_model,
+    base_model,
     from_b64,
     outside_root_to_404,
     reads_base64,
@@ -57,7 +58,6 @@
     save_file,
 )
 from .utils.ipycompat import Bool, ContentsManager, from_dict
-from traitlets import default
 
 
 class PostgresContentsManager(PostgresManagerMixin, ContentsManager):
@@ -376,26 +376,89 @@ def save(self, model, path):
             model['message'] = validation_message
         return model
 
+    @outside_root_to_404
+    def rename_files(self, old_paths, new_paths):
+        """
+        Rename multiple objects at once. This function is specific to this
+        implementation of ContentsManager and is not in the base class.
+
+        Parameters
+        ----------
+        old_paths : list
+            List of paths for existing files or directories to be renamed. The
+            index position of each path should align with the index of its new
+            path in the `new_paths` parameter.
+        new_paths : list
+            List of new paths to which the files or directories should be
+            named. The index position of each path should align with the index
+            of its existing path in the `old_paths` parameter.
+
+        Returns
+        -------
+        renamed : int
+            The count of paths that were successfully renamed.
+
+        Raises
+        ------
+        FileExists
+            If one of the new paths already exists as a file.
+        DirectoryExists
+            If one of the new paths already exists as a directory.
+        RenameRoot
+            If one of the old paths given is the root path.
+        PathOutsideRoot
+            If one of the new paths given is outside of the root path.
+        """
+        if len(set(old_paths)) != len(old_paths):
+            self.do_409(
+                'The list of paths to rename cannot contain duplicates.',
+            )
+        if len(set(new_paths)) != len(new_paths):
+            self.do_409(
+                'The list of new path names cannot contain duplicates.',
+            )
+
+        renamed = 0
+
+        with self.engine.begin() as db:
+            for old_path, new_path in zip(old_paths, new_paths):
+                try:
+                    if self.file_exists(old_path):
+                        rename_file(db, self.user_id, old_path, new_path)
+                    elif self.dir_exists(old_path):
+                        rename_directory(db, self.user_id, old_path, new_path)
+                    else:
+                        self.no_such_entity(old_path)
+                except (FileExists, DirectoryExists):
+                    self.already_exists(new_path)
+                except RenameRoot as e:
+                    self.do_409(str(e))
+                except (web.HTTPError, PathOutsideRoot):
+                    raise
+                except Exception as e:
+                    self.log.exception(
+                        'Error renaming file/directory from %s to %s',
+                        old_path,
+                        new_path,
+                    )
+                    self.do_500(
+                        u'Unexpected error while renaming %s: %s'
+                        % (old_path, e)
+                    )
+                renamed += 1
+
+        self.log.info('Successfully renamed %d paths.', renamed)
+        return renamed
+
     @outside_root_to_404
     def rename_file(self, old_path, path):
         """
         Rename object from old_path to path.
 
-        NOTE: This method is unfortunately named on the base class.  It
-        actually moves a file or a directory.
+        NOTE: This method is unfortunately named on the base class. It actually
+              moves files and directories as well.
         """
-        with self.engine.begin() as db:
-            try:
-                if self.file_exists(old_path):
-                    rename_file(db, self.user_id, old_path, path)
-                elif self.dir_exists(old_path):
-                    rename_directory(db, self.user_id, old_path, path)
-                else:
-                    self.no_such_entity(path)
-            except (FileExists, DirectoryExists):
-                self.already_exists(path)
-            except RenameRoot as e:
-                self.do_409(str(e))
+        return self.rename_files([old_path], [path])
 
     def _delete_non_directory(self, path):
         with self.engine.begin() as db:
diff --git a/pgcontents/query.py b/pgcontents/query.py
index 781a9ac..a0ddc97 100644
--- a/pgcontents/query.py
+++ b/pgcontents/query.py
@@ -1,8 +1,6 @@
 """
 Database Queries for PostgresContentsManager.
 """
-from textwrap import dedent
-
 from sqlalchemy import (
     and_,
     cast,
@@ -420,32 +418,18 @@ def rename_file(db, user_id, old_api_path, new_api_path):
     """
     Rename a file.
     """
-
     # Overwriting existing files is disallowed.
     if file_exists(db, user_id, new_api_path):
         raise FileExists(new_api_path)
 
-    old_dir, old_name = split_api_filepath(old_api_path)
     new_dir, new_name = split_api_filepath(new_api_path)
-    if old_dir != new_dir:
-        raise ValueError(
-            dedent(
-                """
-                Can't rename object to new directory.
-                Old Path: {old_api_path}
-                New Path: {new_api_path}
-                """.format(
-                    old_api_path=old_api_path,
-                    new_api_path=new_api_path
-                )
-            )
-        )
 
     db.execute(
         files.update().where(
             _file_where(user_id, old_api_path),
         ).values(
             name=new_name,
+            parent_name=new_dir,
             created_at=func.now(),
         )
     )
@@ -470,7 +454,13 @@ def rename_directory(db, user_id, old_api_path, new_api_path):
     db.execute('SET CONSTRAINTS '
                'pgcontents.directories_parent_user_id_fkey DEFERRED')
 
-    # Update name column for the directory that's being renamed
+    old_api_dir, old_name = split_api_filepath(old_api_path)
+    new_api_dir, new_name = split_api_filepath(new_api_path)
+    new_db_dir = from_api_dirname(new_api_dir)
+
+    # Update the name and parent_name columns for the directory that is being
+    # renamed. The parent_name column will not change for a simple rename, but
+    # will if the directory is moving.
     db.execute(
         directories.update().where(
             and_(
@@ -479,34 +469,36 @@ def rename_directory(db, user_id, old_api_path, new_api_path):
             )
         ).values(
             name=new_db_path,
+            parent_name=new_db_dir,
         )
     )
 
-    # Update the name and parent_name of any descendant directories.  Do
-    # this in a single statement so the non-deferrable check constraint
-    # is satisfied.
+    # Update the name and parent_name of any descendant directories. Do this in
+    # a single statement so the non-deferrable check constraint is satisfied.
     db.execute(
         directories.update().where(
             and_(
                 directories.c.user_id == user_id,
                 directories.c.name.startswith(old_db_path),
                 directories.c.parent_name.startswith(old_db_path),
-            )
+            ),
         ).values(
             name=func.concat(
                 new_db_path,
-                func.right(directories.c.name, -func.length(old_db_path))
+                func.right(directories.c.name, -func.length(old_db_path)),
             ),
             parent_name=func.concat(
                 new_db_path,
                 func.right(
                     directories.c.parent_name,
-                    -func.length(old_db_path)
-                )
+                    -func.length(old_db_path),
+                ),
             ),
         )
     )
 
+    return True
+
 
 def save_file(db, user_id, path, content, encrypt_func, max_size_bytes):
     """
diff --git a/pgcontents/tests/test_hybrid_manager.py b/pgcontents/tests/test_hybrid_manager.py
index 34e64da..518b989 100644
--- a/pgcontents/tests/test_hybrid_manager.py
+++ b/pgcontents/tests/test_hybrid_manager.py
@@ -11,6 +11,8 @@
     join as osjoin,
 )
 from posixpath import join as pjoin
+
+from mock import Mock
 from six import (
     iteritems,
     itervalues,
@@ -88,6 +90,21 @@ def setUp(self):
     def test_get_file_id(self):
         pass
 
+    # This test also uses `get_file_id`, but it is not pertinent to the crucial
+    # parts of the test so just mock out.
+    def test_rename_file(self):
+        HybridContentsManager.get_file_id = Mock()
+        try:
+            super(PostgresTestCase, self).test_rename_file()
+        finally:
+            del HybridContentsManager.get_file_id
+
+    # This test calls `rename_files` which HybridContentsManager is also not
+    # expected to dispatch to as PostgresContentsManager is the only contents
+    # manager that implements it.
+    def test_move_multiple_objects(self):
+        pass
+
     def set_pgmgr_attribute(self, name, value):
         setattr(self._pgmanager, name, value)
 
diff --git a/pgcontents/tests/test_pgmanager.py b/pgcontents/tests/test_pgmanager.py
index 45247ee..8ddbe57 100644
--- a/pgcontents/tests/test_pgmanager.py
+++ b/pgcontents/tests/test_pgmanager.py
@@ -185,6 +185,71 @@ def test_get_file_id(self):
         cm.rename(path, updated_path)
         self.assertEqual(id_, cm.get_file_id(updated_path))
 
+    def test_rename_file(self):
+        cm = self.contents_manager
+        nb, nb_name, nb_path = self.new_notebook()
+        nb_model = cm.get(nb_path)
+        assert nb_model['name'] == 'Untitled.ipynb'
+
+        # 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')
+        assert renamed == 1
+        assert cm.get('new_name.ipynb')['path'] == 'new_name.ipynb'
+        assert cm.get_file_id('new_name.ipynb') == file_id
+
+        # The old file name should no longer be found.
+        with assertRaisesHTTPError(self, 404):
+            cm.get('Untitled.ipynb')
+
+        # Test that renaming outside of the root fails.
+        with assertRaisesHTTPError(self, 404):
+            cm.rename_file('../foo', '../bar')
+
+        # Test that renaming something to itself fails.
+        with assertRaisesHTTPError(self, 409):
+            cm.rename_file('new_name.ipynb', 'new_name.ipynb')
+
+        # Test that attempting to rename something twice fails.
+        with assertRaisesHTTPError(self, 409):
+            cm.rename_files(
+                old_paths=['new_name.ipynb', 'new_name.ipynb'],
+                new_paths=['new_name_2.ipynb', 'new_name_3.ipynb'],
+            )
+
+        # Test that trying to rename two different things as the same fails.
+        nb, nb_name, nb_path = self.new_notebook()
+        assert cm.get(nb_path)['name'] == 'Untitled.ipynb'
+        with assertRaisesHTTPError(self, 409):
+            cm.rename_files(
+                old_paths=['new_name.ipynb', 'Untitled.ipynb'],
+                new_paths=['new_name_2.ipynb', 'new_name_2.ipynb'],
+            )
+
+    def test_move_file(self):
+        cm = self.contents_manager
+        nb, nb_name, nb_path = self.new_notebook()
+        nb_model = cm.get(nb_path)
+        folder_model = cm.new_untitled(type='directory')
+        nb_destination = 'Untitled Folder/Untitled.ipynb'
+        assert nb_model['name'] == 'Untitled.ipynb'
+        assert nb_model['path'] == 'Untitled.ipynb'
+        assert folder_model['name'] == 'Untitled Folder'
+        assert folder_model['path'] == 'Untitled Folder'
+
+        # A rename of the file into another directory.
+        renamed = cm.rename_file('Untitled.ipynb', nb_destination)
+        from pdb import set_trace; set_trace()
+        assert renamed == 1
+
+        updated_notebook_model = cm.get(nb_destination)
+        assert updated_notebook_model['name'] == 'Untitled.ipynb'
+        assert updated_notebook_model['path'] == nb_destination
+
+        # The old file name should no longer be found.
+        with assertRaisesHTTPError(self, 404):
+            cm.get('Untitled.ipynb')
+
     def test_rename_directory(self):
         """
         Create a directory hierarchy that looks like:
@@ -244,6 +309,166 @@ def test_rename_directory(self):
         # Verify that we can now create a new notebook in the changed directory
         cm.new_untitled('foo/bar_changed', ext='.ipynb')
 
+    def test_move_empty_directory(self):
+        cm = self.contents_manager
+
+        parent_folder_model = cm.new_untitled(type='directory')
+        child_folder_model = cm.new_untitled(type='directory')
+        assert parent_folder_model['name'] == 'Untitled Folder'
+        assert parent_folder_model['path'] == 'Untitled Folder'
+        assert child_folder_model['name'] == 'Untitled Folder 1'
+        assert child_folder_model['path'] == 'Untitled Folder 1'
+
+        # A rename moving one folder into the other.
+        child_folder_destination = 'Untitled Folder/Untitled Folder 1'
+        renamed = cm.rename_file('Untitled Folder 1', child_folder_destination)
+        assert renamed == 1
+
+        updated_parent_model = cm.get('Untitled Folder')
+        assert updated_parent_model['path'] == 'Untitled Folder'
+        assert len(updated_parent_model['content']) == 1
+
+        with assertRaisesHTTPError(self, 404):
+            # Should raise a 404 because the contents manager should not be
+            # able to find a folder with this path.
+            cm.get('Untitled Folder 1')
+
+        # Confirm that the child folder has moved into the parent folder.
+        updated_child_model = cm.get(child_folder_destination)
+        assert updated_child_model['name'] == 'Untitled Folder 1'
+        assert (
+            updated_child_model['path'] == child_folder_destination
+        )
+
+        # Test moving it back up.
+        renamed = cm.rename_file(
+            updated_child_model['path'],
+            'Untitled Folder 1',
+        )
+        assert renamed == 1
+
+        updated_parent_model = cm.get('Untitled Folder')
+        assert len(updated_parent_model['content']) == 0
+
+        with assertRaisesHTTPError(self, 404):
+            cm.get('Untitled Folder/Untitled Folder 1')
+
+        updated_child_model = cm.get('Untitled Folder 1')
+        assert updated_child_model['name'] == 'Untitled Folder 1'
+        assert updated_child_model['path'] == 'Untitled Folder 1'
+
+    def test_move_populated_directory(self):
+        cm = self.contents_manager
+
+        all_dirs = [
+            'foo', 'foo/bar', 'foo/bar/populated_dir',
+            'biz', 'biz/buz',
+        ]
+
+        for dir_ in all_dirs:
+            if dir_ == 'foo/bar/populated_dir':
+                self.make_populated_dir(dir_)
+                self.check_populated_dir_files(dir_)
+            else:
+                self.make_dir(dir_)
+
+        # Move the populated directory over to "biz".
+        renamed = cm.rename_file('foo/bar/populated_dir', 'biz/populated_dir')
+        assert renamed == 1
+
+        bar_model = cm.get('foo/bar')
+        assert len(bar_model['content']) == 0
+
+        biz_model = cm.get('biz')
+        assert len(biz_model['content']) == 2
+
+        with assertRaisesHTTPError(self, 404):
+            cm.get('foo/bar/populated_dir')
+
+        populated_dir_model = cm.get('biz/populated_dir')
+        assert populated_dir_model['name'] == 'populated_dir'
+        assert populated_dir_model['path'] == 'biz/populated_dir'
+        self.check_populated_dir_files('biz/populated_dir')
+
+        # Test moving a directory with sub-directories and files that go
+        # multiple layers deep.
+        self.make_populated_dir('biz/populated_dir/populated_sub_dir')
+        self.make_dir('biz/populated_dir/populated_sub_dir/empty_dir')
+        cm.rename('biz/populated_dir', 'populated_dir')
+
+        populated_dir_model = cm.get('populated_dir')
+        assert populated_dir_model['name'] == 'populated_dir'
+        assert populated_dir_model['path'] == 'populated_dir'
+        self.check_populated_dir_files('populated_dir')
+        self.check_populated_dir_files('populated_dir/populated_sub_dir')
+
+        empty_dir_model = cm.get('populated_dir/populated_sub_dir/empty_dir')
+        assert empty_dir_model['name'] == 'empty_dir'
+        assert (
+            empty_dir_model['path'] ==
+            'populated_dir/populated_sub_dir/empty_dir'
+        )
+        assert len(empty_dir_model['content']) == 0
+
+    def test_move_multiple_objects(self):
+        cm = self.contents_manager
+        nb_1, nb_name_1, nb_path_1 = self.new_notebook()
+        nb_2, nb_name_2, nb_path_2 = self.new_notebook()
+        nb_model_1 = cm.get(nb_path_1)
+        nb_model_2 = cm.get(nb_path_2)
+        folder_model_1 = cm.new_untitled(type='directory')
+        folder_model_2 = cm.new_untitled(type='directory')
+        folder_path_1 = folder_model_1['path']
+        folder_path_2 = folder_model_2['path']
+
+        assert nb_model_1['path'] == 'Untitled.ipynb'
+        assert nb_model_2['path'] == 'Untitled1.ipynb'
+        assert folder_path_1 == 'Untitled Folder'
+        assert folder_path_2 == 'Untitled Folder 1'
+
+        # First test that if there is a single bad path given then none of the
+        # paths change.
+        old_api_paths = [nb_path_1, nb_path_2, folder_path_2]
+        new_api_paths = [
+            'Badpath/Untitled.ipynb',
+            'Untitled Folder/Untitled1.ipynb',
+            'Untitled Folder/Untitled Folder 1',
+        ]
+
+        with assertRaisesHTTPError(self, 500):
+            renamed = cm.rename_files(old_api_paths, new_api_paths)
+
+        # Nothing should have changed.
+        assert cm.get(nb_path_1)['path'] == 'Untitled.ipynb'
+        assert cm.get(nb_path_2)['path'] == 'Untitled1.ipynb'
+        assert cm.get(folder_path_2)['path'] == 'Untitled Folder 1'
+
+        # Now test a successful change of all three.
+        old_api_paths = [nb_path_1, nb_path_2, folder_path_2]
+        new_api_paths = [
+            'Untitled Folder/Untitled.ipynb',
+            'Untitled Folder/Untitled1.ipynb',
+            'Untitled Folder/Untitled Folder 1',
+        ]
+
+        # Rename both files and one of the directories into the other directory
+        # all at once.
+        renamed = cm.rename_files(old_api_paths, new_api_paths)
+        assert renamed == 3
+
+        updated_folder_model_1 = cm.get(folder_path_1)
+        assert len(updated_folder_model_1['content']) == 3
+
+        for new_path in new_api_paths:
+            updated_model = cm.get(new_path)
+            assert updated_model['path'] == new_path
+            assert updated_model['name'] == new_path.split('/')[-1]
+
+        # The old file name should no longer be found.
+        for old_path in old_api_paths:
+            with assertRaisesHTTPError(self, 404):
+                cm.get(old_path)
+
     def test_max_file_size(self):
 
         cm = self.contents_manager

From e82e744b07238a45a44245ddbb9b316aff80692c Mon Sep 17 00:00:00 2001
From: David Michalowicz <dmichalowicz@quantopian.com>
Date: Fri, 19 Jul 2019 10:42:50 -0400
Subject: [PATCH 2/4] MAINT: Remove function for renaming multiple files at
 once

---
 pgcontents/pgmanager.py                 | 101 ++++-----------
 pgcontents/query.py                     |   3 -
 pgcontents/tests/test_hybrid_manager.py |  19 +--
 pgcontents/tests/test_pgcontents_api.py |  31 +++++
 pgcontents/tests/test_pgmanager.py      | 158 +++++-------------------
 5 files changed, 91 insertions(+), 221 deletions(-)

diff --git a/pgcontents/pgmanager.py b/pgcontents/pgmanager.py
index 3e5037a..f6baeeb 100644
--- a/pgcontents/pgmanager.py
+++ b/pgcontents/pgmanager.py
@@ -376,80 +376,6 @@ def save(self, model, path):
             model['message'] = validation_message
         return model
 
-    @outside_root_to_404
-    def rename_files(self, old_paths, new_paths):
-        """
-        Rename multiple objects at once. This function is specific to this
-        implementation of ContentsManager and is not in the base class.
-
-        Parameters
-        ----------
-        old_paths : list
-            List of paths for existing files or directories to be renamed. The
-            index position of each path should align with the index of its new
-            path in the `new_paths` parameter.
-        new_paths : list
-            List of new paths to which the files or directories should be
-            named. The index position of each path should align with the index
-            of its existing path in the `old_paths` parameter.
-
-        Returns
-        -------
-        renamed : int
-            The count of paths that were successfully renamed.
-
-        Raises
-        ------
-        FileExists
-            If one of the new paths already exists as a file.
-        DirectoryExists
-            If one of the new paths already exists as a directory.
-        RenameRoot
-            If one of the old paths given is the root path.
-        PathOutsideRoot
-            If one of the new paths given is outside of the root path.
-        """
-        if len(set(old_paths)) != len(old_paths):
-            self.do_409(
-                'The list of paths to rename cannot contain duplicates.',
-            )
-        if len(set(new_paths)) != len(new_paths):
-            self.do_409(
-                'The list of new path names cannot contain duplicates.',
-            )
-
-        renamed = 0
-
-        with self.engine.begin() as db:
-            for old_path, new_path in zip(old_paths, new_paths):
-                try:
-                    if self.file_exists(old_path):
-                        rename_file(db, self.user_id, old_path, new_path)
-                    elif self.dir_exists(old_path):
-                        rename_directory(db, self.user_id, old_path, new_path)
-                    else:
-                        self.no_such_entity(old_path)
-                except (FileExists, DirectoryExists):
-                    self.already_exists(new_path)
-                except RenameRoot as e:
-                    self.do_409(str(e))
-                except (web.HTTPError, PathOutsideRoot):
-                    raise
-                except Exception as e:
-                    self.log.exception(
-                        'Error renaming file/directory from %s to %s',
-                        old_path,
-                        new_path,
-                    )
-                    self.do_500(
-                        u'Unexpected error while renaming %s: %s'
-                        % (old_path, e)
-                    )
-                renamed += 1
-
-        self.log.info('Successfully renamed %d paths.', renamed)
-        return renamed
-
     @outside_root_to_404
     def rename_file(self, old_path, path):
         """
@@ -458,7 +384,32 @@ def rename_file(self, old_path, path):
         NOTE: This method is unfortunately named on the base class. It actually
               moves files and directories as well.
         """
-        return self.rename_files([old_path], [path])
+        with self.engine.begin() as db:
+            try:
+                if self.file_exists(old_path):
+                    rename_file(db, self.user_id, old_path, path)
+                elif self.dir_exists(old_path):
+                    rename_directory(db, self.user_id, old_path, path)
+                else:
+                    self.no_such_entity(old_path)
+                    from pdb import set_trace; set_trace()
+                    return 'bad_path_failure'
+            except (FileExists, DirectoryExists):
+                self.already_exists(path)
+            except RenameRoot as e:
+                self.do_409(str(e))
+            except (web.HTTPError, PathOutsideRoot):
+                raise
+            except Exception as e:
+                self.log.exception(
+                    'Error renaming file/directory from %s to %s',
+                    old_path,
+                    path,
+                )
+                self.do_500(
+                    u'Unexpected error while renaming %s: %s'
+                    % (old_path, e)
+                )
 
     def _delete_non_directory(self, path):
         with self.engine.begin() as db:
diff --git a/pgcontents/query.py b/pgcontents/query.py
index a0ddc97..b38d6c6 100644
--- a/pgcontents/query.py
+++ b/pgcontents/query.py
@@ -454,7 +454,6 @@ def rename_directory(db, user_id, old_api_path, new_api_path):
     db.execute('SET CONSTRAINTS '
                'pgcontents.directories_parent_user_id_fkey DEFERRED')
 
-    old_api_dir, old_name = split_api_filepath(old_api_path)
     new_api_dir, new_name = split_api_filepath(new_api_path)
     new_db_dir = from_api_dirname(new_api_dir)
 
@@ -497,8 +496,6 @@ def rename_directory(db, user_id, old_api_path, new_api_path):
         )
     )
 
-    return True
-
 
 def save_file(db, user_id, path, content, encrypt_func, max_size_bytes):
     """
diff --git a/pgcontents/tests/test_hybrid_manager.py b/pgcontents/tests/test_hybrid_manager.py
index 518b989..c333337 100644
--- a/pgcontents/tests/test_hybrid_manager.py
+++ b/pgcontents/tests/test_hybrid_manager.py
@@ -90,21 +90,6 @@ def setUp(self):
     def test_get_file_id(self):
         pass
 
-    # This test also uses `get_file_id`, but it is not pertinent to the crucial
-    # parts of the test so just mock out.
-    def test_rename_file(self):
-        HybridContentsManager.get_file_id = Mock()
-        try:
-            super(PostgresTestCase, self).test_rename_file()
-        finally:
-            del HybridContentsManager.get_file_id
-
-    # This test calls `rename_files` which HybridContentsManager is also not
-    # expected to dispatch to as PostgresContentsManager is the only contents
-    # manager that implements it.
-    def test_move_multiple_objects(self):
-        pass
-
     def set_pgmgr_attribute(self, name, value):
         setattr(self._pgmanager, name, value)
 
@@ -304,6 +289,10 @@ def test_cant_delete_root(self):
             with assertRaisesHTTPError(self, 400):
                 cm.delete(prefix)
 
+    # def test_cant_rename_across_roots(self):
+    #     from pdb import set_trace; set_trace()
+    #     a = 2
+
     def tearDown(self):
         for dir_ in itervalues(self.temp_dirs):
             dir_.cleanup()
diff --git a/pgcontents/tests/test_pgcontents_api.py b/pgcontents/tests/test_pgcontents_api.py
index 6cabdc3..90a6b41 100644
--- a/pgcontents/tests/test_pgcontents_api.py
+++ b/pgcontents/tests/test_pgcontents_api.py
@@ -197,6 +197,37 @@ def test_delete_non_empty_dir(self):
             # be deleted)
             super(_APITestBase, self).test_delete_non_empty_dir()
 
+    def test_checkpoints_move_with_file(self):
+        # Read initial file state.
+        self.api.read('foo/a.ipynb')
+
+        # Create a checkpoint of initial state.
+        response = self.api.new_checkpoint('foo/a.ipynb')
+        response_json = response.json()
+
+        # Move the file down.
+        self.api.rename('foo/a.ipynb', 'foo/bar/a.ipynb')
+
+        # Looking for checkpoints in the old location should yield no results.
+        self.assertEqual(self.api.get_checkpoints('foo/a.ipynb').json(), [])
+
+        # Looking for checkpoints in the new location should work.
+        checkpoints = self.api.get_checkpoints('foo/bar/a.ipynb').json()
+        self.assertEqual(checkpoints, [response_json])
+
+        # Move the file back up.
+        self.api.rename('foo/bar/a.ipynb', 'foo/a.ipynb')
+
+        # Looking for checkpoints in the old location should yield no results.
+        self.assertEqual(
+            self.api.get_checkpoints('foo/bar/a.ipynb').json(),
+            [],
+        )
+
+        # Looking for checkpoints in the new location should work.
+        checkpoints = self.api.get_checkpoints('foo/a.ipynb').json()
+        self.assertEqual(checkpoints, [response_json])
+
 
 def _test_delete_non_empty_dir_fail(self, path):
     with assert_http_error(400):
diff --git a/pgcontents/tests/test_pgmanager.py b/pgcontents/tests/test_pgmanager.py
index 8ddbe57..ee700ba 100644
--- a/pgcontents/tests/test_pgmanager.py
+++ b/pgcontents/tests/test_pgmanager.py
@@ -188,67 +188,40 @@ def test_get_file_id(self):
     def test_rename_file(self):
         cm = self.contents_manager
         nb, nb_name, nb_path = self.new_notebook()
-        nb_model = cm.get(nb_path)
-        assert nb_model['name'] == 'Untitled.ipynb'
+        assert nb_name == 'Untitled.ipynb'
 
         # 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')
-        assert renamed == 1
+        cm.rename(nb_path, 'new_name.ipynb')
         assert cm.get('new_name.ipynb')['path'] == 'new_name.ipynb'
-        assert cm.get_file_id('new_name.ipynb') == file_id
 
         # The old file name should no longer be found.
         with assertRaisesHTTPError(self, 404):
-            cm.get('Untitled.ipynb')
+            cm.get(nb_name)
 
         # Test that renaming outside of the root fails.
         with assertRaisesHTTPError(self, 404):
-            cm.rename_file('../foo', '../bar')
+            cm.rename('../foo', '../bar')
 
         # Test that renaming something to itself fails.
         with assertRaisesHTTPError(self, 409):
-            cm.rename_file('new_name.ipynb', 'new_name.ipynb')
+            cm.rename('new_name.ipynb', 'new_name.ipynb')
 
-        # Test that attempting to rename something twice fails.
-        with assertRaisesHTTPError(self, 409):
-            cm.rename_files(
-                old_paths=['new_name.ipynb', 'new_name.ipynb'],
-                new_paths=['new_name_2.ipynb', 'new_name_3.ipynb'],
-            )
+        # Test that renaming a non-existent file fails.
+        with self.assertRaisesHTTPError(self, 409):
+            cm.rename('non_existent.ipynb', 'some_name.ipynb')
 
-        # Test that trying to rename two different things as the same fails.
-        nb, nb_name, nb_path = self.new_notebook()
-        assert cm.get(nb_path)['name'] == 'Untitled.ipynb'
-        with assertRaisesHTTPError(self, 409):
-            cm.rename_files(
-                old_paths=['new_name.ipynb', 'Untitled.ipynb'],
-                new_paths=['new_name_2.ipynb', 'new_name_2.ipynb'],
-            )
-
-    def test_move_file(self):
-        cm = self.contents_manager
-        nb, nb_name, nb_path = self.new_notebook()
-        nb_model = cm.get(nb_path)
-        folder_model = cm.new_untitled(type='directory')
-        nb_destination = 'Untitled Folder/Untitled.ipynb'
-        assert nb_model['name'] == 'Untitled.ipynb'
-        assert nb_model['path'] == 'Untitled.ipynb'
-        assert folder_model['name'] == 'Untitled Folder'
-        assert folder_model['path'] == 'Untitled Folder'
-
-        # A rename of the file into another directory.
-        renamed = cm.rename_file('Untitled.ipynb', nb_destination)
-        from pdb import set_trace; set_trace()
-        assert renamed == 1
+        # Now test moving a file.
+        self.make_dir('My Folder')
+        nb_destination = 'My Folder/new_name.ipynb'
+        cm.rename_file('new_name.ipynb', nb_destination)
 
         updated_notebook_model = cm.get(nb_destination)
-        assert updated_notebook_model['name'] == 'Untitled.ipynb'
+        assert updated_notebook_model['name'] == 'new_name.ipynb'
         assert updated_notebook_model['path'] == nb_destination
 
         # The old file name should no longer be found.
         with assertRaisesHTTPError(self, 404):
-            cm.get('Untitled.ipynb')
+            cm.get('new_name.ipynb')
 
     def test_rename_directory(self):
         """
@@ -312,50 +285,39 @@ def test_rename_directory(self):
     def test_move_empty_directory(self):
         cm = self.contents_manager
 
-        parent_folder_model = cm.new_untitled(type='directory')
-        child_folder_model = cm.new_untitled(type='directory')
-        assert parent_folder_model['name'] == 'Untitled Folder'
-        assert parent_folder_model['path'] == 'Untitled Folder'
-        assert child_folder_model['name'] == 'Untitled Folder 1'
-        assert child_folder_model['path'] == 'Untitled Folder 1'
+        self.make_dir('Parent Folder')
+        self.make_dir('Child Folder')
 
         # A rename moving one folder into the other.
-        child_folder_destination = 'Untitled Folder/Untitled Folder 1'
-        renamed = cm.rename_file('Untitled Folder 1', child_folder_destination)
-        assert renamed == 1
+        child_folder_destination = 'Parent Folder/Child Folder'
+        cm.rename_file('Child Folder', child_folder_destination)
 
-        updated_parent_model = cm.get('Untitled Folder')
-        assert updated_parent_model['path'] == 'Untitled Folder'
+        updated_parent_model = cm.get('Parent Folder')
+        assert updated_parent_model['path'] == 'Parent Folder'
         assert len(updated_parent_model['content']) == 1
 
         with assertRaisesHTTPError(self, 404):
             # Should raise a 404 because the contents manager should not be
             # able to find a folder with this path.
-            cm.get('Untitled Folder 1')
+            cm.get('Child Folder')
 
         # Confirm that the child folder has moved into the parent folder.
         updated_child_model = cm.get(child_folder_destination)
-        assert updated_child_model['name'] == 'Untitled Folder 1'
-        assert (
-            updated_child_model['path'] == child_folder_destination
-        )
+        assert updated_child_model['name'] == 'Child Folder'
+        assert updated_child_model['path'] == child_folder_destination
 
         # Test moving it back up.
-        renamed = cm.rename_file(
-            updated_child_model['path'],
-            'Untitled Folder 1',
-        )
-        assert renamed == 1
+        cm.rename_file('Parent Folder/Child Folder', 'Child Folder')
 
-        updated_parent_model = cm.get('Untitled Folder')
+        updated_parent_model = cm.get('Parent Folder')
         assert len(updated_parent_model['content']) == 0
 
         with assertRaisesHTTPError(self, 404):
-            cm.get('Untitled Folder/Untitled Folder 1')
+            cm.get('Parent Folder/Child Folder')
 
-        updated_child_model = cm.get('Untitled Folder 1')
-        assert updated_child_model['name'] == 'Untitled Folder 1'
-        assert updated_child_model['path'] == 'Untitled Folder 1'
+        updated_child_model = cm.get('Child Folder')
+        assert updated_child_model['name'] == 'Child Folder'
+        assert updated_child_model['path'] == 'Child Folder'
 
     def test_move_populated_directory(self):
         cm = self.contents_manager
@@ -373,8 +335,7 @@ def test_move_populated_directory(self):
                 self.make_dir(dir_)
 
         # Move the populated directory over to "biz".
-        renamed = cm.rename_file('foo/bar/populated_dir', 'biz/populated_dir')
-        assert renamed == 1
+        cm.rename_file('foo/bar/populated_dir', 'biz/populated_dir')
 
         bar_model = cm.get('foo/bar')
         assert len(bar_model['content']) == 0
@@ -410,65 +371,6 @@ def test_move_populated_directory(self):
         )
         assert len(empty_dir_model['content']) == 0
 
-    def test_move_multiple_objects(self):
-        cm = self.contents_manager
-        nb_1, nb_name_1, nb_path_1 = self.new_notebook()
-        nb_2, nb_name_2, nb_path_2 = self.new_notebook()
-        nb_model_1 = cm.get(nb_path_1)
-        nb_model_2 = cm.get(nb_path_2)
-        folder_model_1 = cm.new_untitled(type='directory')
-        folder_model_2 = cm.new_untitled(type='directory')
-        folder_path_1 = folder_model_1['path']
-        folder_path_2 = folder_model_2['path']
-
-        assert nb_model_1['path'] == 'Untitled.ipynb'
-        assert nb_model_2['path'] == 'Untitled1.ipynb'
-        assert folder_path_1 == 'Untitled Folder'
-        assert folder_path_2 == 'Untitled Folder 1'
-
-        # First test that if there is a single bad path given then none of the
-        # paths change.
-        old_api_paths = [nb_path_1, nb_path_2, folder_path_2]
-        new_api_paths = [
-            'Badpath/Untitled.ipynb',
-            'Untitled Folder/Untitled1.ipynb',
-            'Untitled Folder/Untitled Folder 1',
-        ]
-
-        with assertRaisesHTTPError(self, 500):
-            renamed = cm.rename_files(old_api_paths, new_api_paths)
-
-        # Nothing should have changed.
-        assert cm.get(nb_path_1)['path'] == 'Untitled.ipynb'
-        assert cm.get(nb_path_2)['path'] == 'Untitled1.ipynb'
-        assert cm.get(folder_path_2)['path'] == 'Untitled Folder 1'
-
-        # Now test a successful change of all three.
-        old_api_paths = [nb_path_1, nb_path_2, folder_path_2]
-        new_api_paths = [
-            'Untitled Folder/Untitled.ipynb',
-            'Untitled Folder/Untitled1.ipynb',
-            'Untitled Folder/Untitled Folder 1',
-        ]
-
-        # Rename both files and one of the directories into the other directory
-        # all at once.
-        renamed = cm.rename_files(old_api_paths, new_api_paths)
-        assert renamed == 3
-
-        updated_folder_model_1 = cm.get(folder_path_1)
-        assert len(updated_folder_model_1['content']) == 3
-
-        for new_path in new_api_paths:
-            updated_model = cm.get(new_path)
-            assert updated_model['path'] == new_path
-            assert updated_model['name'] == new_path.split('/')[-1]
-
-        # The old file name should no longer be found.
-        for old_path in old_api_paths:
-            with assertRaisesHTTPError(self, 404):
-                cm.get(old_path)
-
     def test_max_file_size(self):
 
         cm = self.contents_manager

From 3c563be628e6bbf9b84a2fb095d53c42e16ccddb Mon Sep 17 00:00:00 2001
From: David Michalowicz <dmichalowicz@quantopian.com>
Date: Fri, 19 Jul 2019 11:15:06 -0400
Subject: [PATCH 3/4] TEST: Test that renames cannot happen across roots

---
 pgcontents/hybridmanager.py             |  1 -
 pgcontents/pgmanager.py                 |  2 --
 pgcontents/tests/test_hybrid_manager.py | 10 ++++++----
 pgcontents/tests/test_pgmanager.py      | 10 +++++-----
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/pgcontents/hybridmanager.py b/pgcontents/hybridmanager.py
index c9a2241..9c2862c 100644
--- a/pgcontents/hybridmanager.py
+++ b/pgcontents/hybridmanager.py
@@ -221,7 +221,6 @@ def _extra_root_dirs(self):
 
     save = path_dispatch2('save', 'model', True)
     rename = path_dispatch_old_new('rename', False)
-    rename_file = path_dispatch_old_new('rename_file', False)
 
     __get = path_dispatch1('get', True)
     __delete = path_dispatch1('delete', False)
diff --git a/pgcontents/pgmanager.py b/pgcontents/pgmanager.py
index f6baeeb..f887f72 100644
--- a/pgcontents/pgmanager.py
+++ b/pgcontents/pgmanager.py
@@ -392,8 +392,6 @@ def rename_file(self, old_path, path):
                     rename_directory(db, self.user_id, old_path, path)
                 else:
                     self.no_such_entity(old_path)
-                    from pdb import set_trace; set_trace()
-                    return 'bad_path_failure'
             except (FileExists, DirectoryExists):
                 self.already_exists(path)
             except RenameRoot as e:
diff --git a/pgcontents/tests/test_hybrid_manager.py b/pgcontents/tests/test_hybrid_manager.py
index c333337..652f5a4 100644
--- a/pgcontents/tests/test_hybrid_manager.py
+++ b/pgcontents/tests/test_hybrid_manager.py
@@ -12,7 +12,6 @@
 )
 from posixpath import join as pjoin
 
-from mock import Mock
 from six import (
     iteritems,
     itervalues,
@@ -289,9 +288,12 @@ def test_cant_delete_root(self):
             with assertRaisesHTTPError(self, 400):
                 cm.delete(prefix)
 
-    # def test_cant_rename_across_roots(self):
-    #     from pdb import set_trace; set_trace()
-    #     a = 2
+    def test_cant_rename_across_managers(self):
+        cm = self.contents_manager
+        cm.new_untitled(ext='.ipynb')
+
+        with assertRaisesHTTPError(self, 400):
+            cm.rename('Untitled.ipynb', 'A/Untitled.ipynb')
 
     def tearDown(self):
         for dir_ in itervalues(self.temp_dirs):
diff --git a/pgcontents/tests/test_pgmanager.py b/pgcontents/tests/test_pgmanager.py
index ee700ba..2e8484d 100644
--- a/pgcontents/tests/test_pgmanager.py
+++ b/pgcontents/tests/test_pgmanager.py
@@ -207,13 +207,13 @@ def test_rename_file(self):
             cm.rename('new_name.ipynb', 'new_name.ipynb')
 
         # Test that renaming a non-existent file fails.
-        with self.assertRaisesHTTPError(self, 409):
+        with assertRaisesHTTPError(self, 404):
             cm.rename('non_existent.ipynb', 'some_name.ipynb')
 
         # Now test moving a file.
         self.make_dir('My Folder')
         nb_destination = 'My Folder/new_name.ipynb'
-        cm.rename_file('new_name.ipynb', nb_destination)
+        cm.rename('new_name.ipynb', nb_destination)
 
         updated_notebook_model = cm.get(nb_destination)
         assert updated_notebook_model['name'] == 'new_name.ipynb'
@@ -290,7 +290,7 @@ def test_move_empty_directory(self):
 
         # A rename moving one folder into the other.
         child_folder_destination = 'Parent Folder/Child Folder'
-        cm.rename_file('Child Folder', child_folder_destination)
+        cm.rename('Child Folder', child_folder_destination)
 
         updated_parent_model = cm.get('Parent Folder')
         assert updated_parent_model['path'] == 'Parent Folder'
@@ -307,7 +307,7 @@ def test_move_empty_directory(self):
         assert updated_child_model['path'] == child_folder_destination
 
         # Test moving it back up.
-        cm.rename_file('Parent Folder/Child Folder', 'Child Folder')
+        cm.rename('Parent Folder/Child Folder', 'Child Folder')
 
         updated_parent_model = cm.get('Parent Folder')
         assert len(updated_parent_model['content']) == 0
@@ -335,7 +335,7 @@ def test_move_populated_directory(self):
                 self.make_dir(dir_)
 
         # Move the populated directory over to "biz".
-        cm.rename_file('foo/bar/populated_dir', 'biz/populated_dir')
+        cm.rename('foo/bar/populated_dir', 'biz/populated_dir')
 
         bar_model = cm.get('foo/bar')
         assert len(bar_model['content']) == 0

From 4fb66e14e3fad85d9b77f221ed335004650d5500 Mon Sep 17 00:00:00 2001
From: David Michalowicz <dmichalowicz@quantopian.com>
Date: Fri, 19 Jul 2019 14:31:49 -0400
Subject: [PATCH 4/4] BUG: Checkpoints were not being moved for directory
 renames

---
 pgcontents/query.py                     | 22 +++++++++++++++++
 pgcontents/tests/test_pgcontents_api.py | 33 ++++++++++++++++++-------
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/pgcontents/query.py b/pgcontents/query.py
index b38d6c6..2969f64 100644
--- a/pgcontents/query.py
+++ b/pgcontents/query.py
@@ -650,6 +650,9 @@ def move_single_remote_checkpoint(db,
 def move_remote_checkpoints(db, user_id, src_api_path, dest_api_path):
     src_db_path = from_api_filename(src_api_path)
     dest_db_path = from_api_filename(dest_api_path)
+
+    # Update the paths of the checkpoints for the file being renamed. If the
+    # source path is for a directory then this is a no-op.
     db.execute(
         remote_checkpoints.update().where(
             and_(
@@ -661,6 +664,25 @@ def move_remote_checkpoints(db, user_id, src_api_path, dest_api_path):
         ),
     )
 
+    # If the given source path is for a directory, update the paths of the
+    # checkpoints for all files in that directory and its subdirectories.
+    db.execute(
+        remote_checkpoints.update().where(
+            and_(
+                remote_checkpoints.c.user_id == user_id,
+                remote_checkpoints.c.path.startswith(src_db_path),
+            ),
+        ).values(
+            path=func.concat(
+                dest_db_path,
+                func.right(
+                    remote_checkpoints.c.path,
+                    -func.length(src_db_path),
+                ),
+            ),
+        )
+    )
+
 
 def get_remote_checkpoint(db, user_id, api_path, checkpoint_id, decrypt_func):
     db_path = from_api_filename(api_path)
diff --git a/pgcontents/tests/test_pgcontents_api.py b/pgcontents/tests/test_pgcontents_api.py
index 90a6b41..90f7322 100644
--- a/pgcontents/tests/test_pgcontents_api.py
+++ b/pgcontents/tests/test_pgcontents_api.py
@@ -198,9 +198,6 @@ def test_delete_non_empty_dir(self):
             super(_APITestBase, self).test_delete_non_empty_dir()
 
     def test_checkpoints_move_with_file(self):
-        # Read initial file state.
-        self.api.read('foo/a.ipynb')
-
         # Create a checkpoint of initial state.
         response = self.api.new_checkpoint('foo/a.ipynb')
         response_json = response.json()
@@ -215,17 +212,23 @@ def test_checkpoints_move_with_file(self):
         checkpoints = self.api.get_checkpoints('foo/bar/a.ipynb').json()
         self.assertEqual(checkpoints, [response_json])
 
-        # Move the file back up.
-        self.api.rename('foo/bar/a.ipynb', 'foo/a.ipynb')
-
-        # Looking for checkpoints in the old location should yield no results.
+        # Rename the directory that the file is in.
+        self.api.rename('foo/bar', 'foo/car')
         self.assertEqual(
             self.api.get_checkpoints('foo/bar/a.ipynb').json(),
             [],
         )
+        checkpoints = self.api.get_checkpoints('foo/car/a.ipynb').json()
+        self.assertEqual(checkpoints, [response_json])
 
-        # Looking for checkpoints in the new location should work.
-        checkpoints = self.api.get_checkpoints('foo/a.ipynb').json()
+        # Now move the directory that the file is in.
+        self.make_dir('foo/buz')
+        self.api.rename('foo/car', 'foo/buz/car')
+        self.assertEqual(
+            self.api.get_checkpoints('foo/car/a.ipynb').json(),
+            [],
+        )
+        checkpoints = self.api.get_checkpoints('foo/buz/car/a.ipynb').json()
         self.assertEqual(checkpoints, [response_json])
 
 
@@ -405,6 +408,18 @@ def teardown_class(cls):
         super(PostgresContentsFileCheckpointsAPITest, cls).teardown_class()
         cls.td.cleanup()
 
+    def test_checkpoints_move_with_file(self):
+        # This test fails for this suite because the FileCheckpoints class is
+        # not recognizing any checkpoints when renaming a directory. See:
+        #     https://github.com/jupyter/notebook/blob/bd6396d31e56f311e4022215
+        #     25f9db7686834150/notebook/services/contents/filecheckpoints.py#L9
+        #     8-L99
+        # It looks like this is a bug upstream, as I can imagine that method
+        # wanting to list out all checkpoints for the given path if the path is
+        # a directory. For now we filed an issue to track this:
+        #     https://github.com/quantopian/pgcontents/issues/68
+        pass
+
 
 def postgres_checkpoints_config():
     """