Add some helper methods to load and save config settings (#1472)

* Add some helper methods to load and save config settings.

* Fix comment.

api/admin/controller/patron_auth_service_self_tests.py

@@ -132,10 +132,8 @@ def run_tests(self, integration: IntegrationConfiguration) -> Dict[str, Any]:
             )
 
         protocol_class = self.get_protocol_class(integration)
-        settings = protocol_class.settings_class()(**integration.settings_dict)
-        library_settings = protocol_class.library_settings_class()(
-            **library_configuration.settings_dict
-        )
+        settings = protocol_class.settings_load(integration)
+        library_settings = protocol_class.library_settings_load(library_configuration)
 
         value, _ = protocol_class.run_self_tests(
             self.db,

api/authenticator.py

@@ -348,20 +348,8 @@ def register_provider(
                 f"Implementation class {impl_cls} is not an AuthenticationProvider."
             )
         try:
-            if not isinstance(integration.parent.settings_dict, dict):
-                raise CannotLoadConfiguration(
-                    f"Settings for {impl_cls.__name__} authentication provider for "
-                    f"library {self.library_short_name} are not a dictionary."
-                )
-            if not isinstance(integration.settings_dict, dict):
-                raise CannotLoadConfiguration(
-                    f"Library settings for {impl_cls.__name__} authentication provider for "
-                    f"library {self.library_short_name} are not a dictionary."
-                )
-            settings = impl_cls.settings_class()(**integration.parent.settings_dict)
-            library_settings = impl_cls.library_settings_class()(
-                **integration.settings_dict
-            )
+            settings = impl_cls.settings_load(integration.parent)
+            library_settings = impl_cls.library_settings_load(integration)
             provider = impl_cls(
                 self.library_id,  # type: ignore[arg-type]
                 integration.parent_id,  # type: ignore[arg-type]

api/circulation.py

@@ -635,7 +635,7 @@ def integration_configuration(self) -> IntegrationConfiguration:
 
     @property
     def settings(self) -> SettingsType:
-        return self.settings_class()(**self.integration_configuration().settings_dict)
+        return self.settings_load(self.integration_configuration())
 
     def library_settings(self, library: Library | int) -> LibrarySettingsType | None:
         library_id = library.id if isinstance(library, Library) else library
@@ -644,7 +644,7 @@ def library_settings(self, library: Library | int) -> LibrarySettingsType | None
         libconfig = self.integration_configuration().for_library(library_id=library_id)
         if libconfig is None:
             return None
-        config = self.library_settings_class()(**libconfig.settings_dict)
+        config = self.library_settings_load(libconfig)
         return config
 
     @abstractmethod

api/discovery/opds_registration.py

@@ -129,7 +129,7 @@ def for_integration(
         if integration_obj is None:
             return None
 
-        settings = cls.settings_class().construct(**integration_obj.settings_dict)
+        settings = cls.settings_load(integration_obj)
         return cls(integration_obj, settings)
 
     @staticmethod

api/overdrive.py

@@ -398,7 +398,7 @@ def __init__(self, _db, collection):
             # from the parent (the main Overdrive account), except for the
             # library ID, which we already set.
             parent_integration = collection.parent.integration_configuration
-            parent_config = self.settings_class()(**parent_integration.settings_dict)
+            parent_config = self.settings_load(parent_integration)
             for key in OverdriveConstants.OVERDRIVE_CONFIGURATION_KEYS:
                 parent_value = getattr(parent_config, key, None)
                 setattr(self._configuration, key, parent_value)

core/integration/base.py

@@ -1,12 +1,75 @@
 from __future__ import annotations
 
 from abc import ABC, abstractmethod
-from typing import Any, Generic, Type, TypeVar
+from typing import TYPE_CHECKING, Any, Dict, Generic, Mapping, Protocol, Type, TypeVar
 
 from sqlalchemy.orm import Session
+from sqlalchemy.orm.attributes import Mapped, flag_modified
 
 from core.integration.settings import BaseSettings
 
+if TYPE_CHECKING:
+    from core.model import IntegrationConfiguration, IntegrationLibraryConfiguration
+
+
+class IntegrationConfigurationProtocol(Protocol):
+    settings_dict: Mapped[Dict[str, Any]]
+
+
+T = TypeVar("T", bound=BaseSettings)
+
+
+def integration_settings_load(
+    settings_cls: Type[T],
+    integration: IntegrationConfigurationProtocol,
+) -> T:
+    """
+    Load the settings object for an integration from the database.
+
+    These settings ARE NOT validated when loaded from the database. It is assumed that
+    the settings have already been validated when they were saved to the database. This
+    speeds up the loading of the settings from the database.
+
+    :param settings_cls: The settings class that the settings should be loaded into.
+    :param integration: The integration to load the settings from. This should be a
+        SQLAlchemy model with a settings_dict JSONB column.
+
+    :return: An instance of the settings class loaded with the settings from the database.
+    """
+    settings_dict = integration.settings_dict
+    return settings_cls.construct(**settings_dict)
+
+
+def integration_settings_update(
+    settings_cls: Type[BaseSettings],
+    integration: IntegrationConfigurationProtocol,
+    new_settings: BaseSettings | Mapping[str, Any],
+    merge: bool = False,
+) -> None:
+    """
+    Update the settings for an integration in the database.
+
+    The settings are validated before being saved to the database, and SQLAlchemy is
+    notified that the settings_dict column has been modified.
+
+    :param settings_cls: The settings class to use to validate the settings.
+    :param integration: The integration to update. This should be a SQLAlchemy model
+        with a settings_dict JSONB column.
+    :param new_settings: The new settings to update the integration with. This can either
+        be a BaseSettings object, or a dictionary of settings.
+    :param merge: If True, the new settings will be merged with the existing settings. With
+        the new settings taking precedence. If False, the new settings will replace the existing
+        settings.
+    """
+    settings_dict = integration.settings_dict if merge else {}
+    new_settings_dict = (
+        new_settings.dict() if isinstance(new_settings, BaseSettings) else new_settings
+    )
+    settings_dict.update(new_settings_dict)
+    integration.settings_dict = settings_cls(**settings_dict).dict()
+    flag_modified(integration, "settings_dict")
+
+
 SettingsType = TypeVar("SettingsType", bound=BaseSettings, covariant=True)
 LibrarySettingsType = TypeVar("LibrarySettingsType", bound=BaseSettings, covariant=True)
 
@@ -30,6 +93,31 @@ def settings_class(cls) -> Type[SettingsType]:
         """Get the settings for this integration"""
         ...
 
+    @classmethod
+    def settings_load(cls, integration: IntegrationConfiguration) -> SettingsType:
+        """
+        Load the settings object for this integration from the database.
+
+        See the documentation for `integration_settings_load` for more details.
+        """
+        return integration_settings_load(cls.settings_class(), integration)
+
+    @classmethod
+    def settings_update(
+        cls,
+        integration: IntegrationConfiguration,
+        new_settings: BaseSettings | Mapping[str, Any],
+        merge: bool = False,
+    ) -> None:
+        """
+        Update the settings for this integration in the database.
+
+        See the documentation for `integration_settings_update` for more details.
+        """
+        integration_settings_update(
+            cls.settings_class(), integration, new_settings, merge
+        )
+
     @classmethod
     def protocol_details(cls, db: Session) -> dict[str, Any]:
         """Add any additional details about this protocol to be
@@ -51,6 +139,33 @@ def library_settings_class(cls) -> Type[LibrarySettingsType]:
         """Get the library settings for this integration"""
         ...
 
+    @classmethod
+    def library_settings_load(
+        cls, integration: IntegrationLibraryConfiguration
+    ) -> LibrarySettingsType:
+        """
+        Load the library settings object for this integration from the database.
+
+        See the documentation for `integration_settings_load` for more details.
+        """
+        return integration_settings_load(cls.library_settings_class(), integration)
+
+    @classmethod
+    def library_settings_update(
+        cls,
+        integration: IntegrationLibraryConfiguration,
+        new_settings: BaseSettings | Mapping[str, Any],
+        merge: bool = False,
+    ) -> None:
+        """
+        Update the settings for this library integration in the database.
+
+        See the documentation for `integration_settings_update` for more details.
+        """
+        integration_settings_update(
+            cls.library_settings_class(), integration, new_settings, merge
+        )
+
 
 class HasChildIntegrationConfiguration(HasIntegrationConfiguration[SettingsType], ABC):
     @classmethod

core/model/library.py

@@ -29,13 +29,13 @@
 )
 from sqlalchemy.dialects.postgresql import JSONB
 from sqlalchemy.orm import Mapped, Query, relationship
-from sqlalchemy.orm.attributes import flag_modified
 from sqlalchemy.orm.session import Session
 from sqlalchemy.sql.functions import func
 
 from core.configuration.library import LibrarySettings
 from core.entrypoint import EntryPoint
 from core.facets import FacetConstants
+from core.integration.base import integration_settings_load, integration_settings_update
 from core.model import Base, get_one
 from core.model.announcements import Announcement
 from core.model.customlist import customlist_sharedlibrary
@@ -300,15 +300,14 @@ def settings(self) -> LibrarySettings:
                     "settings_dict for library %s is not a dict: %r"
                     % (self.short_name, self.settings_dict)
                 )
-            settings = LibrarySettings.construct(**self.settings_dict)
+            settings = integration_settings_load(LibrarySettings, self)
             self._settings = settings
         return settings
 
     def update_settings(self, new_settings: LibrarySettings) -> None:
         """Update the settings for this integration"""
         self._settings = None
-        self.settings_dict.update(new_settings.dict())
-        flag_modified(self, "settings_dict")
+        integration_settings_update(LibrarySettings, self, new_settings, merge=True)
 
     @property
     def all_collections(self) -> Generator[Collection, None, None]:

core/opds_import.py

@@ -41,6 +41,7 @@
 from core.config import IntegrationException
 from core.connection_config import ConnectionSetting
 from core.coverage import CoverageFailure
+from core.integration.base import integration_settings_load
 from core.integration.settings import (
     BaseSettings,
     ConfigurationFormItem,
@@ -327,8 +328,8 @@ def __init__(
         # we don't, e.g. accidentally get our IP banned from
         # gutenberg.org.
         self.http_get = http_get or Representation.cautious_http_get
-        self.settings = self.settings_class().construct(
-            **collection.integration_configuration.settings_dict
+        self.settings = integration_settings_load(
+            self.settings_class(), collection.integration_configuration
         )
 
     @classmethod

tests/api/admin/controller/test_discovery_services.py

@@ -200,8 +200,7 @@ def test_discovery_services_post_create(
         assert service.id == int(response.get_data(as_text=True))
         assert self.protocol == service.protocol
         assert (
-            OpdsRegistrationService.settings_class()(**service.settings_dict).url
-            == "http://registry.url"
+            OpdsRegistrationService.settings_load(service).url == "http://registry.url"
         )
 
     def test_discovery_services_post_edit(
@@ -232,9 +231,7 @@ def test_discovery_services_post_edit(
         assert self.protocol == discovery_service.protocol
         assert (
             "http://new_registry_url.com"
-            == OpdsRegistrationService.settings_class()(
-                **discovery_service.settings_dict
-            ).url
+            == OpdsRegistrationService.settings_load(discovery_service).url
         )
 
     def test_check_name_unique(

tests/api/admin/controller/test_library.py

@@ -210,7 +210,7 @@ def test_libraries_get_with_multiple_libraries(
                 FacetConstants.ORDER_TITLE,
                 FacetConstants.ORDER_AUTHOR,
             ] == settings_dict.get("facets_enabled_order")
-            assert ["French"] == settings_dict.get("large_collection_languages")
+            assert ["fre"] == settings_dict.get("large_collection_languages")
 
     def test_libraries_post_errors(self, settings_ctrl_fixture):
         with settings_ctrl_fixture.request_context_with_admin("/", method="POST"):

tests/api/admin/controller/test_patron_auth.py

@@ -499,9 +499,7 @@ def test_patron_auth_services_post_create(
         assert auth_service is not None
         assert auth_service.id == int(response.response[0])  # type: ignore[index]
         assert SimpleAuthenticationProvider.__module__ == auth_service.protocol
-        settings = SimpleAuthenticationProvider.settings_class()(
-            **auth_service.settings_dict
-        )
+        settings = SimpleAuthenticationProvider.settings_load(auth_service)
         assert settings.test_identifier == "user"
         assert settings.test_password == "pass"
         [library_config] = auth_service.library_configurations
@@ -591,9 +589,7 @@ def test_patron_auth_services_post_edit(
         assert auth_service.id == int(response.response[0])  # type: ignore[index]
         assert SimpleAuthenticationProvider.__module__ == auth_service.protocol
         assert isinstance(auth_service.settings_dict, dict)
-        settings = SimpleAuthenticationProvider.settings_class()(
-            **auth_service.settings_dict
-        )
+        settings = SimpleAuthenticationProvider.settings_load(auth_service)
         assert settings.test_identifier == "user"
         assert settings.test_password == "pass"
         [library_config] = auth_service.library_configurations

tests/api/test_authenticator.py

@@ -682,24 +682,16 @@ def test_configuration_exception_during_from_config_stored(
         # propagated.
         # Create an integration destined to raise CannotLoadConfiguration..
         library = db.default_library()
-        misconfigured, _ = create_millenium_auth_integration(library, url="millenium")
-
-        # ... and one destined to raise ImportError.
         unknown, _ = create_auth_integration_configuration("unknown protocol", library)
 
         auth = LibraryAuthenticator.from_config(db.session, db.default_library())
 
         # The LibraryAuthenticator exists but has no AuthenticationProviders.
         assert auth.basic_auth_provider is None
 
-        # Both integrations have left their trace in
-        # initialization_exceptions.
-        not_configured = auth.initialization_exceptions[(misconfigured.id, library.id)]
-        assert isinstance(not_configured, CannotLoadConfiguration)
-        assert "Could not instantiate MilleniumPatronAPI" in str(not_configured)
-
+        # The integration has left its trace in initialization_exceptions.
         not_found = auth.initialization_exceptions[(unknown.id, library.id)]
-        assert isinstance(not_configured, CannotLoadConfiguration)
+        assert isinstance(not_found, CannotLoadConfiguration)
         assert "Unable to load implementation for external integration" in str(
             not_found
         )

tests/api/test_axis.py

@@ -21,6 +21,7 @@
     Axis360CirculationMonitor,
     Axis360FulfillmentInfo,
     Axis360FulfillmentInfoResponseParser,
+    Axis360Settings,
     AxisCollectionReaper,
     AxisNowManifest,
     BibliographicParser,
@@ -35,6 +36,7 @@
 from api.web_publication_manifest import FindawayManifest, SpineItem
 from core.analytics import Analytics
 from core.coverage import CoverageFailure
+from core.integration.base import integration_settings_update
 from core.metadata_layer import (
     CirculationData,
     ContributorData,
@@ -790,16 +792,22 @@ def test_integration_settings_url(
         self, setting, setting_value, is_valid, expected, axis360: Axis360Fixture
     ):
         config = axis360.collection.integration_configuration
-        settings = config.settings_dict.copy()
-        settings[setting] = setting_value
-        config.settings_dict = settings
+        config.settings_dict[setting] = setting_value
 
         if is_valid:
+            integration_settings_update(
+                Axis360Settings, config, {setting: setting_value}, merge=True
+            )
             api = MockAxis360API(axis360.db.session, axis360.collection)
             assert api.base_url == expected
         else:
             pytest.raises(
-                ProblemError, MockAxis360API, axis360.db.session, axis360.collection
+                ProblemError,
+                integration_settings_update,
+                Axis360Settings,
+                config,
+                {setting: setting_value},
+                merge=True,
             )