From 38efb650b417fa82057705229424ea965a8034b0 Mon Sep 17 00:00:00 2001 From: Alex Lowe Date: Fri, 20 Oct 2023 10:12:36 -0400 Subject: [PATCH] feat: lazy-loading services (don't need a project on launch) --- craft_application/services/__init__.py | 3 +- craft_application/services/base.py | 53 +++++++++++++++++-- craft_application/services/lifecycle.py | 6 +-- craft_application/services/package.py | 2 +- craft_application/services/provider.py | 6 +-- craft_application/services/service_factory.py | 37 +++++++------ tests/conftest.py | 12 +++-- tests/integration/conftest.py | 2 +- tests/integration/services/test_lifecycle.py | 2 +- tests/unit/conftest.py | 2 +- tests/unit/services/test_base.py | 49 +++++++++++++++++ tests/unit/services/test_lifecycle.py | 6 +-- tests/unit/services/test_package.py | 2 +- tests/unit/services/test_provider.py | 2 +- tests/unit/services/test_service_factory.py | 2 +- 15 files changed, 145 insertions(+), 41 deletions(-) create mode 100644 tests/unit/services/test_base.py diff --git a/craft_application/services/__init__.py b/craft_application/services/__init__.py index 961320ac..f7c2b016 100644 --- a/craft_application/services/__init__.py +++ b/craft_application/services/__init__.py @@ -15,7 +15,7 @@ # along with this program. If not, see . """Service classes for the business logic of various categories of command.""" -from craft_application.services.base import BaseService +from craft_application.services.base import BaseService, EagerService from craft_application.services.lifecycle import LifecycleService from craft_application.services.package import PackageService from craft_application.services.provider import ProviderService @@ -23,6 +23,7 @@ __all__ = [ "BaseService", + "EagerService", "LifecycleService", "PackageService", "ProviderService", diff --git a/craft_application/services/base.py b/craft_application/services/base.py index b86cf57b..6074b8b6 100644 --- a/craft_application/services/base.py +++ b/craft_application/services/base.py @@ -21,23 +21,68 @@ from craft_cli import emit +from craft_application import errors, models + if typing.TYPE_CHECKING: - from craft_application import models from craft_application.application import AppMetadata from craft_application.services import ServiceFactory # ignoring the fact that this abstract class has no abstract methods. class BaseService(metaclass=abc.ABCMeta): # noqa: B024 - """A service containing the actual business logic of one or more commands.""" + """Abstract base class for a service. Should not be used directly. + + BaseService allows lazy-loading of a project. EagerService should be used + for services that always require a project on load. + """ def __init__( - self, app: AppMetadata, project: models.Project, services: ServiceFactory + self, + app: AppMetadata, + services: ServiceFactory, + *, + project_getter: typing.Callable[[], models.Project], ) -> None: self._app = app - self._project = project self._services = services + self._project: models.Project | None = None + self._project_getter = project_getter def setup(self) -> None: """Application-specific service preparation.""" emit.debug("setting up service") + + def _get_project(self) -> models.Project: + """Get the project for this app run. + + returns: a project model + raises: CraftError if the project cannot be loaded. + + Caches the project in self._project otherwise. + """ + if self._project is None: + try: + self._project = self._project_getter() + # Intentionally catching a broad exception to provide a more useful error. + except Exception as exc: # noqa: BLE001 + raise errors.CraftError( + "Project could not be loaded.", details=str(exc) + ) from exc + return self._project + + +# ignoring the fact that this abstract class has no abstract methods. +class EagerService(BaseService, metaclass=abc.ABCMeta): + """A service that requires the project from startup.""" + + _project: models.Project + + def __init__( + self, + app: AppMetadata, + services: ServiceFactory, + *, + project: models.Project, + ) -> None: + super().__init__(app, services, project_getter=lambda: project) + self._project = project diff --git a/craft_application/services/lifecycle.py b/craft_application/services/lifecycle.py index 82049036..58bf0ce0 100644 --- a/craft_application/services/lifecycle.py +++ b/craft_application/services/lifecycle.py @@ -101,7 +101,7 @@ def _get_step(step_name: str) -> Step: raise RuntimeError(f"Invalid target step {step_name!r}") from None -class LifecycleService(base.BaseService): +class LifecycleService(base.EagerService): """Create and manage the parts lifecycle. :param app: An AppMetadata object containing metadata about the application. @@ -116,15 +116,15 @@ class LifecycleService(base.BaseService): def __init__( # noqa: PLR0913 (too many arguments) self, app: AppMetadata, - project: Project, services: ServiceFactory, *, + project: Project, work_dir: Path | str, cache_dir: Path | str, build_for: str, **lifecycle_kwargs: Any, # noqa: ANN401 - eventually used in an Any ) -> None: - super().__init__(app, project, services) + super().__init__(app, services, project=project) self._work_dir = work_dir self._cache_dir = cache_dir self._build_for = build_for diff --git a/craft_application/services/package.py b/craft_application/services/package.py index 5eebf807..30355153 100644 --- a/craft_application/services/package.py +++ b/craft_application/services/package.py @@ -27,7 +27,7 @@ from craft_application import models -class PackageService(base.BaseService): +class PackageService(base.EagerService): """Business logic for creating packages.""" @abc.abstractmethod diff --git a/craft_application/services/provider.py b/craft_application/services/provider.py index b47e5061..255f0df2 100644 --- a/craft_application/services/provider.py +++ b/craft_application/services/provider.py @@ -42,7 +42,7 @@ from craft_application.services import ServiceFactory -class ProviderService(base.BaseService): +class ProviderService(base.EagerService): """Manager for craft_providers in an application. :param app: Metadata about this application. @@ -55,13 +55,13 @@ class ProviderService(base.BaseService): def __init__( # noqa: PLR0913 (too many arguments) self, app: AppMetadata, - project: models.Project, services: ServiceFactory, *, + project: models.Project, work_dir: pathlib.Path, install_snap: bool = True, ) -> None: - super().__init__(app, project, services) + super().__init__(app, services, project=project) self._provider: craft_providers.Provider | None = None self._work_dir = work_dir self.snaps: list[Snap] = [] diff --git a/craft_application/services/service_factory.py b/craft_application/services/service_factory.py index 6b3df492..fa0a19ba 100644 --- a/craft_application/services/service_factory.py +++ b/craft_application/services/service_factory.py @@ -71,24 +71,31 @@ def __getattr__(self, service: str) -> services.BaseService: instantiated service as an instance attribute, allowing the same service instance to be reused for the entire run of the application. """ - if self.project is None: # pyright: ignore[reportUnnecessaryComparison] - raise errors.ApplicationError( - "ServiceFactory requires a project to be set before getting a service.", - app_name=self.app.name, - docs_url="https://github.com/canonical/craft-application/pull/40#discussion_r1253593262", - ) service_cls_name = "".join(word.title() for word in service.split("_")) service_cls_name += "Class" classes = dataclasses.asdict(self) if service_cls_name not in classes: raise AttributeError(service) cls = getattr(self, service_cls_name) - if issubclass(cls, services.BaseService): - kwargs = self._service_kwargs.get(service, {}) - instance = cls(self.app, self.project, self, **kwargs) - instance.setup() - setattr(self, service, instance) - # Mypy and pyright interpret this differently. - # Pyright - return instance # type: ignore[no-any-return] - raise TypeError(f"{cls.__name__} is not a service class") + if not issubclass(cls, services.BaseService): + raise TypeError(f"{cls.__name__} is not a service class") + + kwargs = self._service_kwargs.get(service, {}) + + if issubclass(cls, services.EagerService): + # Eager services require the project on load. + if self.project is None: # pyright: ignore[reportUnnecessaryComparison] + raise errors.ApplicationError( + f"{cls.__name__} requires a project to be set before loading.", + app_name=self.app.name, + docs_url="https://github.com/canonical/craft-application/pull/40#discussion_r1253593262", + ) + kwargs.setdefault("project", self.project) + else: + kwargs.setdefault("project_getter", lambda: self.project) + + instance = cls(self.app, self, **kwargs) + instance.setup() + setattr(self, service, instance) + # Mypy and pyright interpret this differently. + return instance # type: ignore[no-any-return] diff --git a/tests/conftest.py b/tests/conftest.py index 4256c4fd..d593e566 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -103,8 +103,8 @@ def lifecycle_service( service = services.LifecycleService( app_metadata, - fake_project, fake_services, + project=fake_project, work_dir=work_dir, cache_dir=cache_dir, platform=None, @@ -128,10 +128,11 @@ class FakeProviderService(services.ProviderService): def __init__( self, app: application.AppMetadata, - project: models.Project, services: services.ServiceFactory, + *, + project: models.Project, ): - super().__init__(app, project, services, work_dir=pathlib.Path()) + super().__init__(app, services, project=project, work_dir=pathlib.Path()) return FakeProviderService @@ -160,14 +161,15 @@ class FakeLifecycleService(services.LifecycleService): def __init__( self, app: application.AppMetadata, - project: models.Project, services: services.ServiceFactory, + *, + project: models.Project, **lifecycle_kwargs: Any, ): super().__init__( app, - project, services, + project=project, work_dir=tmp_path / "work", cache_dir=tmp_path / "cache", platform=None, diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 6d657c7b..af164468 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -45,8 +45,8 @@ def provider_service(app_metadata, fake_project, fake_services): """Provider service with install snap disabled for integration tests""" return provider.ProviderService( app_metadata, - fake_project, fake_services, + project=fake_project, work_dir=pathlib.Path(), install_snap=False, ) diff --git a/tests/integration/services/test_lifecycle.py b/tests/integration/services/test_lifecycle.py index a6e8cc6d..7d730de3 100644 --- a/tests/integration/services/test_lifecycle.py +++ b/tests/integration/services/test_lifecycle.py @@ -35,8 +35,8 @@ def parts_lifecycle(app_metadata, fake_project, fake_services, tmp_path, request service = LifecycleService( app_metadata, - fake_project, fake_services, + project=fake_project, work_dir=tmp_path / "work", cache_dir=tmp_path / "cache", platform=None, diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index b685a8e8..8844c5a0 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -25,7 +25,7 @@ @pytest.fixture() def provider_service(app_metadata, fake_project, fake_services, tmp_path): return services.ProviderService( - app_metadata, fake_project, fake_services, work_dir=tmp_path + app_metadata, fake_services, project=fake_project, work_dir=tmp_path ) diff --git a/tests/unit/services/test_base.py b/tests/unit/services/test_base.py new file mode 100644 index 00000000..58cab6d4 --- /dev/null +++ b/tests/unit/services/test_base.py @@ -0,0 +1,49 @@ +# This file is part of craft-application. +# +# Copyright 2023 Canonical Ltd. +# +# This program is free software: you can redistribute it and/or modify it +# under the terms of the GNU Lesser General Public License version 3, as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranties of MERCHANTABILITY, +# SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR PURPOSE. +# See the GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License +# along with this program. If not, see . +"""Unit tests for base service class.""" +import pytest +from craft_application import errors, services + + +class FakeLazyService(services.BaseService): + pass + + +def test_lazy_service_load_success(app_metadata, fake_services, fake_project): + def project_getter(): + return fake_project + + service = FakeLazyService( + app_metadata, fake_services, project_getter=project_getter + ) + + assert service._project is None + assert service._get_project() == fake_project + assert service._project == fake_project + + +def test_lazy_service_load_failure(app_metadata, fake_services): + def project_getter(): + raise ValueError("No service for you!") + + service = FakeLazyService( + app_metadata, fake_services, project_getter=project_getter + ) + + assert service._project is None + with pytest.raises(errors.CraftError, match=r"^Project could not be loaded.$"): + service._get_project() + assert service._project is None diff --git a/tests/unit/services/test_lifecycle.py b/tests/unit/services/test_lifecycle.py index 25f512a5..13ad42a5 100644 --- a/tests/unit/services/test_lifecycle.py +++ b/tests/unit/services/test_lifecycle.py @@ -48,8 +48,8 @@ def fake_parts_lifecycle(app_metadata, fake_project, fake_services, tmp_path): build_for = util.get_host_architecture() fake_service = FakePartsLifecycle( app_metadata, - fake_project, fake_services, + project=fake_project, work_dir=work_dir, cache_dir=cache_dir, build_for=build_for, @@ -213,8 +213,8 @@ def test_get_step_failure(step_name): def test_init_success(app_metadata, fake_project, fake_services, tmp_path): service = lifecycle.LifecycleService( app_metadata, - fake_project, fake_services, + project=fake_project, work_dir=tmp_path, cache_dir=tmp_path, platform=None, @@ -246,8 +246,8 @@ def test_init_parts_error( service = lifecycle.LifecycleService( app_metadata, - fake_project, fake_services, + project=fake_project, work_dir=tmp_path, cache_dir=tmp_path, platform=None, diff --git a/tests/unit/services/test_package.py b/tests/unit/services/test_package.py index a0f2ea26..96cb7416 100644 --- a/tests/unit/services/test_package.py +++ b/tests/unit/services/test_package.py @@ -36,7 +36,7 @@ def metadata(self) -> models.BaseMetadata: def test_write_metadata(tmp_path, app_metadata, fake_project, fake_services): - service = FakePackageService(app_metadata, fake_project, fake_services) + service = FakePackageService(app_metadata, fake_services, project=fake_project) metadata_file = tmp_path / "metadata.yaml" assert not metadata_file.exists() diff --git a/tests/unit/services/test_provider.py b/tests/unit/services/test_provider.py index 5ad877ec..e3d61203 100644 --- a/tests/unit/services/test_provider.py +++ b/tests/unit/services/test_provider.py @@ -33,8 +33,8 @@ def test_install_snap(app_metadata, fake_project, fake_services, install_snap, snaps): service = provider.ProviderService( app_metadata, - fake_project, fake_services, + project=fake_project, work_dir=pathlib.Path(), install_snap=install_snap, ) diff --git a/tests/unit/services/test_service_factory.py b/tests/unit/services/test_service_factory.py index 5cc53e01..7f6081bb 100644 --- a/tests/unit/services/test_service_factory.py +++ b/tests/unit/services/test_service_factory.py @@ -79,7 +79,7 @@ def __new__(cls, *args, **kwargs): check.equal(factory.package, MockPackageService.mock_class.return_value) with check: MockPackageService.mock_class.assert_called_once_with( - app_metadata, fake_project, factory, **kwargs + app_metadata, factory, project=fake_project, **kwargs )