From 3812cf5abf559b4cf0577e8b79efea620dd24020 Mon Sep 17 00:00:00 2001 From: Zach Sailer Date: Tue, 16 Nov 2021 16:40:13 -0800 Subject: [PATCH 1/6] allow the session manager to persist on disk --- jupyter_server/services/sessions/sessionmanager.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/jupyter_server/services/sessions/sessionmanager.py b/jupyter_server/services/sessions/sessionmanager.py index bc441a5567..79b7aceb89 100644 --- a/jupyter_server/services/sessions/sessionmanager.py +++ b/jupyter_server/services/sessions/sessionmanager.py @@ -13,6 +13,7 @@ from traitlets.config.configurable import LoggingConfigurable from traitlets import Instance +from traitlets import Unicode from jupyter_server.utils import ensure_async from jupyter_server.traittypes import InstanceFromClasses @@ -20,6 +21,11 @@ class SessionManager(LoggingConfigurable): + database_path = Unicode( + default_value=":memory:", + help="Filesystem path to SQLite Database file. Does not write to file by default." + ).tag(config=True) + kernel_manager = Instance("jupyter_server.services.kernels.kernelmanager.MappingKernelManager") contents_manager = InstanceFromClasses( [ @@ -39,7 +45,7 @@ def cursor(self): if self._cursor is None: self._cursor = self.connection.cursor() self._cursor.execute( - """CREATE TABLE session + """CREATE TABLE IF NOT EXISTS session (session_id, path, name, type, kernel_id)""" ) return self._cursor @@ -48,7 +54,8 @@ def cursor(self): def connection(self): """Start a database connection""" if self._connection is None: - self._connection = sqlite3.connect(":memory:") + # Set isolation level to None to autocommit all changes to the database. + self._connection = sqlite3.connect(self.database_path, isolation_level=None) self._connection.row_factory = sqlite3.Row return self._connection From a5e1e864a359ad4be3783234d5cbe001136c5888 Mon Sep 17 00:00:00 2001 From: Zach Sailer Date: Mon, 6 Dec 2021 10:41:26 -0800 Subject: [PATCH 2/6] validate the filepath given to a persistent session manager and add unit tests --- .../services/sessions/sessionmanager.py | 35 ++++++- .../tests/services/sessions/test_manager.py | 97 +++++++++++++++++++ 2 files changed, 129 insertions(+), 3 deletions(-) diff --git a/jupyter_server/services/sessions/sessionmanager.py b/jupyter_server/services/sessions/sessionmanager.py index 79b7aceb89..95c407d87b 100644 --- a/jupyter_server/services/sessions/sessionmanager.py +++ b/jupyter_server/services/sessions/sessionmanager.py @@ -1,7 +1,9 @@ """A base class session manager.""" # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. +import os import uuid +import pathlib try: import sqlite3 @@ -14,6 +16,8 @@ from traitlets.config.configurable import LoggingConfigurable from traitlets import Instance from traitlets import Unicode +from traitlets import validate +from traitlets import TraitError from jupyter_server.utils import ensure_async from jupyter_server.traittypes import InstanceFromClasses @@ -21,11 +25,36 @@ class SessionManager(LoggingConfigurable): - database_path = Unicode( + database_filepath = Unicode( default_value=":memory:", - help="Filesystem path to SQLite Database file. Does not write to file by default." + help=( + "Filesystem path to SQLite Database file. Does " + "not write to file by default. :memory: (default) stores the " + "database in-memory and is not persistent beyond " + "the current session." + ) ).tag(config=True) + @validate("database_filepath") + def _validate_database_filepath(self, proposal): + value = proposal["value"] + if value == ":memory:": + return value + path = pathlib.Path(value) + if path.exists(): + # Verify that the database path is not a directory. + if path.is_dir(): + raise TraitError("`database_filepath` expected a file path, but the given path is a directory.") + # If the file exists, but it's empty, its a valid entry. + if os.stat(path).st_size == 0: + return value + # Verify that database path is an SQLite 3 Database by checking its header. + with open(value, "rb") as f: + header = f.read(100) + if not header.startswith(b'SQLite format 3'): + raise TraitError("The file does not look like ") + return value + kernel_manager = Instance("jupyter_server.services.kernels.kernelmanager.MappingKernelManager") contents_manager = InstanceFromClasses( [ @@ -55,7 +84,7 @@ def connection(self): """Start a database connection""" if self._connection is None: # Set isolation level to None to autocommit all changes to the database. - self._connection = sqlite3.connect(self.database_path, isolation_level=None) + self._connection = sqlite3.connect(self.database_filepath, isolation_level=None) self._connection.row_factory = sqlite3.Row return self._connection diff --git a/jupyter_server/tests/services/sessions/test_manager.py b/jupyter_server/tests/services/sessions/test_manager.py index 97af3175c4..9d25014975 100644 --- a/jupyter_server/tests/services/sessions/test_manager.py +++ b/jupyter_server/tests/services/sessions/test_manager.py @@ -1,5 +1,7 @@ import pytest +import pathlib from tornado import web +from traitlets import TraitError from jupyter_server._tz import isoformat from jupyter_server._tz import utcnow @@ -264,3 +266,98 @@ async def test_bad_delete_session(session_manager): await session_manager.delete_session(bad_kwarg="23424") # Bad keyword with pytest.raises(web.HTTPError): await session_manager.delete_session(session_id="23424") # nonexistent + + + +async def test_bad_database_filepath(jp_runtime_dir): + kernel_manager = DummyMKM() + + # Try to write to a path that's a directory, not a file. + path_id_directory = str(jp_runtime_dir) + with pytest.raises(TraitError) as err: + SessionManager( + kernel_manager=kernel_manager, + contents_manager=ContentsManager(), + database_filepath=str(path_id_directory) + ) + + # Try writing to file that's not a database + non_db_file = jp_runtime_dir.joinpath("non_db_file.db") + non_db_file.write_bytes(b"this is a bad file") + + with pytest.raises(TraitError) as err: + SessionManager( + kernel_manager=kernel_manager, + contents_manager=ContentsManager(), + database_filepath=str(non_db_file) + ) + + +async def test_good_database_filepath(jp_runtime_dir): + kernel_manager = DummyMKM() + + # Try writing to an empty file. + empty_file = jp_runtime_dir.joinpath("empty.db") + empty_file.write_bytes(b"") + + session_manager = SessionManager( + kernel_manager=kernel_manager, + contents_manager=ContentsManager(), + database_filepath=str(empty_file) + ) + + await session_manager.create_session( + path="/path/to/test.ipynb", kernel_name="python", type="notebook" + ) + # Assert that the database file exists + assert empty_file.exists() + + # Close the current session manager + del session_manager + + # Try writing to a file that already exists. + session_manager = SessionManager( + kernel_manager=kernel_manager, + contents_manager=ContentsManager(), + database_filepath=str(empty_file) + ) + + assert session_manager.database_filepath == str(empty_file) + +async def test_session_persistence(jp_runtime_dir): + session_db_path = jp_runtime_dir.joinpath("test-session.db") + # Kernel manager needs to persist. + kernel_manager = DummyMKM() + + # Initialize a session and start a connection. + # This should create the session database the first time. + session_manager = SessionManager( + kernel_manager=kernel_manager, + contents_manager=ContentsManager(), + database_filepath=str(session_db_path) + ) + + session = await session_manager.create_session( + path="/path/to/test.ipynb", kernel_name="python", type="notebook" + ) + + # Assert that the database file exists + assert session_db_path.exists() + + with open(session_db_path, "rb") as f: + header = f.read(100) + + assert header.startswith(b'SQLite format 3') + + # Close the current session manager + del session_manager + + # Get a new session_manager + session_manager = SessionManager( + kernel_manager=kernel_manager, + contents_manager=ContentsManager(), + database_filepath=str(session_db_path) + ) + + # Assert that the session database persists. + session = await session_manager.get_session(session_id=session["id"]) From 2c289bb06b5c3b8f31827f3523dccac3693206ac Mon Sep 17 00:00:00 2001 From: Zach Sailer Date: Mon, 6 Dec 2021 10:49:31 -0800 Subject: [PATCH 3/6] update help string --- jupyter_server/services/sessions/sessionmanager.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jupyter_server/services/sessions/sessionmanager.py b/jupyter_server/services/sessions/sessionmanager.py index 95c407d87b..e73f580006 100644 --- a/jupyter_server/services/sessions/sessionmanager.py +++ b/jupyter_server/services/sessions/sessionmanager.py @@ -28,10 +28,10 @@ class SessionManager(LoggingConfigurable): database_filepath = Unicode( default_value=":memory:", help=( - "Filesystem path to SQLite Database file. Does " - "not write to file by default. :memory: (default) stores the " - "database in-memory and is not persistent beyond " - "the current session." + "Th filesystem path to SQLite Database file " + "(e.g. /path/to/session_database.db). By default, the session " + "database is stored in-memory (i.e. `:memory:` setting from sqlite3) " + "and does not persist when the current Jupyter Server shuts down." ) ).tag(config=True) From fe3658df996cf830d8e2457e869b37f871bd980e Mon Sep 17 00:00:00 2001 From: Zach Sailer Date: Mon, 6 Dec 2021 10:52:46 -0800 Subject: [PATCH 4/6] finish comments in unit test --- jupyter_server/tests/services/sessions/test_manager.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/jupyter_server/tests/services/sessions/test_manager.py b/jupyter_server/tests/services/sessions/test_manager.py index 9d25014975..e85ad2dbdb 100644 --- a/jupyter_server/tests/services/sessions/test_manager.py +++ b/jupyter_server/tests/services/sessions/test_manager.py @@ -274,6 +274,7 @@ async def test_bad_database_filepath(jp_runtime_dir): # Try to write to a path that's a directory, not a file. path_id_directory = str(jp_runtime_dir) + # Should raise an error because the path is a directory. with pytest.raises(TraitError) as err: SessionManager( kernel_manager=kernel_manager, @@ -281,10 +282,12 @@ async def test_bad_database_filepath(jp_runtime_dir): database_filepath=str(path_id_directory) ) - # Try writing to file that's not a database + # Try writing to file that's not a valid SQLite 3 database file. non_db_file = jp_runtime_dir.joinpath("non_db_file.db") non_db_file.write_bytes(b"this is a bad file") + # Should raise an error because the file doesn't + # start with an SQLite database file header. with pytest.raises(TraitError) as err: SessionManager( kernel_manager=kernel_manager, From fb4f33c11f2cfb2ac9d295130c87218bc3c00e5f Mon Sep 17 00:00:00 2001 From: Zach Sailer Date: Mon, 6 Dec 2021 11:04:12 -0800 Subject: [PATCH 5/6] precommit cleanup --- .../services/sessions/sessionmanager.py | 10 +++++---- .../tests/services/sessions/test_manager.py | 21 +++++++++---------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/jupyter_server/services/sessions/sessionmanager.py b/jupyter_server/services/sessions/sessionmanager.py index e73f580006..8d736f3b93 100644 --- a/jupyter_server/services/sessions/sessionmanager.py +++ b/jupyter_server/services/sessions/sessionmanager.py @@ -2,8 +2,8 @@ # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. import os -import uuid import pathlib +import uuid try: import sqlite3 @@ -32,7 +32,7 @@ class SessionManager(LoggingConfigurable): "(e.g. /path/to/session_database.db). By default, the session " "database is stored in-memory (i.e. `:memory:` setting from sqlite3) " "and does not persist when the current Jupyter Server shuts down." - ) + ), ).tag(config=True) @validate("database_filepath") @@ -44,14 +44,16 @@ def _validate_database_filepath(self, proposal): if path.exists(): # Verify that the database path is not a directory. if path.is_dir(): - raise TraitError("`database_filepath` expected a file path, but the given path is a directory.") + raise TraitError( + "`database_filepath` expected a file path, but the given path is a directory." + ) # If the file exists, but it's empty, its a valid entry. if os.stat(path).st_size == 0: return value # Verify that database path is an SQLite 3 Database by checking its header. with open(value, "rb") as f: header = f.read(100) - if not header.startswith(b'SQLite format 3'): + if not header.startswith(b"SQLite format 3"): raise TraitError("The file does not look like ") return value diff --git a/jupyter_server/tests/services/sessions/test_manager.py b/jupyter_server/tests/services/sessions/test_manager.py index e85ad2dbdb..3ca8da8df1 100644 --- a/jupyter_server/tests/services/sessions/test_manager.py +++ b/jupyter_server/tests/services/sessions/test_manager.py @@ -1,5 +1,4 @@ import pytest -import pathlib from tornado import web from traitlets import TraitError @@ -268,7 +267,6 @@ async def test_bad_delete_session(session_manager): await session_manager.delete_session(session_id="23424") # nonexistent - async def test_bad_database_filepath(jp_runtime_dir): kernel_manager = DummyMKM() @@ -276,10 +274,10 @@ async def test_bad_database_filepath(jp_runtime_dir): path_id_directory = str(jp_runtime_dir) # Should raise an error because the path is a directory. with pytest.raises(TraitError) as err: - SessionManager( + SessionManager( kernel_manager=kernel_manager, contents_manager=ContentsManager(), - database_filepath=str(path_id_directory) + database_filepath=str(path_id_directory), ) # Try writing to file that's not a valid SQLite 3 database file. @@ -289,10 +287,10 @@ async def test_bad_database_filepath(jp_runtime_dir): # Should raise an error because the file doesn't # start with an SQLite database file header. with pytest.raises(TraitError) as err: - SessionManager( + SessionManager( kernel_manager=kernel_manager, contents_manager=ContentsManager(), - database_filepath=str(non_db_file) + database_filepath=str(non_db_file), ) @@ -306,7 +304,7 @@ async def test_good_database_filepath(jp_runtime_dir): session_manager = SessionManager( kernel_manager=kernel_manager, contents_manager=ContentsManager(), - database_filepath=str(empty_file) + database_filepath=str(empty_file), ) await session_manager.create_session( @@ -322,11 +320,12 @@ async def test_good_database_filepath(jp_runtime_dir): session_manager = SessionManager( kernel_manager=kernel_manager, contents_manager=ContentsManager(), - database_filepath=str(empty_file) + database_filepath=str(empty_file), ) assert session_manager.database_filepath == str(empty_file) + async def test_session_persistence(jp_runtime_dir): session_db_path = jp_runtime_dir.joinpath("test-session.db") # Kernel manager needs to persist. @@ -337,7 +336,7 @@ async def test_session_persistence(jp_runtime_dir): session_manager = SessionManager( kernel_manager=kernel_manager, contents_manager=ContentsManager(), - database_filepath=str(session_db_path) + database_filepath=str(session_db_path), ) session = await session_manager.create_session( @@ -350,7 +349,7 @@ async def test_session_persistence(jp_runtime_dir): with open(session_db_path, "rb") as f: header = f.read(100) - assert header.startswith(b'SQLite format 3') + assert header.startswith(b"SQLite format 3") # Close the current session manager del session_manager @@ -359,7 +358,7 @@ async def test_session_persistence(jp_runtime_dir): session_manager = SessionManager( kernel_manager=kernel_manager, contents_manager=ContentsManager(), - database_filepath=str(session_db_path) + database_filepath=str(session_db_path), ) # Assert that the session database persists. From 425632ead3ea4a6e7f9a8cace87e398d29b2744d Mon Sep 17 00:00:00 2001 From: Zach Sailer Date: Mon, 6 Dec 2021 13:26:16 -0800 Subject: [PATCH 6/6] allow empty session datbase file --- jupyter_server/services/sessions/sessionmanager.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/jupyter_server/services/sessions/sessionmanager.py b/jupyter_server/services/sessions/sessionmanager.py index 8d736f3b93..dbf4faad6f 100644 --- a/jupyter_server/services/sessions/sessionmanager.py +++ b/jupyter_server/services/sessions/sessionmanager.py @@ -1,7 +1,6 @@ """A base class session manager.""" # Copyright (c) Jupyter Development Team. # Distributed under the terms of the Modified BSD License. -import os import pathlib import uuid @@ -47,14 +46,12 @@ def _validate_database_filepath(self, proposal): raise TraitError( "`database_filepath` expected a file path, but the given path is a directory." ) - # If the file exists, but it's empty, its a valid entry. - if os.stat(path).st_size == 0: - return value # Verify that database path is an SQLite 3 Database by checking its header. with open(value, "rb") as f: header = f.read(100) - if not header.startswith(b"SQLite format 3"): - raise TraitError("The file does not look like ") + + if not header.startswith(b"SQLite format 3") and not header == b"": + raise TraitError("The given file is not an SQLite database file.") return value kernel_manager = Instance("jupyter_server.services.kernels.kernelmanager.MappingKernelManager")