fix(registry): report the protocol-name collision register_protocol hid (#675)

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
+    <pcapkit.protocols.protocol.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
+    <pcapkit.protocols.protocol.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
+    <pcapkit.protocols.protocol.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__)
 
 

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__
+        <pcapkit.protocols.protocol.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