diff --git a/docs/source/contributing/conventions/mint-criterion.rst b/docs/source/contributing/conventions/mint-criterion.rst index 225dae6bc..962ebe81a 100644 --- a/docs/source/contributing/conventions/mint-criterion.rst +++ b/docs/source/contributing/conventions/mint-criterion.rst @@ -10,7 +10,7 @@ a ``_missing_`` override, which decides what happens when a value has no member. MRO, so they carry no declared-but-unassigned range to resolve. 121 define one themselves; the other four are the transport subclasses of :class:`~pcapkit.const.reg.apptype.apptype.AppType`, which inherit -``AppType._missing_`` (:file:`pcapkit/const/reg/apptype/apptype.py:2958`). Three +``AppType._missing_`` (:file:`pcapkit/const/reg/apptype/apptype.py:2994`). Three behaviours follow from it, and which one a given range gets is a **design decision, not a style preference**: diff --git a/pcapkit/const/reg/apptype/apptype.py b/pcapkit/const/reg/apptype/apptype.py index c45069caf..d77c96f76 100644 --- a/pcapkit/const/reg/apptype/apptype.py +++ b/pcapkit/const/reg/apptype/apptype.py @@ -17,6 +17,7 @@ from pcapkit.corekit.enum import EnumLookup, EnumRegistry from pcapkit.corekit.sentinels import NO_DEFAULT +from pcapkit.utilities.exceptions import EnumValueError __all__ = ['TransportProtocol', 'AppType'] @@ -2773,6 +2774,27 @@ def _sanitize_identifier(cls, name: 'str') -> 'str': identifier = f'{identifier}_' return identifier + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + """Reject ``value`` unless it is a port number in ``0``-``65535``. + + Overrides :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` + with the check :meth:`_missing_` makes, and the message it raises, so + :meth:`register` refuses exactly the ports :meth:`get` refuses, in the + same words (:issue:`1792`). + + Args: + value: Port number about to be minted. + + Raises: + EnumValueError: If ``value`` is not an :class:`int` in + ``0``-``65535``. A :exc:`ValueError`, so an ``except + ValueError`` caller is unaffected. + + """ + if not (isinstance(value, int) and 0 <= value <= 65535): + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + @classmethod def register(cls, port: 'int', name: 'str') -> 'AppType': # pylint: disable=arguments-renamed """Mint the first, canonical member at ``port``, in this registry alone. @@ -2793,6 +2815,10 @@ def register(cls, port: 'int', name: 'str') -> 'AppType': # pylint: disable=arg the operation where guessing the right transport for the caller is the wrong default. + Routes ``port`` through :meth:`_validate_value` after the duplicate + check, as :meth:`~pcapkit.corekit.enum.EnumRegistry.register` does, so + a port :meth:`get` refuses cannot be minted here either. + Args: port: Port number to register under. Must not already carry a member of this registry -- see :meth:`register_alias` for @@ -2811,6 +2837,10 @@ def register(cls, port: 'int', name: 'str') -> 'AppType': # pylint: disable=arg is not a valid Python identifier at all; or if the identifier it does sanitise to already names an unrelated member of this registry. + EnumValueError: If ``port`` is not an :class:`int` in + ``0``-``65535``, from :meth:`_validate_value`. A + :exc:`ValueError`, carrying the message :meth:`get` raises for + the same port. """ if cls.__registry__ is None: @@ -2823,6 +2853,7 @@ def register(cls, port: 'int', name: 'str') -> 'AppType': # pylint: disable=arg f'({", ".join(member.svc for member in existing)}); use ' f'{cls.__name__}.register_alias() to add a further service name ' 'for it') + cls._validate_value(port) identifier = cls._sanitize_identifier(name) if not identifier.isidentifier(): diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index 990eb42a0..8c9b4ea37 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -218,9 +218,9 @@ def _validate_value(cls, value: 'Any') -> 'None': Of the registries that override :meth:`EnumRegistry.register` to mint through a path of their own, :meth:`OptionType.register ` - calls it at the same point and + and :meth:`AppType.register ` - does not. + both call it at the same point. :meth:`EnumRegistry.register_alias` does not call it, and does not need to: it refuses any ``value`` that is not already registered, so the value diff --git a/pcapkit/vendor/reg/apptype/apptype.py b/pcapkit/vendor/reg/apptype/apptype.py index a0e48c51d..638eb8cdc 100644 --- a/pcapkit/vendor/reg/apptype/apptype.py +++ b/pcapkit/vendor/reg/apptype/apptype.py @@ -110,6 +110,7 @@ from pcapkit.corekit.enum import EnumLookup, EnumRegistry from pcapkit.corekit.sentinels import NO_DEFAULT +from pcapkit.utilities.exceptions import EnumValueError __all__ = ['TransportProtocol', '{NAME}'] @@ -728,6 +729,27 @@ def _sanitize_identifier(cls, name: 'str') -> 'str': identifier = f'{{identifier}}_' return identifier + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + """Reject ``value`` unless it is a port number in ``0``-``65535``. + + Overrides :meth:`~pcapkit.corekit.enum.EnumLookup._validate_value` + with the check :meth:`_missing_` makes, and the message it raises, so + :meth:`register` refuses exactly the ports :meth:`get` refuses, in the + same words (:issue:`1792`). + + Args: + value: Port number about to be minted. + + Raises: + EnumValueError: If ``value`` is not an :class:`int` in + ``0``-``65535``. A :exc:`ValueError`, so an ``except + ValueError`` caller is unaffected. + + """ + if not ({FLAG}): + raise EnumValueError(f'{{value!r}} is not a valid {{cls.__name__}}') + @classmethod def register(cls, port: 'int', name: 'str') -> '{NAME}': # pylint: disable=arguments-renamed """Mint the first, canonical member at ``port``, in this registry alone. @@ -748,6 +770,10 @@ def register(cls, port: 'int', name: 'str') -> '{NAME}': # pylint: disable=argu the operation where guessing the right transport for the caller is the wrong default. + Routes ``port`` through :meth:`_validate_value` after the duplicate + check, as :meth:`~pcapkit.corekit.enum.EnumRegistry.register` does, so + a port :meth:`get` refuses cannot be minted here either. + Args: port: Port number to register under. Must not already carry a member of this registry -- see :meth:`register_alias` for @@ -766,6 +792,10 @@ def register(cls, port: 'int', name: 'str') -> '{NAME}': # pylint: disable=argu is not a valid Python identifier at all; or if the identifier it does sanitise to already names an unrelated member of this registry. + EnumValueError: If ``port`` is not an :class:`int` in + ``0``-``65535``, from :meth:`_validate_value`. A + :exc:`ValueError`, carrying the message :meth:`get` raises for + the same port. """ if cls.__registry__ is None: @@ -778,6 +808,7 @@ def register(cls, port: 'int', name: 'str') -> '{NAME}': # pylint: disable=argu f'({{", ".join(member.svc for member in existing)}}); use ' f'{{cls.__name__}}.register_alias() to add a further service name ' 'for it') + cls._validate_value(port) identifier = cls._sanitize_identifier(name) if not identifier.isidentifier(): diff --git a/tests/const/test_const_apptype_register_range_1792_unit.py b/tests/const/test_const_apptype_register_range_1792_unit.py new file mode 100644 index 000000000..d75a3ab8d --- /dev/null +++ b/tests/const/test_const_apptype_register_range_1792_unit.py @@ -0,0 +1,131 @@ +# -*- coding: utf-8 -*- +"""``AppType.register`` refuses a port that ``get`` refuses. + +GitHub issue #1792. :meth:`~pcapkit.const.reg.apptype.apptype.AppType.register` +neither called nor overrode +:meth:`~pcapkit.corekit.enum.EnumLookup._validate_value`, so ``TCP.register(70000, +'x')`` minted a member, and ``AppType.get(70000, proto='tcp')`` then returned it +although ``get`` raises :exc:`ValueError` for a port outside ``0``-``65535`` +(#758). ``AppType`` now overrides the hook with :meth:`_missing_`'s own check and +``register`` calls it after its duplicate check, as the base ``register`` does. +The override raises :exc:`~pcapkit.utilities.exceptions.EnumValueError`, as the +registry-protocol convention asks of one, with the message ``get`` raises. + +Each test imports the registries after +:func:`~tests._support.reimport_once_per_class`, whose ``restore`` puts the +class's import back afterwards, so a member minted here does not leak. + +""" +from __future__ import annotations + +import itertools +import unittest + +from tests._support import reimport_once_per_class + +#: Serial for probe names, so that no refusal is a name collision instead. +_SERIAL = itertools.count() + +#: Ports outside ``0``-``65535``, on both sides of the range. +OUT_OF_RANGE = (-1, 65536, 70000) + +#: Ports that are not an :class:`int` at all, and hash like no registered port. +NOT_AN_INT = ('8080', 1.5) + + +def _registries() -> 'tuple[type, ...]': + from pcapkit.const.reg.apptype import DCCP, SCTP, TCP, UDP + + return TCP, UDP, SCTP, DCCP + + +class AppTypeRegisterRange1792Tests(unittest.TestCase): + """``register`` and ``get`` agree on what a port number is.""" + + def setUp(self) -> None: + reimport_once_per_class(self, restore=True) + + def assertRefusedWithoutMinting(self, registry: 'type', port: 'object') -> 'str': + """Assert ``registry.register(port, ...)`` raises and mints nothing. + + The refusal is an :exc:`~pcapkit.utilities.exceptions.EnumValueError`, + and so still a :exc:`ValueError` to an ``except ValueError`` caller. + + Returns: + The message the refusal carried. + + """ + from pcapkit.utilities.exceptions import EnumValueError + + members = dict(registry.__members__) # type: ignore[attr-defined] + with self.assertRaises(ValueError) as caught: + registry.register(port, f'pypcapkit-1792-probe-{next(_SERIAL)}') # type: ignore[attr-defined] + self.assertIsInstance(caught.exception, ValueError) + self.assertIs(type(caught.exception), EnumValueError) + self.assertEqual(dict(registry.__members__), members) # type: ignore[attr-defined] + self.assertFalse(registry.__registry__.getlist(port)) # type: ignore[attr-defined] + return str(caught.exception) + + def test_register_refuses_an_out_of_range_port_as_get_does(self) -> None: + for registry in _registries(): + for port in OUT_OF_RANGE: + with self.subTest(registry=registry.__name__, port=port): + message = self.assertRefusedWithoutMinting(registry, port) + with self.assertRaises(ValueError) as lookup: + registry.get(port) # type: ignore[attr-defined] + self.assertEqual(message, str(lookup.exception)) + self.assertEqual(message, f'{port!r} is not a valid {registry.__name__}') + + def test_register_refuses_a_non_int_port_as_the_constructor_does(self) -> None: + for registry in _registries(): + for port in NOT_AN_INT: + with self.subTest(registry=registry.__name__, port=port): + message = self.assertRefusedWithoutMinting(registry, port) + with self.assertRaises(ValueError) as built: + registry(port) + self.assertEqual(message, str(built.exception)) + + def test_get_still_raises_after_a_refused_register(self) -> None: + from pcapkit.const.reg.apptype import AppType + + for registry in _registries(): + for port in OUT_OF_RANGE: + with self.subTest(registry=registry.__name__, port=port): + with self.assertRaises(ValueError): + registry.register(port, f'pypcapkit-1792-probe-{next(_SERIAL)}') # type: ignore[attr-defined] + with self.assertRaises(ValueError): + registry.get(port) # type: ignore[attr-defined] + with self.assertRaises(ValueError): + AppType.get(port, proto=registry.__transport__) # type: ignore[attr-defined] + + def test_register_still_mints_the_top_port_of_the_range(self) -> None: + from pcapkit.const.reg.apptype import AppType + + port = 65535 + for registry in _registries(): + with self.subTest(registry=registry.__name__): + self.assertFalse(registry.__registry__.getlist(port)) # type: ignore[attr-defined] + member = registry.register(port, 'pypcapkit-1792-top') # type: ignore[attr-defined] + self.assertEqual(member.port, port) + self.assertIs(member.proto, registry.__transport__) # type: ignore[attr-defined] + self.assertIs(registry.get(port), member) # type: ignore[attr-defined] + self.assertIs(AppType.get(port, proto=registry.__transport__), member) # type: ignore[attr-defined] + + def test_validate_value_agrees_with_missing_on_every_probe(self) -> None: + from pcapkit.utilities.exceptions import EnumValueError + + for registry in _registries(): + for port in (0, 65535, *OUT_OF_RANGE, *NOT_AN_INT): + with self.subTest(registry=registry.__name__, port=port): + try: + registry._missing_(port) # type: ignore[attr-defined] + except ValueError as error: + with self.assertRaises(EnumValueError) as caught: + registry._validate_value(port) # type: ignore[attr-defined] + self.assertEqual(str(caught.exception), str(error)) + else: + self.assertIsNone(registry._validate_value(port)) # type: ignore[attr-defined] + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/project/test_rfc_anchor_fragments.py b/tests/project/test_rfc_anchor_fragments.py index 4e56785ae..670a0715a 100644 --- a/tests/project/test_rfc_anchor_fragments.py +++ b/tests/project/test_rfc_anchor_fragments.py @@ -41,7 +41,7 @@ test exists to catch, the same mistake as the allowlist above one layer down. The generators that produce them, :file:`pcapkit/vendor/tcp/mp_tcp_option.py:59` and -:file:`pcapkit/vendor/reg/apptype/apptype.py:1169` (both parsing IANA's +:file:`pcapkit/vendor/reg/apptype/apptype.py:1200` (both parsing IANA's ``RFC8684, Section 3.1`` with the same ``re.fullmatch(r'RFC(?P\\d+)(, Section (?P.*?))?', ...)``, which discards the literal word ``Section`` and keeps only the number), are fixed to emit ``#section-{sec}``, and the ten