Skip to content

Generalise #681's registry overwrite guard: the schema half of every registrar is silent, and the seven code-keyed ones do not say what they displaced #692

Description

@JarryShaw

Following #681, which made register_protocol report the protocol-name collision it had been hiding. The ask, in the owner's words:

i think we should apply what #681 added to other registry as well.

What a survey of every registrar actually found

Surveyed with an AST walk over pcapkit/ rather than a grep, because a grep range terminated early and produced a false reading. There are 12 register() implementations. Classifying them by what they do on a key collision — rather than by whether the body happens to call warn() — gives a different and more useful picture than a warn/silent split:

Behaviour on collision Count Sites
Warns, on presence alone 7 protocols/protocol.py:758, protocols/internet/internet.py:136, protocols/link/link.py:116, protocols/misc/pcap/frame.py:123, protocols/misc/pcapng.py:857, protocols/transport/transport.py:72, protocols/transport/sctp.py:593
Silently overwrites 2 protocols/schema/schema.py:1115 (EnumSchema.register), protocols/schema/misc/pcapng.py:588 (Option.register)
Raises RegistryError 1 corekit/context.py:125 (ContextRegistry.register)
Unkeyed sequence — cannot collide 2 foundation/reassembly/reassembly.py:397, protocols/internet/esp.py:916

Three of those deserve spelling out, because a warn/silent reading of the same list misclassifies them:

  • ContextRegistry.register already refuses a duplicate outright — raise RegistryError(f'context already registered for protocol: {index}'). That is a stricter guard than a warning, so it needs nothing; relaxing it to a warning would be a regression.
  • Reassembly.register appends a callback to __callback_fn__, a list. There is no key, so there is no overwrite.
  • ESPContext.register appends to __associations__, also a list — and duplicate SPIs are a designed feature there, not a mistake: the class documents its associations as being supplied "in order of preference for otherwise equal matches", and ESPContext.match scores them. A guard would flag a supported path.

So exactly one registrar silently overwrites and is in scope here: EnumSchema.register. (schema/misc/pcapng.py:588 is the other, and is #678's.)

EnumSchema.register is the consequential one

Its body is a bare cls.__enum__[code] = schema with no guard, and it is the schema half of 14 public registrars in pcapkit/foundation/registry/protocols.py — IPv4 Option, HIP Parameter, HOPOPT Option, IPv6-Opts Option, IPv6-Route RoutingType, MH Packet/Option/CGAExtension, TCP Option/MPTCP, HTTP FrameType, PCAP-NG BlockType/NameResolutionRecord/DSBSecrets.

The defect this exposes is an asymmetry within a single call. Each of those register_* helpers registers two halves of one binding: a parser class, and a schema class. The parser half has warned on an overwrite for as long as it has existed (protocols/internet/ipv4.py:482, hopopt.py:367, ipv6_opts.py:378, hip.py:651, mh.py:1483/1496/1509, ipv6_route.py:355, application/httpv2.py:367, transport/sctp.py:618/632/645/658). The schema half assigned bare. So one register_ipv4_option call replacing a built-in named the parser it displaced and said nothing about the schema — half a report for one call.

There is a second way into the same registry: EnumSchema.__init_subclass__ assigns cls.__enum__[code] directly, so class MyOption(Option, code=...) — the documented way to add a schema — bypasses register entirely. Guarding only the method would leave the declaration path silently displacing a built-in.

The part of #681 that should not be generalised

#681 deliberately used "key present and the incumbent is a different class" where its siblings warn on presence alone. That refinement is licensed by two properties its registry has and none of the seven do:

  1. Its key is derived from the value (cls.__name__.upper()), and it is the single funnel nine public registrars end in — so registering one class under two codes reaches it twice with nothing displaced. All seven take a caller-supplied code that is independent of the value, so one class under two codes yields two distinct keys and the spurious-warning case cannot arise. A repeat for one code is a caller mistake worth reporting even when the value is unchanged.
  2. __proto__ is pre-seeded with unresolved ModuleDescriptor values (link.py:76, internet.py:91, frame.py:90, misc/pcapng.py:557, sctp.py:355, tcp.py:314, udp.py:88), so an incumbent may be a two-string descriptor while the replacement is the very class it names. "A different class" is not decidable there without resolving the descriptor — forcing the import the descriptor exists to defer, purely to decide whether to warn.

#681 already encoded this as an executable decision: tests/foundation/registry/test_protocols.py:230 asserts the siblings still warn on an identical re-registration, and says in its own docstring that it "fails if anyone ever 'harmonises' the siblings onto the guard used above". Presence-only is correct for all seven and should stay.

What does generalise

The message. #681's names the displaced entry and its replacement; the seven say only 'protocol {code} already registered, overwriting' and stop, so a caller learns that something was displaced and never which class it was. That is the same diagnostic gap, and closing it changes nothing about when the warning fires.

Proposed

  1. Guard EnumSchema.register on presence, with a message naming both classes.
  2. Guard the __init_subclass__ declaration path into the same registry, so the two ways in behave alike.
  3. Give all seven code-keyed registrars the richer message, leaving their presence-only condition exactly as it is.

A presence-only guard is only sound on __enum__ because of #555: _EnumRegistry.__missing__ returns a miss without recording it. On a plain defaultdict a bare registry[code] for an unregistered code inserted the default, so parsing one packet carrying an unknown code would have made the next legitimate registration for that code warn about an entry no caller ever asked for. _EnumRegistry's own docstring already anticipates this guard in those terms. The same hazard was fixed for the parser-layer __proto__ family by #421 and #425/#428.

Relation to #514 and #682

#514's recommended staging names the register_protocol collision as its c1 prerequisite, landed by #681. This is not c1 and does not extend it: it adds no key-space change, so #682's finding that every __proto__ reader degrades silently on a miss is untouched, and the re-keying question stays entirely with #514/#682. What it does do is settle, with tests, that the two guard shapes are deliberately different — so a later re-keying does not have to rediscover why, and cannot quietly harmonise them on the way past.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementIssues requesting a new capability (set by the feature request template)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions