diff --git a/.github/workflows/test-build.yml b/.github/workflows/test-build.yml index b6974df12b..20ce270cf9 100644 --- a/.github/workflows/test-build.yml +++ b/.github/workflows/test-build.yml @@ -269,18 +269,6 @@ jobs: matrix: platform: ["linux/amd64", "linux/arm64"] image: ["scripts", "webapp"] - env: - POSTGRES_USER: palace_user - POSTGRES_PASSWORD: test - POSTGRES_DB: palace_circulation - - services: - postgres: - image: postgres:12 - env: - POSTGRES_USER: ${{ env.POSTGRES_USER }} - POSTGRES_PASSWORD: ${{ env.POSTGRES_PASSWORD }} - POSTGRES_DB: ${{ env.POSTGRES_DB }} steps: - uses: actions/checkout@v4 @@ -297,36 +285,23 @@ jobs: - name: Set up Docker Buildx uses: docker/setup-buildx-action@v3 - - name: Build image - uses: docker/build-push-action@v5 - with: - context: . - file: ./docker/Dockerfile - tags: test_image - load: true - target: ${{ matrix.image }} - cache-from: type=gha,scope=buildkit-${{ github.run_id }} - platforms: ${{ matrix.platform }} - build-args: | - BASE_IMAGE=${{ needs.docker-image-build.outputs.baseimage }} - - - name: Start container - run: > - docker run --rm --name test_container -d --platform ${{ matrix.platform }} - --network ${{job.services.postgres.network}} - -e SIMPLIFIED_PRODUCTION_DATABASE="postgresql://${{ env.POSTGRES_USER }}:${{ env.POSTGRES_PASSWORD }}@postgres:5432/${{ env.POSTGRES_DB }}" - test_image + - name: Build & Start container + run: docker compose up -d --build ${{ matrix.image }} + env: + BUILD_PLATFORM: ${{ matrix.platform }} + BUILD_CACHE_FROM: type=gha,scope=buildkit-${{ github.run_id }} + BUILD_BASE_IMAGE: ${{ needs.docker-image-build.outputs.baseimage }} - name: Run tests - run: ./docker/ci/test_${{ matrix.image }}.sh test_container + run: ./docker/ci/test_${{ matrix.image }}.sh ${{ matrix.image }} - name: Output logs if: failure() - run: docker logs test_container + run: docker logs circulation-${{ matrix.image }}-1 - name: Stop container if: always() - run: docker stop test_container + run: docker compose down docker-image-push: name: Push circ-${{ matrix.image }} diff --git a/README.md b/README.md index bf3a3dab40..ccce54e544 100644 --- a/README.md +++ b/README.md @@ -120,7 +120,7 @@ Elasticsearch is no longer supported. We recommend that you run Opensearch with docker using the following docker commands: ```sh -docker run --name opensearch -d --rm -p 9006:9200 -e "discovery.type=single-node" -e "plugins.security.disabled=true" "opensearchproject/opensearch:1" +docker run --name opensearch -d --rm -p 9200:9200 -e "discovery.type=single-node" -e "plugins.security.disabled=true" "opensearchproject/opensearch:1" docker exec opensearch opensearch-plugin -s install analysis-icu docker restart opensearch ``` @@ -161,6 +161,22 @@ To let the application know which database to use, set the `SIMPLIFIED_PRODUCTIO export SIMPLIFIED_PRODUCTION_DATABASE="postgresql://palace:test@localhost:5432/circ" ``` +#### Opensearch + +To let the application know which Opensearch instance to use, you can set the following environment variables: + +- `PALACE_SEARCH_URL`: The url of the Opensearch instance (**required**). +- `PALACE_SEARCH_INDEX_PREFIX`: The prefix to use for the Opensearch indices. The default is `circulation-works`. + This is useful if you want to use the same Opensearch instance for multiple CM (optional). +- `PALACE_SEARCH_TIMEOUT`: The timeout in seconds to use when connecting to the Opensearch instance. The default is `20` + (optional). +- `PALACE_SEARCH_MAXSIZE`: The maximum size of the connection pool to use when connecting to the Opensearch instance. + (optional). + +```sh +export PALACE_SEARCH_URL="http://localhost:9200" +``` + #### Storage Service The application optionally uses a s3 compatible storage service to store files. To configure the application to use @@ -642,7 +658,7 @@ If you already have elastic search or postgres running locally, you can run them following environment variables: - `SIMPLIFIED_TEST_DATABASE` -- `SIMPLIFIED_TEST_OPENSEARCH` +- `PALACE_TEST_SEARCH_URL` Make sure the ports and usernames are updated to reflect the local configuration. diff --git a/api/admin/config.py b/api/admin/config.py index 9b3f8f2b5a..427d913c86 100644 --- a/api/admin/config.py +++ b/api/admin/config.py @@ -16,7 +16,7 @@ class OperationalMode(str, Enum): class Configuration(LoggerMixin): APP_NAME = "Palace Collection Manager" PACKAGE_NAME = "@thepalaceproject/circulation-admin" - PACKAGE_VERSION = "1.12.0" + PACKAGE_VERSION = "1.13.0" STATIC_ASSETS = { "admin_js": "circulation-admin.js", diff --git a/api/admin/controller/__init__.py b/api/admin/controller/__init__.py index ff613a1d51..02e9438cab 100644 --- a/api/admin/controller/__init__.py +++ b/api/admin/controller/__init__.py @@ -37,16 +37,9 @@ def setup_admin_controllers(manager: CirculationManager): ) from api.admin.controller.patron_auth_services import PatronAuthServicesController from api.admin.controller.reset_password import ResetPasswordController - from api.admin.controller.search_service_self_tests import ( - SearchServiceSelfTestsController, - ) from api.admin.controller.self_tests import SelfTestsController from api.admin.controller.settings import SettingsController from api.admin.controller.sign_in import SignInController - from api.admin.controller.sitewide_services import ( - SearchServicesController, - SitewideServicesController, - ) from api.admin.controller.sitewide_settings import ( SitewideConfigurationSettingsController, ) @@ -92,11 +85,6 @@ def setup_admin_controllers(manager: CirculationManager): manager.admin_individual_admin_settings_controller = ( IndividualAdminSettingsController(manager) ) - manager.admin_sitewide_services_controller = SitewideServicesController(manager) - manager.admin_search_service_self_tests_controller = ( - SearchServiceSelfTestsController(manager) - ) - manager.admin_search_services_controller = SearchServicesController(manager) manager.admin_catalog_services_controller = CatalogServicesController(manager) manager.admin_announcement_service = AnnouncementSettings(manager) manager.admin_search_controller = AdminSearchController(manager) diff --git a/api/admin/controller/search_service_self_tests.py b/api/admin/controller/search_service_self_tests.py deleted file mode 100644 index 2ec4385f9e..0000000000 --- a/api/admin/controller/search_service_self_tests.py +++ /dev/null @@ -1,28 +0,0 @@ -from flask_babel import lazy_gettext as _ - -from api.admin.controller.self_tests import SelfTestsController -from core.external_search import ExternalSearchIndex -from core.model import ExternalIntegration - - -class SearchServiceSelfTestsController(SelfTestsController): - def __init__(self, manager): - super().__init__(manager) - self.type = _("search service") - - def process_search_service_self_tests(self, identifier): - return self._manage_self_tests(identifier) - - def _find_protocol_class(self, integration): - # There's only one possibility for search integrations. - return ExternalSearchIndex, ( - None, - self._db, - ) - - def look_up_by_id(self, identifier): - return self.look_up_service_by_id( - identifier, - ExternalIntegration.OPENSEARCH, - ExternalIntegration.SEARCH_GOAL, - ) diff --git a/api/admin/controller/settings.py b/api/admin/controller/settings.py index c9e6ceaa91..39e3eadcb4 100644 --- a/api/admin/controller/settings.py +++ b/api/admin/controller/settings.py @@ -24,7 +24,6 @@ ) from api.admin.validator import Validator from api.controller.circulation_manager import CirculationManagerController -from core.external_search import ExternalSearchIndex from core.integration.base import ( HasChildIntegrationConfiguration, HasIntegrationConfiguration, @@ -406,11 +405,7 @@ def _get_prior_test_results(self, item, protocol_class=None, *extra_args): self_test_results = None try: - if self.type == "search service": - self_test_results = ExternalSearchIndex.prior_test_results( - self._db, None, self._db, item - ) - elif self.type == "metadata service" and protocol_class: + if self.type == "metadata service" and protocol_class: self_test_results = protocol_class.prior_test_results( self._db, *extra_args ) diff --git a/api/admin/controller/sitewide_services.py b/api/admin/controller/sitewide_services.py deleted file mode 100644 index 15f54cdec3..0000000000 --- a/api/admin/controller/sitewide_services.py +++ /dev/null @@ -1,127 +0,0 @@ -import flask -from flask import Response -from flask_babel import lazy_gettext as _ - -from api.admin.controller.settings import SettingsController -from api.admin.problem_details import ( - INCOMPLETE_CONFIGURATION, - MULTIPLE_SITEWIDE_SERVICES, - NO_PROTOCOL_FOR_NEW_SERVICE, - UNKNOWN_PROTOCOL, -) -from core.external_search import ExternalSearchIndex -from core.model import ExternalIntegration, get_one_or_create -from core.util.problem_detail import ProblemDetail - - -class SitewideServicesController(SettingsController): - def _manage_sitewide_service( - self, - goal, - provider_apis, - service_key_name, - multiple_sitewide_services_detail, - protocol_name_attr="NAME", - ): - protocols = self._get_integration_protocols( - provider_apis, protocol_name_attr=protocol_name_attr - ) - - self.require_system_admin() - if flask.request.method == "GET": - return self.process_get(protocols, goal, service_key_name) - else: - return self.process_post(protocols, goal, multiple_sitewide_services_detail) - - def process_get(self, protocols, goal, service_key_name): - services = self._get_integration_info(goal, protocols) - return { - service_key_name: services, - "protocols": protocols, - } - - def process_post(self, protocols, goal, multiple_sitewide_services_detail): - name = flask.request.form.get("name") - protocol = flask.request.form.get("protocol") - fields = {"name": name, "protocol": protocol} - form_field_error = self.validate_form_fields(protocols, **fields) - if form_field_error: - return form_field_error - - settings = protocols[0].get("settings") - wrong_format = self.validate_formats(settings) - if wrong_format: - return wrong_format - - is_new = False - id = flask.request.form.get("id") - - if id: - # Find an existing service in order to edit it - service = self.look_up_service_by_id(id, protocol, goal) - else: - if protocol: - service, is_new = get_one_or_create( - self._db, ExternalIntegration, protocol=protocol, goal=goal - ) - # There can only be one of each sitewide service. - if not is_new: - self._db.rollback() - return MULTIPLE_SITEWIDE_SERVICES.detailed( - multiple_sitewide_services_detail - ) - else: - return NO_PROTOCOL_FOR_NEW_SERVICE - - if isinstance(service, ProblemDetail): - self._db.rollback() - return service - - name_error = self.check_name_unique(service, name) - if name_error: - self._db.rollback() - return name_error - - protocol_error = self.set_protocols(service, protocol, protocols) - if protocol_error: - self._db.rollback() - return protocol_error - - service.name = name - - if is_new: - return Response(str(service.id), 201) - else: - return Response(str(service.id), 200) - - def validate_form_fields(self, protocols, **fields): - """The 'name' and 'protocol' fields cannot be blank, and the protocol must - be selected from the list of recognized protocols.""" - - name = fields.get("name") - protocol = fields.get("protocol") - - if not name: - return INCOMPLETE_CONFIGURATION - if protocol and protocol not in [p.get("name") for p in protocols]: - return UNKNOWN_PROTOCOL - - -class SearchServicesController(SitewideServicesController): - def __init__(self, manager): - super().__init__(manager) - self.type = _("search service") - - def process_services(self): - detail = _( - "You tried to create a new search service, but a search service is already configured." - ) - return self._manage_sitewide_service( - ExternalIntegration.SEARCH_GOAL, - [ExternalSearchIndex], - "search_services", - detail, - ) - - def process_delete(self, service_id): - return self._delete_integration(service_id, ExternalIntegration.SEARCH_GOAL) diff --git a/api/admin/controller/work_editor.py b/api/admin/controller/work_editor.py index 7363cdc7e1..40b236f478 100644 --- a/api/admin/controller/work_editor.py +++ b/api/admin/controller/work_editor.py @@ -699,6 +699,6 @@ def custom_lists(self, identifier_type, identifier): # NOTE: This may not make a difference until the # works are actually re-indexed. for lane in affected_lanes: - lane.update_size(self._db, self.search_engine) + lane.update_size(self._db, search_engine=self.search_engine) return Response(str(_("Success")), 200) diff --git a/api/admin/routes.py b/api/admin/routes.py index d9de20fc3a..8b37d5441e 100644 --- a/api/admin/routes.py +++ b/api/admin/routes.py @@ -484,32 +484,6 @@ def metadata_service_self_tests(identifier): ) -@app.route("/admin/search_services", methods=["GET", "POST"]) -@returns_json_or_response_or_problem_detail -@requires_admin -@requires_csrf_token -def search_services(): - return app.manager.admin_search_services_controller.process_services() - - -@app.route("/admin/search_service/", methods=["DELETE"]) -@returns_json_or_response_or_problem_detail -@requires_admin -@requires_csrf_token -def search_service(service_id): - return app.manager.admin_search_services_controller.process_delete(service_id) - - -@app.route("/admin/search_service_self_tests/", methods=["GET", "POST"]) -@returns_json_or_response_or_problem_detail -@requires_admin -@requires_csrf_token -def search_service_self_tests(identifier): - return app.manager.admin_search_service_self_tests_controller.process_search_service_self_tests( - identifier - ) - - @app.route("/admin/catalog_services", methods=["GET", "POST"]) @returns_json_or_response_or_problem_detail @requires_admin diff --git a/api/circulation_manager.py b/api/circulation_manager.py index b8df1ce48c..ddf36d173d 100644 --- a/api/circulation_manager.py +++ b/api/circulation_manager.py @@ -31,7 +31,6 @@ from api.problem_details import * from api.saml.controller import SAMLController from core.app_server import ApplicationVersionController, load_facets_from_request -from core.external_search import ExternalSearchIndex from core.feed.annotator.circulation import ( CirculationManagerAnnotator, LibraryAnnotator, @@ -72,16 +71,9 @@ from api.admin.controller.patron_auth_services import PatronAuthServicesController from api.admin.controller.quicksight import QuickSightController from api.admin.controller.reset_password import ResetPasswordController - from api.admin.controller.search_service_self_tests import ( - SearchServiceSelfTestsController, - ) from api.admin.controller.self_tests import SelfTestsController from api.admin.controller.settings import SettingsController from api.admin.controller.sign_in import SignInController - from api.admin.controller.sitewide_services import ( - SearchServicesController, - SitewideServicesController, - ) from api.admin.controller.sitewide_settings import ( SitewideConfigurationSettingsController, ) @@ -130,9 +122,6 @@ class CirculationManager(LoggerMixin): admin_sitewide_configuration_settings_controller: SitewideConfigurationSettingsController admin_library_settings_controller: LibrarySettingsController admin_individual_admin_settings_controller: IndividualAdminSettingsController - admin_sitewide_services_controller: SitewideServicesController - admin_search_service_self_tests_controller: SearchServiceSelfTestsController - admin_search_services_controller: SearchServicesController admin_catalog_services_controller: CatalogServicesController admin_announcement_service: AnnouncementSettings admin_search_controller: AdminSearchController @@ -148,6 +137,7 @@ def __init__( self._db = _db self.services = services self.analytics = services.analytics.analytics() + self.external_search = services.search.index() self.site_configuration_last_update = ( Configuration.site_configuration_last_update(self._db, timeout=0) ) @@ -214,8 +204,6 @@ def load_settings(self): self.auth = Authenticator(self._db, libraries, self.analytics) - self.setup_external_search() - # Track the Lane configuration for each library by mapping its # short name to the top-level lane. new_top_level_lanes = {} @@ -249,14 +237,9 @@ def get_domain(url): url = url.strip() if url == "*": return url - ( - scheme, - netloc, - path, - parameters, - query, - fragment, - ) = urllib.parse.urlparse(url) + scheme, netloc, path, parameters, query, fragment = urllib.parse.urlparse( + url + ) if scheme and netloc: return scheme + "://" + netloc else: @@ -290,28 +273,6 @@ def get_domain(url): max_len=1000, max_age_seconds=authentication_document_cache_time ) - @property - def external_search(self): - """Retrieve or create a connection to the search interface. - - This is created lazily so that a failure to connect only - affects feeds that depend on the search engine, not the whole - circulation manager. - """ - if not self._external_search: - self.setup_external_search() - return self._external_search - - def setup_external_search(self): - try: - self._external_search = self.setup_search() - self.external_search_initialization_exception = None - except Exception as e: - self.log.error("Exception initializing search engine: %s", e) - self._external_search = None - self.external_search_initialization_exception = e - return self._external_search - def log_lanes(self, lanelist=None, level=0): """Output information about the lane layout.""" lanelist = lanelist or self.top_level_lane.sublanes @@ -320,14 +281,6 @@ def log_lanes(self, lanelist=None, level=0): if lane.sublanes: self.log_lanes(lane.sublanes, level + 1) - def setup_search(self): - """Set up a search client.""" - search = ExternalSearchIndex(self._db) - if not search: - self.log.warn("No external search server configured.") - return None - return search - def setup_circulation(self, library, analytics): """Set up the Circulation object.""" return CirculationAPI(self._db, library, analytics=analytics) diff --git a/bin/search_index_refresh b/bin/search_index_refresh index f0dfb2a865..eb78dc0903 100755 --- a/bin/search_index_refresh +++ b/bin/search_index_refresh @@ -8,7 +8,7 @@ import sys bin_dir = os.path.split(__file__)[0] package_dir = os.path.join(bin_dir, "..") sys.path.append(os.path.abspath(package_dir)) -from core.external_search import SearchIndexCoverageProvider from core.scripts import RunWorkCoverageProviderScript +from core.search.coverage_provider import SearchIndexCoverageProvider RunWorkCoverageProviderScript(SearchIndexCoverageProvider).run() diff --git a/core/coverage.py b/core/coverage.py index 556d993c38..3160b02c7c 100644 --- a/core/coverage.py +++ b/core/coverage.py @@ -22,6 +22,7 @@ get_one, ) from core.model.coverage import EquivalencyCoverageRecord +from core.service.container import container_instance from core.util.datetime_helpers import utc_now from core.util.worker_pools import DatabaseJob @@ -201,6 +202,10 @@ def __init__( self.registered_only = registered_only self.collection_id = None + # Call init_resources() to initialize the logging configuration. + self.services = container_instance() + self.services.init_resources() + @property def log(self): if not hasattr(self, "_log"): diff --git a/core/external_search.py b/core/external_search.py index c3fe8af237..fa0aea5337 100644 --- a/core/external_search.py +++ b/core/external_search.py @@ -1,18 +1,15 @@ from __future__ import annotations -import contextlib import datetime import json -import logging import re import time from collections import defaultdict -from collections.abc import Callable, Iterable -from typing import Any +from collections.abc import Iterable from attr import define from flask_babel import lazy_gettext as _ -from opensearch_dsl import SF, Search +from opensearch_dsl import SF from opensearch_dsl.query import ( Bool, DisMax, @@ -27,7 +24,6 @@ ) from opensearch_dsl.query import Query as BaseQuery from opensearch_dsl.query import Range, Regexp, Term, Terms -from opensearchpy import OpenSearch from spellchecker import SpellChecker from core.classifier import ( @@ -36,131 +32,44 @@ GradeLevelClassifier, KeywordBasedClassifier, ) -from core.config import CannotLoadConfiguration -from core.coverage import CoverageFailure, WorkPresentationProvider from core.facets import FacetConstants from core.lane import Pagination from core.metadata_layer import IdentifierData from core.model import ( - Collection, ConfigurationSetting, Contributor, DataSource, Edition, - ExternalIntegration, Identifier, Library, Work, - WorkCoverageRecord, numericrange_to_tuple, ) from core.problem_details import INVALID_INPUT -from core.search.coverage_remover import RemovesSearchCoverage from core.search.migrator import ( SearchDocumentReceiver, - SearchDocumentReceiverType, SearchMigrationInProgress, SearchMigrator, ) -from core.search.revision import SearchSchemaRevision from core.search.revision_directory import SearchRevisionDirectory -from core.search.service import SearchService, SearchServiceOpensearch1 -from core.selftest import HasSelfTests +from core.search.service import SearchService from core.util import Values from core.util.cache import CachedData from core.util.datetime_helpers import from_timestamp from core.util.languages import LanguageNames +from core.util.log import LoggerMixin from core.util.personal_names import display_name_to_sort_name from core.util.problem_detail import ProblemDetail from core.util.stopwords import ENGLISH_STOPWORDS -@contextlib.contextmanager -def mock_search_index(mock=None): - """Temporarily mock the ExternalSearchIndex implementation - returned by the load() class method. - """ - try: - ExternalSearchIndex.MOCK_IMPLEMENTATION = mock - yield mock - finally: - ExternalSearchIndex.MOCK_IMPLEMENTATION = None - - -class ExternalSearchIndex(HasSelfTests): - NAME = ExternalIntegration.OPENSEARCH - - # A test may temporarily set this to a mock of this class. - # While that's true, load() will return the mock instead of - # instantiating new ExternalSearchIndex objects. - MOCK_IMPLEMENTATION = None - - WORKS_INDEX_PREFIX_KEY = "works_index_prefix" - DEFAULT_WORKS_INDEX_PREFIX = "circulation-works" - - TEST_SEARCH_TERM_KEY = "a search term" - DEFAULT_TEST_SEARCH_TERM = "test" - CURRENT_ALIAS_SUFFIX = "current" - - SETTINGS = [ - { - "key": ExternalIntegration.URL, - "label": _("URL"), - "required": True, - "format": "url", - }, - { - "key": WORKS_INDEX_PREFIX_KEY, - "label": _("Index prefix"), - "default": DEFAULT_WORKS_INDEX_PREFIX, - "required": True, - "description": _( - "Any Search indexes needed for this application will be created with this unique prefix. In most cases, the default will work fine. You may need to change this if you have multiple application servers using a single Search server." - ), - }, - { - "key": TEST_SEARCH_TERM_KEY, - "label": _("Test search term"), - "default": DEFAULT_TEST_SEARCH_TERM, - "description": _("Self tests will use this value as the search term."), - }, - ] - - SITEWIDE = True - - @classmethod - def search_integration(cls, _db) -> ExternalIntegration | None: - """Look up the ExternalIntegration for Opensearch.""" - return ExternalIntegration.lookup( - _db, ExternalIntegration.OPENSEARCH, goal=ExternalIntegration.SEARCH_GOAL - ) - - @classmethod - def load(cls, _db, *args, **kwargs): - """Load a generic implementation.""" - if cls.MOCK_IMPLEMENTATION: - return cls.MOCK_IMPLEMENTATION - return cls(_db, *args, **kwargs) - - _bulk: Callable[..., Any] - _revision: SearchSchemaRevision - _revision_base_name: str - _revision_directory: SearchRevisionDirectory - _search: Search - _search_migrator: SearchMigrator - _search_service: SearchService - _search_read_pointer: str - _test_search_term: str - +class ExternalSearchIndex(LoggerMixin): def __init__( self, - _db, - url: str | None = None, - test_search_term: str | None = None, - revision_directory: SearchRevisionDirectory | None = None, + service: SearchService, + revision_directory: SearchRevisionDirectory, version: int | None = None, - custom_client_service: SearchService | None = None, - ): + ) -> None: """Constructor :param revision_directory Override the directory of revisions that will be used. If this isn't provided, @@ -168,57 +77,16 @@ def __init__( :param version The specific revision that will be used. If not specified, the highest version in the revision directory will be used. """ - self.log = logging.getLogger("External search index") - - # We can't proceed without a database. - if not _db: - raise CannotLoadConfiguration( - "Cannot load Search configuration without a database.", - ) - - # Load the search integration. - integration = self.search_integration(_db) - if not integration: - raise CannotLoadConfiguration("No search integration configured.") - - if not url: - url = url or integration.url - test_search_term = integration.setting(self.TEST_SEARCH_TERM_KEY).value - - self._test_search_term = test_search_term or self.DEFAULT_TEST_SEARCH_TERM - - if not url: - raise CannotLoadConfiguration("No URL configured to the search server.") - - # Determine the base name we're going to use for storing revisions. - self._revision_base_name = integration.setting( - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY - ).value - - # Create the necessary search client, and the service used by the schema migrator. - if custom_client_service: - self._search_service = custom_client_service - else: - use_ssl = url.startswith("https://") - self.log.info("Connecting to the search cluster at %s", url) - new_client = OpenSearch(url, use_ssl=use_ssl, timeout=20, maxsize=25) - self._search_service = SearchServiceOpensearch1( - new_client, self._revision_base_name - ) + self._search_service = service # Locate the revision of the search index that we're going to use. # This will fail fast if the requested version isn't available. - self._revision_directory = ( - revision_directory or SearchRevisionDirectory.create() - ) + self._revision_directory = revision_directory if version: self._revision = self._revision_directory.find(version) else: self._revision = self._revision_directory.highest() - # initialize the cached data if not already done so - CachedData.initialize(_db) - # Get references to the read and write pointers. self._search_read_pointer = self._search_service.read_pointer_name() self._search_write_pointer = self._search_service.write_pointer_name() @@ -234,7 +102,8 @@ def start_migration(self) -> SearchMigrationInProgress | None: service=self._search_service, ) return migrator.migrate( - base_name=self._revision_base_name, version=self._revision.version + base_name=self._search_service.base_revision_name, + version=self._revision.version, ) def start_updating_search_documents(self) -> SearchDocumentReceiver: @@ -246,9 +115,6 @@ def start_updating_search_documents(self) -> SearchDocumentReceiver: def clear_search_documents(self) -> None: self._search_service.index_clear_documents(pointer=self._search_write_pointer) - def prime_query_values(self, _db): - JSONQuery.data_sources = _db.query(DataSource).all() - def create_search_doc(self, query_string, filter, pagination, debug): if filter and filter.search_type == "json": query = JSONQuery(query_string, filter) @@ -419,74 +285,6 @@ def remove_work(self, work): pointer=self._search_read_pointer, id=work.id ) - def _run_self_tests(self, _db): - # Helper methods for setting up the self-tests: - - def _search(): - return self.create_search_doc( - self._test_search_term, filter=None, pagination=None, debug=True - ) - - def _works(): - return self.query_works( - self._test_search_term, filter=None, pagination=None, debug=True - ) - - # The self-tests: - - def _search_for_term(): - titles = [(f"{x.sort_title} ({x.sort_author})") for x in _works()] - return titles - - yield self.run_test( - ("Search results for '%s':" % self._test_search_term), _search_for_term - ) - - def _get_raw_doc(): - search = _search() - return json.dumps(search.to_dict(), indent=1) - - yield self.run_test( - ("Search document for '%s':" % (self._test_search_term)), _get_raw_doc - ) - - def _get_raw_results(): - return [json.dumps(x.to_dict(), indent=1) for x in _works()] - - yield self.run_test( - ("Raw search results for '%s':" % (self._test_search_term)), - _get_raw_results, - ) - - def _count_docs(): - service = self.search_service() - client = service.search_client() - return str(client.count()) - - yield self.run_test( - ("Total number of search results for '%s':" % (self._test_search_term)), - _count_docs, - ) - - def _total_count(): - return str(self.count_works(None)) - - yield self.run_test( - "Total number of documents in this search index:", _total_count - ) - - def _collections(): - result = {} - - collections = _db.query(Collection) - for collection in collections: - filter = Filter(collections=[collection]) - result[collection.name] = self.count_works(filter) - - return json.dumps(result, indent=1) - - yield self.run_test("Total number of documents per collection:", _collections) - def initialize_indices(self) -> bool: """Attempt to initialize the indices and pointers for a first time run""" service = self.search_service() @@ -2742,73 +2540,3 @@ def __init__(self, work, hit): def __getattr__(self, k): return getattr(self._work, k) - - -class SearchIndexCoverageProvider(RemovesSearchCoverage, WorkPresentationProvider): - """Make sure all Works have up-to-date representation in the - search index. - """ - - SERVICE_NAME = "Search index coverage provider" - - DEFAULT_BATCH_SIZE = 500 - - OPERATION = WorkCoverageRecord.UPDATE_SEARCH_INDEX_OPERATION - - def __init__(self, *args, **kwargs): - search_index_client = kwargs.pop("search_index_client", None) - super().__init__(*args, **kwargs) - self.search_index_client = search_index_client or ExternalSearchIndex(self._db) - - # - # Try to migrate to the latest schema. If the function returns None, it means - # that no migration is necessary, and we're already at the latest version. If - # we're already at the latest version, then simply upload search documents instead. - # - self.receiver = None - self.migration: None | ( - SearchMigrationInProgress - ) = self.search_index_client.start_migration() - if self.migration is None: - self.receiver: SearchDocumentReceiver = ( - self.search_index_client.start_updating_search_documents() - ) - else: - # We do have a migration, we must clear out the index and repopulate the index - self.remove_search_coverage_records() - - def on_completely_finished(self): - # Tell the search migrator that no more documents are going to show up. - target: SearchDocumentReceiverType = self.migration or self.receiver - target.finish() - - def run_once_and_update_timestamp(self): - # We do not catch exceptions here, so that the on_completely finished should not run - # if there was a runtime error - result = super().run_once_and_update_timestamp() - self.on_completely_finished() - return result - - def process_batch(self, works) -> list[Work | CoverageFailure]: - target: SearchDocumentReceiverType = self.migration or self.receiver - failures = target.add_documents( - documents=self.search_index_client.create_search_documents_from_works(works) - ) - - # Maintain a dictionary of works so that we can efficiently remove failed works later. - work_map: dict[int, Work] = {} - for work in works: - work_map[work.id] = work - - # Remove all the works that failed and create failure records for them. - results: list[Work | CoverageFailure] = [] - for failure in failures: - work = work_map[failure.id] - del work_map[failure.id] - results.append(CoverageFailure(work, repr(failure))) - - # Append all the remaining works that didn't fail. - for work in work_map.values(): - results.append(work) - - return results diff --git a/core/feed/acquisition.py b/core/feed/acquisition.py index 476b9566c9..68dba28292 100644 --- a/core/feed/acquisition.py +++ b/core/feed/acquisition.py @@ -5,6 +5,7 @@ from collections.abc import Callable, Generator from typing import TYPE_CHECKING, Any +from dependency_injector.wiring import Provide, inject from sqlalchemy.orm import Query, Session from api.problem_details import NOT_FOUND_ON_REMOTE @@ -426,6 +427,7 @@ def error_message( # Each classmethod creates a different kind of feed @classmethod + @inject def page( cls, _db: Session, @@ -435,7 +437,7 @@ def page( annotator: CirculationManagerAnnotator, facets: FacetsWithEntryPoint | None, pagination: Pagination | None, - search_engine: ExternalSearchIndex | None, + search_engine: ExternalSearchIndex = Provide["search.index"], ) -> OPDSAcquisitionFeed: works = worklist.works( _db, facets=facets, pagination=pagination, search_engine=search_engine @@ -653,6 +655,7 @@ def single_entry( return None @classmethod + @inject def groups( cls, _db: Session, @@ -662,7 +665,7 @@ def groups( annotator: LibraryAnnotator, pagination: Pagination | None = None, facets: FacetsWithEntryPoint | None = None, - search_engine: ExternalSearchIndex | None = None, + search_engine: ExternalSearchIndex = Provide["search.index"], search_debug: bool = False, ) -> OPDSAcquisitionFeed: """Internal method called by groups() when a grouped feed diff --git a/core/lane.py b/core/lane.py index 4d04fcd3f9..29d7b88f60 100644 --- a/core/lane.py +++ b/core/lane.py @@ -4,9 +4,10 @@ import logging import time from collections import defaultdict -from typing import Any +from typing import TYPE_CHECKING, Any from urllib.parse import quote_plus +from dependency_injector.wiring import Provide, inject from flask_babel import lazy_gettext as _ from opensearchpy.exceptions import OpenSearchException from sqlalchemy import ( @@ -69,6 +70,9 @@ from core.util.opds_writer import OPDSFeed from core.util.problem_detail import ProblemDetail +if TYPE_CHECKING: + from core.external_search import ExternalSearchIndex, WorkSearchResult + class BaseFacets(FacetConstants): """Basic faceting class that doesn't modify a search filter at all. @@ -1745,13 +1749,14 @@ def overview_facets(self, _db, facets): """ return facets + @inject def groups( self, _db, include_sublanes=True, pagination=None, facets=None, - search_engine=None, + search_engine: ExternalSearchIndex = Provide["search.index"], debug=False, ): """Extract a list of samples from each child of this WorkList. This @@ -1814,12 +1819,13 @@ def groups( ): yield work, worklist + @inject def works( self, _db, facets=None, pagination=None, - search_engine=None, + search_engine: ExternalSearchIndex = Provide["search.index"], debug=False, **kwargs, ): @@ -1842,9 +1848,6 @@ def works( that generates such a list when executed. """ - from core.external_search import ExternalSearchIndex - - search_engine = search_engine or ExternalSearchIndex.load(_db) filter = self.filter(_db, facets) hits = search_engine.query_works( query_string=None, filter=filter, pagination=pagination, debug=debug @@ -1997,6 +2000,7 @@ def search( return results + @inject def _groups_for_lanes( self, _db, @@ -2004,7 +2008,7 @@ def _groups_for_lanes( queryable_lanes, pagination, facets, - search_engine=None, + search_engine: ExternalSearchIndex = Provide["search.index"], debug=False, ): """Ask the search engine for groups of featurable works in the @@ -2041,10 +2045,6 @@ def _groups_for_lanes( else: target_size = pagination.size - from core.external_search import ExternalSearchIndex - - search_engine = search_engine or ExternalSearchIndex.load(_db) - if isinstance(self, Lane): parent_lane = self else: @@ -2075,15 +2075,15 @@ def _done_with_lane(lane): by_lane[lane].extend(list(might_need_to_reuse.values())[:num_missing]) used_works = set() - by_lane = defaultdict(list) + by_lane: dict[Lane, list[WorkSearchResult]] = defaultdict(list) working_lane = None - might_need_to_reuse = dict() + might_need_to_reuse: dict[int, WorkSearchResult] = dict() for work, lane in works_and_lanes: if lane != working_lane: # Either we're done with the old lane, or we're just # starting and there was no old lane. if working_lane: - _done_with_lane(working_lane) + _done_with_lane(working_lane) # type: ignore[unreachable] working_lane = lane used_works_this_lane = set() might_need_to_reuse = dict() @@ -2918,12 +2918,12 @@ def uses_customlists(self): return True return False - def update_size(self, _db, search_engine=None): + @inject + def update_size( + self, _db, search_engine: ExternalSearchIndex = Provide["search.index"] + ): """Update the stored estimate of the number of Works in this Lane.""" library = self.get_library(_db) - from core.external_search import ExternalSearchIndex - - search_engine = search_engine or ExternalSearchIndex.load(_db) # Do the estimate for every known entry point. by_entrypoint = dict() @@ -3130,13 +3130,14 @@ def _size_for_facets(self, facets): size = self.size_by_entrypoint[entrypoint_name] return size + @inject def groups( self, _db, include_sublanes=True, pagination=None, facets=None, - search_engine=None, + search_engine: ExternalSearchIndex = Provide["search.index"], debug=False, ): """Return a list of (Work, Lane) 2-tuples @@ -3169,7 +3170,6 @@ def groups( queryable_lanes, pagination=pagination, facets=facets, - search_engine=search_engine, debug=debug, ) diff --git a/core/metadata_layer.py b/core/metadata_layer.py index 9368a522d6..442d754b18 100644 --- a/core/metadata_layer.py +++ b/core/metadata_layer.py @@ -42,7 +42,6 @@ get_one_or_create, ) from core.model.licensing import LicenseFunctions, LicenseStatus -from core.service.container import Services from core.util import LanguageCodes from core.util.datetime_helpers import to_utc, utc_now from core.util.median import median @@ -83,7 +82,7 @@ def __init__( @classmethod @inject def from_license_source( - cls, _db, analytics: Analytics = Provide[Services.analytics.analytics], **args + cls, _db, analytics: Analytics = Provide["analytics.analytics"], **args ): """When gathering data from the license source, overwrite all old data from this source with new data from the same source. Also diff --git a/core/model/collection.py b/core/model/collection.py index b9876ba4ed..82a8b07ead 100644 --- a/core/model/collection.py +++ b/core/model/collection.py @@ -3,6 +3,7 @@ from collections.abc import Generator from typing import TYPE_CHECKING, Any, TypeVar +from dependency_injector.wiring import Provide, inject from sqlalchemy import ( Boolean, Column, @@ -570,7 +571,10 @@ def restrict_to_ready_deliverable_works( ) return query - def delete(self, search_index: ExternalSearchIndex | None = None) -> None: + @inject + def delete( + self, search_index: ExternalSearchIndex = Provide["search.index"] + ) -> None: """Delete a collection. Collections can have hundreds of thousands of @@ -599,7 +603,7 @@ def delete(self, search_index: ExternalSearchIndex | None = None) -> None: # https://docs.sqlalchemy.org/en/14/orm/cascades.html#notes-on-delete-deleting-objects-referenced-from-collections-and-scalar-relationships work.license_pools.remove(pool) if not work.license_pools: - work.delete(search_index) + work.delete(search_index=search_index) _db.delete(pool) diff --git a/core/model/configuration.py b/core/model/configuration.py index 079b5222b2..fe27a01cd8 100644 --- a/core/model/configuration.py +++ b/core/model/configuration.py @@ -287,7 +287,7 @@ def setting(self, key): """ return ConfigurationSetting.for_externalintegration(key, self) - @hybrid_property + @property def url(self): return self.setting(self.URL).value @@ -295,7 +295,7 @@ def url(self): def url(self, new_url): self.set_setting(self.URL, new_url) - @hybrid_property + @property def username(self): return self.setting(self.USERNAME).value @@ -303,7 +303,7 @@ def username(self): def username(self, new_username): self.set_setting(self.USERNAME, new_username) - @hybrid_property + @property def password(self): return self.setting(self.PASSWORD).value diff --git a/core/model/patron.py b/core/model/patron.py index 445657991f..ec756015d3 100644 --- a/core/model/patron.py +++ b/core/model/patron.py @@ -272,7 +272,7 @@ def is_last_loan_activity_stale(self) -> bool: seconds=self.loan_activity_max_age ) - @hybrid_property + @property def last_loan_activity_sync(self): """When was the last time we asked the vendors about this patron's loan activity? diff --git a/core/model/work.py b/core/model/work.py index d53c0e2cfe..36f59c3107 100644 --- a/core/model/work.py +++ b/core/model/work.py @@ -10,6 +10,7 @@ from typing import TYPE_CHECKING, Any, cast import pytz +from dependency_injector.wiring import Provide, inject from sqlalchemy import ( Boolean, Column, @@ -30,7 +31,6 @@ from sqlalchemy.sql.functions import func from core.classifier import Classifier, WorkClassifier -from core.config import CannotLoadConfiguration from core.model import ( Base, PresentationCalculationPolicy, @@ -58,6 +58,7 @@ # Import related models when doing type checking if TYPE_CHECKING: + from core.external_search import ExternalSearchIndex from core.model import CustomListEntry, Library, LicensePool @@ -2169,19 +2170,13 @@ def top_genre(self): ) return genre.name if genre else None - def delete(self, search_index=None): + @inject + def delete( + self, search_index: ExternalSearchIndex = Provide["search.index"] + ) -> None: """Delete the work from both the DB and search index.""" _db = Session.object_session(self) - if search_index is None: - try: - from core.external_search import ExternalSearchIndex - - search_index = ExternalSearchIndex(_db) - except CannotLoadConfiguration as e: - # No search index is configured. This is fine -- just skip that part. - pass - if search_index is not None: - search_index.remove_work(self) + search_index.remove_work(self) _db.delete(self) diff --git a/core/monitor.py b/core/monitor.py index 4f664757c9..b7e3b011d2 100644 --- a/core/monitor.py +++ b/core/monitor.py @@ -920,11 +920,9 @@ class WorkReaper(ReaperMonitor): MODEL_CLASS = Work def __init__(self, *args, **kwargs): - from core.external_search import ExternalSearchIndex - search_index_client = kwargs.pop("search_index_client", None) super().__init__(*args, **kwargs) - self.search_index_client = search_index_client or ExternalSearchIndex(self._db) + self.search_index_client = search_index_client or self.services.search.index() def query(self): return ( @@ -935,7 +933,7 @@ def query(self): def delete(self, work): """Delete work from opensearch and database.""" - work.delete(self.search_index_client) + work.delete(search_index=self.search_index_client) ReaperMonitor.REGISTRY.append(WorkReaper) diff --git a/core/query/customlist.py b/core/query/customlist.py index 8a50bafd49..fb3a4ab87e 100644 --- a/core/query/customlist.py +++ b/core/query/customlist.py @@ -4,6 +4,8 @@ import json from typing import TYPE_CHECKING +from dependency_injector.wiring import Provide, inject + from api.admin.problem_details import ( CUSTOMLIST_ENTRY_NOT_VALID_FOR_LIBRARY, CUSTOMLIST_SOURCE_COLLECTION_MISSING, @@ -13,6 +15,7 @@ from core.model.customlist import CustomList, CustomListEntry from core.model.library import Library from core.model.licensing import LicensePool +from core.service.container import Services from core.util.log import LoggerMixin from core.util.problem_detail import ProblemDetail @@ -60,6 +63,7 @@ def share_locally_with_library( return True @classmethod + @inject def populate_query_pages( cls, _db: Session, @@ -68,9 +72,10 @@ def populate_query_pages( max_pages: int = 100000, page_size: int = 100, json_query: dict | None = None, + search: ExternalSearchIndex = Provide[Services.search.index], ) -> int: """Populate the custom list while paging through the search query results - :param _db: The database conenction + :param _db: The database connection :param custom_list: The list to be populated :param start_page: Offset of the search will be used from here (based on page_size) :param max_pages: Maximum number of pages to search through @@ -78,11 +83,8 @@ def populate_query_pages( :param json_query: If provided, use this json query rather than that of the custom list """ - log = cls.logger() - search = ExternalSearchIndex(_db) - if not custom_list.auto_update_query: - log.info( + cls.logger().info( f"Cannot populate entries: Custom list {custom_list.name} is missing an auto update query" ) return 0 @@ -113,7 +115,7 @@ def populate_query_pages( ## No more works if not len(works): - log.info( + cls.logger().info( f"{custom_list.name} customlist updated with {total_works_updated} works, moving on..." ) break @@ -131,7 +133,7 @@ def populate_query_pages( for work in works: custom_list.add_entry(work, update_external_index=True) - log.info( + cls.logger().info( f"Updated customlist {custom_list.name} with {total_works_updated} works" ) diff --git a/core/scripts.py b/core/scripts.py index 19e839533a..9dcb648c16 100644 --- a/core/scripts.py +++ b/core/scripts.py @@ -16,13 +16,9 @@ from sqlalchemy.orm.attributes import flag_modified from sqlalchemy.orm.exc import MultipleResultsFound, NoResultFound -from core.config import CannotLoadConfiguration, Configuration, ConfigurationConstants +from core.config import Configuration, ConfigurationConstants from core.coverage import CollectionCoverageProviderJob, CoverageProviderProgress -from core.external_search import ( - ExternalSearchIndex, - Filter, - SearchIndexCoverageProvider, -) +from core.external_search import ExternalSearchIndex, Filter from core.integration.goals import Goals from core.lane import Lane from core.metadata_layer import TimestampData @@ -58,6 +54,7 @@ from core.monitor import CollectionMonitor, ReaperMonitor from core.opds_import import OPDSImporter, OPDSImportMonitor from core.query.customlist import CustomListQueries +from core.search.coverage_provider import SearchIndexCoverageProvider from core.search.coverage_remover import RemovesSearchCoverage from core.service.container import Services, container_instance from core.util import fast_query_count @@ -2472,13 +2469,7 @@ def __init__( _db = _db or self._db super().__init__(_db) self.output = output or sys.stdout - try: - self.search = search or ExternalSearchIndex(_db) - except CannotLoadConfiguration: - self.out( - "Here's your problem: the search integration is missing or misconfigured." - ) - raise + self.search = search or self.services.search.index() def out(self, s, *args): if not s.endswith("\n"): @@ -2580,7 +2571,7 @@ class UpdateLaneSizeScript(LaneSweeperScript): def __init__(self, _db=None, *args, **kwargs): super().__init__(_db, *args, **kwargs) search = kwargs.get("search_index_client", None) - self._search: ExternalSearchIndex = search or ExternalSearchIndex(self._db) + self._search: ExternalSearchIndex = search or self.services.search.index() def should_process_lane(self, lane): """We don't want to process generic WorkLists -- there's nowhere @@ -2616,7 +2607,7 @@ class RebuildSearchIndexScript(RunWorkCoverageProviderScript, RemovesSearchCover def __init__(self, *args, **kwargs): search = kwargs.get("search_index_client", None) - self.search: ExternalSearchIndex = search or ExternalSearchIndex(self._db) + self.search: ExternalSearchIndex = search or self.services.search.index() super().__init__(SearchIndexCoverageProvider, *args, **kwargs) def do_run(self): diff --git a/core/search/coverage_provider.py b/core/search/coverage_provider.py new file mode 100644 index 0000000000..5269df0745 --- /dev/null +++ b/core/search/coverage_provider.py @@ -0,0 +1,80 @@ +from __future__ import annotations + +from core.coverage import CoverageFailure, WorkPresentationProvider +from core.model import Work, WorkCoverageRecord +from core.search.coverage_remover import RemovesSearchCoverage +from core.search.migrator import ( + SearchDocumentReceiver, + SearchDocumentReceiverType, + SearchMigrationInProgress, +) + + +class SearchIndexCoverageProvider(RemovesSearchCoverage, WorkPresentationProvider): + """Make sure all Works have up-to-date representation in the + search index. + """ + + SERVICE_NAME = "Search index coverage provider" + + DEFAULT_BATCH_SIZE = 500 + + OPERATION = WorkCoverageRecord.UPDATE_SEARCH_INDEX_OPERATION + + def __init__(self, *args, **kwargs): + search_index_client = kwargs.pop("search_index_client", None) + super().__init__(*args, **kwargs) + self.search_index_client = search_index_client or self.services.search.index() + + # + # Try to migrate to the latest schema. If the function returns None, it means + # that no migration is necessary, and we're already at the latest version. If + # we're already at the latest version, then simply upload search documents instead. + # + self.receiver = None + self.migration: None | ( + SearchMigrationInProgress + ) = self.search_index_client.start_migration() + if self.migration is None: + self.receiver: SearchDocumentReceiver = ( + self.search_index_client.start_updating_search_documents() + ) + else: + # We do have a migration, we must clear out the index and repopulate the index + self.remove_search_coverage_records() + + def on_completely_finished(self): + # Tell the search migrator that no more documents are going to show up. + target: SearchDocumentReceiverType = self.migration or self.receiver + target.finish() + + def run_once_and_update_timestamp(self): + # We do not catch exceptions here, so that the on_completely finished should not run + # if there was a runtime error + result = super().run_once_and_update_timestamp() + self.on_completely_finished() + return result + + def process_batch(self, works) -> list[Work | CoverageFailure]: + target: SearchDocumentReceiverType = self.migration or self.receiver + failures = target.add_documents( + documents=self.search_index_client.create_search_documents_from_works(works) + ) + + # Maintain a dictionary of works so that we can efficiently remove failed works later. + work_map: dict[int, Work] = {} + for work in works: + work_map[work.id] = work + + # Remove all the works that failed and create failure records for them. + results: list[Work | CoverageFailure] = [] + for failure in failures: + work = work_map[failure.id] + del work_map[failure.id] + results.append(CoverageFailure(work, repr(failure))) + + # Append all the remaining works that didn't fail. + for work in work_map.values(): + results.append(work) + + return results diff --git a/core/search/service.py b/core/search/service.py index bf751fd91b..b1a39a9d2c 100644 --- a/core/search/service.py +++ b/core/search/service.py @@ -72,6 +72,11 @@ class SearchService(ABC): sensible types, rather than the untyped pile of JSON the actual search client provides. """ + @property + @abstractmethod + def base_revision_name(self) -> str: + """The base name used for all indexes.""" + @abstractmethod def read_pointer_name(self) -> str: """Get the name used for the read pointer.""" @@ -164,7 +169,7 @@ def __init__(self, client: OpenSearch, base_revision_name: str): self._logger = logging.getLogger(SearchServiceOpensearch1.__name__) self._client = client self._search = Search(using=self._client) - self.base_revision_name = base_revision_name + self._base_revision_name = base_revision_name self._multi_search = MultiSearch(using=self._client) self._indexes_created: list[str] = [] @@ -174,6 +179,10 @@ def __init__(self, client: OpenSearch, base_revision_name: str): body={"persistent": {"action.auto_create_index": "false"}} ) + @property + def base_revision_name(self) -> str: + return self._base_revision_name + def indexes_created(self) -> list[str]: return self._indexes_created diff --git a/core/selftest.py b/core/selftest.py index 5136629b35..2bf4924c03 100644 --- a/core/selftest.py +++ b/core/selftest.py @@ -295,16 +295,7 @@ def store_self_test_results( self, _db: Session, value: dict[str, Any], results: list[SelfTestResult] ) -> None: """Store the results of a self-test in the database.""" - integration: ExternalIntegration | None - from core.external_search import ExternalSearchIndex - - if isinstance(self, ExternalSearchIndex): - integration = self.search_integration(_db) - for idx, result in enumerate(value.get("results")): # type: ignore[arg-type] - if isinstance(results[idx].result, list): - result["result"] = results[idx].result - else: - integration = self.external_integration(_db) + integration = self.external_integration(_db) if integration is not None: integration.setting(self.SELF_TEST_RESULTS_SETTING).value = json.dumps( @@ -325,14 +316,8 @@ def prior_test_results( """ constructor_method = constructor_method or cls instance = constructor_method(*args, **kwargs) - integration: ExternalIntegration | None - - from core.external_search import ExternalSearchIndex - if isinstance(instance, ExternalSearchIndex): - integration = instance.search_integration(_db) - else: - integration = instance.external_integration(_db) + integration = instance.external_integration(_db) if integration: return ( diff --git a/core/service/container.py b/core/service/container.py index 273dbdc3b9..a02f71a752 100644 --- a/core/service/container.py +++ b/core/service/container.py @@ -6,6 +6,8 @@ from core.service.analytics.container import AnalyticsContainer from core.service.logging.configuration import LoggingConfiguration from core.service.logging.container import Logging +from core.service.search.configuration import SearchConfiguration +from core.service.search.container import Search from core.service.storage.configuration import StorageConfiguration from core.service.storage.container import Storage @@ -29,28 +31,43 @@ class Services(DeclarativeContainer): storage=storage, ) - -def create_container() -> Services: - container = Services() - container.config.from_dict( - { - "storage": StorageConfiguration().dict(), - "logging": LoggingConfiguration().dict(), - "analytics": AnalyticsConfiguration().dict(), - } + search = Container( + Search, + config=config.search, ) + + +def wire_container(container: Services) -> None: container.wire( modules=[ - "core.metadata_layer", - "api.odl", "api.axis", "api.bibliotheca", - "api.enki", "api.circulation_manager", + "api.enki", + "api.odl", "api.overdrive", "core.feed.annotator.circulation", + "core.feed.acquisition", + "core.lane", + "core.metadata_layer", + "core.model.collection", + "core.model.work", + "core.query.customlist", ] ) + + +def create_container() -> Services: + container = Services() + container.config.from_dict( + { + "storage": StorageConfiguration().dict(), + "logging": LoggingConfiguration().dict(), + "analytics": AnalyticsConfiguration().dict(), + "search": SearchConfiguration().dict(), + } + ) + wire_container(container) return container diff --git a/core/service/search/configuration.py b/core/service/search/configuration.py new file mode 100644 index 0000000000..b3e2a14e78 --- /dev/null +++ b/core/service/search/configuration.py @@ -0,0 +1,13 @@ +from pydantic import AnyHttpUrl + +from core.service.configuration import ServiceConfiguration + + +class SearchConfiguration(ServiceConfiguration): + url: AnyHttpUrl + index_prefix: str = "circulation-works" + timeout: int = 20 + maxsize: int = 25 + + class Config: + env_prefix = "PALACE_SEARCH_" diff --git a/core/service/search/container.py b/core/service/search/container.py new file mode 100644 index 0000000000..6a171052a2 --- /dev/null +++ b/core/service/search/container.py @@ -0,0 +1,35 @@ +from dependency_injector import providers +from dependency_injector.containers import DeclarativeContainer +from dependency_injector.providers import Provider +from opensearchpy import OpenSearch + +from core.external_search import ExternalSearchIndex +from core.search.revision_directory import SearchRevisionDirectory +from core.search.service import SearchServiceOpensearch1 + + +class Search(DeclarativeContainer): + config = providers.Configuration() + + client: Provider[OpenSearch] = providers.Singleton( + OpenSearch, + hosts=config.url, + timeout=config.timeout, + maxsize=config.maxsize, + ) + + service: Provider[SearchServiceOpensearch1] = providers.Singleton( + SearchServiceOpensearch1, + client=client, + base_revision_name=config.index_prefix, + ) + + revision_directory: Provider[SearchRevisionDirectory] = providers.Singleton( + SearchRevisionDirectory.create, + ) + + index: Provider[ExternalSearchIndex] = providers.Singleton( + ExternalSearchIndex, + service=service, + revision_directory=revision_directory, + ) diff --git a/docker-compose.yml b/docker-compose.yml index 2588473739..f8921f3d47 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,35 +1,51 @@ version: "3.9" -# Common set of CM environment variables +# Common CM setup # see: https://github.com/compose-spec/compose-spec/blob/master/spec.md#extension -x-cm-env-variables: &cm-env-variables - SIMPLIFIED_PRODUCTION_DATABASE: "postgresql://palace:test@pg:5432/circ" - PALACE_STORAGE_ACCESS_KEY: "palace" - PALACE_STORAGE_SECRET_KEY: "test123456789" - PALACE_STORAGE_ENDPOINT_URL: "http://minio:9000" - PALACE_STORAGE_PUBLIC_ACCESS_BUCKET: "public" - PALACE_STORAGE_ANALYTICS_BUCKET: "analytics" - PALACE_STORAGE_URL_TEMPLATE: "http://localhost:9000/{bucket}/{key}" - PALACE_REPORTING_NAME: "TEST CM" +x-cm-variables: &cm + platform: "${BUILD_PLATFORM-}" + environment: + SIMPLIFIED_PRODUCTION_DATABASE: "postgresql://palace:test@pg:5432/circ" + PALACE_SEARCH_URL: "http://os:9200" + PALACE_STORAGE_ACCESS_KEY: "palace" + PALACE_STORAGE_SECRET_KEY: "test123456789" + PALACE_STORAGE_ENDPOINT_URL: "http://minio:9000" + PALACE_STORAGE_PUBLIC_ACCESS_BUCKET: "public" + PALACE_STORAGE_ANALYTICS_BUCKET: "analytics" + PALACE_STORAGE_URL_TEMPLATE: "http://localhost:9000/{bucket}/{key}" + PALACE_REPORTING_NAME: "TEST CM" + depends_on: + pg: + condition: service_healthy + minio: + condition: service_healthy + os: + condition: service_healthy + +x-cm-build: &cm-build + context: . + dockerfile: docker/Dockerfile + args: + - BASE_IMAGE=${BUILD_BASE_IMAGE-ghcr.io/thepalaceproject/circ-baseimage:latest} + cache_from: + - ${BUILD_CACHE_FROM-ghcr.io/thepalaceproject/circ-webapp:main} services: # example docker compose configuration for testing and development webapp: + <<: *cm build: - context: . - dockerfile: docker/Dockerfile + <<: *cm-build target: webapp ports: - "6500:80" - environment: *cm-env-variables scripts: + <<: *cm build: - context: . - dockerfile: docker/Dockerfile + <<: *cm-build target: scripts - environment: *cm-env-variables pg: image: "postgres:12" @@ -37,6 +53,11 @@ services: POSTGRES_USER: palace POSTGRES_PASSWORD: test POSTGRES_DB: circ + healthcheck: + test: ["CMD-SHELL", "pg_isready -U palace -d circ"] + interval: 30s + timeout: 30s + retries: 3 minio: image: "bitnami/minio:2023.2.27" @@ -48,11 +69,22 @@ services: MINIO_ROOT_PASSWORD: "test123456789" MINIO_SCHEME: "http" MINIO_DEFAULT_BUCKETS: "public:download,analytics" + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:9000/minio/health/live"] + interval: 30s + timeout: 20s + retries: 3 os: build: dockerfile: docker/Dockerfile.ci target: opensearch + context: . environment: - discovery.type: single-node - DISABLE_SECURITY_PLUGIN: true + discovery.type: "single-node" + DISABLE_SECURITY_PLUGIN: "true" + healthcheck: + test: curl --silent http://localhost:9200 >/dev/null; if [[ $$? == 52 ]]; then echo 0; else echo 1; fi + interval: 30s + timeout: 10s + retries: 5 diff --git a/docker/ci/check_service_status.sh b/docker/ci/check_service_status.sh index a76ff6d02d..9adbe35f81 100644 --- a/docker/ci/check_service_status.sh +++ b/docker/ci/check_service_status.sh @@ -4,7 +4,7 @@ function wait_for_runit() # The container to run the command in container="$1" - timeout 120s grep -q 'Runit started' <(docker logs "$container" -f 2>&1) + timeout 120s grep -q 'Runit started' <(docker compose logs "$container" -f 2>&1) } # A method to check that runit services are running inside the container @@ -17,7 +17,7 @@ function check_service_status() service="$2" # Check the status of the service. - service_status=$(docker exec "$container" /bin/bash -c "sv check $service") + service_status=$(docker compose exec "$container" /bin/bash -c "sv check $service") # Get the exit code for the sv call. sv_status=$? @@ -34,7 +34,7 @@ function check_crontab() { container="$1" # Installing the crontab will reveal any errors and exit with an error code - $(docker exec "$container" /bin/bash -c "crontab /etc/cron.d/circulation") + $(docker compose exec "$container" /bin/bash -c "crontab /etc/cron.d/circulation") validate_status=$? if [[ "$validate_status" != 0 ]]; then echo " FAIL: crontab is incorrect" @@ -48,7 +48,7 @@ function run_script() { container="$1" script="$2" - output=$(docker exec "$container" /bin/bash -c "$script") + output=$(docker compose exec "$container" /bin/bash -c "$script") script_status=$? if [[ "$script_status" != 0 ]]; then echo " FAIL: script run failed" diff --git a/docker/ci/test_migrations.sh b/docker/ci/test_migrations.sh index 2ebe133e19..e6523c04d6 100755 --- a/docker/ci/test_migrations.sh +++ b/docker/ci/test_migrations.sh @@ -26,7 +26,7 @@ compose_cmd() { run_in_container() { - compose_cmd run --build --rm webapp /bin/bash -c "source env/bin/activate && $*" + compose_cmd run --build --rm --no-deps webapp /bin/bash -c "source env/bin/activate && $*" } cleanup() { @@ -105,10 +105,12 @@ if [[ -z $first_migration_commit ]]; then exit 1 fi -echo "Starting containers and initializing database at commit ${first_migration_commit}" -git checkout -q "${first_migration_commit}" +echo "Starting containers" compose_cmd down compose_cmd up -d pg + +echo "Initializing database at commit ${first_migration_commit}" +git checkout -q "${first_migration_commit}" run_in_container "./bin/util/initialize_instance" initialize_exit_code=$? if [[ $initialize_exit_code -ne 0 ]]; then diff --git a/docker/ci/test_scripts.sh b/docker/ci/test_scripts.sh index d283e87093..55dc74de15 100755 --- a/docker/ci/test_scripts.sh +++ b/docker/ci/test_scripts.sh @@ -12,6 +12,9 @@ source "${dir}/check_service_status.sh" # Wait for container to start wait_for_runit "$container" +# Make sure database initialization completed successfully +timeout 240s grep -q 'Initialization complete' <(docker compose logs "$container" -f 2>&1) + # Make sure that cron is running in the scripts container check_service_status "$container" /etc/service/cron diff --git a/docker/ci/test_webapp.sh b/docker/ci/test_webapp.sh index aa5680b1ce..51241f9198 100755 --- a/docker/ci/test_webapp.sh +++ b/docker/ci/test_webapp.sh @@ -17,10 +17,10 @@ check_service_status "$container" /etc/service/nginx check_service_status "$container" /etc/service/uwsgi # Wait for UWSGI to be ready to accept connections. -timeout 240s grep -q 'WSGI app .* ready in [0-9]* seconds' <(docker logs "$container" -f 2>&1) +timeout 240s grep -q 'WSGI app .* ready in [0-9]* seconds' <(docker compose logs "$container" -f 2>&1) # Make sure the web server is running. -healthcheck=$(docker exec "$container" curl --write-out "%{http_code}" --silent --output /dev/null http://localhost/healthcheck.html) +healthcheck=$(docker compose exec "$container" curl --write-out "%{http_code}" --silent --output /dev/null http://localhost/healthcheck.html) if ! [[ ${healthcheck} == "200" ]]; then exit 1 else @@ -28,7 +28,7 @@ else fi # Also make sure the app server is running. -feed_type=$(docker exec "$container" curl --write-out "%{content_type}" --silent --output /dev/null http://localhost/version.json) +feed_type=$(docker compose exec "$container" curl --write-out "%{content_type}" --silent --output /dev/null http://localhost/version.json) if ! [[ ${feed_type} == "application/json" ]]; then exit 1 else diff --git a/scripts.py b/scripts.py index 1a99db4b73..25515015ce 100644 --- a/scripts.py +++ b/scripts.py @@ -31,7 +31,6 @@ OPDSForDistributorsReaperMonitor, ) from api.overdrive import OverdriveAPI -from core.external_search import ExternalSearchIndex from core.integration.goals import Goals from core.lane import Lane from core.marc import Annotator as MarcAnnotator @@ -96,7 +95,7 @@ def q(self): def run(self): q = self.q() - search_index_client = ExternalSearchIndex(self._db) + search_index_client = self.services.search.index() self.log.info("Attempting to repair metadata for %d works" % q.count()) success = 0 @@ -528,26 +527,12 @@ def initialize_database(self, connection: Connection) -> None: # Create a secret key if one doesn't already exist. ConfigurationSetting.sitewide_secret(session, Configuration.SECRET_KEY) - # Initialize the search client to create the "-current" alias. - try: - ExternalSearchIndex(session) - except CannotLoadConfiguration: - # Opensearch isn't configured, so do nothing. - pass - # Stamp the most recent migration as the current state of the DB alembic_conf = self._get_alembic_config(connection) command.stamp(alembic_conf, "head") - def initialize_search_indexes(self, _db: Session) -> bool: - try: - search = ExternalSearchIndex(_db) - except CannotLoadConfiguration as ex: - self.log.error( - "No search integration found yet, cannot initialize search indices." - ) - self.log.error(f"Error: {ex}") - return False + def initialize_search_indexes(self) -> bool: + search = self._container.search.index() return search.initialize_indices() def initialize(self, connection: Connection): @@ -569,8 +554,7 @@ def initialize(self, connection: Connection): self.initialize_database(connection) self.log.info("Initialization complete.") - with Session(connection) as session: - self.initialize_search_indexes(session) + self.initialize_search_indexes() def run(self) -> None: """ diff --git a/tests/api/admin/controller/test_custom_lists.py b/tests/api/admin/controller/test_custom_lists.py index eff19c8ced..724e431e66 100644 --- a/tests/api/admin/controller/test_custom_lists.py +++ b/tests/api/admin/controller/test_custom_lists.py @@ -449,7 +449,10 @@ def test_custom_list_get(self, admin_librarian_fixture: AdminLibrarianFixture): list.add_entry(work1) list.add_entry(work2) - with admin_librarian_fixture.request_context_with_library_and_admin("/"): + with ( + admin_librarian_fixture.request_context_with_library_and_admin("/"), + admin_librarian_fixture.ctrl.wired_container(), + ): assert isinstance(list.id, int) response = admin_librarian_fixture.manager.admin_custom_lists_controller.custom_list( list.id diff --git a/tests/api/admin/controller/test_feed.py b/tests/api/admin/controller/test_feed.py index 42f3d3f8b5..29415667fc 100644 --- a/tests/api/admin/controller/test_feed.py +++ b/tests/api/admin/controller/test_feed.py @@ -15,7 +15,10 @@ def test_suppressed(self, admin_librarian_fixture): unsuppressed_work = admin_librarian_fixture.ctrl.db.work() - with admin_librarian_fixture.request_context_with_library_and_admin("/"): + with ( + admin_librarian_fixture.request_context_with_library_and_admin("/"), + admin_librarian_fixture.ctrl.wired_container(), + ): response = ( admin_librarian_fixture.manager.admin_feed_controller.suppressed() ) diff --git a/tests/api/admin/controller/test_search_service_self_tests.py b/tests/api/admin/controller/test_search_service_self_tests.py deleted file mode 100644 index f32b615759..0000000000 --- a/tests/api/admin/controller/test_search_service_self_tests.py +++ /dev/null @@ -1,93 +0,0 @@ -from api.admin.problem_details import * -from core.model import ExternalIntegration, create -from core.selftest import HasSelfTests - - -class TestSearchServiceSelfTests: - def test_search_service_self_tests_with_no_identifier(self, settings_ctrl_fixture): - with settings_ctrl_fixture.request_context_with_admin("/"): - response = settings_ctrl_fixture.manager.admin_search_service_self_tests_controller.process_search_service_self_tests( - None - ) - assert response.title == MISSING_IDENTIFIER.title - assert response.detail == MISSING_IDENTIFIER.detail - assert response.status_code == 400 - - def test_search_service_self_tests_with_no_search_service_found( - self, settings_ctrl_fixture - ): - with settings_ctrl_fixture.request_context_with_admin("/"): - response = settings_ctrl_fixture.manager.admin_search_service_self_tests_controller.process_search_service_self_tests( - -1 - ) - assert response == MISSING_SERVICE - assert response.status_code == 404 - - def test_search_service_self_tests_test_get(self, settings_ctrl_fixture): - old_prior_test_results = HasSelfTests.prior_test_results - HasSelfTests.prior_test_results = settings_ctrl_fixture.mock_prior_test_results - search_service, ignore = create( - settings_ctrl_fixture.ctrl.db.session, - ExternalIntegration, - protocol=ExternalIntegration.OPENSEARCH, - goal=ExternalIntegration.SEARCH_GOAL, - ) - # Make sure that HasSelfTest.prior_test_results() was called and that - # it is in the response's self tests object. - with settings_ctrl_fixture.request_context_with_admin("/"): - response = settings_ctrl_fixture.manager.admin_search_service_self_tests_controller.process_search_service_self_tests( - search_service.id - ) - response_search_service = response.get("self_test_results") - - assert response_search_service.get("id") == search_service.id - assert response_search_service.get("name") == search_service.name - assert ( - response_search_service.get("protocol").get("label") - == search_service.protocol - ) - assert response_search_service.get("goal") == search_service.goal - assert ( - response_search_service.get("self_test_results") - == HasSelfTests.prior_test_results() - ) - - HasSelfTests.prior_test_results = old_prior_test_results - - def test_search_service_self_tests_post(self, settings_ctrl_fixture): - old_run_self_tests = HasSelfTests.run_self_tests - HasSelfTests.run_self_tests = settings_ctrl_fixture.mock_run_self_tests - - search_service, ignore = create( - settings_ctrl_fixture.ctrl.db.session, - ExternalIntegration, - protocol=ExternalIntegration.OPENSEARCH, - goal=ExternalIntegration.SEARCH_GOAL, - ) - m = ( - settings_ctrl_fixture.manager.admin_search_service_self_tests_controller.self_tests_process_post - ) - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - response = m(search_service.id) - assert response._status == "200 OK" - assert "Successfully ran new self tests" == response.get_data(as_text=True) - - positional, keyword = settings_ctrl_fixture.run_self_tests_called_with - # run_self_tests was called with positional arguments: - # * The database connection - # * The method to call to instantiate a HasSelfTests implementation - # (None -- this means to use the default ExternalSearchIndex - # constructor.) - # * The database connection again (to be passed into - # the ExternalSearchIndex constructor). - assert ( - settings_ctrl_fixture.ctrl.db.session, - None, - settings_ctrl_fixture.ctrl.db.session, - ) == positional - - # run_self_tests was not called with any keyword arguments. - assert {} == keyword - - # Undo the mock. - HasSelfTests.run_self_tests = old_run_self_tests diff --git a/tests/api/admin/controller/test_search_services.py b/tests/api/admin/controller/test_search_services.py deleted file mode 100644 index ebd470ea0a..0000000000 --- a/tests/api/admin/controller/test_search_services.py +++ /dev/null @@ -1,301 +0,0 @@ -import flask -import pytest -from werkzeug.datastructures import MultiDict - -from api.admin.exceptions import AdminNotAuthorized -from api.admin.problem_details import ( - INCOMPLETE_CONFIGURATION, - INTEGRATION_NAME_ALREADY_IN_USE, - MISSING_SERVICE, - MULTIPLE_SITEWIDE_SERVICES, - NO_PROTOCOL_FOR_NEW_SERVICE, - UNKNOWN_PROTOCOL, -) -from core.external_search import ExternalSearchIndex -from core.model import AdminRole, ExternalIntegration, create, get_one - - -class TestSearchServices: - def test_search_services_get_with_no_services(self, settings_ctrl_fixture): - # Delete the search integration - session = settings_ctrl_fixture.ctrl.db.session - integration = ExternalIntegration.lookup( - session, ExternalIntegration.OPENSEARCH, ExternalIntegration.SEARCH_GOAL - ) - session.delete(integration) - - with settings_ctrl_fixture.request_context_with_admin("/"): - response = ( - settings_ctrl_fixture.manager.admin_search_services_controller.process_services() - ) - assert response.get("search_services") == [] - protocols = response.get("protocols") - assert ExternalIntegration.OPENSEARCH in [p.get("name") for p in protocols] - assert "settings" in protocols[0] - - settings_ctrl_fixture.admin.remove_role(AdminRole.SYSTEM_ADMIN) - settings_ctrl_fixture.ctrl.db.session.flush() - pytest.raises( - AdminNotAuthorized, - settings_ctrl_fixture.manager.admin_search_services_controller.process_services, - ) - - def test_search_services_get_with_one_service(self, settings_ctrl_fixture): - # Delete the pre-existing integration - session = settings_ctrl_fixture.ctrl.db.session - integration = ExternalIntegration.lookup( - session, ExternalIntegration.OPENSEARCH, ExternalIntegration.SEARCH_GOAL - ) - session.delete(integration) - - search_service, ignore = create( - session, - ExternalIntegration, - protocol=ExternalIntegration.OPENSEARCH, - goal=ExternalIntegration.SEARCH_GOAL, - ) - search_service.url = "search url" - search_service.setting( - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY - ).value = "works-index-prefix" - search_service.setting( - ExternalSearchIndex.TEST_SEARCH_TERM_KEY - ).value = "search-term-for-self-tests" - - with settings_ctrl_fixture.request_context_with_admin("/"): - response = ( - settings_ctrl_fixture.manager.admin_search_services_controller.process_services() - ) - [service] = response.get("search_services") - - assert search_service.id == service.get("id") - assert search_service.protocol == service.get("protocol") - assert "search url" == service.get("settings").get(ExternalIntegration.URL) - assert "works-index-prefix" == service.get("settings").get( - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY - ) - assert "search-term-for-self-tests" == service.get("settings").get( - ExternalSearchIndex.TEST_SEARCH_TERM_KEY - ) - - def test_search_services_post_errors(self, settings_ctrl_fixture): - controller = settings_ctrl_fixture.manager.admin_search_services_controller - - # Delete the previous integrations - session = settings_ctrl_fixture.ctrl.db.session - integration = ExternalIntegration.lookup( - session, ExternalIntegration.OPENSEARCH, ExternalIntegration.SEARCH_GOAL - ) - session.delete(integration) - - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", "Name"), - ("protocol", "Unknown"), - ] - ) - response = controller.process_services() - assert response == UNKNOWN_PROTOCOL - - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict([("name", "Name")]) - response = controller.process_services() - assert response == NO_PROTOCOL_FOR_NEW_SERVICE - - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", "Name"), - ("id", "123"), - ] - ) - response = controller.process_services() - assert response == MISSING_SERVICE - - service, ignore = create( - session, - ExternalIntegration, - protocol=ExternalIntegration.OPENSEARCH, - goal=ExternalIntegration.SEARCH_GOAL, - ) - - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", "Name"), - ("protocol", ExternalIntegration.OPENSEARCH), - ] - ) - response = controller.process_services() - assert response.uri == MULTIPLE_SITEWIDE_SERVICES.uri - - session.delete(service) - service, ignore = create( - session, - ExternalIntegration, - protocol="test", - goal="test", - name="name", - ) - - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", service.name), - ("protocol", ExternalIntegration.OPENSEARCH), - ] - ) - response = controller.process_services() - assert response == INTEGRATION_NAME_ALREADY_IN_USE - - service, ignore = create( - session, - ExternalIntegration, - protocol=ExternalIntegration.OPENSEARCH, - goal=ExternalIntegration.SEARCH_GOAL, - ) - - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", "Name"), - ("id", service.id), - ("protocol", ExternalIntegration.OPENSEARCH), - ] - ) - response = controller.process_services() - assert response.uri == INCOMPLETE_CONFIGURATION.uri - - settings_ctrl_fixture.admin.remove_role(AdminRole.SYSTEM_ADMIN) - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("protocol", ExternalIntegration.OPENSEARCH), - (ExternalIntegration.URL, "search url"), - (ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY, "works-index-prefix"), - ] - ) - pytest.raises(AdminNotAuthorized, controller.process_services) - - def test_search_services_post_create(self, settings_ctrl_fixture): - # Delete the previous integrations - session = settings_ctrl_fixture.ctrl.db.session - integration = ExternalIntegration.lookup( - session, ExternalIntegration.OPENSEARCH, ExternalIntegration.SEARCH_GOAL - ) - session.delete(integration) - - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", "Name"), - ("protocol", ExternalIntegration.OPENSEARCH), - (ExternalIntegration.URL, "http://search_url"), - (ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY, "works-index-prefix"), - (ExternalSearchIndex.TEST_SEARCH_TERM_KEY, "sample-search-term"), - ] - ) - response = ( - settings_ctrl_fixture.manager.admin_search_services_controller.process_services() - ) - assert response.status_code == 201 - - service = get_one( - session, - ExternalIntegration, - goal=ExternalIntegration.SEARCH_GOAL, - ) - assert service.id == int(response.response[0]) - assert ExternalIntegration.OPENSEARCH == service.protocol - assert "http://search_url" == service.url - assert ( - "works-index-prefix" - == service.setting(ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY).value - ) - assert ( - "sample-search-term" - == service.setting(ExternalSearchIndex.TEST_SEARCH_TERM_KEY).value - ) - - def test_search_services_post_edit(self, settings_ctrl_fixture): - search_service, ignore = create( - settings_ctrl_fixture.ctrl.db.session, - ExternalIntegration, - protocol=ExternalIntegration.OPENSEARCH, - goal=ExternalIntegration.SEARCH_GOAL, - ) - search_service.url = "search url" - search_service.setting( - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY - ).value = "works-index-prefix" - search_service.setting( - ExternalSearchIndex.TEST_SEARCH_TERM_KEY - ).value = "sample-search-term" - - with settings_ctrl_fixture.request_context_with_admin("/", method="POST"): - flask.request.form = MultiDict( - [ - ("name", "Name"), - ("id", search_service.id), - ("protocol", ExternalIntegration.OPENSEARCH), - (ExternalIntegration.URL, "http://new_search_url"), - ( - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY, - "new-works-index-prefix", - ), - ( - ExternalSearchIndex.TEST_SEARCH_TERM_KEY, - "new-sample-search-term", - ), - ] - ) - response = ( - settings_ctrl_fixture.manager.admin_search_services_controller.process_services() - ) - assert response.status_code == 200 - - assert search_service.id == int(response.response[0]) - assert ExternalIntegration.OPENSEARCH == search_service.protocol - assert "http://new_search_url" == search_service.url - assert ( - "new-works-index-prefix" - == search_service.setting(ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY).value - ) - assert ( - "new-sample-search-term" - == search_service.setting(ExternalSearchIndex.TEST_SEARCH_TERM_KEY).value - ) - - def test_search_service_delete(self, settings_ctrl_fixture): - search_service, ignore = create( - settings_ctrl_fixture.ctrl.db.session, - ExternalIntegration, - protocol=ExternalIntegration.OPENSEARCH, - goal=ExternalIntegration.SEARCH_GOAL, - ) - search_service.url = "search url" - search_service.setting( - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY - ).value = "works-index-prefix" - - with settings_ctrl_fixture.request_context_with_admin("/", method="DELETE"): - settings_ctrl_fixture.admin.remove_role(AdminRole.SYSTEM_ADMIN) - pytest.raises( - AdminNotAuthorized, - settings_ctrl_fixture.manager.admin_search_services_controller.process_delete, - search_service.id, - ) - - settings_ctrl_fixture.admin.add_role(AdminRole.SYSTEM_ADMIN) - response = settings_ctrl_fixture.manager.admin_search_services_controller.process_delete( - search_service.id - ) - assert response.status_code == 200 - - service = get_one( - settings_ctrl_fixture.ctrl.db.session, - ExternalIntegration, - id=search_service.id, - ) - assert None == service diff --git a/tests/api/admin/controller/test_sitewide_services.py b/tests/api/admin/controller/test_sitewide_services.py deleted file mode 100644 index dfa3f8432f..0000000000 --- a/tests/api/admin/controller/test_sitewide_services.py +++ /dev/null @@ -1,34 +0,0 @@ -from api.admin.controller.sitewide_services import * -from core.model import ExternalIntegration - - -class TestSitewideServices: - def test_sitewide_service_management(self, settings_ctrl_fixture): - # The configuration of search and logging collections is delegated to - # the _manage_sitewide_service and _delete_integration methods. - # - # Search collections are more comprehensively tested in test_search_services. - - EI = ExternalIntegration - - class MockSearch(SearchServicesController): - def _manage_sitewide_service(self, *args): - self.manage_called_with = args - - def _delete_integration(self, *args): - self.delete_called_with = args - - controller = MockSearch(settings_ctrl_fixture.manager) - - with settings_ctrl_fixture.request_context_with_admin("/"): - controller.process_services() - goal, apis, key_name, problem = controller.manage_called_with - assert EI.SEARCH_GOAL == goal - assert ExternalSearchIndex in apis - assert "search_services" == key_name - assert "new search service" in problem - - with settings_ctrl_fixture.request_context_with_admin("/"): - id = object() - controller.process_delete(id) - assert (id, EI.SEARCH_GOAL) == controller.delete_called_with diff --git a/tests/api/admin/controller/test_work_editor.py b/tests/api/admin/controller/test_work_editor.py index fbe7b8c626..4c6605a36a 100644 --- a/tests/api/admin/controller/test_work_editor.py +++ b/tests/api/admin/controller/test_work_editor.py @@ -1,4 +1,5 @@ import json +from collections.abc import Generator import feedparser import flask @@ -64,8 +65,12 @@ def __init__(self, controller_fixture: ControllerFixture): @pytest.fixture(scope="function") -def work_fixture(controller_fixture: ControllerFixture) -> WorkFixture: - return WorkFixture(controller_fixture) +def work_fixture( + controller_fixture: ControllerFixture, +) -> Generator[WorkFixture, None, None]: + fixture = WorkFixture(controller_fixture) + with fixture.ctrl.wired_container(): + yield fixture class TestWorkController: diff --git a/tests/api/admin/test_routes.py b/tests/api/admin/test_routes.py index 4ce6f8ac39..6ffeb047d4 100644 --- a/tests/api/admin/test_routes.py +++ b/tests/api/admin/test_routes.py @@ -2,6 +2,7 @@ from collections.abc import Generator from pathlib import Path from typing import Any +from unittest.mock import MagicMock import flask import pytest @@ -80,7 +81,7 @@ def __init__( self.controller_fixture = controller_fixture self.setup_circulation_manager = False if not self.REAL_CIRCULATION_MANAGER: - circ_manager = MockCirculationManager(self.db.session) + circ_manager = MockCirculationManager(self.db.session, MagicMock()) setup_admin_controllers(circ_manager) self.REAL_CIRCULATION_MANAGER = circ_manager @@ -620,45 +621,6 @@ def test_process_delete(self, fixture: AdminRouteFixture): fixture.assert_supported_methods(url, "DELETE") -class TestAdminSearchServices: - CONTROLLER_NAME = "admin_search_services_controller" - - @pytest.fixture(scope="function") - def fixture(self, admin_route_fixture: AdminRouteFixture) -> AdminRouteFixture: - admin_route_fixture.set_controller_name(self.CONTROLLER_NAME) - return admin_route_fixture - - def test_process_services(self, fixture: AdminRouteFixture): - url = "/admin/search_services" - fixture.assert_authenticated_request_calls( - url, fixture.controller.process_services # type: ignore - ) - fixture.assert_supported_methods(url, "GET", "POST") - - def test_process_delete(self, fixture: AdminRouteFixture): - url = "/admin/search_service/" - fixture.assert_authenticated_request_calls( - url, fixture.controller.process_delete, "", http_method="DELETE" # type: ignore - ) - fixture.assert_supported_methods(url, "DELETE") - - -class TestAdminSearchServicesSelfTests: - CONTROLLER_NAME = "admin_search_service_self_tests_controller" - - @pytest.fixture(scope="function") - def fixture(self, admin_route_fixture: AdminRouteFixture) -> AdminRouteFixture: - admin_route_fixture.set_controller_name(self.CONTROLLER_NAME) - return admin_route_fixture - - def test_process_search_service_self_tests(self, fixture: AdminRouteFixture): - url = "/admin/search_service_self_tests/" - fixture.assert_authenticated_request_calls( - url, fixture.controller.process_search_service_self_tests, "" # type: ignore - ) - fixture.assert_supported_methods(url, "GET", "POST") - - class TestAdminCatalogServices: CONTROLLER_NAME = "admin_catalog_services_controller" diff --git a/tests/api/conftest.py b/tests/api/conftest.py index d24165c8e1..10c6427c09 100644 --- a/tests/api/conftest.py +++ b/tests/api/conftest.py @@ -25,7 +25,6 @@ "tests.fixtures.api_overdrive_files", "tests.fixtures.api_routes", "tests.fixtures.authenticator", - "tests.fixtures.container", "tests.fixtures.csv_files", "tests.fixtures.database", "tests.fixtures.files", @@ -36,6 +35,7 @@ "tests.fixtures.opds_files", "tests.fixtures.sample_covers", "tests.fixtures.search", + "tests.fixtures.services", "tests.fixtures.time", "tests.fixtures.tls_server", "tests.fixtures.vendor_id", diff --git a/tests/api/controller/test_crawlfeed.py b/tests/api/controller/test_crawlfeed.py index a868ac55ef..2345196cad 100644 --- a/tests/api/controller/test_crawlfeed.py +++ b/tests/api/controller/test_crawlfeed.py @@ -235,11 +235,6 @@ def works(self, _db, facets, pagination, *args, **kwargs): assert INVALID_INPUT.uri == response.uri assert None == self.page_called_with - # Bad search engine -> problem detail - circulation_fixture.assert_bad_search_index_gives_problem_detail( - lambda: circulation_fixture.manager.opds_feeds._crawlable_feed(**in_kwargs) - ) - # Good pagination data -> feed_class.page() is called. sort_key = ["sort", "pagination", "key"] with circulation_fixture.request_context_with_library( @@ -297,7 +292,10 @@ def works(self, _db, facets, pagination, *args, **kwargs): # Finally, remove the mock feed class and verify that a real OPDS # feed is generated from the result of MockLane.works() del in_kwargs["feed_class"] - with circulation_fixture.request_context_with_library("/"): + with ( + circulation_fixture.request_context_with_library("/"), + circulation_fixture.wired_container(), + ): response = circulation_fixture.manager.opds_feeds._crawlable_feed( **in_kwargs ) diff --git a/tests/api/controller/test_loan.py b/tests/api/controller/test_loan.py index 110cab527f..e713953268 100644 --- a/tests/api/controller/test_loan.py +++ b/tests/api/controller/test_loan.py @@ -1,5 +1,6 @@ import datetime import urllib.parse +from collections.abc import Generator from decimal import Decimal from unittest.mock import MagicMock, patch @@ -84,8 +85,10 @@ def __init__(self, db: DatabaseTransactionFixture): @pytest.fixture(scope="function") -def loan_fixture(db: DatabaseTransactionFixture): - return LoanFixture(db) +def loan_fixture(db: DatabaseTransactionFixture) -> Generator[LoanFixture, None, None]: + fixture = LoanFixture(db) + with fixture.wired_container(): + yield fixture class TestLoanController: diff --git a/tests/api/controller/test_opds_feed.py b/tests/api/controller/test_opds_feed.py index 4ed4885b89..499f3554c8 100644 --- a/tests/api/controller/test_opds_feed.py +++ b/tests/api/controller/test_opds_feed.py @@ -6,9 +6,7 @@ import feedparser from flask import url_for -from api.circulation_manager import CirculationManager from api.lanes import HasSeriesFacets, JackpotFacets, JackpotWorkList -from api.problem_details import REMOTE_INTEGRATION_FAILED from core.app_server import load_facets_from_request from core.entrypoint import AudiobooksEntryPoint, EverythingEntryPoint from core.external_search import SortKeyPagination @@ -78,11 +76,6 @@ def test_feed( == response.uri ) - # Bad search index setup -> Problem detail - circulation_fixture.assert_bad_search_index_gives_problem_detail( - lambda: circulation_fixture.manager.opds_feeds.feed(lane_id) - ) - # Now let's make a real feed. # Set up configuration settings for links and entry points @@ -94,8 +87,11 @@ def test_feed( settings.about = "d" # type: ignore[assignment] # Make a real OPDS feed and poke at it. - with circulation_fixture.request_context_with_library( - "/?entrypoint=Book&size=10" + with ( + circulation_fixture.request_context_with_library( + "/?entrypoint=Book&size=10" + ), + circulation_fixture.wired_container(), ): response = circulation_fixture.manager.opds_feeds.feed( circulation_fixture.english_adult_fiction.id @@ -276,11 +272,6 @@ def test_groups( == response.uri ) - # Bad search index setup -> Problem detail - circulation_fixture.assert_bad_search_index_gives_problem_detail( - lambda: circulation_fixture.manager.opds_feeds.groups(None) - ) - # A grouped feed has no pagination, and the FeaturedFacets # constructor never raises an exception. So we don't need to # test for those error conditions. @@ -491,11 +482,6 @@ def test_search( == response.uri ) - # Bad search index setup -> Problem detail - circulation_fixture.assert_bad_search_index_gives_problem_detail( - lambda: circulation_fixture.manager.opds_feeds.search(None) - ) - # Loading the SearchFacets object from a request can't return # a problem detail, so we can't test that case. @@ -666,28 +652,6 @@ def test_lane_search_params( == response.detail ) - def test_misconfigured_search( - self, circulation_fixture: CirculationControllerFixture - ): - circulation_fixture.add_works(self._EXTRA_BOOKS) - - class BadSearch(CirculationManager): - @property - def setup_search(self): - raise Exception("doomed!") - - circulation = BadSearch(circulation_fixture.db.session) - - # An attempt to call FeedController.search() will return a - # problem detail. - with circulation_fixture.request_context_with_library("/?q=t"): - problem = circulation.opds_feeds.search(None) - assert REMOTE_INTEGRATION_FAILED.uri == problem.uri - assert ( - "The search index for this site is not properly configured." - == problem.detail - ) - def test__qa_feed(self, circulation_fixture: CirculationControllerFixture): circulation_fixture.add_works(self._EXTRA_BOOKS) @@ -702,11 +666,6 @@ def test__qa_feed(self, circulation_fixture: CirculationControllerFixture): m = circulation_fixture.manager.opds_feeds._qa_feed args = (feed_method, "QA test feed", "qa_feed", Facets, worklist_factory) - # Bad search index setup -> Problem detail - circulation_fixture.assert_bad_search_index_gives_problem_detail( - lambda: m(*args) - ) - # Bad faceting information -> Problem detail with circulation_fixture.request_context_with_library("/?order=nosuchorder"): response = m(*args) diff --git a/tests/api/controller/test_work.py b/tests/api/controller/test_work.py index 70796b1775..68ff6ac68a 100644 --- a/tests/api/controller/test_work.py +++ b/tests/api/controller/test_work.py @@ -1,6 +1,7 @@ import datetime import json import urllib.parse +from collections.abc import Generator from typing import Any from unittest.mock import MagicMock @@ -22,7 +23,7 @@ from api.problem_details import NO_SUCH_LANE, NOT_FOUND_ON_REMOTE from core.classifier import Classifier from core.entrypoint import AudiobooksEntryPoint -from core.external_search import SortKeyPagination, mock_search_index +from core.external_search import SortKeyPagination from core.feed.acquisition import OPDSAcquisitionFeed from core.feed.annotator.circulation import LibraryAnnotator from core.feed.types import WorkEntry @@ -64,8 +65,10 @@ def __init__(self, db: DatabaseTransactionFixture): @pytest.fixture(scope="function") -def work_fixture(db: DatabaseTransactionFixture): - return WorkFixture(db) +def work_fixture(db: DatabaseTransactionFixture) -> Generator[WorkFixture, None, None]: + fixture = WorkFixture(db) + with fixture.wired_container(): + yield fixture class TestWorkController: @@ -100,11 +103,6 @@ def test_contributor(self, work_fixture: WorkFixture): contributor = contributor.display_name - # Search index misconfiguration -> Problem detail - work_fixture.assert_bad_search_index_gives_problem_detail( - lambda: work_fixture.manager.work_controller.series(contributor, None, None) - ) - # Bad facet data -> ProblemDetail with work_fixture.request_context_with_library("/?order=nosuchorder"): response = m(contributor, None, None) @@ -187,7 +185,7 @@ def page(cls, **kwargs): kwargs = self.called_with # type: ignore assert work_fixture.db.session == kwargs.pop("_db") - assert work_fixture.manager._external_search == kwargs.pop("search_engine") + assert work_fixture.manager.external_search == kwargs.pop("search_engine") # The feed is named after the contributor the request asked # about. @@ -517,13 +515,6 @@ def test_recommendations(self, work_fixture: WorkFixture): ) assert 400 == response.status_code - # Or if the search index is misconfigured. - work_fixture.assert_bad_search_index_gives_problem_detail( - lambda: work_fixture.manager.work_controller.recommendations( - *args, **kwargs - ) - ) - # If no NoveList API is configured, the lane does not exist. with work_fixture.request_context_with_library("/"): response = work_fixture.manager.work_controller.recommendations(*args) @@ -666,13 +657,6 @@ def test_related_books(self, work_fixture: WorkFixture): # creation process -- an invalid entrypoint will simply be # ignored. - # Bad search index setup -> Problem detail - work_fixture.assert_bad_search_index_gives_problem_detail( - lambda: work_fixture.manager.work_controller.related( - identifier.type, identifier.identifier - ) - ) - # The mock search engine will return this Work for every # search. That means this book will show up as a 'same author' # recommendation, a 'same series' recommentation, and a @@ -699,13 +683,12 @@ def test_related_books(self, work_fixture: WorkFixture): mock_api.setup_method(metadata) # Now, ask for works related to work_fixture.english_1. - with mock_search_index(work_fixture.manager.external_search): - with work_fixture.request_context_with_library("/?entrypoint=Book"): - response = work_fixture.manager.work_controller.related( - work_fixture.identifier.type, - work_fixture.identifier.identifier, - novelist_api=mock_api, - ) + with work_fixture.request_context_with_library("/?entrypoint=Book"): + response = work_fixture.manager.work_controller.related( + work_fixture.identifier.type, + work_fixture.identifier.identifier, + novelist_api=mock_api, + ) assert 200 == response.status_code assert OPDSFeed.ACQUISITION_FEED_TYPE == response.headers["content-type"] feed = feedparser.parse(response.data) @@ -870,11 +853,6 @@ def test_series(self, work_fixture: WorkFixture): ) assert 400 == response.status_code - # Or if the search index isn't set up. - work_fixture.assert_bad_search_index_gives_problem_detail( - lambda: work_fixture.manager.work_controller.series(series_name, None, None) - ) - # Set up the mock search engine to return our work no matter # what query it's given. The fact that this book isn't # actually in the series doesn't matter, since determining diff --git a/tests/api/discovery/test_opds_registration.py b/tests/api/discovery/test_opds_registration.py index 60e67f23a5..1cc079f0a7 100644 --- a/tests/api/discovery/test_opds_registration.py +++ b/tests/api/discovery/test_opds_registration.py @@ -746,7 +746,7 @@ def process_library( # type: ignore[override] "--stage=testing", "--registry-url=http://registry.com/", ] - manager = MockCirculationManager(db.session) + manager = MockCirculationManager(db.session, MagicMock()) script.do_run(cmd_args=cmd_args, manager=manager) # One library was processed. diff --git a/tests/api/feed/fixtures.py b/tests/api/feed/conftest.py similarity index 100% rename from tests/api/feed/fixtures.py rename to tests/api/feed/conftest.py diff --git a/tests/api/feed/test_admin.py b/tests/api/feed/test_admin.py index cd2757880b..1fa405a0cd 100644 --- a/tests/api/feed/test_admin.py +++ b/tests/api/feed/test_admin.py @@ -5,8 +5,9 @@ from core.lane import Pagination from core.model.datasource import DataSource from core.model.measurement import Measurement -from tests.api.feed.fixtures import PatchedUrlFor, patch_url_for # noqa +from tests.api.feed.conftest import PatchedUrlFor from tests.fixtures.database import DatabaseTransactionFixture +from tests.fixtures.search import ExternalSearchFixtureFake class TestOPDS: @@ -20,7 +21,10 @@ def links(self, feed: FeedData, rel=None): return r def test_feed_includes_staff_rating( - self, db: DatabaseTransactionFixture, patch_url_for: PatchedUrlFor + self, + db: DatabaseTransactionFixture, + patch_url_for: PatchedUrlFor, + external_search_fake_fixture: ExternalSearchFixtureFake, ): work = db.work(with_open_access_download=True) lp = work.license_pools[0] @@ -44,7 +48,10 @@ def test_feed_includes_staff_rating( assert Measurement.RATING == entry.computed.ratings[1].additionalType # type: ignore[attr-defined] def test_feed_includes_refresh_link( - self, db: DatabaseTransactionFixture, patch_url_for: PatchedUrlFor + self, + db: DatabaseTransactionFixture, + patch_url_for: PatchedUrlFor, + external_search_fake_fixture: ExternalSearchFixtureFake, ): work = db.work(with_open_access_download=True) lp = work.license_pools[0] @@ -67,7 +74,10 @@ def test_feed_includes_refresh_link( ] def test_feed_includes_suppress_link( - self, db: DatabaseTransactionFixture, patch_url_for: PatchedUrlFor + self, + db: DatabaseTransactionFixture, + patch_url_for: PatchedUrlFor, + external_search_fake_fixture: ExternalSearchFixtureFake, ): work = db.work(with_open_access_download=True) lp = work.license_pools[0] @@ -120,7 +130,10 @@ def test_feed_includes_suppress_link( assert 0 == len(suppress_links) def test_feed_includes_edit_link( - self, db: DatabaseTransactionFixture, patch_url_for: PatchedUrlFor + self, + db: DatabaseTransactionFixture, + patch_url_for: PatchedUrlFor, + external_search_fake_fixture: ExternalSearchFixtureFake, ): work = db.work(with_open_access_download=True) lp = work.license_pools[0] @@ -137,7 +150,10 @@ def test_feed_includes_edit_link( assert edit_link.href and lp.identifier.identifier in edit_link.href def test_suppressed_feed( - self, db: DatabaseTransactionFixture, patch_url_for: PatchedUrlFor + self, + db: DatabaseTransactionFixture, + patch_url_for: PatchedUrlFor, + external_search_fake_fixture: ExternalSearchFixtureFake, ): # Test the ability to show a paginated feed of suppressed works. diff --git a/tests/api/feed/test_annotators.py b/tests/api/feed/test_annotators.py index 3e000914c8..b34ea6c4c2 100644 --- a/tests/api/feed/test_annotators.py +++ b/tests/api/feed/test_annotators.py @@ -25,7 +25,7 @@ from core.model.resource import Hyperlink, Resource from core.model.work import Work from core.util.datetime_helpers import datetime_utc, utc_now -from tests.api.feed.fixtures import PatchedUrlFor, patch_url_for # noqa +from tests.api.feed.conftest import PatchedUrlFor, patch_url_for # noqa from tests.fixtures.database import ( # noqa DatabaseTransactionFixture, DBStatementCounter, diff --git a/tests/api/feed/test_library_annotator.py b/tests/api/feed/test_library_annotator.py index f9b1dff344..ac16ecb550 100644 --- a/tests/api/feed/test_library_annotator.py +++ b/tests/api/feed/test_library_annotator.py @@ -43,9 +43,10 @@ from core.util.datetime_helpers import utc_now from core.util.flask_util import OPDSFeedResponse from core.util.opds_writer import OPDSFeed -from tests.api.feed.fixtures import PatchedUrlFor, patch_url_for # noqa +from tests.api.feed.conftest import PatchedUrlFor, patch_url_for # noqa from tests.fixtures.database import DatabaseTransactionFixture from tests.fixtures.library import LibraryFixture +from tests.fixtures.search import ExternalSearchFixtureFake from tests.fixtures.vendor_id import VendorIDFixture @@ -77,7 +78,9 @@ def __init__(self, db: DatabaseTransactionFixture): @pytest.fixture(scope="function") def annotator_fixture( - db: DatabaseTransactionFixture, patch_url_for: PatchedUrlFor + db: DatabaseTransactionFixture, + patch_url_for: PatchedUrlFor, + external_search_fake_fixture: ExternalSearchFixtureFake, ) -> LibraryAnnotatorFixture: return LibraryAnnotatorFixture(db) diff --git a/tests/api/feed/test_loan_and_hold_annotator.py b/tests/api/feed/test_loan_and_hold_annotator.py index 1af8c79ce3..e57656a722 100644 --- a/tests/api/feed/test_loan_and_hold_annotator.py +++ b/tests/api/feed/test_loan_and_hold_annotator.py @@ -16,6 +16,7 @@ from core.model.licensing import LicensePool from core.model.patron import Loan from tests.fixtures.database import DatabaseTransactionFixture +from tests.fixtures.search import ExternalSearchFixtureFake class TestLibraryLoanAndHoldAnnotator: @@ -195,7 +196,11 @@ def test_choose_best_hold_for_work(self, db: DatabaseTransactionFixture): [hold_1, hold_2] ) - def test_annotate_work_entry(self, db: DatabaseTransactionFixture): + def test_annotate_work_entry( + self, + db: DatabaseTransactionFixture, + external_search_fake_fixture: ExternalSearchFixtureFake, + ): library = db.default_library() patron = db.patron() identifier = db.identifier() diff --git a/tests/api/feed/test_opds_acquisition_feed.py b/tests/api/feed/test_opds_acquisition_feed.py index a8231bb82c..9ee466f8fb 100644 --- a/tests/api/feed/test_opds_acquisition_feed.py +++ b/tests/api/feed/test_opds_acquisition_feed.py @@ -36,7 +36,7 @@ from core.util.datetime_helpers import utc_now from core.util.flask_util import OPDSEntryResponse, OPDSFeedResponse from core.util.opds_writer import OPDSFeed, OPDSMessage -from tests.api.feed.fixtures import PatchedUrlFor, patch_url_for # noqa +from tests.api.feed.conftest import PatchedUrlFor, patch_url_for # noqa from tests.fixtures.database import DatabaseTransactionFixture @@ -996,7 +996,7 @@ class TestEntrypointLinkInsertionFixture: @pytest.fixture() def entrypoint_link_insertion_fixture( - db, + db: DatabaseTransactionFixture, ) -> Generator[TestEntrypointLinkInsertionFixture, None, None]: data = TestEntrypointLinkInsertionFixture() data.db = db @@ -1076,7 +1076,7 @@ def run(wl=None, facets=None): MockAnnotator(), None, facets, - search, + search_engine=search, ) return data.mock.called_with diff --git a/tests/api/mockapi/circulation.py b/tests/api/mockapi/circulation.py index eb693495df..6e3fffe791 100644 --- a/tests/api/mockapi/circulation.py +++ b/tests/api/mockapi/circulation.py @@ -11,11 +11,9 @@ PatronActivityCirculationAPI, ) from api.circulation_manager import CirculationManager -from core.external_search import ExternalSearchIndex from core.integration.settings import BaseSettings -from core.model import DataSource, Hold, Loan, get_one_or_create -from core.model.configuration import ExternalIntegration -from tests.mocks.search import ExternalSearchIndexFake +from core.model import DataSource, Hold, Loan +from core.service.container import Services class MockPatronActivityCirculationAPI(PatronActivityCirculationAPI, ABC): @@ -173,25 +171,8 @@ def api_for_license_pool(self, licensepool): class MockCirculationManager(CirculationManager): d_circulation: MockCirculationAPI - def __init__(self, db: Session): - super().__init__(db) - - def setup_search(self): - """Set up a search client.""" - integration, _ = get_one_or_create( - self._db, - ExternalIntegration, - goal=ExternalIntegration.SEARCH_GOAL, - protocol=ExternalIntegration.OPENSEARCH, - ) - integration.set_setting( - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY, "test_index" - ) - integration.set_setting( - ExternalSearchIndex.TEST_SEARCH_TERM_KEY, "a search term" - ) - integration.url = "http://does-not-exist.com/" - return ExternalSearchIndexFake(self._db) + def __init__(self, db: Session, services: Services): + super().__init__(db, services) def setup_circulation(self, library, analytics): """Set up the Circulation object.""" diff --git a/tests/api/test_controller_cm.py b/tests/api/test_controller_cm.py index 7be453e768..c4aa58176f 100644 --- a/tests/api/test_controller_cm.py +++ b/tests/api/test_controller_cm.py @@ -1,7 +1,6 @@ from unittest.mock import MagicMock from api.authenticator import LibraryAuthenticator -from api.circulation_manager import CirculationManager from api.config import Configuration from api.custom_index import CustomIndexView from api.problem_details import * @@ -18,7 +17,6 @@ # TODO: we can drop this when we drop support for Python 3.6 and 3.7 from tests.fixtures.api_controller import CirculationControllerFixture from tests.fixtures.database import IntegrationConfigurationFixture -from tests.mocks.search import SearchServiceFake class TestCirculationManager: @@ -34,7 +32,6 @@ def test_load_settings( # Certain fields of the CirculationManager have certain values # which are about to be reloaded. - manager._external_search = object() manager.auth = object() manager.patron_web_domains = object() @@ -109,9 +106,6 @@ def mock_for_library(incoming_library): LibraryAuthenticator, ) - # The ExternalSearch object has been reset. - assert isinstance(manager.external_search.search_service(), SearchServiceFake) - # So have the patron web domains, and their paths have been # removed. assert {"http://sitewide", "http://registration"} == manager.patron_web_domains @@ -146,24 +140,6 @@ def mock_for_library(incoming_library): # Restore the CustomIndexView.for_library implementation CustomIndexView.for_library = old_for_library - def test_exception_during_external_search_initialization_is_stored( - self, circulation_fixture: CirculationControllerFixture - ): - class BadSearch(CirculationManager): - @property - def setup_search(self): - raise Exception("doomed!") - - circulation = BadSearch(circulation_fixture.db.session) - - # We didn't get a search object. - assert None == circulation.external_search - - # The reason why is stored here. - ex = circulation.external_search_initialization_exception - assert isinstance(ex, Exception) - assert "doomed!" == str(ex) - def test_annotator(self, circulation_fixture: CirculationControllerFixture): # Test our ability to find an appropriate OPDSAnnotator for # any request context. diff --git a/tests/api/test_lanes.py b/tests/api/test_lanes.py index f4379e50a0..7beab88a52 100644 --- a/tests/api/test_lanes.py +++ b/tests/api/test_lanes.py @@ -1048,7 +1048,7 @@ def test_works( lane = CrawlableCollectionBasedLane() lane.initialize([db.default_collection()]) search = external_search_fake_fixture.external_search - search.query_works = MagicMock(return_value=[]) # type: ignore [method-assign] + search.query_works = MagicMock(return_value=[]) lane.works( db.session, facets=CrawlableFacets.default(None), search_engine=search ) diff --git a/tests/api/test_scripts.py b/tests/api/test_scripts.py index d172241f6f..3d9f0367c1 100644 --- a/tests/api/test_scripts.py +++ b/tests/api/test_scripts.py @@ -15,7 +15,6 @@ from api.adobe_vendor_id import AuthdataUtility from api.config import Configuration from api.novelist import NoveListAPI -from core.external_search import ExternalSearchIndex from core.integration.goals import Goals from core.marc import MARCExporter, MarcExporterLibrarySettings, MarcExporterSettings from core.model import ( @@ -41,7 +40,7 @@ NovelistSnapshotScript, ) from tests.fixtures.library import LibraryFixture -from tests.fixtures.search import EndToEndSearchFixture +from tests.fixtures.search import EndToEndSearchFixture, ExternalSearchFixtureFake if TYPE_CHECKING: from tests.fixtures.authenticator import SimpleAuthIntegrationFixture @@ -615,15 +614,11 @@ def test_initialize_database(self, db: DatabaseTransactionFixture): with patch( "scripts.SessionManager", autospec=SessionManager ) as session_manager: - with patch( - "scripts.ExternalSearchIndex", autospec=ExternalSearchIndex - ) as search_index: - with patch("scripts.command") as alemic_command: - script.initialize_database(mock_db) + with patch("scripts.command") as alemic_command: + script.initialize_database(mock_db) session_manager.initialize_data.assert_called_once() session_manager.initialize_schema.assert_called_once() - search_index.assert_called_once() alemic_command.stamp.assert_called_once() def test_migrate_database(self, db: DatabaseTransactionFixture): @@ -645,67 +640,59 @@ def test_find_alembic_ini(self, db: DatabaseTransactionFixture): assert conf.attributes["connection"] == mock_connection.engine assert conf.attributes["configure_logger"] is False - def test_initialize_search_indexes( - self, end_to_end_search_fixture: EndToEndSearchFixture + def test_initialize_search_indexes_mocked( + self, + external_search_fake_fixture: ExternalSearchFixtureFake, + caplog: LogCaptureFixture, ): - db = end_to_end_search_fixture.db - search = end_to_end_search_fixture.external_search_index - base_name = search._revision_base_name + caplog.set_level(logging.WARNING) + script = InstanceInitializationScript() - _mockable_search = ExternalSearchIndex(db.session) - _mockable_search.start_migration = MagicMock() # type: ignore [method-assign] - _mockable_search.search_service = MagicMock() # type: ignore [method-assign] - _mockable_search.log = MagicMock() + search_service = external_search_fake_fixture.external_search + search_service.start_migration = MagicMock() + search_service.search_service = MagicMock() - def mockable_search(*args): - return _mockable_search + # To fake "no migration is available", mock all the values + search_service.start_migration.return_value = None + search_service.search_service().is_pointer_empty.return_value = True - # Initially this should not exist, if InstanceInit has not been run - assert search.search_service().read_pointer() == None - - with patch("scripts.ExternalSearchIndex", new=mockable_search): - # To fake "no migration is available", mock all the values - - _mockable_search.start_migration.return_value = None - _mockable_search.search_service().is_pointer_empty.return_value = True - # Migration should fail - assert script.initialize_search_indexes(db.session) == False - # Logs were emitted - assert _mockable_search.log.warning.call_count == 1 - assert ( - "no migration was available" - in _mockable_search.log.warning.call_args[0][0] - ) + # Migration should fail + assert script.initialize_search_indexes() is False + + # Logs were emitted + record = caplog.records.pop() + assert "WARNING" in record.levelname + assert "no migration was available" in record.message - _mockable_search.search_service.reset_mock() - _mockable_search.start_migration.reset_mock() - _mockable_search.log.reset_mock() + search_service.search_service.reset_mock() + search_service.start_migration.reset_mock() - # In case there is no need for a migration, read pointer exists as a non-empty pointer - _mockable_search.search_service().is_pointer_empty.return_value = False - # Initialization should pass, as a no-op - assert script.initialize_search_indexes(db.session) == True - assert _mockable_search.start_migration.call_count == 0 + # In case there is no need for a migration, read pointer exists as a non-empty pointer + search_service.search_service().is_pointer_empty.return_value = False + + # Initialization should pass, as a no-op + assert script.initialize_search_indexes() is True + assert search_service.start_migration.call_count == 0 + + def test_initialize_search_indexes( + self, end_to_end_search_fixture: EndToEndSearchFixture + ): + search = end_to_end_search_fixture.external_search_index + base_name = end_to_end_search_fixture.external_search.service.base_revision_name + script = InstanceInitializationScript() + + # Initially this should not exist, if InstanceInit has not been run + assert search.search_service().read_pointer() is None # Initialization should work now - assert script.initialize_search_indexes(db.session) == True + assert script.initialize_search_indexes() is True # Then we have the latest version index assert ( search.search_service().read_pointer() == search._revision.name_for_index(base_name) ) - def test_initialize_search_indexes_no_integration( - self, db: DatabaseTransactionFixture - ): - script = InstanceInitializationScript() - script._log = MagicMock() - # No integration mean no migration - assert script.initialize_search_indexes(db.session) == False - assert script._log.error.call_count == 2 - assert "No search integration" in script._log.error.call_args[0][0] - class TestLanguageListScript: def test_languages(self, db: DatabaseTransactionFixture): diff --git a/tests/core/conftest.py b/tests/core/conftest.py index 1a48ae45fd..bb6feff578 100644 --- a/tests/core/conftest.py +++ b/tests/core/conftest.py @@ -3,7 +3,6 @@ pytest_plugins = [ "tests.fixtures.announcements", "tests.fixtures.csv_files", - "tests.fixtures.container", "tests.fixtures.database", "tests.fixtures.library", "tests.fixtures.opds2_files", diff --git a/tests/core/models/test_collection.py b/tests/core/models/test_collection.py index a33602ab9e..a8f887a3df 100644 --- a/tests/core/models/test_collection.py +++ b/tests/core/models/test_collection.py @@ -19,6 +19,7 @@ from core.model.licensing import Hold, License, LicensePool, Loan from core.model.work import Work from tests.fixtures.database import DatabaseTransactionFixture +from tests.fixtures.services import ServicesFixture class ExampleCollectionFixture: @@ -551,7 +552,11 @@ def expect(qu, works): setting.value = json.dumps([DataSource.OVERDRIVE, DataSource.FEEDBOOKS]) expect(qu, [overdrive_ebook]) - def test_delete(self, example_collection_fixture: ExampleCollectionFixture): + def test_delete( + self, + example_collection_fixture: ExampleCollectionFixture, + services_fixture_wired: ServicesFixture, + ): """Verify that Collection.delete will only operate on collections flagged for deletion, and that deletion cascades to all relevant related database objects. @@ -649,13 +654,16 @@ def test_delete(self, example_collection_fixture: ExampleCollectionFixture): index.remove_work.assert_called_once_with(work) # If no search_index is passed into delete() (the default behavior), - # we try to instantiate the normal ExternalSearchIndex object. Since - # no search index is configured, this will raise an exception -- but - # delete() will catch the exception and carry out the delete, - # without trying to delete any Works from the search index. + # then it will use the search index injected from the services + # container. collection2.marked_for_deletion = True collection2.delete() + # The search index was injected and told to remove the second work. + services_fixture_wired.search_fixture.index_mock.remove_work.assert_called_once_with( + work2 + ) + # We've now deleted every LicensePool created for this test. assert 0 == db.session.query(LicensePool).count() assert [] == work2.license_pools diff --git a/tests/core/models/test_work.py b/tests/core/models/test_work.py index f2a12c2710..f8408de5aa 100644 --- a/tests/core/models/test_work.py +++ b/tests/core/models/test_work.py @@ -260,7 +260,7 @@ def test_calculate_presentation( assert (utc_now() - work.last_update_time) < datetime.timedelta(seconds=2) # type: ignore[unreachable] # The index has not been updated. - assert [] == external_search_fake_fixture.search.documents_all() + assert [] == external_search_fake_fixture.service.documents_all() # The Work now has a complete set of WorkCoverageRecords # associated with it, reflecting all the operations that @@ -480,7 +480,7 @@ def test_set_presentation_ready_based_on_content( assert True == work.presentation_ready # The work has not been added to the search index. - assert [] == external_search_fake_fixture.search.documents_all() + assert [] == external_search_fake_fixture.service.documents_all() # But the work of adding it to the search engine has been # registered. @@ -1606,7 +1606,7 @@ def mock_reset_coverage(operation): # The work was not added to the search index when we called # external_index_needs_updating. That happens later, when the # WorkCoverageRecord is processed. - assert [] == external_search_fake_fixture.search.documents_all() + assert [] == external_search_fake_fixture.service.documents_all() def test_for_unchecked_subjects(self, db: DatabaseTransactionFixture): w1 = db.work(with_license_pool=True) diff --git a/tests/core/search/test_migration_states.py b/tests/core/search/test_migration_states.py index aa3ba92c01..f9c6f0fe19 100644 --- a/tests/core/search/test_migration_states.py +++ b/tests/core/search/test_migration_states.py @@ -14,8 +14,9 @@ import pytest -from core.external_search import ExternalSearchIndex, SearchIndexCoverageProvider +from core.external_search import ExternalSearchIndex from core.scripts import RunWorkCoverageProviderScript +from core.search.coverage_provider import SearchIndexCoverageProvider from core.search.document import SearchMappingDocument from core.search.revision import SearchSchemaRevision from core.search.revision_directory import SearchRevisionDirectory @@ -29,20 +30,19 @@ def test_initial_migration_case( ): fx = external_search_fixture db = fx.db + index = fx.index + service = fx.service # Ensure we are in the initial state, no test indices and pointer available - prefix = fx.integration.setting( - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY - ).value - all_indices = fx.search.indices.get("*") + prefix = fx.search_config.index_prefix + all_indices = fx.client.indices.get("*") for index_name in all_indices.keys(): - assert prefix not in index_name - - client = ExternalSearchIndex(db.session) + if prefix in index_name: + fx.client.indices.delete(index_name) # We cannot make any requests before we intitialize with pytest.raises(Exception) as raised: - client.query_works("") + index.query_works("") assert "index_not_found" in str(raised.value) # When a new sytem comes up the first code to run is the InstanceInitailization script @@ -50,22 +50,22 @@ def test_initial_migration_case( InstanceInitializationScript().initialize(db.session.connection()) # Ensure we have created the index and pointers - new_index_name = client._revision.name_for_index(client._revision_base_name) - empty_index_name = client.search_service()._empty(client._revision_base_name) # type: ignore [attr-defined] - all_indices = fx.search.indices.get("*") + new_index_name = index._revision.name_for_index(prefix) + empty_index_name = service._empty(prefix) + all_indices = fx.client.indices.get("*") assert prefix in new_index_name assert new_index_name in all_indices.keys() assert empty_index_name in all_indices.keys() - assert fx.search.indices.exists_alias( - client._search_read_pointer, index=new_index_name + assert fx.client.indices.exists_alias( + index._search_read_pointer, index=new_index_name ) - assert fx.search.indices.exists_alias( - client._search_write_pointer, index=new_index_name + assert fx.client.indices.exists_alias( + index._search_write_pointer, index=new_index_name ) # The same client should work without issue once the pointers are setup - assert client.query_works("").hits == [] + assert index.query_works("").hits == [] def test_migration_case(self, external_search_fixture: ExternalSearchFixture): fx = external_search_fixture @@ -85,23 +85,24 @@ def mapping_document(self) -> SearchMappingDocument: return SearchMappingDocument() client = ExternalSearchIndex( - db.session, + fx.search_container.service(), revision_directory=SearchRevisionDirectory( {MOCK_VERSION: MockSchema(MOCK_VERSION)} ), ) + # The search client works just fine assert client.query_works("") is not None receiver = client.start_updating_search_documents() receiver.add_documents([{"work_id": 123}]) receiver.finish() - mock_index_name = client._revision.name_for_index(client._revision_base_name) + mock_index_name = client._revision.name_for_index(fx.service.base_revision_name) assert str(MOCK_VERSION) in mock_index_name # The mock index does not exist yet with pytest.raises(Exception) as raised: - fx.search.indices.get(mock_index_name) + fx.client.indices.get(mock_index_name) assert "index_not_found" in str(raised.value) # This should run the migration @@ -110,10 +111,10 @@ def mapping_document(self) -> SearchMappingDocument: ).run() # The new version is created, and the aliases point to the right index - assert fx.search.indices.get(mock_index_name) is not None - assert mock_index_name in fx.search.indices.get_alias( + assert fx.client.indices.get(mock_index_name) is not None + assert mock_index_name in fx.client.indices.get_alias( name=client._search_read_pointer ) - assert mock_index_name in fx.search.indices.get_alias( + assert mock_index_name in fx.client.indices.get_alias( name=client._search_write_pointer ) diff --git a/tests/core/search/test_service.py b/tests/core/search/test_service.py index 4192e51996..59c9ad6c7c 100644 --- a/tests/core/search/test_service.py +++ b/tests/core/search/test_service.py @@ -30,7 +30,7 @@ def test_create_empty_idempotent( self, external_search_fixture: ExternalSearchFixture ): """Creating the empty index is idempotent.""" - service = SearchServiceOpensearch1(external_search_fixture.search, BASE_NAME) + service = SearchServiceOpensearch1(external_search_fixture.client, BASE_NAME) service.create_empty_index() # Log the index so that the fixture cleans it up afterward. @@ -38,7 +38,7 @@ def test_create_empty_idempotent( service.create_empty_index() - indices = external_search_fixture.search.indices.client.indices + indices = external_search_fixture.client.indices.client.indices assert indices is not None assert indices.exists("base-empty") @@ -46,7 +46,7 @@ def test_create_index_idempotent( self, external_search_fixture: ExternalSearchFixture ): """Creating any index is idempotent.""" - service = SearchServiceOpensearch1(external_search_fixture.search, BASE_NAME) + service = SearchServiceOpensearch1(external_search_fixture.client, BASE_NAME) revision = BasicMutableRevision(23) service.index_create(revision) service.index_create(revision) @@ -54,23 +54,23 @@ def test_create_index_idempotent( # Log the index so that the fixture cleans it up afterward. external_search_fixture.record_index("base-v23") - indices = external_search_fixture.search.indices.client.indices + indices = external_search_fixture.client.indices.client.indices assert indices is not None assert indices.exists(revision.name_for_index("base")) def test_read_pointer_none(self, external_search_fixture: ExternalSearchFixture): """The read pointer is initially unset.""" - service = SearchServiceOpensearch1(external_search_fixture.search, BASE_NAME) + service = SearchServiceOpensearch1(external_search_fixture.client, BASE_NAME) assert None == service.read_pointer() def test_write_pointer_none(self, external_search_fixture: ExternalSearchFixture): """The write pointer is initially unset.""" - service = SearchServiceOpensearch1(external_search_fixture.search, BASE_NAME) + service = SearchServiceOpensearch1(external_search_fixture.client, BASE_NAME) assert None == service.write_pointer() def test_read_pointer_set(self, external_search_fixture: ExternalSearchFixture): """Setting the read pointer works.""" - service = SearchServiceOpensearch1(external_search_fixture.search, BASE_NAME) + service = SearchServiceOpensearch1(external_search_fixture.client, BASE_NAME) revision = BasicMutableRevision(23) service.index_create(revision) @@ -84,7 +84,7 @@ def test_read_pointer_set_empty( self, external_search_fixture: ExternalSearchFixture ): """Setting the read pointer to the empty index works.""" - service = SearchServiceOpensearch1(external_search_fixture.search, BASE_NAME) + service = SearchServiceOpensearch1(external_search_fixture.client, BASE_NAME) service.create_empty_index() # Log the index so that the fixture cleans it up afterward. @@ -95,7 +95,7 @@ def test_read_pointer_set_empty( def test_write_pointer_set(self, external_search_fixture: ExternalSearchFixture): """Setting the write pointer works.""" - service = SearchServiceOpensearch1(external_search_fixture.search, BASE_NAME) + service = SearchServiceOpensearch1(external_search_fixture.client, BASE_NAME) revision = BasicMutableRevision(23) service.index_create(revision) @@ -112,7 +112,7 @@ def test_populate_index_idempotent( self, external_search_fixture: ExternalSearchFixture ): """Populating an index is idempotent.""" - service = SearchServiceOpensearch1(external_search_fixture.search, BASE_NAME) + service = SearchServiceOpensearch1(external_search_fixture.client, BASE_NAME) revision = BasicMutableRevision(23) mappings = revision.mapping_document() @@ -150,7 +150,7 @@ def test_populate_index_idempotent( service.index_submit_documents("base-v23", documents) service.index_submit_documents("base-v23", documents) - indices = external_search_fixture.search.indices.client.indices + indices = external_search_fixture.client.indices.client.indices assert indices is not None assert indices.exists(revision.name_for_index("base")) assert indices.get(revision.name_for_index("base"))["base-v23"]["mappings"] == { diff --git a/tests/core/test_external_search.py b/tests/core/test_external_search.py index b92d1b17a6..dcc8972f2d 100644 --- a/tests/core/test_external_search.py +++ b/tests/core/test_external_search.py @@ -34,7 +34,6 @@ QueryParseException, QueryParser, SearchBase, - SearchIndexCoverageProvider, SortKeyPagination, WorkSearchResult, ) @@ -56,6 +55,7 @@ from core.model.work import Work from core.problem_details import INVALID_INPUT from core.scripts import RunWorkCoverageProviderScript +from core.search.coverage_provider import SearchIndexCoverageProvider from core.search.document import SearchMappingDocument from core.search.revision import SearchSchemaRevision from core.search.revision_directory import SearchRevisionDirectory @@ -132,85 +132,6 @@ def query_works_multi(self, queries, debug=False): assert pagination.offset == default.offset assert pagination.size == default.size - def test__run_self_tests(self, end_to_end_search_fixture: EndToEndSearchFixture): - transaction = end_to_end_search_fixture.db - session = transaction.session - index = end_to_end_search_fixture.external_search_index - - # Intrusively set the search term to something useful. - index._test_search_term = "How To Search" - - # Start with an up-to-date but empty index. - index.start_migration().finish() - - # First, see what happens when the search returns no results. - test_results = [x for x in index._run_self_tests(session)] - - assert "Search results for 'How To Search':" == test_results[0].name - assert True == test_results[0].success - assert [] == test_results[0].result - - assert "Search document for 'How To Search':" == test_results[1].name - assert True == test_results[1].success - assert {} != test_results[1].result - - assert "Raw search results for 'How To Search':" == test_results[2].name - assert True == test_results[2].success - assert [] == test_results[2].result - - assert ( - "Total number of search results for 'How To Search':" - == test_results[3].name - ) - assert True == test_results[3].success - assert "0" == test_results[3].result - - assert "Total number of documents in this search index:" == test_results[4].name - assert True == test_results[4].success - assert "0" == test_results[4].result - - assert "Total number of documents per collection:" == test_results[5].name - assert True == test_results[5].success - assert "{}" == test_results[5].result - - # Set up the search index so it will return a result. - work = end_to_end_search_fixture.external_search.default_work( - title="How To Search" - ) - work.presentation_ready = True - work.presentation_edition.subtitle = "How To Search" - work.presentation_edition.series = "Classics" - work.summary_text = "How To Search!" - work.presentation_edition.publisher = "Project Gutenberg" - work.last_update_time = datetime_utc(2019, 1, 1) - work.license_pools[0].licenses_available = 100000 - - docs = index.start_updating_search_documents() - docs.add_documents(index.create_search_documents_from_works([work])) - docs.finish() - - test_results = [x for x in index._run_self_tests(session)] - - assert "Search results for 'How To Search':" == test_results[0].name - assert True == test_results[0].success - assert [f"How To Search ({work.author})"] == test_results[0].result - - assert ( - "Total number of search results for 'How To Search':" - == test_results[3].name - ) - assert True == test_results[3].success - assert "1" == test_results[3].result - - assert "Total number of documents in this search index:" == test_results[4].name - assert True == test_results[4].success - assert "1" == test_results[4].result - - assert "Total number of documents per collection:" == test_results[5].name - assert True == test_results[5].success - result = json.loads(test_results[5].result) - assert {"Default Collection": 1} == result - class TestSearchV5: def test_character_filters(self): @@ -978,7 +899,10 @@ def pages(worklist): while pagination: pages.append( worklist.works( - session, facets, pagination, fixture.external_search_index + session, + facets, + pagination, + search_engine=fixture.external_search_index, ) ) pagination = pagination.next_page @@ -1106,14 +1030,14 @@ def pages(worklist): expect([data.lincoln_vampire], "fantasy") def test_remove_work(self, end_to_end_search_fixture: EndToEndSearchFixture): - search = end_to_end_search_fixture.external_search.search + client = end_to_end_search_fixture.external_search.client data = self._populate_works(end_to_end_search_fixture) end_to_end_search_fixture.populate_search_index() end_to_end_search_fixture.external_search_index.remove_work(data.moby_dick) end_to_end_search_fixture.external_search_index.remove_work(data.moby_duck) # Immediately querying never works, the search index needs to refresh its cache/index/data - search.indices.refresh() + client.indices.refresh() end_to_end_search_fixture.expect_results([], "Moby") @@ -2113,7 +2037,11 @@ def assert_featured(description, worklist, facets, expect): # available books will show up before all of the unavailable # books. only_availability_matters = worklist.works( - session, facets, None, fixture.external_search_index, debug=True + session, + facets, + None, + search_engine=fixture.external_search_index, + debug=True, ) assert 5 == len(only_availability_matters) last_two = only_availability_matters[-2:] @@ -4723,7 +4651,9 @@ def test_works_not_presentation_ready_kept_in_index( # All three works were inserted into the index, even the one # that's not presentation-ready. ids = set( - map(lambda d: d["_id"], external_search_fake_fixture.search.documents_all()) + map( + lambda d: d["_id"], external_search_fake_fixture.service.documents_all() + ) ) assert {w1.id, w2.id, w3.id} == ids @@ -4736,7 +4666,9 @@ def test_works_not_presentation_ready_kept_in_index( ) docs.finish() assert {w1.id, w2.id, w3.id} == set( - map(lambda d: d["_id"], external_search_fake_fixture.search.documents_all()) + map( + lambda d: d["_id"], external_search_fake_fixture.service.documents_all() + ) ) assert [] == failures @@ -4750,7 +4682,7 @@ def test_search_connection_timeout( external_search_fake_fixture.db, ) - search.search.set_failing_mode( + search.service.set_failing_mode( mode=SearchServiceFailureMode.FAIL_INDEXING_DOCUMENTS_TIMEOUT ) work = transaction.work() @@ -4766,7 +4698,7 @@ def test_search_connection_timeout( # Submissions are not retried by the base service assert [work.id] == [ - docs["_id"] for docs in search.search.document_submission_attempts + docs["_id"] for docs in search.service.document_submission_attempts ] def test_search_single_document_error( @@ -4777,7 +4709,7 @@ def test_search_single_document_error( external_search_fake_fixture.db, ) - search.search.set_failing_mode( + search.service.set_failing_mode( mode=SearchServiceFailureMode.FAIL_INDEXING_DOCUMENTS ) work = transaction.work() @@ -4793,7 +4725,7 @@ def test_search_single_document_error( # Submissions are not retried by the base service assert [work.id] == [ - docs["_id"] for docs in search.search.document_submission_attempts + docs["_id"] for docs in search.service.document_submission_attempts ] @@ -4942,7 +4874,7 @@ def test_success( assert [work] == results # The work was added to the search index. - search_service = external_search_fake_fixture.search + search_service = external_search_fake_fixture.service assert 1 == len(search_service.documents_all()) def test_failure( @@ -4953,7 +4885,7 @@ def test_failure( work = db.work() work.set_presentation_ready() index = external_search_fake_fixture.external_search - external_search_fake_fixture.search.set_failing_mode( + external_search_fake_fixture.service.set_failing_mode( SearchServiceFailureMode.FAIL_INDEXING_DOCUMENTS ) diff --git a/tests/core/test_lane.py b/tests/core/test_lane.py index 2676883d20..f3959ff4c6 100644 --- a/tests/core/test_lane.py +++ b/tests/core/test_lane.py @@ -1,6 +1,7 @@ import datetime import logging import random +from typing import cast from unittest.mock import MagicMock, call import pytest @@ -15,7 +16,7 @@ EntryPoint, EverythingEntryPoint, ) -from core.external_search import Filter, WorkSearchResult, mock_search_index +from core.external_search import ExternalSearchIndex, Filter, WorkSearchResult from core.lane import ( DatabaseBackedFacets, DatabaseBackedWorkList, @@ -2341,7 +2342,11 @@ def works_for_hits(self, _db, work_ids, facets=None): # Ask the WorkList for a page of works, using the search index # to drive the query instead of the database. result = wl.works( - db.session, facets, mock_pagination, search_client, mock_debug + db.session, + facets, + mock_pagination, + search_engine=cast(ExternalSearchIndex, search_client), + debug=mock_debug, ) # MockSearchClient.query_works was used to grab a list of work @@ -3551,8 +3556,7 @@ def count_works(self, filter): fiction = db.lane(display_name="Fiction", fiction=True) fiction.size = 44 fiction.size_by_entrypoint = {"Nonexistent entrypoint": 33} - with mock_search_index(search_engine): - fiction.update_size(db.session) + fiction.update_size(db.session, search_engine=search_engine) # The lane size is also calculated individually for every # enabled entry point. EverythingEntryPoint is used for the @@ -4222,7 +4226,6 @@ def test_groups( fixture.external_search.db, fixture.external_search.db.session, ) - fixture.external_search_index.start_migration().finish() # type: ignore [union-attr] # Tell the fixture to call our populate_works method. # In this library, the groups feed includes at most two books diff --git a/tests/core/test_local_analytics_provider.py b/tests/core/test_local_analytics_provider.py index 7b54da8f5b..0ac174b275 100644 --- a/tests/core/test_local_analytics_provider.py +++ b/tests/core/test_local_analytics_provider.py @@ -10,7 +10,6 @@ if TYPE_CHECKING: from tests.fixtures.database import DatabaseTransactionFixture - from tests.fixtures.services import MockServicesFixture class LocalAnalyticsProviderFixture: @@ -21,18 +20,16 @@ class LocalAnalyticsProviderFixture: def __init__( self, transaction: DatabaseTransactionFixture, - mock_services_fixture: MockServicesFixture, ): self.transaction = transaction - self.services = mock_services_fixture.services self.la = LocalAnalyticsProvider() @pytest.fixture() def local_analytics_provider_fixture( - db: DatabaseTransactionFixture, mock_services_fixture: MockServicesFixture + db: DatabaseTransactionFixture, ) -> LocalAnalyticsProviderFixture: - return LocalAnalyticsProviderFixture(db, mock_services_fixture) + return LocalAnalyticsProviderFixture(db) class TestLocalAnalyticsProvider: diff --git a/tests/core/test_s3_analytics_provider.py b/tests/core/test_s3_analytics_provider.py index bcb64444d1..f957259182 100644 --- a/tests/core/test_s3_analytics_provider.py +++ b/tests/core/test_s3_analytics_provider.py @@ -3,7 +3,7 @@ import datetime import json from typing import TYPE_CHECKING -from unittest.mock import MagicMock +from unittest.mock import MagicMock, create_autospec import pytest @@ -11,29 +11,25 @@ from core.classifier import Classifier from core.config import CannotLoadConfiguration from core.model import CirculationEvent, DataSource, MediaTypes +from core.service.storage.s3 import S3Service if TYPE_CHECKING: from tests.fixtures.database import DatabaseTransactionFixture - from tests.fixtures.services import MockServicesFixture class S3AnalyticsFixture: - def __init__( - self, db: DatabaseTransactionFixture, services_fixture: MockServicesFixture - ) -> None: + def __init__(self, db: DatabaseTransactionFixture) -> None: self.db = db - self.services = services_fixture.services - self.analytics_storage = services_fixture.storage.analytics + + self.analytics_storage = create_autospec(S3Service) self.analytics_provider = S3AnalyticsProvider( - services_fixture.services.storage.analytics(), + self.analytics_storage, ) @pytest.fixture(scope="function") -def s3_analytics_fixture( - db: DatabaseTransactionFixture, mock_services_fixture: MockServicesFixture -): - return S3AnalyticsFixture(db, mock_services_fixture) +def s3_analytics_fixture(db: DatabaseTransactionFixture): + return S3AnalyticsFixture(db) class TestS3AnalyticsProvider: @@ -52,13 +48,8 @@ def timestamp_to_string(timestamp): def test_exception_is_raised_when_no_analytics_bucket_configured( self, s3_analytics_fixture: S3AnalyticsFixture ): - # The services container returns None when there is no analytics storage service configured, - # so we override the analytics storage service with None to simulate this situation. - s3_analytics_fixture.services.storage.analytics.override(None) - - provider = S3AnalyticsProvider( - s3_analytics_fixture.services.storage.analytics() - ) + # The services container returns None when there is no analytics storage service configured + provider = S3AnalyticsProvider(None) # Act, Assert with pytest.raises(CannotLoadConfiguration): diff --git a/tests/core/test_scripts.py b/tests/core/test_scripts.py index 18344d44ba..10213984c9 100644 --- a/tests/core/test_scripts.py +++ b/tests/core/test_scripts.py @@ -13,7 +13,7 @@ from api.lanes import create_default_lanes from core.classifier import Classifier -from core.config import CannotLoadConfiguration, Configuration, ConfigurationConstants +from core.config import Configuration, ConfigurationConstants from core.external_search import ExternalSearchIndex, Filter from core.lane import Lane, WorkList from core.metadata_layer import TimestampData @@ -90,6 +90,7 @@ ) from tests.fixtures.database import DatabaseTransactionFixture from tests.fixtures.search import EndToEndSearchFixture, ExternalSearchFixtureFake +from tests.fixtures.services import ServicesFixture class TestScript: @@ -1608,23 +1609,6 @@ def out(self, s, *args): class TestWhereAreMyBooksScript: - def test_no_search_integration(self, db: DatabaseTransactionFixture): - # We can't even get started without a working search integration. - - # We'll also test the out() method by mocking the script's - # standard output and using the normal out() implementation. - # In other tests, which have more complicated output, we mock - # out(), so this verifies that output actually gets written - # out. - output = StringIO() - pytest.raises( - CannotLoadConfiguration, WhereAreMyBooksScript, db.session, output=output - ) - assert ( - "Here's your problem: the search integration is missing or misconfigured.\n" - == output.getvalue() - ) - @pytest.mark.skip( reason="This test currently freezes inside pytest and has to be killed with SIGKILL." ) @@ -2018,7 +2002,9 @@ def test_do_run( # The mock methods were called with the values we expect. assert {work.id, work2.id} == set( - map(lambda d: d["_id"], external_search_fake_fixture.search.documents_all()) + map( + lambda d: d["_id"], external_search_fake_fixture.service.documents_all() + ) ) # The script returned a list containing a single @@ -2286,38 +2272,33 @@ def test_process_custom_list( assert custom_list.auto_update_last_update == frozen_time.time_to_freeze assert custom_list1.auto_update_last_update == frozen_time.time_to_freeze - def test_search_facets(self, end_to_end_search_fixture: EndToEndSearchFixture): - with patch("core.query.customlist.ExternalSearchIndex") as mock_index: - fixture = end_to_end_search_fixture - db, session = ( - fixture.external_search.db, - fixture.external_search.db.session, - ) - data = self._populate_works(fixture) - fixture.populate_search_index() + def test_search_facets( + self, db: DatabaseTransactionFixture, services_fixture_wired: ServicesFixture + ): + mock_index = services_fixture_wired.search_fixture.index_mock - last_updated = datetime.datetime.now() - datetime.timedelta(hours=1) - custom_list, _ = db.customlist() - custom_list.library = db.default_library() - custom_list.auto_update_enabled = True - custom_list.auto_update_query = json.dumps( - dict(query=dict(key="title", value="Populated Book")) - ) - custom_list.auto_update_facets = json.dumps( - dict(order="title", languages="fr", media=["book", "audio"]) - ) - custom_list.auto_update_last_update = last_updated + last_updated = datetime.datetime.now() - datetime.timedelta(hours=1) + custom_list, _ = db.customlist() + custom_list.library = db.default_library() + custom_list.auto_update_enabled = True + custom_list.auto_update_query = json.dumps( + dict(query=dict(key="title", value="Populated Book")) + ) + custom_list.auto_update_facets = json.dumps( + dict(order="title", languages="fr", media=["book", "audio"]) + ) + custom_list.auto_update_last_update = last_updated - script = CustomListUpdateEntriesScript(session) - script.process_custom_list(custom_list) + script = CustomListUpdateEntriesScript(db.session) + script.process_custom_list(custom_list) - assert mock_index().query_works.call_count == 1 - filter: Filter = mock_index().query_works.call_args_list[0][0][1] - assert filter.sort_order[0] == { - "sort_title": "asc" - } # since we asked for title ordering this should come up first - assert filter.languages == ["fr"] - assert filter.media == ["book", "audio"] + assert mock_index.query_works.call_count == 1 + filter: Filter = mock_index.query_works.call_args_list[0][0][1] + assert filter.sort_order[0] == { + "sort_title": "asc" + } # since we asked for title ordering this should come up first + assert filter.languages == ["fr"] + assert filter.media == ["book", "audio"] @freeze_time("2022-01-01", as_kwarg="frozen_time") def test_no_last_update( diff --git a/tests/fixtures/api_controller.py b/tests/fixtures/api_controller.py index ae4d01ba38..0187413c9b 100644 --- a/tests/fixtures/api_controller.py +++ b/tests/fixtures/api_controller.py @@ -37,9 +37,11 @@ IntegrationConfiguration, IntegrationLibraryConfiguration, ) +from core.service.container import Services, wire_container from core.util import base64 from tests.api.mockapi.circulation import MockCirculationManager from tests.fixtures.database import DatabaseTransactionFixture +from tests.mocks.search import ExternalSearchIndexFake class ControllerFixtureSetupOverrides: @@ -97,11 +99,17 @@ def __init__( # were created in the test setup. app.config["PRESERVE_CONTEXT_ON_EXCEPTION"] = False + # Set up the fake search index. + self.search_index = ExternalSearchIndexFake() + self.services_container = Services() + self.services_container.search.index.override(self.search_index) + if setup_cm: # NOTE: Any reference to self._default_library below this # point in this method will cause the tests in # TestScopedSession to hang. self.set_base_url() + app.manager = self.circulation_manager_setup() def set_base_url(self): @@ -110,6 +118,18 @@ def set_base_url(self): ) base_url.value = "http://test-circulation-manager/" + def wire_container(self): + wire_container(self.services_container) + + def unwire_container(self): + self.services_container.unwire() + + @contextmanager + def wired_container(self): + self.wire_container() + yield + self.unwire_container() + def circulation_manager_setup_with_session( self, session: Session, overrides: ControllerFixtureSetupOverrides | None = None ) -> CirculationManager: @@ -159,7 +179,9 @@ def circulation_manager_setup_with_session( self.default_patron = self.default_patrons[self.library] self.authdata = AuthdataUtility.from_config(self.library) - self.manager = MockCirculationManager(session) + + # Create mock CM instance + self.manager = MockCirculationManager(session, self.services_container) # Set CirculationAPI and top-level lane for the default # library, for convenience in tests. @@ -339,33 +361,6 @@ def add_works(self, works: list[WorkSpec]): ) self.manager.external_search.mock_query_works_multi(self.works) - def assert_bad_search_index_gives_problem_detail(self, test_function): - """Helper method to test that a controller method serves a problem - detail document when the search index isn't set up. - - Mocking a broken search index is a lot of work; thus the helper method. - """ - old_setup = self.manager.setup_external_search - old_value = self.manager._external_search - - try: - self.manager._external_search = None - self.manager.setup_external_search = lambda: None - with self.request_context_with_library("/"): - response = test_function() - assert 502 == response.status_code - assert ( - "http://librarysimplified.org/terms/problem/remote-integration-failed" - == response.uri - ) - assert ( - "The search index for this site is not properly configured." - == response.detail - ) - finally: - self.manager.setup_external_search = old_setup - self.manager._external_search = old_value - @pytest.fixture(scope="function") def circulation_fixture(db: DatabaseTransactionFixture): diff --git a/tests/fixtures/api_routes.py b/tests/fixtures/api_routes.py index e9823dbad5..3ecdb0b69a 100644 --- a/tests/fixtures/api_routes.py +++ b/tests/fixtures/api_routes.py @@ -1,6 +1,7 @@ import logging from collections.abc import Generator from typing import Any +from unittest.mock import MagicMock import flask import pytest @@ -128,7 +129,7 @@ def __init__( self.controller_fixture = controller_fixture self.setup_circulation_manager = False if not RouteTestFixture.REAL_CIRCULATION_MANAGER: - manager = MockCirculationManager(self.db.session) + manager = MockCirculationManager(self.db.session, MagicMock()) RouteTestFixture.REAL_CIRCULATION_MANAGER = manager app = MockApp() diff --git a/tests/fixtures/container.py b/tests/fixtures/container.py deleted file mode 100644 index 8d30774930..0000000000 --- a/tests/fixtures/container.py +++ /dev/null @@ -1,9 +0,0 @@ -import pytest - -from core.service.container import container_instance - - -@pytest.fixture(autouse=True) -def services_container_instance(): - # This creates and wires the container - return container_instance() diff --git a/tests/fixtures/search.py b/tests/fixtures/search.py index bb54290942..63f7c4c750 100644 --- a/tests/fixtures/search.py +++ b/tests/fixtures/search.py @@ -1,74 +1,82 @@ -import logging -import os -from collections.abc import Iterable +from __future__ import annotations + +from collections.abc import Generator import pytest from opensearchpy import OpenSearch - -from core.external_search import ExternalSearchIndex, SearchIndexCoverageProvider -from core.model import ExternalIntegration, Work +from pydantic import AnyHttpUrl + +from core.external_search import ExternalSearchIndex +from core.model import Work +from core.search.coverage_provider import SearchIndexCoverageProvider +from core.search.service import SearchServiceOpensearch1 +from core.service.configuration import ServiceConfiguration +from core.service.container import Services, wire_container +from core.service.search.container import Search +from core.util.log import LoggerMixin from tests.fixtures.database import DatabaseTransactionFixture +from tests.fixtures.services import ServicesFixture from tests.mocks.search import SearchServiceFake -class ExternalSearchFixture: +class SearchTestConfiguration(ServiceConfiguration): + url: AnyHttpUrl + index_prefix: str = "test_index" + timeout: int = 20 + maxsize: int = 25 + + class Config: + env_prefix = "PALACE_TEST_SEARCH_" + + +class ExternalSearchFixture(LoggerMixin): """ - These tests require opensearch to be running locally. If it's not, or there's - an error creating the index, the tests will pass without doing anything. + These tests require opensearch to be running locally. Tests for opensearch are useful for ensuring that we haven't accidentally broken a type of search by changing analyzers or queries, but search needs to be tested manually to ensure that it works well overall, with a realistic index. """ - integration: ExternalIntegration - db: DatabaseTransactionFixture - search: OpenSearch - _indexes_created: list[str] + def __init__(self, db: DatabaseTransactionFixture, services: Services): + self.search_config = SearchTestConfiguration() + self.services_container = services - def __init__(self): + # Set up our testing search instance in the services container + self.search_container = Search() + self.search_container.config.from_dict(self.search_config.dict()) + self.services_container.search.override(self.search_container) + + self._indexes_created: list[str] = [] + self.db = db + self.client: OpenSearch = services.search.client() + self.service: SearchServiceOpensearch1 = services.search.service() + self.index: ExternalSearchIndex = services.search.index() self._indexes_created = [] - self._logger = logging.getLogger(ExternalSearchFixture.__name__) - - @classmethod - def create(cls, db: DatabaseTransactionFixture) -> "ExternalSearchFixture": - fixture = ExternalSearchFixture() - fixture.db = db - fixture.integration = db.external_integration( - ExternalIntegration.OPENSEARCH, - goal=ExternalIntegration.SEARCH_GOAL, - url=fixture.url, - settings={ - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY: "test_index", - ExternalSearchIndex.TEST_SEARCH_TERM_KEY: "test_search_term", - }, - ) - fixture.search = OpenSearch(fixture.url, use_ssl=False, timeout=20, maxsize=25) - return fixture - @property - def url(self) -> str: - env = os.environ.get("SIMPLIFIED_TEST_OPENSEARCH") - if env is None: - raise OSError("SIMPLIFIED_TEST_OPENSEARCH is not defined.") - return env + # Make sure the services container is wired up with the newly created search container + wire_container(self.services_container) def record_index(self, name: str): - self._logger.info(f"Recording index {name} for deletion") + self.log.info(f"Recording index {name} for deletion") self._indexes_created.append(name) def close(self): for index in self._indexes_created: try: - self._logger.info(f"Deleting index {index}") - self.search.indices.delete(index) + self.log.info(f"Deleting index {index}") + self.client.indices.delete(index) except Exception as e: - self._logger.info(f"Failed to delete index {index}: {e}") + self.log.info(f"Failed to delete index {index}: {e}") # Force test index deletion - self.search.indices.delete("test_index*") - self._logger.info("Waiting for operations to complete.") - self.search.indices.refresh() + self.client.indices.delete("test_index*") + self.log.info("Waiting for operations to complete.") + self.client.indices.refresh() + + # Unwire the services container + self.services_container.unwire() + self.services_container.search.reset_override() return None def default_work(self, *args, **kwargs): @@ -83,54 +91,38 @@ def default_work(self, *args, **kwargs): return work def init_indices(self): - client = ExternalSearchIndex(self.db.session) - client.initialize_indices() + self.index.initialize_indices() @pytest.fixture(scope="function") def external_search_fixture( - db: DatabaseTransactionFixture, -) -> Iterable[ExternalSearchFixture]: + db: DatabaseTransactionFixture, services_fixture: ServicesFixture +) -> Generator[ExternalSearchFixture, None, None]: """Ask for an external search system.""" """Note: You probably want EndToEndSearchFixture instead.""" - data = ExternalSearchFixture.create(db) - yield data - data.close() + fixture = ExternalSearchFixture(db, services_fixture.services) + yield fixture + fixture.close() class EndToEndSearchFixture: """An external search system fixture that can be populated with data for end-to-end tests.""" """Tests are expected to call the `populate()` method to populate the fixture with test-specific data.""" - external_search: ExternalSearchFixture - external_search_index: ExternalSearchIndex - db: DatabaseTransactionFixture - - def __init__(self): - self._logger = logging.getLogger(EndToEndSearchFixture.__name__) - @classmethod - def create(cls, transaction: DatabaseTransactionFixture) -> "EndToEndSearchFixture": - data = EndToEndSearchFixture() - data.db = transaction - data.external_search = ExternalSearchFixture.create(transaction) - data.external_search_index = ExternalSearchIndex(transaction.session) - return data + def __init__(self, search_fixture: ExternalSearchFixture): + self.db = search_fixture.db + self.external_search = search_fixture + self.external_search_index = search_fixture.index def populate_search_index(self): """Populate the search index with a set of works. The given callback is passed this fixture instance.""" - - # Create some works. - if not self.external_search.search: - # No search index is configured -- nothing to do. - return - # Add all the works created in the setup to the search index. SearchIndexCoverageProvider( self.external_search.db.session, search_index_client=self.external_search_index, ).run() - self.external_search.search.indices.refresh() + self.external_search.client.indices.refresh() @staticmethod def assert_works(description, expect, actual, should_be_ordered=True): @@ -249,48 +241,43 @@ def close(self): for index in self.external_search_index.search_service().indexes_created(): self.external_search.record_index(index) - self.external_search.close() - @pytest.fixture(scope="function") def end_to_end_search_fixture( - db: DatabaseTransactionFixture, -) -> Iterable[EndToEndSearchFixture]: + external_search_fixture: ExternalSearchFixture, +) -> Generator[EndToEndSearchFixture, None, None]: """Ask for an external search system that can be populated with data for end-to-end tests.""" - data = EndToEndSearchFixture.create(db) - try: - yield data - except Exception: - raise - finally: - data.close() + fixture = EndToEndSearchFixture(external_search_fixture) + yield fixture + fixture.close() class ExternalSearchFixtureFake: - integration: ExternalIntegration - db: DatabaseTransactionFixture - search: SearchServiceFake - external_search: ExternalSearchIndex + def __init__(self, db: DatabaseTransactionFixture, services: Services): + self.db = db + self.services = services + self.search_container = Search() + self.services.search.override(self.search_container) + + self.service = SearchServiceFake() + self.search_container.service.override(self.service) + self.external_search: ExternalSearchIndex = self.services.search.index() + + wire_container(self.services) + + def close(self): + self.services.unwire() + self.services.search.reset_override() @pytest.fixture(scope="function") def external_search_fake_fixture( - db: DatabaseTransactionFixture, -) -> ExternalSearchFixtureFake: + db: DatabaseTransactionFixture, services_fixture: ServicesFixture +) -> Generator[ExternalSearchFixtureFake, None, None]: """Ask for an external search system that can be populated with data for end-to-end tests.""" - data = ExternalSearchFixtureFake() - data.db = db - data.integration = db.external_integration( - ExternalIntegration.OPENSEARCH, - goal=ExternalIntegration.SEARCH_GOAL, - url="http://does-not-exist.com/", - settings={ - ExternalSearchIndex.WORKS_INDEX_PREFIX_KEY: "test_index", - ExternalSearchIndex.TEST_SEARCH_TERM_KEY: "a search term", - }, - ) - data.search = SearchServiceFake() - data.external_search = ExternalSearchIndex( - _db=db.session, custom_client_service=data.search + fixture = ExternalSearchFixtureFake( + db=db, + services=services_fixture.services, ) - return data + yield fixture + fixture.close() diff --git a/tests/fixtures/services.py b/tests/fixtures/services.py index edbeccb524..52600241f6 100644 --- a/tests/fixtures/services.py +++ b/tests/fixtures/services.py @@ -1,42 +1,155 @@ -from unittest.mock import MagicMock +from collections.abc import Generator +from contextlib import contextmanager +from dataclasses import dataclass +from unittest.mock import MagicMock, create_autospec +import boto3 import pytest -from core.service.container import Services +from core.analytics import Analytics +from core.external_search import ExternalSearchIndex +from core.search.revision_directory import SearchRevisionDirectory +from core.search.service import SearchServiceOpensearch1 +from core.service.analytics.container import AnalyticsContainer +from core.service.container import Services, wire_container +from core.service.logging.container import Logging +from core.service.logging.log import setup_logging +from core.service.search.container import Search from core.service.storage.container import Storage from core.service.storage.s3 import S3Service -class MockStorageFixture: - def __init__(self): - self.storage = Storage() - self.analytics = MagicMock(spec=S3Service) - self.storage.analytics.override(self.analytics) - self.public = MagicMock(spec=S3Service) - self.storage.public.override(self.public) - self.s3_client = MagicMock() - self.storage.s3_client.override(self.s3_client) +@contextmanager +def mock_services_container( + services_container: Services, +) -> Generator[None, None, None]: + from core.service import container + + container._container_instance = services_container + yield + container._container_instance = None + + +@dataclass +class ServicesLoggingFixture: + logging_container: Logging + logging_mock: MagicMock @pytest.fixture -def mock_storage_fixture() -> MockStorageFixture: - return MockStorageFixture() +def services_logging_fixture() -> ServicesLoggingFixture: + logging_container = Logging() + logging_mock = create_autospec(setup_logging) + logging_container.logging.override(logging_mock) + return ServicesLoggingFixture(logging_container, logging_mock) -class MockServicesFixture: +@dataclass +class ServicesStorageFixture: + storage_container: Storage + s3_client_mock: MagicMock + analytics_mock: MagicMock + public_mock: MagicMock + + +@pytest.fixture +def services_storage_fixture() -> ServicesStorageFixture: + storage_container = Storage() + s3_client_mock = create_autospec(boto3.client) + analytics_mock = create_autospec(S3Service.factory) + public_mock = create_autospec(S3Service.factory) + storage_container.s3_client.override(s3_client_mock) + storage_container.analytics.override(analytics_mock) + storage_container.public.override(public_mock) + return ServicesStorageFixture( + storage_container, s3_client_mock, analytics_mock, public_mock + ) + + +@dataclass +class ServicesSearchFixture: + search_container: Search + client_mock: MagicMock + service_mock: MagicMock + revision_directory_mock: MagicMock + index_mock: MagicMock + + +@pytest.fixture +def services_search_fixture() -> ServicesSearchFixture: + search_container = Search() + client_mock = create_autospec(boto3.client) + service_mock = create_autospec(SearchServiceOpensearch1) + revision_directory_mock = create_autospec(SearchRevisionDirectory.create) + index_mock = create_autospec(ExternalSearchIndex) + search_container.client.override(client_mock) + search_container.service.override(service_mock) + search_container.revision_directory.override(revision_directory_mock) + search_container.index.override(index_mock) + return ServicesSearchFixture( + search_container, client_mock, service_mock, revision_directory_mock, index_mock + ) + + +@dataclass +class ServicesAnalyticsFixture: + analytics_container: AnalyticsContainer + analytics_mock: MagicMock + + +@pytest.fixture +def services_analytics_fixture() -> ServicesAnalyticsFixture: + analytics_container = AnalyticsContainer() + analytics_mock = create_autospec(Analytics) + analytics_container.analytics.override(analytics_mock) + return ServicesAnalyticsFixture(analytics_container, analytics_mock) + + +class ServicesFixture: """ - Provide a services container with all the services mocked out - by MagicMock objects. + Provide a real services container, with all services mocked out. """ - def __init__(self, storage: MockStorageFixture): + def __init__( + self, + logging: ServicesLoggingFixture, + storage: ServicesStorageFixture, + search: ServicesSearchFixture, + analytics: ServicesAnalyticsFixture, + ) -> None: + self.logging_fixture = logging + self.storage_fixture = storage + self.search_fixture = search + self.analytics_fixture = analytics + self.services = Services() - self.services.storage.override(storage.storage) - self.storage = storage + self.services.logging.override(logging.logging_container) + self.services.storage.override(storage.storage_container) + self.services.search.override(search.search_container) + self.services.analytics.override(analytics.analytics_container) + + +@pytest.fixture(autouse=True) +def services_fixture( + services_logging_fixture: ServicesLoggingFixture, + services_storage_fixture: ServicesStorageFixture, + services_search_fixture: ServicesSearchFixture, + services_analytics_fixture: ServicesAnalyticsFixture, +) -> Generator[ServicesFixture, None, None]: + fixture = ServicesFixture( + logging=services_logging_fixture, + storage=services_storage_fixture, + search=services_search_fixture, + analytics=services_analytics_fixture, + ) + with mock_services_container(fixture.services): + yield fixture @pytest.fixture -def mock_services_fixture( - mock_storage_fixture: MockStorageFixture, -) -> MockServicesFixture: - return MockServicesFixture(mock_storage_fixture) +def services_fixture_wired( + services_fixture: ServicesFixture, +) -> Generator[ServicesFixture, None, None]: + wire_container(services_fixture.services) + yield services_fixture + services_fixture.services.unwire() diff --git a/tests/migration/conftest.py b/tests/migration/conftest.py index c537d93b1a..85957ecd49 100644 --- a/tests/migration/conftest.py +++ b/tests/migration/conftest.py @@ -13,6 +13,7 @@ from core.model import json_serializer from tests.fixtures.database import ApplicationFixture, DatabaseFixture +from tests.fixtures.services import ServicesFixture if TYPE_CHECKING: from pytest_alembic import MigrationContext @@ -21,8 +22,15 @@ import alembic.config +pytest_plugins = [ + "tests.fixtures.services", +] + + @pytest.fixture(scope="function") -def application() -> Generator[ApplicationFixture, None, None]: +def application( + services_fixture: ServicesFixture, +) -> Generator[ApplicationFixture, None, None]: app = ApplicationFixture.create() yield app app.close() diff --git a/tests/migration/test_instance_init_script.py b/tests/migration/test_instance_init_script.py index 62a8f8b4a0..8c413c5487 100644 --- a/tests/migration/test_instance_init_script.py +++ b/tests/migration/test_instance_init_script.py @@ -2,7 +2,7 @@ import sys from io import StringIO from multiprocessing import Process -from unittest.mock import Mock +from unittest.mock import MagicMock, Mock from pytest_alembic import MigrationContext from sqlalchemy import inspect @@ -11,15 +11,19 @@ from core.model import SessionManager from scripts import InstanceInitializationScript from tests.fixtures.database import ApplicationFixture +from tests.fixtures.services import mock_services_container def _run_script() -> None: try: - # Run the script, capturing the log output - script = InstanceInitializationScript() + # Capturing the log output stream = StringIO() logging.basicConfig(stream=stream, level=logging.INFO, force=True) - script.run() + + mock_services = MagicMock() + with mock_services_container(mock_services): + script = InstanceInitializationScript() + script.run() # Set our exit code to the number of upgrades we ran sys.exit(stream.getvalue().count("Running upgrade")) diff --git a/tests/mocks/search.py b/tests/mocks/search.py index eebf8f3992..ffdb9e4870 100644 --- a/tests/mocks/search.py +++ b/tests/mocks/search.py @@ -9,7 +9,6 @@ from opensearchpy import OpenSearchException from core.external_search import ExternalSearchIndex -from core.model import Work from core.model.work import Work from core.search.revision import SearchSchemaRevision from core.search.revision_directory import SearchRevisionDirectory @@ -50,6 +49,10 @@ def __init__(self): self._indexes_created = [] self._document_submission_attempts = [] + @property + def base_revision_name(self) -> str: + return self.base_name + @property def document_submission_attempts(self) -> list[dict]: return self._document_submission_attempts @@ -227,14 +230,14 @@ class ExternalSearchIndexFake(ExternalSearchIndex): def __init__( self, - _db, - url: str | None = None, - test_search_term: str | None = None, revision_directory: SearchRevisionDirectory | None = None, version: int | None = None, ): + revision_directory = revision_directory or SearchRevisionDirectory.create() super().__init__( - _db, url, test_search_term, revision_directory, version, SearchServiceFake() + service=SearchServiceFake(), + revision_directory=revision_directory, + version=version, ) self._mock_multi_works: list[dict] = [] diff --git a/tox.ini b/tox.ini index b95fd8cbe7..f77003a43a 100644 --- a/tox.ini +++ b/tox.ini @@ -16,7 +16,7 @@ passenv = setenv = {api,core}: COVERAGE_FILE = .coverage.{envname} docker: SIMPLIFIED_TEST_DATABASE=postgresql://simplified_test:test@localhost:9005/simplified_circulation_test - docker: SIMPLIFIED_TEST_OPENSEARCH=http://localhost:9007 + docker: PALACE_TEST_SEARCH_URL=http://localhost:9007 core-docker: PALACE_TEST_MINIO_ENDPOINT_URL=http://localhost:9004 core-docker: PALACE_TEST_MINIO_USER=palace core-docker: PALACE_TEST_MINIO_PASSWORD=12345678901234567890