From 6de0b36472a6da5740f83e1b1ba5d143c14b04f3 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Wed, 13 Sep 2023 12:20:00 -0300 Subject: [PATCH 1/2] Remove external integration link code. --- api/admin/controller/settings.py | 91 ++--------------- core/model/__init__.py | 6 +- core/model/collection.py | 8 -- core/model/configuration.py | 81 --------------- tests/core/models/test_configuration.py | 128 ------------------------ tests/fixtures/database.py | 26 ----- 6 files changed, 8 insertions(+), 332 deletions(-) diff --git a/api/admin/controller/settings.py b/api/admin/controller/settings.py index 3b75a54431..3e84dc5eda 100644 --- a/api/admin/controller/settings.py +++ b/api/admin/controller/settings.py @@ -16,7 +16,6 @@ INTEGRATION_NAME_ALREADY_IN_USE, INTEGRATION_URL_ALREADY_IN_USE, INVALID_CONFIGURATION_OPTION, - MISSING_INTEGRATION, MISSING_SERVICE, NO_PROTOCOL_FOR_NEW_SERVICE, NO_SUCH_LIBRARY, @@ -37,7 +36,6 @@ from core.model import ( ConfigurationSetting, ExternalIntegration, - ExternalIntegrationLink, IntegrationConfiguration, IntegrationLibraryConfiguration, Library, @@ -58,66 +56,6 @@ class SettingsController(CirculationManagerController, AdminPermissionsControlle NO_MIRROR_INTEGRATION = "NO_MIRROR" - def _set_storage_external_integration_link( - self, service: ExternalIntegration, purpose: str, setting_key: str - ) -> Optional[ProblemDetail]: - """Either set or delete the external integration link between the - service and the storage integration. - - :param service: Service's ExternalIntegration object - - :param purpose: Service's purpose - - :param setting_key: Key of the configuration setting that must be set in the storage integration. - For example, a specific bucket (MARC, Analytics, etc.). - - :return: ProblemDetail object if the operation failed - """ - mirror_integration_id = flask.request.form.get("mirror_integration_id") - - if not mirror_integration_id: - return None - - # If no storage integration was selected, then delete the existing - # external integration link. - if mirror_integration_id == self.NO_MIRROR_INTEGRATION: - current_integration_link = get_one( - self._db, - ExternalIntegrationLink, - library_id=None, - external_integration_id=service.id, - purpose=purpose, - ) - - if current_integration_link: - self._db.delete(current_integration_link) - else: - storage_integration = get_one( - self._db, ExternalIntegration, id=mirror_integration_id - ) - - # Only get storage integrations that have a specific configuration setting set. - # For example: a specific bucket. - if ( - not storage_integration - or not storage_integration.setting(setting_key).value - ): - return MISSING_INTEGRATION - - current_integration_link_created, ignore = get_one_or_create( - self._db, - ExternalIntegrationLink, - library_id=None, - external_integration_id=service.id, - purpose=purpose, - ) - - current_integration_link_created.other_integration_id = ( - storage_integration.id - ) - - return None - def _get_settings_class( self, registry: IntegrationRegistry, protocol_name: str, is_child=False ) -> Type[BaseSettings] | ProblemDetail | None: @@ -233,29 +171,14 @@ def _get_integration_info(self, goal, protocols): settings = dict() for setting in protocol.get("settings", []): key = setting.get("key") - - # If the setting is a covers or books mirror, we need to get - # the value from ExternalIntegrationLink and - # not from a ConfigurationSetting. - if key.endswith("mirror_integration_id"): - storage_integration = get_one( - self._db, - ExternalIntegrationLink, - external_integration_id=service.id, - ) - if storage_integration: - value = str(storage_integration.other_integration_id) - else: - value = self.NO_MIRROR_INTEGRATION + if setting.get("type") in ("list", "menu"): + value = ConfigurationSetting.for_externalintegration( + key, service + ).json_value else: - if setting.get("type") in ("list", "menu"): - value = ConfigurationSetting.for_externalintegration( - key, service - ).json_value - else: - value = ConfigurationSetting.for_externalintegration( - key, service - ).value + value = ConfigurationSetting.for_externalintegration( + key, service + ).value settings[key] = value service_info = dict( diff --git a/core/model/__init__.py b/core/model/__init__.py index 069c48d324..8be9191579 100644 --- a/core/model/__init__.py +++ b/core/model/__init__.py @@ -546,11 +546,7 @@ def _bulk_operation(self): CollectionMissing, collections_identifiers, ) -from .configuration import ( - ConfigurationSetting, - ExternalIntegration, - ExternalIntegrationLink, -) +from .configuration import ConfigurationSetting, ExternalIntegration from .contributor import Contribution, Contributor from .coverage import BaseCoverageRecord, CoverageRecord, Timestamp, WorkCoverageRecord from .credential import Credential diff --git a/core/model/collection.py b/core/model/collection.py index a3a83329fd..a24f0e4e2c 100644 --- a/core/model/collection.py +++ b/core/model/collection.py @@ -1,7 +1,6 @@ # Collection, CollectionIdentifier, CollectionMissing from __future__ import annotations -import logging from abc import ABCMeta, abstractmethod from typing import TYPE_CHECKING, List, Optional @@ -947,13 +946,6 @@ def delete(self, search_index=None): # Delete the ExternalIntegration associated with this # Collection, assuming it wasn't deleted already. if self.external_integration: - for link in self.external_integration.links: - if link.other_integration and link.other_integration.goal == "storage": - logging.info( - f"Deletion of collection {self.name} is disassociating " - f"storage integration {link.other_integration.name}." - ) - _db.delete(self.external_integration) # Now delete the Collection itself. diff --git a/core/model/configuration.py b/core/model/configuration.py index 696368053e..fdc5efe454 100644 --- a/core/model/configuration.py +++ b/core/model/configuration.py @@ -30,39 +30,6 @@ from core.model import Collection # noqa: autoflake -class ExternalIntegrationLink(Base): - - __tablename__ = "externalintegrationslinks" - - NO_MIRROR_INTEGRATION = "NO_MIRROR" - # Possible purposes that a storage external integration can be used for. - # These string literals may be stored in the database, so changes to them - # may need to be accompanied by a DB migration. - COVERS = "covers_mirror" - COVERS_KEY = f"{COVERS}_integration_id" - - OPEN_ACCESS_BOOKS = "books_mirror" - OPEN_ACCESS_BOOKS_KEY = f"{OPEN_ACCESS_BOOKS}_integration_id" - - PROTECTED_ACCESS_BOOKS = "protected_access_books_mirror" - PROTECTED_ACCESS_BOOKS_KEY = f"{PROTECTED_ACCESS_BOOKS}_integration_id" - - ANALYTICS = "analytics_mirror" - ANALYTICS_KEY = f"{ANALYTICS}_integration_id" - - MARC = "MARC_mirror" - - id = Column(Integer, primary_key=True) - external_integration_id = Column( - Integer, ForeignKey("externalintegrations.id"), index=True - ) - library_id = Column(Integer, ForeignKey("libraries.id"), index=True) - other_integration_id = Column( - Integer, ForeignKey("externalintegrations.id"), index=True - ) - purpose = Column(Unicode, index=True) - - class ExternalIntegration(Base): """An external integration contains configuration for connecting @@ -233,20 +200,6 @@ class ExternalIntegration(Base): foreign_keys="Collection.external_integration_id", ) - links: Mapped[List[ExternalIntegrationLink]] = relationship( - "ExternalIntegrationLink", - backref="integration", - foreign_keys="ExternalIntegrationLink.external_integration_id", - cascade="all, delete-orphan", - ) - - other_links: Mapped[List[ExternalIntegrationLink]] = relationship( - "ExternalIntegrationLink", - backref="other_integration", - foreign_keys="ExternalIntegrationLink.other_integration_id", - cascade="all, delete-orphan", - ) - libraries: Mapped[List[Library]] = relationship( "Library", back_populates="integrations", @@ -269,40 +222,6 @@ def for_goal(cls, _db, goal): return integrations - @classmethod - def for_collection_and_purpose(cls, _db, collection, purpose): - """Find the ExternalIntegration for the collection. - - :param collection: Use the mirror configuration for this Collection. - :param purpose: Use the purpose of the mirror configuration. - """ - qu = ( - _db.query(cls) - .join( - ExternalIntegrationLink, - ExternalIntegrationLink.other_integration_id == cls.id, - ) - .filter( - ExternalIntegrationLink.external_integration_id - == collection.external_integration_id, - ExternalIntegrationLink.purpose == purpose, - ) - ) - integrations = qu.all() - if not integrations: - raise CannotLoadConfiguration( - "No storage integration for collection '%s' and purpose '%s' is configured." - % (collection.name, purpose) - ) - if len(integrations) > 1: - raise CannotLoadConfiguration( - "Multiple integrations found for collection '%s' and purpose '%s'" - % (collection.name, purpose) - ) - - [integration] = integrations - return integration - @classmethod def lookup(cls, _db, protocol, goal, library=None): diff --git a/tests/core/models/test_configuration.py b/tests/core/models/test_configuration.py index e7f7fe8ee6..41ae7d5ca0 100644 --- a/tests/core/models/test_configuration.py +++ b/tests/core/models/test_configuration.py @@ -18,7 +18,6 @@ ConfigurationSetting, ConfigurationStorage, ExternalIntegration, - ExternalIntegrationLink, HasExternalIntegration, ) from core.model.datasource import DataSource @@ -411,61 +410,6 @@ def test_duplicate_library_integration_setting( pytest.raises(IntegrityError, db.session.flush) -class TestExternalIntegrationLink: - def test_relationships(self, db: DatabaseTransactionFixture): - # Create a collection with two storage external integrations. - collection = db.collection( - name="Collection", - protocol=ExternalIntegration.OVERDRIVE, - ) - - storage1 = db.external_integration( - name="integration1", - protocol="protocol", - ) - storage2 = db.external_integration( - name="integration2", - protocol="protocol", - goal="storage", - username="username", - password="password", - ) - - # Two external integration links need to be created to associate - # the collection's external integration with the two storage - # external integrations. - s1_external_integration_link = db.external_integration_link( - integration=collection.external_integration, - other_integration=storage1, - purpose="covers_mirror", - ) - s2_external_integration_link = db.external_integration_link( - integration=collection.external_integration, - other_integration=storage2, - purpose="books_mirror", - ) - - qu = db.session.query(ExternalIntegrationLink).order_by( - ExternalIntegrationLink.other_integration_id - ) - external_integration_links = qu.all() - - assert len(external_integration_links) == 2 - assert external_integration_links[0].other_integration_id == storage1.id - assert external_integration_links[1].other_integration_id == storage2.id - - # When a storage integration is deleted, the related external - # integration link row is deleted, and the relationship with the - # collection is removed. - db.session.delete(storage1) - - qu = db.session.query(ExternalIntegrationLink) - external_integration_links = qu.all() - - assert len(external_integration_links) == 1 - assert external_integration_links[0].other_integration_id == storage2.id - - class ExampleExternalIntegrationFixture: external_integration: ExternalIntegration database_fixture: DatabaseTransactionFixture @@ -531,33 +475,6 @@ def test_for_library_and_goal( db.default_library().name, goal ) in str(excinfo.value) - def test_for_collection_and_purpose( - self, example_externalintegration_fixture: ExampleExternalIntegrationFixture - ): - db = example_externalintegration_fixture.database_fixture - wrong_purpose = "isbn" - collection = db.collection() - - with pytest.raises(CannotLoadConfiguration) as excinfo: - ExternalIntegration.for_collection_and_purpose( - db.session, collection, wrong_purpose - ) - assert ( - "No storage integration for collection '%s' and purpose '%s' is configured" - % (collection.name, wrong_purpose) - in str(excinfo.value) - ) - - external_integration = db.external_integration("some protocol") - collection.external_integration_id = external_integration.id - purpose = "covers_mirror" - db.external_integration_link(integration=external_integration, purpose=purpose) - - integration = ExternalIntegration.for_collection_and_purpose( - db.session, collection=collection, purpose=purpose - ) - assert isinstance(integration, ExternalIntegration) - def test_with_setting_value( self, example_externalintegration_fixture: ExampleExternalIntegrationFixture ): @@ -713,51 +630,6 @@ def test_custom_accept_header( integration.custom_accept_header = "custom header" assert integration.custom_accept_header == "custom header" - def test_delete( - self, example_externalintegration_fixture: ExampleExternalIntegrationFixture - ): - """Ensure that ExternalIntegration.delete clears all orphan ExternalIntegrationLinks.""" - session = example_externalintegration_fixture.database_fixture.session - db = example_externalintegration_fixture.database_fixture - - integration1 = db.external_integration( - "protocol", - ExternalIntegration.LICENSE_GOAL, - libraries=[db.default_library()], - ) - integration2 = db.external_integration( - "storage", - "storage goal", - libraries=[db.default_library()], - ) - - # Set up a link associating integration2 with integration1. - link1 = db.external_integration_link( - integration1, - db.default_library(), - integration2, - ExternalIntegrationLink.PROTECTED_ACCESS_BOOKS, - ) - link2 = db.external_integration_link( - integration1, - db.default_library(), - integration2, - ExternalIntegrationLink.COVERS, - ) - - # Delete integration1. - session.delete(integration1) - - # Ensure that there are no orphan links. - links = session.query(ExternalIntegrationLink).all() - for link in (link1, link2): - assert link not in links - - # Ensure that the first integration was successfully removed. - external_integrations = session.query(ExternalIntegration).all() - assert integration1 not in external_integrations - assert integration2 in external_integrations - SETTING1_KEY = "setting1" SETTING1_LABEL = "Setting 1's label" diff --git a/tests/fixtures/database.py b/tests/fixtures/database.py index ec1362ac24..2eaa1274b9 100644 --- a/tests/fixtures/database.py +++ b/tests/fixtures/database.py @@ -37,7 +37,6 @@ DeliveryMechanism, Edition, ExternalIntegration, - ExternalIntegrationLink, Genre, Hyperlink, Identifier, @@ -727,31 +726,6 @@ def external_integration( return integration - def external_integration_link( - self, - integration=None, - library=None, - other_integration=None, - purpose="covers_mirror", - ): - integration = integration or self.external_integration("some protocol") - other_integration = other_integration or self.external_integration( - "some other protocol" - ) - - library_id = library.id if library else None - - external_integration_link, ignore = get_one_or_create( - self.session, - ExternalIntegrationLink, - library_id=library_id, - external_integration_id=integration.id, - other_integration_id=other_integration.id, - purpose=purpose, - ) - - return external_integration_link - def integration_configuration( self, protocol: str, goal=None, libraries=None, name=None, **kwargs ): From 1563020fb0e866ad4d1a5ab7734c1b98d2a52c50 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Wed, 13 Sep 2023 12:26:59 -0300 Subject: [PATCH 2/2] Add migration. --- ...a80073d5_remove_externalintegrationlink.py | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 alembic/versions/20230913_5d71a80073d5_remove_externalintegrationlink.py diff --git a/alembic/versions/20230913_5d71a80073d5_remove_externalintegrationlink.py b/alembic/versions/20230913_5d71a80073d5_remove_externalintegrationlink.py new file mode 100644 index 0000000000..1bff2f4e0a --- /dev/null +++ b/alembic/versions/20230913_5d71a80073d5_remove_externalintegrationlink.py @@ -0,0 +1,90 @@ +"""Remove ExternalIntegrationLink. + +Revision ID: 5d71a80073d5 +Revises: 1c566151741f +Create Date: 2023-09-13 15:23:07.566404+00:00 + +""" +import sqlalchemy as sa + +from alembic import op + +# revision identifiers, used by Alembic. +revision = "5d71a80073d5" +down_revision = "1c566151741f" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.drop_index( + "ix_externalintegrationslinks_external_integration_id", + table_name="externalintegrationslinks", + ) + op.drop_index( + "ix_externalintegrationslinks_library_id", + table_name="externalintegrationslinks", + ) + op.drop_index( + "ix_externalintegrationslinks_other_integration_id", + table_name="externalintegrationslinks", + ) + op.drop_index( + "ix_externalintegrationslinks_purpose", table_name="externalintegrationslinks" + ) + op.drop_table("externalintegrationslinks") + + +def downgrade() -> None: + op.create_table( + "externalintegrationslinks", + sa.Column("id", sa.INTEGER(), autoincrement=True, nullable=False), + sa.Column( + "external_integration_id", sa.INTEGER(), autoincrement=False, nullable=True + ), + sa.Column("library_id", sa.INTEGER(), autoincrement=False, nullable=True), + sa.Column( + "other_integration_id", sa.INTEGER(), autoincrement=False, nullable=True + ), + sa.Column("purpose", sa.VARCHAR(), autoincrement=False, nullable=True), + sa.ForeignKeyConstraint( + ["external_integration_id"], + ["externalintegrations.id"], + name="externalintegrationslinks_external_integration_id_fkey", + ), + sa.ForeignKeyConstraint( + ["library_id"], + ["libraries.id"], + name="externalintegrationslinks_library_id_fkey", + ), + sa.ForeignKeyConstraint( + ["other_integration_id"], + ["externalintegrations.id"], + name="externalintegrationslinks_other_integration_id_fkey", + ), + sa.PrimaryKeyConstraint("id", name="externalintegrationslinks_pkey"), + ) + op.create_index( + "ix_externalintegrationslinks_purpose", + "externalintegrationslinks", + ["purpose"], + unique=False, + ) + op.create_index( + "ix_externalintegrationslinks_other_integration_id", + "externalintegrationslinks", + ["other_integration_id"], + unique=False, + ) + op.create_index( + "ix_externalintegrationslinks_library_id", + "externalintegrationslinks", + ["library_id"], + unique=False, + ) + op.create_index( + "ix_externalintegrationslinks_external_integration_id", + "externalintegrationslinks", + ["external_integration_id"], + unique=False, + )