Skip to content

Commit cacef95

Browse files
committed
fix(registry): re-registering a stored ModuleDescriptor keeps it (#1364)
- ProtocolBase.register and the Internet, Link, Transport, Frame and PCAPNG overrides resolved a ModuleDescriptor argument to its class before comparing it with the stored entry, so handing back the stored descriptor warned and replaced the deferred entry with the class. They now compare first: the stored entry, or a descriptor equal to it, is a silent no-op that keeps the stored object. A different entry still warns, and invalid input still raises RegistryError. - Extractor.register_dumper already keeps its shipped descriptors since #1376; it is pinned in the new test. New tests/protocols/test_registry_stored_descriptor_unit.py: 6 tests; 7 fail on origin/main (6 subtests and the ProtocolBase case).
1 parent ec2ba13 commit cacef95

7 files changed

Lines changed: 180 additions & 14 deletions

File tree

‎pcapkit/protocols/internet/internet.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,13 +157,17 @@ def register(cls, code: 'Enum_TransType', protocol: 'ModuleDescriptor[ProtocolBa
157157
guard this shares with ``register_protocol``.
158158
159159
"""
160+
incumbent = cls.__proto__.get(code)
161+
if incumbent is not None and (incumbent is protocol or (
162+
isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor)
163+
and incumbent == protocol)):
164+
return
160165
if isinstance(protocol, ModuleDescriptor):
161166
protocol = protocol.klass
162167
if not isinstance(protocol, type):
163168
raise RegistryError(f'protocol must be a class, not {protocol!r}')
164169
if not issubclass(protocol, ProtocolBase):
165170
raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}')
166-
incumbent = cls.__proto__.get(code)
167171
if incumbent is not None and incumbent is not protocol:
168172
warn(f'protocol {code} already registered, overwriting '
169173
f'{incumbent!r} with {protocol!r}', RegistryWarning)

‎pcapkit/protocols/link/link.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,13 +138,17 @@ def register(cls, code: 'Enum_EtherType', protocol: 'ModuleDescriptor[ProtocolBa
138138
guard this shares with ``register_protocol``.
139139
140140
"""
141+
incumbent = cls.__proto__.get(code)
142+
if incumbent is not None and (incumbent is protocol or (
143+
isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor)
144+
and incumbent == protocol)):
145+
return
141146
if isinstance(protocol, ModuleDescriptor):
142147
protocol = protocol.klass
143148
if not isinstance(protocol, type):
144149
raise RegistryError(f'protocol must be a class, not {protocol!r}')
145150
if not issubclass(protocol, ProtocolBase):
146151
raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}')
147-
incumbent = cls.__proto__.get(code)
148152
if incumbent is not None and incumbent is not protocol:
149153
warn(f'protocol {code} already registered, overwriting '
150154
f'{incumbent!r} with {protocol!r}', RegistryWarning)

‎pcapkit/protocols/misc/pcap/frame.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,13 +146,17 @@ def register(cls, code: 'Enum_LinkType', protocol: 'ModuleDescriptor[ProtocolBas
146146
guard this shares with ``register_protocol``.
147147
148148
"""
149+
incumbent = cls.__proto__.get(code)
150+
if incumbent is not None and (incumbent is protocol or (
151+
isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor)
152+
and incumbent == protocol)):
153+
return
149154
if isinstance(protocol, ModuleDescriptor):
150155
protocol = protocol.klass
151156
if not isinstance(protocol, type):
152157
raise RegistryError(f'protocol must be a class, not {protocol!r}')
153158
if not issubclass(protocol, ProtocolBase):
154159
raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}')
155-
incumbent = cls.__proto__.get(code)
156160
if incumbent is not None and incumbent is not protocol:
157161
warn(f'protocol {code} already registered, overwriting '
158162
f'{incumbent!r} with {protocol!r}', RegistryWarning)

‎pcapkit/protocols/misc/pcapng.py‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -996,18 +996,23 @@ def register(cls, code: 'Enum_LinkType', protocol: 'ModuleDescriptor[ProtocolBas
996996
entry and its replacement so a caller can tell *what* was lost.
997997
Fires only when the incumbent differs from the replacement, and
998998
an unresolved module descriptor counts as different from the
999-
class it names. This registry is separate from the PCAP one, so
999+
class it names; handing back the stored entry, or a descriptor
1000+
equal to it, is a silent no-op that keeps it. This registry is separate from the PCAP one, so
10001001
:func:`~pcapkit.foundation.registry.protocols.register_linktype`
10011002
writing to both cannot make either warn about the other.
10021003
10031004
"""
1005+
incumbent = cls.__proto__.get(code)
1006+
if incumbent is not None and (incumbent is protocol or (
1007+
isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor)
1008+
and incumbent == protocol)):
1009+
return
10041010
if isinstance(protocol, ModuleDescriptor):
10051011
protocol = protocol.klass
10061012
if not isinstance(protocol, type):
10071013
raise RegistryError(f'protocol must be a class, not {protocol!r}')
10081014
if not issubclass(protocol, ProtocolBase):
10091015
raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}')
1010-
incumbent = cls.__proto__.get(code)
10111016
if incumbent is not None and incumbent is not protocol:
10121017
warn(f'protocol {code} already registered, overwriting '
10131018
f'{incumbent!r} with {protocol!r}', RegistryWarning)

‎pcapkit/protocols/protocol.py‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -904,22 +904,27 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor | Type[ProtocolBase]'
904904
The guard matches :func:`register_protocol
905905
<pcapkit.foundation.registry.protocols.register_protocol>`'s: it
906906
fires only when the incumbent differs from the replacement, so
907-
re-registering the exact same class object under the same ``code``
908-
is a silent no-op rather than a warning about nothing displaced.
909-
The comparison is a plain ``is`` that resolves nothing, so an
910-
incumbent left as an unresolved
911-
:class:`~pcapkit.corekit.module.ModuleDescriptor` still reports as
912-
different from the class it names, and the deferred import stays
913-
deferred.
907+
handing back the entry already stored under ``code`` -- the same
908+
class object, or a :class:`~pcapkit.corekit.module.ModuleDescriptor`
909+
equal to the stored one -- is a silent no-op rather than a warning
910+
about nothing displaced, and the stored entry is kept as it is
911+
(:issue:`1364`). The comparison runs before ``protocol`` is
912+
resolved and resolves nothing, so a stored descriptor stays
913+
deferred, and an incumbent left as an unresolved descriptor still
914+
reports as different from the class it names.
914915
915916
"""
917+
incumbent = cls.__proto__.get(code)
918+
if incumbent is not None and (incumbent is protocol or (
919+
isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor)
920+
and incumbent == protocol)):
921+
return
916922
if isinstance(protocol, ModuleDescriptor):
917923
protocol = protocol.klass
918924
if not isinstance(protocol, type):
919925
raise RegistryError(f'protocol must be a class, not {protocol!r}')
920926
if not issubclass(protocol, ProtocolBase):
921927
raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}')
922-
incumbent = cls.__proto__.get(code)
923928
if incumbent is not None and incumbent is not protocol:
924929
warn(f'protocol {code} already registered, overwriting '
925930
f'{incumbent!r} with {protocol!r}', RegistryWarning)

‎pcapkit/protocols/transport/transport.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,13 +112,17 @@ def register(cls, code: 'int', protocol: 'ModuleDescriptor[ProtocolBase] | Type[
112112
if cls is Transport:
113113
raise UnsupportedCall(f'{cls.__name__} is an abstract class')
114114

115+
incumbent = cls.__proto__.get(code)
116+
if incumbent is not None and (incumbent is protocol or (
117+
isinstance(incumbent, ModuleDescriptor) and isinstance(protocol, ModuleDescriptor)
118+
and incumbent == protocol)):
119+
return
115120
if isinstance(protocol, ModuleDescriptor):
116121
protocol = protocol.klass
117122
if not isinstance(protocol, type):
118123
raise RegistryError(f'protocol must be a class, not {protocol!r}')
119124
if not issubclass(protocol, ProtocolBase):
120125
raise RegistryError(f'protocol must be a Protocol subclass, not {protocol!r}')
121-
incumbent = cls.__proto__.get(code)
122126
if incumbent is not None and incumbent is not protocol:
123127
warn(f'port {code} already registered, overwriting '
124128
f'{incumbent!r} with {protocol!r}', RegistryWarning)
Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
# -*- coding: utf-8 -*-
2+
"""GitHub issue #1364: re-registering a stored module descriptor keeps it.
3+
4+
``ProtocolBase.register`` and the ``Internet``, ``Link``, ``Transport``,
5+
``Frame`` and ``PCAPNG`` overrides resolved a
6+
:class:`~pcapkit.corekit.module.ModuleDescriptor` argument to its class before
7+
comparing it with the stored entry. Handing back the stored descriptor itself
8+
therefore warned of an overwrite and replaced the deferred entry with the
9+
class. The registrars now compare first: the stored entry, or a descriptor
10+
equal to it, is a silent no-op that keeps the stored object, while a different
11+
entry still warns. ``Extractor.register_dumper`` is pinned alongside; #1376
12+
already keeps its shipped descriptors.
13+
14+
Every registry is process-wide, so each case runs under
15+
:func:`unittest.mock.patch.dict`. Classes are imported inside each test, after
16+
:func:`~tests._support.reimport_once_per_class`.
17+
18+
"""
19+
20+
import importlib
21+
import unittest
22+
import warnings
23+
from unittest import mock
24+
25+
from tests._support import reimport_once_per_class
26+
27+
#: ``(module, class)`` for each registrar, picked so its ``__proto__`` ships
28+
#: module descriptors. IPv4 and Ethernet reach the ``Internet`` and ``Link``
29+
#: overrides; there is no class whose populated ``__proto__`` reaches
30+
#: ``ProtocolBase.register`` itself, so that one is exercised on a scratch
31+
#: subclass in its own test.
32+
SITES = (
33+
('pcapkit.protocols.internet.ipv4', 'IPv4'),
34+
('pcapkit.protocols.link.ethernet', 'Ethernet'),
35+
('pcapkit.protocols.transport.tcp', 'TCP'),
36+
('pcapkit.protocols.transport.udp', 'UDP'),
37+
('pcapkit.protocols.misc.pcap.frame', 'Frame'),
38+
('pcapkit.protocols.misc.pcapng', 'PCAPNG'),
39+
)
40+
41+
42+
class TestRegistryStoredDescriptor(unittest.TestCase):
43+
"""Pin when a protocol registrar warns about a stored descriptor."""
44+
45+
def setUp(self) -> None:
46+
reimport_once_per_class(self)
47+
48+
def _count(self, call, *args) -> int: # type: ignore[no-untyped-def]
49+
"""Run ``call(*args)`` and return how many registry warnings it raised."""
50+
from pcapkit.utilities.warnings import RegistryWarning
51+
52+
with warnings.catch_warnings(record=True) as caught:
53+
warnings.simplefilter('always')
54+
call(*args)
55+
return sum(issubclass(item.category, RegistryWarning) for item in caught)
56+
57+
def _each_site(self, check): # type: ignore[no-untyped-def]
58+
"""Run ``check(register, registry)`` for every site, each in its own ``subTest``."""
59+
from pcapkit.corekit.module import ModuleDescriptor
60+
61+
for module, name in SITES:
62+
cls = getattr(importlib.import_module(module), name)
63+
registry = cls.__proto__
64+
self.assertTrue(any(isinstance(value, ModuleDescriptor) for value in registry.values()), name)
65+
with self.subTest(site=name), mock.patch.dict(registry):
66+
check(cls.register, registry)
67+
68+
def test_reregistering_the_stored_descriptor_is_silent(self) -> None:
69+
from pcapkit.corekit.module import ModuleDescriptor
70+
71+
def check(register, registry): # type: ignore[no-untyped-def]
72+
for code, value in list(registry.items()):
73+
if not isinstance(value, ModuleDescriptor):
74+
continue
75+
self.assertEqual(self._count(register, code, value), 0, code)
76+
self.assertIs(registry[code], value)
77+
# An equal descriptor built afresh is the same entry, and the
78+
# stored object is kept.
79+
self.assertEqual(self._count(register, code, ModuleDescriptor(*value)), 0, code)
80+
self.assertIs(registry[code], value)
81+
self._each_site(check)
82+
83+
def test_reregistering_the_stored_class_is_silent(self) -> None:
84+
def check(register, registry): # type: ignore[no-untyped-def]
85+
code, value = next(iter(registry.items()))
86+
klass = value.klass
87+
self.assertEqual(self._count(register, code, klass), 1)
88+
self.assertIs(registry[code], klass)
89+
self.assertEqual(self._count(register, code, klass), 0)
90+
self.assertIs(registry[code], klass)
91+
self._each_site(check)
92+
93+
def test_a_different_entry_still_warns(self) -> None:
94+
from pcapkit.corekit.module import ModuleDescriptor
95+
from pcapkit.protocols.misc.raw import Raw
96+
97+
def check(register, registry): # type: ignore[no-untyped-def]
98+
code = next(iter(registry))
99+
other = ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw')
100+
self.assertEqual(self._count(register, code, other), 1)
101+
self.assertIs(registry[code], Raw)
102+
self._each_site(check)
103+
104+
def test_an_invalid_entry_equal_to_a_descriptor_still_raises(self) -> None:
105+
from pcapkit.utilities.exceptions import RegistryError
106+
107+
def check(register, registry): # type: ignore[no-untyped-def]
108+
code, value = next(iter(registry.items()))
109+
with self.assertRaisesRegex(RegistryError, r'^protocol must be a class, not '):
110+
register(code, tuple(value))
111+
self.assertIs(registry[code], value)
112+
self._each_site(check)
113+
114+
def test_protocol_base_register_keeps_the_stored_descriptor(self) -> None:
115+
from pcapkit.corekit.module import ModuleDescriptor
116+
from pcapkit.protocols.misc.raw import Raw
117+
118+
descriptor = ModuleDescriptor('pcapkit.protocols.misc.raw', 'Raw')
119+
scratch = type('W1364Raw', (Raw,), {'__proto__': {1: descriptor}})
120+
self.assertEqual(self._count(scratch.register, 1, descriptor), 0)
121+
self.assertIs(scratch.__proto__[1], descriptor)
122+
self.assertEqual(self._count(scratch.register, 1, ModuleDescriptor(*descriptor)), 0)
123+
self.assertIs(scratch.__proto__[1], descriptor)
124+
other = ModuleDescriptor('pcapkit.protocols.misc.null', 'NoPayload')
125+
self.assertEqual(self._count(scratch.register, 1, other), 1)
126+
self.assertIs(scratch.__proto__[1], other.klass)
127+
128+
def test_register_dumper_keeps_the_stored_descriptor(self) -> None:
129+
from pcapkit.corekit.module import ModuleDescriptor
130+
from pcapkit.foundation.extraction import Extractor
131+
132+
with mock.patch.dict(Extractor.__output__):
133+
entry = Extractor.__output__['json']
134+
self.assertIsInstance(entry[0], ModuleDescriptor)
135+
self.assertEqual(self._count(Extractor.register_dumper, 'json', *entry), 0)
136+
self.assertIs(Extractor.__output__['json'][0], entry[0])
137+
138+
139+
if __name__ == '__main__':
140+
unittest.main()

0 commit comments

Comments
 (0)