From fdad57e412a7888c7270c65496cf6d6b001f7e0b Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 18:25:09 -0400 Subject: [PATCH] fix(registry): report the protocol-name collision register_protocol hid `register_protocol` keyed `pcapkit.protocols.__proto__` on `cls.__name__.upper()` with a bare assignment, and three dispatchable protocol classes are all named `HTTP`, so registering one silently displaced whichever was there. - Warn with `RegistryWarning` when the key is held by a *different* class, naming both the displaced and the replacing class -- the module is the only thing that distinguishes the three `HTTP` classes, so a message that omitted it would not say which one was lost. - Re-registering the same class stays silent. Every wrapper registrar funnels into this function, so one class under two codes reaches it twice with nothing displaced; warning there would be noise on a supported path, and the filter it invites is what would hide the real collision. - Document the collision, why this guard differs from the presence-only siblings, and why re-keying the registry belongs to #514. Tests register two different classes named `HTTP` and assert the warning in both directions, cover the `Protocol.__init_subclass__` path that reaches the collision with no registry call in user code, pin the quiet cases, and assert the code-keyed sibling registries still warn on an identical re-registration. Coverage on the touched module holds at 88% with misses flat at 27. Fixes #675 --- pcapkit/foundation/registry/protocols.py | 67 +++++- tests/foundation/registry/test_protocols.py | 222 ++++++++++++++++++++ 2 files changed, 288 insertions(+), 1 deletion(-) diff --git a/pcapkit/foundation/registry/protocols.py b/pcapkit/foundation/registry/protocols.py index 88ae8ad45e..f1f0a771ee 100644 --- a/pcapkit/foundation/registry/protocols.py +++ b/pcapkit/foundation/registry/protocols.py @@ -56,6 +56,7 @@ from pcapkit.protocols.transport.udp import UDP from pcapkit.utilities.exceptions import RegistryError from pcapkit.utilities.logging import get_logger +from pcapkit.utilities.warnings import RegistryWarning, warn if TYPE_CHECKING: from typing import Any, Iterator, Optional, Type @@ -150,14 +151,78 @@ def register_protocol(protocol: 'Type[Protocol]') -> 'None': :class:`~pcapkit.protocols.protocol.Protocol`, and will be registered to the :data:`pcapkit.protocols.__proto__` registry. + The registry is keyed on ``protocol.__name__.upper()``, which is **not** + unique across this package: three dispatchable protocol classes are all + named ``HTTP`` -- the generic base + :class:`pcapkit.protocols.application.http.HTTP` and the two version + implementations :class:`pcapkit.protocols.application.httpv1.HTTP` and + :class:`pcapkit.protocols.application.httpv2.HTTP` -- so all three compete + for the single key ``'HTTP'``. Registering one of them replaces whichever + was there, and the replacement is observable through every reader of the + registry, e.g. :meth:`ProtocolBase.expand_comp + `, which resolves a + bare protocol name through it. + + Per #675 the overwrite is now reported rather than silent, matching + :meth:`ProtocolBase.register + ` 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. + + 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 + readers outside this module cannot absorb on their own -- the registry is a + documented public attribute, and its readers look a bare name up and then + degrade *silently* on a miss rather than raising, so a re-keying would not + announce itself either. :meth:`ProtocolBase.expand_comp + ` falls back to the + name as a plain string, and the ``protocol`` properties on + :class:`~pcapkit.foundation.reassembly.reassembly.ReassemblyMeta` and + :class:`~pcapkit.foundation.traceflow.traceflow.TraceFlowMeta` fall back to + :class:`~pcapkit.protocols.misc.raw.Raw`. Re-keying is therefore part of the + registry redesign in #514, and reporting the collision here is the step that + redesign is sequenced behind. + Args: protocol: Protocol class. + Raises: + pcapkit.utilities.exceptions.RegistryError: If ``protocol`` is not a + :class:`~pcapkit.protocols.protocol.ProtocolBase` subclass. + + Warns: + pcapkit.utilities.warnings.RegistryWarning: If the registry already + holds a *different* class under this protocol's name, i.e. the + registration displaced another protocol class. Re-registering the + same class under the same name is silent. + """ if not issubclass(protocol, Protocol): raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}') - protocol_registry[protocol.__name__.upper()] = protocol + name = protocol.__name__.upper() + incumbent = protocol_registry.get(name) + if incumbent is not None and incumbent is not protocol: + warn(f'protocol {name} already registered, overwriting {incumbent!r} ' + f'with {protocol!r}', RegistryWarning) + + protocol_registry[name] = protocol logger.debug('registered protocol: %s', protocol.__name__) diff --git a/tests/foundation/registry/test_protocols.py b/tests/foundation/registry/test_protocols.py index 3b8844525d..d03c20a416 100644 --- a/tests/foundation/registry/test_protocols.py +++ b/tests/foundation/registry/test_protocols.py @@ -3,6 +3,7 @@ import importlib.util import inspect import unittest +import warnings from unittest import mock from tests._support import purge_modules @@ -46,6 +47,227 @@ class UnitProtocol(ProtocolBase): return UnitProtocol + def _guard_registry(self, registry, key) -> None: + """Restore ``key`` in ``registry`` on teardown, absence included. + + Every registry in this package is process-global and order-dependent, so + a test that writes into one has to put back exactly what it found. A + plain read-and-restore is not enough: a key that was *absent* has no + value to restore, and writing one back leaves a stray entry that the + next test in the process inherits -- the class of defect #674 is open + for. Hence the sentinel and the ``pop``. + + """ + missing = object() + previous = registry.get(key, missing) + + def restore() -> None: + if previous is missing: + registry.pop(key, None) + else: + registry[key] = previous + + self.addCleanup(restore) + + @staticmethod + def _registry_warnings(caught, category) -> 'list[str]': + """The messages of the captured warnings that are of ``category``. + + Filtering by category matters: the parse and import paths raise other + warning types, and a bare count of everything captured would make an + assertion about "no warning" pass or fail for unrelated reasons. + + """ + return [str(item.message) for item in caught + if issubclass(item.category, category)] + + def test_register_protocol_warns_when_a_colliding_name_overwrites(self) -> None: + """Three dispatchable classes are all named ``HTTP``, so they share a key. + + The registry is keyed on ``protocol.__name__.upper()``, so + :class:`pcapkit.protocols.application.http.HTTP`, + :class:`pcapkit.protocols.application.httpv1.HTTP` and + :class:`pcapkit.protocols.application.httpv2.HTTP` all land on ``'HTTP'`` + and registering one replaces whichever was there. Before #675 the + replacement was silent -- no exception, no warning -- so a later bare-name + lookup simply returned a different class than it had before, with nothing + connecting the symptom to the registration that caused it. + + """ + from pcapkit.foundation.registry import protocols as registry + from pcapkit.protocols.application import http, httpv1, httpv2 + from pcapkit.utilities.warnings import RegistryWarning + + self._guard_registry(registry.protocol_registry, 'HTTP') + + # The premise the defect rests on: three *different* classes, one key. + colliding = (http.HTTP, httpv1.HTTP, httpv2.HTTP) + self.assertEqual(len(set(colliding)), 3) + self.assertEqual({cls.__name__.upper() for cls in colliding}, {'HTTP'}) + + registry.protocol_registry['HTTP'] = http.HTTP + + # Both directions, so the test is not pinned to one registration order. + for incumbent, replacement in ((http.HTTP, httpv2.HTTP), + (httpv2.HTTP, httpv1.HTTP)): + with self.subTest(replacing=incumbent.__module__): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + registry.register_protocol(replacement) + + # The overwrite still happens. This fix reports the collision, + # it does not resolve it -- resolving it is #514's business. + self.assertIs(registry.protocol_registry['HTTP'], replacement) + + messages = self._registry_warnings(caught, RegistryWarning) + self.assertEqual(len(messages), 1) + + # Naming both classes is the point of the message: 'HTTP already + # registered' on its own does not say *which* HTTP was lost, and + # the module is the only thing that distinguishes these three. + # ``repr`` rather than ``__module__`` because + # 'pcapkit.protocols.application.http' is a prefix of + # '...application.httpv1', so a substring test on the module name + # alone would pass on the wrong class. + self.assertIn(repr(incumbent), messages[0]) + self.assertIn(repr(replacement), messages[0]) + self.assertLess(messages[0].index(repr(incumbent)), + messages[0].index(repr(replacement))) + + def test_a_protocol_subclass_shadowing_a_builtin_name_warns(self) -> None: + """The collision is reachable without calling the registrar at all. + + :meth:`Protocol.__init_subclass__ + ` calls + :func:`~pcapkit.foundation.registry.protocols.register_protocol` + unconditionally, so merely *defining* a subclass of the public + :class:`~pcapkit.protocols.protocol.Protocol` under a name a built-in + already holds displaces the built-in's entry. That is the sharpest form + of #675: there is no call to any registry function anywhere in the + user's code, so before the fix nothing at all marked the moment the + dispatch table changed meaning. + + """ + from pcapkit.foundation.registry import protocols as registry + from pcapkit.protocols.protocol import Protocol + from pcapkit.utilities.warnings import RegistryWarning + + incumbent = registry.protocol_registry['HTTP'] + self._guard_registry(registry.protocol_registry, 'HTTP') + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + + class HTTP(Protocol): + """A user protocol that happens to be named like the built-in.""" + + self.assertIs(registry.protocol_registry['HTTP'], HTTP) + self.assertIsNot(registry.protocol_registry['HTTP'], incumbent) + + messages = self._registry_warnings(caught, RegistryWarning) + self.assertEqual(len(messages), 1) + self.assertIn(repr(incumbent), messages[0]) + self.assertIn(repr(HTTP), messages[0]) + + 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 + 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` + does -- reaches it twice with the same class and nothing displaced. A + presence-only guard would warn there, and a caller who filters + :exc:`~pcapkit.utilities.warnings.RegistryWarning` to silence that noise + stops seeing the ``HTTP`` collision the warning exists for. + + """ + from pcapkit.foundation.registry import protocols as registry + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.protocols.transport.udp import UDP + from pcapkit.utilities.warnings import RegistryWarning + + UnitProtocol = self._unit_protocol() + self._guard_registry(registry.protocol_registry, 'UNITPROTOCOL') + + for label in ('fresh-key', 'identical-repeat'): + with self.subTest(case=label): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + registry.register_protocol(UnitProtocol) + + self.assertIs(registry.protocol_registry['UNITPROTOCOL'], UnitProtocol) + self.assertEqual(self._registry_warnings(caught, RegistryWarning), []) + + # ... and the real call shape, through two wrapper registrars. Each ends + # in ``register_protocol(module)``, so this reaches the guard twice with + # the same class. + tcp_registry = TCP.__dict__['__proto__'] + udp_registry = UDP.__dict__['__proto__'] + tcp_port, udp_port = 65500, 65501 + + # Asserted rather than assumed: an already-occupied port would make the + # *sibling* registry warn, and the count below could not tell the two + # sources of warning apart. + self.assertNotIn(tcp_port, tcp_registry) + self.assertNotIn(udp_port, udp_registry) + self._guard_registry(tcp_registry, tcp_port) + self._guard_registry(udp_registry, udp_port) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + registry.register_tcp(tcp_port, UnitProtocol) + registry.register_udp(udp_port, UnitProtocol) + + self.assertIs(tcp_registry[tcp_port], UnitProtocol) + 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. + + """ + from pcapkit.const.reg.ethertype import EtherType + from pcapkit.const.reg.linktype import LinkType + from pcapkit.protocols.link.link import Link + from pcapkit.protocols.misc.pcap.frame import Frame + from pcapkit.protocols.misc.raw import Raw + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.warnings import RegistryWarning + + cases = ( + ('link-ethertype', Link, EtherType.Internet_Protocol_version_4), + ('pcap-frame-linktype', Frame, LinkType.ETHERNET), + ('tcp-port', TCP, 80), + ) + for label, owner, code in cases: + with self.subTest(registry=label): + sibling = owner.__dict__['__proto__'] + 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. + 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) + def test_register_protocol_validates_and_updates_registry(self) -> None: from pcapkit.foundation.registry import protocols as registry from pcapkit.utilities.exceptions import RegistryError