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/pgmanager.py b/pgcontents/pgmanager.py index 5aa418a..f887f72 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): @@ -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: diff --git a/pgcontents/query.py b/pgcontents/query.py index 781a9ac..2969f64 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,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) diff --git a/pgcontents/tests/test_hybrid_manager.py b/pgcontents/tests/test_hybrid_manager.py index 34e64da..652f5a4 100644 --- a/pgcontents/tests/test_hybrid_manager.py +++ b/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() diff --git a/pgcontents/tests/test_pgcontents_api.py b/pgcontents/tests/test_pgcontents_api.py index 6cabdc3..90f7322 100644 --- a/pgcontents/tests/test_pgcontents_api.py +++ b/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(): """ diff --git a/pgcontents/tests/test_pgmanager.py b/pgcontents/tests/test_pgmanager.py index 45247ee..2e8484d 100644 --- a/pgcontents/tests/test_pgmanager.py +++ b/pgcontents/tests/test_pgmanager.py @@ -185,6 +185,44 @@ 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() + assert nb_name == 'Untitled.ipynb' + + # A simple rename of the file within the same directory. + cm.rename(nb_path, 'new_name.ipynb') + assert cm.get('new_name.ipynb')['path'] == 'new_name.ipynb' + + # The old file name should no longer be found. + with assertRaisesHTTPError(self, 404): + cm.get(nb_name) + + # Test that renaming outside of the root fails. + with assertRaisesHTTPError(self, 404): + cm.rename('../foo', '../bar') + + # Test that renaming something to itself fails. + with assertRaisesHTTPError(self, 409): + cm.rename('new_name.ipynb', 'new_name.ipynb') + + # Test that renaming a non-existent file fails. + 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('new_name.ipynb', nb_destination) + + updated_notebook_model = cm.get(nb_destination) + 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('new_name.ipynb') + def test_rename_directory(self): """ Create a directory hierarchy that looks like: @@ -244,6 +282,95 @@ 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 + + self.make_dir('Parent Folder') + self.make_dir('Child Folder') + + # A rename moving one folder into the other. + child_folder_destination = 'Parent Folder/Child Folder' + cm.rename('Child Folder', child_folder_destination) + + 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('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'] == 'Child Folder' + assert updated_child_model['path'] == child_folder_destination + + # Test moving it back up. + cm.rename('Parent Folder/Child Folder', 'Child Folder') + + updated_parent_model = cm.get('Parent Folder') + assert len(updated_parent_model['content']) == 0 + + with assertRaisesHTTPError(self, 404): + cm.get('Parent Folder/Child Folder') + + 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 + + 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". + cm.rename('foo/bar/populated_dir', 'biz/populated_dir') + + 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_max_file_size(self): cm = self.contents_manager