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