diff --git a/CHANGELOG.md b/CHANGELOG.md index 23c29b1e9..89f1477b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,7 @@ Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) an - conflicting TCP overlaps resolve first-write-wins, per [RFC 9293](https://datatracker.ietf.org/doc/html/rfc9293) section 3.10, where they had silently resolved last-write-wins ([#443](https://github.com/JarryShaw/PyPCAPKit/issues/443), [#478](https://github.com/JarryShaw/PyPCAPKit/pull/478)). A deliberate, narrow behaviour break: a conforming retransmission carries identical bytes. IP fragment reassembly keeps last-write-wins, because [RFC 791](https://datatracker.ietf.org/doc/html/rfc791) specifies the opposite resolution, and records the disagreement instead ([#482](https://github.com/JarryShaw/PyPCAPKit/pull/482)). - renames with no compatibility alias left behind: `HoleDiscriptor` is spelled `HoleDescriptor` and its package alias `TCP_HoleDiscriptor` is `TCP_HoleDescriptor` ([#350](https://github.com/JarryShaw/PyPCAPKit/pull/350)). - subclass registration is **opt-in** for `Engine`, `Reassembly`, `TraceFlow` and `dumpkit`'s `Dumper` ([#514](https://github.com/JarryShaw/PyPCAPKit/issues/514)). Each registers if and only if its registry keyword is given -- `engine=` for `Engine`, `protocol=` for `Reassembly` and `TraceFlow`, `fmt=` for `Dumper`. Previously an absent keyword fell back to the class' own name, so *every* subclass of the public class was registered, and declining meant subclassing the parallel `*Base` class under an alias, which every built-in does (hence the public classes had **0** subclasses against the `*Base` classes' 9, 5, 2 and 3). **This breaks out-of-tree code that subclasses one of the four and relies on the derived key**; pass the keyword, or call the matching `register_*` function. Nothing the library ships is affected, and the `*Base` classes remain importable. Two silent failures are now loud: an unrecognised class keyword raises `UnsupportedCall` instead of being swallowed by `**kwargs` (passing `name=` to a `Reassembly` subclass silently ignored the key, since `protocol=` is the real one), and so does `Dumper`'s `ext=` without `fmt=`. A class attribute is not an opt-in: `__engine_name__` and `__protocol_name__` still set the name a class reports, registered or not. Each metaclass gained a class-level `registry` property mirroring `EnumSchema.registry`, and a `Dumper` subclass no longer touches the filesystem while its `class` statement runs (inferring `fmt` from `kind` meant instantiating it against a `NamedTemporaryFile`). `Engine`'s keyword is `engine=` rather than the `name=` first shipped, because `name` cannot be a class keyword on Python 3.10: `mcls`, `name`, `bases` and `namespace` collide with `abc.ABCMeta.__new__`'s parameters (positional-or-keyword before 3.11, positional-only from 3.11), raising `TypeError` before the hook runs. Those four are the whole collision surface (measured on 3.10.21, 3.11.15 and 3.14.7), and `engine=`, `protocol=` and `fmt=` are outside it. There is no `name=` alias: a keyword that works on some interpreters and not others is the trap being removed. +- **a breaking change to** `Extractor.register_engine`, `register_reassembly` and `register_traceflow`, and the `register_extractor_*` wrappers over them: each now accepts only a subclass of the public `Engine`, `Reassembly` or `TraceFlow` ([#1016](https://github.com/JarryShaw/PyPCAPKit/issues/1016)). They had accepted a subclass of the matching `*Base` class since [#513](https://github.com/JarryShaw/PyPCAPKit/issues/513), which was a tolerance for pcapkit's own built-ins (all 13 derive from the base, none from the public class) rather than a contract: `*Base` is documented as internal, and the public class is the one carrying the registration hook. Third-party code that subclasses `EngineBase`, `ReassemblyBase` or `TraceFlowBase` directly and registers through these functions now gets `RegistryError` naming the public class; subclass the public class instead. The error messages are unchanged. The built-ins are unaffected because they are declared directly in `Extractor`'s class-body mappings as `ModuleDescriptor` literals and never pass through a registrar at all; a new internal path, `Extractor._register_internal_engine` and its two siblings, checks the base for programmatic internal registration and is what the [#513](https://github.com/JarryShaw/PyPCAPKit/issues/513) regression test now exercises. `register_protocol` is untouched: every in-house protocol derives from `ProtocolBase` and none from `Protocol`. - extraction is around 46% faster on a 1,117-frame HTTP capture, with byte-identical output ([#420](https://github.com/JarryShaw/PyPCAPKit/pull/420)). A reassembled datagram's payload is analysed on first read rather than eagerly, cutting IP reassembly's own cost by 90.7% and TCP's by 23.7% (IP reassembly submits a datagram for every frame, fragmented or not) ([#424](https://github.com/JarryShaw/PyPCAPKit/pull/424)). Flow tracing over the same capture went from 1416.6 ms to 744.0 ms, because the flow dumper handed each record to a `Frame` constructor that re-dissected the whole stack to return the bytes it had just been given; options are no longer parsed twice either ([#427](https://github.com/JarryShaw/PyPCAPKit/pull/427)). - **a breaking change to** `Extractor.engine` and `Extractor.record_header`: both are now declared to return `EngineBase[_P]` rather than `Engine`, and the private `_exeng` attribute follows. Both built-in engines subclass `EngineBase` directly, so neither was ever an `Engine`; the declaration now names the class the object has, and the `cast` calls that hid the gap now launder only the frame type parameter. Nothing is lost: `Engine` adds only the `__init_subclass__` registration hook, and the frame type parameter is preserved. Two things are visible to a type checker. Code passing the result to an `Engine`-typed parameter must now accept `EngineBase`; and because the old declarations were *unparameterised*, `engine.read_frame()` and `record_header().read_frame()` used to be `Any` and are now the extractor's own frame type, so an assignment that relied on `Any` there no longer type-checks ([#1022](https://github.com/JarryShaw/PyPCAPKit/issues/1022)). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index baf2f12d4..1731b223d 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -827,6 +827,24 @@ Changed four are the whole collision surface (measured on 3.10.21, 3.11.15 and 3.14.7), and ``engine=``, ``protocol=`` and ``fmt=`` are outside it. There is no ``name=`` alias: a keyword that works on some interpreters and not others is the trap being removed. +* **a breaking change to** ``Extractor.register_engine``, ``register_reassembly`` and + ``register_traceflow``, and the ``register_extractor_*`` wrappers over them: each + now accepts only a subclass of the public ``Engine``, ``Reassembly`` or + ``TraceFlow`` (:issue:`1016`). They had accepted a subclass of the matching + ``*Base`` class since :issue:`513`, which was a tolerance for pcapkit's own + built-ins (all 13 derive from the base, none from the public class) rather than + a contract: ``*Base`` is documented as internal, and the public class is the one + carrying the registration hook. Third-party code that subclasses ``EngineBase``, + ``ReassemblyBase`` or ``TraceFlowBase`` directly and registers through these + functions now gets ``RegistryError`` naming the public class; subclass the public + class instead. The error messages are unchanged. The built-ins are unaffected + because they are declared directly in ``Extractor``'s class-body mappings as + ``ModuleDescriptor`` literals and never pass through a registrar at all; a new + internal path, ``Extractor._register_internal_engine`` and its two siblings, + checks the base for programmatic internal registration and is what the + :issue:`513` regression test now exercises. ``register_protocol`` is + untouched: every in-house protocol derives from ``ProtocolBase`` and none from + ``Protocol``. * extraction is around 46% faster on a 1,117-frame HTTP capture, with byte-identical output (:pr:`420`). A reassembled datagram's payload is analysed on first read rather than eagerly, cutting IP reassembly's own cost by 90.7% and TCP's by 23.7% diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index 93b8a39ce..60c4173b7 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 -156 entries, and no entry carries an inline kind label:: +157 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/engines/engine.py b/pcapkit/foundation/engines/engine.py index a13c399e1..226212c02 100644 --- a/pcapkit/foundation/engines/engine.py +++ b/pcapkit/foundation/engines/engine.py @@ -53,7 +53,7 @@ def module(cls) -> 'str': return cls.__module__ @property - def registry(cls) -> 'dict[str, ModuleDescriptor[Engine] | Type[Engine]]': + def registry(cls) -> 'dict[str, ModuleDescriptor[EngineBase] | Type[EngineBase]]': """Mapping of engine names to engine classes. Note: diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index 58ea481e4..a8c32ed74 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -223,9 +223,11 @@ class Extractor(Generic[_P]): }, ) # type: DefaultDict[str, tuple[ModuleDescriptor[Dumper] | Type[Dumper], str | None]] - #: Engine mapping for extracting frames. The values should be a tuple representing - #: the module name and class name, or an :class:`~pcapkit.foundation.engines.engine.Engine` - #: subclass. + #: Engine mapping for extracting frames. The values should be a module descriptor + #: or an :class:`~pcapkit.foundation.engines.engine.EngineBase` subclass: every + #: built-in derives from the base directly, and the public ``register_engine`` + #: additionally admits only :class:`~pcapkit.foundation.engines.engine.Engine` + #: subclasses. __engine__ = { 'scapy': ModuleDescriptor('pcapkit.foundation.engines.scapy', 'Scapy'), 'dpkt': ModuleDescriptor('pcapkit.foundation.engines.dpkt', 'DPKT'), @@ -239,23 +241,27 @@ class Extractor(Generic[_P]): # explains how ``import_test`` tells them apart. 'pcap_ct': ModuleDescriptor('pcapkit.foundation.engines.pcap_ct', 'PCAP_CT'), 'pypcapfile': ModuleDescriptor('pcapkit.foundation.engines.pypcapfile', 'PyPCAPFile'), - } # type: dict[str, ModuleDescriptor[Engine] | Type[Engine]] + } # type: dict[str, ModuleDescriptor[EngineBase] | Type[EngineBase]] - #: Reassembly support mapping for extracting frames. The values should be a tuple - #: representing the module name and class name, or a :class:`~pcapkit.foundation.reassembly.reassembly.Reassembly` - #: subclass. + #: Reassembly support mapping for extracting frames. The values should be a module + #: descriptor or a :class:`~pcapkit.foundation.reassembly.reassembly.ReassemblyBase` + #: subclass: every built-in derives from the base directly, and the public + #: ``register_reassembly`` additionally admits only + #: :class:`~pcapkit.foundation.reassembly.reassembly.Reassembly` subclasses. __reassembly__ = { 'ipv4': ModuleDescriptor('pcapkit.foundation.reassembly.ipv4', 'IPv4'), 'ipv6': ModuleDescriptor('pcapkit.foundation.reassembly.ipv6', 'IPv6'), 'tcp': ModuleDescriptor('pcapkit.foundation.reassembly.tcp', 'TCP'), - } # type: dict[str, ModuleDescriptor[Reassembly] | Type[Reassembly]] + } # type: dict[str, ModuleDescriptor[ReassemblyBase] | Type[ReassemblyBase]] - #: Flow tracing support mapping for extracting frames. The values should be a tuple - #: representing the module name and class name, or a :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlow` - #: subclass. + #: Flow tracing support mapping for extracting frames. The values should be a module + #: descriptor or a :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlowBase` + #: subclass: every built-in derives from the base directly, and the public + #: ``register_traceflow`` additionally admits only + #: :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlow` subclasses. __traceflow__ = { 'tcp': ModuleDescriptor('pcapkit.foundation.traceflow.tcp', 'TCP'), - } # type: dict[str, ModuleDescriptor[TraceFlow] | Type[TraceFlow]] + } # type: dict[str, ModuleDescriptor[TraceFlowBase] | Type[TraceFlowBase]] ########################################################################## # Properties. @@ -425,17 +431,48 @@ def register_engine(cls, name: 'str', engine: 'ModuleDescriptor[Engine] | Type[E the identity guard GitHub issue :issue:`718` gave the code-keyed registrars, extended here by GitHub issue :issue:`739`. + Arguments: + name: engine name + engine: module descriptor or an :class:`~pcapkit.foundation.engines.engine.Engine` + subclass; a class deriving only from + :class:`~pcapkit.foundation.engines.engine.EngineBase` is refused, as + that is the base for pcapkit's own engines, which the library + registers itself (GitHub issue :issue:`1016`) + + Raises: + RegistryError: If ``engine`` is not a class, or not an ``Engine`` subclass. + + Warns: + RegistryWarning: If a different class is already registered under + ``name``; it is overwritten. + + """ + if isinstance(engine, ModuleDescriptor): + engine = engine.klass + # 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; + # they go through :meth:`_register_internal_engine` instead, which checks the + # base. Checking the base here is what #513 did, and #1016 narrowed it back. + if not issubclass(engine, Engine): + raise RegistryError(f'engine must be an Engine subclass, not {engine!r}') + cls._register_internal_engine(name, engine) + + @classmethod + def _register_internal_engine(cls, name: 'str', engine: 'ModuleDescriptor[EngineBase] | Type[EngineBase]') -> 'None': + """Register an engine for pcapkit's own use, bypassing the public guard. + + Unlike :meth:`register_engine`, accepts a class deriving only from + :class:`~pcapkit.foundation.engines.engine.EngineBase`, which is how every + built-in engine is written. Not part of the public API. + Arguments: name: engine name engine: module descriptor or an :class:`~pcapkit.foundation.engines.engine.EngineBase` subclass - (an :class:`~pcapkit.foundation.engines.engine.Engine` subclass - is one too, but no built-in engine is: they all derive from the - base directly); the ``Type[Engine]`` hint in the signature is - narrower than this check Raises: - RegistryError: If ``engine`` is not a class, or not an ``EngineBase`` subclass. + RegistryError: If ``engine`` is not an ``EngineBase`` subclass. Warns: RegistryWarning: If a different class is already registered under @@ -444,14 +481,8 @@ def register_engine(cls, name: 'str', engine: 'ModuleDescriptor[Engine] | Type[E """ if isinstance(engine, ModuleDescriptor): engine = engine.klass - # NOTE: checked against ``EngineBase`` rather than ``Engine``: every built-in - # engine subclasses the base directly (``engines/pcap.py`` declares - # ``class PCAP(EngineBase[Frame])``) precisely so that it is *not* - # auto-registered by ``Engine.__init_subclass__``, which made this check - # reject pcapkit's own classes. ``Engine`` is itself an ``EngineBase``, so - # this only widens. See #513. if not issubclass(engine, EngineBase): - raise RegistryError(f'engine must be an Engine subclass, not {engine!r}') + raise RegistryError(f'Extractor._register_internal_engine: engine must be an EngineBase subclass, not {engine!r}') incumbent = cls.__engine__.get(name) if incumbent is not None and incumbent is not engine: warn(f'engine {name} already registered, overwriting', RegistryWarning) @@ -474,13 +505,13 @@ def register_reassembly(cls, protocol: 'str', reassembly: 'ModuleDescriptor[Reas Arguments: protocol: protocol name reassembly: module descriptor or a - :class:`~pcapkit.foundation.reassembly.reassembly.ReassemblyBase` - subclass (a :class:`~pcapkit.foundation.reassembly.reassembly.Reassembly` - subclass is one too, but no built-in reassembly class is: they all - derive from the base directly) + :class:`~pcapkit.foundation.reassembly.reassembly.Reassembly` subclass; a class deriving + only from :class:`~pcapkit.foundation.reassembly.reassembly.ReassemblyBase` is refused, as + that is the base for pcapkit's own classes, which the library registers + itself (GitHub issue :issue:`1016`) Raises: - RegistryError: If ``reassembly`` is not a class, or not a ``ReassemblyBase`` subclass. + RegistryError: If ``reassembly`` is not a class, or not a ``Reassembly`` subclass. Warns: RegistryWarning: If a different class is already registered under @@ -489,10 +520,38 @@ def register_reassembly(cls, protocol: 'str', reassembly: 'ModuleDescriptor[Reas """ if isinstance(reassembly, ModuleDescriptor): reassembly = reassembly.klass - # NOTE: ``ReassemblyBase`` rather than ``Reassembly``, for the reason given in - # :meth:`register_engine` above -- see #513. - if not issubclass(reassembly, ReassemblyBase): + # NOTE: ``Reassembly`` rather than ``ReassemblyBase``, for the reason given in + # :meth:`register_engine` above -- see #1016. Built-ins go through + # :meth:`_register_internal_reassembly`. + if not issubclass(reassembly, Reassembly): raise RegistryError(f'reassembly must be a Reassembly subclass, not {reassembly!r}') + cls._register_internal_reassembly(protocol, reassembly) + + @classmethod + def _register_internal_reassembly(cls, protocol: 'str', reassembly: 'ModuleDescriptor[ReassemblyBase] | Type[ReassemblyBase]') -> 'None': + """Register a reassembly class for pcapkit's own use, bypassing the public guard. + + Unlike :meth:`register_reassembly`, accepts a class deriving only from + :class:`~pcapkit.foundation.reassembly.reassembly.ReassemblyBase`, which is how every + built-in is written. Not part of the public API. + + Arguments: + protocol: protocol name + reassembly: module descriptor or a + :class:`~pcapkit.foundation.reassembly.reassembly.ReassemblyBase` subclass + + Raises: + RegistryError: If ``reassembly`` is not a ``ReassemblyBase`` subclass. + + Warns: + RegistryWarning: If a different class is already registered under + ``protocol``; it is overwritten. + + """ + if isinstance(reassembly, ModuleDescriptor): + reassembly = reassembly.klass + if not issubclass(reassembly, ReassemblyBase): + raise RegistryError(f'Extractor._register_internal_reassembly: reassembly must be a ReassemblyBase subclass, not {reassembly!r}') incumbent = cls.__reassembly__.get(protocol) if incumbent is not None and incumbent is not reassembly: warn(f'reassembly {protocol} already registered, overwriting', RegistryWarning) @@ -515,13 +574,13 @@ def register_traceflow(cls, protocol: 'str', traceflow: 'ModuleDescriptor[TraceF Arguments: protocol: protocol name traceflow: module descriptor or a - :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlowBase` - subclass (a :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlow` - subclass is one too, but no built-in flow tracing class is: they all - derive from the base directly) + :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlow` subclass; a class deriving + only from :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlowBase` is refused, as + that is the base for pcapkit's own classes, which the library registers + itself (GitHub issue :issue:`1016`) Raises: - RegistryError: If ``traceflow`` is not a class, or not a ``TraceFlowBase`` subclass. + RegistryError: If ``traceflow`` is not a class, or not a ``TraceFlow`` subclass. Warns: RegistryWarning: If a different class is already registered under @@ -530,10 +589,38 @@ def register_traceflow(cls, protocol: 'str', traceflow: 'ModuleDescriptor[TraceF """ if isinstance(traceflow, ModuleDescriptor): traceflow = traceflow.klass - # NOTE: ``TraceFlowBase`` rather than ``TraceFlow``, for the reason given in - # :meth:`register_engine` above -- see #513. - if not issubclass(traceflow, TraceFlowBase): + # NOTE: ``TraceFlow`` rather than ``TraceFlowBase``, for the reason given in + # :meth:`register_engine` above -- see #1016. Built-ins go through + # :meth:`_register_internal_traceflow`. + if not issubclass(traceflow, TraceFlow): raise RegistryError(f'traceflow must be a TraceFlow subclass, not {traceflow!r}') + cls._register_internal_traceflow(protocol, traceflow) + + @classmethod + def _register_internal_traceflow(cls, protocol: 'str', traceflow: 'ModuleDescriptor[TraceFlowBase] | Type[TraceFlowBase]') -> 'None': + """Register a traceflow class for pcapkit's own use, bypassing the public guard. + + Unlike :meth:`register_traceflow`, accepts a class deriving only from + :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlowBase`, which is how every + built-in is written. Not part of the public API. + + Arguments: + protocol: protocol name + traceflow: module descriptor or a + :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlowBase` subclass + + Raises: + RegistryError: If ``traceflow`` is not a ``TraceFlowBase`` subclass. + + Warns: + RegistryWarning: If a different class is already registered under + ``protocol``; it is overwritten. + + """ + if isinstance(traceflow, ModuleDescriptor): + traceflow = traceflow.klass + if not issubclass(traceflow, TraceFlowBase): + raise RegistryError(f'Extractor._register_internal_traceflow: traceflow must be a TraceFlowBase subclass, not {traceflow!r}') incumbent = cls.__traceflow__.get(protocol) if incumbent is not None and incumbent is not traceflow: warn(f'traceflow {protocol} already registered, overwriting', RegistryWarning) diff --git a/pcapkit/foundation/reassembly/reassembly.py b/pcapkit/foundation/reassembly/reassembly.py index 2e48160d2..60e105420 100644 --- a/pcapkit/foundation/reassembly/reassembly.py +++ b/pcapkit/foundation/reassembly/reassembly.py @@ -81,7 +81,7 @@ def protocol(cls) -> 'Type[ProtocolBase]': return protocol_registry.get(cls.name.upper(), Raw) @property - def registry(cls) -> 'dict[str, ModuleDescriptor[Reassembly] | Type[Reassembly]]': + def registry(cls) -> 'dict[str, ModuleDescriptor[ReassemblyBase] | Type[ReassemblyBase]]': """Mapping of protocol names to reassembly classes. Note: diff --git a/pcapkit/foundation/registry/foundation.py b/pcapkit/foundation/registry/foundation.py index 5c52f5924..0eeb1fb0c 100644 --- a/pcapkit/foundation/registry/foundation.py +++ b/pcapkit/foundation/registry/foundation.py @@ -72,10 +72,9 @@ def register_extractor_engine(name: 'str', module: 'ModuleDescriptor[Engine] | T Arguments: name: engine name module: module name or module descriptor or an - :class:`~pcapkit.foundation.engines.engine.EngineBase` subclass - (an :class:`~pcapkit.foundation.engines.engine.Engine` subclass - is one too, but no built-in engine is: they all derive from the - base directly) + :class:`~pcapkit.foundation.engines.engine.Engine` subclass (a class + deriving only from :class:`~pcapkit.foundation.engines.engine.EngineBase` + is refused: that is the base for pcapkit's own engines) class\_: class name """ @@ -290,10 +289,9 @@ def register_extractor_reassembly(protocol: 'str', module: 'str | ModuleDescript Arguments: protocol: protocol name module: module name or module descriptor or a - :class:`~pcapkit.foundation.reassembly.reassembly.ReassemblyBase` subclass - (a :class:`~pcapkit.foundation.reassembly.reassembly.Reassembly` - subclass is one too, but no built-in reassembly class is: they all - derive from the base directly) + :class:`~pcapkit.foundation.reassembly.reassembly.Reassembly` subclass (a class + deriving only from :class:`~pcapkit.foundation.reassembly.reassembly.ReassemblyBase` + is refused: that is the base for pcapkit's own classes) class\_: class name """ @@ -325,10 +323,9 @@ def register_extractor_traceflow(protocol: 'str', module: 'str | ModuleDescripto Arguments: protocol: protocol name module: module name or module descriptor or a - :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlowBase` subclass - (a :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlow` - subclass is one too, but no built-in flow tracing class is: they all - derive from the base directly) + :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlow` subclass (a class + deriving only from :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlowBase` + is refused: that is the base for pcapkit's own classes) class\_: class name """ diff --git a/pcapkit/foundation/traceflow/traceflow.py b/pcapkit/foundation/traceflow/traceflow.py index 073280311..d365b8bb7 100644 --- a/pcapkit/foundation/traceflow/traceflow.py +++ b/pcapkit/foundation/traceflow/traceflow.py @@ -82,7 +82,7 @@ def protocol(cls) -> 'Type[ProtocolBase]': return protocol_registry.get(cls.name.upper(), Raw) @property - def registry(cls) -> 'dict[str, ModuleDescriptor[TraceFlow] | Type[TraceFlow]]': + def registry(cls) -> 'dict[str, ModuleDescriptor[TraceFlowBase] | Type[TraceFlowBase]]': """Mapping of protocol names to flow tracing classes. Note: diff --git a/tests/foundation/registry/test_foundation.py b/tests/foundation/registry/test_foundation.py index 54df12f3a..f32cd9be6 100644 --- a/tests/foundation/registry/test_foundation.py +++ b/tests/foundation/registry/test_foundation.py @@ -104,7 +104,7 @@ def test_callback_and_extractor_registration_wrappers(self) -> None: self.assertIsInstance(register.call_args.args[1], registry.ModuleDescriptor) def test_registration_accepts_pcapkit_own_builtin_classes(self) -> None: - """The built-ins pass the ``issubclass`` gate, and non-subclasses still fail. + """The built-ins pass the internal ``issubclass`` gate, and non-subclasses still fail. This is the regression for GitHub issue #513. Every sibling test in this module mocks ``Extractor.register_*`` away, so none of them reaches the @@ -124,52 +124,106 @@ def test_registration_accepts_pcapkit_own_builtin_classes(self) -> None: Deliberately does **not** mock, because the point is to exercise the gate. """ + # NOTE: GitHub issue #1016 moved this assertion. #513 was fixed by widening the + # public ``register_*`` guards to the ``*Base`` classes; #1016 narrowed them back + # to ``Engine``/``Reassembly``/``TraceFlow`` (the registration surface third + # parties extend), so the public door now refuses the built-ins -- see + # ``test_public_registration_rejects_base_only_classes`` below. What #513 bought + # -- that pcapkit's *own* classes are registrable through a real, unmocked gate -- + # is kept by pointing this test at the internal ``Extractor._register_internal_*`` + # path instead of the public wrappers. The subject classes and the read-back + # are unchanged. + # # NOTE: imported inside the test because ``setUp`` purges ``pcapkit`` from # ``sys.modules``, which is why every sibling test imports locally too. # ``Extractor`` is needed by name here so the registries can be read back. from pcapkit.foundation.engines.pcap import PCAP as PCAP_Engine from pcapkit.foundation.extraction import Extractor from pcapkit.foundation.reassembly.ipv4 import IPv4 as IPv4_Reassembly - from pcapkit.foundation.registry.foundation import (register_extractor_engine, - register_extractor_reassembly, - register_extractor_traceflow) from pcapkit.foundation.traceflow.tcp import TCP as TCP_TraceFlow from pcapkit.utilities.exceptions import RegistryError - for name, func, klass, store in ( - ('engine', register_extractor_engine, PCAP_Engine, + cases = ( + ('engine', Extractor._register_internal_engine, PCAP_Engine, Extractor.__engine__), - ('reassembly', register_extractor_reassembly, IPv4_Reassembly, + ('reassembly', Extractor._register_internal_reassembly, IPv4_Reassembly, Extractor.__reassembly__), - ('traceflow', register_extractor_traceflow, TCP_TraceFlow, + ('traceflow', Extractor._register_internal_traceflow, TCP_TraceFlow, Extractor.__traceflow__), - ): - with self.subTest(kind=name, accepted=True): - # NOTE: a distinct key per call, so this neither collides with the - # built-in registrations already present nor emits the - # ``already registered, overwriting`` warning. - key = f'unit-513-{name}' - func(key, klass) - - # NOTE: the registry is read back rather than merely asserting that - # no exception escaped. Without this, the test passes against a - # helper that runs the ``issubclass`` gate and then silently drops - # the class -- measured, not hypothetical: inserting a bare - # ``return`` after the check and before the registry write leaves - # this test reporting ``1 passed, 6 subtests passed``, exit 0. - self.assertIn(key, store) - self.assertIs(store[key], klass) - - # And the gate still rejects something that is genuinely not a subclass -- - # the widening must not have turned the check into a no-op. - for name, func in ( - ('engine', register_extractor_engine), - ('reassembly', register_extractor_reassembly), - ('traceflow', register_extractor_traceflow), - ): - with self.subTest(kind=name, accepted=False): - with self.assertRaises(RegistryError): - func(f'unit-513-reject-{name}', int) # type: ignore[arg-type] + ) + + # NOTE: ``registry`` is one global table, so the writes are rolled back. + with mock.patch.dict(Extractor.__engine__), \ + mock.patch.dict(Extractor.__reassembly__), \ + mock.patch.dict(Extractor.__traceflow__): + for name, func, klass, store in cases: + with self.subTest(kind=name, accepted=True): + # NOTE: a distinct key per call, so this neither collides with the + # built-in registrations already present nor emits the + # ``already registered, overwriting`` warning. + key = f'unit-513-{name}' + func(key, klass) + + # NOTE: the registry is read back rather than merely asserting that + # no exception escaped. Without this, the test passes against a + # helper that runs the ``issubclass`` gate and then silently drops + # the class -- measured, not hypothetical: inserting a bare + # ``return`` after the check and before the registry write leaves + # this test reporting ``1 passed, 6 subtests passed``, exit 0. + self.assertIn(key, store) + self.assertIs(store[key], klass) + + # And the gate still rejects something that is genuinely not a subclass -- + # the internal path must not have turned the check into a no-op. + for name, func, _klass, _store in cases: + with self.subTest(kind=name, accepted=False): + with self.assertRaises(RegistryError): + func(f'unit-513-reject-{name}', int) # type: ignore[arg-type] + + def test_public_registration_rejects_base_only_classes(self) -> None: + """GitHub issue #1016: the public door takes the public class, not the ``*Base``. + + The built-ins derive from ``EngineBase``/``ReassemblyBase``/``TraceFlowBase`` + directly, and third parties extend ``Engine``/``Reassembly``/``TraceFlow`` + because those carry the registration hook. The ``register_extractor_*`` + wrappers therefore refuse a base-only class -- the real built-ins here -- + with a message naming the public class, and leave the registry untouched. + Unmocked, for the same reason as the #513 test above. + + """ + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.foundation.engines.pcap import PCAP as PCAP_Engine + from pcapkit.foundation.extraction import Extractor + from pcapkit.foundation.reassembly.ipv4 import IPv4 as IPv4_Reassembly + from pcapkit.foundation.registry.foundation import (register_extractor_engine, + register_extractor_reassembly, + register_extractor_traceflow) + from pcapkit.foundation.traceflow.tcp import TCP as TCP_TraceFlow + from pcapkit.utilities.exceptions import RegistryError + + with mock.patch.dict(Extractor.__engine__), \ + mock.patch.dict(Extractor.__reassembly__), \ + mock.patch.dict(Extractor.__traceflow__): + for name, func, klass, store, message in ( + ('engine', register_extractor_engine, PCAP_Engine, + Extractor.__engine__, 'engine must be an Engine subclass'), + ('reassembly', register_extractor_reassembly, IPv4_Reassembly, + Extractor.__reassembly__, 'reassembly must be a Reassembly subclass'), + ('traceflow', register_extractor_traceflow, TCP_TraceFlow, + Extractor.__traceflow__, 'traceflow must be a TraceFlow subclass'), + ): + key = f'unit-1016-{name}' + with self.subTest(kind=name, form='class'): + with self.assertRaisesRegex(RegistryError, message): + func(key, klass) # type: ignore[arg-type] + self.assertNotIn(key, store) + + # A descriptor is unwrapped and then checked, not waved through. + with self.subTest(kind=name, form='descriptor'): + descriptor = ModuleDescriptor(klass.__module__, klass.__name__) + with self.assertRaisesRegex(RegistryError, message): + func(key, descriptor) # type: ignore[arg-type] + self.assertNotIn(key, store) if __name__ == '__main__': diff --git a/tests/foundation/test_extraction.py b/tests/foundation/test_extraction.py index 1eb788a1c..677f592ae 100644 --- a/tests/foundation/test_extraction.py +++ b/tests/foundation/test_extraction.py @@ -295,6 +295,117 @@ def submit(self) -> tuple[object, ...]: Extractor.register_traceflow('unit-trace-descriptor', ModuleDescriptor('unit_extraction_trace_mod', 'UnitTraceFlow')) + def test_public_register_guards_take_public_class_and_internal_path_takes_base(self) -> None: + """GitHub issue #1016: ``register_*`` checks the public class, the internal path the base. + + A subclass of the public ``Engine``/``Reassembly``/``TraceFlow`` is still + accepted by the public door; a class deriving only from the ``*Base`` is + refused there, with a message that names the public class, and accepted by + ``Extractor._register_internal_*``. The ``registry`` is one global table, so + every write here is rolled back. + + """ + from pcapkit.foundation.engines.engine import Engine, EngineBase + from pcapkit.foundation.extraction import Extractor + from pcapkit.foundation.reassembly.reassembly import Reassembly, ReassemblyBase + from pcapkit.foundation.traceflow.traceflow import TraceFlow, TraceFlowBase + from pcapkit.utilities.exceptions import RegistryError + + class BaseOnlyEngine(EngineBase[str]): + __engine_name__ = 'BaseOnlyEngine' + __engine_module__ = __name__ + + def run(self) -> None: + pass + + def read_frame(self) -> str: + return 'frame' + + class PublicEngine(Engine[str]): + __engine_name__ = 'PublicEngine' + __engine_module__ = __name__ + + def run(self) -> None: + pass + + def read_frame(self) -> str: + return 'frame' + + class BaseOnlyReassembly(ReassemblyBase[object, object, tuple[str], object]): + def reassembly(self, info: object) -> None: + pass + + def submit(self, buf: object, **kwargs: object) -> list[object]: + return [] + + class PublicReassembly(Reassembly[object, object, tuple[str], object]): + def reassembly(self, info: object) -> None: + pass + + def submit(self, buf: object, **kwargs: object) -> list[object]: + return [] + + class BaseOnlyTraceFlow(TraceFlowBase[str, object, object, object]): + def dump(self, packet: object) -> None: + pass + + def trace(self, packet: object, *, output: bool = False): + return object() if output else 'trace' + + def submit(self) -> tuple[object, ...]: + return () + + class PublicTraceFlow(TraceFlow[str, object, object, object]): + def dump(self, packet: object) -> None: + pass + + def trace(self, packet: object, *, output: bool = False): + return object() if output else 'trace' + + def submit(self) -> tuple[object, ...]: + return () + + cases = ( + ('engine', Extractor.register_engine, Extractor._register_internal_engine, + BaseOnlyEngine, PublicEngine, Extractor.__engine__, + 'engine must be an Engine subclass'), + ('reassembly', Extractor.register_reassembly, Extractor._register_internal_reassembly, + BaseOnlyReassembly, PublicReassembly, Extractor.__reassembly__, + 'reassembly must be a Reassembly subclass'), + ('traceflow', Extractor.register_traceflow, Extractor._register_internal_traceflow, + BaseOnlyTraceFlow, PublicTraceFlow, Extractor.__traceflow__, + 'traceflow must be a TraceFlow subclass'), + ) + + with mock.patch.dict(Extractor.__engine__), \ + mock.patch.dict(Extractor.__reassembly__), \ + mock.patch.dict(Extractor.__traceflow__): + for kind, public, internal, base_only, public_cls, store, message in cases: + with self.subTest(kind=kind): + # The public door refuses a base-only class, leaving no trace. + with self.assertRaisesRegex(RegistryError, message): + public(f'unit-1016-{kind}-base', base_only) # type: ignore[arg-type] + self.assertNotIn(f'unit-1016-{kind}-base', store) + + # A subclass of the public class is still accepted. + public(f'unit-1016-{kind}-public', public_cls) # type: ignore[arg-type] + self.assertIs(store[f'unit-1016-{kind}-public'], public_cls) + + # The internal path accepts the base-only class, and the public one too. + internal(f'unit-1016-{kind}-internal', base_only) + self.assertIs(store[f'unit-1016-{kind}-internal'], base_only) + internal(f'unit-1016-{kind}-internal-public', public_cls) + self.assertIs(store[f'unit-1016-{kind}-internal-public'], public_cls) + + # And it is still a gate: it keeps the overwrite warning, and + # refuses what is not a ``*Base`` subclass. + with mock.patch('pcapkit.foundation.extraction.warn') as warn: + internal(f'unit-1016-{kind}-internal', public_cls) + warn.assert_called_once() + with self.assertRaises(RegistryError): + internal(f'unit-1016-{kind}-bad', object) # type: ignore[arg-type] + self.assertNotIn(f'unit-1016-{kind}-bad', store) + def test_register_engine_identity_guard(self) -> None: """GitHub issue #739: re-registering the same engine class is silent.