From 1f05f64020283a60b9766b072f704f7858eaf235 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 19 Sep 2026 20:06:13 -0400 Subject: [PATCH] fix(foundation): make register_mh_extension take schema keyword-only (#516) - register_mh_extension declared `schema` as POSITIONAL_OR_KEYWORD while its fourteen schema-taking siblings all declare it keyword-only; add the missing `*` so the fifteen share one call shape. - tests: add test_schema_registrars_share_one_signature_contract, which discovers every schema-taking register_* helper from `__all__` and pins `code`/`meth` positional and `schema` keyword-only with a None default across all fifteen, rather than only the one that had regressed. - the new test also calls each helper with a third positional argument and requires the interpreter's own arity TypeError, asserting that error is not a pcapkit BaseError: RegistryError derives from TypeError, so a bare assertRaises(TypeError) there would pass against the unfixed code. Verified against the unfixed signature: tests/foundation/registry/test_protocols.py reports 1 failed / 69 subtests passed (exit 1) without the `*`, and 4 passed / 70 subtests passed (exit 0) with it. --- pcapkit/foundation/registry/protocols.py | 2 +- tests/foundation/registry/test_protocols.py | 85 +++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/pcapkit/foundation/registry/protocols.py b/pcapkit/foundation/registry/protocols.py index f009c79509..421f75974f 100644 --- a/pcapkit/foundation/registry/protocols.py +++ b/pcapkit/foundation/registry/protocols.py @@ -534,7 +534,7 @@ def register_mh_option(code: 'MH_Option', meth: 'str | tuple[MH_OptionParser, MH # NOTE: pcapkit.protocols.internet.mh.MH.__extension__ -def register_mh_extension(code: 'MH_CGAExtension', meth: 'str | tuple[MH_ExtensionParser, MH_ExtensionConstructor]', +def register_mh_extension(code: 'MH_CGAExtension', meth: 'str | tuple[MH_ExtensionParser, MH_ExtensionConstructor]', *, schema: 'Optional[Type[Schema_MH_CGAExtension]]' = None) -> 'None': """Register a CGA extension parser. diff --git a/tests/foundation/registry/test_protocols.py b/tests/foundation/registry/test_protocols.py index f521424500..3b8844525d 100644 --- a/tests/foundation/registry/test_protocols.py +++ b/tests/foundation/registry/test_protocols.py @@ -1,6 +1,7 @@ from __future__ import annotations import importlib.util +import inspect import unittest from unittest import mock @@ -9,6 +10,28 @@ RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) +#: Public ``register_*`` helpers that take a schema class. Listed so the signature +#: contract test cannot pass vacuously should discovery ever stop finding them; the +#: test loops over whatever it discovers, so a newly added sibling is covered +#: without this tuple having to be updated. +SCHEMA_REGISTRARS = ( + 'register_hip_parameter', + 'register_hopopt_option', + 'register_http_frame', + 'register_ipv4_option', + 'register_ipv6_opts_option', + 'register_ipv6_route_routing', + 'register_mh_extension', + 'register_mh_message', + 'register_mh_option', + 'register_pcapng_block', + 'register_pcapng_option', + 'register_pcapng_record', + 'register_pcapng_secrets', + 'register_tcp_mp_option', + 'register_tcp_option', +) + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class ProtocolRegistryTests(unittest.TestCase): @@ -207,6 +230,68 @@ def test_option_like_registry_wrappers_validate_methods_and_register_schema(self finally: delattr(owner, attr) + def test_schema_registrars_share_one_signature_contract(self) -> None: + """Every schema-taking ``register_*`` helper exposes the same call shape. + + ``code`` and ``meth`` are positional, ``schema`` is keyword-only. See #516, + where :func:`~pcapkit.foundation.registry.protocols.register_mh_extension` + was missing the ``*`` that its fourteen siblings carry and so accepted + ``schema`` positionally. + + """ + from pcapkit.foundation.registry import protocols as registry + from pcapkit.utilities.exceptions import BaseError + + siblings = {} + for name in registry.__all__: + if not name.startswith('register_'): + continue + signature = inspect.signature(getattr(registry, name)) + if 'schema' in signature.parameters: + siblings[name] = signature + + for name in SCHEMA_REGISTRARS: + self.assertIn(name, siblings) + + sentinel = object() + for name, signature in sorted(siblings.items()): + with self.subTest(func=name): + parameters = signature.parameters + + schema = parameters['schema'] + self.assertIs(schema.kind, inspect.Parameter.KEYWORD_ONLY) + self.assertIsNone(schema.default) + + for positional in ('code', 'meth'): + parameter = parameters[positional] + self.assertIn(parameter.kind, (inspect.Parameter.POSITIONAL_ONLY, + inspect.Parameter.POSITIONAL_OR_KEYWORD)) + self.assertIs(parameter.default, inspect.Parameter.empty) + + # Binding is the contract stated behaviourally: ``code`` and ``meth`` + # take positionally, ``schema`` only by keyword. Nothing is called, + # so this stays free of registry side effects. + signature.bind(sentinel, sentinel) + signature.bind(sentinel, sentinel, schema=sentinel) + with self.assertRaises(TypeError): + signature.bind(sentinel, sentinel, sentinel) + + # The assertions above describe :mod:`inspect`'s model of the + # signature; this one is the function itself refusing the + # positional form. A keyword-only parameter is rejected while + # the interpreter binds arguments, before the body runs, so no + # registry state is touched even though this is a real call. + with self.assertRaises(TypeError) as caught: + getattr(registry, name)(sentinel, sentinel, sentinel) + + # ``RegistryError`` derives from ``TypeError``, so a bare + # ``assertRaises(TypeError)`` would also be satisfied by a body + # that accepted a third positional and then rejected its value + # -- exactly the #516 behaviour this test exists to forbid. + # Pin the interpreter's arity error, not a library one. + self.assertNotIsInstance(caught.exception, BaseError) + self.assertRegex(str(caught.exception), 'positional argument') + if __name__ == '__main__': unittest.main()