From f39c145d25d7c0ddc1b77e835049f4a7058d8a6a Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 6 Oct 2026 16:51:42 -0400 Subject: [PATCH] =?UTF-8?q?fix(pcap):=20Frame.from=5Fdata=20cannot=20be=20?= =?UTF-8?q?called=20=E2=80=94=20it=20has=20no=20way=20to=20pass=20num=20an?= =?UTF-8?q?d=20header?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ProtocolBase.from_data now takes **kwargs and forwards them to the construction, overriding what _make_data returns, so Frame can be rebuilt with from_data(info, num=..., header=...) and PCAPNG with num/sct/ctx. Caller keywords get the strict construction-keyword check. Drops the stale ts_src/prio examples from the _check_construction_keywords comment (both fixed in #1115), and the _make_data patch in test_make_data_keys_unit. Closes #1126 --- pcapkit/protocols/misc/pcap/frame.py | 6 ++ pcapkit/protocols/protocol.py | 33 ++++++-- .../misc/pcap/test_frame_from_data_runtime.py | 80 +++++++++++++++++++ tests/protocols/test_make_data_keys_unit.py | 16 +--- 4 files changed, 115 insertions(+), 20 deletions(-) create mode 100644 tests/protocols/misc/pcap/test_frame_from_data_runtime.py diff --git a/pcapkit/protocols/misc/pcap/frame.py b/pcapkit/protocols/misc/pcap/frame.py index bef4acda88..ec3ca56dbf 100644 --- a/pcapkit/protocols/misc/pcap/frame.py +++ b/pcapkit/protocols/misc/pcap/frame.py @@ -463,6 +463,12 @@ def _make_data(cls, data: 'Data_Frame') -> 'dict[str, Any]': # type: ignore[ove Returns: Key-value pairs for protocol construction. + Notes: + The frame index and the global header are context of the PCAP file + rather than fields of the frame, so they are not returned here: pass + them to :meth:`~pcapkit.protocols.protocol.ProtocolBase.from_data` as + ``num`` and ``header``. + """ return { 'ts_sec': data.frame_info.ts_sec, diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 73b7df3afc..9958c859eb 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -246,10 +246,9 @@ def _check_construction_keywords(cls: 'type', kwargs: 'dict[str, Any]', # NOTE: A warning rather than an error, because nobody typed these: they are # whatever ``_make_data`` returned, so the defect is a key of that mapping # disagreeing with the signature it is spread into, and the person who meets - # it cannot fix it. Raising would turn every such mismatch -- e.g. ``Frame`` - # returns ``ts_src`` for ``ts_sec`` and ``L2TPv2`` ``prio`` for ``priority`` - # -- into a broken ``from_data``, where the field is merely dropped; the - # warning makes that audible instead. + # it cannot fix it. Raising would turn every such mismatch into a broken + # ``from_data``, where the field is merely dropped; the warning makes that + # audible instead. # # No explicit ``stacklevel``: the default blames the innermost frame outside # :mod:`pcapkit`, which is the ``from_data`` call the reader wants to be @@ -796,24 +795,42 @@ def from_schema(cls, schema: '_ST | dict[str, Any]') -> 'Self': return self @classmethod - def from_data(cls, data: '_PT | dict[str, Any]') -> 'Self': + def from_data(cls, data: '_PT | dict[str, Any]', **kwargs: 'Any') -> 'Self': """Create protocol instance from data. Args: data: Protocol data. + **kwargs: Construction keywords that ``data`` does not carry, such as + the ``num`` and ``header`` of + :class:`~pcapkit.protocols.misc.pcap.frame.Frame`, or the + ``num``, ``sct`` and ``ctx`` of + :class:`~pcapkit.protocols.misc.pcapng.PCAPNG`. They take + precedence over the keywords :meth:`_make_data` returns. Returns: Protocol instance. + Raises: + UnsupportedCall: If any of ``kwargs`` matches no parameter of the + protocol, as for a direct construction. + """ if not isinstance(data, Data): data = cast('_PT', cls.__data__.from_dict(data)) + # NOTE: The caller typed these, so they are checked strictly here, as a + # direct construction would check them. The out-of-band keywords + # ``__init__`` consumes are left out of it. + _check_construction_keywords(cls, { + key: val for key, val in kwargs.items() + if key not in ('_layer', '_protocol', '__context__') + }) + self = cls.__new__(cls) - kwargs = self._make_data(data) + kwargs = {**self._make_data(data), **kwargs} - # NOTE: These keywords came out of ``_make_data``, not out of a caller, so - # the construction keyword check of ``__init__`` warns here instead of + # NOTE: The rest came out of ``_make_data``, not out of a caller, so the + # construction keyword check of ``__init__`` warns here instead of # raising: a key of that mapping which disagrees with the signature it is # spread into is a defect in this protocol, and the caller of # ``from_data`` can do nothing about it. Set for the duration of the call diff --git a/tests/protocols/misc/pcap/test_frame_from_data_runtime.py b/tests/protocols/misc/pcap/test_frame_from_data_runtime.py new file mode 100644 index 0000000000..e96e441907 --- /dev/null +++ b/tests/protocols/misc/pcap/test_frame_from_data_runtime.py @@ -0,0 +1,80 @@ +# -*- coding: utf-8 -*- +"""``Frame.from_data`` rebuilds a captured frame. + +GitHub issue #1126: :meth:`~pcapkit.protocols.protocol.ProtocolBase.from_data` +took nothing but the data, so the ``num`` and ``header`` that +:class:`~pcapkit.protocols.misc.pcap.frame.Frame` requires could not reach it, +and every call raised :exc:`TypeError`. ``from_data`` now forwards extra +keywords to the construction, so the caller passes both alongside the data. + +The frames are read from :file:`examples/captures/in.pcap`, so this belongs to +the runtime tier. + +""" +from __future__ import annotations + +import importlib.util +import os +import unittest + +from tests._support import reimport_once_per_class, sample_path + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class FrameFromDataRuntimeTests(unittest.TestCase): + def setUp(self) -> None: + reimport_once_per_class(self) + + def test_from_data_round_trips_every_frame_byte_for_byte(self) -> None: + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.pcap.header import Header + + path = sample_path('in.pcap') + size = os.path.getsize(path) + with open(path, 'rb') as file: + header = Header(file) + num = 0 + while file.tell() < size: + num += 1 + start = file.tell() + frame = Frame(file, num=num, header=header.info) + end = start + 16 + frame.info.frame_info.incl_len + file.seek(start) + octets = file.read(end - start) + + with self.subTest(frame=num): + rebuilt = Frame.from_data(frame.info, num=num, header=header.info) + self.assertEqual(bytes(rebuilt), octets) + self.assertEqual(rebuilt.info.number, num) + + self.assertEqual(num, 6) + + def test_from_data_keyword_overrides_make_data(self) -> None: + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.pcap.header import Header + + with open(sample_path('in.pcap'), 'rb') as file: + header = Header(file) + frame = Frame(file, num=1, header=header.info) + + rebuilt = Frame.from_data(frame.info, num=1, header=header.info, ts_sec=42) + self.assertEqual(rebuilt.info.frame_info.ts_sec, 42) + + def test_from_data_rejects_a_misspelled_keyword(self) -> None: + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.pcap.header import Header + from pcapkit.utilities.exceptions import UnsupportedCall + + with open(sample_path('in.pcap'), 'rb') as file: + header = Header(file) + frame = Frame(file, num=1, header=header.info) + + with self.assertRaisesRegex(UnsupportedCall, "'heaedr'"): + Frame.from_data(frame.info, num=1, heaedr=header.info) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/test_make_data_keys_unit.py b/tests/protocols/test_make_data_keys_unit.py index e95f870f61..9c46cdedae 100644 --- a/tests/protocols/test_make_data_keys_unit.py +++ b/tests/protocols/test_make_data_keys_unit.py @@ -22,7 +22,6 @@ import io import unittest import warnings -from unittest import mock RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -62,18 +61,11 @@ def test_frame_ts_sec_survives_from_data(self) -> None: frame = Frame(num=1, header=header.info, ts_sec=1234, ts_usec=5, packet=b'\x00' * 14) - # NOTE: ``Frame`` also needs ``num`` and ``header``, which are not part - # of its data, so wrap ``_make_data`` to supply them and still go - # through the real :meth:`from_data` and the real keys. - make_data = Frame._make_data - - def with_context(cls: 'type', data: 'object') -> 'dict[str, object]': - return {**make_data(data), 'num': 1, 'header': header.info} - - with mock.patch.object(Frame, '_make_data', classmethod(with_context)), \ - warnings.catch_warnings(record=True) as caught: + # NOTE: The frame index and the global header are not part of the + # frame's data, so they are passed to :meth:`from_data` alongside it. + with warnings.catch_warnings(record=True) as caught: warnings.simplefilter('always') - rebuilt = Frame.from_data(frame.info) + rebuilt = Frame.from_data(frame.info, num=1, header=header.info) self.assertNoUnknownField(caught) self.assertEqual(rebuilt.info.frame_info.ts_sec, 1234)