Skip to content

Commit ecb0c78

Browse files
fix(plugins): isolate malformed plugins and proposals, warn on duplicate names, honour late network opt-in (#379)
Plugin failures escaping isolation (#297): - _cap_and_stamp drops proposals for a column the plugin was not given (an expert's own column; a backend's frame columns) instead of letting them reach the executor, where they raised KeyError. Logged once per plugin so a confused expert does not warn for every column. - The provenance merge and dataclasses.replace are guarded: a proposal with non-mapping provenance is logged and dropped rather than raising. A non-string risk is ranked as "high", as unknown risks already were. - _max_risk requires a string, so unhashable metadata such as max_risk=["low"] falls back to the documented "high" default instead of raising TypeError. - _ensure_entry_points registers each entry point inside its own try. Previously a _register failure escaped after _entry_points_loaded was set, so every later entry point in the group was never registered. - A plugin raising inside propose() stays isolated (unchanged). Duplicate plugin names (#298): - _register logs a WARNING naming the replaced and replacing plugin when a name is reused within a kind. Last registration still wins, as docs/plugins.md describes; re-registering the same object is silent. Late FRESHDATA_ALLOW_NETWORK_PLUGINS opt-in (#299): - A network-using plugin registered without allow_network=True is stored with network_blocked=True. Every registry accessor re-reads the environment variable, so setting it after registration activates the plugin and unsetting it deactivates it again. allow_network=True stays unconditional, missing dependencies still win, and without the variable network plugins remain inactive (offline by default). Tests in tests/test_plugin_isolation.py cover each issue's reproduction; docs/plugins.md describes the dropped proposals, entry-point skipping, the duplicate-name warning and when the variable is read. Closes #297 Closes #298 Closes #299
1 parent 82dba99 commit ecb0c78

3 files changed

Lines changed: 555 additions & 42 deletions

File tree

‎docs/plugins.md‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ fd.register_expert(MyExpert())
4545
`fd.register_backend(...)`, `fd.register_validator(...)`,
4646
`fd.register_comparator(...)`, and `fd.register_exporter(...)` work the same
4747
way. Comparator names may not shadow the built-in comparison kinds
48-
(registration raises); duplicate plugin names are last-wins with a warning.
48+
(registration raises); duplicate plugin names (within one kind) are last-wins
49+
with a warning logged on the `freshdata.plugins` logger.
4950

5051
A comparator is any named callable; an exporter needs `name` +
5152
`export(report) -> str | dict`:
@@ -85,8 +86,9 @@ my_exporter = "my_pkg:MyExporter"
8586
```
8687

8788
The value points at a class (or any zero-arg factory) that FreshData
88-
instantiates on first use. A broken entry point is logged and skipped — it can
89-
never break `import freshdata`.
89+
instantiates on first use. A broken entry point (one that fails to import,
90+
instantiate, or register) is logged and skipped — it can never break
91+
`import freshdata`, and the other entry points still register.
9092

9193
Introspect what's registered:
9294

@@ -103,7 +105,10 @@ fd.registered_plugins("expert") # just experts
103105
- Validators build `freshdata.QualityFinding.create(...)` records.
104106

105107
Anything else you return is dropped with a warning — the registry validates the
106-
output type so a mistake in a plugin can't corrupt the report.
108+
output type so a mistake in a plugin can't corrupt the report. Proposals for a
109+
column the plugin was not given (an expert's own column, or a backend's frame),
110+
and malformed proposals (e.g. a non-mapping `provenance`), are dropped the same
111+
way.
107112

108113
## 3. Required metadata
109114

@@ -125,9 +130,12 @@ Two of these are **enforced**, not advisory:
125130
- **`uses_network=True` disables the plugin by default.** A network-using
126131
plugin is registered but *inactive* until you opt in with
127132
`fd.register_expert(plugin, allow_network=True)` or
128-
`FRESHDATA_ALLOW_NETWORK_PLUGINS=1`. FreshData's own runtime never calls the
129-
network; a network plugin is your explicit choice, disclosed in
130-
`fd.registered_plugins()`.
133+
`FRESHDATA_ALLOW_NETWORK_PLUGINS=1`. The environment variable is checked
134+
each time the registry is consulted (e.g. at `fd.clean` time), so setting it
135+
after registration takes effect — and unsetting it disables the plugin again
136+
unless it was registered with `allow_network=True`. FreshData's own runtime
137+
never calls the network; a network plugin is your explicit choice, disclosed
138+
in `fd.registered_plugins()`.
131139

132140
If `requires` names a package that isn't installed, the plugin is registered but
133141
inactive, with the reason recorded — no `ImportError` at clean time.

‎src/freshdata/plugins.py‎

Lines changed: 139 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@
4747
from typing import TYPE_CHECKING, Any
4848

4949
if TYPE_CHECKING: # pragma: no cover - typing only
50+
from collections.abc import Callable
51+
5052
import pandas as pd
5153

5254
from .findings import QualityFinding
@@ -107,7 +109,9 @@ def _requires(obj: object) -> tuple[str, ...]:
107109

108110
def _max_risk(obj: object) -> str:
109111
raw = getattr(obj, "max_risk", "high")
110-
return raw if raw in _RISK_RANK else "high"
112+
# A non-string (possibly unhashable) value gets the same treatment as an
113+
# unknown string: the documented default ceiling, "high".
114+
return raw if isinstance(raw, str) and raw in _RISK_RANK else "high"
111115

112116

113117
def _semantic_types(obj: object) -> tuple[str, ...]:
@@ -156,6 +160,12 @@ class RegisteredPlugin:
156160
max_risk: str = "high"
157161
semantic_types: tuple[str, ...] = ()
158162
source: str = "explicit" # "explicit" | "entry_point"
163+
#: True for a network-using plugin registered without ``allow_network=True``
164+
#: (and otherwise eligible). Its ``active`` flag then follows
165+
#: ``FRESHDATA_ALLOW_NETWORK_PLUGINS`` as read each time the registry is
166+
#: consulted, not just at registration.
167+
network_blocked: bool = False
168+
_warned: set[str] = field(default_factory=set, repr=False, compare=False)
159169

160170
def describe(self) -> dict[str, Any]:
161171
return {
@@ -187,6 +197,11 @@ def bucket(self, kind: str) -> dict[str, RegisteredPlugin]:
187197

188198
_REGISTRY = _Registry()
189199

200+
_NETWORK_DISABLED_REASON = (
201+
"network-using plugin disabled by default; pass allow_network=True "
202+
"or set FRESHDATA_ALLOW_NETWORK_PLUGINS=1 to enable"
203+
)
204+
190205

191206
def _register(kind: str, obj: object, *, allow_network: bool, source: str) -> RegisteredPlugin:
192207
name = _plugin_name(obj, fallback=type(obj).__name__)
@@ -196,30 +211,67 @@ def _register(kind: str, obj: object, *, allow_network: bool, source: str) -> Re
196211

197212
active = True
198213
reason: str | None = None
214+
network_blocked = False
199215
missing = _missing_requirements(obj)
200216
if kind == "comparator" and name in _RESERVED_COMPARATOR_NAMES:
201217
active = False
202218
reason = "shadows a built-in comparison kind"
203219
elif missing:
204220
active = False
205221
reason = f"requires missing dependency: {', '.join(missing)}"
206-
elif uses_network and not _network_allowed(allow_network):
207-
active = False
208-
reason = (
209-
"network-using plugin disabled by default; pass allow_network=True "
210-
"or set FRESHDATA_ALLOW_NETWORK_PLUGINS=1 to enable"
211-
)
222+
elif uses_network and not allow_network:
223+
# Gated on the environment variable, re-read on every registry access
224+
# (see _refresh_network_gates) so a later opt-in takes effect.
225+
network_blocked = True
226+
active = _network_allowed(False)
227+
reason = None if active else _NETWORK_DISABLED_REASON
212228

213229
record = RegisteredPlugin(
214-
name=name, kind=kind, obj=obj, active=active, inactive_reason=reason,
215-
uses_network=uses_network, max_risk=max_risk, semantic_types=semantic_types,
230+
name=name,
231+
kind=kind,
232+
obj=obj,
233+
active=active,
234+
inactive_reason=reason,
235+
uses_network=uses_network,
236+
max_risk=max_risk,
237+
semantic_types=semantic_types,
216238
source=source,
239+
network_blocked=network_blocked,
217240
)
218-
_REGISTRY.bucket(kind)[name] = record
241+
bucket = _REGISTRY.bucket(kind)
242+
previous = bucket.get(name)
243+
if previous is not None and previous.obj is not obj:
244+
log.warning(
245+
"%s plugin name %r is already registered (%s, source=%s); replacing it "
246+
"with %s (source=%s): the last registration wins",
247+
kind,
248+
name,
249+
type(previous.obj).__name__,
250+
previous.source,
251+
type(obj).__name__,
252+
source,
253+
)
254+
bucket[name] = record
219255
log.debug("registered %s plugin %r (active=%s)", kind, name, active)
220256
return record
221257

222258

259+
def _refresh_network_gates() -> None:
260+
"""Re-evaluate env-gated network plugins against the current environment."""
261+
allowed = _network_allowed(False)
262+
for kind in _ENTRY_POINT_GROUPS:
263+
for rec in _REGISTRY.bucket(kind).values():
264+
if rec.network_blocked:
265+
rec.active = allowed
266+
rec.inactive_reason = None if allowed else _NETWORK_DISABLED_REASON
267+
268+
269+
def _sync_registry() -> None:
270+
"""Discover entry points once, then apply the current network opt-in."""
271+
_ensure_entry_points()
272+
_refresh_network_gates()
273+
274+
223275
# --------------------------------------------------------------------------- #
224276
# public registration API (re-exported as fd.register_*) #
225277
# --------------------------------------------------------------------------- #
@@ -291,7 +343,7 @@ def clear_plugins(kind: str | None = None) -> None:
291343

292344
def registered_plugins(kind: str | None = None) -> list[dict[str, Any]]:
293345
"""Introspect every registered plugin (active and inactive)."""
294-
_ensure_entry_points()
346+
_sync_registry()
295347
buckets = [kind] if kind else ["expert", "backend", "validator", "comparator", "exporter"]
296348
out: list[dict[str, Any]] = []
297349
for k in buckets:
@@ -322,43 +374,93 @@ def _ensure_entry_points() -> None:
322374
_REGISTRY._entry_points_loaded = True # set first: never retry a bad group
323375
for kind, group in _ENTRY_POINT_GROUPS.items():
324376
for ep in _entry_points_for(group):
377+
# Load *and* register inside the guard: malformed metadata on one
378+
# entry point must not stop the rest of the group from registering.
325379
try:
326380
factory = ep.load()
327381
obj = factory() if callable(factory) else factory
382+
_register(kind, obj, allow_network=False, source="entry_point")
328383
except Exception as exc: # noqa: BLE0001 - a broken plugin must not break import
329-
log.warning("freshdata plugin entry point %r failed to load: %s", ep.name, exc)
330-
continue
331-
_register(kind, obj, allow_network=False, source="entry_point")
384+
log.warning(
385+
"freshdata plugin entry point %r failed to load: %s",
386+
getattr(ep, "name", ep),
387+
exc,
388+
)
332389

333390

334391
# --------------------------------------------------------------------------- #
335392
# safe adapters #
336393
# --------------------------------------------------------------------------- #
337394

338395

396+
def _column_allowed(column: object, allowed: Callable[[object], bool]) -> bool:
397+
try:
398+
return bool(allowed(column))
399+
except Exception: # noqa: BLE001 - e.g. an unhashable column label
400+
return False
401+
402+
339403
def _cap_and_stamp(
340-
proposals: object, name: str, kind: str, max_risk: str,
404+
proposals: object,
405+
record: RegisteredPlugin,
406+
kind: str,
407+
*,
408+
allowed_column: Callable[[object], bool],
341409
) -> list[SemanticProposal]:
342-
"""Validate plugin output, drop over-risk proposals, and stamp provenance."""
410+
"""Validate plugin output, drop over-risk, off-target or malformed
411+
proposals, and stamp provenance."""
343412
from .semantic.types import SemanticProposal # noqa: PLC0415
344413

414+
name, max_risk = record.name, record.max_risk
345415
if not isinstance(proposals, (list, tuple)):
346-
log.warning("plugin %s %r returned %s, not a list of proposals; ignored",
347-
kind, name, type(proposals).__name__)
416+
log.warning(
417+
"plugin %s %r returned %s, not a list of proposals; ignored",
418+
kind,
419+
name,
420+
type(proposals).__name__,
421+
)
348422
return []
349423
cap = _RISK_RANK[max_risk]
350424
out: list[SemanticProposal] = []
351425
for p in proposals:
352426
if not isinstance(p, SemanticProposal):
353427
log.warning("plugin %s %r emitted a non-proposal %r; dropped", kind, name, type(p))
354428
continue
355-
if _RISK_RANK.get(p.risk, 2) > cap:
356-
log.warning("plugin %s %r emitted risk=%s above its declared max_risk=%s; dropped",
357-
kind, name, p.risk, max_risk)
429+
if not _column_allowed(p.column, allowed_column):
430+
# A plugin may only propose for the column/frame it was handed;
431+
# anything else would reach the executor as an unknown column.
432+
# Log once per plugin: a confused plugin repeats this per column.
433+
if "column" not in record._warned:
434+
record._warned.add("column")
435+
log.warning(
436+
"plugin %s %r proposed for column %r it was not given; "
437+
"such proposals are dropped (logged once per plugin)",
438+
kind,
439+
name,
440+
p.column,
441+
)
442+
continue
443+
risk_rank = _RISK_RANK.get(p.risk, 2) if isinstance(p.risk, str) else 2
444+
if risk_rank > cap:
445+
log.warning(
446+
"plugin %s %r emitted risk=%s above its declared max_risk=%s; dropped",
447+
kind,
448+
name,
449+
p.risk,
450+
max_risk,
451+
)
452+
continue
453+
try:
454+
provenance = {
455+
**(dict(p.provenance) if p.provenance else {}),
456+
"plugin": name,
457+
"plugin_kind": kind,
458+
}
459+
stamped = dataclasses.replace(p, backend=f"plugin:{name}", provenance=provenance)
460+
except Exception as exc: # noqa: BLE001 - malformed proposal must not break a clean
461+
log.warning("plugin %s %r emitted a malformed proposal (%s); dropped", kind, name, exc)
358462
continue
359-
provenance = {**(dict(p.provenance) if p.provenance else {}),
360-
"plugin": name, "plugin_kind": kind}
361-
out.append(dataclasses.replace(p, backend=f"plugin:{name}", provenance=provenance))
463+
out.append(stamped)
362464
return out
363465

364466

@@ -383,7 +485,8 @@ def propose(self, series: pd.Series, info: SemanticColumnInfo) -> list[SemanticP
383485
except Exception as exc: # noqa: BLE0001 - isolate plugin failure
384486
log.warning("expert plugin %r.propose() failed: %s", self.name, exc)
385487
return []
386-
return _cap_and_stamp(raw, self.name, "expert", self._record.max_risk)
488+
column = info.name
489+
return _cap_and_stamp(raw, self._record, "expert", allowed_column=lambda c: c == column)
387490

388491

389492
class _SafeBackend:
@@ -413,7 +516,8 @@ def propose(self, df: pd.DataFrame, ctx: SemanticContext, budget: object) -> lis
413516
except Exception as exc: # noqa: BLE0001 - isolate plugin failure
414517
log.warning("backend plugin %r.propose() failed: %s", self.name, exc)
415518
return []
416-
return _cap_and_stamp(raw, self.name, "backend", self._record.max_risk)
519+
columns = df.columns
520+
return _cap_and_stamp(raw, self._record, "backend", allowed_column=lambda c: c in columns)
417521

418522

419523
class _SafeValidator:
@@ -488,53 +592,53 @@ def export(self, report: object) -> str | dict:
488592

489593
def active_experts() -> tuple[_SafeExpert, ...]:
490594
"""Active plugin experts, wrapped for isolation (built-ins are separate)."""
491-
_ensure_entry_points()
595+
_sync_registry()
492596
return tuple(_SafeExpert(rec) for rec in _REGISTRY.experts.values() if rec.active)
493597

494598

495599
def active_backend_names() -> tuple[str, ...]:
496-
_ensure_entry_points()
600+
_sync_registry()
497601
return tuple(name for name, rec in _REGISTRY.backends.items() if rec.active)
498602

499603

500604
def get_active_backend(name: str) -> _SafeBackend | None:
501-
_ensure_entry_points()
605+
_sync_registry()
502606
rec = _REGISTRY.backends.get(name)
503607
return _SafeBackend(rec) if rec is not None and rec.active else None
504608

505609

506610
def known_backend_names() -> tuple[str, ...]:
507611
"""Registered backend names (active or not) — for the ``strict`` name check."""
508-
_ensure_entry_points()
612+
_sync_registry()
509613
return tuple(_REGISTRY.backends)
510614

511615

512616
def active_validators() -> tuple[_SafeValidator, ...]:
513-
_ensure_entry_points()
617+
_sync_registry()
514618
return tuple(_SafeValidator(rec) for rec in _REGISTRY.validators.values() if rec.active)
515619

516620

517621
def get_active_comparator(name: str) -> _SafeComparator | None:
518622
"""The comparator registered under *name*, or ``None`` (used by ER scoring)."""
519-
_ensure_entry_points()
623+
_sync_registry()
520624
rec = _REGISTRY.comparators.get(name)
521625
return _SafeComparator(rec) if rec is not None and rec.active else None
522626

523627

524628
def known_comparator_names() -> tuple[str, ...]:
525629
"""Registered comparator names (active or not) — for config validation."""
526-
_ensure_entry_points()
630+
_sync_registry()
527631
return tuple(_REGISTRY.comparators)
528632

529633

530634
def get_active_exporter(name: str) -> _SafeExporter | None:
531-
_ensure_entry_points()
635+
_sync_registry()
532636
rec = _REGISTRY.exporters.get(name)
533637
return _SafeExporter(rec) if rec is not None and rec.active else None
534638

535639

536640
def active_exporter_names() -> tuple[str, ...]:
537-
_ensure_entry_points()
641+
_sync_registry()
538642
return tuple(name for name, rec in _REGISTRY.exporters.items() if rec.active)
539643

540644

0 commit comments

Comments
 (0)