From 02b6fd1780fddf2c6d36e543118b639d51e9ae74 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Mon, 6 Nov 2023 13:24:33 -0400 Subject: [PATCH] Update how we load settings from database (#1500) * Update how we load settings from database. * Make sure config data in tests validates * More test fixes * Mypy fix --- core/integration/base.py | 8 ++-- .../controller/test_library_registrations.py | 10 ++--- tests/api/test_circulationapi.py | 7 +++- tests/api/test_controller_multilib.py | 9 ++++- tests/api/test_controller_scopedsession.py | 9 ++++- tests/api/test_controller_work.py | 2 + tests/api/test_lanes.py | 4 +- tests/core/integration/test_base.py | 40 +++++++++++++++++-- tests/core/models/test_configuration.py | 16 ++++---- tests/core/models/test_library.py | 11 ----- tests/core/test_opds2_import.py | 2 +- tests/core/test_opds_import.py | 13 ++++-- tests/fixtures/database.py | 15 +++---- 13 files changed, 96 insertions(+), 50 deletions(-) diff --git a/core/integration/base.py b/core/integration/base.py index 2aea7fa127..db17e20ae6 100644 --- a/core/integration/base.py +++ b/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( diff --git a/tests/api/admin/controller/test_library_registrations.py b/tests/api/admin/controller/test_library_registrations.py index 2445f58311..889b566c92 100644 --- a/tests/api/admin/controller/test_library_registrations.py +++ b/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. diff --git a/tests/api/test_circulationapi.py b/tests/api/test_circulationapi.py index 2150cefa64..de1012ff0e 100644 --- a/tests/api/test_circulationapi.py +++ b/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) diff --git a/tests/api/test_controller_multilib.py b/tests/api/test_controller_multilib.py index 4e725fce1c..60488f1c51 100644 --- a/tests/api/test_controller_multilib.py +++ b/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 diff --git a/tests/api/test_controller_scopedsession.py b/tests/api/test_controller_scopedsession.py index 797a7b25f6..f5addd55f4 100644 --- a/tests/api/test_controller_scopedsession.py +++ b/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 diff --git a/tests/api/test_controller_work.py b/tests/api/test_controller_work.py index 092e2e3ae0..29f9dc2897 100644 --- a/tests/api/test_controller_work.py +++ b/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. diff --git a/tests/api/test_lanes.py b/tests/api/test_lanes.py index b4f762f886..b02782c42e 100644 --- a/tests/api/test_lanes.py +++ b/tests/api/test_lanes.py @@ -1163,7 +1163,7 @@ 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 ) @@ -1171,7 +1171,7 @@ def test_constructor(self, db: DatabaseTransactionFixture): 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 ) diff --git a/tests/core/integration/test_base.py b/tests/core/integration/test_base.py index bd4cb4ec3a..30f9cac628 100644 --- a/tests/core/integration/test_base.py +++ b/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: diff --git a/tests/core/models/test_configuration.py b/tests/core/models/test_configuration.py index 4ac3f35691..351ede607e 100644 --- a/tests/core/models/test_configuration.py +++ b/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 diff --git a/tests/core/models/test_library.py b/tests/core/models/test_library.py index 4d7c7f71f0..49b98e6cc0 100644 --- a/tests/core/models/test_library.py +++ b/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" diff --git a/tests/core/test_opds2_import.py b/tests/core/test_opds2_import.py index 8a7d1c5556..dc50b228d7 100644 --- a/tests/core/test_opds2_import.py +++ b/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 diff --git a/tests/core/test_opds_import.py b/tests/core/test_opds_import.py index e0451bc128..171b644f06 100644 --- a/tests/core/test_opds_import.py +++ b/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() diff --git a/tests/fixtures/database.py b/tests/fixtures/database.py index a143b67413..3674522d2c 100644 --- a/tests/fixtures/database.py +++ b/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