Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
21 changes: 21 additions & 0 deletions docs/source/changelog/1.5.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
-----------------
Expand Down
2 changes: 1 addition & 1 deletion docs/source/contributing/conventions/process.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions pcapkit/foundation/extraction.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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`.
Expand Down Expand Up @@ -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`.
Expand Down
2 changes: 2 additions & 0 deletions pcapkit/foundation/registry/protocols.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}')

Expand Down
2 changes: 2 additions & 0 deletions pcapkit/foundation/traceflow/traceflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
45 changes: 45 additions & 0 deletions tests/foundation/registry/test_foundation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
24 changes: 24 additions & 0 deletions tests/foundation/registry/test_protocols.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 38 additions & 0 deletions tests/foundation/test_extraction.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
Loading