Apply PR #6609 to 6.5.x (Fix rename_file and delete_file to handle hidden files properly) (#6660)

* Fix the path form for rename_file

* Fix tests for rename_file to give values in relative paths

* Fix tests to be in line with jupyter-server

* Fix for determining whether a file is hidden and tests for delete_file

Co-authored-by: yacchin1205 <[email protected]>

Author: yacchin1205

notebook/services/contents/filemanager.py

@@ -526,14 +526,14 @@ def delete_file(self, path):
         os_path = self._get_os_path(path)
         rm = os.unlink
 
+        if is_hidden(os_path, self.root_dir) and not self.allow_hidden:
+            raise web.HTTPError(400, f'Cannot delete file or directory {os_path!r}')
+
         four_o_four = "file or directory does not exist: %r" % path
 
         if not self.exists(path):
             raise web.HTTPError(404, four_o_four)
 
-        if is_hidden(os_path, self.root_dir) and not self.allow_hidden:
-            raise web.HTTPError(400, f'Cannot delete file or directory {os_path!r}')
-
         def is_non_empty_dir(os_path):
             if os.path.isdir(os_path):
                 # A directory containing only leftover checkpoints is
@@ -575,16 +575,16 @@ def rename_file(self, old_path, new_path):
         if new_path == old_path:
             return
 
-        if (is_hidden(old_path, self.root_dir) or is_hidden(new_path, self.root_dir)) and not self.allow_hidden:
-            raise web.HTTPError(400, f'Cannot rename file or directory {old_path!r}')
-
         # Perform path validation prior to converting to os-specific value since this
         # is still relative to root_dir.
         self._validate_path(new_path)
 
         new_os_path = self._get_os_path(new_path)
         old_os_path = self._get_os_path(old_path)
 
+        if (is_hidden(old_os_path, self.root_dir) or is_hidden(new_os_path, self.root_dir)) and not self.allow_hidden:
+            raise web.HTTPError(400, f'Cannot rename file or directory {old_path!r}')
+
         # Should we proceed with the move?
         if os.path.exists(new_os_path) and not samefile(old_os_path, new_os_path):
             raise web.HTTPError(409, f'File already exists: {new_path}')

notebook/services/contents/tests/test_manager.py

@@ -185,37 +185,26 @@ def test_403(self):
     def test_400(self):
         #Test Delete behavior
         #Test delete of file in hidden directory
-        with self.assertRaises(HTTPError) as excinfo:
-            with TemporaryDirectory() as td:
-                cm = FileContentsManager(root_dir=td)
-                hidden_dir = '.hidden'
-                file_in_hidden_path = os.path.join(hidden_dir,'visible.txt')
-                _make_dir(cm, hidden_dir)
-                model = cm.new(path=file_in_hidden_path)
-                os_path = cm._get_os_path(model['path'])
+        with TemporaryDirectory() as td:
+            cm = FileContentsManager(root_dir=td)
+            hidden_dir = '.hidden'
+            file_in_hidden_path = os.path.join(hidden_dir,'visible.txt')
+            _make_dir(cm, hidden_dir)
+
+            with self.assertRaises(HTTPError) as excinfo:
+                cm.delete_file(file_in_hidden_path)
+            self.assertEqual(excinfo.exception.status_code, 400)
 
-                try:
-                    result = cm.delete_file(os_path)
-                except HTTPError as e:
-                    self.assertEqual(e.status_code, 400)
-                else:
-                    self.fail("Should have raised HTTPError(400)")
         #Test delete hidden file in visible directory
-        with self.assertRaises(HTTPError) as excinfo:
-            with TemporaryDirectory() as td:
-                cm = FileContentsManager(root_dir=td)
-                hidden_dir = 'visible'
-                file_in_hidden_path = os.path.join(hidden_dir,'.hidden.txt')
-                _make_dir(cm, hidden_dir)
-                model = cm.new(path=file_in_hidden_path)
-                os_path = cm._get_os_path(model['path'])
+        with TemporaryDirectory() as td:
+            cm = FileContentsManager(root_dir=td)
+            hidden_dir = 'visible'
+            file_in_hidden_path = os.path.join(hidden_dir,'.hidden.txt')
+            _make_dir(cm, hidden_dir)
 
-                try:
-                    result = cm.delete_file(os_path)
-                except HTTPError as e:
-                    self.assertEqual(e.status_code, 400)
-                else:
-                    self.fail("Should have raised HTTPError(400)")
+            with self.assertRaises(HTTPError) as excinfo:
+                cm.delete_file(file_in_hidden_path)
+            self.assertEqual(excinfo.exception.status_code, 400)
 
         #Test Save behavior
         #Test save of file in hidden directory
@@ -253,76 +242,56 @@ def test_400(self):
 
         #Test rename behavior
         #Test rename with source file in hidden directory
-        with self.assertRaises(HTTPError) as excinfo:
-            with TemporaryDirectory() as td:
-                cm = FileContentsManager(root_dir=td)
-                hidden_dir = '.hidden'
-                file_in_hidden_path = os.path.join(hidden_dir,'visible.txt')
-                _make_dir(cm, hidden_dir)
-                model = cm.new(path=file_in_hidden_path)
-                old_path = cm._get_os_path(model['path'])
-                new_path = "new.txt"
+        with TemporaryDirectory() as td:
+            cm = FileContentsManager(root_dir=td)
+            hidden_dir = '.hidden'
+            file_in_hidden_path = os.path.join(hidden_dir,'visible.txt')
+            _make_dir(cm, hidden_dir)
+            old_path = file_in_hidden_path
+            new_path = "new.txt"
 
-                try:
-                    result = cm.rename_file(old_path, new_path)
-                except HTTPError as e:
-                    self.assertEqual(e.status_code, 400)
-                else:
-                    self.fail("Should have raised HTTPError(400)")
+            with self.assertRaises(HTTPError) as excinfo:
+                cm.rename_file(old_path, new_path)
+            self.assertEqual(excinfo.exception.status_code, 400)
 
         #Test rename of dest file in hidden directory
-        with self.assertRaises(HTTPError) as excinfo:
-            with TemporaryDirectory() as td:
-                cm = FileContentsManager(root_dir=td)
-                hidden_dir = '.hidden'
-                file_in_hidden_path = os.path.join(hidden_dir,'visible.txt')
-                _make_dir(cm, hidden_dir)
-                model = cm.new(path=file_in_hidden_path)
-                new_path = cm._get_os_path(model['path'])
-                old_path = "old.txt"
+        with TemporaryDirectory() as td:
+            cm = FileContentsManager(root_dir=td)
+            hidden_dir = '.hidden'
+            file_in_hidden_path = os.path.join(hidden_dir,'visible.txt')
+            _make_dir(cm, hidden_dir)
+            new_path = file_in_hidden_path
+            old_path = "old.txt"
 
-                try:
-                    result = cm.rename_file(old_path, new_path)
-                except HTTPError as e:
-                    self.assertEqual(e.status_code, 400)
-                else:
-                    self.fail("Should have raised HTTPError(400)")
+            with self.assertRaises(HTTPError) as excinfo:
+                cm.rename_file(old_path, new_path)
+            self.assertEqual(excinfo.exception.status_code, 400)
 
         #Test rename with hidden source file in visible directory
-        with self.assertRaises(HTTPError) as excinfo:
-            with TemporaryDirectory() as td:
-                cm = FileContentsManager(root_dir=td)
-                hidden_dir = 'visible'
-                file_in_hidden_path = os.path.join(hidden_dir,'.hidden.txt')
-                _make_dir(cm, hidden_dir)
-                model = cm.new(path=file_in_hidden_path)
-                old_path = cm._get_os_path(model['path'])
-                new_path = "new.txt"
+        with TemporaryDirectory() as td:
+            cm = FileContentsManager(root_dir=td)
+            hidden_dir = 'visible'
+            file_in_hidden_path = os.path.join(hidden_dir,'.hidden.txt')
+            _make_dir(cm, hidden_dir)
+            old_path = file_in_hidden_path
+            new_path = "new.txt"
 
-                try:
-                    result = cm.rename_file(old_path, new_path)
-                except HTTPError as e:
-                    self.assertEqual(e.status_code, 400)
-                else:
-                    self.fail("Should have raised HTTPError(400)")
+            with self.assertRaises(HTTPError) as excinfo:
+                cm.rename_file(old_path, new_path)
+            self.assertEqual(excinfo.exception.status_code, 400)
 
         #Test rename with hidden dest file in visible directory
-        with self.assertRaises(HTTPError) as excinfo:
-            with TemporaryDirectory() as td:
-                cm = FileContentsManager(root_dir=td)
-                hidden_dir = 'visible'
-                file_in_hidden_path = os.path.join(hidden_dir,'.hidden.txt')
-                _make_dir(cm, hidden_dir)
-                model = cm.new(path=file_in_hidden_path)
-                new_path = cm._get_os_path(model['path'])
-                old_path = "old.txt"
-
-                try:
-                    result = cm.rename_file(old_path, new_path)
-                except HTTPError as e:
-                    self.assertEqual(e.status_code, 400)
-                else:
-                    self.fail("Should have raised HTTPError(400)")
+        with TemporaryDirectory() as td:
+            cm = FileContentsManager(root_dir=td)
+            hidden_dir = 'visible'
+            file_in_hidden_path = os.path.join(hidden_dir,'.hidden.txt')
+            _make_dir(cm, hidden_dir)
+            new_path = file_in_hidden_path
+            old_path = "old.txt"
+
+            with self.assertRaises(HTTPError) as excinfo:
+                cm.rename_file(old_path, new_path)
+            self.assertEqual(excinfo.exception.status_code, 400)
 
     @skipIf(sys.platform.startswith('win'), "Can't test hidden files on Windows")
     def test_404(self):