Merge pull request #1004 from sirosen/experimental-tokenstorage-refactor

Refactor experimental SQLiteTokenStorage ("v2") to better accord with its parent class

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.

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:
         """

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"

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