fix(corekit): fall EnumRegistry.get() back to a str key's value (#860)

pcapkit/corekit/enum.py

@@ -264,13 +264,43 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self':
         returns the member the alias points at, not a separate object -- so
         two names for one assignment resolve to one enum.
 
-        It never mints. Registering a member is :meth:`register`'s job and
+        It never mints -- see #864 for the ``default`` path, which does and
+        should not. Registering a member is :meth:`register`'s job and
         nobody else's, which is the ruling #775 exists to carry out: *"so that
         we dont create registered enums out of unrecognised/unregistered
         values, unless user/caller explicitly created them"*. A value inside a
         registry's declared-but-unassigned range still resolves, through that
         registry's own ``_missing_`` and :meth:`_unregistered_member`, to a
-        member that is deliberately absent from the lookup tables.
+        member that is deliberately absent from the lookup tables -- for a
+        non-``str`` key; the ``str`` case is qualified below.
+
+        For a ``str`` key, a name match wins over a value match -- the two are
+        checked in that order, so a string that happens to be both a member's
+        name and a *different* member's value resolves to the name's member,
+        matching what already happened for a name that resolves today. The
+        value side of that check is a plain ``_value2member_map_`` lookup, not
+        ``cls(key)``: on a registry whose own ``_missing_`` mints for an
+        unrecognised value -- :class:`~pcapkit.const.ftp.command.FEATCode`'s
+        does, directly, via :func:`~aenum.extend_enum` -- routing a failed
+        *name* lookup through the constructor would let a mere ``get()`` call
+        mint a permanent member where it previously just raised. Restricting
+        the value side of ``key`` to an already-registered value keeps *that
+        side* non-minting on every ``str``-valued registry, not only the ones
+        without a minting ``_missing_`` -- it is a claim about the value side
+        alone, not about this method as a whole: an unregistered ``default``
+        still reaches ``cls(default)`` below, and can mint there just the same.
+
+        That restriction has a cost the paragraph above glosses over: a
+        *declared-but-unassigned* value -- the case resolved there through
+        ``_missing_`` and :meth:`_unregistered_member` without either lookup
+        table growing -- is for that exact reason invisible to the
+        ``_value2member_map_`` check above. Such a value resolves through
+        ``cls(value)`` but not through ``get(value)`` when ``value`` is a
+        ``str``; the non-``str`` path below has no such gap, since it always
+        calls ``cls(key)`` and so always reaches ``_missing_``. Closing that
+        gap here would mean calling ``cls(key)`` for a ``str`` value too,
+        which reopens the exact minting hazard the paragraph above exists to
+        avoid -- so the asymmetry is deliberate, not an oversight.
 
         Args:
             key: Name or value to look up.
@@ -292,6 +322,8 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self':
             try:
                 return cls._member_map_[key]
             except KeyError:
+                if key in cls._value2member_map_:
+                    return cls._value2member_map_[key]
                 if default is NO_DEFAULT:
                     raise
                 return cls(default)  # type: ignore[call-arg]

tests/const/test_const_registry_protocol.py

@@ -26,7 +26,7 @@
 import unittest
 from typing import TYPE_CHECKING
 
-from aenum import IntEnum, IntFlag, StrEnum
+from aenum import IntEnum, IntFlag, StrEnum, extend_enum
 
 from pcapkit.corekit.enum import EnumRegistry
 from tests._support import ISOLATED_PREFIXES, purge_modules, restore_modules, snapshot_modules
@@ -698,10 +698,15 @@ class _Str(EnumRegistry, StrEnum):
 #: their crawler's own ``process()``/``context()`` with a bespoke,
 #: mint-on-lookup ``get()``/``_missing_`` pair that does not share
 #: :mod:`pcapkit.vendor.default`'s template at all -- converting any of them
-#: onto :class:`~pcapkit.corekit.enum.EnumRegistry` would silently change
-#: behaviour rather than just move it, per the base ``get()``'s own
-#: str-key-never-tries-the-value-path limitation measured in
-#: :class:`StrEnumValuePathLimitationTests` below. The remaining four
+#: onto :class:`~pcapkit.corekit.enum.EnumRegistry` would still silently
+#: change behaviour rather than just move it, since four of the six are
+#: :class:`~aenum.StrEnum` registries whose own ``__new__`` attaches further
+#: attributes ``_unregistered_member`` does not set. GitHub issue #860 fixed
+#: the base ``get()``'s own str-key-never-tries-the-value-path limitation
+#: this comment used to cite -- measured on a synthetic registry in
+#: :class:`StrEnumValueFallbackTests` below -- but that fix only reaches a
+#: registry that already mixes in the base; actually converting these six is
+#: issue #860's still-open step 2, not this tier. The remaining four
 #: (``reg/apptype``'s transport subclasses) plus ``AppType``/
 #: ``TransportProtocol`` themselves are excluded for the separate reason
 #: :mod:`pcapkit.corekit.enum`'s own module docstring gives: they are tier 2
@@ -1021,34 +1026,176 @@ def test_a_key_that_is_neither_int_nor_str_flips_exception_type(self) -> None:
                 self.assertIn('is not a valid TransType', str(caught.exception))
 
 
-class StrEnumValuePathLimitationTests(unittest.TestCase):
-    """Justifies excluding every :class:`~aenum.StrEnum` const module from
-    this tier (:data:`EXCLUDED_STILL_BESPOKE`'s ``ftp``/``http``/``pcapng``/
-    ``reg.apptype`` entries): the base ``get()``'s ``isinstance(key, str)``
-    branch only ever tries ``cls._member_map_[key]`` -- a *name* lookup -- and
-    never falls back to treating a ``str`` key as a *value*, unlike the
-    integer path. That is pre-existing behaviour of
+class GetDispatchMatrixIntFlagTests(unittest.TestCase):
+    """The same matrix as :class:`GetDispatchMatrixTests`, on an
+    :class:`~aenum.IntFlag` registry rather than a plain
+    :class:`~aenum.IntEnum` one -- GitHub issue #860's own instructions call
+    for measuring both member types the base serves, not just one. Every
+    probe here is unchanged by #860's ``str``-branch fix, since
+    ``HandoverACKFlag.get`` never takes that branch for an ``int`` key or a
+    key that is neither ``int`` nor ``str``; this class exists to pin that
+    the ``IntFlag`` path really is untouched, not to assume it."""
+
+    def setUp(self) -> None:
+        snapshot = snapshot_modules(ISOLATED_PREFIXES)
+        purge_modules(['pcapkit'])
+        self.addCleanup(restore_modules, snapshot, ISOLATED_PREFIXES)
+
+    def test_valid_int_value(self) -> None:
+        from pcapkit.const.mh.handover_ack_flag import HandoverACKFlag
+
+        self.assertIs(HandoverACKFlag.get(0x80), HandoverACKFlag.U)
+
+    def test_valid_name(self) -> None:
+        from pcapkit.const.mh.handover_ack_flag import HandoverACKFlag
+
+        self.assertIs(HandoverACKFlag.get('U'), HandoverACKFlag.U)
+
+    def test_missing_name_without_default_raises_key_error(self) -> None:
+        from pcapkit.const.mh.handover_ack_flag import HandoverACKFlag
+
+        with self.assertRaises(KeyError):
+            HandoverACKFlag.get('Definitely_Not_A_Member')
+
+    def test_missing_name_with_default_falls_back(self) -> None:
+        from pcapkit.const.mh.handover_ack_flag import HandoverACKFlag
+
+        self.assertIs(HandoverACKFlag.get('Definitely_Not_A_Member', 0x40), HandoverACKFlag.P)
+
+    def test_missing_int_without_default_raises_value_error(self) -> None:
+        """``9999`` is outside the ``0..0xFF`` range ``HandoverACKFlag``'s own
+        ``_missing_`` bounds, so this is a genuine miss."""
+        from pcapkit.const.mh.handover_ack_flag import HandoverACKFlag
+
+        with self.assertRaises(ValueError):
+            HandoverACKFlag.get(9999)
+
+    def test_missing_int_with_default_falls_back(self) -> None:
+        from pcapkit.const.mh.handover_ack_flag import HandoverACKFlag
+
+        self.assertIs(HandoverACKFlag.get(9999, 0x40), HandoverACKFlag.P)
+
+    def test_a_key_that_is_neither_int_nor_str_raises_value_error(self) -> None:
+        from pcapkit.const.mh.handover_ack_flag import HandoverACKFlag
+
+        for key in (3.5, None, b'x'):
+            with self.subTest(key=key):
+                with self.assertRaises(ValueError) as caught:
+                    HandoverACKFlag.get(key)
+                self.assertIn('is not a valid HandoverACKFlag', str(caught.exception))
+
+
+class StrEnumValueFallbackTests(unittest.TestCase):
+    """GitHub issue #860, step 1: the base ``get()``'s ``isinstance(key, str)``
+    branch used to try only ``cls._member_map_[key]`` -- a *name* lookup --
+    and never fall back to treating a ``str`` key as a *value*, unlike the
+    integer path. That was pre-existing behaviour of
     :class:`~pcapkit.corekit.enum.EnumRegistry` itself (added by tier 1,
     #855, which never converted a :class:`~aenum.StrEnum` registry either),
-    not something this tier introduces -- reproduced here on a synthetic
-    registry so the exclusion decision is measured rather than assumed, and
-    so a future tier that does take on the ``StrEnum`` registries knows
-    exactly what it would need to fix first.
+    found during #858's review on a synthetic registry rather than a real
+    one, since none of the four bespoke ``StrEnum`` const registries mix in
+    this base yet (:data:`EXCLUDED_STILL_BESPOKE` above) -- that conversion
+    is #860's separate, still-open step 2.
+
+    Fixed here by falling back to a plain ``_value2member_map_`` lookup, not
+    ``cls(key)``: :class:`~pcapkit.const.ftp.command.FEATCode`'s own
+    ``_missing_`` mints directly via :func:`~aenum.extend_enum` for any
+    unrecognised value (that minting call reproduced verbatim in
+    :meth:`test_get_never_mints_on_a_registry_whose_missing_mints_directly`
+    below), so routing the value fallback through the constructor would let
+    a failed *name* lookup mint a permanent member the moment #860's step 2
+    converts a registry like it onto this base -- exactly the "never mints"
+    defect :meth:`~pcapkit.corekit.enum.EnumRegistry.get`'s own docstring
+    rules out. A raw dict lookup can never reach ``_missing_``, so it cannot
+    mint regardless of what a subclass's own ``_missing_`` does.
     """
 
-    def test_a_str_key_that_is_a_value_but_not_a_name_is_not_resolved(self) -> None:
+    def test_a_str_key_that_is_a_value_but_not_a_name_now_resolves(self) -> None:
+        """The defect itself. Fails on ``main`` (raises ``KeyError``, see the
+        module docstring's reproduction), passes here."""
         class _Str(EnumRegistry, StrEnum):
             KNOWN = 'known-value'
 
         # The name resolves, as it always would.
         self.assertIs(_Str.get('KNOWN'), _Str.KNOWN)
 
-        # The *value* -- a different string from the name -- does not, even
-        # though ``_Str('known-value')`` (the constructor) resolves it fine.
-        # This is the gap: get() never reaches the constructor for a str key.
+        # The *value* -- a different string from the name -- now resolves
+        # too, matching what the constructor already did directly.
         self.assertIs(_Str('known-value'), _Str.KNOWN)
+        self.assertIs(_Str.get('known-value'), _Str.KNOWN)
+
+    def test_name_wins_over_a_different_members_value(self) -> None:
+        """Ordering decision: a name match wins over a value match, so a
+        string that is *both* a member's name and a different member's
+        value resolves to the name's member. This already held for a name
+        that resolves before this fix (the name branch is untouched), so it
+        passes on both trees -- it is the ordering this fix's fall-through
+        had to preserve, not new behaviour, and is pinned here so the
+        ordering stays a decision rather than an accident."""
+        class _Str(EnumRegistry, StrEnum):
+            ALPHA = 'BETA'
+            BETA = 'gamma'
+
+        self.assertIs(_Str.get('BETA'), _Str.BETA)
+        self.assertIsNot(_Str.get('BETA'), _Str.ALPHA)
+
+    def test_a_str_value_match_wins_over_a_supplied_default(self) -> None:
+        """A second, compounding half of the same defect: on ``main``, the
+        ``str`` branch's ``except KeyError`` fires for *any* non-name key,
+        so a supplied ``default`` was returned even when the key itself was
+        a perfectly good value -- the default silently overrode a match that
+        should have won. Fails on ``main`` (returns ``_Str('elsewhere')``
+        instead of ``_Str.KNOWN``), passes here."""
+        class _Str(EnumRegistry, StrEnum):
+            KNOWN = 'known-value'
+            ELSEWHERE = 'elsewhere'
+
+        self.assertIs(_Str.get('known-value', 'elsewhere'), _Str.KNOWN)
+
+    def test_missing_value_with_no_match_and_no_default_still_raises_key_error(self) -> None:
+        """A ``str`` key that resolves neither as a name nor as a value, with
+        no default, still raises :exc:`KeyError` -- not :exc:`ValueError` --
+        matching :meth:`~pcapkit.corekit.enum.EnumRegistry.get`'s own
+        documented contract for a name miss. Unchanged by this fix: passes
+        on both trees."""
+        class _Str(EnumRegistry, StrEnum):
+            KNOWN = 'known-value'
+
         with self.assertRaises(KeyError):
-            _Str.get('known-value')
+            _Str.get('totally-unknown')
+
+    def test_get_never_mints_on_a_registry_whose_missing_mints_directly(self) -> None:
+        """The crux this fix has to get right: the minting half of this
+        fixture's ``_missing_`` is verbatim
+        :meth:`~pcapkit.const.ftp.command.FEATCode._missing_`
+        (``pcapkit/const/ftp/command.py:52``) -- ``extend_enum(cls,
+        value.upper(), value)`` for any unrecognised string. Its non-``str``
+        branch differs (delegates to ``super()._missing_`` rather than
+        ``FEATCode``'s own explicit ``ValueError``), which is immaterial to
+        what this test proves. A naive ``return cls(key)`` fallback would
+        mint a permanent member from a mere failed lookup; the
+        ``_value2member_map_`` lookup this fix uses instead never reaches
+        ``_missing_`` at all, so it cannot. Passes on both trees -- ``main``
+        never attempts the value path in the first place, so it cannot mint
+        either; this pins that the fix does not regress that guarantee while
+        closing the gap."""
+        class _MintingStr(EnumRegistry, StrEnum):
+            KNOWN = 'known-value'
+
+            @classmethod
+            def _missing_(cls, value: 'Any') -> 'Any':
+                if not isinstance(value, str):
+                    return super()._missing_(value)
+                return extend_enum(cls, value.upper(), value)
+
+        values_before = {member.value for member in _MintingStr}
+        members_before = set(_MintingStr.__members__)
+
+        with self.assertRaises(KeyError):
+            _MintingStr.get('never-seen-before')
+
+        self.assertEqual(values_before, {member.value for member in _MintingStr})
+        self.assertEqual(members_before, set(_MintingStr.__members__))
 
 
 class NoDefaultSentinelTests(unittest.TestCase):