Move Files and Directories

.gitignore

@@ -40,6 +40,7 @@ htmlcov/
 .noseids
 nosetests.xml
 coverage.xml
+/.pytest_cache/*
 
 # Translations
 *.mo

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):
@@ -381,8 +381,8 @@ 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:
@@ -391,11 +391,23 @@ def rename_file(self, old_path, path):
                 elif self.dir_exists(old_path):
                     rename_directory(db, self.user_id, old_path, path)
                 else:
-                    self.no_such_entity(path)
+                    self.no_such_entity(old_path)
             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:

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,12 @@ 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
+    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,30 +468,30 @@ 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),
+                ),
             ),
         )
     )
@@ -661,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_(
@@ -672,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)

pgcontents/tests/test_hybrid_manager.py

@@ -11,6 +11,7 @@
     join as osjoin,
 )
 from posixpath import join as pjoin
+
 from six import (
     iteritems,
     itervalues,
@@ -287,6 +288,13 @@ def test_cant_delete_root(self):
             with assertRaisesHTTPError(self, 400):
                 cm.delete(prefix)
 
+    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):
             dir_.cleanup()

pgcontents/tests/test_pgcontents_api.py

@@ -197,6 +197,40 @@ def test_delete_non_empty_dir(self):
             # be deleted)
             super(_APITestBase, self).test_delete_non_empty_dir()
 
+    def test_checkpoints_move_with_file(self):
+        # 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])
+
+        # 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])
+
+        # 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])
+
 
 def _test_delete_non_empty_dir_fail(self, path):
     with assert_http_error(400):
@@ -374,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():
     """