diff --git a/reme/config/config_parser.py b/reme/config/config_parser.py index e70ad805..16cf8a6f 100644 --- a/reme/config/config_parser.py +++ b/reme/config/config_parser.py @@ -4,13 +4,13 @@ import json import os import re from collections.abc import Mapping -from importlib import metadata +from importlib.metadata import EntryPoint from pathlib import Path from typing import Any import yaml -from ..entry_point import load_entry_point +from ..entry_point import find_entry_points, load_entry_point, unique_entry_point # Config files are looked up relative to this module's directory _CONFIG_DIR = Path(__file__).parent @@ -127,20 +127,11 @@ def _convert_value(value_str: str) -> Any: return s -def _external_config_entries(name: str) -> list[metadata.EntryPoint]: - """Find installed config providers without importing their packages.""" - return list(metadata.entry_points().select(group=_CONFIG_ENTRY_POINT_GROUP, name=name)) - - -def _external_config_path(name: str, entries: list[metadata.EntryPoint] | None = None) -> Path | None: +def _external_config_path(name: str, entry: EntryPoint | None) -> Path | None: """Resolve an installed plugin config exposed through ``reme.configs``.""" - entries = _external_config_entries(name) if entries is None else entries - if not entries: + if entry is None: return None - if len(entries) > 1: - providers = ", ".join(sorted(entry.value for entry in entries)) - raise ValueError(f"Config '{name}' has multiple installed providers: {providers}") - value = load_entry_point(entries[0], invoke=True) + value = load_entry_point(entry, invoke=True) path = Path(value) if path.suffix not in _SUPPORTED_EXTS or not path.is_file(): raise ValueError(f"Config entry point '{name}' did not resolve to a YAML or JSON file") @@ -154,13 +145,14 @@ def _load_config(name_or_path: str, encoding: str = "utf-8", _stack: tuple[str, raise ValueError(f"Circular config inheritance: {chain}") built_in = _CONFIG_REGISTRY.get(name_or_path) - external_entries = _external_config_entries(name_or_path) + external_entries = find_entry_points(_CONFIG_ENTRY_POINT_GROUP, name_or_path) if built_in is not None and external_entries: raise ValueError(f"Config '{name_or_path}' is provided by both ReMe and an installed distribution") if built_in is not None: return _load_config_path(built_in, name_or_path, encoding, _stack) - external = _external_config_path(name_or_path, external_entries) + external_entry = unique_entry_point(external_entries, name_or_path, provider="Config") + external = _external_config_path(name_or_path, external_entry) if external is not None: return _load_config_path(external, name_or_path, encoding, _stack) diff --git a/reme/entry_point.py b/reme/entry_point.py index 298b2793..fb62b345 100644 --- a/reme/entry_point.py +++ b/reme/entry_point.py @@ -4,6 +4,24 @@ from importlib import metadata from typing import Any +def find_entry_points(group: str, name: str) -> list[metadata.EntryPoint]: + """Return all matching entry points without importing their providers.""" + return list(metadata.entry_points().select(group=group, name=name)) + + +def unique_entry_point( + entries: list[metadata.EntryPoint], + name: str, + *, + provider: str, +) -> metadata.EntryPoint | None: + """Return the sole matching entry point, rejecting ambiguous providers.""" + if len(entries) > 1: + values = ", ".join(sorted(entry.value for entry in entries)) + raise ValueError(f"{provider} '{name}' has multiple installed providers: {values}") + return entries[0] if entries else None + + def load_entry_point(entry: metadata.EntryPoint, *, invoke: bool = False) -> Any: """Load and optionally invoke an entry point without retaining registrations.""" # Import lazily so config parser imports do not pull in the component graph. diff --git a/reme/plugin.py b/reme/plugin.py index 78a88186..43c69950 100644 --- a/reme/plugin.py +++ b/reme/plugin.py @@ -4,13 +4,12 @@ from __future__ import annotations from collections.abc import Iterable, Mapping from dataclasses import dataclass, field -from importlib import metadata from typing import Any from .components.base_component import ComponentMixin from .components.component_registry import ComponentRegistry from .config import deep_merge_config, expand_env_vars -from .entry_point import load_entry_point +from .entry_point import find_entry_points, load_entry_point, unique_entry_point PLUGIN_ENTRY_POINT_GROUP = "reme.plugins" @@ -32,11 +31,6 @@ class Plugin: config: Mapping[str, Any] = field(default_factory=dict) -def _entry_points(group: str, name: str) -> list[metadata.EntryPoint]: - """Return matching entry points for one group and name.""" - return list(metadata.entry_points().select(group=group, name=name)) - - class PluginManager: """Resolve enabled plugins and apply their contributions to one application.""" @@ -55,13 +49,11 @@ class PluginManager: raise ValueError("Plugin name cannot be empty") if name in seen: raise ValueError(f"Plugin '{name}' is enabled more than once") - entries = _entry_points(PLUGIN_ENTRY_POINT_GROUP, name) - if not entries: + entries = find_entry_points(PLUGIN_ENTRY_POINT_GROUP, name) + entry = unique_entry_point(entries, name, provider="Plugin") + if entry is None: raise ValueError(f"Plugin '{name}' is not installed") - if len(entries) > 1: - providers = ", ".join(sorted(entry.value for entry in entries)) - raise ValueError(f"Plugin '{name}' has multiple installed providers: {providers}") - plugin = load_entry_point(entries[0], invoke=True) + plugin = load_entry_point(entry, invoke=True) if not isinstance(plugin, Plugin): raise TypeError(f"Plugin entry point '{name}' did not return reme.plugin.Plugin") if plugin.name != name: diff --git a/tests/unit/test_config_parser.py b/tests/unit/test_config_parser.py index ecf2b68a..67ed2de0 100644 --- a/tests/unit/test_config_parser.py +++ b/tests/unit/test_config_parser.py @@ -21,7 +21,8 @@ def test_load_builtin_config_by_filename_with_suffix(): assert cfg["service"]["backend"] == "http" -def test_builtin_and_external_config_name_collision_fails(monkeypatch): +@pytest.mark.parametrize("provider_count", [1, 2]) +def test_builtin_and_external_config_name_collision_fails(monkeypatch, provider_count): """An installed config cannot be silently shadowed by a built-in name.""" class FakeEntryPoint: @@ -44,8 +45,8 @@ def test_builtin_and_external_config_name_collision_fails(monkeypatch): return [entry for entry in self if entry.name == name] monkeypatch.setattr( - "reme.config.config_parser.metadata.entry_points", - lambda: FakeEntryPoints([FakeEntryPoint()]), + "reme.entry_point.metadata.entry_points", + lambda: FakeEntryPoints([FakeEntryPoint() for _ in range(provider_count)]), ) with pytest.raises(ValueError, match="provided by both ReMe and an installed distribution"): diff --git a/tests/unit/test_plugin.py b/tests/unit/test_plugin.py index 9a3845bd..d5854682 100644 --- a/tests/unit/test_plugin.py +++ b/tests/unit/test_plugin.py @@ -2,7 +2,6 @@ # pylint: disable=missing-class-docstring,missing-function-docstring -from importlib.metadata import EntryPoint from pathlib import Path import pytest @@ -23,8 +22,35 @@ class _PluginComponent(BaseComponent): component_type = "example.reranker" +class _FakeEntryPoint: + def __init__(self, name, value, loader, group): + self.name = name + self.value = value + self._loader = loader + self.group = group + + def load(self): + return self._loader() + + +class _FakeEntryPoints(list): + def select(self, *, group, name): + return [entry for entry in self if entry.group == group and entry.name == name] + + +def _set_entry_points(monkeypatch, *entries): + monkeypatch.setattr("reme.entry_point.metadata.entry_points", lambda: _FakeEntryPoints(entries)) + + def test_plugin_defaults_are_below_application_config(): - manager = PluginManager([Plugin(name="example", config={"jobs": {"task": {"backend": "base", "value": 1}}})]) + manager = PluginManager( + [ + Plugin( + name="example", + config={"jobs": {"task": {"backend": "base", "value": 1}}}, + ), + ], + ) merged = manager.merge_config({"jobs": {"task": {"value": 2}}}) @@ -102,48 +128,45 @@ def test_plugin_backend_collision_fails_with_both_owners(): def test_plugin_manager_loads_explicit_entry_point(monkeypatch): descriptor = Plugin(name="example", backends=(Backend("example_step", _PluginStep),)) - - class FakeEntryPoint: - name = "example" - value = "example:plugin" - - @staticmethod - def load(): - return descriptor - - class FakeEntryPoints(list): - def select(self, *, group, name): - assert group == "reme.plugins" - return [entry for entry in self if entry.name == name] - - monkeypatch.setattr("reme.plugin.metadata.entry_points", lambda: FakeEntryPoints([FakeEntryPoint()])) + _set_entry_points( + monkeypatch, + _FakeEntryPoint("example", "example:plugin", lambda: descriptor, "reme.plugins"), + ) manager = PluginManager.discover(["example"]) assert manager.plugins == (descriptor,) +def test_plugin_manager_rejects_multiple_entry_point_providers(monkeypatch): + descriptor = Plugin(name="example") + _set_entry_points( + monkeypatch, + _FakeEntryPoint("example", "first:plugin", lambda: descriptor, "reme.plugins"), + _FakeEntryPoint("example", "second:plugin", lambda: descriptor, "reme.plugins"), + ) + + with pytest.raises( + ValueError, + match="Plugin 'example' has multiple installed providers: first:plugin, second:plugin", + ): + PluginManager.discover(["example"]) + + def test_plugin_entry_point_import_side_effect_does_not_leak(monkeypatch, tmp_path): class UndeclaredClient(ComponentMixin): component_type = ComponentEnum.CLIENT descriptor = Plugin(name="example") - class FakeEntryPoint: - name = "example" - value = "example:plugin" + def load_plugin(): + R.register(UndeclaredClient, "undeclared-client") + return descriptor - @staticmethod - def load(): - R.register(UndeclaredClient, "undeclared-client") - return descriptor - - class FakeEntryPoints(list): - def select(self, *, group, name): - assert group == "reme.plugins" - return [entry for entry in self if entry.name == name] - - monkeypatch.setattr("reme.plugin.metadata.entry_points", lambda: FakeEntryPoints([FakeEntryPoint()])) + _set_entry_points( + monkeypatch, + _FakeEntryPoint("example", "example:plugin", load_plugin, "reme.plugins"), + ) app = Application( plugins=["example"], @@ -175,24 +198,9 @@ def test_config_can_extend_another_config(tmp_path: Path): def test_config_can_come_from_installed_entry_point(tmp_path: Path, monkeypatch): config = tmp_path / "example.yaml" config.write_text("plugins: [example]\n", encoding="utf-8") - entry = EntryPoint(name="example", value="pathlib:Path", group="reme.configs") - - class LoadedEntryPoint: - name = entry.name - value = entry.value - - @staticmethod - def load(): - return config - - class FakeEntryPoints(list): - def select(self, *, group, name): - assert group == "reme.configs" - return [item for item in self if item.name == name] - - monkeypatch.setattr( - "reme.config.config_parser.metadata.entry_points", - lambda: FakeEntryPoints([LoadedEntryPoint()]), + _set_entry_points( + monkeypatch, + _FakeEntryPoint("example", "example:CONFIG_PATH", lambda: config, "reme.configs"), ) assert _load_config("example") == {"plugins": ["example"]} @@ -205,23 +213,13 @@ def test_config_entry_point_import_side_effect_does_not_leak(tmp_path: Path, mon class UndeclaredClient(ComponentMixin): component_type = ComponentEnum.CLIENT - class LoadedEntryPoint: - name = "side-effect" - value = "example:CONFIG_PATH" + def load_config(): + R.register(UndeclaredClient, "config-side-effect-client") + return config - @staticmethod - def load(): - R.register(UndeclaredClient, "config-side-effect-client") - return config - - class FakeEntryPoints(list): - def select(self, *, group, name): - assert group == "reme.configs" - return [item for item in self if item.name == name] - - monkeypatch.setattr( - "reme.config.config_parser.metadata.entry_points", - lambda: FakeEntryPoints([LoadedEntryPoint()]), + _set_entry_points( + monkeypatch, + _FakeEntryPoint("side-effect", "example:CONFIG_PATH", load_config, "reme.configs"), ) loaded = _load_config("side-effect") diff --git a/tests/unit/test_reme_cli.py b/tests/unit/test_reme_cli.py index f4dfbda0..7430aafc 100644 --- a/tests/unit/test_reme_cli.py +++ b/tests/unit/test_reme_cli.py @@ -16,6 +16,37 @@ from reme.enumeration import ComponentEnum from reme.plugin import Backend, Plugin, PluginManager +def _recording_client(seen, output="ok", base=object): + """Build an async client stub that records construction and calls.""" + + class RecordingClient(base): + """Async client stub backed by a shared call record.""" + + def __init__(self, **kwargs): + seen["client_kwargs"] = kwargs + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc_val, exc_tb): + return None + + async def __call__(self, action: str, **kwargs): + seen["action"] = action + seen["payload"] = kwargs + yield output + + return RecordingClient + + +def _set_client_backend(monkeypatch, client_cls): + monkeypatch.setattr( + reme_module, + "create_application_registry", + lambda: SimpleNamespace(get=lambda component_type, backend: client_cls), + ) + + def test_package_import_does_not_load_optional_core_dependencies(): """The base package leaves optional core dependencies unloaded.""" script = """ @@ -242,29 +273,7 @@ def test_cli_service_exits_nonzero_on_failed_response(capsys): def test_call_server_passes_client_kwargs_to_client(monkeypatch, capsys): """CLI helper forwards connection options to the selected client.""" seen = {} - - class FakeClient: - """Async client stub that records call arguments.""" - - def __init__(self, **kwargs): - seen["client_kwargs"] = kwargs - - async def __aenter__(self): - return self - - async def __aexit__(self, exc_type, exc_val, exc_tb): - return None - - async def __call__(self, action: str, **kwargs): - seen["action"] = action - seen["payload"] = kwargs - yield "ok" - - monkeypatch.setattr( - reme_module, - "create_application_registry", - lambda: SimpleNamespace(get=lambda component_type, backend: FakeClient), - ) + _set_client_backend(monkeypatch, _recording_client(seen)) monkeypatch.setattr(reme_module, "running_app_config", lambda: None) async def run(): @@ -288,29 +297,7 @@ def test_call_server_passes_client_kwargs_to_client(monkeypatch, capsys): def test_call_server_treats_show_metadata_as_client_kwarg(monkeypatch, capsys): """show_metadata controls client display and is not sent as a tool argument.""" seen = {} - - class FakeClient: - """Async client stub that records call arguments.""" - - def __init__(self, **kwargs): - seen["client_kwargs"] = kwargs - - async def __aenter__(self): - return self - - async def __aexit__(self, exc_type, exc_val, exc_tb): - return None - - async def __call__(self, action: str, **kwargs): - seen["action"] = action - seen["payload"] = kwargs - yield "ok" - - monkeypatch.setattr( - reme_module, - "create_application_registry", - lambda: SimpleNamespace(get=lambda component_type, backend: FakeClient), - ) + _set_client_backend(monkeypatch, _recording_client(seen)) monkeypatch.setattr(reme_module, "running_app_config", lambda: None) async def run(): @@ -327,29 +314,7 @@ def test_call_server_treats_show_metadata_as_client_kwarg(monkeypatch, capsys): def test_call_server_passes_shell_parameters_as_payload(monkeypatch, capsys): """Shell-specific parameter names do not collide with client options.""" seen = {} - - class FakeClient: - """Async client stub that records shell request arguments.""" - - def __init__(self, **kwargs): - seen["client_kwargs"] = kwargs - - async def __aenter__(self): - return self - - async def __aexit__(self, exc_type, exc_val, exc_tb): - return None - - async def __call__(self, action: str, **kwargs): - seen["action"] = action - seen["payload"] = kwargs - yield "ok" - - monkeypatch.setattr( - reme_module, - "create_application_registry", - lambda: SimpleNamespace(get=lambda component_type, backend: FakeClient), - ) + _set_client_backend(monkeypatch, _recording_client(seen)) monkeypatch.setattr(reme_module, "running_app_config", lambda: None) async def run(): @@ -365,37 +330,29 @@ def test_call_server_passes_shell_parameters_as_payload(monkeypatch, capsys): def test_call_server_uses_running_plugins_and_their_service_defaults(monkeypatch, capsys): """A bare client call can load the Client backend enabled by the running app.""" seen = {} - - class PluginClient(ComponentMixin): - """Client backend supplied by an enabled plugin.""" - - component_type = ComponentEnum.CLIENT - - def __init__(self, **kwargs): - super().__init__(**kwargs) - seen["client_kwargs"] = kwargs - - async def __aenter__(self): - return self - - async def __aexit__(self, exc_type, exc_val, exc_tb): - return None - - async def __call__(self, action: str, **kwargs): - seen["action"] = action - seen["payload"] = kwargs - yield "plugin-ok" + plugin_client = _recording_client(seen, output="plugin-ok", base=ComponentMixin) + plugin_client.component_type = ComponentEnum.CLIENT manager = PluginManager( [ Plugin( name="example", - backends=(Backend("plugin-client", PluginClient),), - config={"service": {"backend": "plugin-client", "host": "127.0.0.9", "port": 9911}}, + backends=(Backend("plugin-client", plugin_client),), + config={ + "service": { + "backend": "plugin-client", + "host": "127.0.0.9", + "port": 9911, + }, + }, ), ], ) - monkeypatch.setattr(reme_module, "resolve_app_config", lambda **_kwargs: {"service": {"backend": "http"}}) + monkeypatch.setattr( + reme_module, + "resolve_app_config", + lambda **_kwargs: {"service": {"backend": "http"}}, + ) monkeypatch.setattr(reme_module, "running_app_config", lambda: {"plugins": ["example"]}) monkeypatch.setattr(reme_module.PluginManager, "discover", lambda _specs: manager) @@ -410,29 +367,8 @@ def test_call_server_uses_running_plugins_and_their_service_defaults(monkeypatch def test_call_server_skips_local_fallback_when_server_is_running(monkeypatch, capsys): """A usable running config prevents eager parsing of the local fallback.""" - - class FakeClient: - """Minimal running-service client.""" - - def __init__(self, **_kwargs): - pass - - async def __aenter__(self): - return self - - async def __aexit__(self, exc_type, exc_val, exc_tb): - return None - - async def __call__(self, action: str, **kwargs): - assert action == "version" - assert not kwargs - yield "ok" - - monkeypatch.setattr( - reme_module, - "create_application_registry", - lambda: SimpleNamespace(get=lambda component_type, backend: FakeClient), - ) + seen = {} + _set_client_backend(monkeypatch, _recording_client(seen)) monkeypatch.setattr(reme_module, "running_app_config", lambda: {"service": {"backend": "http"}}) def fail_local_resolution(**_kwargs): @@ -442,4 +378,6 @@ def test_call_server_skips_local_fallback_when_server_is_running(monkeypatch, ca asyncio.run(reme_module.call_server("version")) + assert seen["action"] == "version" + assert seen["payload"] == {} assert capsys.readouterr().out == "ok\n"