diff --git a/docs/source/pcapkit/utilities/exceptions.rst b/docs/source/pcapkit/utilities/exceptions.rst index 9fbdac27f5..ce485322b4 100644 --- a/docs/source/pcapkit/utilities/exceptions.rst +++ b/docs/source/pcapkit/utilities/exceptions.rst @@ -260,6 +260,10 @@ It is still an ordinary exception carrying its message, so ``except`` clauses an :no-members: :show-inheritance: +.. autoexception:: pcapkit.utilities.exceptions.EnumKeyError + :no-members: + :show-inheritance: + :exc:`ModuleNotFoundError` Category ----------------------------------- diff --git a/pcapkit/const/reg/apptype/apptype.py b/pcapkit/const/reg/apptype/apptype.py index d07c1d3bf8..54bc17477c 100644 --- a/pcapkit/const/reg/apptype/apptype.py +++ b/pcapkit/const/reg/apptype/apptype.py @@ -92,22 +92,33 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco """Backport support for original codes. Delegates to :meth:`~pcapkit.corekit.enum.EnumLookup.get` for GitHub - issue #877's re-parenting, but keeps this override rather than - dropping it -- two behaviours the base does not reproduce on its own: + issue #877's re-parenting, but keeps this override rather than dropping + it, for one behaviour the base does not reproduce on its own: **case + folding**. This class has always matched a name case-insensitively + (``key.lower()``); the base's own ``str`` branch is case-sensitive. + Lowering ``key`` before delegating reproduces that: every member name + here is already lower-case, so a lowered ``key`` still hits the base's + exact ``_member_map_`` lookup. - * **Case folding.** This class has always matched a name - case-insensitively (``key.lower()``); the base's own ``str`` - branch is case-sensitive. Lowering ``key`` before delegating - reproduces that: every member name here is already lower-case, so - a lowered ``key`` still hits the base's exact ``_member_map_`` - lookup. - * **The refusal.** Maintainer ruling on GitHub PR #836: "Do not - allow extension of TransportProtocol at all." The base's own miss - on a ``str`` key raises a bare :exc:`KeyError`; this class has - always raised :exc:`ValueError` naming the rejected key, which is - what every caller and test here already depends on, so a name - miss is caught and re-raised in that shape rather than left as the - base's own exception. + Case folding is now the *only* thing this override adds. It used to + convert the base's name-miss exception as well -- this class raised + :exc:`ValueError` where the base raised :exc:`KeyError` -- and GitHub + issue #923's ruling retired that conversion: *"Either ``ValueError`` + or ``KeyError``, that's depending on how stdlib's ``Enum`` would raise + on these circumstances."* A stdlib ``E['nosuch']`` raises + :exc:`KeyError`, and #923's census of the 127 concrete + :class:`~pcapkit.corekit.enum.EnumLookup` subclasses -- taken before + #921 re-parented this class, so this class is not among them -- found + 119 already answering a name miss that way against 5 answering with + :exc:`ValueError`. Those 5 are :class:`AppType` and its four transport + registries, and they land there only because their own ``get()`` takes + an :class:`int` port and never accepts a name at all, rather than from + any name-miss policy. So there was no policy here to preserve, and a + name miss now reaches the caller as + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` from the base. + Maintainer ruling on GitHub PR #836 -- "Do not allow extension of + TransportProtocol at all" -- is untouched by that: the refusal is still + a refusal and still mints nothing, only its exception class moved. The base is a :class:`classmethod` (:meth:`~pcapkit.corekit.enum.EnumLookup.get`), so this override @@ -141,16 +152,17 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco :meth:`~pcapkit.corekit.enum.EnumLookup.get`. Raises: - ValueError: If ``key`` names no member, by name or by value, and - there is no usable ``default``. + EnumKeyError: If ``key`` names no member and there is no usable + ``default``. A :exc:`KeyError`, from the base, since GitHub + issue #923 -- it used to be a plain :exc:`ValueError` raised + here. + EnumValueError: If ``key`` is a value no member carries and there + is no usable ``default``. A :exc:`ValueError`, from the base. :meta private: """ if isinstance(key, str): - try: - return super().get(key.lower(), default) - except KeyError: - raise ValueError(f'{key!r} is not a valid {cls.__name__}') from None + return super().get(key.lower(), default) # NOTE: maintainer ruling on this PR (#836): "Do not allow extension # of TransportProtocol at all." A name that is not a declared member # used to mint a brand-new one here, at ``max_val + 1`` (before that, @@ -171,7 +183,9 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco # # NOTE: the delegation below is exception-compatible for the keys this # signature admits -- an unrecognised :class:`int` still reaches the - # caller as the same plain :exc:`ValueError`. It is not compatible for + # caller as a :exc:`ValueError`, now the base's own + # :exc:`~pcapkit.utilities.exceptions.EnumValueError` since GitHub issue + # #923 rather than :mod:`aenum`'s bare one. It is not compatible for # keys outside it: ``None``, a :class:`float` and an unhashable key # used to raise :exc:`AttributeError` from the ``key.lower()`` this # branch no longer reaches, and now raise :exc:`ValueError` (or, for a diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index d7e513ce61..aa7d6db21e 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -91,6 +91,7 @@ from aenum import extend_enum from pcapkit.corekit.sentinels import NO_DEFAULT, NoDefaultType # pylint: disable=unused-import +from pcapkit.utilities.exceptions import BaseError, EnumKeyError, EnumValueError if TYPE_CHECKING: from typing import Any @@ -190,7 +191,12 @@ def _validate_value(cls, value: 'Any') -> 'None': :meth:`get`'s own ``except ValueError`` and falls back to ``default`` just as any other unresolvable value does. An override raising something outside that hierarchy would instead propagate past ``default``, which is - a real difference in behaviour rather than a stylistic preference. + a real difference in behaviour rather than a stylistic preference. With + no usable ``default``, a rejection from this hook reaches the caller + exactly as the override raised it -- :meth:`get` re-raises an in-library + ``ValueError`` unchanged rather than re-wrapping it, so the override's own + message and the single log record it already emitted are what the caller + sees. Called from exactly two places, and the omissions are deliberate: @@ -330,6 +336,46 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': on the same terms; the ``str`` path does not call the hook, for the reason given on :meth:`_validate_value` itself. + Both failure paths raise from :mod:`pcapkit.utilities.exceptions` rather + than a builtin, per the owner's ruling on GitHub issue #923: *"Either + ``ValueError`` or ``KeyError``, that's depending on how stdlib's + ``Enum`` would raise on these circumstances. And we should raise one + from ``pcapkit.utilities.exceptions`` rather builtin exceptions."* The + *shape* is unchanged by that ruling and deliberately so -- a name miss + stays :exc:`KeyError`-derived and a value miss :exc:`ValueError`-derived, + matching ``E['nosuch']`` and ``E(999)`` on a stdlib + :class:`~enum.Enum`, and matching the 119 of this tree's 127 concrete + subclasses that already answered a name miss that way. Only the + provenance changed, so every ``except KeyError`` and ``except + ValueError`` around a call to this method keeps catching. + + Two details of that conversion are worth stating, since neither is + visible from the exception type alone: + + * **The name miss is raised quietly** -- + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` with ``quiet=True``, + so nothing is logged and :data:`sys.tracebacklimit` is left alone. + That is not a cosmetic choice: this method's name miss is in-library + control flow at six call sites, and at + :meth:`~pcapkit.const.http.method.Method.get` it is part of a + *successful* call -- that override catches it in order to mint. A loud + error there would put a :data:`logging.CRITICAL` record on every such + call and set :data:`sys.tracebacklimit` to ``0`` process-wide, which + is exactly the GitHub issue #362 defect + :class:`~pcapkit.utilities.exceptions.BaseError` documents ``quiet`` + for. The value miss takes no such fallback anywhere in this tree, so + it stays loud. + * **An in-library rejection propagates unchanged.** A ``ValueError`` + that is already a :exc:`~pcapkit.utilities.exceptions.BaseError` -- + typically :exc:`~pcapkit.utilities.exceptions.EnumValueError` from a + subclass's :meth:`_validate_value` -- is re-raised as it stands rather + than wrapped, so the subclass's own message survives and the error is + logged once instead of twice. Only :mod:`aenum`'s and :mod:`enum`'s + own "no member carries this value" is converted. This is the same + discrimination :meth:`EnumField.post_process + ` already + makes for the same reason. + Args: key: Name or value to look up. default: An already-registered value to fall back to when @@ -344,13 +390,15 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': The canonical member for ``key``, or for ``default``. Raises: - ValueError: If a value does not resolve and there is no usable - default -- including a value a subclass's - :meth:`_validate_value` rejects, since - :exc:`~pcapkit.utilities.exceptions.EnumValueError` is a - :exc:`ValueError`. - KeyError: If a name does not resolve and there is no usable - default. + EnumValueError: If a value does not resolve and there is no usable + default. Also what a subclass's :meth:`_validate_value` + rejection reaches the caller as, since that hook is documented + to raise this very class and it is passed through rather than + re-wrapped. A :exc:`ValueError`, so an + ``except ValueError`` caller is unaffected. + EnumKeyError: If a name does not resolve and there is no usable + default. A :exc:`KeyError`, so an ``except KeyError`` caller is + unaffected. """ if isinstance(key, str): @@ -360,14 +408,17 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': if key in cls._value2member_map_: return cls._value2member_map_[key] if default is NO_DEFAULT or default not in cls._value2member_map_: - raise + raise EnumKeyError(f'{key!r} is not a valid {cls.__name__}', + quiet=True) from None return cls._value2member_map_[default] try: cls._validate_value(key) return cls(key) # type: ignore[call-arg] - except ValueError: + except ValueError as error: if default is NO_DEFAULT or default not in cls._value2member_map_: - raise + if isinstance(error, BaseError): + raise + raise EnumValueError(str(error)) from error return cls._value2member_map_[default] @classmethod @@ -391,8 +442,8 @@ def get_all(cls, key: 'Any') -> 'tuple[Self, ...]': carrying the same value. Raises: - ValueError: As :meth:`get` with no default, for a value. - KeyError: As :meth:`get` with no default, for a name. + EnumValueError: As :meth:`get` with no default, for a value. + EnumKeyError: As :meth:`get` with no default, for a name. """ canonical = cls.get(key) diff --git a/pcapkit/protocols/application/ngap.py b/pcapkit/protocols/application/ngap.py index 92d071278f..7b7d704eac 100644 --- a/pcapkit/protocols/application/ngap.py +++ b/pcapkit/protocols/application/ngap.py @@ -104,7 +104,7 @@ from pcapkit.const.ngap.procedure_code import ProcedureCode as Enum_ProcedureCode from pcapkit.const.ngap.protocol_ie import ProtocolIE as Enum_ProtocolIE -from pcapkit.corekit.enum import NO_DEFAULT, EnumLookup +from pcapkit.corekit.enum import EnumLookup from pcapkit.corekit.infoclass import Info from pcapkit.protocols.application.application import Application from pcapkit.protocols.data.application.ngap import IE as Data_IE @@ -219,9 +219,30 @@ class Criticality(EnumLookup, IntEnum): """[Criticality] What a receiver must do with an IE it does not understand. Members are named for the ASN.1 identifiers rather than upper-cased, so - that :meth:`Criticality.get` resolves a name decoded by |pycrate|_ through - the standard member map. The values are the ``ENUMERATED`` indices, which - is what goes on the wire. + that :meth:`~pcapkit.corekit.enum.EnumLookup.get` resolves a name decoded + by |pycrate|_ through the standard member map. The values are the + ``ENUMERATED`` indices, which is what goes on the wire. + + Carries no ``get`` of its own. GitHub issue #877 re-parented this class onto + :class:`~pcapkit.corekit.enum.EnumLookup` and kept a delegating override for + one reason only: it converted the base's name-miss :exc:`KeyError` into a + :exc:`ValueError`, so that an unknown *name* and an unknown *value* reported + identically. GitHub issue #923's ruling retired that conversion -- a name + miss is :exc:`KeyError`-shaped, exactly as ``E['nosuch']`` is on a stdlib + :class:`~enum.Enum` -- which left the override a pure pass-through, so it + went with it. + + The one thing that could have made the deletion unsafe is ``default`` + handling, and there the inherited base is identical to what the override + forwarded: ``get('nosuch', Criticality.reject)`` answers ``reject`` on both, + and ``get('nosuch', 99)`` raises on both, since a ``default`` naming no + registered value is not honoured. That is also what keeps the deletion safe + on a closed ASN.1 ``ENUMERATED`` with no extension marker -- a member-valued + ``default`` resolves through ``_value2member_map_`` and never through the + constructor, so no path here can mint a fourth value; see :meth:`_missing_`, + which refuses one outright. The only loss is the narrower + ``int | str | Criticality`` key annotation, a typing nicety rather than + behaviour. """ @@ -232,99 +253,6 @@ class Criticality(EnumLookup, IntEnum): #: Ignore the IE, carry on, and report it. notify = 2 - @classmethod - def get(cls, key: 'int | str | Criticality', default: 'Any' = NO_DEFAULT) -> 'Criticality': - """Backport support for original codes. - - Delegates to :meth:`~pcapkit.corekit.enum.EnumLookup.get` for GitHub - issue #877's re-parenting. For every key the signature admits, the - base reproduces the branch this override used to hand-roll: a - ``Criticality`` key is also an :class:`int` (this is an - :class:`~aenum.IntEnum`) and resolves through the base's non-``str`` - path, ``cls(key)``, which -- exactly like the removed - ``isinstance(key, Criticality): return key`` branch -- hands back the - identical, canonical member rather than a new one; a plain - :class:`str` resolves through the base's name lookup, the same - ``Criticality[key]`` this override used to spell directly. - - Outside that signature the two do differ, which is worth stating - rather than leaving for someone to discover. ``cls(key)`` accepts - anything :class:`int` equality accepts, so ``get(1.0)`` now returns - ``Criticality.ignore`` where the removed code raised - :exc:`ValueError`, and an unhashable key raises :exc:`ValueError` - rather than the :exc:`TypeError` the old ``Criticality[key]`` lookup - produced. Both are out of contract, no caller in this tree can reach - them -- the three live call sites pass pycrate-decoded :class:`str` - or :class:`int` -- and the new shape is what every other - :class:`~pcapkit.corekit.enum.EnumLookup` subclass already does, so - this is alignment rather than a regression. - - The one behaviour the base does not reproduce is the exception this - class has always raised for an unresolved name: a bare - :exc:`KeyError` there, versus this class's own :exc:`ValueError` - naming the rejected key -- so a name miss is still caught and - re-raised in that shape. An unresolved *value* is unaffected either - way: it already reaches the caller as :exc:`ValueError`, raised by - :meth:`_missing_` below, on both the removed code path and the - base's. - - The base is a :class:`classmethod` - (:meth:`~pcapkit.corekit.enum.EnumLookup.get`), so this override - moves from :class:`staticmethod` to :class:`classmethod` to - delegate at all -- the same move GitHub issue #908 and #915 made for - :meth:`~pcapkit.const.http.method.Method.get`. Every call site in - this tree calls this method by name; none take it as a bare - callable or introspect ``__func__``, so the switch is not - caller-visible. - - Unlike :meth:`ProcedureCode.get ` - and :meth:`ProtocolIE.get `, - this still never manufactures a member for an in-range value it has - not seen -- ``Criticality`` cannot grow, unlike those two, which - answer with a throwaway, non-registering member (see - :meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member`) for - a registry 3GPP keeps assigning new codes to. It is an ASN.1 - ``ENUMERATED`` with no extension marker, so a fourth value is - unencodable and a lookup for one is a bug rather than a version skew - -- see :meth:`_missing_`. - - ``default`` did not exist on this override before this change -- - the original had no such parameter at all, which is a genuine LSP - violation once this class' ``get`` is a :class:`classmethod` - override of one that has it (mypy's ``[override]`` check catches - exactly this shape). Forwarded verbatim to - :meth:`~pcapkit.corekit.enum.EnumLookup.get` rather than - reimplemented, so it behaves exactly as the base's own ``default`` - does: a fallback to an *already-registered* member, resolved - through ``_value2member_map_`` and never through the constructor, - so passing one still cannot mint a fourth. Every call site in this - tree omits it, so this is purely an added, backward-compatible - capability -- not the "declared, documented and ignored" shape an - earlier revision of this docstring rejected, since it is now - genuinely honoured rather than a parameter that would have to be - silently dropped. - - Args: - key: Key to get enum item. - default: An already-registered value to fall back to when - ``key`` resolves to nothing. :data:`~pcapkit.corekit.enum. - NO_DEFAULT`, the default, means *no default*. - - Returns: - The matching member. - - Raises: - ValueError: If ``key`` names no member and there is no usable - ``default``. Raised for an unknown name as well as an - unknown value, so that the two ways of getting this wrong - do not report differently. - - :meta private: - """ - try: - return super().get(key, default) - except KeyError: - raise ValueError('%r is not a valid %s' % (key, cls.__name__)) from None @classmethod def _missing_(cls, value: 'int') -> 'NoReturn': diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index e69552635c..ba674b73a1 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -487,7 +487,8 @@ UpdateNotificationMessage as Schema_UpdateNotificationMessage from pcapkit.protocols.schema.internet.mh import VendorSpecificOption as Schema_VendorSpecificOption from pcapkit.protocols.schema.schema import Schema -from pcapkit.utilities.exceptions import EnumValueError, ProtocolError, UnsupportedCall +from pcapkit.utilities.exceptions import (EnumKeyError, EnumValueError, ProtocolError, + UnsupportedCall) from pcapkit.utilities.warnings import ProtocolWarning, RegistryWarning, warn if TYPE_CHECKING: @@ -633,12 +634,21 @@ def get(key: 'int | str') -> 'FastBindingAcknowledgmentStatus': The matching member. Raises: - EnumValueError: If ``key`` names no member -- raised for an - unknown name the same way :meth:`_missing_` raises for an - unknown value, so the two ways of getting this wrong do not - report differently. There is no ``default`` parameter: this - enumeration cannot grow, so a default that could only ever be - ignored would be worse than one that is absent. + EnumKeyError: If ``key`` names no member. A :exc:`KeyError`, per the + owner's ruling on GitHub issue #923 -- *"Either ``ValueError`` + or ``KeyError``, that's depending on how stdlib's ``Enum`` would + raise on these circumstances"* -- since a stdlib + ``E['nosuch']`` raises :exc:`KeyError` and only the *value* + miss :meth:`_missing_` reports is :exc:`ValueError`-shaped. This + used to raise :exc:`~pcapkit.utilities.exceptions.EnumValueError` + so that the two ways of getting it wrong reported identically; + that is exactly the conversion #923 rejects, and 119 of this + tree's 127 concrete + :class:`~pcapkit.corekit.enum.EnumLookup` subclasses already + answered a name miss with a :exc:`KeyError`. There is no + ``default`` parameter: this enumeration cannot grow, so a + default that could only ever be ignored would be worse than one + that is absent. """ if isinstance(key, int): @@ -646,8 +656,8 @@ def get(key: 'int | str') -> 'FastBindingAcknowledgmentStatus': try: return FastBindingAcknowledgmentStatus[key] # type: ignore[misc] except KeyError: - raise EnumValueError('%r is not a valid %s' % - (key, FastBindingAcknowledgmentStatus.__name__)) from None + raise EnumKeyError('%r is not a valid %s' % + (key, FastBindingAcknowledgmentStatus.__name__)) from None @classmethod def _missing_(cls, value: 'int') -> 'NoReturn': @@ -725,12 +735,21 @@ def get(key: 'int | str') -> 'IPv6AddressPrefixCode': The matching member. Raises: - EnumValueError: If ``key`` names no member -- raised for an - unknown name the same way :meth:`_missing_` raises for an - unknown value, so the two ways of getting this wrong do not - report differently. There is no ``default`` parameter: this - enumeration cannot grow, so a default that could only ever be - ignored would be worse than one that is absent. + EnumKeyError: If ``key`` names no member. A :exc:`KeyError`, per the + owner's ruling on GitHub issue #923 -- *"Either ``ValueError`` + or ``KeyError``, that's depending on how stdlib's ``Enum`` would + raise on these circumstances"* -- since a stdlib + ``E['nosuch']`` raises :exc:`KeyError` and only the *value* + miss :meth:`_missing_` reports is :exc:`ValueError`-shaped. This + used to raise :exc:`~pcapkit.utilities.exceptions.EnumValueError` + so that the two ways of getting it wrong reported identically; + that is exactly the conversion #923 rejects, and 119 of this + tree's 127 concrete + :class:`~pcapkit.corekit.enum.EnumLookup` subclasses already + answered a name miss with a :exc:`KeyError`. There is no + ``default`` parameter: this enumeration cannot grow, so a + default that could only ever be ignored would be worse than one + that is absent. """ if isinstance(key, int): @@ -738,8 +757,8 @@ def get(key: 'int | str') -> 'IPv6AddressPrefixCode': try: return IPv6AddressPrefixCode[key] # type: ignore[misc] except KeyError: - raise EnumValueError('%r is not a valid %s' % - (key, IPv6AddressPrefixCode.__name__)) from None + raise EnumKeyError('%r is not a valid %s' % + (key, IPv6AddressPrefixCode.__name__)) from None @classmethod def _missing_(cls, value: 'int') -> 'NoReturn': diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index dace8303b1..7c55b35bdf 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -50,6 +50,7 @@ 'StructError', # struct.error 'StreamEOFError', # EOFError 'MissingKeyError', 'FragmentError', 'PacketError', # KeyError + 'EnumKeyError', # KeyError 'ModuleNotFound', # ModuleNotFoundError ] @@ -372,7 +373,14 @@ class NoDefaultValue(BaseError, ValueError): class EnumValueError(BaseError, ValueError): - """No member of a closed enumeration carries this value.""" + """No member of an enumeration carries this value. + + The value-miss half of the pair whose name-miss half is + :exc:`~pcapkit.utilities.exceptions.EnumKeyError`; see that one for the + stdlib :class:`~enum.Enum` shape both follow, and for why the two halves + derive from different builtins. + + """ class FieldValueError(BaseError, ValueError): @@ -468,6 +476,38 @@ class PacketError(BaseError, KeyError): """Invalid packet dict.""" +class EnumKeyError(BaseError, KeyError): + """No member of an enumeration carries this name. + + The name-miss half of the pair whose value-miss half is + :exc:`~pcapkit.utilities.exceptions.EnumValueError`, and the split between + them follows stdlib :class:`~enum.Enum` rather than this package's own + taste: ``E['nosuch']`` raises :exc:`KeyError` and ``E(999)`` raises + :exc:`ValueError`, so a lookup that misses by *name* is + :exc:`KeyError`-derived and one that misses by *value* is + :exc:`ValueError`-derived. The owner's ruling on GitHub issue #923, + verbatim: *"Either ``ValueError`` or ``KeyError``, that's depending on how + stdlib's ``Enum`` would raise on these circumstances. And we should raise + one from ``pcapkit.utilities.exceptions`` rather builtin exceptions."* + + Deriving from :exc:`KeyError` is what makes that ruling cheap to carry out: + :meth:`~pcapkit.corekit.enum.EnumLookup.get` raised a bare builtin + :exc:`KeyError` on a name miss until #923, and six in-library call sites + catch it -- :meth:`~pcapkit.const.http.method.Method.get` catches it in + order to *mint*, so for that one a failed name lookup is part of a + successful call. Every one of them keeps catching, unchanged. + + Note: + Distinct from :exc:`~pcapkit.utilities.exceptions.MissingKeyError`, + which is deliberately not reused here: that one reports an absent + *mapping* key, as :class:`~pcapkit.corekit.multidict.MultiDict` and the + :mod:`pcapkit.toolkit` extractors raise it, and conflating the two + would leave a caller unable to tell a registry that has no such member + from a packet dict that has no such field. + + """ + + ############################################################################## # ModuleNotFoundError session. ############################################################################## diff --git a/pcapkit/vendor/reg/apptype/apptype.py b/pcapkit/vendor/reg/apptype/apptype.py index 221ded5190..56016c60a8 100644 --- a/pcapkit/vendor/reg/apptype/apptype.py +++ b/pcapkit/vendor/reg/apptype/apptype.py @@ -185,22 +185,33 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco """Backport support for original codes. Delegates to :meth:`~pcapkit.corekit.enum.EnumLookup.get` for GitHub - issue #877's re-parenting, but keeps this override rather than - dropping it -- two behaviours the base does not reproduce on its own: - - * **Case folding.** This class has always matched a name - case-insensitively (``key.lower()``); the base's own ``str`` - branch is case-sensitive. Lowering ``key`` before delegating - reproduces that: every member name here is already lower-case, so - a lowered ``key`` still hits the base's exact ``_member_map_`` - lookup. - * **The refusal.** Maintainer ruling on GitHub PR #836: "Do not - allow extension of TransportProtocol at all." The base's own miss - on a ``str`` key raises a bare :exc:`KeyError`; this class has - always raised :exc:`ValueError` naming the rejected key, which is - what every caller and test here already depends on, so a name - miss is caught and re-raised in that shape rather than left as the - base's own exception. + issue #877's re-parenting, but keeps this override rather than dropping + it, for one behaviour the base does not reproduce on its own: **case + folding**. This class has always matched a name case-insensitively + (``key.lower()``); the base's own ``str`` branch is case-sensitive. + Lowering ``key`` before delegating reproduces that: every member name + here is already lower-case, so a lowered ``key`` still hits the base's + exact ``_member_map_`` lookup. + + Case folding is now the *only* thing this override adds. It used to + convert the base's name-miss exception as well -- this class raised + :exc:`ValueError` where the base raised :exc:`KeyError` -- and GitHub + issue #923's ruling retired that conversion: *"Either ``ValueError`` + or ``KeyError``, that's depending on how stdlib's ``Enum`` would raise + on these circumstances."* A stdlib ``E['nosuch']`` raises + :exc:`KeyError`, and #923's census of the 127 concrete + :class:`~pcapkit.corekit.enum.EnumLookup` subclasses -- taken before + #921 re-parented this class, so this class is not among them -- found + 119 already answering a name miss that way against 5 answering with + :exc:`ValueError`. Those 5 are :class:`AppType` and its four transport + registries, and they land there only because their own ``get()`` takes + an :class:`int` port and never accepts a name at all, rather than from + any name-miss policy. So there was no policy here to preserve, and a + name miss now reaches the caller as + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` from the base. + Maintainer ruling on GitHub PR #836 -- "Do not allow extension of + TransportProtocol at all" -- is untouched by that: the refusal is still + a refusal and still mints nothing, only its exception class moved. The base is a :class:`classmethod` (:meth:`~pcapkit.corekit.enum.EnumLookup.get`), so this override @@ -234,16 +245,17 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco :meth:`~pcapkit.corekit.enum.EnumLookup.get`. Raises: - ValueError: If ``key`` names no member, by name or by value, and - there is no usable ``default``. + EnumKeyError: If ``key`` names no member and there is no usable + ``default``. A :exc:`KeyError`, from the base, since GitHub + issue #923 -- it used to be a plain :exc:`ValueError` raised + here. + EnumValueError: If ``key`` is a value no member carries and there + is no usable ``default``. A :exc:`ValueError`, from the base. :meta private: """ if isinstance(key, str): - try: - return super().get(key.lower(), default) - except KeyError: - raise ValueError(f'{{key!r}} is not a valid {{cls.__name__}}') from None + return super().get(key.lower(), default) # NOTE: maintainer ruling on this PR (#836): "Do not allow extension # of TransportProtocol at all." A name that is not a declared member # used to mint a brand-new one here, at ``max_val + 1`` (before that, @@ -264,7 +276,9 @@ def get(cls, key: 'int | str', default: 'Any' = NO_DEFAULT) -> 'TransportProtoco # # NOTE: the delegation below is exception-compatible for the keys this # signature admits -- an unrecognised :class:`int` still reaches the - # caller as the same plain :exc:`ValueError`. It is not compatible for + # caller as a :exc:`ValueError`, now the base's own + # :exc:`~pcapkit.utilities.exceptions.EnumValueError` since GitHub issue + # #923 rather than :mod:`aenum`'s bare one. It is not compatible for # keys outside it: ``None``, a :class:`float` and an unhashable key # used to raise :exc:`AttributeError` from the ``key.lower()`` this # branch no longer reaches, and now raise :exc:`ValueError` (or, for a diff --git a/tests/const/test_const_apptype_split_unit.py b/tests/const/test_const_apptype_split_unit.py index fca0f44bfd..9c25137bc5 100644 --- a/tests/const/test_const_apptype_split_unit.py +++ b/tests/const/test_const_apptype_split_unit.py @@ -1448,11 +1448,14 @@ def test_get_refuses_an_unrecognised_name_rather_than_minting_it(self) -> None: self.assertNotIn('unit_test_836_bogus', TransportProtocol.__members__) before = len(TransportProtocol.__members__) - with self.assertRaises(ValueError) as caught: + # KeyError-shaped since GitHub issue #923 retired this override's + # ``KeyError`` -> ``ValueError`` conversion; the message is unchanged, + # and so is what this test is about -- refused, not minted. + with self.assertRaises(KeyError) as caught: TransportProtocol.get('unit_test_836_bogus') - # Plain ValueError -- not the ProtocolError the old - # show_flag_values-based decoding briefly answered with for a - # minted 9, back before this ruling retired minting entirely. + # Not the ProtocolError the old show_flag_values-based decoding + # briefly answered with for a minted 9, back before this ruling + # retired minting entirely. self.assertNotIsInstance(caught.exception, ProtocolError) self.assertIn('unit_test_836_bogus', str(caught.exception)) self.assertIn('is not a valid', str(caught.exception)) @@ -1462,7 +1465,7 @@ def test_get_refuses_an_unrecognised_name_rather_than_minting_it(self) -> None: # than taking the next integer after a member that was never created. self.assertEqual(len(TransportProtocol.__members__), before) self.assertNotIn('unit_test_836_bogus', TransportProtocol.__members__) - with self.assertRaises(ValueError): + with self.assertRaises(KeyError): TransportProtocol.get('unit_test_836_bogus_two') self.assertEqual(len(TransportProtocol.__members__), before) @@ -1490,7 +1493,9 @@ def test_get_refuses_a_composite_spelled_string(self) -> None: from pcapkit.const.reg.apptype import TransportProtocol self.assertNotIn('tcp|udp', TransportProtocol.__members__) - with self.assertRaises(ValueError) as caught: + # KeyError-shaped since GitHub issue #923; see + # ``test_get_refuses_an_unrecognised_name_rather_than_minting_it``. + with self.assertRaises(KeyError) as caught: TransportProtocol.get('tcp|udp') self.assertIn('tcp|udp', str(caught.exception)) self.assertIn('is not a valid', str(caught.exception)) diff --git a/tests/const/test_const_enum_builtin_parity.py b/tests/const/test_const_enum_builtin_parity.py index d95e926091..03588af7cf 100644 --- a/tests/const/test_const_enum_builtin_parity.py +++ b/tests/const/test_const_enum_builtin_parity.py @@ -872,7 +872,10 @@ def test_transport_protocol_can_no_longer_be_extended_at_runtime(self) -> None: TransportProtocol(9) before = len(TransportProtocol.__members__) - with self.assertRaises(ValueError): + # KeyError, not ValueError, since GitHub issue #923 retired this + # override's ``KeyError`` -> ``ValueError`` conversion. What this test + # is about -- refused rather than registered -- is unchanged. + with self.assertRaises(KeyError): TransportProtocol.get('quic') self.assertNotIn('quic', TransportProtocol.__members__) self.assertEqual(len(TransportProtocol.__members__), before) @@ -880,7 +883,7 @@ def test_transport_protocol_can_no_longer_be_extended_at_runtime(self) -> None: # A second unrecognised name is refused identically -- there is no # ``max + 1`` left to walk to, since nothing registers in the first # place. - with self.assertRaises(ValueError): + with self.assertRaises(KeyError): TransportProtocol.get('quic2') self.assertEqual(len(TransportProtocol.__members__), before) diff --git a/tests/const/test_const_enum_no_mint.py b/tests/const/test_const_enum_no_mint.py index 49d53059ee..7f79417b1c 100644 --- a/tests/const/test_const_enum_no_mint.py +++ b/tests/const/test_const_enum_no_mint.py @@ -2682,10 +2682,14 @@ def test_get_still_refuses_an_unrecognised_name(self) -> None: """GitHub PR #836's ruling against extending ``TransportProtocol`` at all is untouched by the ``auto()`` change -- :meth:`~pcapkit.const. reg.apptype.apptype.TransportProtocol.get` still has no ``_missing_`` - of its own and still refuses outright rather than minting.""" + of its own and still refuses outright rather than minting. + + :exc:`KeyError`-shaped since GitHub issue #923, which retired the + override's ``KeyError`` -> ``ValueError`` conversion; the refusal itself + is what this test is about and that is unchanged.""" from pcapkit.const.reg.apptype.apptype import TransportProtocol - with self.assertRaises(ValueError): + with self.assertRaises(KeyError): TransportProtocol.get('not-a-real-transport') def test_stale_power_of_two_comment_is_gone(self) -> None: diff --git a/tests/corekit/test_enum_get_exception_provenance_923_unit.py b/tests/corekit/test_enum_get_exception_provenance_923_unit.py new file mode 100644 index 0000000000..bd0a3ecc39 --- /dev/null +++ b/tests/corekit/test_enum_get_exception_provenance_923_unit.py @@ -0,0 +1,570 @@ +# -*- coding: utf-8 -*- +"""GitHub issue #923: :meth:`~pcapkit.corekit.enum.EnumLookup.get` raises from +:mod:`pcapkit.utilities.exceptions`, in stdlib :class:`~enum.Enum`'s shape. + +The owner's ruling, verbatim: *"Either ``ValueError`` or ``KeyError``, that's +depending on how stdlib's ``Enum`` would raise on these circumstances. And we +should raise one from ``pcapkit.utilities.exceptions`` rather builtin +exceptions."* + +Measured on Python 3.14.7, that fixes the shape rather than leaving it open: +``E['nosuch']`` raises :exc:`KeyError` and ``E(999)`` raises :exc:`ValueError`. +The base already matched it -- a census of the 127 concrete +:class:`~pcapkit.corekit.enum.EnumLookup` subclasses found 119 answering a +string name miss with :exc:`KeyError`, 5 with :exc:`ValueError` and 3 minting -- +so this issue changes **provenance only**: the bare builtin :exc:`KeyError` from +``cls._member_map_[key]`` becomes +:exc:`~pcapkit.utilities.exceptions.EnumKeyError`, and :mod:`aenum`'s own bare +:exc:`ValueError` from ``cls(key)`` becomes +:exc:`~pcapkit.utilities.exceptions.EnumValueError`. + +Deriving :exc:`~pcapkit.utilities.exceptions.EnumKeyError` from :exc:`KeyError` +is what keeps the blast radius at nothing: six in-library sites catch the base's +name miss -- :meth:`~pcapkit.const.http.method.Method.get`, +:mod:`pcapkit.toolkit.scapy`, and two each in +:mod:`pcapkit.protocols.internet.hopopt` and +:mod:`pcapkit.protocols.internet.ipv6_opts` -- and ``Method.get`` catches it in +order to **mint**, so for that one a name miss is part of a *successful* call. +:class:`MethodStillMintsTests` pins that one directly. + +Three call sites also changed, all of them cases of the minority shape the +ruling rejects: + +* ``TransportProtocol.get`` (and its codegen twin in + :mod:`pcapkit.vendor.reg.apptype.apptype`) dropped its ``KeyError`` -> + ``ValueError`` conversion, keeping only the ``key.lower()`` call -- + :class:`TransportProtocolCaseFoldingTests`. +* ``Criticality.get`` was deleted outright, its body having become a pure + pass-through -- :class:`CriticalityGetDeletionTests`. +* :class:`~pcapkit.protocols.internet.mh.FastBindingAcknowledgmentStatus` and + :class:`~pcapkit.protocols.internet.mh.IPv6AddressPrefixCode` already raised + from :mod:`pcapkit.utilities.exceptions` on a *name* miss, but as + ``EnumValueError`` -- the right provenance in the wrong shape -- + :class:`MobilityHeaderNameMissTests`. + +On the tree before this change every test below fails at import, since +:exc:`~pcapkit.utilities.exceptions.EnumKeyError` does not exist there; with the +exception added but the base left alone, each fails on its own assertion instead. +Two are regression guards rather than defect repros and cannot fail on the +pre-#923 tree -- :meth:`TransportProtocolCaseFoldingTests. +test_upper_case_still_resolves` and the resolving half of +:class:`CriticalityGetDeletionTests` -- and they are here because they are what +the deletion and the reduction could plausibly have broken. + +""" +from __future__ import annotations + +import ast +import importlib +import pathlib +import re +import sys +import unittest +from typing import TYPE_CHECKING + +from aenum import IntEnum, StrEnum + +from pcapkit.corekit.enum import EnumLookup, EnumRegistry +from pcapkit.utilities.exceptions import EnumKeyError, EnumValueError +from pcapkit.utilities.logging import DEVMODE, logger +from tests.utilities._harness import capture + +if TYPE_CHECKING: + from typing import Any + +__all__ = [ + 'EnumKeyErrorClassTests', 'BaseGetProvenanceTests', 'BaseGetQuietnessTests', + 'MethodStillMintsTests', 'TransportProtocolCaseFoldingTests', + 'CriticalityGetDeletionTests', 'MobilityHeaderNameMissTests', + 'VendorTemplateParityTests', 'TRANSPORT_GET', +] + +#: The region both halves of the ``TransportProtocol`` pair must spell +#: identically -- the whole ``get`` classmethod, from its decorator to its last +#: ``return``. Matched rather than sliced by line number so that neither half +#: moving keeps the check from finding it. +TRANSPORT_GET = re.compile( + r"\n @classmethod\n def get\(cls, key: 'int \| str'.*?" + r"\n return super\(\)\.get\(key, default\)\n", re.S) + + +class _Closed(EnumLookup, IntEnum): + """A closed set on the bare lookup tier, with no hooks of its own.""" + + one = 1 + two = 2 + + +class _Strings(EnumRegistry, StrEnum): + """A ``str``-valued registry, for the name-before-value precedence.""" + + plain = 'plain-value' + + +class _Ranged(EnumLookup, IntEnum): + """Declares a range, so ``_validate_value`` raises in-library.""" + + low = 1 + + @classmethod + def _validate_value(cls, value: 'Any') -> 'None': + if not (isinstance(value, int) and 0 <= value <= 8): + raise EnumValueError(f'{value!r} is not a valid {cls.__name__}') + + +class EnumKeyErrorClassTests(unittest.TestCase): + """The class this issue adds, and why it is not + :exc:`~pcapkit.utilities.exceptions.MissingKeyError` reused.""" + + def test_it_is_a_key_error_and_an_in_library_error(self) -> 'None': + from pcapkit.utilities.exceptions import BaseError + + self.assertTrue(issubclass(EnumKeyError, BaseError)) + self.assertTrue(issubclass(EnumKeyError, KeyError)) + self.assertFalse(issubclass(EnumKeyError, ValueError)) + + def test_it_is_exported(self) -> 'None': + import pcapkit.utilities.exceptions as exceptions + + self.assertIn('EnumKeyError', exceptions.__all__) + + def test_it_is_distinct_from_missing_key_error(self) -> 'None': + """Deliberately a sibling rather than a reuse. + + :exc:`~pcapkit.utilities.exceptions.MissingKeyError` reports an absent + *mapping* key -- :class:`~pcapkit.corekit.multidict.MultiDict` raises it, + and so do both :mod:`pcapkit.toolkit` extractors for an absent packet + field. Reusing it here would leave a caller unable to tell "this + registry has no such member" from "this packet dict has no such field", + which is the distinction :mod:`pcapkit.toolkit.scapy` relies on when it + converts one into the other. + """ + from pcapkit.utilities.exceptions import MissingKeyError + + self.assertFalse(issubclass(EnumKeyError, MissingKeyError)) + self.assertFalse(issubclass(MissingKeyError, EnumKeyError)) + + def test_the_value_half_is_unchanged(self) -> 'None': + """The pair, so that a future edit cannot quietly reshape one half.""" + from pcapkit.utilities.exceptions import BaseError + + self.assertTrue(issubclass(EnumValueError, BaseError)) + self.assertTrue(issubclass(EnumValueError, ValueError)) + self.assertFalse(issubclass(EnumValueError, KeyError)) + + +class BaseGetProvenanceTests(unittest.TestCase): + """:meth:`~pcapkit.corekit.enum.EnumLookup.get`'s two failure paths.""" + + def test_a_name_miss_raises_the_in_library_key_error(self) -> 'None': + with self.assertRaises(EnumKeyError) as caught: + _Closed.get('nosuch') + self.assertIsInstance(caught.exception, KeyError) + self.assertNotIsInstance(caught.exception, ValueError) + self.assertIn('nosuch', str(caught.exception)) + self.assertIn('_Closed', str(caught.exception)) + + def test_a_value_miss_raises_the_in_library_value_error(self) -> 'None': + with self.assertRaises(EnumValueError) as caught: + _Closed.get(99) + self.assertIsInstance(caught.exception, ValueError) + self.assertNotIsInstance(caught.exception, KeyError) + self.assertIn('99', str(caught.exception)) + + def test_an_unusable_default_reaches_the_same_two(self) -> 'None': + """``default`` naming no registered value falls through to ``key``'s own + error, so it must reach the caller as the same class -- and must not + name the default, which is the pin GitHub issue #864 added.""" + with self.assertRaises(EnumKeyError) as caught_key: + _Strings.get('nosuch-name', 'not-a-member') + self.assertIn('nosuch-name', str(caught_key.exception)) + self.assertNotIn('not-a-member', str(caught_key.exception)) + + with self.assertRaises(EnumValueError): + _Closed.get(99, 98) + + def test_a_usable_default_still_raises_nothing(self) -> 'None': + self.assertIs(_Closed.get('nosuch', 1), _Closed.one) + self.assertIs(_Closed.get(99, 2), _Closed.two) + + def test_old_call_sites_keep_catching(self) -> 'None': + """The whole reason the shape is preserved: the six in-library + ``except KeyError`` sites, and any caller's own, keep working.""" + try: + _Closed.get('nosuch') + except KeyError as error: + caught = error + else: # pragma: no cover + self.fail('a name miss no longer raises a KeyError') + self.assertIsInstance(caught, EnumKeyError) + + try: + _Closed.get(99) + except ValueError as error: + caught_value = error # type: ValueError + else: # pragma: no cover + self.fail('a value miss no longer raises a ValueError') + self.assertIsInstance(caught_value, EnumValueError) + + def test_an_in_library_rejection_is_passed_through_not_rewrapped(self) -> 'None': + """A ``_validate_value`` override's own + :exc:`~pcapkit.utilities.exceptions.EnumValueError` is what the caller + sees, rather than a second one wrapping it -- so the override's message + survives and the error is logged once, not twice.""" + with self.assertRaises(EnumValueError) as caught: + _Ranged.get(99) + self.assertIn('is not a valid _Ranged', str(caught.exception)) + self.assertIsNone(caught.exception.__cause__) + + # Still honours ``default``, as the hook's own docstring promises. + self.assertIs(_Ranged.get(99, 1), _Ranged.low) + + def test_get_all_reports_the_same_two(self) -> 'None': + """:meth:`~pcapkit.corekit.enum.EnumLookup.get_all` forwards to + ``get``, so its documented ``Raises:`` has to move with it.""" + with self.assertRaises(EnumKeyError): + _Closed.get_all('nosuch') + with self.assertRaises(EnumValueError): + _Closed.get_all(99) + + def test_nothing_is_minted_on_either_path(self) -> 'None': + before_names = dict(_Strings._member_map_) + before_values = dict(_Strings._value2member_map_) + with self.assertRaises(EnumKeyError): + _Strings.get('nosuch-name') + self.assertEqual(_Strings._member_map_, before_names) + self.assertEqual(_Strings._value2member_map_, before_values) + + +class BaseGetQuietnessTests(unittest.TestCase): + """The name miss is raised with ``quiet=True``, and that is load-bearing. + + :meth:`~pcapkit.const.http.method.Method.get` catches it in order to mint, + so a loud error there would put one :data:`logging.CRITICAL` record on the + logger -- and set :data:`sys.tracebacklimit` to ``0`` process-wide -- for + every *successful* ``Method.get`` call that mints. That is the GitHub issue + #362 defect :class:`~pcapkit.utilities.exceptions.BaseError` documents + ``quiet`` for, and :mod:`pcapkit.protocols.protocol` states the same rule + inline: *"A pcapkit exception here would log at CRITICAL for something that + is handled two lines later."* + """ + + def setUp(self) -> 'None': + self._saved = getattr(sys, 'tracebacklimit', None) + if hasattr(sys, 'tracebacklimit'): + del sys.tracebacklimit + + def tearDown(self) -> 'None': + if self._saved is None: + if hasattr(sys, 'tracebacklimit'): + del sys.tracebacklimit + else: + sys.tracebacklimit = self._saved + + def test_a_name_miss_logs_nothing(self) -> 'None': + with capture(logger) as recorder: + with self.assertRaises(EnumKeyError): + _Closed.get('nosuch') + self.assertEqual(recorder.messages, []) + + @unittest.skipIf(DEVMODE, 'tracebacklimit is only set outside development mode') + def test_a_name_miss_leaves_tracebacklimit_alone(self) -> 'None': + with capture(logger): + with self.assertRaises(EnumKeyError): + _Closed.get('nosuch') + self.assertFalse(hasattr(sys, 'tracebacklimit')) + + def test_the_minting_override_stays_silent_end_to_end(self) -> 'None': + """The call this is actually for: a ``Method.get`` that mints is a + successful call and must emit nothing at all.""" + from pcapkit.const.http.method import Method + + with capture(logger) as recorder: + minted = Method.get('QUIET-923-PROBE') + self.assertEqual(recorder.messages, []) + self.assertEqual(minted.value, 'QUIET-923-PROBE') + + def test_the_value_miss_is_loud_which_is_what_quiet_is_measured_against(self) -> 'None': + """The control. Without it ``recorder.messages == []`` above would also + pass if :class:`~pcapkit.utilities.exceptions.BaseError` had simply + stopped logging, rather than this one raise being quiet.""" + with capture(logger) as recorder: + with self.assertRaises(EnumValueError): + _Closed.get(99) + self.assertEqual([level for level, _ in recorder.messages], ['CRITICAL']) + self.assertIn('EnumValueError', recorder.messages[0][1]) + + +class MethodStillMintsTests(unittest.TestCase): + """The sharpest call site in the blast radius. + + :meth:`~pcapkit.const.http.method.Method.get` is wired directly to the + old contract: it catches the base's name miss *in order to* hand back an + unregistered member. If :exc:`~pcapkit.utilities.exceptions.EnumKeyError` + had not derived from :exc:`KeyError`, this would raise instead of minting. + """ + + def test_an_unregistered_method_token_still_mints(self) -> 'None': + from pcapkit.const.http.method import Method + + before_names = len(Method._member_map_) + before_values = len(Method._value2member_map_) + + minted = Method.get('FROBNICATE-923') + + self.assertEqual(minted.name, 'FROBNICATE-923') + self.assertEqual(minted.value, 'FROBNICATE-923') + # ``_unregistered_member`` hands back a throwaway, so neither lookup + # table grew -- the GitHub issue #860 contract, unchanged here. + self.assertEqual(len(Method._member_map_), before_names) + self.assertEqual(len(Method._value2member_map_), before_values) + + def test_a_registered_method_still_resolves_by_name_and_by_value(self) -> 'None': + from pcapkit.const.http.method import Method + + self.assertIs(Method.get('GET'), Method.GET) + self.assertIs(Method.get('BASELINE-CONTROL'), Method.BASELINE_CONTROL) + + +class TransportProtocolCaseFoldingTests(unittest.TestCase): + """``TransportProtocol.get`` survives, reduced to its ``key.lower()``. + + Case-insensitivity is not on the base and ``get('TCP')`` resolving is live + public behaviour -- :meth:`~pcapkit.const.reg.apptype.apptype.AppType. + _dispatch` is the one live call site and it passes ``proto.lower()``, but the + public surface takes either casing. The exception conversion is what went. + """ + + def test_upper_case_still_resolves(self) -> 'None': + """A regression guard, not a defect repro: it passes on the pre-#923 + tree too, and is here because reducing the override is exactly what + could have dropped the fold along with the conversion.""" + from pcapkit.const.reg.apptype.apptype import TransportProtocol + + self.assertIs(TransportProtocol.get('TCP'), TransportProtocol.tcp) + self.assertIs(TransportProtocol.get('Udp'), TransportProtocol.udp) + self.assertIs(TransportProtocol.get('SCTP'), TransportProtocol.sctp) + self.assertIs(TransportProtocol.get('tcp'), TransportProtocol.tcp) + self.assertIs(TransportProtocol.get(1), TransportProtocol.tcp) + + def test_the_override_no_longer_converts_the_name_miss(self) -> 'None': + from pcapkit.const.reg.apptype.apptype import TransportProtocol + + with self.assertRaises(EnumKeyError) as caught: + TransportProtocol.get('quic') + self.assertIsInstance(caught.exception, KeyError) + self.assertNotIsInstance(caught.exception, ValueError) + self.assertIn('quic', str(caught.exception)) + self.assertNotIn('quic', TransportProtocol.__members__) + + def test_the_value_miss_is_the_in_library_value_error(self) -> 'None': + from pcapkit.const.reg.apptype.apptype import TransportProtocol + + with self.assertRaises(EnumValueError) as caught: + TransportProtocol.get(0x20) + self.assertIsInstance(caught.exception, ValueError) + + def test_the_override_carries_no_handler_or_raise_left(self) -> 'None': + """Read off the source, because "reduced to the ``.lower()`` call" is a + claim about the body and not only about what it raises. A conversion + re-added in a different shape -- catching + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` by name, say -- would + pass every assertion above. + + An :mod:`ast` walk rather than a substring search over the text: this + method's body is mostly comment, and two of those comments legitimately + contain the word *except* (``exception-compatible``) and *raise* + (``used to raise AttributeError``), so a textual check reports a handler + that is not there. + """ + import pcapkit.const.reg.apptype.apptype as const_module + + committed = pathlib.Path(const_module.__file__).read_text(encoding='utf-8') + node = None # type: ast.FunctionDef | None + for candidate in ast.walk(ast.parse(committed)): + if isinstance(candidate, ast.ClassDef) and candidate.name == 'TransportProtocol': + for inner in candidate.body: + if isinstance(inner, ast.FunctionDef) and inner.name == 'get': + node = inner + self.assertIsNotNone(node, 'no TransportProtocol.get in the generated module') + + self.assertEqual([], [sub for sub in ast.walk(node) if isinstance(sub, ast.Try)]) + self.assertEqual([], [sub for sub in ast.walk(node) if isinstance(sub, ast.Raise)]) + region = TRANSPORT_GET.search(committed) + self.assertIsNotNone(region, 'no TransportProtocol.get region in the generated module') + self.assertIn('super().get(key.lower(), default)', + region.group(0)) # type: ignore[union-attr] + + +class CriticalityGetDeletionTests(unittest.TestCase): + """``Criticality.get`` is gone, and the inherited base answers identically. + + Its body had become a pure pass-through once the ``KeyError`` -> + ``ValueError`` conversion went, so every assertion here is about the + *inherited* :meth:`~pcapkit.corekit.enum.EnumLookup.get`. ``default`` + handling is the one thing that could have made the deletion unsafe, so it is + pinned in both directions. + """ + + def test_the_override_is_gone(self) -> 'None': + from pcapkit.protocols.application.ngap import Criticality + + self.assertNotIn('get', Criticality.__dict__) + + def test_names_and_values_resolve_as_before(self) -> 'None': + """Invariants rather than repros -- they pass on the pre-#923 tree too, + and are the ones the deletion had to preserve.""" + from pcapkit.protocols.application.ngap import Criticality + + self.assertIs(Criticality.get('reject'), Criticality.reject) + self.assertIs(Criticality.get('ignore'), Criticality.ignore) + self.assertIs(Criticality.get('notify'), Criticality.notify) + self.assertIs(Criticality.get(0), Criticality.reject) + self.assertIs(Criticality.get(2), Criticality.notify) + self.assertIs(Criticality.get(Criticality.reject), Criticality.reject) + + def test_a_name_miss_now_reports_in_the_stdlib_shape(self) -> 'None': + from pcapkit.protocols.application.ngap import Criticality + + with self.assertRaises(EnumKeyError) as caught: + Criticality.get('nosuch') + self.assertIsInstance(caught.exception, KeyError) + self.assertNotIsInstance(caught.exception, ValueError) + self.assertIn('nosuch', str(caught.exception)) + + def test_a_member_valued_default_is_honoured(self) -> 'None': + from pcapkit.protocols.application.ngap import Criticality + + self.assertIs(Criticality.get('nosuch', Criticality.reject), Criticality.reject) + self.assertIs(Criticality.get('nosuch', Criticality.notify), Criticality.notify) + + def test_a_default_naming_no_member_is_not_honoured(self) -> 'None': + """The other half, and the one that makes the deletion safe on a closed + ASN.1 ``ENUMERATED``: a default resolves through + ``_value2member_map_`` and never through the constructor, so no path + here can mint a fourth value.""" + from pcapkit.protocols.application.ngap import Criticality + + before = len(Criticality.__members__) + with self.assertRaises(EnumKeyError): + Criticality.get('nosuch', 99) + self.assertEqual(len(Criticality.__members__), before) + + def test_a_value_miss_still_goes_through_missing(self) -> 'None': + from pcapkit.protocols.application.ngap import Criticality + + with self.assertRaises(EnumValueError) as caught: + Criticality.get(3) + self.assertIsInstance(caught.exception, ValueError) + self.assertIn('3', str(caught.exception)) + + +class MobilityHeaderNameMissTests(unittest.TestCase): + """The two :rfc:`5568` inline enumerations found by #923's own census. + + Both already raised from :mod:`pcapkit.utilities.exceptions` on a name miss + -- the right provenance -- but chose ``EnumValueError`` deliberately, *"so + the two ways of getting this wrong do not report differently"*. That is the + conversion this ruling rejects, so the name half moves to + :exc:`~pcapkit.utilities.exceptions.EnumKeyError` and only ``_missing_``'s + value half stays :exc:`ValueError`-shaped. + """ + + def test_a_name_miss_is_key_shaped(self) -> 'None': + from pcapkit.protocols.internet.mh import (FastBindingAcknowledgmentStatus, + IPv6AddressPrefixCode) + + for enum_cls in (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode): + with self.subTest(enum=enum_cls.__name__): + with self.assertRaises(EnumKeyError) as caught: + enum_cls.get('Vendor_specific') + self.assertIsInstance(caught.exception, KeyError) + self.assertNotIsInstance(caught.exception, ValueError) + self.assertIn('Vendor_specific', str(caught.exception)) + self.assertIn(enum_cls.__name__, str(caught.exception)) + + def test_a_value_miss_stays_value_shaped(self) -> 'None': + from pcapkit.protocols.internet.mh import (FastBindingAcknowledgmentStatus, + IPv6AddressPrefixCode) + + for enum_cls, unassigned in ((FastBindingAcknowledgmentStatus, 77), + (IPv6AddressPrefixCode, 200)): + with self.subTest(enum=enum_cls.__name__): + with self.assertRaises(EnumValueError) as caught: + enum_cls.get(unassigned) + self.assertIsInstance(caught.exception, ValueError) + self.assertNotIsInstance(caught.exception, KeyError) + + def test_a_declared_name_and_value_still_resolve(self) -> 'None': + from pcapkit.protocols.internet.mh import (FastBindingAcknowledgmentStatus, + IPv6AddressPrefixCode) + + self.assertIs(FastBindingAcknowledgmentStatus.get('Insufficient_resources'), + FastBindingAcknowledgmentStatus.Insufficient_resources) + self.assertIs(FastBindingAcknowledgmentStatus.get(130), + FastBindingAcknowledgmentStatus.Insufficient_resources) + self.assertIs(IPv6AddressPrefixCode.get('NAR_Prefix'), + IPv6AddressPrefixCode.NAR_Prefix) + self.assertIs(IPv6AddressPrefixCode.get(4), IPv6AddressPrefixCode.NAR_Prefix) + + def test_neither_class_grows(self) -> 'None': + from pcapkit.protocols.internet.mh import (FastBindingAcknowledgmentStatus, + IPv6AddressPrefixCode) + + for enum_cls in (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode): + with self.subTest(enum=enum_cls.__name__): + before = len(list(enum_cls)) + with self.assertRaises(EnumKeyError): + enum_cls.get('nosuch_923') + self.assertEqual(len(list(enum_cls)), before) + + +@unittest.skipUnless(importlib.util.find_spec('requests') is not None, + 'pcapkit.vendor needs requests') +class VendorTemplateParityTests(unittest.TestCase): + """A regeneration must not undo the change. + + ``pcapkit/const/`` is generated from ``pcapkit/vendor/``, so the committed + module passing everything above is not evidence that the template agrees + with it -- the next crawl would simply revert it, which is the trap + ``TransportProtocol.get`` living inside an f-string template sets. The same + proof shape as ``test_the_tcp_flags_template_renders_the_committed_module`` + and ``test_the_vendor_templates_still_emit_the_fix``: render the template and + compare against the committed file. Needs no network -- the crawl supplies + only the enumeration block and the ``_missing_`` branches, neither of which + the ``get`` region below interpolates, so placeholder arguments render it + verbatim. + """ + + def test_the_template_renders_the_committed_get_region(self) -> 'None': + from tests.const.test_const_enum_lookup import _normalize + + vendor_module = importlib.import_module('pcapkit.vendor.reg.apptype.apptype') + const_module = importlib.import_module('pcapkit.const.reg.apptype.apptype') + + rendered = _normalize(vendor_module.BASE( + 'AppType', 'Application Layer Protocol Numbers [AppType]', + '', '', '', 'pcapkit.vendor.reg.apptype.apptype', + )) + committed = pathlib.Path( + const_module.__file__ # type: ignore[arg-type] + ).read_text(encoding='utf-8') + + rendered_region = TRANSPORT_GET.search(rendered) + committed_region = TRANSPORT_GET.search(committed) + self.assertIsNotNone(rendered_region, 'no TransportProtocol.get in the rendered template') + self.assertIsNotNone(committed_region, 'no TransportProtocol.get in the generated module') + + # Not vacuous: both halves have to carry the reduced body, so a template + # that still converts cannot match a committed module that does not, and + # a pair that agree on the *old* body fails here too. + self.assertIn('super().get(key.lower(), default)', + rendered_region.group(0)) # type: ignore[union-attr] + self.assertNotIn("raise ValueError(f'{key!r} is not a valid", + rendered_region.group(0)) # type: ignore[union-attr] + self.assertEqual(rendered_region.group(0), # type: ignore[union-attr] + committed_region.group(0)) # type: ignore[union-attr] + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/corekit/test_enum_lookup_reparent_877_unit.py b/tests/corekit/test_enum_lookup_reparent_877_unit.py index 1eef1538c6..4efd07f2f7 100644 --- a/tests/corekit/test_enum_lookup_reparent_877_unit.py +++ b/tests/corekit/test_enum_lookup_reparent_877_unit.py @@ -26,11 +26,18 @@ already defined their own ``get`` -- both as a :class:`staticmethod`, while :meth:`~pcapkit.corekit.enum.EnumLookup.get` is a :class:`classmethod`, the exact trap GitHub issue #908 hit and #915 fixed for -:meth:`~pcapkit.const.http.method.Method.get`. Both are now classmethods -that delegate, each keeping only the behaviour the base does not reproduce -on its own -- :class:`TransportProtocolGetTests` and -:class:`CriticalityGetTests` pin that each of those kept behaviours is -unchanged, not merely that the delegation compiles. +:meth:`~pcapkit.const.http.method.Method.get`. Both became classmethods that +delegate, each keeping only the behaviour the base does not reproduce on its +own -- :class:`TransportProtocolGetTests` and :class:`CriticalityGetTests` pin +that each of those kept behaviours is unchanged, not merely that the +delegation compiles. + +GitHub issue #923 then reduced that pair to one. Both overrides converted the +base's name-miss :exc:`KeyError` into a :exc:`ValueError`, and #923's ruling +retired the conversion -- so ``Criticality.get``, whose *only* remaining job +that was, is gone entirely and :class:`CriticalityGetTests` now exercises the +inherited base; ``TransportProtocol.get`` survives, reduced to its +``key.lower()`` call, because case folding has no equivalent on the base. The other nine are pure re-parenting -- no ``get`` or ``_missing_`` of their own to reconcile -- so :class:`PureReparentGetTests` pins the one thing that @@ -272,17 +279,28 @@ def test_int_path_unchanged(self) -> None: self.assertIs(TransportProtocol.get(1), TransportProtocol.tcp) - def test_unrecognised_name_still_raises_value_error_naming_the_key(self) -> None: - """Maintainer ruling on PR #836: refuse, never mint. The base's own - miss on a ``str`` key raises a bare ``KeyError``; this override still - converts it to the ``ValueError`` every caller and test here already - depends on.""" + def test_unrecognised_name_still_raises_naming_the_key(self) -> None: + """Maintainer ruling on PR #836: refuse, never mint. Still refused, and + still nothing minted -- but GitHub issue #923 retired the + ``KeyError`` -> ``ValueError`` conversion this override used to do, so + the refusal now reaches the caller as the base's own + :exc:`~pcapkit.utilities.exceptions.EnumKeyError`. Updated from the + pre-#923 tree, which asserted ``assertNotIsInstance(..., KeyError)``: + that assertion described the minority shape -- #923's census found 119 + of the 127 concrete subclasses answering a name miss with a + :exc:`KeyError` -- and #923's ruling is that a name miss follows stdlib + ``E['nosuch']`` and stays :exc:`KeyError`-derived. The message is + unchanged, so the two ``assertIn``\\ s below are the same ones that + passed before.""" + from pcapkit.utilities.exceptions import EnumKeyError + from pcapkit.const.reg.apptype.apptype import TransportProtocol before = len(TransportProtocol.__members__) - with self.assertRaises(ValueError) as caught: + with self.assertRaises(EnumKeyError) as caught: TransportProtocol.get('quic') - self.assertNotIsInstance(caught.exception, KeyError) + self.assertIsInstance(caught.exception, KeyError) + self.assertNotIsInstance(caught.exception, ValueError) self.assertIn('quic', str(caught.exception)) self.assertIn('is not a valid', str(caught.exception)) self.assertEqual(len(TransportProtocol.__members__), before) @@ -298,22 +316,35 @@ def test_default_is_a_new_capability_not_exercised_before(self) -> None: self.assertIs(TransportProtocol.get('bogus', default=TransportProtocol.udp), TransportProtocol.udp) - # Omitted, as every call site in this tree omits it: raises exactly - # as before this change. - with self.assertRaises(ValueError): + # Omitted, as every call site in this tree omits it: still raises, as + # a KeyError since GitHub issue #923 rather than a ValueError. + with self.assertRaises(KeyError): TransportProtocol.get('bogus') class CriticalityGetTests(unittest.TestCase): """:class:`~pcapkit.protocols.application.ngap.Criticality` kept its own - ``get`` too, now delegating -- case-**sensitive**, unlike - ``TransportProtocol``, which is the one designed divergence between the - two overrides this issue re-parented.""" - - def test_get_is_now_a_classmethod(self) -> None: + ``get`` for this issue, then lost it to GitHub issue #923: the only thing + the override still did that the base does not was convert the base's + name-miss :exc:`KeyError` into a :exc:`ValueError`, and #923's ruling + retired that conversion, leaving a pure pass-through. Every behaviour + below is therefore now the *inherited* base's, which is exactly what + makes the deletion worth pinning -- case-**sensitive**, unlike + ``TransportProtocol``, which keeps its override for the case folding the + base has no equivalent of.""" + + def test_get_is_inherited_rather_than_overridden(self) -> None: + """GitHub issue #923 deleted the override. ``get`` is still a + :class:`classmethod` reached through the class -- it is now + :meth:`~pcapkit.corekit.enum.EnumLookup.get` itself, which is what + the rest of this class exercises.""" + from pcapkit.corekit.enum import EnumLookup from pcapkit.protocols.application.ngap import Criticality + self.assertNotIn('get', Criticality.__dict__) self.assertIsInstance(inspect.getattr_static(Criticality, 'get'), classmethod) + self.assertIs(inspect.getattr_static(Criticality, 'get'), + inspect.getattr_static(EnumLookup, 'get')) def test_name_lookup(self) -> None: from pcapkit.protocols.application.ngap import Criticality @@ -337,40 +368,61 @@ def test_a_criticality_instance_resolves_to_itself(self) -> None: self.assertIs(Criticality.get(Criticality.reject), Criticality.reject) def test_case_sensitivity_is_unlike_transport_protocol(self) -> None: - """Deliberately the opposite of ``TransportProtocol.get`` -- this - override never lower-cases, so an upper-cased spelling must be - refused rather than folded.""" + """Deliberately the opposite of ``TransportProtocol.get`` -- nothing + here lower-cases, so an upper-cased spelling must be refused rather + than folded. Now :exc:`KeyError`-shaped, per GitHub issue #923.""" from pcapkit.protocols.application.ngap import Criticality - with self.assertRaises(ValueError) as caught: + with self.assertRaises(KeyError) as caught: Criticality.get('REJECT') self.assertIn('REJECT', str(caught.exception)) - def test_unresolved_name_raises_value_error_not_key_error(self) -> None: - """The base's own miss on a ``str`` key raises a bare ``KeyError``; - this override still converts it to the ``ValueError`` its own - ``_missing_`` already uses for an unresolved value, so the two ways - of getting this wrong report identically -- exactly as before this - change.""" + def test_unresolved_name_raises_key_error_not_value_error(self) -> None: + """The inverse of what this test asserted before GitHub issue #923. + + The deleted override converted the base's name-miss :exc:`KeyError` + into a :exc:`ValueError` so that an unknown name and an unknown value + reported identically. #923's ruling is that they must *not*: a stdlib + ``E['nosuch']`` raises :exc:`KeyError` and ``E(999)`` raises + :exc:`ValueError`, so the two ways of getting this wrong are + deliberately distinguishable, and only the provenance of each moved + into :mod:`pcapkit.utilities.exceptions`.""" + from pcapkit.utilities.exceptions import EnumKeyError + from pcapkit.protocols.application.ngap import Criticality - with self.assertRaises(ValueError) as caught: + with self.assertRaises(EnumKeyError) as caught: Criticality.get('NoSuchMember') - self.assertNotIsInstance(caught.exception, KeyError) + self.assertIsInstance(caught.exception, KeyError) + self.assertNotIsInstance(caught.exception, ValueError) self.assertIn('NoSuchMember', str(caught.exception)) def test_unresolved_value_still_raises_via_missing(self) -> None: + """Still a :exc:`ValueError`, and now this class's own + :exc:`~pcapkit.utilities.exceptions.EnumValueError`: ``_missing_`` + raises a bare :exc:`ValueError` and the base converts it, per GitHub + issue #923's "no builtin exceptions" half.""" + from pcapkit.utilities.exceptions import EnumValueError + from pcapkit.protocols.application.ngap import Criticality - with self.assertRaises(ValueError): + with self.assertRaises(EnumValueError) as caught: Criticality.get(3) - - def test_default_is_a_new_capability_not_exercised_before(self) -> None: + self.assertIsInstance(caught.exception, ValueError) + self.assertIn('3', str(caught.exception)) + + def test_default_is_honoured_by_the_inherited_base(self) -> None: + """The one thing that could have made GitHub issue #923's deletion of + this override unsafe. The override forwarded ``default`` verbatim, so + the inherited base has to answer identically: a member-valued default + resolves, and one naming no registered value is not honoured.""" from pcapkit.protocols.application.ngap import Criticality self.assertIs(Criticality.get('NoSuchMember', default=Criticality.ignore), Criticality.ignore) - with self.assertRaises(ValueError): + with self.assertRaises(KeyError): + Criticality.get('NoSuchMember', default=99) + with self.assertRaises(KeyError): Criticality.get('NoSuchMember') diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index d63d948c07..8d06b98c16 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -1544,17 +1544,30 @@ def handover_initiate(code: int) -> object: enum_cls(value) with self.subTest('get() backports string and integer lookups, and no longer mints'): + # An unknown *name* is ``EnumKeyError`` since GitHub issue #923 -- + # both ``get`` overrides used to raise ``EnumValueError`` for it, so + # that a name miss and a value miss reported identically, and that + # is the conversion #923's ruling rejects: stdlib ``E['nosuch']`` + # raises ``KeyError``, so the name half is ``KeyError``-derived and + # only ``_missing_``'s value half stays ``ValueError``-derived. + from pcapkit.utilities.exceptions import EnumKeyError + self.assertIs(FastBindingAcknowledgmentStatus.get(130), FastBindingAcknowledgmentStatus.Insufficient_resources) self.assertIs(FastBindingAcknowledgmentStatus.get('Reason_unspecified'), FastBindingAcknowledgmentStatus.Reason_unspecified) - with self.assertRaises(EnumValueError): + with self.assertRaises(EnumKeyError) as caught: FastBindingAcknowledgmentStatus.get('Vendor_specific') + self.assertIsInstance(caught.exception, KeyError) + self.assertNotIsInstance(caught.exception, ValueError) + self.assertIn('Vendor_specific', str(caught.exception)) self.assertIs(IPv6AddressPrefixCode.get(4), IPv6AddressPrefixCode.NAR_Prefix) self.assertIs(IPv6AddressPrefixCode.get('New_Care_of_Address'), IPv6AddressPrefixCode.New_Care_of_Address) - with self.assertRaises(EnumValueError): + with self.assertRaises(EnumKeyError) as caught: IPv6AddressPrefixCode.get('Vendor_specific') + self.assertIsInstance(caught.exception, KeyError) + self.assertNotIsInstance(caught.exception, ValueError) with self.subTest('make round trips both enums byte-identically'): for status, code in [ @@ -1635,12 +1648,18 @@ def test_mh_local_enums_raise_and_do_not_alias(self) -> None: # that same member -- a name that lies about itself. Both calls # must now raise independently, and neither may touch the # class's own lookup tables while doing so. + # + # ``EnumKeyError`` rather than ``EnumValueError`` since GitHub + # issue #923: these are *name* misses, and #923's ruling keeps a + # name miss ``KeyError``-shaped after stdlib ``E['nosuch']``. + from pcapkit.utilities.exceptions import EnumKeyError + for enum_cls in (FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode): before_members = dict(enum_cls._member_map_) # pylint: disable=no-member before_values = dict(enum_cls._value2member_map_) # pylint: disable=no-member - with self.assertRaises(EnumValueError): + with self.assertRaises(EnumKeyError): enum_cls.get('bogus_one') - with self.assertRaises(EnumValueError): + with self.assertRaises(EnumKeyError): enum_cls.get('bogus_two') self.assertEqual(enum_cls._member_map_, before_members) # pylint: disable=no-member self.assertEqual(enum_cls._value2member_map_, before_values) # pylint: disable=no-member