Merge pull request #7 from quantopian/rename-directories-changes

TimShawver · TimShawver · commit 86054f1612aa · 2015-05-19T13:11:14.000-04:00
Rename directories changes
diff --git 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
@@ -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
@@ -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
@@ -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,
