diff --git a/changelog.d/20240712_155421_sirosen_experimental_tokenstorage_refactor.rst b/changelog.d/20240712_155421_sirosen_experimental_tokenstorage_refactor.rst new file mode 100644 index 000000000..2a3f30220 --- /dev/null +++ b/changelog.d/20240712_155421_sirosen_experimental_tokenstorage_refactor.rst @@ -0,0 +1,17 @@ +Changed +~~~~~~~ + +- The ``SQLiteTokenStorage`` component in ``globus_sdk.experimental`` has been + changed in several ways to improve its interface. (:pr:`NUMBER`) + + - ``:memory:`` is no longer accepted as a database name. Attempts to use it + will trigger errors directing users to use ``MemoryTokenStorage`` instead. + + - Parent directories for a target file are automatically created, and this + behavior is inherited from the ``FileTokenStorage`` base class. (This was + previously a feature only of the ``JSONTokenStorage``.) + + - The ``config_storage`` table has been removed from the generated database + schema, the schema version number has been incremented to ``2``, and + methods and parameters related to manipulation of ``config_storage`` have + been removed. diff --git a/src/globus_sdk/experimental/tokenstorage/base.py b/src/globus_sdk/experimental/tokenstorage/base.py index 28cb88397..ff8f08149 100644 --- a/src/globus_sdk/experimental/tokenstorage/base.py +++ b/src/globus_sdk/experimental/tokenstorage/base.py @@ -3,6 +3,7 @@ import abc import contextlib import os +import pathlib import typing as t from globus_sdk.services.auth import OAuthTokenResponse @@ -107,7 +108,20 @@ class FileTokenStorage(TokenStorage, metaclass=abc.ABCMeta): files. """ - filename: str + def __init__(self, filename: pathlib.Path | str, *, namespace: str = "DEFAULT"): + """ + :param filename: the name of the file to write to and read from + :param namespace: A user-supplied namespace for partitioning token data + """ + self.filename = str(filename) + self._ensure_containing_dir_exists() + super().__init__(namespace=namespace) + + def _ensure_containing_dir_exists(self) -> None: + """ + Ensure that the directory containing the given filename exists. + """ + os.makedirs(os.path.dirname(self.filename), exist_ok=True) def file_exists(self) -> bool: """ diff --git a/src/globus_sdk/experimental/tokenstorage/json.py b/src/globus_sdk/experimental/tokenstorage/json.py index d605d4067..5807e671c 100644 --- a/src/globus_sdk/experimental/tokenstorage/json.py +++ b/src/globus_sdk/experimental/tokenstorage/json.py @@ -1,8 +1,6 @@ from __future__ import annotations import json -import os -import pathlib import typing as t from globus_sdk.experimental.tokenstorage.base import FileTokenStorage @@ -35,21 +33,6 @@ class JSONTokenStorage(FileTokenStorage): # the supported versions (data not in these versions causes an error) supported_versions = ("1.0", "2.0") - def __init__(self, filename: pathlib.Path | str, *, namespace: str = "DEFAULT"): - """ - :param filename: the name of the file to write to and read from - :param namespace: A user-supplied namespace for partitioning token data - """ - self.filename = str(filename) - self._ensure_containing_dir_exists() - super().__init__(namespace=namespace) - - def _ensure_containing_dir_exists(self) -> None: - """ - Ensure that the directory containing the given filename exists. - """ - os.makedirs(os.path.dirname(self.filename), exist_ok=True) - def _invalid(self, msg: str) -> t.NoReturn: raise ValueError( f"{msg} while loading from '{self.filename}' for JSON Token Storage" diff --git a/src/globus_sdk/experimental/tokenstorage/sqlite.py b/src/globus_sdk/experimental/tokenstorage/sqlite.py index b01fc64be..be23b2f1a 100644 --- a/src/globus_sdk/experimental/tokenstorage/sqlite.py +++ b/src/globus_sdk/experimental/tokenstorage/sqlite.py @@ -6,6 +6,7 @@ import textwrap import typing as t +from globus_sdk import exc from globus_sdk.experimental.tokenstorage.base import FileTokenStorage from globus_sdk.version import __version__ @@ -28,48 +29,39 @@ class SQLiteTokenStorage(FileTokenStorage): def __init__( self, - dbname: pathlib.Path | str, + filename: pathlib.Path | str, *, connect_params: dict[str, t.Any] | None = None, namespace: str = "DEFAULT", ): """ - :param dbname: The name of the DB file to write to and read from. If the string - ":memory:" is used, an in-memory database will be used instead. + :param filename: The name of the DB file to write to and read from. :param connect_params: A pass-through dictionary for fine-tuning the SQLite connection. :param namespace: A user-supplied namespace for partitioning token data. """ - self.filename = self.dbname = str(dbname) - self._connection = self._init_and_connect(connect_params) - super().__init__(namespace=namespace) + if filename == ":memory:": + raise exc.GlobusSDKUsageError( + "SQLiteTokenStorage cannot be used with a ':memory:' database. " + "If you want to store tokens in memory, use MemoryTokenStorage instead." + ) - def _is_memory_db(self) -> bool: - return self.dbname == ":memory:" + super().__init__(filename, namespace=namespace) + self._connection = self._init_and_connect(connect_params) def _init_and_connect( self, connect_params: dict[str, t.Any] | None, ) -> sqlite3.Connection: - init_tables = self._is_memory_db() or not self.file_exists() connect_params = connect_params or {} - if init_tables and not self._is_memory_db(): # real file needs to be created + if not self.file_exists(): with self.user_only_umask(): conn: sqlite3.Connection = sqlite3.connect( - self.dbname, **connect_params + self.filename, **connect_params ) - else: - conn = sqlite3.connect(self.dbname, **connect_params) - if init_tables: conn.executescript( textwrap.dedent( """ - CREATE TABLE config_storage ( - namespace VARCHAR NOT NULL, - config_name VARCHAR NOT NULL, - config_data_json VARCHAR NOT NULL, - PRIMARY KEY (namespace, config_name) - ); CREATE TABLE token_storage ( namespace VARCHAR NOT NULL, resource_server VARCHAR NOT NULL, @@ -92,10 +84,18 @@ def _init_and_connect( "VALUES (?, ?)", [ ("globus-sdk.version", __version__), - ("globus-sdk.database_schema_version", "1"), + # schema_version=1 indicates a schema built with the original + # SQLiteAdapter + # schema_version=2 indicates one built with SQLiteTokenStorage + # + # a schema_version of 1 therefore indicates that there should be + # a 'config_storage' table present + ("globus-sdk.database_schema_version", "2"), ], ) conn.commit() + else: + conn = sqlite3.connect(self.filename, **connect_params) return conn def close(self) -> None: @@ -104,68 +104,12 @@ def close(self) -> None: """ self._connection.close() - def store_config( - self, config_name: str, config_dict: t.Mapping[str, t.Any] - ) -> None: - """ - Store a config dict under the current namespace in the config table. - Allows arbitrary configuration data to be namespaced under the namespace, so - that application config may be associated with the stored token data. - - Uses sqlite "REPLACE" to perform the operation. - - :param config_name: A string name for the configuration value - :param config_dict: A dict of config which will be stored serialized as JSON - """ - self._connection.execute( - "REPLACE INTO config_storage(namespace, config_name, config_data_json) " - "VALUES (?, ?, ?)", - (self.namespace, config_name, json.dumps(config_dict)), - ) - self._connection.commit() - - def read_config(self, config_name: str) -> dict[str, t.Any] | None: - """ - Load a config dict under the current namespace in the config table. - If no value is found, returns None - - :param config_name: A string name for the configuration value - """ - row = self._connection.execute( - "SELECT config_data_json FROM config_storage " - "WHERE namespace=? AND config_name=?", - (self.namespace, config_name), - ).fetchone() - - if row is None: - return None - config_data_json = row[0] - val = json.loads(config_data_json) - if not isinstance(val, dict): - raise ValueError("reading config data and got non-dict result") - return val - - def remove_config(self, config_name: str) -> bool: - """ - Delete a previously stored configuration value. - - Returns True if data was deleted, False if none was found to delete. - - :param config_name: A string name for the configuration value - """ - rowcount = self._connection.execute( - "DELETE FROM config_storage WHERE namespace=? AND config_name=?", - (self.namespace, config_name), - ).rowcount - self._connection.commit() - return rowcount != 0 - def store_token_data_by_resource_server( self, token_data_by_resource_server: dict[str, TokenData] ) -> None: """ Given a dict of token data indexed by resource server, convert the data into - JSON dicts and write it to ``self.dbname`` under the current namespace + JSON dicts and write it to ``self.filename`` under the current namespace :param token_data_by_resource_server: a ``dict`` of ``TokenData`` objects indexed by their ``resource_server``. @@ -219,17 +163,11 @@ def remove_token_data(self, resource_server: str) -> bool: self._connection.commit() return rowcount != 0 - def iter_namespaces( - self, *, include_config_namespaces: bool = False - ) -> t.Iterator[str]: + def iter_namespaces(self) -> t.Iterator[str]: """ Iterate over the namespaces which are in use in this storage adapter's database. The presence of tokens for a namespace does not indicate that those tokens are valid, only that they have been stored and have not been removed. - - :param include_config_namespaces: Include namespaces which appear only in the - configuration storage section of the sqlite database. By default, only - namespaces which were used for token storage will be returned """ seen: set[str] = set() for row in self._connection.execute( @@ -238,11 +176,3 @@ def iter_namespaces( namespace = row[0] seen.add(namespace) yield namespace - - if include_config_namespaces: - for row in self._connection.execute( - "SELECT DISTINCT namespace FROM config_storage;" - ): - namespace = row[0] - if namespace not in seen: - yield namespace diff --git a/tests/functional/tokenstorage_v2/test_sqlite_token_storage.py b/tests/functional/tokenstorage_v2/test_sqlite_token_storage.py index d9c9fe447..cbe0c04bf 100644 --- a/tests/functional/tokenstorage_v2/test_sqlite_token_storage.py +++ b/tests/functional/tokenstorage_v2/test_sqlite_token_storage.py @@ -1,5 +1,6 @@ import pytest +from globus_sdk import exc from globus_sdk.experimental.tokenstorage import SQLiteTokenStorage from globus_sdk.tokenstorage import SQLiteAdapter @@ -9,9 +10,6 @@ def db_file(tmp_path): return tmp_path / "test.db" -MEMORY_DBNAME = ":memory:" - - @pytest.fixture def adapters_to_close(): data = set() @@ -21,8 +19,10 @@ def adapters_to_close(): @pytest.fixture -def make_adapter(adapters_to_close): +def make_adapter(adapters_to_close, db_file): def func(*args, **kwargs): + if len(args) == 0 and "filename" not in kwargs: + args = (db_file,) ret = SQLiteTokenStorage(*args, **kwargs) adapters_to_close.add(ret) return ret @@ -30,46 +30,24 @@ def func(*args, **kwargs): return func -@pytest.mark.parametrize( - "success, use_file, kwargs", - [ - (False, False, {}), - (False, False, {"namespace": "foo"}), - (True, False, {"dbname": MEMORY_DBNAME}), - (True, False, {"dbname": MEMORY_DBNAME, "namespace": "foo"}), - (True, True, {}), - (True, True, {"namespace": "foo"}), - (False, True, {"dbname": MEMORY_DBNAME}), - (False, True, {"dbname": MEMORY_DBNAME, "namespace": "foo"}), - ], -) -def test_constructor(success, use_file, kwargs, db_file, make_adapter): - if success: - if use_file: - make_adapter(db_file, **kwargs) - else: - make_adapter(**kwargs) - else: - with pytest.raises(TypeError): - if use_file: - make_adapter(db_file, **kwargs) - else: - make_adapter(**kwargs) - - -def test_store_and_retrieve_simple_config(make_adapter): - adapter = make_adapter(MEMORY_DBNAME) - store_val = {"val1": True, "val2": None, "val3": 1.4} - adapter.store_config("myconf", store_val) - read_val = adapter.read_config("myconf") - assert read_val == store_val - assert read_val is not store_val +@pytest.mark.parametrize("kwargs", [{}, {"namespace": "foo"}]) +def test_constructor(kwargs, db_file, make_adapter): + make_adapter(db_file, **kwargs) + assert db_file.exists() + + +def test_constructor_rejects_memory_db(make_adapter): + with pytest.raises( + exc.GlobusSDKUsageError, + match="SQLiteTokenStorage cannot be used with a ':memory:' database", + ): + make_adapter(":memory:") def test_store_and_get_token_data_by_resource_server( mock_token_data_by_resource_server, make_adapter ): - adapter = make_adapter(MEMORY_DBNAME) + adapter = make_adapter() adapter.store_token_data_by_resource_server(mock_token_data_by_resource_server) gotten = adapter.get_token_data_by_resource_server() @@ -101,19 +79,14 @@ def test_multiple_adapters_store_and_retrieve_different_namespaces( assert data == {} -def test_load_missing_config_data(make_adapter): - adapter = make_adapter(MEMORY_DBNAME) - assert adapter.read_config("foo") is None - - def test_load_missing_token_data(make_adapter): - adapter = make_adapter(MEMORY_DBNAME) + adapter = make_adapter() assert adapter.get_token_data_by_resource_server() == {} assert adapter.get_token_data("resource_server_1") is None def test_remove_tokens(mock_response, make_adapter): - adapter = make_adapter(MEMORY_DBNAME) + adapter = make_adapter() adapter.store_token_response(mock_response) removed = adapter.remove_token_data("resource_server_1") @@ -125,55 +98,22 @@ def test_remove_tokens(mock_response, make_adapter): assert not removed -def test_remove_config(make_adapter): - adapter = make_adapter(MEMORY_DBNAME) - store_val = {"val1": True, "val2": None, "val3": 1.4} - adapter.store_config("myconf", store_val) - adapter.store_config("myconf2", store_val) - removed = adapter.remove_config("myconf") - assert removed - read_val = adapter.read_config("myconf") - assert read_val is None - read_val = adapter.read_config("myconf2") - assert read_val == store_val - - removed = adapter.remove_config("myconf") - assert not removed - - def test_iter_namespaces(mock_response, db_file, make_adapter): foo_adapter = make_adapter(db_file, namespace="foo") bar_adapter = make_adapter(db_file, namespace="bar") - baz_adapter = make_adapter(db_file, namespace="baz") - for adapter in [foo_adapter, bar_adapter, baz_adapter]: + for adapter in [foo_adapter, bar_adapter]: assert list(adapter.iter_namespaces()) == [] - assert list(adapter.iter_namespaces(include_config_namespaces=True)) == [] foo_adapter.store_token_response(mock_response) - for adapter in [foo_adapter, bar_adapter, baz_adapter]: + for adapter in [foo_adapter, bar_adapter]: assert list(adapter.iter_namespaces()) == ["foo"] - assert list(adapter.iter_namespaces(include_config_namespaces=True)) == ["foo"] bar_adapter.store_token_response(mock_response) - for adapter in [foo_adapter, bar_adapter, baz_adapter]: - assert set(adapter.iter_namespaces()) == {"foo", "bar"} - assert set(adapter.iter_namespaces(include_config_namespaces=True)) == { - "foo", - "bar", - } - - baz_adapter.store_config("some_conf", {}) - - for adapter in [foo_adapter, bar_adapter, baz_adapter]: + for adapter in [foo_adapter, bar_adapter]: assert set(adapter.iter_namespaces()) == {"foo", "bar"} - assert set(adapter.iter_namespaces(include_config_namespaces=True)) == { - "foo", - "bar", - "baz", - } def test_backwards_compatible_storage(mock_response, db_file, make_adapter):