From 0f84641544867ad93285f20bbfb426a0e2b4b7d9 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 23 Sep 2026 18:19:05 -0400 Subject: [PATCH] fix(registry): add register_protocol's identity guard to sibling registrars - Ten code-keyed registrars (ProtocolBase, Internet, Link, Transport, SCTP, Frame, PCAPNG, EnumSchema's register + __init_subclass__, and pcapng.py's Option.register) warned on mere presence, so re-registering the exact same class under the same code emitted a misleading "overwriting X with X". Guard each on presence AND identity, matching register_protocol's guard from #681/#711. - Option.register needed its own fix: __init_subclass__ loops over a code list with no deduplication, so code=[b, b] reached it twice with the same class and warned about a self-overwrite. Rewrote its docstring, dropping the now-false admission that this could not happen. - Updated each site's docstring: the "fires on presence alone, deliberate" rationale (added by #695) no longer holds now the guard changed. - Repurposed test_sibling_registries_still_warn_on_an_identical_re_registration (tests/foundation/registry/test_protocols.py) and fixed five other pre-existing tests that re-registered a literal same object as their "overwrite" case. - Added one same-object no-op test per site (nine total), plus a tenth covering EnumSchema.__init_subclass__'s class-declaration path, and two more for Option.register's own code=[b, b] shape (silent on the same class, still warns once on a genuine displacement). - Cross-review found three prose sites that still argued the rejected reasoning: register_protocol's own docstring (foundation/registry/ protocols.py) claiming every sibling warns on mere presence, a test_pcapng_unit.py test docstring claiming __init_subclass__ passes each code exactly once, and a one-line summary in test_enum_schema_registry_unit.py calling the guard presence-only. Fixed all three; grepped every test file this PR touches for the same phrasing, no further instances. Did not apply #711's id() disambiguation to the siblings -- see PR body. Build: targeted pytest run (9 files), 204 passed, 1930 subtests, exit 0. Option.register: test_pcapng_unit.py, 81 passed, 1753 subtests, pcapng.py at 100% line/branch coverage, exit 0. Re-verified with the two other touched test files: 104 passed, 1838 subtests, exit 0. Fixes #718. --- pcapkit/foundation/registry/protocols.py | 32 ++--- pcapkit/protocols/internet/internet.py | 12 +- pcapkit/protocols/link/link.py | 14 ++- pcapkit/protocols/misc/pcap/frame.py | 12 +- pcapkit/protocols/misc/pcapng.py | 9 +- pcapkit/protocols/protocol.py | 36 +++--- pcapkit/protocols/schema/misc/pcapng.py | 32 +++-- pcapkit/protocols/schema/schema.py | 30 +++-- pcapkit/protocols/transport/sctp.py | 10 +- pcapkit/protocols/transport/transport.py | 12 +- tests/foundation/registry/test_protocols.py | 52 ++++---- .../protocols/internet/test_internet_unit.py | 32 ++++- tests/protocols/link/test_link_unit.py | 41 ++++++- .../misc/pcap/test_header_frame_unit.py | 42 ++++++- tests/protocols/misc/test_pcapng_unit.py | 116 ++++++++++++++++-- .../schema/test_enum_schema_registry_unit.py | 110 ++++++++++++++++- tests/protocols/test_protocol_base_unit.py | 29 ++++- tests/protocols/transport/test_sctp_unit.py | 31 ++++- .../transport/test_transport_unit.py | 51 +++++++- 19 files changed, 571 insertions(+), 132 deletions(-) diff --git a/pcapkit/foundation/registry/protocols.py b/pcapkit/foundation/registry/protocols.py index 6f725dae94..775fc9a891 100644 --- a/pcapkit/foundation/registry/protocols.py +++ b/pcapkit/foundation/registry/protocols.py @@ -168,22 +168,22 @@ def register_protocol(protocol: 'Type[Protocol]') -> 'None': ` and the other overwrite-warning registries. - The guard here deliberately reads "key present **and** incumbent is a - different class", where every sibling registry warns on mere presence. The - difference is in where the key comes from. A sibling is keyed on a ``code`` - the caller passes, so a repeat call is a caller mistake worth reporting even - when the value is identical. This registry's key is *derived* from the class, - and this function is the funnel every wrapper registrar calls -- - :func:`register_tcp`, :func:`register_udp`, :func:`register_apptype`, - :func:`register_linktype` and the rest all end in - ``register_protocol(module)``. So registering one class under two codes, a - supported and documented thing to do, reaches this function twice with the - same class and nothing at stake; on a presence-only guard that would warn - about an overwrite that overwrote nothing. Warning on the harmless case is - not free: it is what teaches a caller to filter - :exc:`~pcapkit.utilities.warnings.RegistryWarning` wholesale, and that - filter is what would then hide the ``HTTP`` collision this warning exists to - surface. + The guard here reads "key present **and** incumbent is a different + class" -- presence alone is not enough. This registry's key is *derived* + from the class rather than supplied by a caller, and this function is the + funnel every wrapper registrar calls -- :func:`register_tcp`, + :func:`register_udp`, :func:`register_apptype`, :func:`register_linktype` + and the rest all end in ``register_protocol(module)``. So registering one + class under two codes, a supported and documented thing to do, reaches + this function twice with the same class and nothing at stake; a + presence-only guard would warn about an overwrite that overwrote nothing. + Warning on the harmless case is not free: it is what teaches a caller to + filter :exc:`~pcapkit.utilities.warnings.RegistryWarning` wholesale, and + that filter is what would then hide the ``HTTP`` collision this warning + exists to surface. The sibling ``register`` methods across the package -- + each keyed on a caller-supplied ``code`` rather than a name derived from + the class -- apply the same identity criterion as of GitHub issue #718; + before that they warned on presence alone, and none of them does now. Making the key itself unique would resolve the collision rather than merely reporting it, but it is a registry-format change that the bare-name diff --git a/pcapkit/protocols/internet/internet.py b/pcapkit/protocols/internet/internet.py index fe6af9279d..a98d91093e 100644 --- a/pcapkit/protocols/internet/internet.py +++ b/pcapkit/protocols/internet/internet.py @@ -153,18 +153,20 @@ def register(cls, code: 'Enum_TransType', protocol: 'ModuleDescriptor[Protocol] pcapkit.utilities.warnings.RegistryWarning: If this transport-layer protocol number is already registered, naming the displaced entry and its replacement so a caller can tell *what* was lost. - Fires on presence alone -- see :meth:`ProtocolBase.register - ` for why that - differs from ``register_protocol``. + Fires only when the incumbent differs from the replacement -- + see :meth:`ProtocolBase.register + ` for the + guard this shares with ``register_protocol``. """ if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not issubclass(protocol, Protocol): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - if code in cls.__proto__: + incumbent = cls.__proto__.get(code) + if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' - f'{cls.__proto__[code]!r} with {protocol!r}', RegistryWarning) + f'{incumbent!r} with {protocol!r}', RegistryWarning) cls.__proto__[code] = protocol ########################################################################## diff --git a/pcapkit/protocols/link/link.py b/pcapkit/protocols/link/link.py index 70bec0e849..7732e3066b 100644 --- a/pcapkit/protocols/link/link.py +++ b/pcapkit/protocols/link/link.py @@ -132,19 +132,21 @@ def register(cls, code: 'Enum_EtherType', protocol: 'ModuleDescriptor[Protocol] Warns: pcapkit.utilities.warnings.RegistryWarning: If this EtherType is already registered, naming the displaced entry and its - replacement so a caller can tell *what* was lost. Fires on - presence alone -- see :meth:`ProtocolBase.register - ` for why that - differs from ``register_protocol``. + replacement so a caller can tell *what* was lost. Fires only + when the incumbent differs from the replacement -- see + :meth:`ProtocolBase.register + ` for the + guard this shares with ``register_protocol``. """ if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not issubclass(protocol, Protocol): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - if code in cls.__proto__: + incumbent = cls.__proto__.get(code) + if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' - f'{cls.__proto__[code]!r} with {protocol!r}', RegistryWarning) + f'{incumbent!r} with {protocol!r}', RegistryWarning) cls.__proto__[code] = protocol ########################################################################## diff --git a/pcapkit/protocols/misc/pcap/frame.py b/pcapkit/protocols/misc/pcap/frame.py index 2a05f037ac..a39f234b73 100644 --- a/pcapkit/protocols/misc/pcap/frame.py +++ b/pcapkit/protocols/misc/pcap/frame.py @@ -140,18 +140,20 @@ def register(cls, code: 'Enum_LinkType', protocol: 'ModuleDescriptor[Protocol] | pcapkit.utilities.warnings.RegistryWarning: If this link type is already registered against PCAP frames, naming the displaced entry and its replacement so a caller can tell *what* was lost. - Fires on presence alone -- see :meth:`ProtocolBase.register - ` for why that - differs from ``register_protocol``. + Fires only when the incumbent differs from the replacement -- + see :meth:`ProtocolBase.register + ` for the + guard this shares with ``register_protocol``. """ if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not issubclass(protocol, Protocol): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - if code in cls.__proto__: + incumbent = cls.__proto__.get(code) + if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' - f'{cls.__proto__[code]!r} with {protocol!r}', RegistryWarning) + f'{incumbent!r} with {protocol!r}', RegistryWarning) cls.__proto__[code] = protocol def index(self, name: 'str | Protocol | Type[Protocol]') -> 'int': diff --git a/pcapkit/protocols/misc/pcapng.py b/pcapkit/protocols/misc/pcapng.py index 4788dc4aa2..5b9fe9bb21 100644 --- a/pcapkit/protocols/misc/pcapng.py +++ b/pcapkit/protocols/misc/pcapng.py @@ -874,7 +874,9 @@ def register(cls, code: 'Enum_LinkType', protocol: 'ModuleDescriptor[Protocol] | pcapkit.utilities.warnings.RegistryWarning: If this link type is already registered against PCAP-NG blocks, naming the displaced entry and its replacement so a caller can tell *what* was lost. - Note this registry is separate from the PCAP one, so + Fires only when the incumbent differs from the replacement, so + re-registering the same class is a silent no-op. Note this + registry is separate from the PCAP one, so :func:`~pcapkit.foundation.registry.protocols.register_linktype` writing to both cannot make either warn about the other. @@ -883,9 +885,10 @@ def register(cls, code: 'Enum_LinkType', protocol: 'ModuleDescriptor[Protocol] | protocol = protocol.klass if not issubclass(protocol, Protocol): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - if code in cls.__proto__: + incumbent = cls.__proto__.get(code) + if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' - f'{cls.__proto__[code]!r} with {protocol!r}', RegistryWarning) + f'{incumbent!r} with {protocol!r}', RegistryWarning) cls.__proto__[code] = protocol @classmethod diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index c71cafee9f..5e694c9cf6 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -778,32 +778,30 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor | Type[ProtocolBase]' only that something was. Note: - The guard fires on the mere presence of ``code``, including when the - incumbent and the replacement denote the same protocol. That is - deliberate, and differs from :func:`register_protocol - `, which - additionally requires the incumbent to be a *different* class. Two - things separate them. This - registry is keyed on a ``code`` the caller supplies, independently of - the value, so registering one class under two codes yields two keys - and never reaches the same key twice -- the spurious-warning case - that motivated the narrower guard cannot arise here, while a repeat - call for one code is a caller mistake worth reporting even when the - value is unchanged. And the incumbent may still be an unresolved - :class:`~pcapkit.corekit.module.ModuleDescriptor` while the - replacement is the very class it names, so "a different class" is not - decidable here without resolving the descriptor -- forcing the import - that the descriptor exists to defer, purely to decide whether to - warn. + The guard now matches :func:`register_protocol + `'s: it + fires only when the incumbent differs from the replacement, so + re-registering the exact same class object under the same ``code`` + is a silent no-op rather than a warning about nothing displaced. + GitHub issue #718 corrected the previous presence-only guard here, + which read every repeat registration as a caller mistake even when + the value was unchanged. The identity check does not reintroduce + the concern that guard was written to avoid: it is a plain ``is`` + comparison, so an incumbent left as an unresolved + :class:`~pcapkit.corekit.module.ModuleDescriptor` is never equal to + the resolved replacement without the descriptor being resolved -- + the comparison itself resolves nothing, so the deferred import + stays deferred and such an incumbent still reports as different. """ if isinstance(protocol, ModuleDescriptor): protocol = protocol.klass if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - if code in cls.__proto__: + incumbent = cls.__proto__.get(code) + if incumbent is not None and incumbent is not protocol: warn(f'protocol {code} already registered, overwriting ' - f'{cls.__proto__[code]!r} with {protocol!r}', RegistryWarning) + f'{incumbent!r} with {protocol!r}', RegistryWarning) cls.__proto__[code] = protocol @classmethod diff --git a/pcapkit/protocols/schema/misc/pcapng.py b/pcapkit/protocols/schema/misc/pcapng.py index 99de4fea11..4161b3e3b1 100644 --- a/pcapkit/protocols/schema/misc/pcapng.py +++ b/pcapkit/protocols/schema/misc/pcapng.py @@ -719,18 +719,22 @@ def register(code: 'Enum_OptionType', cls: 'Type[Option]', ns: 'Optional[str]' = every other registry in the package does -- the lookup that follows cannot tell a deliberate replacement from an accidental one, so an unreported overwrite is a parser silently swapped out for another. See - `#681 `__ for the same - guard on ``register_protocol``. - - Presence alone is the test here, rather than #681's presence *and a - different class*: that one keys on a name **derived** from the class, so - the wrapper registrars reach it twice with the same class on a supported - path and warning there would be noise. This keys on a caller-supplied - ``code``, and :meth:`__init_subclass__` passes each code exactly once per - subclass, so a second arrival is a second deliberate call -- which is what - the seven sibling :meth:`register` methods on + `#681 `__ for the + guard ``register_protocol`` added first, which this one now matches. + + The guard is identity-based: it fires only when the incumbent differs + from ``cls``, so re-registering the exact same class under the same + ``code`` is a silent no-op -- in every namespace ``targets`` reaches -- + rather than a warning about nothing displaced. That is what keeps + :meth:`__init_subclass__` honest: it loops over a ``code`` list with no + deduplication, so a repeated or aliased entry reaches this method twice + with the same class, and the second call now finds itself already the + incumbent. GitHub issue #718 corrected the previous presence-only + guard, which read every such repeat as a caller mistake whether or not + the value had actually changed -- the same fix the sibling + :meth:`register` methods on :class:`~pcapkit.protocols.protocol.ProtocolBase` and friends already - assume. + received. Note: ``ns='opt'`` fans one registration out across every namespace, so the @@ -742,7 +746,7 @@ def register(code: 'Enum_OptionType', cls: 'Type[Option]', ns: 'Optional[str]' = registering an ``opt``-namespace code into a brand-new namespace is exactly what that copy is for. - Membership is tested with ``in``, never by subscripting. The + Membership is tested with ``.get()``, never by subscripting. The per-namespace registries are :class:`collections.defaultdict`\\ s -- only the outer one is the miss-safe :class:`~pcapkit.protocols.schema.schema._EnumRegistry` -- so reading @@ -760,7 +764,9 @@ def register(code: 'Enum_OptionType', cls: 'Type[Option]', ns: 'Optional[str]' = targets = list(Option.registry) if ns == 'opt' else [ns] if not fresh: - clash = [key for key in targets if code in Option.registry[key]] + clash = [key for key in targets + if (incumbent := Option.registry[key].get(code)) is not None + and incumbent is not cls] if clash: warn(f'PCAP-NG: [Option {code}] option already registered in ' f'namespace(s) {", ".join(repr(key) for key in clash)}, ' diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 183dc9082b..fceca0e6a3 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -1122,8 +1122,16 @@ def __init_subclass__(cls, /, code: 'Optional[_ET | Iterable[_ET]]' = None, *arg # does not mean the same thing for every subclass. codes = code if isinstance(code, collections.abc.Iterable) else (code,) for _code in codes: - if _code in cls.__enum__: - incumbent = cls.__enum__[_code] # type: ignore[index] + # This loop visits every element of an iterable ``code``, so a + # repeated member -- or two members that are the same object, + # as an :class:`enum.Enum` alias makes possible -- reaches this + # twice for the same key with ``cls`` on both sides: incumbent + # on the first pass, replacement on the second. So ordinary + # subclassing syntax reaches this guard, e.g. + # ``class X(Base, code=[A, A])`` or ``code=[A, B]`` with + # ``A is B``, with no second call to this method needed. + incumbent = cls.__enum__.get(_code) # type: ignore[arg-type] + if incumbent is not None and incumbent is not cls: warn(f'schema {_code} already registered, overwriting ' f'{incumbent!r} with {cls!r}', RegistryWarning) cls.__enum__[_code] = (cls) # type: ignore[index] @@ -1153,12 +1161,14 @@ def register(cls, code: '_ET', schema: 'Type[Self]') -> 'None': parser it displaced and said nothing about the schema. The guard here closes that asymmetry. - It fires on the mere presence of ``code``, as the code-keyed parser - registries do and unlike :func:`register_protocol - `, whose key - is derived from the value it stores. ``code`` here is supplied by the - caller and is independent of ``schema``, so a repeat is a caller - mistake worth reporting even when the value is unchanged. + It fires only when the incumbent differs from the replacement, the + same guard :func:`register_protocol + ` applies, + even though ``code`` here -- unlike ``register_protocol``'s key -- + is supplied by the caller and independent of ``schema``. GitHub + issue #718 corrected the previous presence-only guard: a repeat + call that names the exact same schema object is a caller replaying + a registration, not a mistake, so it is now a silent no-op. Presence is a faithful "was this really registered" test only because :class:`_EnumRegistry` returns a miss without recording it. A plain @@ -1175,8 +1185,8 @@ def register(cls, code: '_ET', schema: 'Type[Self]') -> 'None': here, so it is guarded separately. """ - if code in cls.__enum__: - incumbent = cls.__enum__[code] # type: ignore[index] + incumbent = cls.__enum__.get(code) # type: ignore[arg-type] + if incumbent is not None and incumbent is not schema: warn(f'schema {code} already registered, overwriting ' f'{incumbent!r} with {schema!r}', RegistryWarning) diff --git a/pcapkit/protocols/transport/sctp.py b/pcapkit/protocols/transport/sctp.py index d2ec503b7a..ff2a5ec336 100644 --- a/pcapkit/protocols/transport/sctp.py +++ b/pcapkit/protocols/transport/sctp.py @@ -616,8 +616,9 @@ def register(cls, code: 'Enum_PayloadProtocolIdentifier | int', protocol: 'Modul Warns: pcapkit.utilities.warnings.RegistryWarning: If this PPID is already registered, naming the displaced entry and its replacement so a - caller can tell *what* was lost. Fires on presence alone, as the - port-keyed :meth:`Transport.register + caller can tell *what* was lost. Fires only when the incumbent + differs from the replacement, as the port-keyed + :meth:`Transport.register ` it overrides does. @@ -626,9 +627,10 @@ def register(cls, code: 'Enum_PayloadProtocolIdentifier | int', protocol: 'Modul protocol = protocol.klass if not issubclass(protocol, ProtocolBase): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - if code in cls.__proto__: + incumbent = cls.__proto__.get(code) + if incumbent is not None and incumbent is not protocol: warn(f'payload protocol identifier {code} already registered, overwriting ' - f'{cls.__proto__[code]!r} with {protocol!r}', RegistryWarning) + f'{incumbent!r} with {protocol!r}', RegistryWarning) cls.__proto__[code] = protocol @classmethod diff --git a/pcapkit/protocols/transport/transport.py b/pcapkit/protocols/transport/transport.py index 5f5009906e..b22da886ee 100644 --- a/pcapkit/protocols/transport/transport.py +++ b/pcapkit/protocols/transport/transport.py @@ -94,10 +94,11 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor[Protocol] | Type[Prot Warns: pcapkit.utilities.warnings.RegistryWarning: If this port is already registered, naming the displaced entry and its replacement so a - caller can tell *what* was lost. Fires on presence alone -- see + caller can tell *what* was lost. Fires only when the + incumbent differs from the replacement -- see :meth:`ProtocolBase.register - ` for why that - differs from ``register_protocol``. + ` for the + guard this shares with ``register_protocol``. Note: ``cls.__proto__`` belongs to the concrete protocol, not to @@ -113,9 +114,10 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor[Protocol] | Type[Prot protocol = protocol.klass if not issubclass(protocol, Protocol): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - if code in cls.__proto__: + incumbent = cls.__proto__.get(code) + if incumbent is not None and incumbent is not protocol: warn(f'port {code} already registered, overwriting ' - f'{cls.__proto__[code]!r} with {protocol!r}', RegistryWarning) + f'{incumbent!r} with {protocol!r}', RegistryWarning) cls.__proto__[code] = protocol @classmethod diff --git a/tests/foundation/registry/test_protocols.py b/tests/foundation/registry/test_protocols.py index abda0c2664..33e333c4da 100644 --- a/tests/foundation/registry/test_protocols.py +++ b/tests/foundation/registry/test_protocols.py @@ -172,11 +172,10 @@ class HTTP(Protocol): def test_register_protocol_stays_quiet_when_nothing_is_displaced(self) -> None: """A fresh key and an identical re-registration are both silent. - This pins the one place this registrar deliberately departs from its - siblings, which warn on mere key presence. A sibling is keyed on a - ``code`` the caller passes, so a repeat call there is a caller mistake - worth reporting even when the value is identical. This registry's key is - *derived* from the class, and this function is the funnel every wrapper + Since #718, the code-keyed siblings share this identity guard too -- + see ``test_sibling_registries_now_stay_quiet_on_an_identical_re_registration`` + in this file. Here the key is *derived* from the class rather than + supplied by the caller, and this function is the funnel every wrapper registrar ends in, so registering one class under two codes -- supported, and exactly what :func:`~pcapkit.foundation.registry.protocols.register_tcp` followed by :func:`~pcapkit.foundation.registry.protocols.register_udp` @@ -227,16 +226,20 @@ def test_register_protocol_stays_quiet_when_nothing_is_displaced(self) -> None: self.assertIs(udp_registry[udp_port], UnitProtocol) self.assertEqual(self._registry_warnings(caught, RegistryWarning), []) - def test_sibling_registries_still_warn_on_an_identical_re_registration(self) -> None: - """The presence-only guard elsewhere is left exactly as it was. - - :func:`~pcapkit.foundation.registry.protocols.register_protocol` now - warns only when a *different* class is displaced. That relaxation must - not leak into the code-keyed registries, which report a repeat - registration even when the incumbent and the replacement are the same - object. Asserted with the same class twice, which is precisely the case - the two guards disagree about -- so this fails if anyone ever - "harmonises" the siblings onto the guard used above. + def test_sibling_registries_now_stay_quiet_on_an_identical_re_registration(self) -> None: + """GitHub issue #718: the siblings now share ``register_protocol``'s guard. + + This test used to pin the opposite: the code-keyed registries warned + on presence alone, even when the incumbent and the replacement were + the same object, and #695's commit message argued that was + deliberate -- the key is caller-supplied rather than derived from the + value, so "present and different" supposedly had nothing to fix there. + #718 found that reasoning did not hold (a caller replaying a + registration is not a mistake merely because the key happens to be + explicit), added the identity guard, and this test now pins *that*: a + repeat registration naming the exact same class stays silent, exactly + as :func:`~pcapkit.foundation.registry.protocols.register_protocol` + already behaved. """ from pcapkit.const.reg.ethertype import EtherType @@ -258,25 +261,26 @@ def test_sibling_registries_still_warn_on_an_identical_re_registration(self) -> self._guard_registry(sibling, code) # Seed the incumbent, then register the very same class again. - # Nothing changes value, so only a presence-only guard warns. + # Nothing changes value, so the identity guard must stay quiet. sibling[code] = Raw with warnings.catch_warnings(record=True) as caught: warnings.simplefilter('always') owner.register(code, Raw) self.assertIs(sibling[code], Raw) - self.assertEqual( - len(self._registry_warnings(caught, RegistryWarning)), 1) + self.assertEqual(self._registry_warnings(caught, RegistryWarning), []) def test_sibling_registries_name_what_they_displaced(self) -> None: """Every code-keyed registrar says *what* it overwrote, not just that it did. - The presence-only condition is deliberately unchanged -- see the test - above -- but the message was ``'protocol {code} already registered, - overwriting'`` and stopped there, so a caller learned that something had - been displaced and never which class it was. That is the same diagnostic - gap #675 closed for :func:`~pcapkit.foundation.registry.protocols.\ - register_protocol`, and it is the part of #681 that does generalise. + Independent of the identity guard the test above now pins (#718): a + genuine overwrite -- two different classes -- still warns and still + names both. The message used to be ``'protocol {code} already + registered, overwriting'`` and stop there, so a caller learned that + something had been displaced and never which class it was. That is the + same diagnostic gap #675 closed for + :func:`~pcapkit.foundation.registry.protocols.register_protocol`, and + it is the part of #681 that does generalise. All seven code-keyed registrars are covered, including the two that reach :meth:`Transport.register diff --git a/tests/protocols/internet/test_internet_unit.py b/tests/protocols/internet/test_internet_unit.py index 9ee9eb280d..c51e2fe022 100644 --- a/tests/protocols/internet/test_internet_unit.py +++ b/tests/protocols/internet/test_internet_unit.py @@ -69,10 +69,40 @@ def test_layer_register_and_protocol_byte_reader(self) -> None: self.assertIs(DummyInternet.__dict__['__proto__'][TransType.get(251)], Raw) with self.assertRaises(RegistryError): DummyInternet.register(TransType.get(252), object) # type: ignore[arg-type] + + class Other(Raw): + pass + + # GitHub issue #718 added an identity guard, so re-registering the + # exact same class at the same code -- what this used to do -- is now + # a silent no-op; see + # ``test_register_stays_quiet_on_same_object_reregistration`` for that + # case. A genuinely different class is needed to still warn here. with mock.patch('pcapkit.protocols.internet.internet.warn') as warn: - DummyInternet.register(TransType.get(250), Raw) + DummyInternet.register(TransType.get(250), Other) warn.assert_called_once() + def test_register_stays_quiet_on_same_object_reregistration(self) -> None: + """GitHub issue #718: replaying a call with the identical class is a no-op. + + Before the fix the guard fired on presence alone, so the *second* of + two calls registering the exact same class under the exact same code + warned about an overwrite that never happened. Matches the identity + guard :func:`register_protocol + ` already + applies. + + """ + from pcapkit.const.reg.transtype import TransType + from pcapkit.protocols.misc.raw import Raw + + DummyInternet = self._make_internet_class() + + with mock.patch('pcapkit.protocols.internet.internet.warn') as warn: + DummyInternet.register(TransType.get(250), Raw) + DummyInternet.register(TransType.get(250), Raw) + warn.assert_not_called() + def test_decode_next_layer_updates_info_and_protochain(self) -> None: from pcapkit.corekit.protochain import ProtoChain diff --git a/tests/protocols/link/test_link_unit.py b/tests/protocols/link/test_link_unit.py index f70e41d312..59cac6587c 100644 --- a/tests/protocols/link/test_link_unit.py +++ b/tests/protocols/link/test_link_unit.py @@ -496,8 +496,18 @@ def test_link_base_layer_registry_and_protocol_reader(self) -> None: try: Link.register(custom_code, ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw')) self.assertIs(registry[custom_code], Raw) + + class Other(Raw): + pass + + # ``register`` resolves a ``ModuleDescriptor`` before storing it, + # so the entry above is already the plain ``Raw`` class, not the + # descriptor -- passing ``Raw`` again would now be the same-object + # no-op GitHub issue #718 added an identity guard for (see + # ``test_link_register_stays_quiet_on_same_object_reregistration``). + # A genuinely different class is needed to still warn here. with mock.patch('pcapkit.protocols.link.link.warn') as warn: - Link.register(custom_code, Raw) + Link.register(custom_code, Other) warn.assert_called_once() with self.assertRaises(RegistryError): Link.register(custom_code, object) @@ -507,6 +517,35 @@ def test_link_base_layer_registry_and_protocol_reader(self) -> None: else: registry[custom_code] = original + def test_link_register_stays_quiet_on_same_object_reregistration(self) -> None: + """GitHub issue #718: replaying a call with the identical class is a no-op. + + Before the fix the guard fired on presence alone, so the *second* of + two calls registering the exact same class under the exact same code + warned about an overwrite that never happened. Matches the identity + guard :func:`register_protocol + ` already + applies. + + """ + from pcapkit.const.reg.ethertype import EtherType + from pcapkit.protocols.link.link import Link + from pcapkit.protocols.misc.raw import Raw + + registry = Link.__dict__['__proto__'] + custom_code = EtherType.get(0x88B6) + original = registry.get(custom_code) + try: + with mock.patch('pcapkit.protocols.link.link.warn') as warn: + Link.register(custom_code, Raw) + Link.register(custom_code, Raw) + warn.assert_not_called() + finally: + if original is None: + registry.pop(custom_code, None) + else: + registry[custom_code] = original + def test_ethernet_properties_read_make_and_mac_helpers(self) -> None: from pcapkit.const.reg.ethertype import EtherType from pcapkit.protocols.link.ethernet import Ethernet diff --git a/tests/protocols/misc/pcap/test_header_frame_unit.py b/tests/protocols/misc/pcap/test_header_frame_unit.py index 4f370b5573..68046eaa9f 100644 --- a/tests/protocols/misc/pcap/test_header_frame_unit.py +++ b/tests/protocols/misc/pcap/test_header_frame_unit.py @@ -207,8 +207,18 @@ def test_frame_make_index_length_and_make_data(self) -> None: try: Frame.register(code, ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw')) self.assertIs(registry[code], Raw) + + class Other(Raw): + pass + + # ``register`` resolves a ``ModuleDescriptor`` before storing it, + # so the entry above is already the plain ``Raw`` class, not the + # descriptor -- passing ``Raw`` again would now be the same-object + # no-op GitHub issue #718 added an identity guard for (see + # ``test_frame_register_stays_quiet_on_same_object_reregistration``). + # A genuinely different class is needed to still warn here. with mock.patch('pcapkit.protocols.misc.pcap.frame.warn') as warn: - Frame.register(code, Raw) + Frame.register(code, Other) warn.assert_called_once() with self.assertRaises(RegistryError): Frame.register(LinkType.USER1, object) # type: ignore[arg-type] @@ -316,6 +326,36 @@ def fromtimestamp(cls, *args): self.assertIs(frame._decode_next_layer(decoded_no_chain, LinkType.NULL, 0), decoded_no_chain) self.assertEqual(decoded_no_chain['protocols'], '') + def test_frame_register_stays_quiet_on_same_object_reregistration(self) -> None: + """GitHub issue #718: replaying a call with the identical class is a no-op. + + Before the fix the guard fired on presence alone, so the *second* of + two calls registering the exact same class under the exact same code + warned about an overwrite that never happened. Matches the identity + guard :func:`register_protocol + ` already + applies. + + """ + from pcapkit.const.reg.linktype import LinkType + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.raw import Raw + + registry = Frame.__dict__['__proto__'] + code = LinkType.USER2 + had_original = code in registry + original = registry.get(code) + try: + with mock.patch('pcapkit.protocols.misc.pcap.frame.warn') as warn: + Frame.register(code, Raw) + Frame.register(code, Raw) + warn.assert_not_called() + finally: + if had_original: + registry[code] = original + else: + registry.pop(code, None) + def test_frame_data_holds_its_own_record_not_the_next(self) -> None: """A frame's raw data must be the record it parsed, at its own offset. diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index 6de7128fed..39ee7eeaf5 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -509,8 +509,18 @@ def make_block(block=None, *, body=b'custom'): try: PCAPNG.register(proto_code, ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw')) self.assertIs(proto_map[proto_code], Raw) + + class Other(Raw): + pass + + # ``register`` resolves a ``ModuleDescriptor`` before storing it, + # so the entry above is already the plain ``Raw`` class, not the + # descriptor -- passing ``Raw`` again would now be the same-object + # no-op GitHub issue #718 added an identity guard for (see + # ``test_pcapng_register_stays_quiet_on_same_object_reregistration``). + # A genuinely different class is needed to still warn here. with mock.patch('pcapkit.protocols.misc.pcapng.warn') as warn: - PCAPNG.register(proto_code, Raw) + PCAPNG.register(proto_code, Other) warn.assert_called_once() with self.assertRaises(RegistryError): PCAPNG.register(LinkType.USER1, object) # type: ignore[arg-type] @@ -635,6 +645,36 @@ def make_block(block=None, *, body=b'custom'): self.assertIs(pcapng._decode_next_layer(decoded_no_chain, LinkType.NULL, 0), decoded_no_chain) self.assertEqual(decoded_no_chain['protocols'], '') + def test_pcapng_register_stays_quiet_on_same_object_reregistration(self) -> None: + """GitHub issue #718: replaying a call with the identical class is a no-op. + + Before the fix the guard fired on presence alone, so the *second* of + two calls registering the exact same class under the exact same code + warned about an overwrite that never happened. Matches the identity + guard :func:`register_protocol + ` already + applies. + + """ + from pcapkit.const.reg.linktype import LinkType + from pcapkit.protocols.misc.pcapng import PCAPNG + from pcapkit.protocols.misc.raw import Raw + + proto_map = PCAPNG.__dict__['__proto__'] + code = LinkType.USER2 + had_original = code in proto_map + original = proto_map.get(code) + try: + with mock.patch('pcapkit.protocols.misc.pcapng.warn') as warn: + PCAPNG.register(code, Raw) + PCAPNG.register(code, Raw) + warn.assert_not_called() + finally: + if had_original: + proto_map[code] = original + else: + proto_map.pop(code, None) + def test_pcapng_interface_option_constructors_and_scope_guards(self) -> None: from pcapkit.const.pcapng.block_type import BlockType from pcapkit.const.pcapng.option_type import OptionType @@ -4385,15 +4425,13 @@ def test_a_first_registration_is_silent(self) -> None: self.assertIs(option.registry['if'][unregistered], UnknownOption) def test_re_registering_an_option_code_reports_the_collision(self) -> None: - """Presence alone is the test, as it is for the seven sibling registrars. + """A genuine displacement -- a different class under the same code -- still warns. - #681 tested presence *and a different class* because it keys on a name - derived from the class, which the wrapper registrars reach twice with the - same class. This keys on a caller-supplied code that - ``__init_subclass__`` passes exactly once per subclass, so a second - arrival is a second deliberate call -- and reporting it is what the - ``ProtocolBase``, ``Frame``, ``Internet``, ``PCAPNG``, ``SCTP``, - ``Transport`` and ``Link`` registrars already do. + The guard is identity-based, matching every sibling registrar: presence + alone is not the test, only whether the incumbent differs from the + replacement. ``if_name`` already names ``IF_NameOption``, so replacing + it with ``UnknownOption`` is a real displacement, and it is reported + exactly as it was before the guard changed. """ from pcapkit.const.pcapng.option_type import OptionType @@ -4470,7 +4508,7 @@ def test_a_namespace_created_by_the_registration_is_not_a_collision(self) -> Non UnknownOption) def test_the_collision_check_does_not_insert_a_default(self) -> None: - """Membership with ``in``, never by subscripting. + """Membership is tested with ``.get()``, never by subscripting. Only the outer registry is the miss-safe ``_EnumRegistry``; the per-namespace ones are plain :class:`collections.defaultdict`\\ s, so @@ -4492,6 +4530,64 @@ def test_the_collision_check_does_not_insert_a_default(self) -> None: after = {key: len(value) for key, value in option.registry.items()} self.assertEqual(after, before) + def test_repeated_code_list_registering_the_same_class_is_silent(self) -> None: + """GitHub issue #718's shape: ``code=[b, b]`` must not double-count. + + ``__init_subclass__`` loops over ``code`` with no deduplication, so a + repeated entry reaches :meth:`Option.register` twice with the *same* + class -- the second call finds itself already the incumbent. Before + the identity guard this still hit the presence-only check and warned + about an overwrite that never happened, since the class was + overwriting itself. + + """ + from pcapkit.const.pcapng.option_type import OptionType + from pcapkit.protocols.schema.misc.pcapng import _IF_Option + from pcapkit.utilities.warnings import RegistryWarning + + option = self._snapshot() + fresh_code = OptionType.get(0x00FB, namespace='if') + self.assertNotIn(fresh_code, option.registry['if']) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + + class _RepeatedCodeOption(_IF_Option, code=[fresh_code, fresh_code]): + pass + + registry_warnings = [entry for entry in caught + if entry.category is RegistryWarning] + self.assertEqual(registry_warnings, []) + self.assertIs(option.registry['if'][fresh_code], _RepeatedCodeOption) + + def test_repeated_code_registration_still_warns_once_for_a_genuine_displacement(self) -> None: + """A real displacement inside a repeated-code call still warns -- exactly once. + + Simulates what ``__init_subclass__`` does for ``code=[b, b]`` when ``b`` + already names a different incumbent: two calls to :meth:`Option.register` + with the same new class. The first call genuinely displaces + ``IF_NameOption`` and must warn; the second finds itself already the + incumbent and must not warn again. The presence-only guard warned on + both, reporting the same displacement twice. + + """ + from pcapkit.const.pcapng.option_type import OptionType + from pcapkit.protocols.schema.misc.pcapng import IF_NameOption, UnknownOption + from pcapkit.utilities.warnings import RegistryWarning + + option = self._snapshot() + self.assertIs(option.registry['if'][OptionType.if_name], IF_NameOption) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + option.register(OptionType.if_name, UnknownOption) + option.register(OptionType.if_name, UnknownOption) + + registry_warnings = [entry for entry in caught + if entry.category is RegistryWarning] + self.assertEqual(len(registry_warnings), 1) + self.assertIs(option.registry['if'][OptionType.if_name], UnknownOption) + if __name__ == '__main__': unittest.main() diff --git a/tests/protocols/schema/test_enum_schema_registry_unit.py b/tests/protocols/schema/test_enum_schema_registry_unit.py index f180af1540..70324a5c6b 100644 --- a/tests/protocols/schema/test_enum_schema_registry_unit.py +++ b/tests/protocols/schema/test_enum_schema_registry_unit.py @@ -177,10 +177,11 @@ class EnumSchemaRegistryOverwriteTests(unittest.TestCase): :meth:`register`; guarding only the method would leave it silent. Generalises the guard GitHub issue #675 added to ``register_protocol`` in - #681. The condition here is presence alone, as the code-keyed parser - registries use, rather than #681's "present *and* a different class" -- that - narrower form is licensed by a key *derived* from the value, which this - registry's caller-supplied ``code`` is not. + #681. GitHub issue #718 then brought the condition here in line with + #681's "present *and* a different class": a repeat call that names the + exact same schema object is a silent no-op, even though this registry's + ``code`` is caller-supplied rather than *derived* from the value the way + ``register_protocol``'s key is. """ def setUp(self) -> None: @@ -317,8 +318,107 @@ class Replacement(BaseSchema): self.assertIs(BaseSchema.registry[Code.two], Replacement) self.assertEqual(self._registry_warnings(caught, RegistryWarning), []) + def test_register_stays_quiet_on_same_object_reregistration(self) -> None: + """GitHub issue #718: replaying a call with the identical schema is a no-op. + + Before the fix the guard fired on presence alone, so a caller + idempotently re-asserting a binding it already made -- registering + the exact same schema object under the exact same code a second time + -- warned about an overwrite that never happened, exactly as + unhelpfully as a genuine collision. Matches the identity guard + :func:`register_protocol + ` already + applies. + + """ + from pcapkit.utilities.warnings import RegistryWarning + + Code, BaseSchema = self._base_schema() + + class Incumbent(BaseSchema, code=Code.one): + pass + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + BaseSchema.register(Code.one, Incumbent) + + self.assertIs(BaseSchema.registry[Code.one], Incumbent) + self.assertEqual(self._registry_warnings(caught, RegistryWarning), []) + + def test_declaring_the_same_class_twice_stays_quiet(self) -> None: + """The declaration path's identity guard, called directly a second time. + + This calls :meth:`EnumSchema.__init_subclass__` a second time, + directly, on the class it already ran for, and pins that it stays + silent under GitHub issue #718's fix. It is not the only way to reach + this guard through the declaration path -- see + :meth:`test_a_repeated_code_in_the_declaration_list_stays_quiet` below + for the ordinary-syntax route, a repeated or aliased member in + ``code``'s iterable form. + + """ + from pcapkit.utilities.warnings import RegistryWarning + + Code, BaseSchema = self._base_schema() + + class Twice(BaseSchema, code=Code.one): + pass + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + Twice.__init_subclass__(code=Code.one) + + self.assertIs(BaseSchema.registry[Code.one], Twice) + self.assertEqual(self._registry_warnings(caught, RegistryWarning), []) + + def test_a_repeated_code_in_the_declaration_list_stays_quiet(self) -> None: + """Ordinary subclassing syntax reaches the guard too, via a repeated code. + + :meth:`EnumSchema.__init_subclass__` loops over every element of an + iterable ``code``, so a member repeated in the list -- or two distinct + members that are the same object, as an :class:`enum.Enum` alias makes + possible -- reaches the same key twice with ``cls`` on both sides, + with no second call to ``__init_subclass__`` needed: this is a live + path through normal syntax, not merely the direct-call case pinned + above. Not a contrived shape either: GitHub issue #721 shipped + ``R1CounterParameter(Parameter, code=[R1_Counter, R1_COUNTER])``, and + several :mod:`pcapkit.const` enums (e.g. ``reg.linktype.LinkType``, + ``esp.cipher.Cipher``) declare real aliases that would make an + ``A is B`` pair like the alias case below invisible at the call site. + + """ + from pcapkit.protocols.schema.schema import EnumSchema + from pcapkit.utilities.warnings import RegistryWarning + + Code, BaseSchema = self._base_schema() + + class AliasCode(enum.IntEnum): + one = 1 + uno = 1 # alias of `one` + + self.assertIs(AliasCode.uno, AliasCode.one) + + class AliasBaseSchema(EnumSchema[AliasCode]): + __default__ = lambda: object # noqa: E731 + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + + class RepeatedMember(BaseSchema, code=[Code.one, Code.one]): + pass + + class AliasPair(AliasBaseSchema, code=[AliasCode.one, AliasCode.uno]): + pass + + # Both declarations reach the guard on the second element of their + # ``code`` list, with the class under construction as both incumbent + # and replacement -- so neither warns. + self.assertEqual(self._registry_warnings(caught, RegistryWarning), []) + self.assertIs(BaseSchema.registry[Code.one], RepeatedMember) + self.assertIs(AliasBaseSchema.registry[AliasCode.one], AliasPair) + def test_a_lookup_miss_still_does_not_make_a_later_register_warn(self) -> None: - """The #555 retention fix is what makes a presence-only guard safe here. + """The #555 retention fix is what makes the identity guard safe here. On a plain :class:`collections.defaultdict` a bare ``registry[code]`` for an unregistered code inserted the default, so parsing one packet carrying diff --git a/tests/protocols/test_protocol_base_unit.py b/tests/protocols/test_protocol_base_unit.py index ccbfe13527..bc91c046ee 100644 --- a/tests/protocols/test_protocol_base_unit.py +++ b/tests/protocols/test_protocol_base_unit.py @@ -243,9 +243,16 @@ def test_register_analyze_and_next_layer_paths(self) -> None: DummyProtocol.register(9, ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw')) self.assertIs(DummyProtocol.__proto__[9], Raw) + class Other(Raw): + pass + + # GitHub issue #718 added an identity guard, so the second call needs + # a genuinely different class from the first to still warn -- see + # ``test_register_stays_quiet_on_same_object_reregistration`` for the + # same-object case this used to (incorrectly) exercise. with mock.patch('pcapkit.protocols.protocol.warn') as warn: DummyProtocol.register(1, Raw) - DummyProtocol.register(1, Raw) + DummyProtocol.register(1, Other) self.assertGreaterEqual(warn.call_count, 1) DummyProtocol.__proto__ = collections.defaultdict( @@ -344,6 +351,26 @@ def __index__(cls) -> int: no_stop._exproto = 'raw' self.assertFalse(no_stop._check_term_threshold()) + def test_register_stays_quiet_on_same_object_reregistration(self) -> None: + """GitHub issue #718: replaying a call with the identical class is a no-op. + + Before the fix the guard fired on presence alone, so the *second* of + two calls registering the exact same class under the exact same code + warned about an overwrite that never happened -- this is the case + ``test_register_analyze_and_next_layer_paths`` used to (incorrectly) + exercise. Matches the identity guard :func:`register_protocol + ` already + applies. + + """ + DummyProtocol, _, _ = self._make_protocol_class() + from pcapkit.protocols.misc.raw import Raw + + with mock.patch('pcapkit.protocols.protocol.warn') as warn: + DummyProtocol.register(1, Raw) + DummyProtocol.register(1, Raw) + self.assertEqual(warn.call_count, 0) + def test_lookup_registry_reads_the_fallback_without_recording_it(self) -> None: """The non-recording lookup is generic over what the registry holds. diff --git a/tests/protocols/transport/test_sctp_unit.py b/tests/protocols/transport/test_sctp_unit.py index 4fdf5e1947..ef7b7bfc43 100644 --- a/tests/protocols/transport/test_sctp_unit.py +++ b/tests/protocols/transport/test_sctp_unit.py @@ -1053,17 +1053,44 @@ def test_register_warns_on_overwrite(self) -> None: from pcapkit.protocols.transport import sctp as sctp_module from pcapkit.protocols.transport.sctp import SCTP + class Other(Raw): + pass + # 4243 rather than 60: NGAP holds 60 by default, so registering it once # would already be the overwrite this test means to trigger on the # *second* call, and the assertion on the call count would pass for the - # wrong reason. + # wrong reason. The second call also has to use a genuinely different + # class from the first: GitHub issue #718 added an identity guard here + # (see ``test_register_stays_quiet_on_same_object_reregistration``), so + # replaying the first call's exact object would no longer warn. with self._proto_registry(): SCTP.register(4243, Raw) with mock.patch.object(sctp_module, 'warn') as warned: - SCTP.register(4243, Raw) + SCTP.register(4243, Other) self.assertEqual(warned.call_count, 1) self.assertIn('payload protocol identifier', warned.call_args.args[0]) + def test_register_stays_quiet_on_same_object_reregistration(self) -> None: + """GitHub issue #718: replaying a call with the identical class is a no-op. + + Before the fix the guard fired on presence alone, so the *second* of + two calls that register the exact same class under the exact same + code -- what this test performs -- warned about an overwrite that + never happened. Mirrors the identity guard :func:`register_protocol + ` already + applies. + + """ + from pcapkit.protocols.misc.raw import Raw + from pcapkit.protocols.transport import sctp as sctp_module + from pcapkit.protocols.transport.sctp import SCTP + + with self._proto_registry(): + SCTP.register(4243, Raw) + with mock.patch.object(sctp_module, 'warn') as warned: + SCTP.register(4243, Raw) + self.assertEqual(warned.call_count, 0) + def test_register_sctp_wrapper_writes_the_ppid_registry(self) -> None: from pcapkit.foundation.registry.protocols import register_sctp from pcapkit.protocols.misc.raw import Raw diff --git a/tests/protocols/transport/test_transport_unit.py b/tests/protocols/transport/test_transport_unit.py index e528efa74a..b5e9b8e0e5 100644 --- a/tests/protocols/transport/test_transport_unit.py +++ b/tests/protocols/transport/test_transport_unit.py @@ -61,10 +61,59 @@ def __index__(cls) -> int: DummyTransport.register(82, ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw')) self.assertIs(DummyTransport.__dict__['__proto__'][82], Raw) + + class Other(Raw): + pass + + # GitHub issue #718 added an identity guard, so registering the same + # ``Raw`` object port 80 already holds -- what this used to do -- + # would now be a silent no-op; see + # ``test_register_stays_quiet_on_same_object_reregistration`` for + # that case. A genuinely different class is needed to still warn. with mock.patch('pcapkit.protocols.transport.transport.warn') as warn: - DummyTransport.register(80, Raw) + DummyTransport.register(80, Other) warn.assert_called_once() + def test_register_stays_quiet_on_same_object_reregistration(self) -> None: + """GitHub issue #718: replaying a call with the identical class is a no-op. + + Before the fix the guard fired on presence alone, so registering port + 80's ``Raw`` incumbent with the exact same ``Raw`` object -- what + ``test_register_validates_protocols_and_abstract_base`` used to do -- + warned about an overwrite that never happened. Matches the identity + guard :func:`register_protocol + ` already + applies. + + """ + from pcapkit.protocols.misc.raw import Raw + from pcapkit.protocols.transport.transport import Transport + + class DummyTransport(Transport): + __proto__ = collections.defaultdict(lambda: Raw, {80: Raw}) + + @property + def name(self) -> str: + return 'Dummy Transport' + + @property + def length(self) -> int: + return 0 + + def read(self, length: int | None = None, **kwargs: object) -> object: + raise NotImplementedError + + def make(self, **kwargs: object) -> object: + raise NotImplementedError + + @classmethod + def __index__(cls) -> int: + return 250 + + with mock.patch('pcapkit.protocols.transport.transport.warn') as warn: + DummyTransport.register(80, Raw) + warn.assert_not_called() + def test_analyze_prefers_source_port_match(self) -> None: from pcapkit.protocols.transport.transport import Transport