feat(integrations): plugin architecture for lookup clients (Phase 3.5)#55
Conversation
- Reject whitespace-only names (was: passed silently) - Reject non-string name types with TypeError (was: crashed later inside sorted()) - Test fixture clears registry on both setup and teardown Refs #22
Loads plugins from the 'label_hub.integrations' entry-points group. Each plugin is loaded, instantiated, validated against the IntegrationPlugin Protocol, and registered independently — a single broken third-party plugin logs an error and is skipped without preventing the others from loading. Failure modes covered: load-time exception, non-Plugin export, name collision with an already-registered plugin. Refs #22
…iscovery enforcement The Protocol's @runtime_checkable property is now enforced by _discover_plugins in app.integrations.__init__, not by the registry itself. Update the docstring to reflect where the isinstance check actually lives. Refs #22
…iscovery - _logger.exception() instead of error() on load/instantiate guards to capture stack traces for production debugging - except (ValueError, TypeError) on register() guard — non-string plugin name no longer escapes and kills the discovery loop - Add regression test for the TypeError-escape path - Docstring updated: four failure modes (was three) Refs #22
Renames SnipeITClient → SnipeITPlugin and adds name/display_name attributes so the class satisfies the IntegrationPlugin Protocol. File moves preserve git history. lookup_service.py has no direct SnipeITClient import — it uses the _LookupClient structural Protocol — so no alias shim is needed there. The test top-level import uses `SnipeITPlugin as SnipeITClient` to keep all call-site names in the test body unchanged. Refs #22
Renames SpoolmanClient → SpoolmanPlugin and adds name/display_name attributes so the class satisfies the IntegrationPlugin Protocol. File moves preserve git history. lookup_service.py has no direct SpoolmanClient import — it uses the _LookupClient structural Protocol — so no alias shim is needed there. Test imports use SpoolmanPlugin directly (clean, no alias) since all call-site names were updated in place. Also fixes test_snipeit_plugin.py: the cross-app inheritance test (test_not_found_error_is_app_lookup_not_found) imported SpoolmanNotFoundError from the now-deleted app.services.spoolman_client path; updated to use app.integrations.spoolman.plugin. Refs #22
Renames GrocyClient → GrocyPlugin and adds name/display_name attributes so the class satisfies the IntegrationPlugin Protocol. File moves preserve git history. With this commit all three integration clients live under app/integrations/ and the cross-app inheritance test in test_snipeit_plugin.py now covers all three plugins through their new import paths. The next step (Phase 6) drops AppLookupService's constructor-injection in favor of IntegrationRegistry lookup. Refs #22
Each plugin's __init__ now takes no arguments and reads its config (base_url, api_key, timeout) from app.config.get_settings(). This is required for the entry_points discovery, which instantiates plugins via plugin_cls() with no args. Three timeout fields were added to Settings (snipeit_timeout, grocy_timeout, spoolman_timeout, all PRINTER_HUB_-prefixed, default 5.0). The snipeit_api_key and grocy_api_key fields are SecretStr — the plugin constructors call .get_secret_value() internally. Test fixtures replace constructor-arg injection with monkeypatch.setenv() + get_settings.cache_clear(). Each module gets an autouse fixture that wires the PRINTER_HUB_* env vars to the fake host used by respx mocks. Tests that need a different value (e.g. trailing-slash, bearer-header) override the env var directly before constructing the plugin. Refs #22
…n with Registry Drops the _AppName Literal, AVAILABLE_APPS constant, _LookupClient Protocol, and the three-positional-argument constructor. Service now dispatches through IntegrationRegistry.get() — new plugins registered via entry_points become available without touching this module. UnknownAppError wraps IntegrationNotFoundError and preserves the operationally distinct contract: unknown app = caller bug (not data state). The error message propagates from the Registry so it naturally lists registered plugin names. The available_apps property delegates to IntegrationRegistry.names(), returning a sorted list of currently registered plugin names. Test suite replaces MagicMock constructor injection with a _StubPlugin fixture that populates and cleans the Registry around each test. All seven prior test cases are preserved as equivalent Registry-based tests. Refs #22
…on writes Five tests previously wrote to os.environ[...] directly. The mutations were not undone at test-end; they only happened to work because the _stub_settings autouse fixture + cache_clear() pattern restored consistent state before the next test. Under pytest-xdist or any test-reordering this would become flaky. Switching to monkeypatch.setenv() makes each test fully self-contained — pytest restores the previous env-var state automatically at teardown. Also removes the in-function `import os` statements that are no longer needed. Refs #22
Snipe-IT, Spoolman, and Grocy plugins are declared under the label_hub.integrations entry-points group, so importing app.integrations at app start triggers automatic discovery and registration via IntegrationRegistry. Third-party plugins follow the same pattern in their own pyproject.toml. Refs #22
README covers three areas: - Adding a bundled plugin (in this repo) - Adding a third-party plugin (external pip package) - Plugin contract, defensive loading, and the test fixture pattern Refs #22
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a flexible plugin architecture for external lookup integrations. By moving from hardcoded clients to a registry-based system, the application now supports extensible third-party plugins that register automatically via entry-points. This change improves maintainability, enables modular deployments, and simplifies the integration of new external services. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a plugin-based architecture for external integrations (Snipe-IT, Grocy, and Spoolman) using Python entry points and a central registry. It refactors the existing clients into standalone plugins and updates the AppLookupService to dynamically discover and route requests through the IntegrationRegistry. Feedback suggests improving the documentation to explicitly mention that bundled plugins require corresponding fields in the core configuration and to advise third-party plugins to manage their own settings via environment variables or local Pydantic classes, as they cannot extend the core Settings object.
| # Local import avoids a load-time cycle. | ||
| from app.config import get_settings | ||
| settings = get_settings() | ||
| self._base_url = settings.myapp_url.rstrip("/") |
There was a problem hiding this comment.
This example assumes myapp_url exists on the Settings object. When adding a bundled plugin, the author must also add the corresponding fields to backend/app/config.py. It would be helpful to explicitly mention this step in the guide.
| |---|---|---| | ||
| | `name` | `str` | Canonical id used in templates (`TemplateSchema.app`) and audit logs. Must be unique across all registered plugins. | | ||
| | `display_name` | `str` | UI label, e.g. shown in template-picker dropdowns. | | ||
| | `__init__(self)` | `None` | Must accept no positional or keyword arguments — entry-points discovery instantiates plugins with `plugin_cls()`. Read configuration from `app.config.get_settings()` via a local import. | |
There was a problem hiding this comment.
The instruction to read configuration from app.config.get_settings() is only applicable to bundled plugins. Third-party plugins cannot extend the core Settings class without modifying the core repository (due to extra="ignore" in Settings.model_config). For external plugins, the documentation should recommend reading configuration directly from environment variables or using a local Pydantic BaseSettings class to maintain proper decoupling.
There was a problem hiding this comment.
Pull request overview
Refactors the built-in Snipe-IT / Grocy / Spoolman lookup clients into an integration plugin architecture discovered via setuptools entry points (label_hub.integrations), and switches AppLookupService to dispatch through a central IntegrationRegistry.
Changes:
- Added
IntegrationPluginProtocol +IntegrationRegistry, plus entry-point discovery inapp.integrations(defensive loading and error logging). - Converted the three integrations into plugins with no-arg constructors that read config via
get_settings(). - Updated tests and settings to support the new plugin + registry behavior (including discovery tests and per-integration settings fixtures).
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/app/config.py | Adds per-integration timeout settings used by plugin constructors. |
| backend/app/integrations/init.py | Implements entry-point discovery and registers plugins defensively at import time. |
| backend/app/integrations/README.md | Documents the plugin contract, entry-point registration, defensive loading, and test patterns. |
| backend/app/integrations/base.py | Defines the IntegrationPlugin runtime-checkable Protocol. |
| backend/app/integrations/grocy/init.py | Marks Grocy as a plugin package. |
| backend/app/integrations/grocy/plugin.py | Converts Grocy client into a GrocyPlugin reading settings via get_settings(). |
| backend/app/integrations/registry.py | Adds the central IntegrationRegistry + IntegrationNotFoundError. |
| backend/app/integrations/snipeit/init.py | Marks Snipe-IT as a plugin package. |
| backend/app/integrations/snipeit/plugin.py | Converts Snipe-IT client into a SnipeITPlugin reading settings via get_settings(). |
| backend/app/integrations/spoolman/init.py | Marks Spoolman as a plugin package. |
| backend/app/integrations/spoolman/plugin.py | Converts Spoolman client into a SpoolmanPlugin reading settings via get_settings(). |
| backend/app/services/lookup_service.py | Switches dispatch to IntegrationRegistry.get() and exposes available_apps via registry. |
| backend/pyproject.toml | Declares bundled plugins under [project.entry-points."label_hub.integrations"]. |
| backend/tests/unit/integrations/init.py | Initializes the integrations unit test package. |
| backend/tests/unit/integrations/test_base.py | Adds Protocol smoke tests (runtime-checkable + type hints contract). |
| backend/tests/unit/integrations/test_discovery.py | Adds entry-point discovery tests (success + multiple failure modes). |
| backend/tests/unit/integrations/test_grocy_plugin.py | Updates Grocy tests to use GrocyPlugin() + settings via env vars. |
| backend/tests/unit/integrations/test_registry.py | Adds unit tests for registry validation, sorting, and runtime protocol checks. |
| backend/tests/unit/integrations/test_snipeit_plugin.py | Updates Snipe-IT tests to use SnipeITPlugin() + settings via env vars. |
| backend/tests/unit/integrations/test_spoolman_plugin.py | Updates Spoolman tests to use SpoolmanPlugin() + settings via env vars. |
| backend/tests/unit/services/test_lookup_service.py | Updates lookup service tests to use registry-based dispatch with stub plugins. |
Comments suppressed due to low confidence (1)
backend/app/integrations/spoolman/plugin.py:33
- The SpoolmanPlugin docstring mentions an "optional API key", but this plugin does not use or accept an API key (and Settings has no spoolman_api_key). Please adjust the docstring to match the actual configuration surface so plugin authors/users aren’t misled.
| from app.integrations.registry import ( | ||
| IntegrationNotFoundError, | ||
| IntegrationRegistry, | ||
| ) |
| """Add `plugin` under its `.name`. | ||
|
|
||
| Rejects empty, whitespace-only, or non-string names and duplicates. | ||
| """ | ||
| if not isinstance(plugin.name, str): | ||
| raise TypeError( | ||
| f"IntegrationPlugin name must be a string; got {type(plugin.name).__name__}" | ||
| ) | ||
| if not plugin.name.strip(): | ||
| raise ValueError( | ||
| f"IntegrationPlugin requires a non-empty name; got {plugin.name!r}" | ||
| ) | ||
| if plugin.name in cls._plugins: | ||
| raise ValueError( | ||
| f"Plugin {plugin.name!r} already registered" | ||
| ) | ||
| cls._plugins[plugin.name] = plugin |
CI's ruff format --check step caught formatting drift in the 12 files added/modified during Phase 3.5. Local pre-checks ran ruff check (lint) but not ruff format (formatting); fixed by running ruff format on the affected files. No logic changes. Refs #22
- README: document the config.py step for bundled plugins and the external-plugin-config pattern for third-party plugins (Gemini) - lookup_service.py: side-effect-import app.integrations to ensure the registry is populated even when main.py is not on the call path (Copilot) - registry.py: reject plugin names with leading/trailing whitespace in addition to whitespace-only; whitespace in a canonical id is always wrong (Copilot, plus regression test) - spoolman/plugin.py: docstring no longer claims an api_key field that does not exist (Copilot suppressed) Refs #22
|
All 5 review findings addressed in
Tests: 149/149 (148 + new padded-name test). Ruff check + format + mypy strict all clean. |
Phase 3.5 — Integration Plugin Architecture
Refactors the three hardcoded lookup-clients (snipeit, spoolman, grocy)
into a plugin pattern analogous to the existing
PrinterModelregistry.Plugins register via setuptools entry-points (group
label_hub.integrations) so third-party integrations install viapip installwithout touching the core repo.Changes
New
app/integrations/base.py—IntegrationPluginProtocol(
@runtime_checkable)app/integrations/registry.py—IntegrationRegistryanalogous toModelRegistry, with hardened input validationapp/integrations/__init__.py—entry_pointsdiscovery at importtime, defensive against four failure modes
app/integrations/{snipeit,spoolman,grocy}/plugin.py— relocated andrenamed; classes implement the Protocol
app/integrations/README.md— how to add a plugin (bundled orthird-party), plugin contract, defensive-loading model, test fixture
pattern
Modified
pyproject.toml— bundled plugins declared under[project.entry-points."label_hub.integrations"]app/services/lookup_service.py— usesIntegrationRegistry.get(name)instead of constructor-injection; drops
_AppNameLiteral,AVAILABLE_APPSconstant,_LookupClientProtocolapp/config.py— adds*_timeoutfields for the three integrations__init__reads config fromapp.config.get_settings()sothe no-arg constructor
plugin_cls()(used by_discover_plugins)works at import time
Deleted
app/services/{snipeit,spoolman,grocy}_client.pytests/unit/integrations/)How to add a new integration
Bundled: drop
plugin.pyinapp/integrations/<name>/, register inpyproject.toml. Third-party: ship a Python package with the sameentry-points group —
pip install <pkg>registers it automatically.See
app/integrations/README.md.Test plan
pytest— 148/148 greenpytest --cov=app— 91.95% (well above the 80% threshold)ruff check .— no errorsmypy app/integrations app/services tests/unit/integrations --strict— clean (no issues in 24 source files)
PRINTER_HUB_*env vars set, importingapp.integrationsdiscovers all three plugins via entry_points;IntegrationRegistry.names()andAppLookupService().available_appsboth return['grocy', 'snipeit', 'spoolman']services/) pass(
*NotFoundErrorsubclassesAppLookupNotFoundErrorfor allthree plugins)
Notes for reviewers
_AppNameLiteral is gone, but the public API surface is unchanged:AppLookupService().lookup(source_app, identifier)returnsLabelData;UnknownAppErrorstill raised on unknown apps.IntegrationRegistryis hardened against empty, whitespace-only, andnon-string
namevalues (Phase-1 review feedback)._discover_pluginscatches four failure modes (load exception,instantiation exception, Protocol mismatch, name collision /
type-error from registry). All failures log via
logging(levelERROR) with the entry-point name — a single broken third-party
plugin cannot prevent the rest from registering.
os.environdirectlynow use
monkeypatch.setenvfor proper test isolation(pytest-xdist compatible).
Refs #22 — sets the stage for Phase 4 (default templates), which will
reference
TemplateSchema.appstrings against this registry.