Update how we load settings from database

core/integration/base.py

@@ -26,9 +26,9 @@ def integration_settings_load(
     """
     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.
+    The settings are validated when loaded from the database, this is done rather
+    than using construct() because there are some types that need to get type converted
+    when round tripping from the database (such as enum) and construct() doesn't do that.
 
     :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
@@ -37,7 +37,7 @@ def integration_settings_load(
     :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)
+    return settings_cls(**settings_dict)
 
 
 def integration_settings_update(

tests/api/admin/controller/test_library_registrations.py

@@ -37,7 +37,7 @@ def test_discovery_service_library_registrations_get(
 
         # Here's a discovery service.
         discovery_service = create_integration_configuration.discovery_service(
-            url="http://service-url/"
+            url="http://service-url.com/"
         )
 
         # We successfully registered this library with the service.
@@ -89,7 +89,7 @@ def test_discovery_service_library_registrations_get(
         # registration link.
         root_catalog = dict(links=[dict(href="http://register-here/", rel="register")])
         requests_mock.get(
-            "http://service-url/",
+            "http://service-url.com/",
             json=root_catalog,
             headers={"Content-Type": OpdsRegistrationService.OPDS_2_TYPE},
         )
@@ -137,7 +137,7 @@ def test_discovery_service_library_registrations_get(
             # happened.  The target of the first request is the URL to
             # the discovery service's main catalog. The second request
             # is to the "register" link found in that catalog.
-            assert ["service-url", "register-here"] == [
+            assert ["service-url.com", "register-here"] == [
                 r.hostname for r in requests_mock.request_history
             ]
 
@@ -174,7 +174,7 @@ def test_discovery_service_library_registrations_get(
             # there will be no second request.
             requests_mock.reset()
             requests_mock.get(
-                "http://service-url/",
+                "http://service-url.com/",
                 json=REMOTE_INTEGRATION_FAILED.response[0],
                 status_code=502,
             )
@@ -239,7 +239,7 @@ def test_discovery_service_library_registrations_post(
 
         # Create an IntegrationConfiguration to avoid that problem in future tests.
         discovery_service = create_integration_configuration.discovery_service(
-            url="http://register-here/"
+            url="http://register-here.com/"
         )
 
         # We might not get a library short name.

tests/api/test_circulationapi.py

@@ -886,10 +886,15 @@ def test_borrow_hold_limit_reached(
 
     def test_fulfill_errors(self, circulation_api: CirculationAPIFixture):
         # Here's an open-access title.
+        collection = circulation_api.db.collection(
+            protocol=ExternalIntegration.OPDS_IMPORT, data_source_name="OPDS"
+        )
         circulation_api.pool.open_access = True
+        circulation_api.pool.collection = collection
+
         circulation_api.circulation.remotes[
             circulation_api.pool.data_source.name
-        ] = OPDSAPI(circulation_api.db.session, circulation_api.collection)
+        ] = OPDSAPI(circulation_api.db.session, collection)
 
         # The patron has the title on loan.
         circulation_api.pool.loan_to(circulation_api.patron)

tests/api/test_controller_multilib.py

@@ -1,4 +1,5 @@
 from core.model import Collection, ExternalIntegration, get_one_or_create
+from core.opds_import import OPDSAPI
 from tests.fixtures.api_controller import (
     CirculationControllerFixture,
     ControllerFixtureSetupOverrides,
@@ -21,7 +22,13 @@ def make_default_collection(_db, library):
                 name=f"{controller_fixture.db.fresh_str()} (for multi-library test)",
             )
             collection.create_external_integration(ExternalIntegration.OPDS_IMPORT)
-            collection.create_integration_configuration(ExternalIntegration.OPDS_IMPORT)
+            integration = collection.create_integration_configuration(
+                ExternalIntegration.OPDS_IMPORT
+            )
+            settings = OPDSAPI.settings_class()(
+                external_account_id="http://url.com", data_source="OPDS"
+            )
+            OPDSAPI.settings_update(integration, settings)
             library.collections.append(collection)
             return collection
 

tests/api/test_controller_scopedsession.py

@@ -13,6 +13,7 @@
     Library,
     create,
 )
+from core.opds_import import OPDSAPI
 from tests.fixtures.api_controller import (
     ControllerFixture,
     ControllerFixtureSetupOverrides,
@@ -60,7 +61,13 @@ def make_default_collection(self, session: Session, library):
             name=self.fresh_id() + " (collection for scoped session)",
         )
         collection.create_external_integration(ExternalIntegration.OPDS_IMPORT)
-        collection.create_integration_configuration(ExternalIntegration.OPDS_IMPORT)
+        integration = collection.create_integration_configuration(
+            ExternalIntegration.OPDS_IMPORT
+        )
+        settings = OPDSAPI.settings_class()(
+            external_account_id="http://url.com", data_source="OPDS"
+        )
+        OPDSAPI.settings_update(integration, settings)
         library.collections.append(collection)
         return collection
 

tests/api/test_controller_work.py

@@ -71,6 +71,7 @@ def work_fixture(db: DatabaseTransactionFixture):
 class TestWorkController:
     def test_contributor(self, work_fixture: WorkFixture):
         m = work_fixture.manager.work_controller.contributor
+        work_fixture.collection.data_source = None
 
         # Find a real Contributor put in the system through the setup
         # process.
@@ -843,6 +844,7 @@ def test_related_no_works(self, work_fixture: WorkFixture):
         assert result == NOT_FOUND_ON_REMOTE
 
     def test_series(self, work_fixture: WorkFixture):
+        work_fixture.collection.data_source = None
         # Test the ability of the series() method to generate an OPDS
         # feed representing all the books in a given series, subject
         # to an optional language and audience restriction.

tests/api/test_lanes.py

@@ -1163,15 +1163,15 @@ def test_constructor(self, db: DatabaseTransactionFixture):
         ] = available_now
 
         assert (
-            "License source {[Unknown]} - Medium {Book} - Collection name {%s}"
+            "License source {OPDS} - Medium {Book} - Collection name {%s}"
             % db.default_collection().name
             == default_ebooks.display_name
         )
         assert [db.default_collection().id] == default_ebooks.collection_ids
         assert [Edition.BOOK_MEDIUM] == default_ebooks.media
 
         assert (
-            "License source {[Unknown]} - Medium {Audio} - Collection name {%s}"
+            "License source {OPDS} - Medium {Audio} - Collection name {%s}"
             % db.default_collection().name
             == default_audio.display_name
         )

tests/core/integration/test_base.py

@@ -1,11 +1,15 @@
+import datetime
+from enum import Enum
 from functools import partial
 from unittest.mock import MagicMock, Mock, patch
 
 import pytest
 
 from core.integration.base import integration_settings_load, integration_settings_update
+from core.integration.goals import Goals
 from core.integration.settings import BaseSettings
 from core.model import IntegrationConfiguration
+from tests.fixtures.database import DatabaseTransactionFixture
 
 
 class BaseFixture:
@@ -31,10 +35,40 @@ def base_fixture():
 
 def test_integration_settings_load(base_fixture: BaseFixture) -> None:
     return_value: BaseSettings = base_fixture.load()
-    base_fixture.mock_settings_cls.construct.assert_called_once_with(
-        test="test", number=123
+    base_fixture.mock_settings_cls.assert_called_once_with(test="test", number=123)
+    assert return_value is base_fixture.mock_settings_cls.return_value
+
+
+def test_integration_settings_roundtrip(db: DatabaseTransactionFixture) -> None:
+    class TestEnum(Enum):
+        FOO = "foo"
+        BAR = "bar"
+
+    class TestSettings(BaseSettings):
+        test: str
+        number: int
+        enum: TestEnum
+        date: datetime.date
+
+    # Create a settings object and save it to the database
+    settings = TestSettings(
+        test="test", number=123, enum=TestEnum.FOO, date=datetime.date.today()
     )
-    assert return_value is base_fixture.mock_settings_cls.construct.return_value
+    integration = db.integration_configuration(protocol="test", goal=Goals.LICENSE_GOAL)
+    integration_settings_update(TestSettings, integration, settings)
+    settings_dict = integration.settings_dict.copy()
+
+    # Expire this object in the session, so that we can be sure that the integration data
+    # gets round-tripped from the database, which includes a JSON serialization step.
+    db.session.flush()
+    db.session.expire(integration)
+
+    # Load the settings from the database and check that the settings_dict is different
+    # due to the JSON serialization, but that once we load the settings object, it is
+    # equal to the original settings object.
+    assert integration.settings_dict != settings_dict
+    settings_roundtripped = integration_settings_load(TestSettings, integration)
+    assert settings_roundtripped == settings
 
 
 def test_integration_settings_update_no_merge(base_fixture: BaseFixture) -> None:

tests/core/models/test_configuration.py

@@ -3,9 +3,9 @@
 
 from core.config import CannotLoadConfiguration, Configuration
 from core.model import create, get_one
-from core.model.collection import Collection
 from core.model.configuration import ConfigurationSetting, ExternalIntegration
 from core.model.datasource import DataSource
+from core.opds_import import OPDSAPI
 from tests.fixtures.database import DatabaseTransactionFixture
 
 
@@ -516,18 +516,20 @@ def test_data_source(
         # data source.
         collection = db.collection(protocol=ExternalIntegration.OVERDRIVE)
         assert collection.data_source is not None
-        assert DataSource.OVERDRIVE == collection.data_source.name
+        assert collection.data_source.name == DataSource.OVERDRIVE
 
         # For OPDS Import collections, data source is a setting which
         # might not be present.
-        assert None == db.default_collection().data_source
+        opds_collection = db.collection(protocol=ExternalIntegration.OPDS_IMPORT)
+        assert opds_collection.data_source is None
 
         # data source will be automatically created if necessary.
-        DatabaseTransactionFixture.set_settings(
-            db.default_collection().integration_configuration,
-            **{Collection.DATA_SOURCE_NAME_SETTING: "New Data Source"}
+        settings = OPDSAPI.settings_class()(
+            external_account_id="http://url.com/feed", data_source="New Data Source"
         )
-        assert "New Data Source" == db.default_collection().data_source.name
+        OPDSAPI.settings_update(opds_collection.integration_configuration, settings)
+        assert isinstance(opds_collection.data_source, DataSource)
+        assert opds_collection.data_source.name == "New Data Source"  # type: ignore[unreachable]
 
     def test_set_key_value_pair(
         self, example_externalintegration_fixture: ExampleExternalIntegrationFixture

tests/core/models/test_library.py

@@ -1,7 +1,6 @@
 import pytest
 from Crypto.PublicKey.RSA import RsaKey, import_key
 
-from core.configuration.library import LibrarySettings
 from core.model.configuration import ConfigurationSetting
 from core.model.library import Library
 from tests.fixtures.database import DatabaseTransactionFixture
@@ -236,16 +235,6 @@ def test_settings(self, db: DatabaseTransactionFixture):
         with pytest.raises(ValueError):
             library.settings
 
-        # We don't validate settings when loaded from the database, since
-        # we assume they were validated when they were set.
-        library.settings_dict = {}
-        settings = library.settings
-
-        # This would normally not be possible, because website is
-        # a required property.
-        assert isinstance(settings, LibrarySettings)
-        assert not hasattr(settings, "website")
-
         # Test with a properly formatted settings dict.
         library2 = db.library()
         assert library2.settings.website == "http://library.com"

tests/core/test_opds2_import.py

@@ -465,7 +465,7 @@ class Opds2ApiFixture:
     def __init__(self, db: DatabaseTransactionFixture, mock_http: MagicMock):
         self.patron = db.patron()
         self.collection: Collection = db.collection(
-            protocol=ExternalIntegration.OPDS2_IMPORT
+            protocol=ExternalIntegration.OPDS2_IMPORT, data_source_name="test"
         )
         self.integration = self.collection.create_external_integration(
             ExternalIntegration.OPDS2_IMPORT

tests/core/test_opds_import.py

@@ -1368,6 +1368,9 @@ def test_update_work_for_edition_having_multiple_license_pools(
 
     def test_assert_importable_content(self, db: DatabaseTransactionFixture):
         session = db.session
+        collection = db.collection(
+            protocol=ExternalIntegration.OPDS_IMPORT, data_source_name="OPDS"
+        )
 
         class Mock(OPDSImporter):
             """An importer that may or may not be able to find
@@ -1403,7 +1406,7 @@ class NoLinks(Mock):
         do_get = MagicMock()
 
         # Here, there are no links at all.
-        importer = NoLinks(session, db.default_collection(), do_get)
+        importer = NoLinks(session, collection, do_get)
         with pytest.raises(IntegrationException) as excinfo:
             importer.assert_importable_content("feed", "url")
         assert "No open-access links were found in the OPDS feed." in str(excinfo.value)
@@ -1430,7 +1433,7 @@ class BadLinks(Mock):
                 ),
             ]
 
-        bad_links_importer = BadLinks(session, db.default_collection(), do_get)
+        bad_links_importer = BadLinks(session, collection, do_get)
         with pytest.raises(IntegrationException) as excinfo:
             bad_links_importer.assert_importable_content(
                 "feed", "url", max_get_attempts=2
@@ -1467,7 +1470,7 @@ def _is_open_access_link(self, url, type):
                     return False
                 return "this is a book"
 
-        good_link_importer = GoodLink(session, db.default_collection(), do_get)
+        good_link_importer = GoodLink(session, collection, do_get)
         result = good_link_importer.assert_importable_content(
             "feed", "url", max_get_attempts=5
         )
@@ -2303,7 +2306,9 @@ class OPDSAPIFixture:
     def __init__(self, db: DatabaseTransactionFixture):
         self.db = db
         self.session = db.session
-        self.collection = db.collection(protocol=OPDSAPI.label())
+        self.collection = db.collection(
+            protocol=OPDSAPI.label(), data_source_name="OPDS"
+        )
         self.api = OPDSAPI(self.session, self.collection)
 
         self.mock_patron = MagicMock()

tests/fixtures/database.py

@@ -166,17 +166,12 @@ def __init__(
     def _make_default_library(self) -> Library:
         """Ensure that the default library exists in the given database."""
         library = self.library("default", "default")
-        collection, ignore = get_one_or_create(
-            self._session, Collection, name="Default Collection"
+        collection = self.collection(
+            "Default Collection",
+            protocol=ExternalIntegration.OPDS_IMPORT,
+            data_source_name="OPDS",
         )
-        integration = collection.create_external_integration(
-            ExternalIntegration.OPDS_IMPORT
-        )
-        integration.goal = ExternalIntegration.LICENSE_GOAL
-        config = collection.create_integration_configuration(
-            ExternalIntegration.OPDS_IMPORT
-        )
-        config.for_library(library.id, create=True)
+        collection.integration_configuration.for_library(library.id, create=True)
         if collection not in library.collections:
             library.collections.append(collection)
         return library