PP-398 Maintain type coercions during collections settings mutation (#…

…1347)

* Maintain type coercions during collections settings mutation

* Alembic migration script to coerce license goal settings to the right types

* Use pydantic for type coercion

alembic/versions/20230905_2b672c6fb2b9_type_coerce_collection_settings.py

@@ -0,0 +1,79 @@
+"""Type coerce collection settings
+
+Revision ID: 2b672c6fb2b9
+Revises: 0df58829fc1a
+Create Date: 2023-09-05 06:40:35.739869+00:00
+
+"""
+import json
+from typing import Any, Callable, Dict, Type
+
+from pydantic import parse_obj_as
+
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "2b672c6fb2b9"
+down_revision = "0df58829fc1a"
+branch_labels = None
+depends_on = None
+
+
+def _bool(value):
+    return value in ("true", "True", True)
+
+
+# All the settings types that have non-str types
+ALL_SETTING_TYPES: Dict[str, Type[Any]] = {
+    "verify_certificate": bool,
+    "default_reservation_period": bool,
+    "loan_limit": int,
+    "hold_limit": int,
+    "max_retry_count": int,
+    "ebook_loan_duration": int,
+    "default_loan_duration": int,
+}
+
+
+def _coerce_types(settings: dict) -> None:
+    """Coerce the types, in-place"""
+    setting_type: Callable
+    for setting_name, setting_type in ALL_SETTING_TYPES.items():
+        if setting_name in settings:
+            settings[setting_name] = parse_obj_as(setting_type, settings[setting_name])
+
+
+def upgrade() -> None:
+    connection = op.get_bind()
+    # Fetch all integration settings with the 'licenses' goal
+    results = connection.execute(
+        f"SELECT id, settings from integration_configurations where goal='LICENSE_GOAL';"
+    ).fetchall()
+
+    # For each integration setting, we check id any of the non-str
+    # keys are present in the DB
+    # We then type-coerce that value
+    for settings_id, settings in results:
+        _coerce_types(settings)
+        connection.execute(
+            "UPDATE integration_configurations SET settings=%s where id=%s",
+            json.dumps(settings),
+            settings_id,
+        )
+
+    # Do the same for any Library settings
+    results = connection.execute(
+        f"SELECT parent_id, settings from integration_library_configurations;"
+    ).fetchall()
+
+    for settings_id, settings in results:
+        _coerce_types(settings)
+        connection.execute(
+            "UPDATE integration_library_configurations SET settings=%s where parent_id=%s",
+            json.dumps(settings),
+            settings_id,
+        )
+
+
+def downgrade() -> None:
+    """There is no need to revert the types back to strings"""

api/admin/controller/collection_settings.py

@@ -356,10 +356,10 @@ def process_settings(
 
         # validate then apply
         try:
-            settings_class(**collection_settings)
+            validated_settings = settings_class(**collection_settings)
         except ProblemError as ex:
             return ex.problem_detail
-        collection.integration_configuration.settings_dict = collection_settings
+        collection.integration_configuration.settings_dict = validated_settings.dict()
         return None
 
     def _set_external_integration_link(

api/admin/controller/settings.py

@@ -382,10 +382,10 @@ def _set_configuration_library(
         config = None
 
         # Validate first
-        protocol_class.library_settings_class()(**info_copy)
+        validated_data = protocol_class.library_settings_class()(**info_copy)
         # Attach the configuration
         config = configuration.for_library(cast(int, library.id), create=True)
-        config.settings_dict = info_copy
+        config.settings_dict = validated_data.dict()
         return config
 
     def _set_integration_library(self, integration, library_info, protocol):

core/model/collection.py

@@ -325,10 +325,11 @@ def default_loan_period(self, library, medium=EditionConstants.BOOK_MEDIUM):
         that someone who borrows a non-open-access item from this
         collection has it for this number of days.
         """
-        return (
+        value = (
             self.default_loan_period_setting(library, medium)
             or self.STANDARD_DEFAULT_LOAN_PERIOD
         )
+        return value
 
     @classmethod
     def loan_period_key(cls, medium=EditionConstants.BOOK_MEDIUM):

tests/api/admin/controller/test_collections.py

@@ -178,7 +178,6 @@ def test_collections_get_collection_protocols(
     def test_collections_get_collections_with_multiple_collections(
         self, settings_ctrl_fixture: SettingsControllerFixture
     ):
-
         old_prior_test_results = HasSelfTests.prior_test_results
         setattr(
             HasCollectionSelfTests,
@@ -666,6 +665,7 @@ def test_collections_post_edit(
                     ("overdrive_client_key", "user2"),
                     ("overdrive_client_secret", "password"),
                     ("overdrive_website_id", "1234"),
+                    ("max_retry_count", "10"),
                     (
                         "libraries",
                         json.dumps([{"short_name": "L1", "ils_name": "the_ils"}]),
@@ -684,6 +684,11 @@ def test_collections_post_edit(
             "overdrive_client_key"
         )
 
+        # Type coercion stays intact
+        assert 10 == collection.integration_configuration.settings_dict.get(
+            "max_retry_count"
+        )
+
         # A library now has access to the collection.
         assert [collection] == l1.collections
 
@@ -754,6 +759,50 @@ def test_collections_post_edit(
         # The collection now has a parent.
         assert parent == collection.parent
 
+        library = settings_ctrl_fixture.ctrl.db.default_library()
+        collection2 = settings_ctrl_fixture.ctrl.db.collection(
+            name="Collection 2", protocol=ExternalIntegration.ODL
+        )
+        with settings_ctrl_fixture.request_context_with_admin("/", method="POST"):
+            flask.request.form = ImmutableMultiDict(
+                [
+                    ("id", str(collection2.id)),
+                    ("name", "Collection 2"),
+                    ("protocol", ExternalIntegration.ODL),
+                    ("external_account_id", "1234"),
+                    ("username", "user"),
+                    ("password", "password"),
+                    ("data_source", "datasource"),
+                    ("passphrase_hint", "passphrase_hint"),
+                    ("passphrase_hint_url", "http://passphrase_hint_url.com"),
+                    (
+                        "libraries",
+                        json.dumps(
+                            [
+                                {
+                                    "short_name": library.short_name,
+                                    "ebook_loan_duration": "200",
+                                }
+                            ]
+                        ),
+                    ),
+                ]
+            )
+            response = (
+                settings_ctrl_fixture.manager.admin_collection_settings_controller.process_collections()
+            )
+            assert response.status_code == 200
+
+        settings_ctrl_fixture.ctrl.db.session.refresh(collection2)
+        assert len(collection2.integration_configuration.library_configurations) == 1
+        # The library configuration value was correctly coerced to int
+        assert (
+            collection2.integration_configuration.library_configurations[
+                0
+            ].settings_dict.get("ebook_loan_duration")
+            == 200
+        )
+
     def _base_collections_post_request(self, collection):
         """A template for POST requests to the collections controller."""
         return [

tests/migration/test_20230905_2b672c6fb2b9.py

@@ -0,0 +1,110 @@
+import json
+from dataclasses import dataclass
+from typing import Any, Dict, Optional, Protocol
+
+import pytest
+from pytest_alembic import MigrationContext
+from sqlalchemy.engine import Connection, Engine
+
+from tests.migration.conftest import CreateLibrary
+
+
+@dataclass
+class IntegrationConfiguration:
+    id: int
+    settings: Dict[str, Any]
+
+
+class CreateConfiguration(Protocol):
+    def __call__(
+        self, connection: Connection, protocol: str, name: str, settings: Dict[str, Any]
+    ) -> IntegrationConfiguration:
+        ...
+
+
+@pytest.fixture
+def create_integration_configuration() -> CreateConfiguration:
+    def insert_config(
+        connection: Connection, protocol: str, name: str, settings: Dict[str, Any]
+    ) -> IntegrationConfiguration:
+        connection.execute(
+            "INSERT INTO integration_configurations (goal, protocol, name, settings, self_test_results) VALUES (%s, %s, %s, %s, '{}')",
+            "LICENSE_GOAL",
+            protocol,
+            name,
+            json.dumps(settings),
+        )
+        return fetch_config(connection, name=name)
+
+    return insert_config
+
+
+def fetch_config(
+    connection: Connection, name: Optional[str] = None, parent_id: Optional[int] = None
+) -> IntegrationConfiguration:
+    if name is not None:
+        _id, settings = connection.execute(  # type: ignore[misc]
+            "SELECT id, settings FROM integration_configurations where name=%s", name
+        ).fetchone()
+    else:
+        _id, settings = connection.execute(  # type: ignore[misc]
+            "SELECT parent_id, settings FROM integration_library_configurations where parent_id=%s",
+            parent_id,
+        ).fetchone()
+    return IntegrationConfiguration(_id, settings)
+
+
+MIGRATION_UID = "2b672c6fb2b9"
+
+
+def test_settings_coersion(
+    alembic_runner: MigrationContext,
+    alembic_engine: Engine,
+    create_library: CreateLibrary,
+    create_integration_configuration: CreateConfiguration,
+) -> None:
+    alembic_runner.migrate_down_to(MIGRATION_UID)
+    alembic_runner.migrate_down_one()
+
+    with alembic_engine.connect() as connection:
+        config = create_integration_configuration(
+            connection,
+            "Axis 360",
+            "axis-test-1",
+            dict(
+                verify_certificate="true",
+                loan_limit="20",
+                key="value",
+            ),
+        )
+
+        library_id = create_library(connection)
+
+        library_settings = dict(
+            hold_limit="30",
+            max_retry_count="2",
+            ebook_loan_duration="10",
+            default_loan_duration="11",
+            unchanged="value",
+        )
+        connection.execute(
+            "INSERT INTO integration_library_configurations (library_id, parent_id, settings) VALUES (%s, %s, %s)",
+            library_id,
+            config.id,
+            json.dumps(library_settings),
+        )
+        alembic_runner.migrate_up_one()
+
+        axis_config = fetch_config(connection, name="axis-test-1")
+        assert axis_config.settings["verify_certificate"] == True
+        assert axis_config.settings["loan_limit"] == 20
+        # Unknown settings remain as-is
+        assert axis_config.settings["key"] == "value"
+
+        odl_config = fetch_config(connection, parent_id=config.id)
+        assert odl_config.settings["hold_limit"] == 30
+        assert odl_config.settings["max_retry_count"] == 2
+        assert odl_config.settings["ebook_loan_duration"] == 10
+        assert odl_config.settings["default_loan_duration"] == 11
+        # Unknown settings remain as-is
+        assert odl_config.settings["unchanged"] == "value"