From 0a2d79b72843ecc57e1c2a52a5a20573c64f5ac9 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 01:36:31 -0400 Subject: [PATCH] fix(protocol): refuse a construction keyword no signature declares (#617) **Behaviour change to a public API.** Building a protocol through its constructor with a keyword that names nothing now raises `UnsupportedCall` instead of discarding it. * Every `make` in the tree ends its signature with `**kwargs` and reads nothing out of it, so a misspelled keyword was accepted, dropped, and the field it named kept its default -- wrong octets, with nothing said. That is what #602 cost: `seq=1` where `TCP.make` spells the parameter `seq_no`, and 25 fixture frames carried sequence number `0` against an empty `warnings` list. The schema layer already warns `UnknownFieldWarning` for a field it does not know; this closes the asymmetry from the other end. * Checked in `ProtocolBase.__init__`, against the union of every keyword-taking parameter of `make`, `read`, `pack`, `unpack`, `__post_init__` and `__init__` across the MRO -- wider than `make` alone because `__post_init__` hands one `**kwargs` to the construction *and* the parse, so `HIP.read`'s `extension` legitimately travels through `HIP.make`. Parsing is untouched, and a direct `SomeProtocol.make(...)` call is not covered -- documented, and pinned by a test. * New `ProtocolBase.__keywords__`: a set for a keyword read out of `**kwargs` by name, `None` for a dispatcher that cannot enumerate its own. `HTTP` is the one user of `None`; it is not inherited, so `HTTPv1`/`HTTPv2` stay checked. * Reading that attribute needs an absent-versus-`None` marker, and it is `_Absent`, an instance of a `@final` `_AbsentType` carrying `__bool__` and `__repr__`, rather than a bare `object()`. `None` is unavailable because it is the opt-out above, and `NoValue` is not reused: it is documented as the value of `FieldBase.default`, and `protocol.py` imports nothing from `corekit.fields` today. * `from_data` warns rather than raises -- its keywords come from `_make_data`, not from a caller. That makes three latent defects audible instead of fatal: `Frame` returns `ts_src` for `ts_sec`, `L2TPv2` `prio` for `priority`, and `Header` a `magic_number` `make` does not take. * Fixed the four misspellings this surfaced, all #602 residue: `_TCP_BASE` in `examples/generators/dispatch.py` and three stale copies under `tests/protocols/transport/`. Fixes #617. New `tests/protocols/test_construction_keyword_check_unit.py`: 22 tests, 37 subtests, of which 15 fail on `main` for the keyword check and 2 more for the sentinel. Two further sentinel tests check the behaviour rather than the marker's shape, and catch the mistake a swap like this actually makes -- comparing against a second instance of the right type fails them with `TypeError: '_AbsentType' object is not iterable`. The sentinel's six statements are covered by that file alone, so `protocol.py` holds 98% coverage with the missed-statement count unchanged at 9. The 21 generated fixtures are byte-identical and the generator log is unchanged. --- docs/source/ext.rst | 56 ++ examples/generators/dispatch.py | 16 +- pcapkit/protocols/application/http.py | 12 + pcapkit/protocols/protocol.py | 355 +++++++- .../test_construction_keyword_check_unit.py | 860 ++++++++++++++++++ .../test_option_generator_tcp_base_unit.py | 72 +- tests/protocols/test_protocol_base_unit.py | 17 +- .../test_tcp_mptcp_capable_length_unit.py | 6 +- .../test_tcp_mptcp_join_flag_ordering_unit.py | 45 +- .../test_tcp_mptcp_length_arithmetic_unit.py | 6 +- .../transport/test_tcp_mptcp_subtype_unit.py | 6 +- 11 files changed, 1405 insertions(+), 46 deletions(-) create mode 100644 tests/protocols/test_construction_keyword_check_unit.py diff --git a/docs/source/ext.rst b/docs/source/ext.rst index d12d27cf0..5315f48f7 100644 --- a/docs/source/ext.rst +++ b/docs/source/ext.rst @@ -227,6 +227,62 @@ The following code snippet shows how to create a new protocol class: # register protocol class register_ethertype(EtherType.Internet_Protocol_version_4, MyIPv4) +.. important:: + + **Declare every construction keyword your protocol accepts.** Since #617, + building a protocol *through its constructor* with a keyword that no signature + declares raises :exc:`~pcapkit.utilities.exceptions.UnsupportedCall` rather than + discarding it, so a misspelling costs an exception instead of a silently wrong + field. The accepted set is read from :func:`inspect.signature` -- the union of every + keyword-taking parameter of ``make``, ``read``, ``pack``, ``unpack``, + ``__post_init__`` and ``__init__`` anywhere in the class's MRO -- which is + wider than ``make`` alone because :meth:`ProtocolBase.__post_init__ + ` hands the same + ``**kwargs`` to the construction and to the parse, so a keyword only ``read`` + declares still travels through ``make``. + + Two shapes a signature cannot express are declared on the class instead, via + :attr:`ProtocolBase.__keywords__ + `: + + .. code-block:: python + + class MyIPv4(Internet[IPv4Data, IPv4Schema], + schema=IPv4Schema, data=IPv4Data): + #: A keyword read out of ``**kwargs`` by name rather than declared as a + #: parameter, as ``ESP.read`` does with ``packet``. Unioned down the MRO. + __keywords__ = frozenset({'my_extra_keyword'}) + + def read(self, length=None, **kwargs): + extra = kwargs.get('my_extra_keyword') + ... + + Setting it to :obj:`None` skips the check entirely, and is meant only for a + *dispatcher* whose real signature belongs to a class chosen at call time -- + :meth:`HTTP.make ` is the one + such class in the library. Unlike a set, :obj:`None` is not inherited, so a + subclass of a dispatcher is checked normally. Prefer a declared parameter to + either: it is also what documents the keyword to your callers. + + Parsing is unaffected, and so is :meth:`ProtocolBase.from_data + `, which warns + :exc:`~pcapkit.utilities.warnings.UnknownFieldWarning` instead -- its keywords + come from your ``_make_data``, so a mismatch there is a disagreement between + two of your own mappings rather than a caller's typo. + + .. warning:: + + The check lives in :meth:`ProtocolBase.__init__ + `, where every producer's + keywords converge, so it covers ``SomeProtocol(...)`` and the ``pack`` it + leads to -- but **not a direct ``SomeProtocol.make(...)`` call**, which still + discards an undeclared keyword in silence. ``object.__new__(cls).make(...)`` + is the idiom that reaches it, used by this package's own tests and by + :meth:`HTTP.make ` to reach its + versioned implementation. Covering that would mean interposing on every + ``make`` in the tree, which is a larger change than #617 and was deliberately + not made. Construct through the constructor to get the check. + .. note:: Registering after the fact, as above, is one option. The other is passing diff --git a/examples/generators/dispatch.py b/examples/generators/dispatch.py index cdeba0426..7c70f755e 100644 --- a/examples/generators/dispatch.py +++ b/examples/generators/dispatch.py @@ -91,9 +91,19 @@ #: TCP header fields held constant across every case that needs a TCP segment. #: Only ``dstport``/``payload`` vary. -_TCP_BASE = dict(srcport=50000, seq=1, ack=0, ns=False, cwr=False, ece=False, - urg=False, ack_flag=False, psh=False, rst=False, syn=True, - fin=False, window=8192, checksum=b'\x00\x00', urgent_pointer=0) +#: +#: Every key is a parameter :meth:`TCP.make ` +#: declares, which is not a style point: this mapping used to read ``seq=1``, +#: ``ack=0``, ``ack_flag=False`` and ``urgent_pointer=0``, of which only ``ack`` +#: was a parameter at all -- and it is the acknowledgement *flag*, so the +#: acknowledgement number was never set while the flag was. The other three were +#: absorbed by ``make``'s trailing ``**kwargs`` and discarded, so every segment +#: built here carried sequence number ``0`` however plainly this said ``1``. That +#: is the same defect as #602, and since #617 an undeclared keyword raises +#: :exc:`~pcapkit.utilities.exceptions.UnsupportedCall` rather than going quiet. +_TCP_BASE = dict(srcport=50000, seq_no=1, ack_no=0, ns=False, cwr=False, ece=False, + urg=False, ack=False, psh=False, rst=False, syn=True, + fin=False, window=8192, checksum=b'\x00\x00', urgent=0) ############################################################################### diff --git a/pcapkit/protocols/application/http.py b/pcapkit/protocols/application/http.py index 4cbdb2aba..092f71efc 100644 --- a/pcapkit/protocols/application/http.py +++ b/pcapkit/protocols/application/http.py @@ -41,6 +41,18 @@ class HTTP(Application[_PT, _ST], Generic[_PT, _ST]): #: Saved subclass protocol data (only for HTTP base class). _http: 'HTTP[_PT, _ST]' + #: This class is a version dispatcher rather than a protocol with a header of + #: its own, so its construction keywords cannot be enumerated: :meth:`make` + #: declares only ``version`` and forwards everything else to + #: :meth:`HTTPv1.make ` or + #: :meth:`HTTPv2.make ` + #: according to that value -- so the set of names that is correct here depends + #: on an argument. :obj:`None` therefore opts out of the construction keyword + #: check that :meth:`ProtocolBase.__init__ + #: ` performs (#617); the + #: two versioned classes are checked normally when constructed directly. + __keywords__ = None + ########################################################################## # Properties. ########################################################################## diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 0432389a9..39ec547b7 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -14,8 +14,10 @@ import abc import collections import contextlib +import difflib import enum import functools +import inspect import io import os import shutil @@ -38,11 +40,11 @@ from pcapkit.protocols.schema.misc.raw import Raw as Schema_Raw from pcapkit.protocols.schema.schema import Schema from pcapkit.utilities.chardet import detect -from pcapkit.utilities.compat import cached_property +from pcapkit.utilities.compat import cached_property, final from pcapkit.utilities.decorators import beholder, seekset from pcapkit.utilities.exceptions import (ProtocolNotFound, ProtocolNotImplemented, RegistryError, StructError, UnsupportedCall) -from pcapkit.utilities.warnings import RegistryWarning, warn +from pcapkit.utilities.warnings import RegistryWarning, UnknownFieldWarning, warn if TYPE_CHECKING: from enum import IntEnum as StdlibEnum @@ -63,6 +65,239 @@ # readable characters' order list readable = [ord(char) for char in filter(lambda char: not char.isspace(), string.printable)] +#: Keywords that configure the construction rather than naming a field, and are +#: therefore consumed by :meth:`ProtocolBase.__init__ +#: ` or by the schema layer +#: instead of by a :meth:`make `. They +#: are declared by no signature, so :func:`_declared_keywords` cannot find them +#: and they are listed here instead. +#: +#: ``packet`` is here because the library puts it there itself, rather than +#: because a caller might: :meth:`ProtocolBase.__init__ +#: ` injects +#: ``packet=self.packet.payload`` into every parsed ``_info``, so the default +#: :meth:`ProtocolBase._make_data +#: ` -- which is +#: ``data.to_dict()`` -- carries it into the keywords that +#: :meth:`ProtocolBase.from_data ` +#: reconstructs from. Refusing it would make ``from_data`` fail on any protocol +#: whose ``make`` does not happen to declare a ``packet``, starting with +#: :class:`~pcapkit.protocols.misc.null.NoPayload`, which is reached for the +#: innermost layer of every packet. It is a field name for some protocols all the +#: same -- :meth:`HIP.make ` takes the +#: HIP packet *type* under that name -- and listing it here does not change how it +#: binds, only that it is never refused. +OUT_OF_BAND_KEYWORDS = frozenset({'_layer', '_protocol', '__context__', + '__packet__', 'packet'}) + + +@final +class _AbsentType: + """Type of :data:`_Absent`, the absent-key sentinel. + + A distinct class rather than a bare :obj:`object` so that the sentinel has a + name of its own in a traceback or a debugger, and so that a type checker has + something to name where ``object()`` would give it nothing. It + follows :class:`~pcapkit.corekit.fields.field.NoValueType`, which does the + same job for an unset field default; this is a sibling of it rather than a + reuse, since that one is documented as the default value of + :attr:`FieldBase.default ` + and means "no value was given", not "this key is not here". + + """ + + def __bool__(self) -> 'Literal[False]': + """Return :obj:`False`.""" + return False + + def __repr__(self) -> 'str': + """Return :obj:`str` representation of the sentinel.""" + return '' + + +#: _AbsentType: Absent-versus-:obj:`None` sentinel for reading ``__keywords__`` +#: out of a class :attr:`~object.__dict__`, where :obj:`None` is a meaningful +#: value -- it is the opt-out that says the class cannot enumerate its keywords, +#: c.f. :attr:`ProtocolBase.__keywords__ +#: `. Never leaves this +#: module: it is read in :func:`_declared_keywords` and discarded there. +_Absent = _AbsentType() + +#: Cache for :func:`_declared_keywords`, keyed by protocol class. A protocol's +#: signatures do not change after the class is created, and the walk below is +#: :math:`O(\\text{MRO} \\times \\text{methods})`, so it is done once per class +#: rather than once per constructed packet. +_DECLARED_KEYWORDS = {} # type: dict[type, Optional[frozenset[str]]] + +#: Methods that a construction keyword may legitimately be destined for. The +#: keywords handed to :class:`Protocol` are forwarded to all of them -- see +#: :meth:`ProtocolBase.__post_init__ +#: `, which passes the +#: same ``**kwargs`` to :meth:`pack ` +#: (and through it to ``make``) *and* to :meth:`unpack +#: ` (and through it to ``read``). +_KEYWORD_CONSUMERS = ('make', 'read', 'pack', 'unpack', '__post_init__', '__init__') + + +def _declared_keywords(cls: 'type') -> 'Optional[frozenset[str]]': + """Collect every keyword the protocol ``cls`` declares a parameter for. + + Args: + cls: Protocol class to inspect. + + Returns: + Names of every keyword-acceptable parameter declared by any of + :data:`_KEYWORD_CONSUMERS` anywhere in the MRO of ``cls``, plus every + entry of :attr:`ProtocolBase.__keywords__ + ` found there, + plus :data:`OUT_OF_BAND_KEYWORDS`. :obj:`None` if ``cls`` *itself* sets + ``__keywords__`` to :obj:`None`, meaning its keywords cannot be enumerated + and are not to be checked -- inherited :obj:`None` does not count, for the + reason given at the read below. + + The union is deliberately wider than the signature of ``cls.make`` alone, + because a keyword reaching ``make`` is not necessarily *for* ``make``: + :meth:`ProtocolBase.__post_init__ + ` hands one + ``**kwargs`` to both the construction and the parse of the packet it has just + constructed, so a keyword declared by ``read`` travels through ``make`` as + well. :class:`~pcapkit.protocols.internet.hip.HIP` is the live example -- + :meth:`HIP.read ` declares + ``extension`` and :meth:`HIP.make ` + does not, yet :meth:`HIP.__post_init__ + ` forwards it to both. + Rejecting on ``make`` alone would reject that, which is correct code. + + The walk covers the whole MRO rather than the most derived override of each + method, for the same reason: a subclass that declares its own keyword and + forwards the rest to its parent must not make the parent's keywords + unreachable. + + """ + try: + return _DECLARED_KEYWORDS[cls] + except KeyError: + pass + + unchecked = False + names = set(OUT_OF_BAND_KEYWORDS) + for klass in cls.__mro__: + # NOTE: A keyword read out of ``**kwargs`` by name rather than declared + # as a parameter is invisible to :func:`inspect.signature`, so the class + # says so itself. Read per class in the MRO, for the same reason the + # methods are: a subclass should not have to repeat its parents'. + keywords = klass.__dict__.get('__keywords__', _Absent) + if keywords is None: + # NOTE: The :obj:`None` opt-out is *not* inherited, unlike a set, + # which is unioned down the MRO. It describes how the class that + # declares it dispatches, which is not a property its subclasses + # share: :class:`~pcapkit.protocols.application.http.HTTP` cannot + # enumerate its keywords because it forwards them to whichever of + # :class:`HTTPv1 ` and + # :class:`HTTPv2 ` the + # ``version`` names -- but those two declare theirs in full, and + # inheriting the opt-out would silently exempt the very classes that + # can be checked. A subclass that dispatches in turn says so itself. + if klass is cls: + unchecked = True + elif keywords is not _Absent: + names.update(keywords) + + for method in _KEYWORD_CONSUMERS: + # NOTE: Read from ``__dict__`` rather than with :func:`getattr`, so + # that each class in the MRO contributes its *own* definition instead + # of the most derived one over and over. An ``@overload``-decorated + # stub is overwritten by the implementation that follows it, which is + # what lands here. + func = klass.__dict__.get(method) + if func is None: + continue + + try: + signature = inspect.signature(func) + except (TypeError, ValueError): # pragma: no cover + # NOTE: A C-implemented or otherwise unintrospectable callable is + # skipped rather than fatal: failing to widen the accepted set is + # a false rejection, so the safe move is to keep walking. + continue + + for name, param in signature.parameters.items(): + if name in ('self', 'cls'): + continue + if param.kind in (inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.KEYWORD_ONLY): + names.add(name) + + declared = None if unchecked else frozenset(names) + _DECLARED_KEYWORDS[cls] = declared + return declared + + +def _check_construction_keywords(cls: 'type', kwargs: 'dict[str, Any]', + strict: 'bool' = True) -> 'None': + """Reject construction keywords that the protocol ``cls`` declares nowhere. + + Args: + cls: Protocol class being constructed. + kwargs: Keywords remaining after :meth:`ProtocolBase.__init__ + ` has consumed the + out-of-band ones. + strict: Whether an unexpected keyword is an error. :data:`True` for a + caller's own construction; :data:`False` when the keywords were + generated by :meth:`ProtocolBase._make_data + ` rather than + written by anybody -- see :meth:`ProtocolBase.from_data + `. + + Raises: + UnsupportedCall: If ``strict`` and any keyword matches no parameter of + :data:`_KEYWORD_CONSUMERS` anywhere in the MRO of ``cls``. + + Warns: + UnknownFieldWarning: The same finding when not ``strict``. + + """ + declared = _declared_keywords(cls) + if declared is None: + return + + unexpected = sorted(key for key in kwargs if key not in declared) + if not unexpected: + return + + # NOTE: The whole point of the check is a misspelling, so name the neighbour + # that was probably meant: ``seq`` for ``seq_no`` and ``ack_flag`` for + # ``ack`` are both a :func:`difflib.get_close_matches` hit, and the message + # is the only place the caller looks before reading the signature. + report = [] # type: list[str] + for key in unexpected: + suggestions = difflib.get_close_matches(key, declared, n=1) + report.append(f'{key!r} (did you mean {suggestions[0]!r}?)' if suggestions else repr(key)) + listed = ', '.join(report) + + if strict: + raise UnsupportedCall(f'{cls.__name__}: unexpected keyword(s): {listed}') + + # 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 is not the person who can fix it. Raising would also turn three latent + # defects of exactly that shape into a broken ``from_data`` -- ``Frame`` + # returns ``ts_src`` for ``ts_sec``, ``Header`` an undeclared + # ``magic_number``, ``L2TPv2`` ``prio`` for ``priority`` -- each of which has + # been losing that field in silence and each of which belongs to its own + # change. This is what makes them audible meanwhile. + # + # No explicit ``stacklevel``: the default blames the innermost frame outside + # :mod:`pcapkit`, which is the ``from_data`` call the reader wants to be + # pointed at, and it stays right if the frames between here and there ever + # change, where a hardcoded count would not. It is also what + # :meth:`Schema.__update__ ` + # passes for the warning this one is the counterpart of. + warn(f'{cls.__name__}._make_data returned keyword(s) that no signature of ' + f'{cls.__name__} declares, so they are discarded: {listed}', + UnknownFieldWarning) + class ProtocolMeta(abc.ABCMeta): """Meta class to add dynamic support to :class:`Protocol`. @@ -124,6 +359,46 @@ class ProtocolBase(Generic[_PT, _ST], metaclass=ProtocolMeta): lambda: ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw'), ) + #: Construction keywords this protocol consumes out of ``**kwargs`` instead + #: of declaring as a parameter, e.g. with ``kwargs.get('spam')`` in + #: :meth:`read` -- as :meth:`ESP.read ` + #: does with ``packet``. :func:`~pcapkit.protocols.protocol._declared_keywords` + #: finds a protocol's keywords by reading its signatures, which cannot see + #: such a name, so a protocol that consumes one names it here and the + #: construction check of :meth:`__init__` accepts it. The union over the MRO + #: is used, so a subclass need not repeat its parents' entries. + #: + #: Declaring the parameter is preferable where it is possible, since that is + #: also what documents the keyword to the caller and to :mod:`inspect`. This + #: is for the cases where it is not -- a keyword handled uniformly for a whole + #: family of names, say -- and *not* a way to reopen the silence #617 closed: + #: it is opt-in per class, so it can only ever exempt a name whose author + #: wrote it down. + #: + #: :obj:`None` means the keywords cannot be enumerated at all and the check is + #: skipped for this protocol. That is for a *dispatcher*, whose real signature + #: belongs to a class chosen at call time: + #: :meth:`HTTP.make ` declares + #: only ``version`` and forwards everything else to + #: :meth:`HTTPv1.make ` or + #: :meth:`HTTPv2.make ` + #: depending on that value, so no set of names is right for it. Use it only + #: for that shape; a protocol that forgoes the check gets the pre-#617 + #: behaviour back, and with it the silence. Unlike a set, the :obj:`None` is + #: **not** inherited: a subclass of a dispatcher is checked normally unless it + #: dispatches too and says so, because ``HTTPv1`` and ``HTTPv2`` declare their + #: keywords in full and exempting them along with their base would forgo the + #: check on the only two classes here that can have it. + __keywords__: 'Optional[frozenset[str]]' = frozenset() + + #: Whether this instance is being rebuilt by :meth:`from_data` from a parsed + #: data model, as against constructed from keywords somebody wrote. It governs + #: only whether the construction keyword check of :meth:`__init__` raises or + #: warns (#617), and is set for the duration of that call alone -- the class + #: level :data:`False` is what every other code path sees, including an + #: instance built without going through ``__init__`` at all. + __reconstructing__: 'bool' = False + #: Caller supplied parsing context, c.f. :mod:`pcapkit.corekit.context`. #: :meth:`self.__init__ ` replaces this with a real #: :class:`~pcapkit.corekit.context.ContextRegistry`; the class level @@ -264,6 +539,34 @@ def make(self, **kwargs: 'Any') -> '_ST': Returns: Curated protocol schema data. + Note: + The ``**kwargs`` here absorbs the keywords that + :meth:`ProtocolBase.__post_init__ + ` hands to the + parse as well as to the construction, so an implementation is not + expected to declare every keyword it is called with. It is *not* a + place for a caller to put a keyword no signature declares: since + #617, building a protocol *through its constructor* with such a + keyword raises :exc:`~pcapkit.utilities.exceptions.UnsupportedCall` + from :meth:`ProtocolBase.__init__ + ` rather than + discarding it. + + Warning: + **Calling this method directly is not checked**, and still discards an + undeclared keyword in silence. The check lives in + :meth:`ProtocolBase.__init__ + `, so it covers + ``SomeProtocol(...)`` and the :meth:`pack` it leads to, but not + ``SomeProtocol.make(...)`` on an instance obtained some other way -- + ``object.__new__(cls).make(**kwargs)`` is the idiom, used by this + package's own tests and by :meth:`HTTP.make + ` to reach its versioned + implementation. Covering it would mean interposing on every ``make`` + in the tree rather than on the one place their keywords converge, which + is a larger change than #617 and deliberately not made here. Construct + through the constructor to get the check. + """ def pack(self, **kwargs: 'Any') -> 'bytes': @@ -492,8 +795,19 @@ def from_data(cls, data: '_PT | dict[str, Any]') -> 'Self': self = cls.__new__(cls) kwargs = self._make_data(data) - # initialize protocol instance - self.__init__(**kwargs) # type: ignore[misc] + # NOTE: These keywords came out of ``_make_data``, not out of a caller, so + # the construction keyword check of ``__init__`` (#617) 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 + # and removed afterwards, so an instance built this way is afterwards + # indistinguishable from one built directly. + self.__reconstructing__ = True + try: + # initialize protocol instance + self.__init__(**kwargs) # type: ignore[misc] + finally: + del self.__reconstructing__ return self @@ -544,6 +858,15 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional :meth:`self._import_next_layer `. **kwargs: Arbitrary keyword arguments. + Raises: + UnsupportedCall: When constructing (``file`` is :obj:`None`), if a + keyword names no parameter of this protocol's :meth:`make`, + :meth:`read`, :meth:`pack`, :meth:`unpack`, + :meth:`__post_init__` or :meth:`__init__`, anywhere in the MRO, + and is not listed in :attr:`__keywords__`. See #617; until then + such a keyword was silently discarded. Parsing (``file`` is + given) is unaffected. + Note: Three of the keywords above are *out-of-band*: they configure the parse rather than describing the packet, and every one of them is @@ -640,6 +963,30 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional if parsing and '__packet__' not in kwargs and isinstance(kwargs.get('packet'), dict): kwargs['__packet__'] = dict(kwargs['packet']) + # NOTE: Construction only. A keyword that names no parameter of this + # protocol is a mistake rather than a value, and until #617 it was + # silently discarded: every ``make`` in the tree ends its signature with + # ``**kwargs`` and never reads it, so the keyword reached the schema as + # nothing at all and the field kept its default. The cost was measured on + # #602, where ``TCP_BASE`` asked for ``seq=1`` -- which ``TCP.make`` + # spells ``seq_no`` -- and 25 generated fixture frames carried ``seq = 0`` + # with an empty ``warnings`` list to show for it. The schema layer has + # never been that permissive: :meth:`Schema.__update__ + # ` warns + # :exc:`~pcapkit.utilities.warnings.UnknownFieldWarning` for a field it + # does not know, and this closes the asymmetry from the other end. + # + # Parsing is left alone. There, the keywords are not field values but + # whatever the engines and the four ``_import_next_layer`` + # implementations forward -- ``alias``, ``packet``, and the limits + # normalised above -- and a protocol has no way to know which of its + # ancestors' keywords its parent chose to pass on. Nothing was ever lost + # that way either: a dropped parse keyword changes how a packet is read, + # not what the octets say. + if not parsing: + _check_construction_keywords( + type(self), kwargs, strict=not self.__reconstructing__) + # post-init customisations self.__post_init__(file, length, **kwargs) # type: ignore[arg-type] diff --git a/tests/protocols/test_construction_keyword_check_unit.py b/tests/protocols/test_construction_keyword_check_unit.py new file mode 100644 index 000000000..931dc9f1c --- /dev/null +++ b/tests/protocols/test_construction_keyword_check_unit.py @@ -0,0 +1,860 @@ +# -*- coding: utf-8 -*- +"""A construction keyword no signature declares is refused, not discarded. + +Every ``make`` in the tree ends its signature with ``**kwargs: 'Any'`` and reads +nothing out of it, so until issue #617 a keyword it did not declare was accepted, +dropped, and the field it named kept its default. That is the worst shape a defect +can take in a packet library: the octets are wrong, nothing says so, and the +mapping that produced them reads correctly. + +The cost is not hypothetical. Issue #602 was +:data:`examples.generators.options.TCP_BASE` asking for ``seq=1`` where +:meth:`TCP.make ` spells the parameter +``seq_no``. Measured on the tree before this fix:: + + declared seq = 1 built info.seq = 0 + warnings captured: [] + +Twenty-five generated fixture frames carried sequence number ``0`` while the +generator said ``1``, one octet per frame, and the only reason anybody found out +was that somebody read the mapping against the signature. Issues #541 and #556 +were the same family of silence. + +The schema layer has never been so permissive: :meth:`Schema.__update__ +` warns +:exc:`~pcapkit.utilities.warnings.UnknownFieldWarning` for a field it does not +know (:file:`pcapkit/protocols/schema/schema.py`). The asymmetry between the two +is what #617 is about, and +:meth:`AsymmetryTests.test_the_schema_layer_still_warns_where_construction_now_raises` +pins both halves of it in one place. + +What is checked, and what is deliberately not +--------------------------------------------- + +The check runs in :meth:`ProtocolBase.__init__ +` rather than in ``make``, +because ``make`` is not the only consumer of the keywords it is handed: +:meth:`ProtocolBase.__post_init__ +` passes one ``**kwargs`` +to the construction *and* to the parse of what it has just constructed, so a +keyword declared only by ``read`` travels through ``make`` as well. +:class:`~pcapkit.protocols.internet.hip.HIP` is the live example and +:class:`ForwardedKeywordTests` is where it is pinned; rejecting on ``make``'s +signature alone would reject correct code. + +Two things are left alone on purpose, and each has a test saying so: + +:meth:`ScopeTests.test_the_parse_path_is_unaffected` + Parsing. There the keywords are not field values but whatever the engines and + the four ``_import_next_layer`` implementations forward -- ``alias``, + ``packet``, and the parse limits -- and a protocol cannot know which of its + ancestors' keywords its parent chose to pass on. Nothing was ever lost that + way either: a dropped parse keyword changes how a packet is read, not what + the octets say. + +:meth:`ScopeTests.test_out_of_band_keywords_are_accepted_while_constructing` + The out-of-band keywords, which configure the call rather than naming a + field and are consumed before any ``make`` sees them. + +And one thing is softened rather than left alone, in :class:`ReconstructionTests`: +:meth:`ProtocolBase.from_data ` +warns where a caller would be raised at, because the keywords it spreads came out +of :meth:`ProtocolBase._make_data +` rather than out of anybody's +editor. Three protocols have a mismatch of exactly that kind today and have been +losing a field to it in silence; they are recorded there. + +This module is unit tier: it constructs its own octets and reads no capture, so +it runs on a fresh checkout with nothing generated. + +""" + +from __future__ import annotations + +import collections +import importlib.util +import inspect +import io +import unittest +import warnings +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from typing import Any + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: A correct TCP header, spelled the way :meth:`TCP.make +#: ` declares it. The baseline every +#: rejection below is a single-keyword departure from, so that a test failing +#: says "this keyword" rather than "something in here". +GOOD_HEADER = { + 'srcport': 50000, 'dstport': 80, 'seq_no': 1, 'ack_no': 0, + 'syn': True, 'window': 8192, 'checksum': b'\x00\x00', 'urgent': 0, +} + +#: The three misspellings of #602, as ``(wrong spelling, the parameter meant)``. +MISSPELLINGS = ( + ('seq', 'seq_no'), + ('ack_flag', 'ack'), + ('urgent_pointer', 'urgent'), +) + + +def _protocol_class(**namespace: 'Any') -> 'Any': + """Build a minimal :class:`~pcapkit.protocols.protocol.Protocol` subclass. + + Purpose-built rather than borrowed from a real protocol, because the shapes + under test here -- a ``read`` that declares what ``make`` does not, a class + that names a keyword in ``__keywords__`` -- have to be varied one at a time, + and no single real protocol offers all of them. The real protocols are pinned + by signature instead, in :class:`ForwardedKeywordTests`. + + Args: + **namespace: Extra class attributes, e.g. an overriding ``read`` or a + ``__keywords__``. + + Returns: + The protocol class, whose ``make`` declares ``spam`` and nothing else. + + """ + from pcapkit.corekit.fields.misc import PayloadField + from pcapkit.corekit.infoclass import info_final + from pcapkit.corekit.protochain import ProtoChain + from pcapkit.protocols.data.data import Data + from pcapkit.protocols.protocol import Protocol + from pcapkit.protocols.schema.schema import Schema, schema_final + + @info_final + class DummyData(Data): + value: int = 0 + + @schema_final + class DummySchema(Schema): + payload: bytes = PayloadField(length=lambda packet: packet['__length__'], default=b'') + + # NOTE: A ``class`` statement rather than a ``type()`` call with a namespace + # dict, on both counts: a generic base needs ``__mro_entries__`` resolution + # that ``type()`` does not do, and ``read``/``make`` have to be *in* the body + # because :class:`abc.ABCMeta` computes ``__abstractmethods__`` at class + # creation and assigning them afterwards leaves the class abstract. + class DummyProtocol(Protocol[DummyData, DummySchema], + schema=DummySchema, data=DummyData): + __layer__ = 'Internet' + __proto__ = collections.defaultdict(lambda: None) + + @property + def name(self) -> 'str': + return 'Dummy Protocol' + + @property + def length(self) -> 'int': + return 2 + + def read(self, length: 'int | None' = None, **kwargs: 'Any') -> 'DummyData': + from pcapkit.protocols.misc.null import NoPayload + + self._next = NoPayload() + self._protos = ProtoChain(type(self), self.alias, basis=self._next.protochain) + return DummyData(value=0) + + def make(self, spam: 'bytes' = b'ab', **kwargs: 'Any') -> 'DummySchema': + return DummySchema(payload=spam) + + @classmethod + def __index__(cls) -> 'int': # type: ignore[override] + return 250 + + # Overriding a method that is already concrete, so this does not reopen + # ``__abstractmethods__``. Nothing reads the signatures until the first + # construction, so a later assignment is still seen by the check. + for attribute, value in namespace.items(): + setattr(DummyProtocol, attribute, value) + + return DummyProtocol + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class RejectionTests(unittest.TestCase): + """An undeclared construction keyword raises rather than vanishing.""" + + def test_the_three_misspellings_of_the_defect_are_each_refused(self) -> None: + """Each key of #602, put back one at a time, raises and is named. + + ``TCP_BASE`` is correct today, so reproducing the report means + reintroducing the misspelling. Before the fix each of these built a + segment whose field kept its default -- ``seq=1`` giving ``info.seq == 0`` + -- and captured no warning at all. + + """ + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import UnsupportedCall + + for wrong, right in MISSPELLINGS: + keywords = dict(GOOD_HEADER) + keywords[wrong] = keywords.pop(right, 0) + + with self.subTest(keyword=wrong): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + with self.assertRaises(UnsupportedCall) as context: + TCP(**keywords) + + message = str(context.exception) + self.assertIn('TCP', message) + self.assertIn(repr(wrong), message) + # The exception is the whole signal; nothing is also warned. + self.assertEqual([str(item.message) for item in caught], []) + + def test_the_correct_spelling_still_builds_the_segment_it_describes(self) -> None: + """The check rejects only what it should. + + A rejection test on its own is satisfied by a check that rejects + everything, which would be a far worse defect than the one being fixed. + This is the other half: the same header, spelled correctly, still + produces the octets :rfc:`9293` puts at those offsets. + + """ + from pcapkit.protocols.transport.tcp import TCP + + segment = TCP(**GOOD_HEADER) + octets = bytes(segment) + + self.assertEqual(segment.info.seq, 1) + self.assertEqual(octets[0:2], (50000).to_bytes(2, 'big')) + self.assertEqual(octets[4:8], (1).to_bytes(4, 'big')) + self.assertTrue(octets[13] & 0x02) + + def test_every_keyword_the_signature_declares_is_accepted(self) -> None: + """No declared parameter of a real ``make`` is refused. + + Derived from :func:`inspect.signature` rather than from a list, so a + parameter added to ``TCP.make`` tomorrow is covered without an edit here. + A hand-written list is exactly the thing that would let the check drift + into rejecting a legitimate keyword. + + """ + from pcapkit.protocols.protocol import _declared_keywords + from pcapkit.protocols.transport.tcp import TCP + + accepted = _declared_keywords(TCP) + for name, parameter in inspect.signature(TCP.make).parameters.items(): + # ``self`` is a parameter of the unbound function and never a keyword + # a caller may pass, so the accepted set excludes it on purpose. + if name == 'self' or parameter.kind not in (parameter.POSITIONAL_OR_KEYWORD, + parameter.KEYWORD_ONLY): + continue + with self.subTest(keyword=name): + self.assertIn(name, accepted) + + def test_several_unexpected_keywords_are_all_reported(self) -> None: + """The message names every offender, not just the first. + + A caller who has misspelled two keywords should learn both in one run, + rather than fixing one and rediscovering the other. + + """ + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import UnsupportedCall + + with self.assertRaises(UnsupportedCall) as context: + TCP(srcport=1, dstport=2, zzz_second=2, aaa_first=1) + + message = str(context.exception) + self.assertIn(repr('aaa_first'), message) + self.assertIn(repr('zzz_second'), message) + # Sorted, so the message is the same however the caller ordered them. + self.assertLess(message.index('aaa_first'), message.index('zzz_second')) + + def test_a_near_miss_is_named_in_the_message(self) -> None: + """A misspelling one edit from a real parameter suggests it. + + The whole population this check exists for is typists, so the message + carries the neighbour rather than making the caller open the signature. + ``seq`` for ``seq_no`` is the exact case that cost #602 its fixture bytes. + + """ + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import UnsupportedCall + + with self.assertRaises(UnsupportedCall) as context: + TCP(**{**GOOD_HEADER, 'seq': GOOD_HEADER['seq_no']}) + + self.assertIn("did you mean 'seq_no'?", str(context.exception)) + + def test_the_exception_is_the_one_the_library_already_uses(self) -> None: + """``UnsupportedCall``, as for an unexpected *class* keyword. + + Not a new exception type: the library already answers "you passed a + keyword I do not accept" with + :exc:`~pcapkit.utilities.exceptions.UnsupportedCall`, in five places with + this exact message shape -- ``protocol.py:1064``, + ``dumpkit/common.py:142``, ``foundation/reassembly/reassembly.py:580``, + ``foundation/engines/engine.py:313`` and + ``foundation/traceflow/traceflow.py:514``. Reusing it keeps one answer to + one question rather than adding a second. + + The honest caveat, recorded so it is a decision rather than an oversight: + all five of those reject a *class* keyword at ``__init_subclass__`` time, + which is a narrower kind of "unexpected keyword" than a field value passed + to a constructor. And ``UnsupportedCall`` carries :exc:`AttributeError`, + where a reader expecting the stdlib's ``unexpected keyword argument`` would + reach for :exc:`TypeError` -- and where an ``except AttributeError`` written + for duck-typing could swallow this. Consistency with the five precedents was + preferred to a sixth spelling of the same idea, but the trade is real. + + """ + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import BaseError, UnsupportedCall + + with self.assertRaises(UnsupportedCall) as context: + TCP(srcport=1, no_such_tcp_field=1) + + self.assertIsInstance(context.exception, BaseError) + self.assertIsInstance(context.exception, AttributeError) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class ForwardedKeywordTests(unittest.TestCase): + """A keyword declared downstream of ``make`` is still accepted.""" + + def test_hip_declares_extension_on_read_and_not_on_make(self) -> None: + """The real asymmetry the check has to tolerate. + + :meth:`HIP.read ` declares + ``extension``, :meth:`HIP.make ` + does not, and :meth:`HIP.__post_init__ + ` forwards it to both -- + so ``examples/generators/options.py``'s ``_hip_build`` passing + ``extension=True`` on the construction path is correct code that a check + against ``make`` alone would have broken. Asserted from the signatures, so + it fails if either side of the asymmetry moves. + + """ + from pcapkit.protocols.internet.hip import HIP + from pcapkit.protocols.protocol import _declared_keywords + + def keywords(method: 'Any') -> 'set[str]': + return { + name for name, parameter in inspect.signature(method).parameters.items() + if parameter.kind in (parameter.POSITIONAL_OR_KEYWORD, + parameter.KEYWORD_ONLY) + } + + self.assertIn('extension', keywords(HIP.read)) + self.assertNotIn('extension', keywords(HIP.make)) + self.assertIn('extension', _declared_keywords(HIP)) + + def test_a_keyword_only_read_declares_is_accepted_end_to_end(self) -> None: + """The same shape, constructed rather than inspected. + + HIP's own construction needs a valid parameter set and a matching + version, which is a statement about HIP rather than about this check, so + the end-to-end half is done on a protocol built for it. ``eggs`` is + declared by ``read`` only, and the construction has to survive it. + + """ + def read(self, length: 'int | None' = None, *, eggs: 'int' = 0, + **kwargs: 'Any') -> 'Any': + from pcapkit.corekit.protochain import ProtoChain + from pcapkit.protocols.misc.null import NoPayload + + self._next = NoPayload() + self._protos = ProtoChain(type(self), self.alias, basis=self._next.protochain) + self.seen_eggs = eggs + return type(self).__data__(value=eggs) + + protocol = _protocol_class(read=read) + instance = protocol(spam=b'ab', eggs=7) + + self.assertEqual(instance.seen_eggs, 7) + self.assertEqual(bytes(instance), b'ab') + + def test_keywords_declared_by_a_class_attribute_are_accepted(self) -> None: + """``__keywords__`` covers a name read out of ``**kwargs``. + + Signatures cannot show a keyword a method reads with + ``kwargs.get('spam')`` -- which :meth:`ESP.read + ` does with ``packet``, and which + a third-party protocol may do with anything. Such a protocol names the + keyword on the class instead, so the check has an answer other than + "break it". Opt-in per class, so it can only exempt a name whose author + wrote it down. + + """ + def read(self, length: 'int | None' = None, **kwargs: 'Any') -> 'Any': + from pcapkit.corekit.protochain import ProtoChain + from pcapkit.protocols.misc.null import NoPayload + + self._next = NoPayload() + self._protos = ProtoChain(type(self), self.alias, basis=self._next.protochain) + self.seen_eggs = kwargs.get('eggs') + return type(self).__data__(value=0) + + from pcapkit.utilities.exceptions import UnsupportedCall + + undeclared = _protocol_class(read=read) + with self.assertRaises(UnsupportedCall): + undeclared(spam=b'ab', eggs=7) + + declared = _protocol_class(read=read, __keywords__=frozenset({'eggs'})) + self.assertEqual(declared(spam=b'ab', eggs=7).seen_eggs, 7) + + def test_a_subclass_inherits_the_declarations_of_its_parents(self) -> None: + """The accepted set is the union over the MRO, not the most derived. + + A subclass that declares its own keyword and forwards the rest must not + make its parent's keywords unreachable, and a subclass that declares + nothing must not lose its parent's ``__keywords__``. + + """ + from pcapkit.protocols.protocol import _declared_keywords + + parent = _protocol_class(__keywords__=frozenset({'eggs'})) + + class Child(parent): # type: ignore[misc,valid-type] + __keywords__ = frozenset({'beans'}) + + def make(self, ham: 'bytes' = b'ab', **kwargs: 'Any') -> 'Any': + return type(self).__schema__(payload=ham) + + accepted = _declared_keywords(Child) + for name in ('eggs', 'beans', 'ham', 'spam'): + with self.subTest(keyword=name): + self.assertIn(name, accepted) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class ScopeTests(unittest.TestCase): + """What the check deliberately leaves alone.""" + + def test_the_parse_path_is_unaffected(self) -> None: + """Dissection still tolerates a keyword nothing declares. + + The engines and every ``_import_next_layer`` forward ``alias``, + ``packet`` and the parse limits down a chain whose members cannot know + what their parents chose to pass on, so rejecting there would break + dissection rather than catch a misspelling -- and nothing is lost either + way, because a dropped parse keyword changes how a packet is read, not + what its octets say. + + """ + from pcapkit.protocols.transport.tcp import TCP + + octets = bytes(TCP(**GOOD_HEADER)) + parsed = TCP(io.BytesIO(octets), len(octets), + no_such_field=1, alias='whatever') + + self.assertEqual(parsed.info.seq, 1) + self.assertEqual(parsed.info.srcport.port, 50000) + + def test_a_direct_make_call_is_not_checked(self) -> None: + """The limitation, recorded rather than left to be discovered. + + The check sits in :meth:`ProtocolBase.__init__ + `, where every producer's + keywords converge, so ``SomeProtocol(...)`` is covered and a *direct* + ``SomeProtocol.make(...)`` is not: it still absorbs an undeclared keyword + and discards it. ``object.__new__(cls).make(**kwargs)`` is the idiom that + reaches it -- used by several modules of this suite, and by + :meth:`HTTP.make ` itself to + reach its versioned implementation. + + Closing it would mean interposing on each of the thirty ``make`` + implementations rather than on the one place their keywords meet, which is + a larger change than #617. Written down here because the alternative is + somebody inferring from the docstrings that ``make`` validates, and because + if the gap is ever closed this test goes red and gets deleted. + + """ + from pcapkit.protocols.internet.ipv4 import IPv4 + + bare = object.__new__(IPv4) + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + schema = bare.make(offst=5, protocl=6, payload=b'\xaa' * 8) + + # Both keywords are misspellings -- ``offset`` and ``protocol`` -- and both + # go the way they always did: absorbed, dropped, nothing said. + self.assertFalse(hasattr(schema, 'offst')) + self.assertEqual([str(item.message) for item in caught], []) + + def test_out_of_band_keywords_are_accepted_while_constructing(self) -> None: + """The keywords that configure the call rather than naming a field. + + ``_layer``, ``_protocol``, ``__context__`` and ``__packet__`` are + consumed by :meth:`ProtocolBase.__init__ + ` or by the schema + layer, so no ``make`` declares them and the check has to know them by + name. A regression here would refuse a caller that is setting a parse + limit on something it is constructing. + + ``packet`` is the fifth, and is there for a different reason -- see + :meth:`test_from_data_survives_the_injected_packet_keyword`. + + """ + from pcapkit.protocols.protocol import OUT_OF_BAND_KEYWORDS + from pcapkit.protocols.transport.tcp import TCP + + segment = TCP(**GOOD_HEADER, _layer='Internet', _protocol='tcp', + __context__=None, __packet__={}) + + self.assertEqual(segment.info.seq, 1) + self.assertEqual(segment._exlayer, 'Internet') + self.assertEqual( + OUT_OF_BAND_KEYWORDS, + frozenset({'_layer', '_protocol', '__context__', '__packet__', 'packet'})) + + def test_from_data_survives_the_injected_packet_keyword(self) -> None: + """Rebuilding a parsed packet is not a caller misspelling something. + + :meth:`ProtocolBase.__init__ ` + injects ``packet=self.packet.payload`` into every parsed ``_info``, and the + default :meth:`ProtocolBase._make_data + ` is ``data.to_dict()`` + -- so ``from_data`` hands ``packet`` to a ``make`` that very often does not + declare it. :class:`~pcapkit.protocols.misc.null.NoPayload` is the case + that matters, because :meth:`ProtocolBase._make_payload + ` reaches it for the + innermost layer of every packet: refusing ``packet`` broke ``from_data`` + for all of them. Measured before ``packet`` was made out-of-band:: + + UnsupportedCall: NoPayload: unexpected keyword(s): 'packet' + + What is asserted is that the rebuild happens at all. Whether it reproduces + the original octets is a separate question about ``from_data``'s fidelity, + which this change neither improves nor worsens. + + """ + from pcapkit.protocols.internet.ipv4 import IPv4 + + octets = bytes(IPv4(src='127.0.0.1', dst='127.0.0.2')) + parsed = IPv4(io.BytesIO(octets), len(octets)) + rebuilt = IPv4.from_data(parsed.info) + + self.assertIsInstance(rebuilt, IPv4) + self.assertEqual(rebuilt.info.src, parsed.info.src) + self.assertEqual(rebuilt.info.dst, parsed.info.dst) + + def test_a_dispatcher_may_decline_the_check(self) -> None: + """``__keywords__ = None`` skips it, for a protocol that cannot enumerate. + + :meth:`HTTP.make ` declares + only ``version`` and forwards every other keyword to + :class:`HTTPv1 ` or + :class:`HTTPv2 ` according to + that value, so no fixed set of names is correct for it. Measured before the + opt-out existed:: + + UnsupportedCall: HTTP: unexpected keyword(s): 'http_version', 'method', 'uri' + + Asserted through the real class rather than a dummy, because the point is + that this one protocol declines and the rest do not. The construction still + fails, with :exc:`~pcapkit.utilities.exceptions.ProtocolError` from + ``HTTPv1.make`` itself -- which is the outcome + :meth:`HTTPUnitTests.test_http_construction_reaches_the_versioned_make_callee` + already pins, and is the proof that the keywords reached the delegate + rather than being refused on the way. + + """ + from pcapkit.protocols.application.http import HTTP + from pcapkit.protocols.application.httpv1 import HTTP as HTTPv1 + from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 + from pcapkit.protocols.protocol import _declared_keywords + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import ProtocolError + + self.assertIsNone(_declared_keywords(HTTP)) + self.assertIsNotNone(_declared_keywords(TCP)) + + # The opt-out is not inherited. Both versioned classes declare their + # keywords in full, and exempting them along with their base would forgo + # the check on the only two HTTP classes that can carry it -- which is what + # happened on the first attempt at this, measured before the ``klass is + # cls`` guard: every HTTP subclass came back unchecked. + for versioned, keyword in ((HTTPv1, 'http_version'), (HTTPv2, 'sid')): + with self.subTest(protocol=versioned.__module__): + accepted = _declared_keywords(versioned) + self.assertIsNotNone(accepted) + self.assertIn(keyword, accepted) + + with self.assertRaises(ProtocolError) as context: + HTTP(version=1, http_version='1.1', method='GET', uri='/') + self.assertEqual(str(context.exception), 'HTTP/1: invalid format') + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class ReconstructionTests(unittest.TestCase): + """``from_data`` warns where a caller would be raised at, and why.""" + + def test_from_data_warns_instead_of_raising(self) -> None: + """A ``_make_data`` key the signature does not take is audible, not fatal. + + :meth:`ProtocolBase.from_data ` + spreads whatever :meth:`ProtocolBase._make_data + ` returned into + ``__init__``, so nobody typed those keywords: a mismatch is a defect in the + protocol's own pair of mappings, and the caller who meets it cannot fix it. + Raising there would also convert three latent defects of that shape into a + broken ``from_data`` -- see + :meth:`test_the_three_known_make_data_mismatches_are_recorded`. + + ``L2TPv2`` is used because it is one of the three and therefore exercises + the real path rather than a contrived one. When its ``_make_data`` is fixed + this test goes red, which is the point: the entry is then deleted. + + """ + from pcapkit.protocols.link.l2tpv2 import L2TPv2 + from pcapkit.utilities.warnings import UnknownFieldWarning + + octets = bytes(L2TPv2(version=2, tunnel_id=1, session_id=2, payload=b'ab')) + parsed = L2TPv2(io.BytesIO(octets), len(octets)) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + rebuilt = L2TPv2.from_data(parsed.info) + + self.assertIsInstance(rebuilt, L2TPv2) + messages = [item for item in caught + if isinstance(item.message, UnknownFieldWarning)] + self.assertEqual(len(messages), 1, [str(item.message) for item in caught]) + self.assertIn("'prio'", str(messages[0].message)) + self.assertIn('_make_data', str(messages[0].message)) + + def test_the_three_known_make_data_mismatches_are_recorded(self) -> None: + """The latent defects this check made visible, named so they can be fixed. + + Each of these has ``_make_data`` returning a key that no signature of the + same protocol declares, so :meth:`ProtocolBase.from_data + ` has been dropping that + field in silence -- a frame's timestamp, an L2TPv2 priority bit, and a + capture's byte order. They are recorded rather than fixed here because each + is a defect in its own protocol rather than in this mechanism, and because + two of them need a decision about what ``make`` should be called rather than + a rename. + + Written as an expected-failure table for the reason the round-trip module + writes its own that way: fixing one of these turns this red and the entry + gets deleted, where a silent skip would leave the defect recorded forever. + + """ + from pcapkit.protocols.link.l2tpv2 import L2TPv2 + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.pcap.header import Header + from pcapkit.protocols.protocol import _declared_keywords + + # protocol -> the ``_make_data`` key it returns that nothing declares + recorded = {Frame: 'ts_src', L2TPv2: 'prio', Header: 'magic_number'} + + for protocol, key in recorded.items(): + with self.subTest(protocol=protocol.__name__): + accepted = _declared_keywords(protocol) + self.assertIsNotNone(accepted) + self.assertNotIn(key, accepted, ( + f'{protocol.__name__} now declares {key!r}, so its _make_data ' + f'mismatch is fixed -- delete this entry' + )) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class AsymmetryTests(unittest.TestCase): + """The two layers, and what each now does with a name it does not know.""" + + def test_the_schema_layer_still_warns_where_construction_now_raises(self) -> None: + """One unknown name, two layers, both of them audible. + + This is the asymmetry #617 reported, asserted from both ends so that it + cannot drift apart again unnoticed. The schema keeps warning rather than + raising: :meth:`Schema.__update__ + ` is the constructor of + every schema in the tree and runs on the parse path too, so tightening it + is a separate change with a far wider blast radius -- deliberately out of + scope here, and recorded as such. + + """ + from pcapkit.protocols.schema.transport.tcp import TCP as Schema_TCP + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import UnsupportedCall + from pcapkit.utilities.warnings import UnknownFieldWarning + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + Schema_TCP(srcport=1, dstport=2, no_such_field=3) + + self.assertEqual([type(item.message) for item in caught], [UnknownFieldWarning]) + self.assertIn('not a valid field name', str(caught[0].message)) + + with self.assertRaises(UnsupportedCall): + TCP(srcport=1, dstport=2, no_such_field=3) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class SentinelTests(unittest.TestCase): + """The absent-key marker is an instance of a named class, not a bare object. + + :func:`~pcapkit.protocols.protocol._declared_keywords` has to tell three + answers apart when it reads ``__keywords__`` out of a class + :attr:`~object.__dict__`: a set of names, the :obj:`None` that declines the + check, and the key not being there at all. :obj:`None` is taken by the second + of those -- :attr:`ProtocolBase.__keywords__ + ` is typed + ``Optional[frozenset[str]]`` and :class:`~pcapkit.protocols.application.http.HTTP` + sets it -- so the third needs a marker of its own. + + That marker is :data:`~pcapkit.protocols.protocol._Absent`, an instance of + :class:`~pcapkit.protocols.protocol._AbsentType` following + :class:`~pcapkit.corekit.fields.field.NoValueType`, which is how this library + already spells a singleton marker. It was a bare ``object()`` when #640 was + first raised; a bare ``object()`` has no name in a traceback, no informative + :func:`repr`, and no type a checker can hold anyone to. + + The four tests here split two and two, and the split is worth knowing before + reading them: + + :meth:`test_the_absent_marker_is_an_instance_of_its_own_type` and + :meth:`test_the_absent_marker_cannot_be_confused_with_another_singleton` + pin the marker's *shape*. Both name ``_Absent``, so on the bare + ``object()`` they fail at the import -- which makes them a check that the + rename happened, with the substance behind the gate. + + :meth:`test_the_absent_marker_never_reaches_the_accepted_names` and + :meth:`test_the_three_answers_stay_distinct_across_the_swap` + pin the *behaviour*, and **both pass on the bare** ``object()`` **too**. + That is the point of them rather than a weakness: they are the evidence + that the swap changed nothing, which a shape assertion cannot give. What + they catch is the mistake this kind of swap actually makes -- comparing + against a second instance of the right class, which reads correctly and + typechecks -- measured by mutating the ``.get`` default to a fresh + ``_AbsentType()``, at which point both fail with ``TypeError: + '_AbsentType' object is not iterable`` from ``names.update(keywords)`` + while both shape tests pass through the mutation unharmed. + + So neither pair is sufficient alone: the first pair cannot tell a correct + marker from a correctly-named broken one, and the second cannot tell + ``object()`` from a typed singleton. + + """ + + def test_the_absent_marker_is_an_instance_of_its_own_type(self) -> None: + """It has a type of its own, and the falsiness the convention carries.""" + from pcapkit.corekit.fields.field import NoValue, NoValueType + from pcapkit.protocols.protocol import _Absent, _AbsentType + + self.assertIsInstance(_Absent, _AbsentType) + + # The point of #640's review comment: ``type(object())`` is ``object``, + # which says nothing about what the value is for. + self.assertIsNot(type(_Absent), object) + + # Falsy, exactly as ``NoValue`` is. + self.assertFalse(_Absent) + self.assertFalse(NoValue) + + # And ``@final``. ``typing.final`` only records ``__final__`` on the + # decorated class from 3.11 on, and 3.10 is in the CI matrix, so the two + # cases are spelled out rather than folded into one ``getattr`` comparison + # of two defaults -- that form reads as an assertion but degrades to + # ``None == None`` below 3.11, passing whether or not either class is + # decorated at all. ``NoValueType`` is the probe for which case this is, + # so the two classes cannot drift apart either way. + if hasattr(NoValueType, '__final__'): + self.assertIs(_AbsentType.__final__, True) # type: ignore[attr-defined] + else: # pragma: no cover + self.assertFalse(hasattr(_AbsentType, '__final__')) + + # A bare ``object()`` reads as ````. + self.assertEqual(repr(_Absent), '') + + def test_the_absent_marker_cannot_be_confused_with_another_singleton(self) -> None: + """No other singleton in the library answers an ``is`` against it. + + :data:`~pcapkit.corekit.fields.field.NoValue` is the near neighbour and + the one deliberately *not* reused here: it is documented as the default + value of :attr:`FieldBase.default + ` and means "no value was + given", where this one means "this key is not here". Sharing an instance + between the two would make either site's marker satisfy the other's test. + + """ + from pcapkit.corekit.fields.field import NoValue, NoValueType + from pcapkit.protocols.protocol import _Absent, _AbsentType + + self.assertIsNot(_Absent, NoValue) + self.assertNotIsInstance(_Absent, NoValueType) + self.assertNotIsInstance(NoValue, _AbsentType) + + # Nor does it compare *equal* to any of them: neither class defines + # ``__eq__``, so identity is the only way either is ever true, and this + # says so rather than leaving it to be assumed. + for other in (None, NotImplemented, Ellipsis, NoValue, object(), frozenset(), ''): + with self.subTest(other=type(other).__name__): + self.assertIsNot(_Absent, other) + self.assertFalse(_Absent == other) + + def test_the_absent_marker_never_reaches_the_accepted_names(self) -> None: + """A class with no ``__keywords__`` anywhere in its MRO still reads clean. + + The failure mode this pins is an identity comparison against the wrong + singleton: the marker then falls through to ``names.update(keywords)``, + which raises :exc:`TypeError` because it is not iterable, or -- worse, if + it ever were -- lands in the accepted set as a member that is not a + keyword name at all. Asserted against a plain class rather than a + protocol, because :class:`~pcapkit.protocols.protocol.ProtocolBase` + declares ``__keywords__`` itself, so no protocol ever reaches the branch + with the whole MRO silent. + + """ + from pcapkit.protocols.protocol import OUT_OF_BAND_KEYWORDS, _declared_keywords + + class Bare: + """No ``__keywords__``, so every class in the MRO takes the absent branch.""" + + def make(self, spam: 'int' = 0, **kwargs: 'Any') -> 'None': + """Declare one keyword, so the result is not merely empty.""" + + self.assertNotIn('__keywords__', Bare.__dict__) + self.assertNotIn('__keywords__', object.__dict__) + + declared = _declared_keywords(Bare) + + self.assertIsNotNone(declared) + self.assertIn('spam', declared) + self.assertTrue(OUT_OF_BAND_KEYWORDS <= declared) + self.assertEqual([name for name in declared if not isinstance(name, str)], []) + + def test_the_three_answers_stay_distinct_across_the_swap(self) -> None: + """Absent, :obj:`None`, and a declared set are still three outcomes. + + A marker that is falsy -- which this one is, following the convention -- + would be read as an opt-out by any ``if not keywords`` written later, and + an empty ``frozenset()`` is the value that makes the two + indistinguishable under truthiness while staying distinct under ``is``. + So the empty set is checked alongside the populated one. + + """ + from pcapkit.protocols.protocol import _declared_keywords + + # Declined: the class names ``None`` itself. + self.assertIsNone(_declared_keywords(_protocol_class(__keywords__=None))) + + # Declared, and falsy. Still checked, and ``spam`` still comes off ``make``. + empty = _declared_keywords(_protocol_class(__keywords__=frozenset())) + self.assertIsNotNone(empty) + self.assertIn('spam', empty) + + # Declared and populated, and inherited from neither of the above. + named = _declared_keywords(_protocol_class(__keywords__=frozenset({'ham'}))) + self.assertIsNotNone(named) + self.assertIn('ham', named) + self.assertIn('spam', named) + + # Absent: nothing of its own, so it takes ``ProtocolBase``'s empty set. + absent = _declared_keywords(_protocol_class()) + self.assertIsNotNone(absent) + self.assertNotIn('ham', absent) + self.assertIn('spam', absent) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/test_option_generator_tcp_base_unit.py b/tests/protocols/test_option_generator_tcp_base_unit.py index c4a0f2613..e1a323eac 100644 --- a/tests/protocols/test_option_generator_tcp_base_unit.py +++ b/tests/protocols/test_option_generator_tcp_base_unit.py @@ -21,6 +21,16 @@ which is the acknowledgement *flag*, not the acknowledgement number ``ack_no``. The pair was the wrong way round, and neither half said so. +That silence is gone: issue #617 has :meth:`ProtocolBase.__init__ +` check a construction keyword +against the protocol's signatures and raise +:exc:`~pcapkit.utilities.exceptions.UnsupportedCall` for one that no signature +declares. ``TCP.make`` still ends in ``**kwargs`` and still reads nothing out of +it -- the check is upstream of it -- so the account above remains the account of +how the defect was possible, and +:meth:`TCPBaseKeywordTests.test_an_undeclared_keyword_is_now_refused_rather_than_absorbed` +is where the new behaviour is pinned. + Why the expected header is written out here ------------------------------------------- @@ -229,35 +239,71 @@ def test_tcp_base_is_the_expected_header(self) -> None: options = _load_generator() self.assertEqual(dict(options.TCP_BASE), EXPECTED_HEADER) - def test_make_still_has_the_kwargs_that_hid_the_defect(self) -> None: - """``make`` really does absorb an undeclared keyword without complaint. + def test_an_undeclared_keyword_is_now_refused_rather_than_absorbed(self) -> None: + """Constructing with an undeclared keyword raises. + + This test used to assert the opposite, and the change is the point. + ``make`` still ends its signature with ``**kwargs`` and still reads + nothing out of it, so on its own it would still absorb a misspelling; what + changed in #617 is that :meth:`ProtocolBase.__init__ + ` now checks the + keywords against the signatures before ``make`` is reached, and refuses a + name none of them declares. That closes the asymmetry with + :class:`~pcapkit.protocols.schema.schema.Schema` construction, which has + always warned :exc:`~pcapkit.utilities.warnings.UnknownFieldWarning` for + an unknown field. - Without this, the check above looks like a style rule. It is not: the - reason a misspelling cost twenty-five wrong fixture frames instead of a - :exc:`TypeError` is the ``**kwargs`` at the end of the signature, and - that nothing ever inspects it. Recording the behaviour here means a - change to it -- ``make`` starting to reject or warn about the leftovers, - as :class:`~pcapkit.protocols.schema.schema.Schema` construction already - warns about an unknown field -- turns this red rather than passing - silently. + The ``**kwargs`` assertion is kept because it says why the check has to + live outside ``make``: were the signature closed, Python would raise on + its own and none of this would be needed. """ from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import UnsupportedCall signature = inspect.signature(TCP.make) self.assertTrue( any(parameter.kind is parameter.VAR_KEYWORD for parameter in signature.parameters.values()), - 'TCP.make no longer takes **kwargs, so an undeclared keyword would ' - 'raise instead of being dropped', + 'TCP.make no longer takes **kwargs, so Python itself would reject an ' + 'undeclared keyword and the check in ProtocolBase.__init__ would be ' + 'redundant rather than load-bearing', ) with warnings.catch_warnings(record=True) as caught: warnings.simplefilter('always') - TCP(srcport=50000, dstport=80, no_such_tcp_field=12345) + with self.assertRaises(UnsupportedCall) as context: + TCP(srcport=50000, dstport=80, no_such_tcp_field=12345) + self.assertIn('no_such_tcp_field', str(context.exception)) + # A warning would be the weaker fix the issue weighed and rejected; the + # silence here is now the silence of an exception having been raised. self.assertEqual([str(warning.message) for warning in caught], []) + def test_the_three_keys_of_the_defect_are_each_refused(self) -> None: + """Each misspelling of #602 raises, named, rather than being dropped. + + ``TCP_BASE`` is correct today, so the mapping itself can no longer + demonstrate the defect. Putting the three keys back one at a time is what + keeps the original report reproducible: before the fix every one of them + built a segment whose field kept its default -- ``seq=1`` produced + ``info.seq == 0`` -- with an empty ``warnings`` list. + + """ + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import UnsupportedCall + + options = _load_generator() + for wrong, right in (('seq', 'seq_no'), ('ack_flag', 'ack'), + ('urgent_pointer', 'urgent')): + keywords = dict(options.TCP_BASE) + keywords[wrong] = keywords.pop(right) + + with self.subTest(keyword=wrong): + with self.assertRaises(UnsupportedCall) as context: + TCP(**keywords) + self.assertIn(repr(wrong), str(context.exception)) + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class TCPBaseHeaderTests(unittest.TestCase): diff --git a/tests/protocols/test_protocol_base_unit.py b/tests/protocols/test_protocol_base_unit.py index 0e819ed9a..ccbfe1352 100644 --- a/tests/protocols/test_protocol_base_unit.py +++ b/tests/protocols/test_protocol_base_unit.py @@ -48,16 +48,25 @@ def name(self) -> str: def length(self) -> int: return 2 - def read(self, length: int | None = None, **kwargs: object) -> DummyData: - data = DummyData(value=kwargs.get('value', 0)) - self._next = kwargs.get('next_protocol') + # NOTE: ``value`` and ``next_protocol`` are declared parameters rather + # than names read out of ``**kwargs``, and ``make`` declares the + # un-prefixed parse limits, because since #617 a construction keyword + # no signature declares is refused rather than dropped. Both spellings + # are faithful to what the library does -- ``IPv4.make`` really does + # take a ``protocol`` -- and declaring them is the pattern #617 asks + # for, as against naming them in ``__keywords__``. + def read(self, length: int | None = None, *, value: int = 0, + next_protocol: object = None, **kwargs: object) -> DummyData: + data = DummyData(value=value) + self._next = next_protocol if self._next is None: from pcapkit.protocols.misc.null import NoPayload self._next = NoPayload() self._protos = ProtoChain(self.__class__, self.alias, basis=self._next.protochain) return data - def make(self, packet: bytes = b'ab', **kwargs: object) -> DummySchema: + def make(self, packet: bytes = b'ab', layer: object = None, + protocol: object = None, **kwargs: object) -> DummySchema: return DummySchema(payload=packet) @classmethod diff --git a/tests/protocols/transport/test_tcp_mptcp_capable_length_unit.py b/tests/protocols/transport/test_tcp_mptcp_capable_length_unit.py index fb624bf7d..9889a2963 100644 --- a/tests/protocols/transport/test_tcp_mptcp_capable_length_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_capable_length_unit.py @@ -242,10 +242,10 @@ class TCPMPTCPCapablePublicConstructorUnitTests(unittest.TestCase): #: Header fields shared by every constructed TCP segment in this class, matching #: :data:`examples.generators.options.TCP_BASE`. TCP_BASE = { - 'srcport': 50000, 'dstport': 80, 'seq': 1, 'ack': 0, - 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack_flag': False, + 'srcport': 50000, 'dstport': 80, 'seq_no': 1, 'ack_no': 0, + 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack': False, 'psh': False, 'rst': False, 'syn': True, 'fin': False, - 'window': 8192, 'checksum': b'\x00\x00', 'urgent_pointer': 0, + 'window': 8192, 'checksum': b'\x00\x00', 'urgent': 0, 'payload': b'', } diff --git a/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py b/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py index de75d24c2..6ab3faa1e 100644 --- a/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py @@ -143,13 +143,20 @@ #: names :meth:`TCP.make ` actually declares. #: #: Deliberately *not* a copy of :data:`examples.generators.options.TCP_BASE`, which the -#: sibling modules in this directory reuse. That mapping passes ``'seq': 1`` and -#: ``'ack_flag': False``, neither of which is a parameter of ``make`` -- both are -#: swallowed by its ``**kwargs`` -- while its ``'ack': 0`` binds to ``ack``, the -#: *acknowledgement flag*, not to ``ack_no``. Measured: ``TCP(**TCP_BASE).info.seq`` is -#: ``0``, not the ``1`` the mapping reads as. Harmless where those modules use it, since -#: they assert nothing about the sequence number, but this module dispatches on the ACK -#: flag and must not leave which-``ack``-is-which to inference. +#: sibling modules in this directory reuse -- and, unlike them, this dict omits +#: ``syn``/``ack`` altogether, since every case here passes those two explicitly to +#: select a layout, where a shared constant could not. Until #617, those sibling +#: modules' own copies of that mapping additionally passed ``'seq': 1`` and +#: ``'ack_flag': False``, neither of which was a parameter of ``make`` -- both were +#: silently swallowed by its ``**kwargs`` -- while their ``'ack': 0`` bound to ``ack``, +#: the *acknowledgement flag*, not to ``ack_no``. Measured pre-#617: +#: ``TCP(**TCP_BASE).info.seq`` read back ``0``, not the ``1`` the mapping appeared to +#: set. That was harmless where those modules used it, since none of them asserted +#: anything about the sequence number -- but #617 turns any keyword ``make`` does not +#: declare into ``UnsupportedCall`` instead of a silent no-op, so those mappings have +#: since been corrected to ``seq_no``/``ack_no``/``ack`` rather than merely tolerated. +#: This module dispatches on the ACK flag and always has, so it could never have +#: afforded to leave which-``ack``-is-which to inference, #617 or not. TCP_HEADER = { 'srcport': 50000, 'dstport': 80, 'seq_no': 1, 'ack_no': 0, 'ns': False, 'cwr': False, 'ece': False, 'urg': False, @@ -279,18 +286,30 @@ class TCPMPTCPJoinFlagOrderingUnitTests(JoinLayoutMixin, unittest.TestCase): """ def test_the_issue_reproduction_constructs(self) -> None: - """#587's reproduction, verbatim, returns an instance instead of raising. - - Kept exactly as the issue filed it -- including ``seq=0, ack=0``, which are not - ``make``'s parameter names for the sequence and acknowledgement numbers -- so - that the case a reader can paste from the issue is the case pinned here. + """#587's reproduction returns an instance instead of raising. + + This used to keep ``seq=0, ack=0`` exactly as the issue filed it, since neither + is ``make``'s parameter name for the sequence and acknowledgement numbers and a + reader pasting the issue's own text would hit precisely this call. #617 removes + that option: ``seq`` is not declared by ``make``'s (or ``read``'s, ``pack``'s, + ``unpack``'s, ``__post_init__``'s, or ``__init__``'s) signature at all -- the + parameter is ``seq_no`` -- so where it used to be silently absorbed by ``make``'s + trailing ``**kwargs`` and leave the sequence number at its default, it now raises + :exc:`~pcapkit.utilities.exceptions.UnsupportedCall`, for a reason that has + nothing to do with what this test is pinning. ``ack=0`` needed no change: ``ack`` + *is* a real parameter -- the connection flag, not the acknowledgement number -- + and ``0`` is an accepted falsy value for it, which is exactly why the issue's + original text could leave it alone too. So only ``seq`` is renamed to ``seq_no`` + here; everything else, including the value ``0``, is unchanged, and the case + still returns an instance rather than raising ``AttributeError: 'TCP' object has + no attribute '_flags'``, which is what #587 is about. """ from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption from pcapkit.const.tcp.option import Option as Enum_Option from pcapkit.protocols.transport.tcp import TCP - tcp = TCP(srcport=1, dstport=2, seq=0, ack=0, syn=True, + tcp = TCP(srcport=1, dstport=2, seq_no=0, ack=0, syn=True, options=[(Enum_Option.Multipath_TCP, {'subtype': Enum_MPTCPOption.MP_JOIN, 'backup': False, 'addr_id': 1, 'token': 7, 'nonce': 9})]) diff --git a/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py b/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py index 7b4fa8b87..62fa45058 100644 --- a/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py @@ -150,10 +150,10 @@ #: :data:`examples.generators.options.TCP_BASE` and #: :data:`tests.protocols.transport.test_tcp_mptcp_subtype_unit.TCP_BASE`. TCP_BASE = { - 'srcport': 50000, 'dstport': 80, 'seq': 1, 'ack': 0, - 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack_flag': False, + 'srcport': 50000, 'dstport': 80, 'seq_no': 1, 'ack_no': 0, + 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack': False, 'psh': False, 'rst': False, 'syn': True, 'fin': False, - 'window': 8192, 'checksum': b'\x00\x00', 'urgent_pointer': 0, + 'window': 8192, 'checksum': b'\x00\x00', 'urgent': 0, 'payload': b'', } diff --git a/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py b/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py index 3522284dd..b3bbaf4e2 100644 --- a/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_subtype_unit.py @@ -106,10 +106,10 @@ #: :data:`examples.generators.options.TCP_BASE` so these cases build through exactly the #: keyword shape the fixture generator uses. TCP_BASE = { - 'srcport': 50000, 'dstport': 80, 'seq': 1, 'ack': 0, - 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack_flag': False, + 'srcport': 50000, 'dstport': 80, 'seq_no': 1, 'ack_no': 0, + 'ns': False, 'cwr': False, 'ece': False, 'urg': False, 'ack': False, 'psh': False, 'rst': False, 'syn': True, 'fin': False, - 'window': 8192, 'checksum': b'\x00\x00', 'urgent_pointer': 0, + 'window': 8192, 'checksum': b'\x00\x00', 'urgent': 0, 'payload': b'', }