diff --git a/pcapkit/const/ftp/command.py b/pcapkit/const/ftp/command.py index 8f53e56a08..1bf838cc9f 100644 --- a/pcapkit/const/ftp/command.py +++ b/pcapkit/const/ftp/command.py @@ -47,6 +47,8 @@ def _missing_(cls, value: 'str') -> 'FEATCode': value: Value to get enum item. """ + if not isinstance(value, str): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) return extend_enum(cls, value.upper(), value) @@ -62,6 +64,18 @@ class CommandType(IntFlag): #: Service execution. S = auto() + @classmethod + def _missing_(cls, value: 'int') -> 'CommandType': + """Lookup function used when value is not found. + + Args: + value: Value to get enum item. + + """ + if not (isinstance(value, int) and 0 <= value <= 0x07): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + return super()._missing_(value) + class ConformanceRequirement(IntEnum): """Expectation for support in modern FTP implementations.""" @@ -309,6 +323,8 @@ def _missing_(cls, value: 'str') -> 'Command': the canonical upper-case member names. """ + if not isinstance(value, str): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) name = value.upper() if name in cls._member_map_: return cls._member_map_[name] # type: ignore[return-value] diff --git a/pcapkit/const/http/method.py b/pcapkit/const/http/method.py index 9b3f27c859..2535dfd4e1 100644 --- a/pcapkit/const/http/method.py +++ b/pcapkit/const/http/method.py @@ -187,6 +187,8 @@ def _missing_(cls, value: 'str') -> 'Method': the canonical upper-case member names. """ + if not isinstance(value, str): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) name = value.upper() if name in cls._member_map_: return cls._member_map_[name] # type: ignore[return-value] diff --git a/pcapkit/const/reg/apptype.py b/pcapkit/const/reg/apptype.py index 21024c0159..03dde586ac 100644 --- a/pcapkit/const/reg/apptype.py +++ b/pcapkit/const/reg/apptype.py @@ -53,6 +53,18 @@ def get(key: 'int | str') -> 'TransportProtocol': max_val = max(TransportProtocol.__members__.values()) return extend_enum(TransportProtocol, key.lower(), max_val * 2) + @classmethod + def _missing_(cls, value: 'int') -> 'TransportProtocol': + """Lookup function used when value is not found. + + Args: + value: Value to get enum item. + + """ + if not (isinstance(value, int) and 0 <= value <= max(cls.__members__.values()) * 2 - 1): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + return super()._missing_(value) + class AppType(StrEnum): """[AppType] Application Layer Protocol Numbers""" diff --git a/pcapkit/const/tcp/flags.py b/pcapkit/const/tcp/flags.py index d011864b93..f5301a30f9 100644 --- a/pcapkit/const/tcp/flags.py +++ b/pcapkit/const/tcp/flags.py @@ -78,3 +78,15 @@ def get(key: 'int | str', default: 'Optional[int]' = -1) -> 'Flags': raise return Flags(default) return Flags[key] # type: ignore[misc] + + @classmethod + def _missing_(cls, value: 'int') -> 'Flags': + """Lookup function used when value is not found. + + Args: + value: Value to get enum item. + + """ + if not (isinstance(value, int) and 0 <= value <= 0xFFFF): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + return super()._missing_(value) diff --git a/pcapkit/vendor/ftp/command.py b/pcapkit/vendor/ftp/command.py index 0afc4d7f4a..6d0af265a8 100644 --- a/pcapkit/vendor/ftp/command.py +++ b/pcapkit/vendor/ftp/command.py @@ -87,6 +87,8 @@ def _missing_(cls, value: 'str') -> 'FEATCode': value: Value to get enum item. """ + if not isinstance(value, str): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) return extend_enum(cls, value.upper(), value) @@ -102,6 +104,18 @@ class CommandType(IntFlag): #: Service execution. S = auto() + @classmethod + def _missing_(cls, value: 'int') -> 'CommandType': + """Lookup function used when value is not found. + + Args: + value: Value to get enum item. + + """ + if not (isinstance(value, int) and 0 <= value <= 0x07): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + return super()._missing_(value) + class ConformanceRequirement(IntEnum): """Expectation for support in modern FTP implementations.""" @@ -171,6 +185,8 @@ def _missing_(cls, value: 'str') -> '{NAME}': the canonical upper-case member names. """ + if not isinstance(value, str): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) name = value.upper() if name in cls._member_map_: return cls._member_map_[name] # type: ignore[return-value] diff --git a/pcapkit/vendor/http/method.py b/pcapkit/vendor/http/method.py index 21ba94be75..0ee4d5bc66 100644 --- a/pcapkit/vendor/http/method.py +++ b/pcapkit/vendor/http/method.py @@ -92,6 +92,8 @@ def _missing_(cls, value: 'str') -> '{NAME}': the canonical upper-case member names. """ + if not isinstance(value, str): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) name = value.upper() if name in cls._member_map_: return cls._member_map_[name] # type: ignore[return-value] diff --git a/pcapkit/vendor/reg/apptype.py b/pcapkit/vendor/reg/apptype.py index 0f823230d4..151c9e37de 100644 --- a/pcapkit/vendor/reg/apptype.py +++ b/pcapkit/vendor/reg/apptype.py @@ -81,6 +81,18 @@ def get(key: 'int | str') -> 'TransportProtocol': max_val = max(TransportProtocol.__members__.values()) return extend_enum(TransportProtocol, key.lower(), max_val * 2) + @classmethod + def _missing_(cls, value: 'int') -> 'TransportProtocol': + """Lookup function used when value is not found. + + Args: + value: Value to get enum item. + + """ + if not (isinstance(value, int) and 0 <= value <= max(cls.__members__.values()) * 2 - 1): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + return super()._missing_(value) + class {NAME}(StrEnum): """[{NAME}] {DOCS}""" diff --git a/pcapkit/vendor/tcp/flags.py b/pcapkit/vendor/tcp/flags.py index 0c1bc8706b..317f4f9641 100644 --- a/pcapkit/vendor/tcp/flags.py +++ b/pcapkit/vendor/tcp/flags.py @@ -34,7 +34,7 @@ } #: Default constant template of enumerate registry from IANA CSV. -LINE = lambda NAME, DOCS, ENUM, MODL: f'''\ +LINE = lambda NAME, DOCS, FLAG, ENUM, MODL: f'''\ # -*- coding: utf-8 -*- # pylint: disable=line-too-long,consider-using-f-string """{(name := DOCS.split(' [', maxsplit=1)[0])} @@ -81,14 +81,30 @@ def get(key: 'int | str', default: 'Optional[int]' = -1) -> '{NAME}': raise return Flags(default) return {NAME}[key] # type: ignore[misc] -'''.strip() # type: Callable[[str, str, str, str], str] + + @classmethod + def _missing_(cls, value: 'int') -> '{NAME}': + """Lookup function used when value is not found. + + Args: + value: Value to get enum item. + + """ + if not ({FLAG}): + raise ValueError('%r is not a valid %s' % (value, cls.__name__)) + return super()._missing_(value) +'''.strip() # type: Callable[[str, str, str, str, str], str] class Flags(Vendor): """TCP Header Flags""" - #: Value limit checker. - FLAG = 'isinstance(value, int) and 4 <= value <= 15' + #: Value limit checker. The registry indexes *bit offsets* 4 through 15, + #: but the members it generates are ``1 << offset``, so the value domain a + #: lookup has to accept is the 16-bit field those bits live in -- a + #: composite such as ``SYN | ACK`` is a legitimate value and only the + #: composite path reaches :meth:`~pcapkit.const.tcp.flags.Flags._missing_`. + FLAG = 'isinstance(value, int) and 0 <= value <= 0xFFFF' #: Link to registry. LINK = 'https://www.iana.org/assignments/tcp-parameters/tcp-header-flags.csv' @@ -149,7 +165,7 @@ def context(self, data: 'list[str]') -> 'str': enum = self.process(data) ENUM = '\n\n '.join(map(lambda s: s.rstrip(), enum)).strip() - return LINE(self.NAME, self.DOCS, ENUM, self.__module__) + return LINE(self.NAME, self.DOCS, self.FLAG, ENUM, self.__module__) if __name__ == '__main__': diff --git a/tests/const/test_const_enum_builtin_parity.py b/tests/const/test_const_enum_builtin_parity.py new file mode 100644 index 0000000000..52786eddf1 --- /dev/null +++ b/tests/const/test_const_enum_builtin_parity.py @@ -0,0 +1,650 @@ +# -*- coding: utf-8 -*- +"""Every :mod:`pcapkit.const` registry must reject an invalid value the way the built-in does. + +GitHub issue #647 counted the generated range guards under :mod:`pcapkit.const` +and found that 113 of the 117 modules raise a bare :exc:`ValueError`. The issue +proposed replacing them with :class:`~pcapkit.utilities.exceptions.EnumError`; +the decision went the other way. A bare :exc:`ValueError` is exactly what +:class:`enum.IntEnum` raises for a value it does not define, mirroring the +built-in is the intent, and +:class:`~pcapkit.utilities.exceptions.EnumError` is ``(BaseError, TypeError)`` -- +not a :exc:`ValueError` at all -- so swapping it in would have diverged from the +built-in *and* walked past the ``except ValueError`` in all 113 generated +``get()`` bodies, silently undoing GitHub issue #584. + +What was left was the inconsistency. Six registries across four modules did +something other than raise: + +* three :class:`~aenum.IntFlag` registries defined no ``_missing_`` at all, so + ``aenum``'s :class:`~aenum.Flag` machinery composed a pseudo-member for any + integer whatsoever. ``Flags(-1)`` returned ``65520`` -- the OR of every + declared TCP header flag -- so a value no 16-bit wire field can hold read back + as *every flag set at once*, and ``Flags(-65536)`` as *none set*. +* three :class:`~aenum.StrEnum` registries reached ``value.upper()`` in their + ``_missing_`` before checking the type, so an integer raised + :exc:`AttributeError` (``'int' object has no attribute 'upper'``) rather than + :exc:`ValueError`. + +All six now carry a guard raising a bare :exc:`ValueError`, emitted by their +templates under :mod:`pcapkit.vendor` rather than hand-written -- the tree is +generated, so a fix that is not in the template is reverted by the next crawl. + +The one deliberate divergence from the built-in is preserved and pinned here: +the *mutable* registries look a value up, miss, and then **register** it with +:func:`~aenum.extend_enum` instead of raising. That is the exception the owner +named, and :class:`ConstEnumRegisterFallbackTests` exists so a future guard +cannot quietly turn a registration into a rejection -- the failure mode GitHub +issues #584 and #623 are both about. + +The guard for the three flag registries delegates in-range composites to +``super()._missing_`` rather than resolving them itself, because GitHub issue +#623 established that an :class:`~aenum.IntFlag` whose ``_missing_`` re-enters +its own constructor recurses without bound. + +""" +from __future__ import annotations + +import enum +import importlib +import importlib.util +import inspect +import pkgutil +import re +import sys +import unittest +from typing import TYPE_CHECKING + +import aenum + +from tests._support import purge_modules + +if TYPE_CHECKING: + from typing import Optional + +#: An integer no wire field in the library is wide enough to carry. Shared with +#: :mod:`tests.const.test_const_enum_get`, which uses the same value for the +#: same reason. +UNRESOLVABLE = 1 << 70 + +#: The six registries GitHub issue #647 converged, as ``(module, class name, +#: what the pre-fix lookup did with an invalid value)``. Named individually as +#: well as swept, so a regression reports *which* registry came apart rather +#: than only that the sweep shrank. +ISSUE_647_OUTLIERS = ( + ('pcapkit.const.tcp.flags', 'Flags', + 'returned 65520, the OR of every declared TCP header flag'), + ('pcapkit.const.ftp.command', 'CommandType', + 'returned 7, the OR of A|P|S'), + ('pcapkit.const.reg.apptype', 'TransportProtocol', + 'returned 15, the OR of tcp|udp|sctp|dccp'), + ('pcapkit.const.ftp.command', 'Command', + "raised AttributeError: 'int' object has no attribute 'upper'"), + ('pcapkit.const.ftp.command', 'FEATCode', + "raised AttributeError: 'int' object has no attribute 'upper'"), + ('pcapkit.const.http.method', 'Method', + "raised AttributeError: 'int' object has no attribute 'upper'"), +) + +#: The registries whose ``_missing_`` ends in :func:`~aenum.extend_enum`, so an +#: unassigned value in their span is *registered* rather than rejected. This is +#: the deliberate divergence from the built-in, not an oversight: their backing +#: registries carry open "Unassigned" spans that a capture can legitimately +#: contain. They still reject a *negative* value, which no span covers. +EXPECTED_TO_REGISTER = frozenset({ + 'pcapkit.const.ipv4.protection_authority.ProtectionAuthority', + 'pcapkit.const.mh.cga_type.CGAType', +}) + +#: The bespoke templates under :mod:`pcapkit.vendor` that carry their own copy of +#: the guard, rather than inheriting the one in :mod:`pcapkit.vendor.default`. +#: Each was rendering a registry that GitHub issue #647 found unguarded. +BESPOKE_TEMPLATES = ( + 'pcapkit.vendor.tcp.flags', + 'pcapkit.vendor.ftp.command', + 'pcapkit.vendor.http.method', + 'pcapkit.vendor.reg.apptype', +) + + +class _StdIntEnum(enum.IntEnum): + """A built-in :class:`enum.IntEnum`, as the reference for what rejection looks like.""" + + A = 0 + + +class _StdStrEnum(str, enum.Enum): + """A built-in string enumeration. + + Spelled ``(str, enum.Enum)`` rather than :class:`enum.StrEnum`, which is + 3.11+, because the CI matrix starts at 3.10. + """ + + A = 'a' + + +def _iter_const_registries() -> 'list[type]': + """Every :class:`~aenum.Enum` subclass defined under :mod:`pcapkit.const`. + + Wider than either sweep in :mod:`tests.const.test_const_enum_lookup` -- + which walks the non-flag :class:`~aenum.IntEnum` classes and the + :class:`~aenum.IntFlag` classes in two separate passes -- and wider than the + one in :mod:`tests.const.test_const_enum_get`, which takes both but not the + :class:`~aenum.StrEnum` registries. Rejecting an invalid value is a contract + all three kinds share, so all three are swept here. + + Keyed on :class:`~aenum.Enum` deliberately: ``issubclass(aenum.IntFlag, + aenum.IntEnum)`` is :data:`False`, because an + :class:`~aenum.IntFlag`'s MRO runs through :class:`~aenum.Flag` rather than + :class:`~aenum.IntEnum`, so a sweep written against + :class:`~aenum.IntEnum` silently skips every flag registry -- which is how + GitHub issue #623 survived the sweep in + :mod:`tests.const.test_const_enum_lookup` and how three of issue #647's six + outliers survived the one in :mod:`tests.const.test_const_enum_get`. + + Collects only classes *defined* in the module being walked, so a + re-exported registry is counted once. + + Returns: + The discovered enum classes, in walk order. + + """ + import pcapkit.const as const_pkg + + classes = [] # type: list[type] + for module_info in pkgutil.walk_packages(const_pkg.__path__, const_pkg.__name__ + '.'): + module = importlib.import_module(module_info.name) + for _, obj in vars(module).items(): + if (inspect.isclass(obj) and issubclass(obj, aenum.Enum) + and obj.__module__ == module_info.name): + classes.append(obj) + return classes + + +def _qualname(obj: 'type') -> 'str': + """The fully qualified name used as a sweep key.""" + return f'{obj.__module__}.{obj.__qualname__}' + + +def _raised_by(obj: 'type', value: 'object') -> 'Optional[type]': + """The exception type ``obj(value)`` raises, or :data:`None` if it returns. + + Args: + obj: Enumeration class to look a value up in. + value: Value to look up. + + Returns: + The exception's type, or :data:`None` when the lookup resolved. + + """ + try: + obj(value) + except BaseException as error: # pylint: disable=broad-except + return type(error) + return None + + +class ConstEnumBuiltinParityTests(unittest.TestCase): + """An invalid value must be rejected with the built-in's own exception.""" + + if TYPE_CHECKING: + enums: 'list[type]' + + @classmethod + def setUpClass(cls) -> None: + purge_modules(['pcapkit']) + cls.enums = _iter_const_registries() + # The out-of-range probe below registers a junk member on the two + # auto-extending registries, which are module-global classes. Purge the + # package afterwards so that pollution cannot reach another module. + cls.addClassCleanup(purge_modules, ['pcapkit']) + + def test_the_sweep_size_is_pinned(self) -> None: + """A registry added or removed needs a fresh look, not a silent pass.""" + flags = [obj for obj in self.enums if issubclass(obj, aenum.Flag)] + strs = [obj for obj in self.enums + if issubclass(obj, str) and not issubclass(obj, aenum.Flag)] + ints = [obj for obj in self.enums + if issubclass(obj, int) and not issubclass(obj, aenum.Flag)] + + # The decomposition is asserted, not just the total, so this sweep stays + # in step with the three narrower ones it overlaps: 111 non-flag IntEnum + # and 7 IntFlag in tests.const.test_const_enum_lookup, and 118 -- their + # sum -- in tests.const.test_const_enum_get. + self.assertEqual(len(ints), 111) + self.assertEqual(len(flags), 7) + self.assertEqual(len(strs), 5) + self.assertEqual(len(self.enums), 123) + self.assertEqual(len({obj.__module__ for obj in self.enums}), 117) + + def test_every_registry_rejects_a_negative_value(self) -> None: + """The registry-wide form of GitHub issue #647. + + No wire field the library parses is signed, so ``-1`` is invalid for + every registry in the tree -- including the mutable ones, whose + "Unassigned" spans are non-negative. + """ + for obj in self.enums: + with self.subTest(enum=_qualname(obj)): + raised = _raised_by(obj, -1) + self.assertIsNotNone( + raised, f'{_qualname(obj)}(-1) resolved instead of raising; ' + f'see GitHub issue #647') + self.assertIs( + raised, ValueError, + f'{_qualname(obj)}(-1) raised {raised.__name__ if raised else None} ' + f'rather than the built-in ValueError; see GitHub issue #647') + + def test_the_exception_type_matches_the_built_in_enum(self) -> None: + """The property GitHub issue #647 settled on, asserted directly. + + A :mod:`pcapkit.const` registry and a built-in :class:`enum.IntEnum` + must raise the *same* exception type for the same invalid value. This is + what rules out the in-library + :class:`~pcapkit.utilities.exceptions.EnumError`, which is + ``(BaseError, TypeError)``: substituting it would leave this assertion + failing on every registry in the tree. + """ + self.assertIs(_raised_by(_StdIntEnum, -1), ValueError) + self.assertIs(_raised_by(_StdStrEnum, -1), ValueError) + + for obj in self.enums: + with self.subTest(enum=_qualname(obj)): + builtin = _StdStrEnum if issubclass(obj, str) else _StdIntEnum + self.assertIs(_raised_by(obj, -1), _raised_by(builtin, -1)) + + def test_the_exception_is_not_an_in_library_one(self) -> None: + """Explicitly *not* ``EnumError``, and explicitly not loud. + + :class:`~pcapkit.utilities.exceptions.BaseError` logs at CRITICAL from + its constructor, so it fires even for an exception that is caught and + discarded -- which the ``except ValueError`` fallback in every generated + ``get()`` does on purpose. A guard raising a + :class:`~pcapkit.utilities.exceptions.BaseError` subclass would log once + per discarded default. + """ + from pcapkit.utilities.exceptions import BaseError, EnumError + + # The reason EnumError is wrong here, asserted rather than asserted-in-prose. + self.assertTrue(issubclass(EnumError, TypeError)) + self.assertFalse(issubclass(EnumError, ValueError)) + + for obj in self.enums: + with self.subTest(enum=_qualname(obj)): + with self.assertRaises(ValueError) as caught: + obj(-1) + self.assertNotIsInstance(caught.exception, BaseError) + + def test_an_out_of_range_value_is_rejected_unless_the_registry_registers_it(self) -> None: + """A value past the registry's own width, which the guards bound.""" + for obj in self.enums: + qualname = _qualname(obj) + with self.subTest(enum=qualname): + raised = _raised_by(obj, UNRESOLVABLE) + if qualname in EXPECTED_TO_REGISTER: + self.assertIsNone( + raised, f'{qualname} no longer registers an unassigned value; ' + f'see GitHub issues #584 and #647') + else: + self.assertIs(raised, ValueError, f'{qualname}({UNRESOLVABLE}) raised ' + f'{raised.__name__ if raised else None}') + + def test_the_registries_named_in_issue_647(self) -> None: + """The six outliers, by name, with what each used to do.""" + for module_name, class_name, before in ISSUE_647_OUTLIERS: + with self.subTest(enum=f'{module_name}.{class_name}', before=before): + obj = getattr(importlib.import_module(module_name), class_name) + + with self.assertRaises(ValueError) as caught: + obj(-1) + self.assertIn(class_name, str(caught.exception)) + self.assertIs(type(caught.exception), ValueError) + + def test_every_new_guard_is_a_classmethod(self) -> None: + """``_missing_`` without ``@classmethod`` raises :exc:`TypeError` for everything. + + ``aenum`` invokes a failed lookup as ``cls._missing_(value)``, so a plain + function binds ``value`` to ``cls`` and leaves ``value`` unfilled. GitHub + issue #492 found two registries like that and + ``ConstMissingClassmethodTests`` in + :mod:`tests.const.test_const_enum_lookup` pins the fix -- but its sweep + is ``_iter_const_int_enums``, which excludes :class:`~aenum.IntFlag`, and + it never sees a :class:`~aenum.StrEnum` either. Every guard GitHub issue + #647 added is on one of those two kinds, so all six would have been + outside it. + """ + checked = 0 + offenders = [] # type: list[str] + for obj in self.enums: + if '_missing_' not in vars(obj): + continue + checked += 1 + if not isinstance(vars(obj)['_missing_'], classmethod): + offenders.append(_qualname(obj)) + + self.assertGreater(checked, 0, 'sweep found no _missing_ overrides at all') + self.assertEqual(offenders, [], + f'_missing_ without @classmethod (GitHub issue #492): {offenders}') + + # The six of issue #647 specifically, since the sweep above would still + # pass if they had no ``_missing_`` at all -- which was the defect. + for module_name, class_name, _ in ISSUE_647_OUTLIERS: + obj = getattr(importlib.import_module(module_name), class_name) + with self.subTest(enum=f'{module_name}.{class_name}'): + self.assertIn('_missing_', vars(obj), + f'{module_name}.{class_name} defines no _missing_; ' + f'see GitHub issue #647') + self.assertIsInstance(vars(obj)['_missing_'], classmethod) + + def test_a_string_registry_rejects_a_non_string_rather_than_crashing(self) -> None: + """The three ``StrEnum`` outliers reached ``value.upper()`` unguarded.""" + from pcapkit.const.ftp.command import Command, FEATCode + from pcapkit.const.http.method import Method + + for obj in (Command, FEATCode, Method): + for value in (-1, 0, 1.5, b'GET', None): + with self.subTest(enum=_qualname(obj), value=value): + raised = _raised_by(obj, value) + self.assertIs(raised, ValueError, + f'{_qualname(obj)}({value!r}) raised ' + f'{raised.__name__ if raised else None}') + + +class ConstFlagCompositeTests(unittest.TestCase): + """A bounded flag registry must still compose the values it does define. + + The guard rejects out-of-range values and hands everything else to + ``super()._missing_``. Getting that wrong in the other direction -- a guard + narrow enough to reject a legitimate composite -- would break parsing for + every packet with more than one flag set, and is exactly what the + ``FLAG = 'isinstance(value, int) and 4 <= value <= 15'`` checker in + :mod:`pcapkit.vendor.tcp.flags` would have done had the template ever + interpolated it: those are *bit offsets*, and the values they generate are + ``1 << offset``. + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_tcp_flag_composites_resolve(self) -> None: + from pcapkit.const.tcp.flags import Flags + + self.assertEqual(int(Flags(0)), 0) + for first, second in (('PSH', 'ACK'), ('SYN', 'FIN'), ('RST', 'URG')): + composite = int(Flags[first]) | int(Flags[second]) + with self.subTest(composite=hex(composite)): + self.assertNotIn(composite, {member.value for member in Flags}) + self.assertEqual(int(Flags(composite)), composite) + self.assertEqual(Flags(composite), Flags[first] | Flags[second]) + + # Every declared bit at once is the widest legitimate value, and it is + # what Flags(-1) used to return. + every = 0 + for member in Flags: + every |= int(member) + self.assertEqual(every, 0xFFF0) + self.assertEqual(int(Flags(every)), every) + + # One past the 16-bit field the registry indexes bits of. + for beyond in (0x10000, -1, -0x10000, UNRESOLVABLE): + with self.subTest(value=beyond): + with self.assertRaises(ValueError): + Flags(beyond) + + # An unassigned low bit is in range and still composes, rather than + # being rejected: bits 0-3 of that field are the data offset. + self.assertEqual(int(Flags(1)), 1) + + def test_the_other_two_flag_registries_compose(self) -> None: + from pcapkit.const.ftp.command import CommandType + from pcapkit.const.reg.apptype import TransportProtocol + + self.assertEqual(int(CommandType(0)), 0) + self.assertEqual(CommandType(0x07), CommandType.A | CommandType.P | CommandType.S) + with self.assertRaises(ValueError): + CommandType(0x08) + + self.assertEqual(int(TransportProtocol(0)), 0) + self.assertEqual(TransportProtocol(0x0F), + TransportProtocol.tcp | TransportProtocol.udp + | TransportProtocol.sctp | TransportProtocol.dccp) + with self.assertRaises(ValueError): + TransportProtocol(0x10) + + @unittest.skipIf(sys.version_info < (3, 11), 'enum.STRICT is 3.11+') + def test_a_bounded_flag_matches_the_built_in_under_its_strict_boundary(self) -> None: + """Where the built-in flag parity claim actually lands. + + :class:`enum.IntFlag` defaults to ``boundary=KEEP`` and *composes* an + out-of-range value rather than raising, so the parity being asserted for + the flag registries is against the built-in's own reject mode, + ``boundary=STRICT``. The exception type is the same either way, which is + the property GitHub issue #647 is about; this test records that the + choice to bound the domain at all is the built-in's ``STRICT`` + behaviour and not an invention. + """ + from pcapkit.const.tcp.flags import Flags + + strict = enum.IntFlag('strict', {'FIN': 1 << 15}, boundary=enum.STRICT) # type: ignore[misc] + keep = enum.IntFlag('keep', {'FIN': 1 << 15}, boundary=enum.KEEP) # type: ignore[misc] + + self.assertIs(_raised_by(strict, -1), ValueError) + self.assertIs(_raised_by(Flags, -1), _raised_by(strict, -1)) + + # And the default the library deliberately does not follow. + self.assertIsNone(_raised_by(keep, -1)) + + +class ConstEnumRegisterFallbackTests(unittest.TestCase): + """The one sanctioned divergence: look up, miss, then register. + + In the owner's words, the const enums mirror the built-in "with one + exception: they contain the missing then register fallback (mutable enums)". + A guard that turned a registration into a rejection would be the regression + GitHub issues #584 and #623 are both about, so it is pinned here rather than + left to the guard tests above to imply. + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def tearDown(self) -> None: + # Every test here registers members on module-global classes. + purge_modules(['pcapkit']) + + def test_a_string_registry_still_registers_an_unknown_name(self) -> None: + from pcapkit.const.ftp.command import Command, FEATCode + from pcapkit.const.http.method import Method + + for obj, unknown in ((Method, 'FROBNICATE'), (Command, 'XYZZY'), (FEATCode, '')): + with self.subTest(enum=_qualname(obj), value=unknown): + before = len(obj.__members__) + registered = obj(unknown) + self.assertGreater(len(obj.__members__), before, + f'{_qualname(obj)}({unknown!r}) did not register; ' + f'see GitHub issue #647') + self.assertIs(obj(unknown), registered) + + def test_a_string_registry_still_matches_case_insensitively(self) -> None: + from pcapkit.const.ftp.command import Command + from pcapkit.const.http.method import Method + + for obj, lowered, member in ((Method, 'get', 'GET'), (Command, 'abor', 'ABOR')): + with self.subTest(enum=_qualname(obj), value=lowered): + before = len(obj.__members__) + self.assertIs(obj(lowered), obj[member]) + self.assertEqual(len(obj.__members__), before, + 'a case-insensitive hit must not register a new member') + + def test_the_guard_leaves_the_other_lookup_paths_alone(self) -> None: + """``get()`` is where the guard's rejection is caught, so it is checked too. + + The string path of a generated ``get`` never reaches ``_missing_`` at all, + and the integer path reaches it inside a ``try``. Both are exercised here + for the four modules GitHub issue #647 touched, so a guard that had + broken either would fail rather than merely go unmeasured. + """ + from pcapkit.const.ftp.command import Command + from pcapkit.const.http.method import Method + from pcapkit.const.reg.apptype import TransportProtocol + from pcapkit.const.tcp.flags import Flags + + # String paths, which bypass ``_missing_``. + self.assertIs(Flags.get('SYN'), Flags.SYN) + self.assertIs(Command.get('abor'), Command.ABOR) + self.assertIs(Method.get('get'), Method.GET) + self.assertIs(TransportProtocol.get('tcp'), TransportProtocol.tcp) + + # Integer paths, which do. + self.assertIs(TransportProtocol.get(1), TransportProtocol.tcp) + self.assertIs(Flags.get(1 << 14), Flags.SYN) + + # And the fallback the guard's ValueError is what triggers: GitHub issue + # #584's ``get(key, default)``, which an EnumError would have walked past. + self.assertEqual(int(Flags.get(UNRESOLVABLE, 0)), 0) + with self.assertRaises(ValueError): + Flags.get(UNRESOLVABLE) + + def test_the_auto_extending_integer_registries_still_extend(self) -> None: + from pcapkit.const.ipv4.protection_authority import ProtectionAuthority + from pcapkit.const.mh.cga_type import CGAType + + for obj in (ProtectionAuthority, CGAType): + with self.subTest(enum=_qualname(obj)): + before = len(obj.__members__) + registered = obj(UNRESOLVABLE) + self.assertGreater(len(obj.__members__), before) + self.assertEqual(int(registered), UNRESOLVABLE) + + # ... and still reject a value no span covers. + with self.assertRaises(ValueError): + obj(-1) + + def test_apptype_still_registers_an_unassigned_port(self) -> None: + """GitHub issue #584's machinery, which runs through ``_missing_``.""" + from pcapkit.const.reg.apptype import AppType, TransportProtocol + + registered = AppType.get(65000, proto=TransportProtocol.tcp) + self.assertEqual(int(registered), 65000) + self.assertIs(AppType.get(65000, proto=TransportProtocol.tcp), registered) + + def test_transport_protocol_can_still_be_extended_at_runtime(self) -> None: + """Why this registry's bound is derived rather than written down. + + ``TransportProtocol.get`` registers an unknown protocol name at + ``max * 2``, so a literal upper bound -- the shape the Mobility Header + flag guards use -- would reject the very member the registry had just + grown, and every composite containing it. The guard reads the bound off + the current members instead, and this test is what pins that: it fails + against a hard-coded ``0x0F``. + """ + from pcapkit.const.reg.apptype import TransportProtocol + + self.assertNotIn('quic', TransportProtocol.__members__) + with self.assertRaises(ValueError): + TransportProtocol(0x10) + + grown = TransportProtocol.get('quic') + self.assertEqual(int(grown), 0x10) + self.assertIn('quic', TransportProtocol.__members__) + + # The new member composes with the old ones, which is the assertion a + # literal bound fails. + self.assertEqual(int(TransportProtocol(0x11)), 0x11) + self.assertEqual(TransportProtocol(0x11), grown | TransportProtocol.tcp) + + # And the bound moved with it rather than disappearing. + with self.assertRaises(ValueError): + TransportProtocol(0x20) + + +class ConstEnumGuardTemplateTests(unittest.TestCase): + """A regeneration must not undo the guard. + + :mod:`pcapkit.const` is generated from :mod:`pcapkit.vendor`, so the + committed tree passing the tests above is not evidence that the templates + agree with it -- the next crawl would simply revert them. The four templates + below each carry their own copy of the guard rather than inheriting the one + in :mod:`pcapkit.vendor.default`, which is why all four had to be edited and + why all four are checked. + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + @unittest.skipUnless(importlib.util.find_spec('requests') is not None, + 'pcapkit.vendor needs requests') + def test_every_bespoke_template_carries_the_guard(self) -> None: + for module_name in BESPOKE_TEMPLATES: + with self.subTest(vendor=module_name): + source = inspect.getsource(importlib.import_module(module_name)) + self.assertIn( + "raise ValueError('%r is not a valid %s' % (value, cls.__name__))", + source, f'{module_name} no longer emits the guard; ' + f'see GitHub issue #647') + + @unittest.skipUnless(importlib.util.find_spec('requests') is not None, + 'pcapkit.vendor needs requests') + def test_the_tcp_flags_template_renders_the_committed_module(self) -> None: + """The whole file, character for character. + + Mirrors ``test_the_vendor_templates_still_emit_the_fix`` in + :mod:`tests.const.test_const_enum_lookup`, which does this for the four + Mobility Header templates. Needs no network: the crawl supplies only the + enumeration block, which is read back out of the committed module. + """ + import pathlib + + from tests.const.test_const_enum_lookup import _normalize + + vendor_module = importlib.import_module('pcapkit.vendor.tcp.flags') + vendor_class = vendor_module.Flags + + const_module = importlib.import_module('pcapkit.const.tcp.flags') + committed = pathlib.Path( + const_module.__file__ # type: ignore[arg-type] + ).read_text(encoding='utf-8') + + block = re.compile( + r'class \w+\(IntFlag\):\n """.*?"""\n\n (.*?)\n\n @staticmethod', re.S) + enum_block = block.search(committed) + self.assertIsNotNone(enum_block, 'no enumeration block in pcapkit.const.tcp.flags') + + rendered = _normalize(vendor_module.LINE( + vendor_class.__name__, vendor_class.__doc__, vendor_class.FLAG, + enum_block.group(1), # type: ignore[union-attr] + 'pcapkit.vendor.tcp.flags', + )) + + self.assertIn('return super()._missing_(value)', rendered) + self.assertEqual(rendered, committed) + + @unittest.skipUnless(importlib.util.find_spec('requests') is not None, + 'pcapkit.vendor needs requests') + def test_the_tcp_flags_checker_covers_the_value_domain_not_the_bit_offsets(self) -> None: + """The checker the template used to drop, and why it could not be kept. + + ``Flags.FLAG`` read ``4 <= value <= 15`` -- the *offsets* the IANA + registry indexes -- while the members it generates are ``1 << offset``. + The template never interpolated it, so it was never wrong in practice; + interpolating it unchanged would have rejected every composite, every + single member above bit 3, and ``Flags(0)``. + """ + from pcapkit.vendor.tcp.flags import Flags as VendorFlags + + checker = VendorFlags.FLAG + self.assertIn('0xFFFF', checker) + + from pcapkit.const.tcp.flags import Flags + + for member in Flags: + value = int(member) + with self.subTest(member=member.name): + self.assertTrue(eval(checker, {'value': value}), # pylint: disable=eval-used + f'the checker rejects its own member {member.name}={value}') + self.assertFalse(eval('isinstance(value, int) and 4 <= value <= 15', + {'value': value}), # pylint: disable=eval-used + f'the old checker accepted {member.name}={value}') + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/const/test_const_enum_get.py b/tests/const/test_const_enum_get.py index 26a65126a7..b17cc788e7 100644 --- a/tests/const/test_const_enum_get.py +++ b/tests/const/test_const_enum_get.py @@ -20,6 +20,11 @@ remaining eight never raise on an out-of-range integer because they auto-extend the whole space, and five carry no ``get(key, default)`` at all. +GitHub issue #647 moved one of those three into the sweep, so the arithmetic is +now 111 + 2 + 5. :class:`~pcapkit.const.tcp.flags.Flags` resolved every integer +only because it defined no ``_missing_`` to bound its domain; it does now, so its +``get`` has a failure for ``default`` to fall back from like the other 110. + Two more registries are outside this sweep because they are :class:`~aenum.StrEnum` rather than integer enums, and both were deliberately left alone: :class:`~pcapkit.const.pcapng.option_type.OptionType` and @@ -66,12 +71,18 @@ UNRESOLVABLE = 1 << 70 #: Enums whose integer path resolves *anything* rather than raising, so there is -#: no fallback for ``default`` to supply: all three auto-extend their unassigned -#: spans across the full integer range. +#: no fallback for ``default`` to supply: both auto-extend their unassigned spans +#: across the full integer range. +#: +#: :class:`~pcapkit.const.tcp.flags.Flags` was a third entry until GitHub issue +#: #647. It resolved anything for a different reason -- it defined no +#: ``_missing_`` at all, so ``aenum`` composed a pseudo-member for any integer, +#: and ``Flags(-1)`` read back as every TCP header flag set at once. It now +#: bounds its own domain like the rest of the tree and so belongs in the sweep +#: below rather than in this set. EXPECTED_TO_RESOLVE_ANYTHING = frozenset({ 'pcapkit.const.ipv4.protection_authority.ProtectionAuthority', 'pcapkit.const.mh.cga_type.CGAType', - 'pcapkit.const.tcp.flags.Flags', }) #: Enums carrying no ``get(key, default)``, so there is no ``default`` to drop. @@ -212,7 +223,7 @@ def test_every_integer_path_consults_the_default(self) -> None: if qualname in EXPECTED_TO_RESOLVE_ANYTHING: # Covered by its own test below. Probing them here would extend # a module-global registry as a side effect of a sweep whose - # subject is something else, and for these three the ``try`` + # subject is something else, and for these two the ``try`` # never raises, so it would assert nothing about the fix. continue with self.subTest(enum=qualname): @@ -221,27 +232,31 @@ def test_every_integer_path_consults_the_default(self) -> None: fallback = next(iter(obj)).value self.assertIs(obj.get(UNRESOLVABLE, fallback), obj(fallback)) covered += 1 - self.assertEqual(covered, 110) + # 111 rather than 110 since GitHub issue #647 gave + # :class:`~pcapkit.const.tcp.flags.Flags` a range guard, which moved it + # out of ``EXPECTED_TO_RESOLVE_ANYTHING`` and into this sweep. + self.assertEqual(covered, 111) def test_the_always_resolving_registries_have_nothing_to_fall_back_to(self) -> None: - """The three registries excused from the sweep, and why. + """The two registries excused from the sweep, and why. Their ``_missing_`` extends for *any* integer, so the integer path never raises and ``default`` has nothing to supply. Asserted rather than merely listed, so ``EXPECTED_TO_RESOLVE_ANYTHING`` cannot quietly grow to hide a registry that does raise. - Kept out of the sweep because probing the two :class:`~aenum.IntEnum` - ones *mutates* the registry: the call permanently registers a member on - a module-global class. That is done deliberately here, and ``setUpClass`` - registers a class cleanup that purges :mod:`pcapkit` afterwards so the - pollution cannot reach another module. + Kept out of the sweep because probing them *mutates* the registry: the + call permanently registers a member on a module-global class. That is + done deliberately here, and ``setUpClass`` registers a class cleanup that + purges :mod:`pcapkit` afterwards so the pollution cannot reach another + module. Measured: ``ProtectionAuthority`` grows 8 members to 9 and ``CGAType`` 7 - to 8, because their ``_missing_`` calls ``extend_enum``. ``Flags`` does - not grow at all -- it is an :class:`~aenum.IntFlag` and returns a - pseudo-member instead -- which is why the shared assertion below is - "resolves", not "extends". + to 8, because their ``_missing_`` calls ``extend_enum``. The shared + assertion below is still "resolves" rather than "extends" because that is + the property excusing them, and it is the property + :class:`~pcapkit.const.tcp.flags.Flags` stopped having in GitHub issue + #647 -- it resolved without extending, and now does neither. """ for qualname in sorted(EXPECTED_TO_RESOLVE_ANYTHING): module_name, _, class_name = qualname.rpartition('.') @@ -253,7 +268,7 @@ def test_the_always_resolving_registries_have_nothing_to_fall_back_to(self) -> N self.assertIsNotNone(resolved) self.assertEqual(int(resolved), UNRESOLVABLE) # It resolves with or without a default, so the sentinel branch - # this change added is never reached for these three. + # this change added is never reached for these two. self.assertIs(resolved, obj.get(UNRESOLVABLE, 0)) @unittest.skipUnless(importlib.util.find_spec('requests') is not None, diff --git a/tests/const/test_const_enum_lookup.py b/tests/const/test_const_enum_lookup.py index 7d46e3560a..04a95cabd1 100644 --- a/tests/const/test_const_enum_lookup.py +++ b/tests/const/test_const_enum_lookup.py @@ -29,9 +29,14 @@ was ``0`` -- no flags set -- and every composite of two defined bits. Overriding ``_missing_`` at all is what caused it: the override shadowed the ``aenum`` :class:`~aenum.Flag` machinery that resolves precisely those values, which is why -:class:`~pcapkit.const.tcp.flags.Flags`, which defines no ``_missing_``, never -had the defect. :class:`~pcapkit.const.mh.binding_ack_flag.BindingACKFlag` and -its three siblings now end in ``return super()._missing_(value)``, the tail +:class:`~pcapkit.const.tcp.flags.Flags`, which at the time defined no +``_missing_``, never had the defect -- and, as GitHub issue #647 then found, had +no range guard either, so it resolved *any* integer and ``Flags(-1)`` read back +as every TCP header flag set at once. It now carries the same guard ending in +``return super()._missing_(value)``, which is what keeps issue #623 fixed while +issue #647 is too; :mod:`tests.const.test_const_enum_builtin_parity` is where +that lands. :class:`~pcapkit.const.mh.binding_ack_flag.BindingACKFlag` and its +three siblings end in that same tail, the one :mod:`pcapkit.vendor.default` emits for every other generated enumeration. So this module carries a companion sweep over the :class:`~aenum.IntFlag` classes, covering the contract the :class:`~aenum.IntEnum` sweep declines to.