Skip to content

Commit

Permalink
Fix tests.
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathangreen committed Sep 19, 2023
1 parent 8755ee4 commit f08692e
Show file tree
Hide file tree
Showing 26 changed files with 127 additions and 85 deletions.
4 changes: 2 additions & 2 deletions api/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
SessionManager,
pg_advisory_lock,
)
from core.service.container import create_container
from core.service.container import container_instance
from core.util import LanguageCodes
from core.util.cache import CachedData
from scripts import InstanceInitializationScript
Expand Down Expand Up @@ -73,7 +73,7 @@ def initialize_circulation_manager():
pass
else:
if getattr(app, "manager", None) is None:
container = create_container()
container = container_instance()
try:
app.manager = CirculationManager(app._db, container)
except Exception:
Expand Down
3 changes: 2 additions & 1 deletion api/google_analytics_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from flask_babel import lazy_gettext as _

from core.model import ConfigurationSetting, ExternalIntegration, Session
from core.service.container import Services
from core.util.http import HTTP

from .config import CannotLoadConfiguration
Expand Down Expand Up @@ -63,7 +64,7 @@ class GoogleAnalyticsProvider:
{"key": TRACKING_ID, "label": _("Tracking ID"), "required": True},
]

def __init__(self, integration, library=None):
def __init__(self, integration, services: Services, library=None):
_db = Session.object_session(integration)
if not library:
raise CannotLoadConfiguration(
Expand Down
14 changes: 6 additions & 8 deletions api/s3_analytics_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@
import string
from typing import Dict, Optional

from dependency_injector.wiring import Provide, inject
from flask_babel import lazy_gettext as _
from sqlalchemy.orm import Session

from core.config import CannotLoadConfiguration
from core.local_analytics_provider import LocalAnalyticsProvider
from core.model import Library, LicensePool, MediaTypes
from core.model.configuration import ConfigurationGrouping
from core.service.storage.container import Storage
from core.service.container import Services
from core.service.storage.s3 import S3Service


Expand All @@ -30,15 +29,13 @@ class S3AnalyticsProvider(LocalAnalyticsProvider):
LocalAnalyticsProvider.SETTINGS + S3AnalyticsProviderConfiguration.to_settings()
)

@inject
def __init__(
self,
integration,
services: Services,
library=None,
s3_service: Optional[S3Service] = Provide[Storage.analytics],
):
self.s3_service = s3_service
super().__init__(integration, library)
super().__init__(integration, services, library)

@staticmethod
def _create_event_object(
Expand Down Expand Up @@ -252,12 +249,13 @@ def _get_storage(self) -> S3Service:
:return: StorageServiceBase object
"""
if not self.s3_service:
s3_storage_service = self.services.storage.analytics()
if s3_storage_service is None:
raise CannotLoadConfiguration(
"No storage service is configured with an analytics bucket."
)

return self.s3_service
return s3_storage_service


Provider = S3AnalyticsProvider
2 changes: 0 additions & 2 deletions bin/odl2_import_monitor
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ sys.path.append(os.path.abspath(package_dir))
from webpub_manifest_parser.odl import ODLFeedParserFactory

from api.odl2 import ODL2Importer, ODL2ImportMonitor

# NOTE: We need to import it explicitly to initialize MirrorUploader.IMPLEMENTATION_REGISTRY
from core.opds2_import import RWPMManifestParser
from core.scripts import RunCollectionMonitorScript

Expand Down
2 changes: 0 additions & 2 deletions bin/odl2_schema_validate
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ sys.path.append(os.path.abspath(package_dir))
from webpub_manifest_parser.odl import ODLFeedParserFactory

from api.odl2 import ODL2Importer

# NOTE: We need to import it explicitly to initialize MirrorUploader.IMPLEMENTATION_REGISTRY
from core.opds2_import import RWPMManifestParser
from core.opds_schema import ODL2SchemaValidation
from core.scripts import RunCollectionMonitorScript
Expand Down
2 changes: 0 additions & 2 deletions bin/odl_import_monitor
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ package_dir = os.path.join(bin_dir, "..")
sys.path.append(os.path.abspath(package_dir))

from api.odl import ODLImporter, ODLImportMonitor

# NOTE: We need to import it explicitly to initialize MirrorUploader.IMPLEMENTATION_REGISTRY
from core.scripts import RunCollectionMonitorScript

RunCollectionMonitorScript(
Expand Down
1 change: 0 additions & 1 deletion bin/opds2_import_monitor
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ sys.path.append(os.path.abspath(package_dir))

from webpub_manifest_parser.opds2 import OPDS2FeedParserFactory

# NOTE: We need to import it explicitly to initialize MirrorUploader.IMPLEMENTATION_REGISTRY
from core.model import ExternalIntegration
from core.opds2_import import OPDS2Importer, OPDS2ImportMonitor, RWPMManifestParser
from core.scripts import OPDSImportScript
Expand Down
1 change: 0 additions & 1 deletion bin/opds2_schema_validate
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ sys.path.append(os.path.abspath(package_dir))

from webpub_manifest_parser.opds2 import OPDS2FeedParserFactory

# NOTE: We need to import it explicitly to initialize MirrorUploader.IMPLEMENTATION_REGISTRY
from core.model.configuration import ExternalIntegration
from core.opds2_import import OPDS2Importer, RWPMManifestParser
from core.opds_schema import OPDS2SchemaValidation
Expand Down
1 change: 0 additions & 1 deletion bin/opds_import_monitor
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ bin_dir = os.path.split(__file__)[0]
package_dir = os.path.join(bin_dir, "..")
sys.path.append(os.path.abspath(package_dir))

# NOTE: We need to import it explicitly to initialize MirrorUploader.IMPLEMENTATION_REGISTRY
from core.scripts import OPDSImportScript

OPDSImportScript().run()
14 changes: 9 additions & 5 deletions core/analytics.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from __future__ import annotations

import importlib
import logging
from collections import defaultdict
Expand All @@ -7,6 +9,7 @@

from .config import CannotLoadConfiguration
from .model import ExternalIntegration
from .service.container import container_instance
from .util.datetime_helpers import utc_now
from .util.log import log_elapsed_time

Expand All @@ -26,7 +29,7 @@ class Analytics:
GLOBAL_ENABLED: Optional[bool] = None
LIBRARY_ENABLED: Set[int] = set()

def __new__(cls, _db, refresh=False) -> "Analytics":
def __new__(cls, _db: Session, refresh: bool = False) -> Analytics:
instance = cls._singleton_instance
if instance is None:
refresh = True
Expand All @@ -44,15 +47,16 @@ def _reset_singleton_instance(cls):
cls._singleton_instance = None

@log_elapsed_time(log_method=log.debug, message_prefix="Initializing instance")
def _initialize_instance(self, _db):
def _initialize_instance(self, _db: Session) -> None:
"""Initialize an instance (usually the singleton) of the class.
We don't use __init__ because it would be run whether or not
a new instance were instantiated.
"""
services = container_instance()
sitewide_providers = []
library_providers = defaultdict(list)
initialization_exceptions: Dict[int, Exception] = {}
initialization_exceptions: Dict[int, Exception | str] = {}
global_enabled = False
library_enabled = set()
# Find a list of all the ExternalIntegrations set up with a
Expand All @@ -68,12 +72,12 @@ def _initialize_instance(self, _db):
provider_class = self._provider_class_from_module(module)
if provider_class:
if not libraries:
provider = provider_class(integration)
provider = provider_class(integration, services)
sitewide_providers.append(provider)
global_enabled = True
else:
for library in libraries:
provider = provider_class(integration, library)
provider = provider_class(integration, services, library)
library_providers[library.id].append(provider)
library_enabled.add(library.id)
else:
Expand Down
4 changes: 3 additions & 1 deletion core/local_analytics_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from sqlalchemy.orm.session import Session

from .model import CirculationEvent, ExternalIntegration, create, get_one
from .service.container import Services


class LocalAnalyticsProvider:
Expand Down Expand Up @@ -41,12 +42,13 @@ class LocalAnalyticsProvider:
},
]

def __init__(self, integration, library=None):
def __init__(self, integration, services: Services, library=None):
self.integration_id = integration.id
self.location_source = (
integration.setting(self.LOCATION_SOURCE).value
or self.LOCATION_SOURCE_DISABLED
)
self.services = services
if library:
self.library_id = library.id
else:
Expand Down
1 change: 1 addition & 0 deletions core/marc.py
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,7 @@ def records(
if not is_new:
cached.representation = representation
cached.end_time = end_time
representation.set_as_mirrored(upload.url)
else:
representation.mirror_exception = str(upload.exception)

Expand Down
2 changes: 1 addition & 1 deletion core/mock_analytics_provider.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
class MockAnalyticsProvider:
"""A mock analytics provider that keeps track of how many times it's called."""

def __init__(self, integration=None, library=None):
def __init__(self, integration=None, services=None, library=None):
self.count = 0
self.event = None
if integration:
Expand Down
4 changes: 2 additions & 2 deletions core/scripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
from .monitor import CollectionMonitor, ReaperMonitor
from .opds_import import OPDSImporter, OPDSImportMonitor
from .overdrive import OverdriveCoreAPI
from .service.container import Services, create_container
from .service.container import Services, container_instance
from .util import fast_query_count
from .util.datetime_helpers import strptime_utc, utc_now
from .util.personal_names import contributor_name_match_ratio, display_name_to_sort_name
Expand Down Expand Up @@ -125,7 +125,7 @@ def __init__(self, _db=None, services: Optional[Services] = None, *args, **kwarg
self._session = _db

if services is None:
services = create_container()
services = container_instance()
self._services = services

def run(self):
Expand Down
16 changes: 16 additions & 0 deletions core/service/container.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,19 @@ def create_container() -> Services:
container = Services()
container.config.from_dict({"storage": StorageConfiguration().dict()})
return container


_container_instance = None


def container_instance() -> Services:
# Create a singleton container instance, I'd like this to be used sparingly
# and eventually have it go away, but there are places in the code that
# are currently difficult to refactor to pass the container into the
# constructor.
# If at all possible please use the container that is stored in the CirculationManager
# or Scripts classes instead of using this function.
global _container_instance
if _container_instance is None:
_container_instance = create_container()
return _container_instance
8 changes: 4 additions & 4 deletions core/service/storage/configuration.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from typing import Optional

import boto3
from pydantic import HttpUrl, parse_obj_as, validator
from pydantic import AnyHttpUrl, parse_obj_as, validator

from core.service.configuration import ServiceConfiguration

Expand All @@ -14,10 +14,10 @@ class StorageConfiguration(ServiceConfiguration):
public_access_bucket: Optional[str] = None
analytics_bucket: Optional[str] = None

endpoint_url: Optional[HttpUrl] = None
endpoint_url: Optional[AnyHttpUrl] = None

url_template: HttpUrl = parse_obj_as(
HttpUrl, "https://{bucket}.s3.{region}.amazonaws.com/{key}"
url_template: AnyHttpUrl = parse_obj_as(
AnyHttpUrl, "https://{bucket}.s3.{region}.amazonaws.com/{key}"
)

@validator("region")
Expand Down
12 changes: 1 addition & 11 deletions core/service/storage/container.py
Original file line number Diff line number Diff line change
@@ -1,21 +1,11 @@
import boto3
from dependency_injector import providers
from dependency_injector.containers import DeclarativeContainer, WiringConfiguration
from dependency_injector.containers import DeclarativeContainer

from core.service.storage.s3 import S3Service


class Storage(DeclarativeContainer):

# See https://python-dependency-injector.ets-labs.org/wiring.html
# This lists modules that contain markers that will be used to wire up
# dependencies from this container automatically.
wiring_config = WiringConfiguration(
modules=[
"api.s3_analytics_provider",
],
)

config = providers.Configuration()

s3_client = providers.Singleton(
Expand Down
3 changes: 2 additions & 1 deletion scripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from datetime import timedelta
from pathlib import Path
from typing import Optional
from unittest.mock import MagicMock

from sqlalchemy import inspect
from sqlalchemy.engine import Connection
Expand Down Expand Up @@ -193,7 +194,7 @@ def __init__(self, _db=None, cmd_args=None, manager=None, *args, **kwargs):
super().__init__(_db, *args, **kwargs)
self.parse_args(cmd_args)
if not manager:
manager = CirculationManager(self._db)
manager = CirculationManager(self._db, MagicMock())
from api.app import app

app.manager = manager
Expand Down
23 changes: 0 additions & 23 deletions tests/api/admin/test_routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -680,29 +680,6 @@ def test_process_search_service_self_tests(self, fixture: AdminRouteFixture):
fixture.assert_supported_methods(url, "GET", "POST")


class TestAdminStorageServices:
CONTROLLER_NAME = "admin_storage_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/storage_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/storage_service/<service_id>"
fixture.assert_authenticated_request_calls(
url, fixture.controller.process_delete, "<service_id>", http_method="DELETE" # type: ignore
)
fixture.assert_supported_methods(url, "DELETE")


class TestAdminCatalogServices:
CONTROLLER_NAME = "admin_catalog_services_controller"

Expand Down
Loading

0 comments on commit f08692e

Please sign in to comment.