From 1db6232db192df8dc92f8c9df4439916a4fb5979 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 14:43:06 -0400 Subject: [PATCH] fix(dumpkit): a nameless flag member no longer renders as `Type::None [0]` (#648) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING CHANGE to dumped output. `make_dumper`'s `object_hook` interpolated `o.name` unguarded, and a `Flag` value composed entirely of undeclared bits has `name is None` rather than a string -- so the literal four characters `None` landed in the name half and `pcapkit.const.tcp.flags.Flags(0)` rendered as `Flags::None [0]` in `json`, `tree`, `text`, `txt`, `plist` and `xml`, out of both `Extractor` and `TraceFlow`. * Three sites, not one, all with character-for-character the same interpolation: the `MultiDict`/`OrderedMultiDict` *key* path, the `addon` branch's `'enum'` key, and the scalar return. They now share one `render_enum` helper, so the guard cannot be applied to two of three. * The fallback is the value's own decimal spelling -- `Flags::0 [0]`, `Flags::8 [8]`. That is what the enumeration libraries already use for an undeclared residue: `Flags(2057).name` is `'ACK|9'`, naming the declared bit and giving the leftovers as one number, so a wholly-undeclared value is the same rendering with no declared bit in front. It also cannot be mistaken for a member name, since a Python identifier may not begin with a digit -- none of the 1867 identifiers under `pcapkit/const` is a bare decimal. `'None'` could be mistaken for one, and `NONE` is a real declared name elsewhere. * Not limited to zero, so the guard is on `name is None` and not on the value. `Flags(1)`, `Flags(8)`, `Flags(9)` and `Flags(65536)` are equally nameless; a fix keyed on zero would have left four of five cases emitting `None`. * Not an `aenum` quirk either -- a stdlib `enum.IntFlag` answers `name is None` identically on CPython 3.14.7, so the guard belongs at the interpolation rather than in a choice of enumeration library. * Five of the seven flag registries in the library are nameless at zero, not one: `Flags` plus the four Mobility Header flag registries. The new test discovers them rather than naming them. * #634 pinned the old string in `test_tcp_udp_unit.py` and said it was "left to its own change"; that assertion and its prose now read `Flags::0 [0]`. A flagless TCP segment is the library's only wire-reachable producer of a nameless member, which makes it the test that notices if the guard goes away. `examples/captures/out.json`, `out.plist` and `out.txt` do not move: regenerating all three from `in.pcap` with and without the change gives byte-identical output, because `render_enum` is the identity on every member that has a name and no committed fixture contains a nameless one. `tests/dumpkit/ tests/foundation/ tests/protocols/transport/test_tcp_udp_unit.py` passes 256 tests and 397 subtests. The new module is 5 tests and 21 subtests and fails on `main` with `'Flags::None [65536]' != 'Flags::65536 [65536]'` among 18 failures; the amended #634 assertion fails there with `'Flags::None [0]' != 'Flags::0 [0]'`. `pcapkit/dumpkit/common.py` holds 100% statement and branch coverage across the change, 71 statements and 32 branches before, 76 and 34 after. One pre-existing failure is unrelated and unchanged by this commit: `test_tcp_runtime.py::…::test_sample_capture_reassembles_every_stream_byte_exactly` raises `FileNotFoundError` for a generated fixture in a tree where `make samples` has not been run, identically with and without the change. Fixes #648 --- docs/source/pcapkit/dumpkit/common.rst | 2 + pcapkit/dumpkit/common.py | 60 +++- .../test_nameless_enum_rendering_unit.py | 286 ++++++++++++++++++ .../protocols/transport/test_tcp_udp_unit.py | 36 ++- 4 files changed, 367 insertions(+), 17 deletions(-) create mode 100644 tests/dumpkit/test_nameless_enum_rendering_unit.py diff --git a/docs/source/pcapkit/dumpkit/common.rst b/docs/source/pcapkit/dumpkit/common.rst index 69bb0e6ca1..2467f8b3a6 100644 --- a/docs/source/pcapkit/dumpkit/common.rst +++ b/docs/source/pcapkit/dumpkit/common.rst @@ -17,6 +17,8 @@ classes. Internal Definitions -------------------- +.. autofunction:: pcapkit.dumpkit.common.render_enum + .. autoclass:: pcapkit.dumpkit.common.DumperBase :members: :show-inheritance: diff --git a/pcapkit/dumpkit/common.py b/pcapkit/dumpkit/common.py index 7451bb681d..f07b70f410 100644 --- a/pcapkit/dumpkit/common.py +++ b/pcapkit/dumpkit/common.py @@ -165,6 +165,60 @@ def __init_subclass__(cls, /, fmt: 'Optional[str]' = None, return super().__init_subclass__() +def render_enum(o: 'enum.Enum | aenum.Enum') -> 'str': + """Render an enumeration member as ``Type::name [value]``. + + This is the spelling every dumped enumeration carries in the ``json``, + ``tree``, ``text``, ``txt``, ``plist`` and ``xml`` output of both + :class:`~pcapkit.foundation.extraction.Extractor` and + :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlow`, so it lives in + one function rather than being spelled out at each of the three places + :func:`make_dumper`'s hook needs it. + + Args: + o: Enumeration member to render. + + Returns: + The member's ``Type::name [value]`` rendering. + + Note: + A :class:`~enum.Flag` value composed **entirely of undeclared bits** has + no name at all -- :attr:`~enum.Enum.name` is :data:`None`, not a string -- + so interpolating it unguarded put the literal four characters ``None`` + into the name half and rendered + :class:`~pcapkit.const.tcp.flags.Flags` ``(0)`` as ``'Flags::None [0]'`` + (GitHub issue #648). + + Two things make that worth a guard rather than a shrug. ``'None'`` is a + plausible member name, so a consumer splitting the rendering on ``::`` + cannot tell it from a member genuinely so named -- and ``NONE`` *is* a + declared name elsewhere in the library. And the defect is not confined to + zero: ``Flags(1)``, ``Flags(8)`` and ``Flags(65536)`` are every bit as + nameless, so a guard written against ``value == 0`` would fix one case + and leave the rest. + + The fallback is the value's own decimal spelling, which is what the + enumeration libraries themselves already use for an undeclared residue: + ``Flags(2057).name`` is ``'ACK|9'``, naming the declared bit and giving + the leftovers as one number. A wholly-undeclared value is that same + rendering with no declared bit to precede it, so ``Flags(9)`` becomes + ``'Flags::9 [9]'`` and ``Flags(0)`` becomes ``'Flags::0 [0]'``. It also + cannot be mistaken for a member name, since a Python identifier may not + begin with a digit -- none of the 1867 identifiers declared under + :mod:`pcapkit.const` is a bare decimal, and none ever can be. + + This is *not* an :mod:`aenum` quirk. A stdlib :class:`enum.IntFlag` built + from the same members answers ``name is None`` identically on CPython + 3.14.7, so the guard belongs here rather than in a choice of enumeration + library. + + """ + name = o.name + if name is None: + name = str(o.value) + return f'{type(o).__name__}::{name} [{o.value}]' + + def make_dumper(output: 'Type[ABCDumper]') -> 'Type[ABCDumper]': """Create a customised :class:`~dictdumper.dumper.Dumper` object. @@ -201,7 +255,7 @@ def object_hook(self, o: 'Any') -> 'Any': temp = collections.defaultdict(list) # type: DefaultDict[str, list[Any]] for key, val in o.items(multi=True): if isinstance(key, (enum.Enum, aenum.Enum)): - key = f'{type(key).__name__}::{key.name} [{key.value}]' + key = render_enum(key) temp[key].append(val) return temp if isinstance(o, dict): @@ -210,10 +264,10 @@ def object_hook(self, o: 'Any') -> 'Any': addon = {key: val for key, val in o.__dict__.items() if not key.startswith('_')} if addon: return { - 'enum': f'{type(o).__name__}::{o.name} [{o.value}]', + 'enum': render_enum(o), **addon, } - return f'{type(o).__name__}::{o.name} [{o.value}]' + return render_enum(o) return super(type(self), self).object_hook(o) # type: ignore[unreachable] def default(self, o: 'Any') -> 'Literal["fallback"]': # pylint: disable=unused-argument diff --git a/tests/dumpkit/test_nameless_enum_rendering_unit.py b/tests/dumpkit/test_nameless_enum_rendering_unit.py new file mode 100644 index 0000000000..b4ec545897 --- /dev/null +++ b/tests/dumpkit/test_nameless_enum_rendering_unit.py @@ -0,0 +1,286 @@ +from __future__ import annotations + +import enum +import importlib +import importlib.util +import pkgutil +import unittest + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Flag values made up *entirely* of bits no member declares. Every one of these +#: has ``name is None``, which is the whole point of #648: the defect is a +#: property of "no declared bits", not of the number zero, so a guard written +#: against ``value == 0`` would fix the first of these and leave the rest. +NAMELESS_VALUES = (0, 1, 8, 9, 65536) + + +class StdFlags(enum.IntFlag): + """A stdlib replica of :class:`pcapkit.const.tcp.flags.Flags`' declared bits. + + Present so the tests can show the nameless pseudo-member is not an + :mod:`aenum` quirk. :mod:`enum` and :mod:`aenum` spell a wholly-undeclared + flag value the same way, so a fix that swapped enumeration libraries would + have changed nothing. + + """ + + ACK = 2048 + SYN = 16384 + + +class AnnotatedFlags(enum.IntFlag): + """A flag enumeration whose pseudo-members carry a public attribute. + + This exists to reach the hook's ``addon`` branch with a *nameless* member. + That branch fires only when ``o.__dict__`` holds a key that does not begin + with an underscore, and a pseudo-member's ``__dict__`` is just + ``{'_value_': …, '_name_': None}`` -- so a plain composite never gets there + and falls through to the scalar return instead. Overriding + :meth:`~enum.Enum._missing_` to annotate the pseudo-member it builds is the + route that does, and it is a documented extension point rather than a poke + at the instance from outside. + + """ + + ACK = 2048 + + @classmethod + def _missing_(cls, value: 'int') -> 'AnnotatedFlags | None': + obj = super()._missing_(value) + if obj is not None: + obj.note = f'undeclared bits {value:#x}' + return obj + + +class BaseDumper: + """Minimal stand-in for :class:`dictdumper.dumper.Dumper`. + + Mirrors the stub in :mod:`tests.dumpkit.test_common_unit`: the hook under + test never reaches ``super().object_hook`` for an enumeration, so nothing + more than a terminating implementation is needed, and this avoids + instantiating a real dumper against the filesystem. + + """ + + def object_hook(self, value): # noqa: ANN001, ANN201 + return {'base': value} + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class NamelessEnumRenderingTests(unittest.TestCase): + """#648 -- a nameless flag member must not render as ``Type::None [n]``. + + :func:`~pcapkit.dumpkit.common.make_dumper`'s ``object_hook`` renders every + enumeration member as ``Type::name [value]``, and it interpolated + :attr:`~enum.Enum.name` unguarded at three separate places. A + :class:`~enum.Flag` value composed entirely of undeclared bits has + ``name is None``, so all three put the literal four characters ``None`` into + the name half. + + The replacement is the value's own decimal spelling -- ``'Flags::0 [0]'``, + ``'Flags::8 [8]'`` -- chosen for three reasons these tests pin: + + * It is what the enumeration libraries themselves already use for an + undeclared residue. ``Flags(2057).name`` is ``'ACK|9'``, naming the + declared bit and giving the leftovers as one decimal number; a + wholly-undeclared value is that rendering with no declared bit in front. + * It cannot be confused with a member name, because a Python identifier may + not begin with a digit. ``'None'`` could be: ``NONE`` is a real declared + name elsewhere in the library, so a consumer splitting the rendering on + ``::`` had no way to tell "no flags set" from a member so named. + * It needs no special case for zero, which matters because the defect never + was about zero. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_scalar_return_renders_a_nameless_member_as_its_value(self) -> None: + """The plain ``return`` at the end of the enumeration branch. + + Covers the nameless value *and* four non-zero ones, because a fix that + special-cased zero would pass on ``Flags(0)`` alone and still emit + ``'Flags::8 [8]'``'s predecessor ``'Flags::None [8]'``. + + """ + from pcapkit.const.tcp.flags import Flags + from pcapkit.dumpkit.common import make_dumper + + dumper = make_dumper(BaseDumper)() + + for value in NAMELESS_VALUES: + with self.subTest(library='aenum', value=value): + member = Flags(value) + # The premise: there is no name to interpolate. + self.assertIsNone(member.name) + rendered = dumper.object_hook(member) + self.assertEqual(rendered, f'Flags::{value} [{value}]') + # The defect, stated as what must no longer appear. Asserted + # separately from the equality above so a future change to the + # rendering cannot quietly reintroduce the literal. + self.assertNotIn('None', rendered) + + with self.subTest(library='enum', value=value): + # Not an ``aenum`` quirk -- stdlib behaves identically. + std = StdFlags(value) + self.assertIsNone(std.name) + self.assertEqual(dumper.object_hook(std), f'StdFlags::{value} [{value}]') + + def test_named_members_are_untouched(self) -> None: + """The control. Only the name half of a *nameless* member changes. + + ``Flags(2049)`` matters most here: its name is ``'ACK|1'``, so it was + never broken, and it is the precedent the fallback follows. If the guard + were written as "compose the name from the value" rather than "use the + value when there is no name", this is the assertion that would catch it. + + """ + from pcapkit.const.tcp.flags import Flags + from pcapkit.dumpkit.common import make_dumper + + dumper = make_dumper(BaseDumper)() + + self.assertEqual(dumper.object_hook(Flags.ACK), 'Flags::ACK [2048]') + self.assertEqual(dumper.object_hook(Flags.SYN | Flags.ACK), 'Flags::ACK|SYN [18432]') + self.assertEqual(dumper.object_hook(Flags(2049)), 'Flags::ACK|1 [2049]') + self.assertEqual(dumper.object_hook(Flags(2057)), 'Flags::ACK|9 [2057]') + + # An explicitly declared zero keeps its declared name, which is the + # clearest demonstration that the guard keys on the *name* and not on the + # value: this member's value is 0 and its rendering is unchanged. + from pcapkit.const.reg.apptype import TransportProtocol + + self.assertEqual(TransportProtocol(0).name, 'undefined') + self.assertEqual(dumper.object_hook(TransportProtocol(0)), + 'TransportProtocol::undefined [0]') + + def test_multidict_key_path_renders_a_nameless_key(self) -> None: + """The ``MultiDict``/``OrderedMultiDict`` *key* path. + + A second, separate interpolation of the same shape. It builds the + dictionary key rather than the value, so a nameless key collapsed every + undeclared flag value onto the single key ``'Flags::None [n]'`` -- and, + for two different nameless values, onto keys distinguished only by the + bracketed half. + + """ + from pcapkit.const.tcp.flags import Flags + from pcapkit.corekit.multidict import MultiDict, OrderedMultiDict + from pcapkit.dumpkit.common import make_dumper + + dumper = make_dumper(BaseDumper)() + + multidict = MultiDict() + multidict.add(Flags(0), 'flagless') + multidict.add(Flags(8), 'undeclared-bit') + multidict.add(Flags.ACK, 'acknowledged') + + converted = dumper.object_hook(multidict) + self.assertEqual(converted['Flags::0 [0]'], ['flagless']) + self.assertEqual(converted['Flags::8 [8]'], ['undeclared-bit']) + self.assertEqual(converted['Flags::ACK [2048]'], ['acknowledged']) + self.assertNotIn('Flags::None [0]', converted) + self.assertNotIn('Flags::None [8]', converted) + + ordered = OrderedMultiDict() + ordered.add(Flags(0), 'first') + ordered.add(Flags(0), 'second') + self.assertEqual(dumper.object_hook(ordered)['Flags::0 [0]'], ['first', 'second']) + + def test_addon_branch_renders_a_nameless_member(self) -> None: + """The third interpolation -- the ``'enum'`` key of the ``addon`` mapping. + + Reached when the member carries public instance attributes, which for a + pseudo-member takes a :meth:`~enum.Enum._missing_` override. No registry + under :mod:`pcapkit.const` is both a flag enumeration and an annotated + one today, so this branch is not reachable from a capture on ``main`` -- + but the interpolation was character-for-character the same as the other + two, so it is guarded with them rather than left as the one place the + literal ``None`` survives. + + """ + from pcapkit.dumpkit.common import make_dumper + + dumper = make_dumper(BaseDumper)() + + member = AnnotatedFlags(8) + self.assertIsNone(member.name) + + converted = dumper.object_hook(member) + self.assertEqual(converted['enum'], 'AnnotatedFlags::8 [8]') + self.assertEqual(converted['note'], 'undeclared bits 0x8') + + # And the named member through the same branch, as the control. + self.assertEqual(dumper.object_hook(AnnotatedFlags.ACK), 'AnnotatedFlags::ACK [2048]') + + def test_no_flag_registry_renders_the_literal_none(self) -> None: + """Every flag enumeration in the library, swept rather than sampled. + + The sweep is the point: #648 was first reported against + :class:`pcapkit.const.tcp.flags.Flags` alone, and five of the seven flag + registries turn out to be nameless at zero -- ``Flags`` plus the four + Mobility Header flag registries. Naming them here would rot the moment + an eighth is added, so they are discovered. + + """ + import aenum + + from pcapkit.dumpkit.common import make_dumper + + dumper = make_dumper(BaseDumper)() + + registries = {} + for module in pkgutil.walk_packages(_const_path(), prefix='pcapkit.const.'): + try: + imported = importlib.import_module(module.name) + except ImportError: # pragma: no cover - a registry that cannot import + continue + for attribute in vars(imported).values(): + if not isinstance(attribute, type) or attribute.__module__ != module.name: + continue + if issubclass(attribute, (enum.Flag, aenum.Flag)): + registries[f'{module.name}.{attribute.__name__}'] = attribute + + # A guard on the sweep itself: an empty mapping would make every + # assertion below vacuous, and that is how this test would rot silently. + self.assertGreaterEqual(len(registries), 7, registries) + + nameless = [] + for label, registry in sorted(registries.items()): + with self.subTest(registry=label): + member = registry(0) + rendered = dumper.object_hook(member) + self.assertNotIn('::None [', rendered) + if member.name is None: + nameless.append(label) + self.assertEqual(rendered, f'{registry.__name__}::0 [0]') + else: + self.assertEqual(rendered, f'{registry.__name__}::{member.name} [0]') + + # Not an incidental detail: if this ever drops to zero the test above + # stops exercising the fix at all and would pass on unfixed code. + self.assertGreaterEqual(len(nameless), 5, nameless) + + +def _const_path() -> 'list[str]': + """The filesystem path of :mod:`pcapkit.const`, for :func:`pkgutil.walk_packages`. + + Taken from the imported package rather than built from ``__file__`` so the + sweep reads the same tree the rest of the test imports from. + + Returns: + A single-element search path for the ``pcapkit.const`` package. + + """ + import pcapkit.const + + return list(pcapkit.const.__path__) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index e374583975..7791caa88c 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -1563,13 +1563,17 @@ def test_a_flagless_segment_seeds_its_connection_flags_as_an_enum(self) -> None: The closing block pins the one difference a consumer can see, so that it is a recorded decision rather than a silent change: a flagless segment's - ``connection`` now dumps as the string ``'Flags::None [0]'`` where it dumped as + ``connection`` now dumps as the string ``'Flags::0 [0]'`` where it dumped as the number ``0``. That is not a regression so much as the removal of an inconsistency -- the field was a number for a flagless segment and a string for - every other one -- but the literal ``None`` in it is a rendering defect of - :func:`~pcapkit.dumpkit.common.make_dumper`'s hook, which interpolates ``o.name`` - without accounting for a nameless composite member. It would do the same to any - zero-valued flag enumeration in the library, so it is left to its own change. + every other one. + + That string read ``'Flags::None [0]'`` when this test was written, because + :func:`~pcapkit.dumpkit.common.make_dumper`'s hook interpolated ``o.name`` + without accounting for a nameless composite member. #648 has since guarded it, + so the name half is now the value's own decimal spelling -- the same spelling + the enumeration library already uses for an undeclared residue, as in + ``Flags(2049).name == 'ACK|1'``. """ import struct @@ -1628,19 +1632,23 @@ def segment(flags_octet: 'int') -> 'bytes': # The one place the change is observable to a consumer, pinned here so it cannot # drift silently. ``make_dumper``'s hook renders any enum member as # ``Type::name [value]``, and an ``aenum.IntFlag`` pseudo-member carrying no bits - # has ``name is None`` -- so a flagless segment dumps as the string - # ``'Flags::None [0]'`` where it used to dump as the number ``0``. Note what that - # replaced: ``connection`` was a JSON *number* for a flagless segment and a - # *string* for every other one, so the field is consistently typed now rather than - # switching type with the flags. The literal ``None`` is a rendering defect in - # ``pcapkit.dumpkit.common`` -- it reads ``o.name`` without accounting for a - # nameless composite member, and would do the same to any zero-valued flag enum in - # the library -- so it is left to its own change rather than fixed from here. + # has ``name is None`` -- so a flagless segment dumps as a *string* where it used + # to dump as the number ``0``. Note what that replaced: ``connection`` was a JSON + # *number* for a flagless segment and a *string* for every other one, so the field + # is consistently typed now rather than switching type with the flags. + # + # The name half of that string was the literal ``None`` until #648 guarded the + # interpolation; it is now the value's own decimal spelling, so the rendering is + # ``'Flags::0 [0]'``. Both halves are asserted here because this segment is the + # only wire-reachable producer of a nameless member in the library, which makes + # this the test that notices if the guard is ever removed. # ``self`` is reached only by the fallback ``super()`` call at the end of the hook, # which an enum never gets to, so the unbound form needs no dumper instance. self.assertIsNone(Enum_Flags(0).name) hook = make_dumper(dictdumper.JSON).object_hook - self.assertEqual(hook(None, proto.info.connection), 'Flags::None [0]') + rendered = hook(None, proto.info.connection) + self.assertEqual(rendered, 'Flags::0 [0]') + self.assertNotIn('None', rendered) self.assertEqual(hook(None, Enum_Flags.ACK), 'Flags::ACK [2048]')