From 39a4c4d8ff8c1e41acc530573d609c3bb0832dd4 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 4 Oct 2026 23:53:22 -0400 Subject: [PATCH] fix(foundation): raise RegistryError, not a leaked TypeError, for a non-class Six registrar guards promised `RegistryError` for a non-class argument and leaked `abc`'s `TypeError` instead. Closes #1021. * Each guard was a bare `if not issubclass(x, Base):`, so a non-class was rejected before the `raise` was reached. Measured, with the raise site read off the traceback rather than the message: Extractor.register_dumper -> TypeError | extraction.py:406 Extractor.register_engine -> TypeError | :123 Extractor.register_reassembly -> TypeError | :123 Extractor.register_traceflow -> TypeError | :123 TraceFlow.register_dumper -> TypeError | traceflow.py:237 register_protocol -> TypeError | :123 Two mechanisms: `Dumper`'s metaclass is plain `type`, so the builtin `issubclass` raises in place; the other targets carry `ABCMeta`-derived metaclasses, so it delegates and `abc` raises. Identical message either way, which is what hid this. * `TraceFlow.register_dumper` matters beyond symmetry. It backs the exported `register_traceflow_dumper`, whose sibling `register_extractor_dumper` has an identical signature and docstring thirty lines away -- so fixing only the `Extractor` side left two indistinguishable public functions raising different exceptions. * The check sits *after* the `ModuleDescriptor` unwrap in the five registrars that accept one, because that branch unwraps rather than short-circuits. `register_protocol` accepts no descriptor. Not breaking: `RegistryError` subclasses `TypeError` via `BaseError`, so `except TypeError` still catches it. Only a caller matching the exact type, or the old message text, sees a difference. No guard's target class changes -- that is #1016's subject -- and a wrong class still raises `RegistryError` as before. No docstring changed: the four `Raises:` clauses already read "is not a class, or not a `X` subclass" and are simply true now. Six of thirteen `issubclass` guards in `pcapkit/`. The `register` classmethods on `ProtocolBase`, `Frame`, `PCAPNG`, `SCTP`, `Link`, `Internet` and `Transport` all still leak. `Transport.register` is **not** exempt as an earlier draft of this message claimed: its `UnsupportedCall` is gated on `cls is Transport`, so it fires only for the abstract class, and the guard below leaks through `TCP.register` and `UDP.register` -- the only way it is ever reached. #1026 tracks all seven. New tests cover all six registrars across non-class inputs including a descriptor resolving to a non-class, plus the unchanged wrong-class path. On main they produce 28 subfailures, every one the leaked `TypeError`. tests/foundation + tests/project: 536 passed, 13 skipped, 1319 subtests passed. --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 21 +++++++++ .../contributing/conventions/process.rst | 2 +- pcapkit/foundation/extraction.py | 8 ++++ pcapkit/foundation/registry/protocols.py | 2 + pcapkit/foundation/traceflow/traceflow.py | 2 + tests/foundation/registry/test_foundation.py | 45 +++++++++++++++++++ tests/foundation/registry/test_protocols.py | 24 ++++++++++ tests/foundation/test_extraction.py | 38 ++++++++++++++++ 9 files changed, 142 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96bcc9c47..5041cd088 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -100,6 +100,7 @@ Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) an - two `#:` autodoc comments in `pcapkit/foundation/traceflow/traceflow.py` named a bare `Type`, which Sphinx resolves against every class named `Type` in the project (five) rather than `typing.Type`, and silently linked to `pcapkit.const.l2tp.type.Type`, an L2TP field-type enum. Line 424 (`#: ~typing.Type[Dumper]: Dumper class.`, spelled out since [#709](https://github.com/JarryShaw/PyPCAPKit/issues/709) fixed it) is the live case: once [#684](https://github.com/JarryShaw/PyPCAPKit/issues/684) rendered `TraceFlow._foutio`, the built docs pointed at the wrong class with no warning. Both sites now spell it `~typing.Type[Dumper]`, as eight other files already do. Line 146 (the first line of `__output__`'s `#:` block) is fixed for the same reason but is currently inert, since the `# type:` comment at line 162 spells the same bare `Type[Dumper]`; it is insurance for whatever eventually renders that type. This pair is part of [#709](https://github.com/JarryShaw/PyPCAPKit/issues/709). Four more bare `Type` sites, the hand-written `:type:` fields at `docs/source/pcapkit/foundation/engines/engine.rst:40`, `.../reassembly/reassembly.rst:33` and `:43`, and `.../traceflow/traceflow.rst:40`, are fixed separately by [#714](https://github.com/JarryShaw/PyPCAPKit/pull/714) ([#709](https://github.com/JarryShaw/PyPCAPKit/issues/709)). - `register_protocol`'s overwrite warning could claim a protocol was replaced with itself. The guard was correct (`incumbent is not protocol`, an identity check from [#681](https://github.com/JarryShaw/PyPCAPKit/pull/681)), but the message built both operands with a bare `repr()`. A factory defining a same-named closure-local class on every call (as `tests/protocols/test_construction_keyword_check_unit.py`'s `_protocol_class` does) produces two distinct classes sharing one `__module__` and `__qualname__`, so a real overwrite read "overwriting X with X." The message now compares the two reprs and, only when they coincide, appends each object's `id()`. `__module__`/`__qualname__` was rejected as the disambiguator: for the reported shape they are exactly what the coinciding repr already renders. The new `test_register_protocol_disambiguates_classes_sharing_a_repr` fails against the unfixed message ([#710](https://github.com/JarryShaw/PyPCAPKit/issues/710)). - five more registrars warned on mere key presence rather than an actual overwrite, outside the wording of [#718](https://github.com/JarryShaw/PyPCAPKit/issues/718)'s identity guard (`incumbent is not None and incumbent is not new`, landed by [#726](https://github.com/JarryShaw/PyPCAPKit/pull/726)), whose issue named only code-keyed registrars: `register_engine`, `register_reassembly` and `register_traceflow` on `Extractor`, and `register_dumper` on both `Extractor` and `TraceFlow`. All five now compare the incumbent by identity before warning; both `register_dumper` sites compare only the stored dumper, so re-registering with just a new file extension stays silent too, judged defensible rather than comparing the full `(dumper, ext)` pair. Non-breaking: a correct caller sees strictly fewer warnings and no change to return value or exception. Five new tests pin the silent/warns-anyway split ([#739](https://github.com/JarryShaw/PyPCAPKit/issues/739)). +- **six registrars** now raise `RegistryError` for a non-class argument, where a bare `TypeError` ("issubclass() arg 1 must be a class") used to escape -- from the guard itself for the two `register_dumper` methods, and from inside `abc` for the other four. They are `register_dumper`, `register_engine`, `register_reassembly` and `register_traceflow` on `Extractor`, `TraceFlow.register_dumper` (reached through `register_traceflow_dumper`), and `pcapkit.foundation.registry.protocols.register_protocol`. Each guard gains an explicit `isinstance(x, type)` test ahead of its `issubclass`; in the five that accept a `ModuleDescriptor` it runs after the descriptor is unwrapped, so a descriptor naming a non-class attribute is rejected too, while `register_protocol` takes no descriptor. No guard's target class changes, and a wrong class still raises `RegistryError` as before. `RegistryError` subclasses `TypeError`, so `except TypeError` still catches it; a caller matching the exact type, or the old message, does not. **Not every site is covered**: six of the thirteen bare `issubclass` guards in the package are fixed here, and the same guard remains in the `register` classmethods of `ProtocolBase`, `Frame`, `PCAPNG`, `SCTP`, `Link`, `Internet` and `Transport`, all of which still leak `TypeError` for a non-class. `Transport.register` is reachable despite its `UnsupportedCall`, which is gated on `cls is Transport` and so fires only for the abstract class -- the guard below it leaks through `TCP.register` and `UDP.register`, which is the only way it is ever called. [#1026](https://github.com/JarryShaw/PyPCAPKit/issues/1026) tracks all seven ([#1021](https://github.com/JarryShaw/PyPCAPKit/issues/1021)). ### pcapkit.protocols diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 9b625a04b..5de7607d6 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -1008,6 +1008,27 @@ Fixed rather than comparing the full ``(dumper, ext)`` pair. Non-breaking: a correct caller sees strictly fewer warnings and no change to return value or exception. Five new tests pin the silent/warns-anyway split (:issue:`739`). +* **six registrars** now raise ``RegistryError`` for a non-class argument, where a bare + ``TypeError`` ("issubclass() arg 1 must be a class") used to escape -- from the guard + itself for the two ``register_dumper`` methods, and from inside ``abc`` for the other + four. They are ``register_dumper``, ``register_engine``, ``register_reassembly`` and + ``register_traceflow`` on ``Extractor``, ``TraceFlow.register_dumper`` (reached + through ``register_traceflow_dumper``), and + ``pcapkit.foundation.registry.protocols.register_protocol``. Each guard gains an + explicit ``isinstance(x, type)`` test ahead of its ``issubclass``; in the five that + accept a ``ModuleDescriptor`` it runs after the descriptor is unwrapped, so a descriptor + naming a non-class attribute is rejected too, while ``register_protocol`` takes no + descriptor. No guard's target class changes, and a wrong class still raises + ``RegistryError`` as before. ``RegistryError`` subclasses ``TypeError``, so + ``except TypeError`` still catches it; a caller matching the exact type, or the old + message, does not. **Not every site is covered**: six of the thirteen bare + ``issubclass`` guards in the package are fixed here, and the same guard remains in + the ``register`` classmethods of ``ProtocolBase``, ``Frame``, ``PCAPNG``, ``SCTP``, + ``Link``, ``Internet`` and ``Transport``, all of which still leak ``TypeError`` for a + non-class. ``Transport.register`` is reachable despite its ``UnsupportedCall``, which + is gated on ``cls is Transport`` and so fires only for the abstract class -- the guard + below it leaks through ``TCP.register`` and ``UDP.register``, which is the only way it + is ever called. :issue:`1026` tracks all seven (:issue:`1021`). pcapkit.protocols ----------------- diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index 32eb9444a..83a1eff84 100644 --- a/docs/source/contributing/conventions/process.rst +++ b/docs/source/contributing/conventions/process.rst @@ -101,7 +101,7 @@ commands down instead of a figure that will be stale by the next merge: The grouping scheme was settled on :issue:`918`: **a section per top-level module, with** ``Added``/``Changed``/``Fixed`` **nested inside each** -- module granularity, not per-file and not per-subpackage. The file carries **9** module-level sections holding -158 entries, and no entry carries an inline kind label:: +159 entries, and no entry carries an inline kind label:: $ grep -cE '^\* \*\*(Added|Changed|Fixed)\*\*' docs/source/changelog/1.5.0.rst 0 diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index fcba44db3..0ccae748e 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -409,6 +409,8 @@ def register_dumper(cls, format: 'str', dumper: 'ModuleDescriptor[Dumper] | Type """ if isinstance(dumper, ModuleDescriptor): dumper = dumper.klass + if not isinstance(dumper, type): + raise RegistryError(f'dumper must be a class, not {dumper!r}') if not issubclass(dumper, Dumper): raise RegistryError(f'dumper must be a Dumper subclass, not {dumper!r}') incumbent_entry = cls.__output__.get(format) @@ -449,6 +451,8 @@ def register_engine(cls, name: 'str', engine: 'ModuleDescriptor[Engine] | Type[E """ if isinstance(engine, ModuleDescriptor): engine = engine.klass + if not isinstance(engine, type): + raise RegistryError(f'engine must be a class, not {engine!r}') # NOTE: checked against the public ``Engine``, the class third-party engines are # meant to extend and the one carrying the ``engine=`` registration hook. The # built-ins derive from ``EngineBase`` directly, so this door refuses them; @@ -520,6 +524,8 @@ def register_reassembly(cls, protocol: 'str', reassembly: 'ModuleDescriptor[Reas """ if isinstance(reassembly, ModuleDescriptor): reassembly = reassembly.klass + if not isinstance(reassembly, type): + raise RegistryError(f'reassembly must be a class, not {reassembly!r}') # NOTE: ``Reassembly`` rather than ``ReassemblyBase``, for the reason given in # :meth:`register_engine` above -- see #1016. Built-ins go through # :meth:`_register_internal_reassembly`. @@ -589,6 +595,8 @@ def register_traceflow(cls, protocol: 'str', traceflow: 'ModuleDescriptor[TraceF """ if isinstance(traceflow, ModuleDescriptor): traceflow = traceflow.klass + if not isinstance(traceflow, type): + raise RegistryError(f'traceflow must be a class, not {traceflow!r}') # NOTE: ``TraceFlow`` rather than ``TraceFlowBase``, for the reason given in # :meth:`register_engine` above -- see #1016. Built-ins go through # :meth:`_register_internal_traceflow`. diff --git a/pcapkit/foundation/registry/protocols.py b/pcapkit/foundation/registry/protocols.py index f7f3ccd87..f0c45eb0d 100644 --- a/pcapkit/foundation/registry/protocols.py +++ b/pcapkit/foundation/registry/protocols.py @@ -209,6 +209,8 @@ class rather than supplied by a caller, and this function is the funnel same class under the same name is silent. """ + if not isinstance(protocol, type): + raise RegistryError(f'protocol must be a class, not {protocol!r}') if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') diff --git a/pcapkit/foundation/traceflow/traceflow.py b/pcapkit/foundation/traceflow/traceflow.py index d365b8bb7..3490b91ec 100644 --- a/pcapkit/foundation/traceflow/traceflow.py +++ b/pcapkit/foundation/traceflow/traceflow.py @@ -234,6 +234,8 @@ def register_dumper(cls, format: 'str', dumper: 'ModuleDescriptor[Dumper] | Type """ if isinstance(dumper, ModuleDescriptor): dumper = dumper.klass + if not isinstance(dumper, type): + raise RegistryError(f'dumper must be a class, not {dumper!r}') if not issubclass(dumper, Dumper): raise RegistryError(f'dumper must be a Dumper subclass, not {dumper!r}') incumbent_entry = cls.__output__.get(format) diff --git a/tests/foundation/registry/test_foundation.py b/tests/foundation/registry/test_foundation.py index f32cd9be6..fa8ec9368 100644 --- a/tests/foundation/registry/test_foundation.py +++ b/tests/foundation/registry/test_foundation.py @@ -64,6 +64,51 @@ def test_engine_and_dumper_registration_wrappers(self) -> None: registry.register_traceflow_dumper('unit-trace-class', NotImplementedIO, ext='.unit') traceflow.assert_called_once_with('unit-trace-class', NotImplementedIO, '.unit') + def test_dumper_registrars_reject_a_non_class_with_registry_error(self) -> None: + """GitHub issue #1021: every dumper registrar raises ``RegistryError``. + + ``register_extractor_dumper`` and ``register_traceflow_dumper`` share a + signature and a docstring, and ``TraceFlow.register_dumper`` carried the same + bare ``issubclass`` guard as ``Extractor.register_dumper``, so a fix to one + left the other leaking ``TypeError``. Nothing is mocked: the point is what the + public wrappers really raise. + """ + import sys + import types + + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.foundation.extraction import Extractor + from pcapkit.foundation.registry import foundation as registry + from pcapkit.foundation.traceflow.traceflow import TraceFlow + from pcapkit.utilities.exceptions import RegistryError + + module = types.ModuleType('unit_foundation_non_class_mod') + module.NOT_A_CLASS = 42 + sys.modules['unit_foundation_non_class_mod'] = module + self.addCleanup(lambda: sys.modules.pop('unit_foundation_non_class_mod', None)) + descriptor = ModuleDescriptor('unit_foundation_non_class_mod', 'NOT_A_CLASS') + + registrars = { + 'Extractor.register_dumper': + lambda value: Extractor.register_dumper('unit-bad', value, '.bad'), + 'TraceFlow.register_dumper': + lambda value: TraceFlow.register_dumper('unit-bad', value, '.bad'), + 'register_extractor_dumper': + lambda value: registry.register_extractor_dumper('unit-bad', value, ext='.bad'), + 'register_traceflow_dumper': + lambda value: registry.register_traceflow_dumper('unit-bad', value, ext='.bad'), + } + for name, register in registrars.items(): + for label, value, expected in ( + ('instance', 42, 'must be a class'), + ('descriptor to non-class', descriptor, 'must be a class'), + ('wrong class', object, 'subclass'), + ): + with self.subTest(registrar=name, value=label): + with self.assertRaises(RegistryError) as caught: + register(value) + self.assertIn(expected, str(caught.exception)) + def test_callback_and_extractor_registration_wrappers(self) -> None: from pcapkit.foundation.registry import foundation as registry diff --git a/tests/foundation/registry/test_protocols.py b/tests/foundation/registry/test_protocols.py index db936da40..ca120944e 100644 --- a/tests/foundation/registry/test_protocols.py +++ b/tests/foundation/registry/test_protocols.py @@ -418,6 +418,30 @@ def test_register_protocol_validates_and_updates_registry(self) -> None: with self.assertRaises(RegistryError): registry.register_protocol(object) # type: ignore[arg-type] + def test_register_protocol_rejects_a_non_class_with_registry_error(self) -> None: + """GitHub issue #1021: a non-class raises ``RegistryError``, not ``TypeError``. + + ``ProtocolBase``'s metaclass is ``ABCMeta``-derived, so the bare + ``issubclass`` guard delegated to ``abc`` and the ``TypeError`` leaked + out of there before the ``raise`` was reached. + """ + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.foundation.registry import protocols as registry + from pcapkit.utilities.exceptions import RegistryError + + for label, value, expected in ( + ('instance', object(), 'must be a class'), + ('string', 'not-a-class', 'must be a class'), + ('none', None, 'must be a class'), + ('descriptor argument (not unwrapped here)', + ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw'), 'must be a class'), + ('wrong class', object, 'Protocol subclass'), + ): + with self.subTest(value=label): + with self.assertRaises(RegistryError) as caught: + registry.register_protocol(value) # type: ignore[arg-type] + self.assertIn(expected, str(caught.exception)) + def test_top_level_link_internet_and_transport_protocol_wrappers(self) -> None: # Members live in the per-transport registries GitHub issue #732 split # AppType into; the base class itself holds none. 3com-amp3 is registered diff --git a/tests/foundation/test_extraction.py b/tests/foundation/test_extraction.py index 677f592ae..9f150b0a7 100644 --- a/tests/foundation/test_extraction.py +++ b/tests/foundation/test_extraction.py @@ -406,6 +406,44 @@ def submit(self) -> tuple[object, ...]: internal(f'unit-1016-{kind}-bad', object) # type: ignore[arg-type] self.assertNotIn(f'unit-1016-{kind}-bad', store) + def test_register_helpers_reject_a_non_class_with_registry_error(self) -> None: + """GitHub issue #1021: a non-class raises ``RegistryError``, not ``TypeError``. + + Each guard was a bare ``issubclass(x, Base)``, which refuses a non-class + itself -- ``register_dumper`` directly (``Dumper`` has a plain ``type`` + metaclass), the other three from inside ``abc`` -- so the documented + ``RegistryError`` was unreachable. A wrong *class* still reaches the + subclass check and raises ``RegistryError`` as before. + """ + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.foundation.extraction import Extractor + from pcapkit.utilities.exceptions import RegistryError + + module = types.ModuleType('unit_extraction_non_class_mod') + module.NOT_A_CLASS = 42 + sys.modules['unit_extraction_non_class_mod'] = module + self.addCleanup(lambda: sys.modules.pop('unit_extraction_non_class_mod', None)) + descriptor = ModuleDescriptor('unit_extraction_non_class_mod', 'NOT_A_CLASS') + + registrars = { + 'dumper': lambda value: Extractor.register_dumper('unit-bad', value, '.bad'), + 'engine': lambda value: Extractor.register_engine('unit-bad', value), + 'reassembly': lambda value: Extractor.register_reassembly('unit-bad', value), + 'traceflow': lambda value: Extractor.register_traceflow('unit-bad', value), + } + for name, register in registrars.items(): + for label, value, expected in ( + ('instance', object(), 'must be a class'), + ('string', 'not-a-class', 'must be a class'), + ('none', None, 'must be a class'), + ('descriptor to non-class', descriptor, 'must be a class'), + ('wrong class', object, 'subclass'), + ): + with self.subTest(registrar=name, value=label): + with self.assertRaises(RegistryError) as caught: + register(value) + self.assertIn(expected, str(caught.exception)) + def test_register_engine_identity_guard(self) -> None: """GitHub issue #739: re-registering the same engine class is silent.