diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e8bc02ad..23c29b1e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -86,6 +86,7 @@ Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) an - 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. - 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)). #### Fixed diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 27d13b53a..baf2f12d4 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -835,6 +835,18 @@ Changed 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 (:pr:`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 (:issue:`1022`). Fixed ~~~~~ diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index f0ccb9a2e..e1b5f0b90 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 -155 entries, and no entry carries an inline kind label:: +156 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 45850be05..58ea481e4 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -180,7 +180,7 @@ class Extractor(Generic[_P]): #: Extraction engine name. _exnam: 'Engines' #: Extraction engine instance. - _exeng: 'Engine[_P]' + _exeng: 'EngineBase[_P]' #: Input file object. _ifile: 'BufferedReader' @@ -351,7 +351,7 @@ def trace(self) -> 'TraceFlowData': raise UnsupportedCall("'Extractor(trace=False)' object has no attribute 'trace'") @property - def engine(self) -> 'Engine': + def engine(self) -> 'EngineBase[_P]': """PCAP extraction engine.""" return self._exeng @@ -606,10 +606,10 @@ def run(self) -> 'None': # pylint: disable=inconsistent-return-statements if self._magic in PCAP_Engine.MAGIC_NUMBER: logger.debug('magic number %r identifies a PCAP file', self._magic) - self._exeng = cast('Engine[_P]', PCAP_Engine(self)) + self._exeng = cast('EngineBase[_P]', PCAP_Engine(self)) elif self._magic in PCAPNG_Engine.MAGIC_NUMBER: logger.debug('magic number %r identifies a PCAP-NG file', self._magic) - self._exeng = cast('Engine[_P]', PCAPNG_Engine(self)) + self._exeng = cast('EngineBase[_P]', PCAPNG_Engine(self)) else: raise FormatError(f'unknown file format: {self._magic!r}') @@ -735,14 +735,13 @@ def make_name(cls, fin: 'str | IO[bytes]' = 'in.pcap', fout: 'str' = 'out', return ifnm, ofnm, fmt, ext, files - def record_header(self) -> 'Engine': + def record_header(self) -> 'EngineBase[_P]': """Read global header. The method will parse the PCAP global header and save the parsed result to its extraction context. Information such as PCAP version, data link - layer protocol type, nanosecond flag and byteorder will also be save - the current :class:`~pcapkit.foundation.engines.engine.Engine` instance - as well. + layer protocol type, nanosecond flag and byteorder are also saved on the + returned engine instance. If TCP flow tracing is enabled, the nanosecond flag and byteorder will be used for the output PCAP file of the traced TCP flows. diff --git a/tests/foundation/test_extraction.py b/tests/foundation/test_extraction.py index 97acf9393..1eb788a1c 100644 --- a/tests/foundation/test_extraction.py +++ b/tests/foundation/test_extraction.py @@ -1,11 +1,14 @@ from __future__ import annotations +import ast import importlib.util +import inspect import io import pathlib import sys import tempfile import types +import typing import unittest import warnings from unittest import mock @@ -158,6 +161,52 @@ def test_properties_success_and_unsupported_paths(self) -> None: with self.assertRaises(UnsupportedCall): _ = unsupported.trace + def test_declared_engine_type_is_satisfied_by_the_built_in_engines(self) -> None: + """``_exeng``, ``engine`` and ``record_header`` must name a type the engine holds. + + Both built-in engines subclass :class:`~pcapkit.foundation.engines.engine.EngineBase` + directly (so that they are not auto-registered), hence neither is an + :class:`~pcapkit.foundation.engines.engine.Engine`. The three declarations + used to say ``Engine`` and two ``cast`` calls hid the gap (#1022). + """ + import pcapkit.foundation.extraction as module + from pcapkit.foundation.engines.pcap import PCAP + from pcapkit.foundation.engines.pcapng import PCAPNG + from pcapkit.foundation.extraction import Extractor + + # ``_exeng`` is annotated under ``TYPE_CHECKING`` only, so it is not visible + # to ``get_type_hints``: read the annotation out of the module source. + tree = ast.parse(inspect.getsource(module)) + klass = next(node for node in tree.body + if isinstance(node, ast.ClassDef) and node.name == 'Extractor') + found = [node for node in ast.walk(klass) + if isinstance(node, ast.AnnAssign) + and isinstance(node.target, ast.Name) and node.target.id == '_exeng'] + self.assertEqual(len(found), 1) + node = found[0].annotation + # quoted (``'EngineBase[_P]'``) or bare (``EngineBase[_P]``) spelling + source = node.value if isinstance(node, ast.Constant) else ast.unparse(node) + attribute = eval(source, vars(module)) # pylint: disable=eval-used + + declared = { + '_exeng': attribute, + 'engine': typing.get_type_hints(Extractor.engine.fget, vars(module))['return'], + 'record_header': typing.get_type_hints(Extractor.record_header, vars(module))['return'], + } + # The frame type parameter is what lets mypy check ``read_frame()`` against + # ``Extractor.__next__``'s ``_P``; a bare class would silently make it ``Any``. + for name, hint in declared.items(): + with self.subTest(declaration=name, check='parameterised'): + self.assertTrue(typing.get_args(hint), + f'{name} is declared {hint!r}, which drops the frame type parameter') + for name, hint in declared.items(): + origin = typing.get_origin(hint) or hint + for engine in (PCAP, PCAPNG): + with self.subTest(declaration=name, engine=engine.__name__): + self.assertTrue(issubclass(engine, origin), + f'{name} is declared {hint!r}, which {engine.__name__} ' + 'does not subclass') + def test_register_helpers_validate_descriptors_and_overwrite_warnings(self) -> None: from pcapkit.corekit.module import ModuleDescriptor from pcapkit.dumpkit.null import NotImplementedIO