diff --git a/pgcontents/pgmanager.py b/pgcontents/pgmanager.py index 3d7429c..ead76d7 100644 --- a/pgcontents/pgmanager.py +++ b/pgcontents/pgmanager.py @@ -51,14 +51,16 @@ ) from .managerbase import PostgresManagerMixin from .query import ( - delete_file, delete_directory, + delete_file, dir_exists, ensure_directory, + file_exists, get_directory, get_file, purge_user, - rename, + rename_directory, + rename_file, save_file, ) @@ -139,11 +141,7 @@ def is_hidden(self, path): @outside_root_to_404 def file_exists(self, path): with self.engine.begin() as db: - try: - get_file(db, self.user_id, path, include_content=False) - return True - except NoSuchFile: - return False + return file_exists(db, self.user_id, path) @outside_root_to_404 def get(self, path, content=True, type=None, format=None): @@ -349,10 +347,18 @@ def save(self, model, path): 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. """ with self.engine.begin() as db: try: - rename(db, self.user_id, old_path, path) + 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) diff --git a/pgcontents/query.py b/pgcontents/query.py index 946958b..08b8ac0 100644 --- a/pgcontents/query.py +++ b/pgcontents/query.py @@ -349,12 +349,15 @@ def file_exists(db, user_id, path): return False -def rename(db, user_id, old_api_path, new_api_path): +def rename_file(db, user_id, old_api_path, new_api_path): """ - Rename a file or a directory. - - TODO: Don't do anything if paths are the same. + 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: @@ -371,74 +374,69 @@ def rename(db, user_id, old_api_path, new_api_path): ) ) - # Only allow writes to extant directories - if not _dir_exists(db, user_id, new_dir): - raise NoSuchDirectory(new_dir) - - # If we're renaming a file - if file_exists(db, user_id, old_api_path): - # Overwriting existing files is disallowed. - if file_exists(db, user_id, new_api_path): - raise FileExists(new_api_path) - - db.execute( - files.update().where( - _file_where(user_id, old_api_path), - ).values( - name=new_name, - created_at=func.now(), - ) + db.execute( + files.update().where( + _file_where(user_id, old_api_path), + ).values( + name=new_name, + created_at=func.now(), ) + ) - # If we're renaming a directory + +def rename_directory(db, user_id, old_api_path, new_api_path): + """ + Rename a directory. + """ old_db_path = from_api_dirname(old_api_path) new_db_path = from_api_dirname(new_api_path) - if _dir_exists(db, user_id, old_db_path): - # Overwriting existing directories is disallowed. - if _dir_exists(db, user_id, new_db_path): - raise DirectoryExists(new_api_path) - # Set this foreign key constraint to deferred so it's not violated - # when we run the first statement to update the name of the directory. - db.execute('SET CONSTRAINTS ' - 'pgcontents.directories_parent_user_id_fkey DEFERRED') + # Overwriting existing directories is disallowed. + if _dir_exists(db, user_id, new_db_path): + raise DirectoryExists(new_api_path) - # Update name column for the directory that's being renamed - db.execute( - directories.update().where( - and_( - directories.c.user_id == user_id, - directories.c.name == old_db_path, - ) - ).values( - name=new_db_path, + # Set this foreign key constraint to deferred so it's not violated + # when we run the first statement to update the name of the directory. + db.execute('SET CONSTRAINTS ' + 'pgcontents.directories_parent_user_id_fkey DEFERRED') + + # Update name column for the directory that's being renamed + db.execute( + directories.update().where( + and_( + directories.c.user_id == user_id, + directories.c.name == old_db_path, ) + ).values( + name=new_db_path, ) + ) - # 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)) - ), - parent_name=func.concat( - new_db_path, - func.right( - directories.c.parent_name, - -func.length(old_db_path) - ) - ), + # 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)) + ), + parent_name=func.concat( + new_db_path, + func.right( + directories.c.parent_name, + -func.length(old_db_path) + ) + ), ) + ) + def check_content(content, max_size_bytes): """ diff --git a/pgcontents/tests/test_pgmanager.py b/pgcontents/tests/test_pgmanager.py index 791b6fc..d614d89 100644 --- a/pgcontents/tests/test_pgmanager.py +++ b/pgcontents/tests/test_pgmanager.py @@ -18,6 +18,7 @@ from __future__ import unicode_literals from base64 import b64encode +from itertools import combinations from IPython.html.services.contents.tests.test_manager import TestContentsManager # noqa @@ -58,6 +59,43 @@ def make_dir(self, api_path): path=api_path, ) + def make_populated_dir(self, api_path): + """ + Create a directory at api_path with a notebook and a text file. + """ + self.make_dir(api_path) + self.contents_manager.new( + path='/'.join([api_path, 'nb.ipynb']) + ) + self.contents_manager.new( + path='/'.join([api_path, 'file.txt']) + ) + + def check_populated_dir_files(self, api_path): + """ + Check that a directory created with make_populated_dir has a + notebook and a text file with expected names. + """ + dirmodel = self.contents_manager.get(api_path) + self.assertEqual(dirmodel['path'], api_path) + self.assertEqual(dirmodel['type'], 'directory') + for entry in dirmodel['content']: + # Skip any subdirectories created after the fact. + if entry['type'] == 'directory': + continue + elif entry['type'] == 'file': + self.assertEqual(entry['name'], 'file.txt') + self.assertEqual( + entry['path'], + '/'.join([api_path, 'file.txt']), + ) + elif entry['type'] == 'notebook': + self.assertEqual(entry['name'], 'nb.ipynb') + self.assertEqual( + entry['path'], + '/'.join([api_path, 'nb.ipynb']), + ) + def tearDown(self): drop_testing_db_tables() migrate_testing_db() @@ -85,77 +123,59 @@ def test_modified_date(self): self.assertGreater(renamed['last_modified'], saved['last_modified']) def test_rename_directory(self): + """ + Create a directory hierarchy that looks like: + + foo/ + ... + bar/ + ... + foo/ + ... + bar/ + ... + bar/ + + then rename /foo/bar -> /foo/bar_changed and verify that all changes + propagate correctly. + """ cm = self.contents_manager - # Create an untitled directory - foo_dir = cm.new_untitled(type='directory') - old_foo_dir_path = foo_dir['path'] + all_dirs = ['foo', 'bar', 'foo/bar', 'foo/bar/foo', 'foo/bar/foo/bar'] + unchanged_dirs = all_dirs[:2] + changed_dirs = all_dirs[2:] - # Change the path on the model and call cm.update to rename - foo_dir_path = 'foo' - foo_dir['path'] = foo_dir_path - foo_dir = cm.update(foo_dir, old_foo_dir_path) + for dir_ in all_dirs: + self.make_populated_dir(dir_) + self.check_populated_dir_files(dir_) - # Check that the cm.update returns a model - assert isinstance(foo_dir, dict) + # Renaming to an extant directory should raise + for src, dest in combinations(all_dirs, 2): + with assertRaisesHTTPError(self, 409): + cm.rename(src, dest) - # Make sure the untitled directory is gone - self.assertRaises(HTTPError, cm.get, old_foo_dir_path) + # Verify that we can't create a new notebook in the (nonexistent) + # target directory + with assertRaisesHTTPError(self, 404): + cm.new_untitled('foo/bar_changed', ext='.ipynb') - # Create a subdirectory - bar_dir = cm.new( - model={'type': 'directory'}, - path='foo/bar', - ) - old_bar_dir_path = bar_dir['path'] + cm.rename('foo/bar', 'foo/bar_changed') - # Create a file in the subdirectory - bar_file = cm.new_untitled(path='foo/bar', type='notebook') - old_bar_file_path = bar_file['path'] + # foo/ and bar/ should be unchanged + for unchanged in unchanged_dirs: + self.check_populated_dir_files(unchanged) - # Create another subdirectory one level deeper. Use 'foo' for the name - # again to catch issues with replacing all instances of a substring - # instead of just the first. - bar2_dir = cm.new( - model={'type': 'directory'}, - path='foo/bar/bar', - ) - old_bar2_dir_path = bar2_dir['path'] - - # Create a file in the two-level deep directory we just created - bar2_file = cm.new_untitled(path=old_bar2_dir_path, type='notebook') - old_bar2_file_path = bar2_file['path'] - - # Change the path of the first bar directory - new_bar_dir_path = 'foo/bar_changed' - bar_dir['path'] = new_bar_dir_path - bar_dir = cm.update(bar_dir, old_bar_dir_path) - self.assertIn('name', bar_dir) - self.assertIn('path', bar_dir) - self.assertEqual(bar_dir['name'], 'bar_changed') - - # Make sure calling cm.get on any old paths throws an exception - self.assertRaises(HTTPError, cm.get, old_bar_dir_path) - self.assertRaises(HTTPError, cm.get, old_bar2_dir_path) - self.assertRaises(HTTPError, cm.get, old_bar_file_path) - self.assertRaises(HTTPError, cm.get, old_bar2_file_path) - - def try_get_new_path(full_old_path): - # replace the first occurence of the old path with the new one - new_path = full_old_path.replace( - old_bar_dir_path, - new_bar_dir_path, - 1 + # foo/bar/ and subdirectories should have leading prefixes changed + for changed_dirname in changed_dirs: + with assertRaisesHTTPError(self, 404): + cm.get(changed_dirname) + new_dirname = changed_dirname.replace( + 'foo/bar', 'foo/bar_changed', 1 ) - new_model = cm.get(new_path) - self.assertIn('name', new_model) - self.assertIn('path', new_model) - - # Make sure the directories and files can be found at their new paths - try_get_new_path(foo_dir_path) # top level foo dir should be unchanged - try_get_new_path(old_bar_file_path) - try_get_new_path(old_bar2_dir_path) - try_get_new_path(old_bar2_file_path) + self.check_populated_dir_files(new_dirname) + + # Verify that we can now create a new notebook in the changed directory + cm.new_untitled('foo/bar_changed', ext='.ipynb') def test_max_file_size(self): diff --git a/setup.py b/setup.py index a63f37e..1e29440 100644 --- a/setup.py +++ b/setup.py @@ -25,15 +25,16 @@ def main(): setup( name='pgcontents', - version='0.0.2', + version='0.1', description="A Postgres-backed ContentsManager for IPython.", author="Scott Sanderson", author_email="ssanderson@quantopian.com", packages=[ 'pgcontents', - 'pgcontents/utils/', 'pgcontents/alembic', 'pgcontents/alembic/versions', + 'pgcontents/tests/', + 'pgcontents/utils/', ], license='Apache 2.0', include_package_data=True,