Skip to content

Commit

Permalink
Fix issue with 2b672c6fb2b9 migration (PP-428) (#1366)
Browse files Browse the repository at this point in the history 
- Fixed the type for default_reservation_period
- Fixed the primary key queries for the library integration configurations
- Additional logging on the migration
- Added a join condition so we are only updating library configurations that need to be updated
- Only write updated settings to the DB when there have been modifications to the settings
- Clean up tests

Co-authored-by: Jonathan Green <[email protected]>
  • Loading branch information
@RishiDiwanTT @jonathangreen
RishiDiwanTT and jonathangreen committed Sep 11, 2023
1 parent ddf44a3 commit 200f295
Show file tree
Hide file tree
Showing 2 changed files with 169 additions and 80 deletions.
94 changes: 63 additions & 31 deletions alembic/versions/20230905_2b672c6fb2b9_type_coerce_collection_settings.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
"""
import json
from typing import Any, Callable, Dict, Type
import logging
from copy import deepcopy
from typing import Any, Dict, Optional, Tuple

from pydantic import parse_obj_as
from pydantic import PositiveInt, ValidationError, parse_obj_as

from alembic import op

Expand All @@ -19,60 +21,90 @@
depends_on = None


def _bool(value):
return value in ("true", "True", True)
log = logging.getLogger(f"palace.migration.{revision}")
log.setLevel(logging.INFO)
log.disabled = False


# 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,
ALL_SETTING_TYPES: Dict[str, Any] = {
"verify_certificate": Optional[bool],
"default_reservation_period": Optional[PositiveInt],
"loan_limit": Optional[PositiveInt],
"hold_limit": Optional[PositiveInt],
"max_retry_count": Optional[PositiveInt],
"ebook_loan_duration": Optional[PositiveInt],
"default_loan_duration": Optional[PositiveInt],
}


def _coerce_types(settings: dict) -> None:
def _coerce_types(original_settings: Dict[str, Any]) -> Tuple[bool, Dict[str, Any]]:
"""Coerce the types, in-place"""
setting_type: Callable
modified = False
modified_settings = deepcopy(original_settings)
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])
if setting_name in original_settings:
# If the setting is an empty string, we set it to None
if original_settings[setting_name] == "":
setting = None
else:
setting = original_settings[setting_name]

try:
modified = True
modified_settings[setting_name] = parse_obj_as(setting_type, setting)
except ValidationError as e:
log.error(
f"Error while parsing setting {setting_name}. Settings: {original_settings}."
)
raise e

return modified, modified_settings


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';"
"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,
)
modified, updated_settings = _coerce_types(settings)
if modified:
log.info(
f"Updating settings for integration_configuration (id:{settings_id}). "
f"Original settings: {settings}. New settings: {updated_settings}."
)
# If any of the values were modified, we update the DB
connection.execute(
"UPDATE integration_configurations SET settings=%s where id=%s",
json.dumps(updated_settings),
settings_id,
)

# Do the same for any Library settings
results = connection.execute(
f"SELECT parent_id, settings from integration_library_configurations;"
"SELECT ilc.parent_id, ilc.library_id, ilc.settings from integration_library_configurations ilc "
"join integration_configurations ic on ilc.parent_id = ic.id where ic.goal='LICENSE_GOAL';"
).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,
)
for parent_id, library_id, settings in results:
modified, updated_settings = _coerce_types(settings)
if modified:
log.info(
f"Updating settings for integration_library_configuration (parent_id:{parent_id}/library_id:{library_id}). "
f"Original settings: {settings}. New settings: {updated_settings}."
)
connection.execute(
"UPDATE integration_library_configurations SET settings=%s where parent_id=%s and library_id=%s",
json.dumps(updated_settings),
parent_id,
library_id,
)


def downgrade() -> None:
Expand Down
155 changes: 106 additions & 49 deletions tests/migration/test_20230905_2b672c6fb2b9.py
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import json
from dataclasses import dataclass
from typing import Any, Dict, Optional, Protocol
from typing import Any, Dict

import pytest
from pytest_alembic import MigrationContext
Expand All @@ -9,49 +8,52 @@
from tests.migration.conftest import CreateLibrary


@dataclass
class IntegrationConfiguration:
id: int
settings: Dict[str, Any]


class CreateConfiguration(Protocol):
class CreateConfiguration:
def __call__(
self, connection: Connection, protocol: str, name: str, settings: Dict[str, Any]
) -> IntegrationConfiguration:
...
self,
connection: Connection,
goal: str,
protocol: str,
name: str,
settings: Dict[str, Any],
) -> int:
integration_configuration = connection.execute(
"INSERT INTO integration_configurations (goal, protocol, name, settings, self_test_results) VALUES (%s, %s, %s, %s, '{}') returning id",
goal,
protocol,
name,
json.dumps(settings),
).fetchone()
assert integration_configuration is not None
assert isinstance(integration_configuration.id, int)
return integration_configuration.id


@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 CreateConfiguration()

return insert_config

def fetch_config(connection: Connection, _id: int) -> Dict[str, Any]:
integration_config = connection.execute(
"SELECT settings FROM integration_configurations where id=%s", _id
).fetchone()
assert integration_config is not None
assert isinstance(integration_config.settings, dict)
return integration_config.settings

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)

def fetch_library_config(
connection: Connection, parent_id: int, library_id: int
) -> Dict[str, Any]:
integration_lib_config = connection.execute(
"SELECT parent_id, settings FROM integration_library_configurations where parent_id=%s and library_id=%s",
parent_id,
library_id,
).fetchone()
assert integration_lib_config is not None
assert isinstance(integration_lib_config.settings, dict)
return integration_lib_config.settings


MIGRATION_UID = "2b672c6fb2b9"
Expand All @@ -67,18 +69,22 @@ def test_settings_coersion(
alembic_runner.migrate_down_one()

with alembic_engine.connect() as connection:
config = create_integration_configuration(
config_id = create_integration_configuration(
connection,
"LICENSE_GOAL",
"Axis 360",
"axis-test-1",
dict(
verify_certificate="true",
loan_limit="20",
default_reservation_period="12",
key="value",
),
)

# Test 2 library configs, to the same parent
library_id = create_library(connection)
library_id2 = create_library(connection)

library_settings = dict(
hold_limit="30",
Expand All @@ -90,21 +96,72 @@ def test_settings_coersion(
connection.execute(
"INSERT INTO integration_library_configurations (library_id, parent_id, settings) VALUES (%s, %s, %s)",
library_id,
config.id,
config_id,
json.dumps(library_settings),
)
library_settings = dict(
hold_limit="31",
max_retry_count="3",
ebook_loan_duration="",
default_loan_duration="12",
unchanged="value1",
)
connection.execute(
"INSERT INTO integration_library_configurations (library_id, parent_id, settings) VALUES (%s, %s, %s)",
library_id2,
config_id,
json.dumps(library_settings),
)

other_config_settings = dict(
verify_certificate="true",
loan_limit="20",
default_reservation_period="12",
key="value",
)
other_config_id = create_integration_configuration(
connection, "PATRON_AUTH_GOAL", "Other", "other-test", other_config_settings
)
connection.execute(
"INSERT INTO integration_library_configurations (library_id, parent_id, settings) VALUES (%s, %s, %s)",
library_id2,
other_config_id,
json.dumps(other_config_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
axis_config = fetch_config(connection, config_id)
assert axis_config["verify_certificate"] == True
assert axis_config["loan_limit"] == 20
assert axis_config["default_reservation_period"] == 12
# Unknown settings remain as-is
assert axis_config["key"] == "value"

odl_config = fetch_library_config(
connection, parent_id=config_id, library_id=library_id
)
assert odl_config["hold_limit"] == 30
assert odl_config["max_retry_count"] == 2
assert odl_config["ebook_loan_duration"] == 10
assert odl_config["default_loan_duration"] == 11
# Unknown settings remain as-is
assert axis_config.settings["key"] == "value"
assert odl_config["unchanged"] == "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
odl_config2 = fetch_library_config(
connection, parent_id=config_id, library_id=library_id2
)
assert odl_config2["hold_limit"] == 31
assert odl_config2["max_retry_count"] == 3
assert odl_config2["ebook_loan_duration"] is None
assert odl_config2["default_loan_duration"] == 12
# Unknown settings remain as-is
assert odl_config.settings["unchanged"] == "value"
assert odl_config2["unchanged"] == "value1"

# Other integration is unchanged
other_config = fetch_config(connection, other_config_id)
assert other_config == other_config_settings
other_library_config = fetch_library_config(
connection, parent_id=other_config_id, library_id=library_id2
)
assert other_library_config == other_config_settings

0 comments on commit 200f295

Please sign in to comment.