From 37c21438ad72f8284736084ed497aa343b72e4e9 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 26 Sep 2026 01:06:19 -0400 Subject: [PATCH] fix(reg,corekit): retype the registry NULL sentinel and raise ProtocolError from ModuleDescriptor.klass (#832) (#833) `NULL` was a plain `str` (`'(null)'`) compared by identity, defined independently in `protocols.py` (13 uses) and `foundation.py` (7 uses), so an equal-but-distinct `'(null)'` from a caller took a different branch than the sentinel itself depending on string interning. That let an omitted `class_` reach `ModuleDescriptor.klass`'s bare `getattr` and surface as `AttributeError: module 'X' has no attribute '(null)'`, and the same bare `AttributeError` leaked for any bad class name across all nine `register_*` call sites that build a descriptor. Add `NullType`/`NULL` to `pcapkit.corekit.module`, the module both registries already import `ModuleDescriptor` from, giving the sentinel a type no caller-supplied string can collide with, and retype every `class_` parameter accordingly. `ModuleDescriptor.klass` now raises `ProtocolError` for a class name that resolves to nothing, and, ahead of `getattr` entirely, for a `name` still `NULL` -- an omitted argument rather than a request for a class literally named `'(null)'`. `NullType` is now a genuine singleton rather than a class this module merely instantiated once: `__new__` always hands back the existing instance, and `__copy__`/`__deepcopy__`/`__reduce__` keep `copy.copy`, `copy.deepcopy` and every `pickle` protocol (0 through 5) on that same object too. Without this, deepcopying a `ModuleDescriptor` minted a second, non-identical `NullType` that reached `getattr` as a non-`str` name and downgraded the clean `ProtocolError` above into a bare `TypeError`. Also added `NULL`/`NullType` to `__all__`, fixed an over-indented continuation line, and extended `test_null_sentinel_is_not_a_string` to assert the singleton claim its docstring made rather than only describing it. Updated #815's `AttributeError` assertion in `test_protocols.py` to `ProtocolError`, keeping its `assertIn("'tcp'", ...)` check that a `str` third argument is resolved as a class name, never sniffed as a transport. Added coverage for the omitted-class-name, explicit-`'(null)'`, sentinel-still-means-absent, and singleton-identity cases; `coverage run` shows 100% on `module.py` and `foundation.py`, and no drop in `protocols.py`. Corrected `docs/source/pcapkit/corekit/module.rst`, a hand-written page `autodoc`/`nitpicky` never regenerates or gates: the `name` property's `:type:` still said `str`, and the page had no entry for `NullType`/`NULL` despite eight cross-references into it from `module.py`'s own docstrings. Added an "Auxiliaries" section documenting both, following the `NoValueType`/`NoValue` precedent in `fields/field.rst`. Verified by building the full site locally with `PYTHONPATH` pointed at this tree -- the venv's editable install otherwise shadows it with the unmodified main checkout -- 56 pre-existing warnings, none from this page or naming `NullType`/`NULL`. Also: the pickle-identity test now asserts outside `subTest` too, since this repo's `pytest-subtests` reports the parent test as passed when only a `subTest` failed inside it (reproduced directly to confirm); and `NullType`'s docstring now notes that `importlib.reload` desyncs the sentinel across modules that already imported it -- structural to sharing one module-level binding, true of the old `str` sentinel too, and unreached in-tree. --- docs/source/pcapkit/corekit/module.rst | 11 +- pcapkit/corekit/module.py | 187 +++++++++++++++++++- pcapkit/foundation/registry/foundation.py | 17 +- pcapkit/foundation/registry/protocols.py | 31 ++-- tests/corekit/test_module.py | 177 +++++++++++++++++- tests/foundation/registry/test_protocols.py | 113 +++++++++++- 6 files changed, 497 insertions(+), 39 deletions(-) diff --git a/docs/source/pcapkit/corekit/module.rst b/docs/source/pcapkit/corekit/module.rst index 0c38961695..f537c589a8 100644 --- a/docs/source/pcapkit/corekit/module.rst +++ b/docs/source/pcapkit/corekit/module.rst @@ -19,9 +19,16 @@ which is originally designed as :obj:`tuple[str, str] `. Module name. .. property:: name - :type: str + :type: str | pcapkit.corekit.module.NullType + + Class name, or :data:`NULL` when whatever built this descriptor never + got one -- see :attr:`klass`. + +Auxiliaries +----------- - Class name. +.. autoclass:: pcapkit.corekit.module.NullType +.. autodata:: pcapkit.corekit.module.NULL Type Variables -------------- diff --git a/pcapkit/corekit/module.py b/pcapkit/corekit/module.py index f63ab3464a..9feb8d5710 100644 --- a/pcapkit/corekit/module.py +++ b/pcapkit/corekit/module.py @@ -12,16 +12,159 @@ import collections import importlib import sys -from typing import TYPE_CHECKING, Generic, TypeVar +from typing import TYPE_CHECKING, Generic, TypeVar, cast -__all__ = ['ModuleDescriptor'] +from pcapkit.utilities.compat import final +from pcapkit.utilities.exceptions import ProtocolError + +__all__ = ['NULL', 'NullType', 'ModuleDescriptor'] if TYPE_CHECKING: - from typing import Type + from typing import Any, Callable, Type + + from typing_extensions import Literal _T = TypeVar('_T') +@final +class NullType: + """Type of :data:`NULL`, the omitted-``class_``/``module`` sentinel. + + A distinct class rather than a plain :class:`str` -- which is what the + registry helpers in :mod:`pcapkit.foundation.registry.protocols` and + :mod:`pcapkit.foundation.registry.foundation` used to define, + independently of each other -- so that ``is`` comparisons against it mean + what they say: no :class:`str` a caller passes, including one that + happens to spell ``'(null)'`` itself, can compare equal to this sentinel + by identity. See GitHub issue #833. + + Genuinely a singleton, not merely a class this module happens to + instantiate once: :meth:`__new__` always hands back the one instance + that already exists, rather than building a new one, so no caller -- + direct, or :mod:`copy`/:mod:`pickle` reconstructing an instance behind + the scenes -- can end up holding a second object that fails an ``is + NULL`` check downstream. :meth:`ModuleDescriptor.klass` makes exactly + that check, and a stricter guard that raises on a second call would be + truer to "singleton" in the abstract, but it would also mean the + module's own ``NULL = NullType()`` below is the only call that is ever + allowed to succeed -- fragile for no real benefit, since nothing here + needs *rejecting* a second construction, only preventing it from + producing a distinct object. + + That still leaves :func:`copy.deepcopy`, :func:`copy.copy` and + :mod:`pickle` unhandled: none of them constructs a new instance by + calling ``NullType()`` themselves, so the guard above never runs for + them. Each is therefore given its own override below, rather than left + to fall back to the default behaviour for a plain object: + + * :func:`copy.copy` and :func:`copy.deepcopy` check for + :meth:`__copy__`/:meth:`__deepcopy__` before ever falling back to + reduction, so :meth:`__deepcopy__` in particular has to be defined -- + its absence is the actual defect this class used to have: deepcopying + a :class:`ModuleDescriptor` recursed into this sentinel, reduced it, + and rebuilt a second, non-identical :class:`NullType` that then read as + an ordinary attribute name to :func:`getattr`, downgrading a clean + :exc:`~pcapkit.utilities.exceptions.ProtocolError` into a bare + :exc:`TypeError` (``attribute name must be string, not 'NullType'``). + * :mod:`pickle` protocols 2 and up reconstruct through + ``cls.__new__(cls)``, which the guarded :meth:`__new__` already keeps + to one instance -- but protocols 0 and 1 reconstruct through + :func:`copyreg._reconstructor`, which calls :func:`object.__new__` + *directly*, bypassing :meth:`__new__` entirely. :meth:`__reduce__` is + defined so that every protocol, not only the ones that happen to go + through this class's own :meth:`__new__`, is routed through the same + module-level getter instead of through reconstruction at all. + + A caveat rather than a defect: :func:`importlib.reload` on this module + re-executes ``NULL = NullType()`` below, producing a *second* singleton + that the reloaded code compares against correctly but that every module + which already imported the pre-reload :data:`NULL` still holds -- so a + comparison spanning the reload sees two "singletons" that are not each + other. :meth:`ModuleDescriptor.klass` faces exactly this class of + problem for the *class* it resolves, which is why it re-reads + :data:`sys.modules` on every call rather than memoising; nothing + equivalent is possible here, because unlike a resolved class there is no + live registry this sentinel could be re-read from. The pre-#833 ``str`` + sentinel had the same fragility for the same reason -- it is a property + of sharing one module-level binding across a reload, not something this + class's singleton guarantees claim to solve -- and nothing in this + package reloads :mod:`pcapkit.corekit.module` after import. + + """ + + #: 'NullType | None': The one instance :meth:`__new__` ever returns, + #: including for the module-level ``NULL = NullType()`` below that + #: creates it in the first place. Kept on the class rather than as a + #: module global so :meth:`__new__` can read and write it without a + #: ``global`` statement. + _instance: 'NullType | None' = None + + def __new__(cls) -> 'NullType': + """Return the one instance of this class there will ever be.""" + if cls._instance is None: + cls._instance = super().__new__(cls) + return cls._instance + + def __bool__(self) -> 'Literal[False]': + """Return :obj:`False`.""" + return False + + def __repr__(self) -> 'str': + """Return :obj:`str` representation of the sentinel.""" + return '' + + def __copy__(self) -> 'NullType': + """Return ``self`` -- there is, and only ever will be, one of these.""" + return self + + def __deepcopy__(self, memo: 'dict[int, Any]') -> 'NullType': + """Return ``self``, for the same reason as :meth:`__copy__`. + + Args: + memo: The :func:`copy.deepcopy` memo table. Unused: returning + ``self`` needs no entry, since nothing about this object is + ever copied. + + """ + return self + + def __reduce__(self) -> 'tuple[Callable[[], NullType], tuple[()]]': + """Reduce to the module-level singleton getter, for every :mod:`pickle` protocol. + + A class that defines :meth:`__reduce__` has it honoured by + :meth:`object.__reduce_ex__` for every protocol uniformly, rather + than only for the ones that would otherwise call + :func:`copyreg._reconstructor` -- so naming :func:`_get_null` here + sidesteps reconstruction, and therefore :meth:`__new__`, altogether. + That makes this correct independent of whatever :meth:`__new__` does, + which is what actually covers protocols 0 and 1; see the class + docstring. + + """ + return (_get_null, ()) + + +#: NullType: Sentinel for an omitted ``class_`` argument to the ``register_*`` +#: helpers in :mod:`pcapkit.foundation.registry.protocols` and +#: :mod:`pcapkit.foundation.registry.foundation`. Defined once, here, rather +#: than once per module: both already import :class:`ModuleDescriptor` from +#: this module, so it is the shared home that needs no new module and creates +#: no import cycle. +NULL = NullType() + + +def _get_null() -> 'NullType': + """Return :data:`NULL`, for :meth:`NullType.__reduce__`. + + A module-level function rather than a lambda or a bound method, so every + :mod:`pickle` protocol -- including 0 and 1, which cannot reference + anything nested inside a class -- can name it. + + """ + return NULL + + class ModuleDescriptor(collections.namedtuple('ModuleDescriptor', ['module', 'name']), Generic[_T]): """Module descriptor contains module name and class name, the actual class can be imported by ``from module import name``.""" @@ -30,13 +173,20 @@ class can be imported by ``from module import name``.""" #: Module name. module: str - #: Class name. - name: str + #: Class name, or :data:`NULL` when whatever built this descriptor never + #: got one -- see :attr:`klass`. + name: 'str | NullType' @property def klass(self) -> 'Type[_T]': """Import class from module. + Raises: + pcapkit.utilities.exceptions.ProtocolError: If :attr:`name` is + :data:`NULL` -- the caller building this descriptor omitted + the class name rather than naming one that turned out wrong -- + or if :attr:`module` has no attribute named :attr:`name`. + Important: The module is read from :data:`sys.modules` first, and :func:`importlib.import_module` is entered only when it is not @@ -65,10 +215,24 @@ def klass(self) -> 'Type[_T]': :func:`isinstance` against the live one. """ + if self.name is NULL: + # ``module`` is a ``str`` and the caller never named a class -- + # GitHub issue #832. Reporting that plainly, before ``getattr`` + # ever sees it, is more useful than letting the sentinel reach + # ``getattr`` and be reported as an absent attribute named + # ``'(null)'``, which is what happened before #833 gave the + # sentinel a type no caller-supplied string can collide with. + raise ProtocolError(f'missing class name for module {self.module!r}: pass an ' + 'explicit class_ argument') + + # ``self.name`` is a ``str`` from here on -- the ``NULL`` case just + # raised above -- so ``getattr`` below always gets a real name. + name = cast('str', self.name) + module = sys.modules.get(self.module) if module is not None: try: - return getattr(module, self.name) + return getattr(module, name) except AttributeError: # ``sys.modules`` also holds modules whose body is still # executing -- a circular import, or another thread part way @@ -77,4 +241,13 @@ def klass(self) -> 'Type[_T]': # the attribute missing; a name that really is absent raises # from the ``getattr`` below instead, with the same message. pass - return getattr(importlib.import_module(self.module), self.name) + try: + return getattr(importlib.import_module(self.module), name) + except AttributeError as error: + # GitHub issue #832: every ``register_*`` helper that builds a + # descriptor from a bad class name used to fail with this bare + # stdlib :exc:`AttributeError` -- a caller cannot catch that as a + # :mod:`pcapkit` error. Re-raised as :exc:`ProtocolError` naming + # both the module and the class that turned out missing; the + # message text itself is unchanged; only the type is not. + raise ProtocolError(str(error)) from error diff --git a/pcapkit/foundation/registry/foundation.py b/pcapkit/foundation/registry/foundation.py index eb42eda1a1..427209c02e 100644 --- a/pcapkit/foundation/registry/foundation.py +++ b/pcapkit/foundation/registry/foundation.py @@ -9,7 +9,7 @@ """ from typing import TYPE_CHECKING, cast, overload -from pcapkit.corekit.module import ModuleDescriptor +from pcapkit.corekit.module import NULL, ModuleDescriptor from pcapkit.foundation.extraction import Extractor from pcapkit.foundation.reassembly.ipv4 import IPv4 as IPv4_Reassembly from pcapkit.foundation.reassembly.ipv6 import IPv6 as IPv6_Reassembly @@ -23,6 +23,7 @@ from dictdumper import Dumper + from pcapkit.corekit.module import NullType from pcapkit.foundation.engines import Engine from pcapkit.foundation.reassembly.reassembly import CallbackFn as Reasm_CallbackFn from pcapkit.foundation.reassembly.reassembly import Reassembly @@ -45,8 +46,6 @@ #: :data:`pcapkit.utilities.logging.logger`. logger = get_logger(__name__) -NULL = '(null)' - ############################################################################### # Engine Registries ############################################################################### @@ -60,7 +59,7 @@ def register_extractor_engine(name: 'str', module: 'str', class_: 'str') -> 'Non # NOTE: pcapkit.foundation.extraction.Extractor.__engine__ def register_extractor_engine(name: 'str', module: 'ModuleDescriptor[Engine] | Type[Engine] | str', - class_: 'str' = NULL) -> 'None': # pylint: disable=redefined-builtin + class_: 'str | NullType' = NULL) -> 'None': # pylint: disable=redefined-builtin r"""Registered a new engine class. Notes: @@ -96,7 +95,7 @@ def register_dumper(format: 'str', module: 'str', class_: 'str', *, ext: 'str') def register_dumper(format: 'str', module: 'ModuleDescriptor[Dumper] | Type[Dumper] | str', - class_: 'str' = NULL, *, ext: 'str') -> 'None': # pylint: disable=redefined-builtin + class_: 'str | NullType' = NULL, *, ext: 'str') -> 'None': # pylint: disable=redefined-builtin r"""Registered a new dumper class. Notes: @@ -135,7 +134,7 @@ def register_extractor_dumper(format: 'str', module: 'str', class_: 'str', *, ex # NOTE: pcapkit.foundation.extraction.Extractor.__output__ def register_extractor_dumper(format: 'str', module: 'ModuleDescriptor[Dumper] | Type[Dumper] | str', - class_: 'str' = NULL, *, ext: 'str') -> 'None': # pylint: disable=redefined-builtin + class_: 'str | NullType' = NULL, *, ext: 'str') -> 'None': # pylint: disable=redefined-builtin r"""Registered a new dumper class. Notes: @@ -168,7 +167,7 @@ def register_traceflow_dumper(format: 'str', module: 'str', class_: 'str', *, ex # NOTE: pcapkit.foundation.traceflow.traceflow.TraceFlow.__output__ def register_traceflow_dumper(format: 'str', module: 'ModuleDescriptor[Dumper] | Type[Dumper] | str', - class_: 'str' = NULL, *, ext: 'str') -> 'None': # pylint: disable=redefined-builtin + class_: 'str | NullType' = NULL, *, ext: 'str') -> 'None': # pylint: disable=redefined-builtin r"""Registered a new dumper class. Notes: @@ -275,7 +274,7 @@ def register_extractor_reassembly(protocol: 'str', module: 'str', class_: 'str') # NOTE: pcapkit.foundation.extraction.Extractor.__reassembly__ def register_extractor_reassembly(protocol: 'str', module: 'str | ModuleDescriptor[Reassembly] | Type[Reassembly]', - class_: 'str' = NULL) -> 'None': # pylint: disable=redefined-builtin + class_: 'str | NullType' = NULL) -> 'None': # pylint: disable=redefined-builtin r"""Registered a new reassembly class. Notes: @@ -307,7 +306,7 @@ def register_extractor_traceflow(protocol: 'str', module: 'str', class_: 'str') # NOTE: pcapkit.foundation.extraction.Extractor.__traceflow__ def register_extractor_traceflow(protocol: 'str', module: 'str | ModuleDescriptor[TraceFlow] | Type[TraceFlow]', - class_: 'str' = NULL) -> 'None': # pylint: disable=redefined-builtin + class_: 'str | NullType' = NULL) -> 'None': # pylint: disable=redefined-builtin r"""Registered a new flow tracing class. Notes: diff --git a/pcapkit/foundation/registry/protocols.py b/pcapkit/foundation/registry/protocols.py index ad928f6fdf..28c2474552 100644 --- a/pcapkit/foundation/registry/protocols.py +++ b/pcapkit/foundation/registry/protocols.py @@ -20,7 +20,7 @@ from pcapkit.const.reg.transtype import TransType as Enum_TransType from pcapkit.const.sctp.payload_protocol_identifier import \ PayloadProtocolIdentifier as Enum_PayloadProtocolIdentifier -from pcapkit.corekit.module import ModuleDescriptor +from pcapkit.corekit.module import NULL, ModuleDescriptor from pcapkit.protocols import __proto__ as protocol_registry from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 from pcapkit.protocols.internet.hip import HIP @@ -80,6 +80,7 @@ PayloadProtocolIdentifier as SCTP_PayloadProtocolIdentifier from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as TCP_MPTCPOption from pcapkit.const.tcp.option import Option as TCP_Option + from pcapkit.corekit.module import NullType from pcapkit.protocols.application.httpv2 import FrameConstructor as HTTP_FrameConstructor from pcapkit.protocols.application.httpv2 import FrameParser as HTTP_FrameParser from pcapkit.protocols.internet.hip import ParameterConstructor as HIP_ParameterConstructor @@ -140,8 +141,6 @@ #: :data:`pcapkit.utilities.logging.logger`. logger = get_logger(__name__) -NULL = '(null)' - # NOTE: pcapkit.protocols.__proto__ def register_protocol(protocol: 'Type[ProtocolBase]') -> 'None': @@ -383,7 +382,7 @@ def register_linktype(code: 'LinkType', module: 'str', class_: 'str') -> 'None': def register_linktype(code: 'LinkType', module: 'str | ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]', - class_: 'str' = NULL) -> 'None': + class_: 'str | NullType' = NULL) -> 'None': r"""Register a new protocol class. Notes: @@ -428,7 +427,7 @@ def register_pcap(code: 'LinkType', module: 'str', class_: 'str') -> 'None': ... # NOTE: pcapkit.protocols.misc.pcap.frame.Frame.__proto__ def register_pcap(code: 'LinkType', module: 'str | ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]', - class_: 'str' = NULL) -> 'None': + class_: 'str | NullType' = NULL) -> 'None': r"""Register a new protocol class. Notes: @@ -465,7 +464,7 @@ def register_pcapng(code: 'LinkType', module: 'str', class_: 'str') -> 'None': . # NOTE: pcapkit.protocols.misc.pcapng.PCAPNG.__proto__ def register_pcapng(code: 'LinkType', module: 'str | ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]', - class_: 'str' = NULL) -> 'None': + class_: 'str | NullType' = NULL) -> 'None': r"""Register a new protocol class. Notes: @@ -507,7 +506,7 @@ def register_ethertype(code: 'EtherType', module: 'str', class_: 'str') -> 'None # NOTE: pcapkit.protocols.link.link.Link.__proto__ def register_ethertype(code: 'EtherType', module: 'str | ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]', - class_: 'str' = NULL) -> 'None': + class_: 'str | NullType' = NULL) -> 'None': r"""Register a new protocol class. Notes: @@ -549,7 +548,7 @@ def register_transtype(code: 'TransType', module: 'str', class_: 'str') -> 'None # NOTE: pcapkit.protocols.internet.internet.Internet.__proto__ def register_transtype(code: 'TransType', module: 'str | ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]', - class_: 'str' = NULL) -> 'None': + class_: 'str | NullType' = NULL) -> 'None': r"""Register a new protocol class. Notes: @@ -794,7 +793,8 @@ def register_apptype(code: 'Enum_AppType', module: 'str', class_: 'str', *transp def register_apptype(code: 'int | Enum_AppType', module: 'str | ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]', - class_: 'str | TransportProtocol' = NULL, *transport: 'TransportProtocol | str') -> 'None': + class_: 'str | TransportProtocol | NullType' = NULL, + *transport: 'TransportProtocol | str') -> 'None': r"""Register a new protocol class. Notes: @@ -844,6 +844,13 @@ def register_apptype(code: 'int | Enum_AppType', module: 'str | ModuleDescriptor ``name``. A composite such as ``tcp | udp``, or its string form ``'tcp|udp'``, names two and is refused for the same reason: one call registers under one transport protocol. + pcapkit.utilities.exceptions.ProtocolError: Raised lazily, from + :attr:`ModuleDescriptor.klass `, + when ``module`` is a :class:`str` and either ``class_`` names no + attribute of it, or ``class_`` was never given at all -- the + latter distinguished from the former rather than reported as a + missing attribute named ``'(null)'``. See GitHub issues #832 and + #833. Important: :class:`~pcapkit.protocols.transport.sctp.SCTP` is deliberately **not** @@ -955,7 +962,7 @@ def register_tcp(code: 'int | Enum_AppType', module: 'str', class_: 'str') -> 'N # NOTE: pcapkit.protocols.transport.tcp.TCP.__proto__ def register_tcp(code: 'int | Enum_AppType', module: 'str | ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]', - class_: 'str' = NULL) -> 'None': + class_: 'str | NullType' = NULL) -> 'None': r"""Register a new protocol class. Notes: @@ -1044,7 +1051,7 @@ def register_udp(code: 'int | Enum_AppType', module: 'str', class_: 'str') -> 'N # NOTE: pcapkit.protocols.transport.udp.UDP.__proto__ def register_udp(code: 'int | Enum_AppType', module: 'str | ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]', - class_: 'str' = NULL) -> 'None': + class_: 'str | NullType' = NULL) -> 'None': r"""Register a new protocol class. Notes: @@ -1083,7 +1090,7 @@ def register_sctp(code: 'int | SCTP_PayloadProtocolIdentifier', module: 'str', c # NOTE: pcapkit.protocols.transport.sctp.SCTP.__proto__ def register_sctp(code: 'int | SCTP_PayloadProtocolIdentifier', module: 'str | ModuleDescriptor[ProtocolBase] | Type[ProtocolBase]', - class_: 'str' = NULL) -> 'None': + class_: 'str | NullType' = NULL) -> 'None': r"""Register a new protocol class. Notes: diff --git a/tests/corekit/test_module.py b/tests/corekit/test_module.py index 0d888f1ec4..73c870f8e7 100644 --- a/tests/corekit/test_module.py +++ b/tests/corekit/test_module.py @@ -1,5 +1,7 @@ from __future__ import annotations +import copy +import pickle import sys import types import unittest @@ -117,14 +119,17 @@ def test_klass_defers_to_import_module_for_a_partially_initialised_module(self) importer.assert_called_once_with('demo.partial') - def test_klass_still_raises_attributeerror_for_a_name_that_is_not_there(self) -> None: + def test_klass_raises_protocolerror_for_a_name_that_is_not_there(self) -> None: """A genuinely absent name must still fail, and say so. The fallback above swallows one :exc:`AttributeError` to retry through :func:`importlib.import_module`. A descriptor naming a class that does - not exist has to come back out of that retry as the same - :exc:`AttributeError` it always raised, rather than as :data:`None` or - as a second, more confusing error. + not exist has to come back out of that retry naming the same missing + attribute it always did -- but as :exc:`~pcapkit.utilities.\ +exceptions.ProtocolError` rather than the bare stdlib :exc:`AttributeError`, + so every one of the nine ``register_*`` call sites that build a + descriptor from a bad class name fails as a :mod:`pcapkit` error a + caller can actually catch. GitHub issue #832. """ target_module = types.ModuleType('demo.incomplete') @@ -132,8 +137,170 @@ def test_klass_still_raises_attributeerror_for_a_name_that_is_not_there(self) -> descriptor = self.module.ModuleDescriptor('demo.incomplete', 'Missing') with mock.patch('importlib.import_module', return_value=target_module): - with self.assertRaisesRegex(AttributeError, 'Missing'): + with self.assertRaisesRegex(self.module.ProtocolError, 'Missing') as caught: descriptor.klass # pylint: disable=pointless-statement + self.assertNotIsInstance(caught.exception, AttributeError) + self.assertIn('demo.incomplete', str(caught.exception)) + + def test_klass_raises_protocolerror_when_class_name_was_never_given(self) -> None: + """An omitted ``class_`` must not be reported as an absent attribute. + + A descriptor built with :data:`NULL` for its ``name`` -- what every + ``register_*`` wrapper does when a :class:`str` ``module`` is given + with no ``class_`` -- means the caller omitted a required argument, + not that they asked for a class literally named ``'(null)'``. Saying + so is the point of GitHub issues #832 and #833 together: the sentinel + must never reach :func:`getattr`, so the failure never mentions + ``'(null)'`` at all, and it must not depend on :func:`importlib.\ +import_module` or :data:`sys.modules` succeeding -- the omission is caught + before either is consulted. + + """ + descriptor = self.module.ModuleDescriptor('demo.omitted', self.module.NULL) + with mock.patch('importlib.import_module', + side_effect=RuntimeError('must not be reached')) as importer: + with self.assertRaises(self.module.ProtocolError) as caught: + descriptor.klass # pylint: disable=pointless-statement + importer.assert_not_called() + message = str(caught.exception) + self.assertIn('demo.omitted', message) + self.assertNotIn('(null)', message) + + def test_klass_treats_an_explicit_null_string_as_a_real_class_name(self) -> None: + """``class_='(null)'`` is a class name, not the sentinel. + + Before GitHub issue #833, :data:`NULL` was the plain :class:`str` + ``'(null)'``, so a descriptor built with that exact string was + indistinguishable from one built from the sentinel default -- an + equal-but-distinct string took whichever branch the identity check + happened to land on. The sentinel is no longer a :class:`str` at all, + so ``'(null)'`` now always resolves as an ordinary (missing) class + name, and the failure names it like any other bad class name would. + + """ + target_module = types.ModuleType('demo.explicit_null') + self._register('demo.explicit_null', target_module) + + descriptor = self.module.ModuleDescriptor('demo.explicit_null', '(null)') + self.assertIsNot(descriptor.name, self.module.NULL) + with self.assertRaisesRegex(self.module.ProtocolError, r"'\(null\)'"): + descriptor.klass # pylint: disable=pointless-statement + + def test_null_sentinel_is_not_a_string(self) -> None: + """:data:`NULL` means "absent", and nothing else compares equal to it. + + The pre-#833 sentinel was the plain string ``'(null)'``, compared by + identity -- so an equal-but-distinct ``'(null)'`` from a caller took a + different branch than the sentinel itself, purely as a function of + string interning. :data:`NULL` is now a dedicated + :class:`~pcapkit.corekit.module.NullType` singleton, so no string a + caller passes can compare equal to it by ``==`` or by ``is``. + + "Singleton" is asserted here, not only claimed: calling + :class:`~pcapkit.corekit.module.NullType` a second time must hand + back :data:`NULL` itself rather than a distinct, equally-valid + instance -- see :meth:`test_null_sentinel_identity_survives_copy_and_pickle` + for what a second instance breaks downstream. + + """ + self.assertNotIsInstance(self.module.NULL, str) + self.assertIsInstance(self.module.NULL, self.module.NullType) + self.assertFalse(self.module.NULL == '(null)') # pylint: disable=unneeded-not + self.assertIsNot(self.module.NULL, '(null)') + self.assertFalse(bool(self.module.NULL)) + self.assertEqual(repr(self.module.NULL), '') + self.assertIs(self.module.NullType(), self.module.NULL) + + def test_null_sentinel_identity_survives_copy_and_pickle(self) -> None: + """:data:`NULL` stays the same object through :mod:`copy` and :mod:`pickle`. + + Before this fix, :class:`~pcapkit.corekit.module.NullType` had no + :meth:`~pcapkit.corekit.module.NullType.__copy__`, + :meth:`~pcapkit.corekit.module.NullType.__deepcopy__` or + :meth:`~pcapkit.corekit.module.NullType.__reduce__` of its own, so + :func:`copy.copy`, :func:`copy.deepcopy` and every :mod:`pickle` + protocol fell back to the default behaviour for a plain object and + each produced a *second*, non-identical :class:`NullType` instance. + Measured on GitHub pull request #835's head, ``375b50c85``: this + test's ``copy.deepcopy`` and ``pickle`` assertions below all fail + with an :class:`AssertionError` there, since the round-tripped object + ``is not`` :data:`NULL`. + + Every pickle protocol the running interpreter supports -- ``0`` + through :data:`pickle.HIGHEST_PROTOCOL` -- is exercised, not only the + default one, because :mod:`pickle` protocols 0 and 1 reconstruct + through :func:`copyreg._reconstructor` -- which calls + :func:`object.__new__` directly -- rather than through + :meth:`NullType.__new__ `, + so a fix that only guards ``__new__`` would still fail those two. + + Each protocol's assertion also runs a second time, outside + :meth:`~unittest.TestCase.subTest`, and the results are asserted + together at the end. This repository's ``pytest-subtests`` reports + the *parent* test node as passed when only a ``subTest`` failed inside + it -- confirmed by reproducing it directly: a single failing + ``subTest`` iteration renders its own ``SUBFAILED`` line while the + owning test method's own line still reads ``PASSED`` -- so a + regression confined to one protocol would otherwise be visible only + to something that reads the ``SUBFAILED`` line specifically. The + plain assertion below has no such blind spot: it fails the test + method itself, the ordinary way, regardless of which protocol(s) + misbehaved. + + """ + NULL = self.module.NULL + + self.assertIs(copy.copy(NULL), NULL) + self.assertIs(copy.deepcopy(NULL), NULL) + + bad_protocols = [] + for protocol in range(pickle.HIGHEST_PROTOCOL + 1): + roundtripped = pickle.loads(pickle.dumps(NULL, protocol=protocol)) + with self.subTest(protocol=protocol): + self.assertIs(roundtripped, NULL) + if roundtripped is not NULL: + bad_protocols.append(protocol) + self.assertEqual(bad_protocols, [], + f'pickle protocol(s) {bad_protocols} did not round-trip to NULL by identity') + + def test_klass_raises_protocolerror_not_typeerror_after_deepcopying_the_descriptor(self) -> None: + """Deepcopying a :class:`ModuleDescriptor` must not degrade its error. + + :attr:`~pcapkit.corekit.module.ModuleDescriptor.klass` checks + ``self.name is NULL`` to tell "the caller never named a class" apart + from "the caller named a class that does not exist" -- see + :meth:`test_klass_raises_protocolerror_when_class_name_was_never_given`. + Before this fix, :func:`copy.deepcopy` on the descriptor recursed into + that check's operand and minted a second, non-identical + :class:`~pcapkit.corekit.module.NullType`, so the identity check + failed silently and ``self.name`` -- still that stray sentinel, now + masquerading as an ordinary class name -- reached :func:`getattr` + directly. :func:`getattr` on a non-:class:`str` name raises a bare + :exc:`TypeError` that :meth:`klass` does not catch, downgrading what + should be a clean :exc:`~pcapkit.utilities.exceptions.ProtocolError` + into ``TypeError: attribute name must be string, not 'NullType'``. + Measured on GitHub pull request #835's head, ``375b50c85``: the + ``assertRaises(self.module.ProtocolError)`` below does not catch that + :exc:`TypeError` there, since :exc:`ProtocolError` derives from + :exc:`BaseError` and :exc:`ValueError`, never :exc:`TypeError`, so the + test fails with the :exc:`TypeError` propagating out uncaught. + + The target module is registered directly into :data:`sys.modules`, + the same way :meth:`_register` does for every other test in this + file, rather than named without registering it: an unregistered name + fails at :func:`importlib.import_module` with + :exc:`ModuleNotFoundError` before :func:`getattr` is ever reached, + which would not exhibit this at all. + + """ + target_module = types.ModuleType('demo.deepcopy_target') + self._register('demo.deepcopy_target', target_module) + + descriptor = self.module.ModuleDescriptor('demo.deepcopy_target', self.module.NULL) + copied = copy.deepcopy(descriptor) + + with self.assertRaises(self.module.ProtocolError): + copied.klass # pylint: disable=pointless-statement if __name__ == '__main__': diff --git a/tests/foundation/registry/test_protocols.py b/tests/foundation/registry/test_protocols.py index 741ef3b197..d976a14115 100644 --- a/tests/foundation/registry/test_protocols.py +++ b/tests/foundation/registry/test_protocols.py @@ -430,7 +430,7 @@ def test_top_level_link_internet_and_transport_protocol_wrappers(self) -> None: from pcapkit.const.reg.linktype import LinkType from pcapkit.const.reg.transtype import TransType from pcapkit.foundation.registry import protocols as registry - from pcapkit.utilities.exceptions import RegistryError + from pcapkit.utilities.exceptions import ProtocolError, RegistryError UnitProtocol = self._unit_protocol() raw_module = ('pcapkit.protocols.misc.raw', 'Raw') @@ -701,11 +701,15 @@ def test_top_level_link_internet_and_transport_protocol_wrappers(self) -> None: # a ``str`` third argument is *always* a class name, even one that # happens to spell a transport's own name. ``'tcp'`` is looked up as # a class in ``raw_module[0]``, which has none, so it fails the same - # way any other wrong class name would: an ``AttributeError`` naming + # way any other wrong class name would: a ``ProtocolError`` naming # ``'tcp'`` itself, not a ``RegistryError`` about an unknown - # transport -- proof it was never coerced as one. + # transport -- proof it was never coerced as one. GitHub issue #832 + # retyped this from a bare stdlib ``AttributeError`` to + # ``ProtocolError`` -- the property that a ``str`` third positional is + # resolved as a class name, never sniffed as a transport, is what has + # to survive; only the exception type changed. with mock.patch.object(registry, 'register_protocol'): - with self.assertRaises(AttributeError) as caught: + with self.assertRaises(ProtocolError) as caught: registry.register_apptype(65204, raw_module[0], 'tcp', TransportProtocol.udp) self.assertIn("'tcp'", str(caught.exception)) self.assertNotIn(65204, registry.TCP.__proto__) @@ -735,6 +739,107 @@ def test_top_level_link_internet_and_transport_protocol_wrappers(self) -> None: self.assertIsInstance(udp_register.call_args.args[1], registry.ModuleDescriptor) self.assertEqual(register_protocol.call_args.args[0].__name__, 'Raw') + def test_register_apptype_omitted_class_name_reports_missing_argument(self) -> None: + """GitHub issues #832 and #833: an omitted ``class_`` must say so. + + ``register_apptype(AppType_TCP.TCP_3exmp, raw_module_name)`` never + supplies ``class_`` at all -- it defaults to :data:`NULL` -- so this + is the omission the two issues describe together: with a :class:`str` + ``module`` and no ``class_``, the failure must name the *argument* as + missing rather than letting the sentinel reach :func:`getattr` and + come back as an attribute named ``'(null)'``. + + """ + from pcapkit.const.reg.apptype import TCP as AppType_TCP + from pcapkit.foundation.registry import protocols as registry + from pcapkit.utilities.exceptions import ProtocolError + + raw_module_name = 'pcapkit.protocols.misc.raw' + port = AppType_TCP.TCP_3exmp.port + + self._guard_registry(registry.TCP.__proto__, port) + with mock.patch.object(registry, 'register_protocol'): + with self.assertRaises(ProtocolError) as caught: + registry.register_apptype(AppType_TCP.TCP_3exmp, raw_module_name) + message = str(caught.exception) + self.assertIn(raw_module_name, message) + self.assertNotIn('(null)', message) + + def test_register_apptype_explicit_null_string_is_a_class_name_not_the_sentinel(self) -> None: + """GitHub issue #833: ``class_='(null)'`` is data, not the sentinel. + + Before the sentinel stopped being a plain :class:`str`, an + equal-but-distinct ``'(null)'`` passed by a caller was indistinguishable + from the default -- both compared equal, and which branch ran depended + on identity/interning rather than intent. Passed explicitly here, it + must be resolved as an ordinary (missing) class name, the same way any + other wrong class name is, and the failure must name the module and + the literal ``'(null)'`` that was looked up. + + """ + from pcapkit.const.reg.apptype import TransportProtocol + from pcapkit.foundation.registry import protocols as registry + from pcapkit.utilities.exceptions import ProtocolError + + raw_module_name = 'pcapkit.protocols.misc.raw' + port = 65213 + + self._guard_registry(registry.TCP.__proto__, port) + with mock.patch.object(registry, 'register_protocol'): + with self.assertRaises(ProtocolError) as caught: + registry.register_apptype(port, raw_module_name, '(null)', TransportProtocol.tcp) + self.assertIn(raw_module_name, str(caught.exception)) + self.assertIn("'(null)'", str(caught.exception)) + + def test_register_apptype_null_sentinel_still_means_absent_with_a_class_module(self) -> None: + """GitHub issue #833's own repro, re-run against the new sentinel. + + With a non-``str`` ``module``, the third positional is never + ``class_`` -- it is the first ``*transport`` element instead, per the + maintainer ruling in #815. :data:`NULL` passed there must still mean + "nothing was named" (falling through to "no transport protocol + given"), while the *string* ``'(null)'`` must be treated as an + ordinary, unrecognised transport name -- proving the two no longer + collide now that :data:`NULL` is not a :class:`str`. + + """ + from pcapkit.foundation.registry import protocols as registry + from pcapkit.utilities.exceptions import RegistryError + + UnitProtocol = self._unit_protocol() + + with self.assertRaises(RegistryError) as caught_absent: + registry.register_apptype(65210, UnitProtocol, class_=registry.NULL) + self.assertIn('no transport protocol given', str(caught_absent.exception)) + + with self.assertRaises(RegistryError) as caught_present: + registry.register_apptype(65211, UnitProtocol, class_='(null)') + self.assertIn("unknown transport protocol: '(null)'", str(caught_present.exception)) + + def test_register_tcp_bad_class_name_raises_protocolerror(self) -> None: + """GitHub issue #832: the fix is family-wide, not one function. + + :func:`register_apptype` is not the only one of the nine ``register_*`` + wrappers that builds a :class:`~pcapkit.corekit.module.ModuleDescriptor` + from a :class:`str` ``module`` -- :func:`register_tcp` is a sibling + that shares the same :attr:`ModuleDescriptor.klass` resolution, so it + must fail a bad class name the same way. + + """ + from pcapkit.foundation.registry import protocols as registry + from pcapkit.utilities.exceptions import ProtocolError + + raw_module_name = 'pcapkit.protocols.misc.raw' + port = 65212 + + self._guard_registry(registry.TCP.__proto__, port) + with mock.patch.object(registry, 'register_protocol'): + with self.assertRaises(ProtocolError) as caught: + registry.register_tcp(port, raw_module_name, 'NotAClass') + message = str(caught.exception) + self.assertIn(raw_module_name, message) + self.assertIn("'NotAClass'", message) + def test_option_like_registry_wrappers_validate_methods_and_register_schema(self) -> None: from pcapkit.const.hip.parameter import Parameter from pcapkit.const.http.frame import Frame as HTTPFrame