Update the unit test's comments with pointers to the relevant issues

Zsailer · Zsailer · commit 36d44b7d0c4a · 2024-04-12T09:38:19.000-07:00
diff --git a/jupyter_server_fileid/manager.py b/jupyter_server_fileid/manager.py
@@ -121,10 +121,7 @@ def _move_recursive(self, old_path: str, new_path: str, path_mgr: Any = os.path)
         for record in records:
             id, old_recpath = record
             new_recpath = path_mgr.join(new_path, path_mgr.relpath(old_recpath, start=old_path))
-            try:
-                self.con.execute("UPDATE Files SET path = ? WHERE id = ?", (new_recpath, id))
-            except Exception as err:
-                self.log.error(err)
+            self.con.execute("UPDATE Files SET path = ? WHERE id = ?", (new_recpath, id))
 
     def _copy_recursive(self, from_path: str, to_path: str, path_mgr: Any = os.path) -> None:
         """Copy all children of a given directory at `from_path` to a new
@@ -137,12 +134,9 @@ def _copy_recursive(self, from_path: str, to_path: str, path_mgr: Any = os.path)
         for record in records:
             (from_recpath,) = record
             to_recpath = path_mgr.join(to_path, path_mgr.relpath(from_recpath, start=from_path))
-            try:
-                self.con.execute(
-                    "INSERT INTO Files (id, path) VALUES (?, ?)", (self._uuid(), to_recpath)
-                )
-            except Exception as err:
-                self.log.error(err)
+            self.con.execute(
+                "INSERT INTO Files (id, path) VALUES (?, ?)", (self._uuid(), to_recpath)
+            )
 
     def _delete_recursive(self, path: str, path_mgr: Any = os.path) -> None:
         """Delete all children of a given directory, delimited by `sep`."""
@@ -319,24 +313,21 @@ def _from_normalized_path(self, path: Optional[str]) -> Optional[str]:
     def _create(self, path: str) -> str:
         path = self._normalize_path(path)
         id = self._uuid()
-        try:
-            self.con.execute("INSERT INTO Files (id, path) VALUES (?, ?)", (id, path))
-        except Exception as err:
-            self.log.error(err)
+        self.con.execute("INSERT INTO Files (id, path) VALUES (?, ?)", (id, path))
         return id
 
     def index(self, path: str) -> str:
-        path = self._normalize_path(path)
-        row = self.con.execute("SELECT id FROM Files WHERE path = ?", (path,)).fetchone()
-        existing_id = row and row[0]
+        with self.con:
+            path = self._normalize_path(path)
+            row = self.con.execute("SELECT id FROM Files WHERE path = ?", (path,)).fetchone()
+            existing_id = row and row[0]
 
-        if existing_id:
-            return existing_id
+            if existing_id:
+                return existing_id
 
-        # create new record
-        id = self._create(path)
-        self.con.commit()
-        return id
+            # create new record
+            id = self._create(path)
+            return id
 
     def get_id(self, path: str) -> Optional[str]:
         path = self._normalize_path(path)
@@ -349,40 +340,36 @@ def get_path(self, id: str) -> Optional[str]:
         return self._from_normalized_path(path)
 
     def move(self, old_path: str, new_path: str) -> None:
-        old_path = self._normalize_path(old_path)
-        new_path = self._normalize_path(new_path)
-        row = self.con.execute("SELECT id FROM Files WHERE path = ?", (old_path,)).fetchone()
-        id = row and row[0]
+        with self.con:
+            old_path = self._normalize_path(old_path)
+            new_path = self._normalize_path(new_path)
+            row = self.con.execute("SELECT id FROM Files WHERE path = ?", (old_path,)).fetchone()
+            id = row and row[0]
 
-        if id:
-            try:
+            if id:
                 self.con.execute("UPDATE Files SET path = ? WHERE path = ?", (new_path, old_path))
-            except Exception as err:
-                self.log.error(err)
-            self._move_recursive(old_path, new_path, posixpath)
-        else:
-            id = self._create(new_path)
+                self._move_recursive(old_path, new_path, posixpath)
+            else:
+                id = self._create(new_path)
 
-        self.con.commit()
-        return id
+            return id
 
     def copy(self, from_path: str, to_path: str) -> Optional[str]:
-        from_path = self._normalize_path(from_path)
-        to_path = self._normalize_path(to_path)
+        with self.con:
+            from_path = self._normalize_path(from_path)
+            to_path = self._normalize_path(to_path)
 
-        id = self._create(to_path)
-        self._copy_recursive(from_path, to_path, posixpath)
+            id = self._create(to_path)
+            self._copy_recursive(from_path, to_path, posixpath)
 
-        self.con.commit()
-        return id
+            return id
 
     def delete(self, path: str) -> None:
-        path = self._normalize_path(path)
+        with self.con:
+            path = self._normalize_path(path)
 
-        self.con.execute("DELETE FROM Files WHERE path = ?", (path,))
-        self._delete_recursive(path, posixpath)
-
-        self.con.commit()
+            self.con.execute("DELETE FROM Files WHERE path = ?", (path,))
+            self._delete_recursive(path, posixpath)
 
     def save(self, path: str) -> None:
         return
@@ -685,13 +672,10 @@ def _create(self, path, stat_info):
         have a unique `ino`.
         """
         id = self._uuid()
-        try:
-            self.con.execute(
-                "INSERT INTO Files (id, path, ino, crtime, mtime, is_dir) VALUES (?, ?, ?, ?, ?, ?)",
-                (id, path, stat_info.ino, stat_info.crtime, stat_info.mtime, stat_info.is_dir),
-            )
-        except Exception as err: 
-            self.log.error(err)
+        self.con.execute(
+            "INSERT INTO Files (id, path, ino, crtime, mtime, is_dir) VALUES (?, ?, ?, ?, ?, ?)",
+            (id, path, stat_info.ino, stat_info.crtime, stat_info.mtime, stat_info.is_dir),
+        )
         return id
 
     def _update(self, id, stat_info=None, path=None):
@@ -709,66 +693,62 @@ def _update(self, id, stat_info=None, path=None):
         dangerous and may throw a runtime error if the file is not guaranteed to
         have a unique `ino`.
         """
-        try:
-            if stat_info and path:
-                self.con.execute(
-                    "UPDATE Files SET ino = ?, crtime = ?, mtime = ?, path = ? WHERE id = ?",
-                    (stat_info.ino, stat_info.crtime, stat_info.mtime, path, id),
-                )
-                return
+        if stat_info and path:
+            self.con.execute(
+                "UPDATE Files SET ino = ?, crtime = ?, mtime = ?, path = ? WHERE id = ?",
+                (stat_info.ino, stat_info.crtime, stat_info.mtime, path, id),
+            )
+            return
 
-            if stat_info:
-                self.con.execute(
-                    "UPDATE Files SET ino = ?, crtime = ?, mtime = ? WHERE id = ?",
-                    (stat_info.ino, stat_info.crtime, stat_info.mtime, id),
-                )
-                return
+        if stat_info:
+            self.con.execute(
+                "UPDATE Files SET ino = ?, crtime = ?, mtime = ? WHERE id = ?",
+                (stat_info.ino, stat_info.crtime, stat_info.mtime, id),
+            )
+            return
 
-            if path:
-                self.con.execute(
-                    "UPDATE Files SET path = ? WHERE id = ?",
-                    (path, id),
-                )
-                return
-        except Exception as err:
-            self.log.error(err)
+        if path:
+            self.con.execute(
+                "UPDATE Files SET path = ? WHERE id = ?",
+                (path, id),
+            )
+            return
 
     def index(self, path, stat_info=None, commit=True):
         """Returns the file ID for the file at `path`, creating a new file ID if
         one does not exist. Returns None only if file does not exist at path."""
-        path = self._normalize_path(path)
-        stat_info = stat_info or self._stat(path)
-        if not stat_info:
-            return None
+        with self.con:
+            path = self._normalize_path(path)
+            stat_info = stat_info or self._stat(path)
+            if not stat_info:
+                return None
 
-        # if file is symlink, then index the path it refers to instead
-        if stat_info.is_symlink:
-            return self.index(os.path.realpath(path))
+            # if file is symlink, then index the path it refers to instead
+            if stat_info.is_symlink:
+                return self.index(os.path.realpath(path))
 
-        # sync file at path and return file ID if it exists
-        id = self._sync_file(path, stat_info)
-        if id is not None:
-            return id
+            # sync file at path and return file ID if it exists
+            id = self._sync_file(path, stat_info)
+            if id is not None:
+                return id
 
-        # otherwise, create a new record and return the file ID
-        id = self._create(path, stat_info)
-        if commit:
-            self.con.commit()
-        return id
+            # otherwise, create a new record and return the file ID
+            id = self._create(path, stat_info)
+            return id
 
     def get_id(self, path):
         """Retrieves the file ID associated with a file path. Returns None if
         the file has not yet been indexed or does not exist at the given
         path."""
-        path = self._normalize_path(path)
-        stat_info = self._stat(path)
-        if not stat_info:
-            return None
+        with self.con:
+            path = self._normalize_path(path)
+            stat_info = self._stat(path)
+            if not stat_info:
+                return None
 
-        # then sync file at path and retrieve id, if any
-        id = self._sync_file(path, stat_info)
-        self.con.commit()
-        return id
+            # then sync file at path and retrieve id, if any
+            id = self._sync_file(path, stat_info)
+            return id
 
     def get_path(self, id):
         """Retrieves the file path associated with a file ID. The file path is
@@ -813,26 +793,26 @@ def get_path(self, id):
     def move(self, old_path, new_path):
         """Handles file moves by updating the file path of the associated file
         ID.  Returns the file ID. Returns None if file does not exist at new_path."""
-        old_path = self._normalize_path(old_path)
-        new_path = self._normalize_path(new_path)
+        with self.con:
+            old_path = self._normalize_path(old_path)
+            new_path = self._normalize_path(new_path)
 
-        # verify file exists at new_path
-        stat_info = self._stat(new_path)
-        if stat_info is None:
-            return None
+            # verify file exists at new_path
+            stat_info = self._stat(new_path)
+            if stat_info is None:
+                return None
 
-        # sync the file and see if it was already indexed
-        #
-        # originally this method did not call _sync_file() for performance
-        # reasons, but this is needed to handle an edge case:
-        # https://github.com/jupyter-server/jupyter_server_fileid/issues/62
-        id = self._sync_file(new_path, stat_info)
-        if id is None:
-            # if no existing record, create a new one
-            id = self._create(new_path, stat_info)
+            # sync the file and see if it was already indexed
+            #
+            # originally this method did not call _sync_file() for performance
+            # reasons, but this is needed to handle an edge case:
+            # https://github.com/jupyter-server/jupyter_server_fileid/issues/62
+            id = self._sync_file(new_path, stat_info)
+            if id is None:
+                # if no existing record, create a new one
+                id = self._create(new_path, stat_info)
 
-        self.con.commit()
-        return id
+            return id
 
     def _copy_recursive(self, from_path: str, to_path: str, _: str = "") -> None:
         """Copy all children of a given directory at `from_path` to a new
@@ -877,13 +857,13 @@ def copy(self, from_path, to_path):
     def delete(self, path):
         """Handles file deletions by deleting the associated record in the File
         table. Returns None."""
-        path = self._normalize_path(path)
+        with self.con:
+            path = self._normalize_path(path)
 
-        if os.path.isdir(path):
-            self._delete_recursive(path)
+            if os.path.isdir(path):
+                self._delete_recursive(path)
 
-        self.con.execute("DELETE FROM Files WHERE path = ?", (path,))
-        self.con.commit()
+            self.con.execute("DELETE FROM Files WHERE path = ?", (path,))
 
     def save(self, path):
         """Handles file saves (edits) by updating recorded stat info.
@@ -896,21 +876,21 @@ def save(self, path):
         JupyterLab.  This would (wrongly) preserve the association b/w the old
         file ID and the current path rather than create a new file ID.
         """
-        path = self._normalize_path(path)
+        with self.con:
+            path = self._normalize_path(path)
 
-        # look up record by ino and path
-        stat_info = self._stat(path)
-        row = self.con.execute(
-            "SELECT id FROM Files WHERE ino = ? AND path = ?", (stat_info.ino, path)
-        ).fetchone()
-        # if no record exists, return early
-        if row is None:
-            return
+            # look up record by ino and path
+            stat_info = self._stat(path)
+            row = self.con.execute(
+                "SELECT id FROM Files WHERE ino = ? AND path = ?", (stat_info.ino, path)
+            ).fetchone()
+            # if no record exists, return early
+            if row is None:
+                return
 
-        # otherwise, update the stat info
-        (id,) = row
-        self._update(id, stat_info)
-        self.con.commit()
+            # otherwise, update the stat info
+            (id,) = row
+            self._update(id, stat_info)
 
     def get_handlers_by_action(self) -> Dict[str, Optional[Callable[[Dict[str, Any]], Any]]]:
         return {
diff --git a/tests/test_manager.py b/tests/test_manager.py
@@ -15,7 +15,6 @@
     LocalFileIdManager,
 )
 
-
 @pytest.fixture
 def test_path(fs_helpers):
     path = "test_path"
@@ -621,7 +620,13 @@ def test_db_journal_mode(any_fid_manager_class, fid_db_path, jp_root_dir, db_jou
         assert actual_journal_mode[0].upper() == expected_journal_mode
 
 
-
+# This test demonstrates an issue raised in 
+# https://github.com/jupyter-server/jupyter_server_fileid/pull/76
+# which was later fixed in 
+# https://github.com/jupyter-server/jupyter_server_fileid/pull/77
+#
+# We use this unit test to catch this edge case and ensure
+# its covered going forward.
 def test_multiple_fileIdManager_connections_after_exception(fid_db_path):
     original_file_path = "/path/to/file"
     copy_location = "/path/to/copy"
@@ -635,15 +640,21 @@ def test_multiple_fileIdManager_connections_after_exception(fid_db_path):
     # Copy the file
     manager_1.copy(original_file_path, copy_location)
     # Try copying the file again.
+    excepted = False
     try:
         manager_1.copy(original_file_path, copy_location)
     # We expect this to fail because the file is already in the database.
     except sqlite3.IntegrityError:
+        excepted = True
         pass
 
-    # Now the database is locked and no other connections can be made.
-    # Start a second connection to the database and demonstrate
-    # that the database is now stuck in a locked state.
+    assert excepted, "Copying to the same location should raise an exception, but it did not here."
+
+    # Previously, these actions locked the database for future connections.
+    # This was fixed in: https://github.com/jupyter-server/jupyter_server_fileid/pull/77
+
+    # Try making a second connection that writes to the DB and
+    # make sure no exceptions were raised.
     manager_2 = ArbitraryFileIdManager(db_path=fid_db_path)
     manager_2.copy(original_file_path, another_copy_location)
 
