Skip to content

fix(registry): add register_protocol's identity guard to sibling registrars #726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JarryShaw merged 1 commit into from
Sep 24, 2026
Merged

fix(registry): add register_protocol's identity guard to sibling registrars #726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions pcapkit/foundation/registry/protocols.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,22 +168,22 @@ def register_protocol(protocol: 'Type[Protocol]') -> 'None':
<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.
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
Expand Down
12 changes: 7 additions & 5 deletions pcapkit/protocols/internet/internet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
<pcapkit.protocols.protocol.ProtocolBase.register>` for why that
differs from ``register_protocol``.
Fires only when the incumbent differs from the replacement --
see :meth:`ProtocolBase.register
<pcapkit.protocols.protocol.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

##########################################################################
Expand Down
14 changes: 8 additions & 6 deletions pcapkit/protocols/link/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
<pcapkit.protocols.protocol.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
<pcapkit.protocols.protocol.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

##########################################################################
Expand Down
12 changes: 7 additions & 5 deletions pcapkit/protocols/misc/pcap/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
<pcapkit.protocols.protocol.ProtocolBase.register>` for why that
differs from ``register_protocol``.
Fires only when the incumbent differs from the replacement --
see :meth:`ProtocolBase.register
<pcapkit.protocols.protocol.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':
Expand Down
9 changes: 6 additions & 3 deletions pcapkit/protocols/misc/pcapng.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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
Expand Down
36 changes: 17 additions & 19 deletions pcapkit/protocols/protocol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
<pcapkit.foundation.registry.protocols.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
<pcapkit.foundation.registry.protocols.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
Expand Down
32 changes: 19 additions & 13 deletions pcapkit/protocols/schema/misc/pcapng.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <https://github.com/JarryShaw/PyPCAPKit/issues/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 <https://github.com/JarryShaw/PyPCAPKit/issues/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
Expand All @@ -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
Expand All @@ -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)}, '
Expand Down
30 changes: 20 additions & 10 deletions pcapkit/protocols/schema/schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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
<pcapkit.foundation.registry.protocols.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
<pcapkit.foundation.registry.protocols.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
Expand All @@ -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)

Expand Down
10 changes: 6 additions & 4 deletions pcapkit/protocols/transport/sctp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
<pcapkit.protocols.transport.transport.Transport.register>` it
overrides does.

Expand All @@ -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
Expand Down
Loading
Loading