From 50813f1dfa29cb5a61ffdca55af9fe70431483a2 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 29 Sep 2026 01:44:23 -0400 Subject: [PATCH] fix(const,vendor,docs): audit registry case sensitivity, fold FEATCode.get (#903) - Record the audit in docs/source/conventions.rst: the lenient criterion's two limbs, the population it covers (127 registries, 24 helper enumerations), and a per-class table of governing source, quoted rule and verdict. - FEATCode.get was case-sensitive with nothing behind it, and RFC 5797 Section 2 states the opposite outright -- IANA keeps FEAT codes unique by case-insensitive comparison -- so give it a folding get, in the crawler template as well as the generated module. - The fold is a fallback only: an exact name or value hit still delegates to the base, so name-before-value precedence and the non-minting str path are unchanged, and nothing calls cls(key). - Correct three stale prose claims this invalidates: the "Four do" override count on the conventions page, and two test-module notes stating FEATCode carries no get of its own. - Remeasure "the 125 classes that reach this method" in pcapkit/corekit/enum.py as 127, after #880's two NGAP registries landed. 15 new tests, 7 of which fail without the fix (measured by reverting the three source files and re-running under plain unittest, since pytest-subtests miscounts a method whose subTests fail). tests/const and tests/corekit pass: 549 passed, 16 skipped, 40970 subtests. mypy and pylint show no new finding; isort clean. --- docs/source/contributing/conventions.rst | 215 +++++++++++- pcapkit/const/ftp/command.py | 83 ++++- pcapkit/corekit/enum.py | 5 +- pcapkit/vendor/ftp/command.py | 83 ++++- tests/const/test_const_enum_no_mint.py | 7 +- .../test_const_ftp_featcode_case_903_unit.py | 329 ++++++++++++++++++ ...st_const_method_case_sensitive_896_unit.py | 7 +- 7 files changed, 706 insertions(+), 23 deletions(-) create mode 100644 tests/const/test_const_ftp_featcode_case_903_unit.py diff --git a/docs/source/contributing/conventions.rst b/docs/source/contributing/conventions.rst index cfc100e86c..fd79360b0e 100644 --- a/docs/source/contributing/conventions.rst +++ b/docs/source/contributing/conventions.rst @@ -337,19 +337,196 @@ to make a lookup work -- which is what keeps ``R1_COUNTER = 129``, two IANA-registered HIP parameters differing only in case, both resolvable. -.. note:: +The Lenient Criterion, in Two Limbs +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ruling above leaves one question open, and +`#903 `__ settled it: does a +specification have to state a **comparison rule** for a registry to be treated +case-insensitively, or does it also count when the authorities merely **disagree +about spelling**? The owner's answer, verbatim: + + I say lenient. TransportProtocol for example should be case-insensitive. Upper or + lower cases are being used everywhere in RFC and IANA themselves so that's an + indication of case insensitivity. + +So the test a new registry has to pass has **two limbs**, and satisfying either one +justifies case-insensitivity: + +1. **A comparison rule in the governing document.** :rfc:`959#section-4.1` for FTP + command codes, :rfc:`5797#section-2` for FTP FEAT codes, :rfc:`6335#section-5.1` + for IANA service names. +2. **A documented spelling disagreement between the specification and the registry.** + If the RFC writes a field one way throughout and the live IANA data writes it + another, then neither authority is treating case as significant, and a lookup that + does would reject a caller holding the spec's own spelling. + +Limb 2 has to be **measured, not assumed** -- count the casings in the registry the +crawler actually reads, and say how many rows carried each. A guess about which way +IANA spells a column is not evidence. + +Where neither limb holds, the lookup is case-sensitive and inherits +:meth:`~pcapkit.corekit.enum.EnumLookup.get` unchanged. + +The Audit, per Class +~~~~~~~~~~~~~~~~~~~~ + +`#903 `__'s sweep, so that a +registry added later has something to check itself against. The owner's scope for it, +verbatim: *"we should audit all registries and then decide if case (in)sensitive."* + +The population it covers, with the counting convention spelled out because the +figures move: **127** :class:`~pcapkit.corekit.enum.EnumRegistry` subclasses, every +one of them under :mod:`pcapkit.const`, across 124 files -- 117 :class:`int`-valued +(of which 5 are flag registries) and 10 :class:`~aenum.StrEnum`-valued. Plus **24** +non-registry enumerations counted by a runtime walk over both the :mod:`enum` and +:mod:`aenum` flavours and including nested classes: 17 top level (3 of them under +:mod:`pcapkit.const` itself) and 7 nested, the nested ones being +``FrameType.Flags`` in :mod:`pcapkit.protocols.schema.application.httpv2` plus its +6 concrete per-frame subclasses. 151 enumerations in total. + +**The** :class:`int`\ **-valued tier, all 117, is case-sensitive, and the criterion is +vacuous on it rather than merely unmet.** A registry whose values are numbers has +nothing for case to apply to; the only way a string reaches +:meth:`~pcapkit.corekit.enum.EnumLookup.get` on one is as a member *name*, and a name +is the Python identifier :meth:`pcapkit.vendor.default.Vendor.safe_name` derives from +the registry's own name column -- it preserves the registrar's casing exactly, but it +is not itself a value any specification states a comparison rule for. Measured across +all 151 enumerations: exactly **one** would collide if names were folded -- +:class:`~pcapkit.const.hip.parameter.Parameter`, on ``R1_Counter`` against +``R1_COUNTER`` -- and **no** enumeration anywhere has two ``str`` *values* that +collide when folded. So folding names is not merely unjustified, it is unsafe in a +measured case; folding values is safe but unjustified except where the table below +says otherwise. + +That leaves the classes with something to decide: - Auditing every registry's existing case behaviour against the specification its - values come from is `#903 `__, - not something this page records per class. The owner's scope for it, verbatim: - *"we should audit all registries and then decide if case (in)sensitive."* Two - findings already have their own issues -- - :class:`~pcapkit.const.ftp.command.Command`'s ``value.upper()`` is backed by - :rfc:`959#section-4.1` (*"Upper and lower case alphabetic characters are to be - treated identically"*), while - :class:`~pcapkit.const.http.method.Method`'s ``key.upper()`` contradicts - :rfc:`9110#section-9.1`, which makes the method token case-sensitive, and is tracked - as `#896 `__. +.. list-table:: + :header-rows: 1 + :widths: 22 20 40 18 + + * - Class + - Governing source + - What it says + - Verdict + * - :class:`~pcapkit.const.ftp.command.Command` + - :rfc:`959#section-4.1` + - *"Upper and lower case alphabetic characters are to be treated + identically."* Limb 1. + - **case-insensitive** -- ``get``/``_missing_`` fold, correctly + * - :class:`~pcapkit.const.ftp.command.FEATCode` + - :rfc:`5797#section-2`, :rfc:`2389#section-3.2` + - *"IANA maintains uniqueness of feature names (FEAT codes) based on + case-insensitive comparison."* Limb 1. Limb 2 holds too: RFC 2389 recommends + upper case on the wire while the registry spells 5 of its 15 codes lower case + (measured: of 64 rows, 11 upper-case / 10 distinct, 52 lower-case / 5 + distinct, 1 blank, 0 mixed). Read §3.2 to the end before concluding it + disagrees: it *opens* by calling the feature-label *"nominally case + sensitive"*, then defers to *"the definitions of specific labels"*, which + RFC 5797 §2 above is. Note also that the §2 sentence is wrapped across a + line break in the RFC's text file, at ``case-`` / ``insensitive``, so a + line-oriented grep for the phrase finds nothing. + - **case-insensitive** -- was a defect; ``get`` now folds + * - :class:`~pcapkit.const.http.method.Method` + - :rfc:`9110#section-9.1` + - *"The method token is case-sensitive."* Explicitly the opposite of limb 1. + - **case-sensitive** -- was a defect, fixed by + `#896 `__ + * - :class:`~pcapkit.const.pcapng.option_type.OptionType` + - ``draft-tuexen-opsawg-pcapng`` + - Nothing states a rule; the draft never discusses option-name case. + - **case-sensitive** -- already exact-matches, conforms + * - :class:`~pcapkit.const.pcapng.tls_key_label.TLSKeyLabel` + - :rfc:`9850#section-4.2` + - Nothing states a rule. Limb 2 fails on measurement: all 10 rows of the RFC's + table and all 10 of the live IANA CSV are upper case, so the authorities + agree. (RFC 9850 notes the labels *"correspond to lowercase labels in the TLS + key schedule"*, but those are a different document's secret names, not a + second spelling of the log label.) + - **case-sensitive** -- no override, conforms + * - ``TransportProtocol`` + - :rfc:`6335#section-8.1.1` + - Nothing states a rule for the ``Transport Protocol`` field -- only *"limited + to one or more of TCP, UDP, SCTP, and DCCP"*. Limb 2 carries it: the RFC and + its §10.2 templates write the field upper case, the live CSV is lower case in + all 14,536 rows (``tcp`` 6608, ``udp`` 6357, blank 1467, ``sctp`` 93, + ``dccp`` 11, zero upper-case). + - **case-insensitive** -- ``get`` folds, and this is the owner's own example + * - :class:`~pcapkit.const.reg.apptype.apptype.AppType` + - -- + - Moot: its ``get`` takes a port number and refuses a non-:class:`int` outright, + so there is no string to fold. It does inherit the row above through + ``_dispatch``, which resolves a ``proto`` string via ``TransportProtocol.get``. + - **n/a** -- int-keyed + * - ``TCP``, ``UDP``, ``SCTP``, ``DCCP`` + - :rfc:`6335#section-5.1` + - *"case is ignored for comparison purposes, so both "http" and "HTTP" denote + the same service."* Limb 1, emphatically -- and these registries' **values + are** service names. + - **unimplemented** -- no service-name lookup exists to fold; see below + * - ``CommandType``, ``ConformanceRequirement`` + - :rfc:`959#section-4.1`, :rfc:`5797#section-2` + - Limb 2 holds on measurement: the RFC and registry pages present the kind and + conformance letters upper case (``A``/``P``/``S``, ``M``/``O``/``H``) while + the CSV columns the crawler reads are lower case in every row (``s`` 26, + ``a`` 18, ``s/p`` 3, blank 1; ``o`` 28, ``m`` 27, ``h`` 7, ``m [1]`` 2). + - **open** -- see below + * - The 5 :mod:`~pcapkit.protocols.internet.mh` and + :mod:`~pcapkit.protocols.application.ngap` helper enumerations + - IANA Mobility Header registries, 3GPP TS 38.413 + - Their values are numeric codes, so the criterion is vacuous exactly as for the + :class:`int` tier above. Three of them -- ``Criticality``, + ``FastBindingAcknowledgmentStatus``, ``IPv6AddressPrefixCode`` -- do override + ``get``, but for signature reasons (no ``default``, and an + :class:`int`/:class:`str` dispatch) rather than for case: each does an exact + ``Cls[key]``. ``LMAAddressCode`` and ``LocalizedRoutingStatus`` carry no + ``get`` at all, so they have no string lookup to fold. Verified by reading all + five. + - **case-sensitive** -- conforms + * - ``WireGuardKeyLabel`` + - ``draft-tuexen-opsawg-pcapng`` + - The draft names the four labels outright (*"The key type is one of + LOCAL_STATIC_PRIVATE_KEY, ..."*) and, as for ``OptionType`` above, never + discusses their case. Both authorities write them upper case. + - **case-sensitive** -- no override, conforms + * - Every other non-registry enumeration + - -- + - pcapkit's own discriminators and bit labels, with no registrar behind them + at all -- ``Completion``, ``ftp.Type``, ``httpv1.Type``, ``FinalisedState``, + ``ESPStatus``, ``PacketDirection``, ``PacketReception``, and the 7 httpv2 + ``Flags``. ``PDUKind`` is the one with an external source and it points the + same way: its values are ASN.1 identifiers from 3GPP TS 38.413, and ASN.1 + identifiers are case-significant by construction. + - **case-sensitive** -- nothing to cite, nothing to change + +Two rows the audit deliberately left open rather than acting on, because each is +wider than a case fix: + +* **A service-name lookup on the** ``AppType`` **transport registries.** This is the + inverse of every other row: :rfc:`6335#section-5.1` *does* make service names + case-insensitive, and ``TCP``/``UDP``/``SCTP``/``DCCP`` hold service names as their + values -- but ``AppType.get`` refuses a non-:class:`int` key, so no service-name + lookup exists for the rule to apply to. Implementing one is new public API on a + 6,000-member registry where one name maps to many ports, which is a ``get_all`` + design question rather than a case fold. +* ``CommandType`` **and** ``ConformanceRequirement``. By parity with + ``TransportProtocol`` -- an :class:`int`-valued enumeration whose *names* are the + specification's own tokens -- the measured spelling disagreement above would make + these two case-insensitive. Nothing looks them up by string today, though: the + crawler translates the CSV's lower-case letters to the upper-case member names at + generation time, and neither class inherits + :class:`~pcapkit.corekit.enum.EnumLookup` yet. Re-parenting them is phase 2 of + `#877 `__, which is where the + question belongs. + +One case fold also lives **outside** any ``get``, and so escapes this convention +entirely: ``_resolve`` in :mod:`pcapkit.protocols.internet.esp` upper-cases its +``value`` before matching it against :class:`~pcapkit.const.esp.cipher.Cipher` and +:class:`~pcapkit.const.esp.integrity.Integrity` member names. :rfc:`7296` states no +comparison rule for IKEv2 transform names -- checked, it does not discuss case at all +-- so that fold is a convenience with no citation behind it. It is a protocol-level +resolver rather than a registry override, which is why the audit records it here +rather than changing it. .. note:: @@ -368,12 +545,22 @@ resolvable. >>> FEATCode.get('') - Measure it on a registry that does **not** override ``get``. Four do -- + The example above is :class:`~pcapkit.const.ftp.command.FEATCode`'s shape, and it + still resolves exactly as shown -- but since + `#903 `__ that class overrides + ``get`` too, so the output is only the base's because its override delegates an + exact name-or-value hit straight through. Measure the base on a registry that does + **not** override ``get`` at all. **Five** do -- :class:`~pcapkit.const.ftp.command.Command`, + :class:`~pcapkit.const.ftp.command.FEATCode`, :class:`~pcapkit.const.http.method.Method`, :class:`~pcapkit.const.pcapng.option_type.OptionType` and :class:`~pcapkit.const.reg.apptype.apptype.AppType` -- and probing one of those - measures the override rather than the base. ``Command.get`` upper-cases its key + measures the override rather than the base. The unconditionally clean witness is + ``tests/corekit/test_enum_lookup_base_unit.py``'s own ``_Str``, a purpose-built + closed set carrying ``angled = ''`` precisely so that the value + fall-through can be measured on a class that defines no ``get``. + ``Command.get`` upper-cases its key before matching, which makes it look as though the base were case-insensitive -- deliberately, since :rfc:`959#section-4.1` treats FTP command codes identically regardless of case. ``Method.get`` used to fold case the same way, but diff --git a/pcapkit/const/ftp/command.py b/pcapkit/const/ftp/command.py index 96c2c20ab8..708f8f308b 100644 --- a/pcapkit/const/ftp/command.py +++ b/pcapkit/const/ftp/command.py @@ -15,10 +15,10 @@ from aenum import IntEnum, IntFlag, StrEnum, auto -from pcapkit.corekit.enum import EnumRegistry +from pcapkit.corekit.enum import NO_DEFAULT, EnumRegistry if TYPE_CHECKING: - from typing import Optional, Type + from typing import Any, Optional, Type __all__ = ['Command'] @@ -91,6 +91,85 @@ class FEATCode(EnumRegistry, StrEnum): def __repr__(self) -> 'str': return f'<{self.__class__.__name__} [{self._name_}]>' + @classmethod + def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'FEATCode': + """Resolve ``key`` case-insensitively, per :rfc:`5797#section-2`. + + One of the few case-insensitive overrides the ruling on GitHub issue + #877 allows, and the registry's own defining document states the + comparison rule outright rather than leaving it to be inferred -- + :rfc:`5797#section-2`, on the ``FEAT Code`` column this class is + generated from: *"IANA maintains uniqueness of feature names (FEAT + codes) based on case-insensitive comparison."* Two FEAT codes + therefore cannot differ only by case, so folding the caller's key + cannot resolve ambiguously. + + :rfc:`2389#section-3.2`, which defines the ``FEAT`` response the + codes appear in, points the same way from the wire side: *"The + feature-label and feature-parms are nominally case sensitive, + however ... it is to be expected that those definitions will usually + specify the label and parameters in a case independent manner. Where + this is done, implementations are recommended to use upper case + letters when transmitting the feature response."* A caller feeding a + ``FEAT`` response line is therefore holding upper case by + recommendation, while 5 of this registry's 15 codes are registered in + lower case -- measured on the IANA CSV: 10 distinct all-upper-case + keywords against ``base``, ``feat``, ``hist``, ``nat6`` and ``secu``, + with none mixed. Without this override ``get('BASE')`` raised + :exc:`KeyError`, which is the defect GitHub issue #903's audit found. + + The obvious objection, answered: :rfc:`5797` uses case *presentationally* + to tell a real keyword from a placeholder -- *"defined FEAT keywords + codes are listed in all uppercase, whereas placeholder keywords ... are + listed in lowercase"* -- so folding might look like it discards that + distinction. It does not. Only the inbound ``key`` is folded; every + member keeps the registrar's own casing, per the same ruling's *"enum + should honour and keep their original writings as in the registrars"*, + so ``get('BASE').name`` is still ``'base'`` and still says placeholder. + And the uniqueness rule quoted above is what makes that safe: a real + keyword ``BASE`` could not be registered alongside the placeholder + ``base``, so there is no second member for the fold to hide. + + Folds only as a *fallback*. An exact name or value hit is delegated to + :meth:`~pcapkit.corekit.enum.EnumLookup.get` untouched, so the base's + own precedence -- name before value -- and its non-minting ``str`` + path both survive: nothing here calls ``cls(key)``, so a key matching + no member, folded or not, still raises rather than growing the + registry. ``get('ZZ-NOT-REAL')`` therefore still raises + :exc:`KeyError` while ``FEATCode('ZZ-NOT-REAL')`` still yields an + unregistered member, exactly as before. + + Args: + key: Name or value to look up. A non-``str`` key is passed + straight through, since case cannot apply to it. + default: As :meth:`~pcapkit.corekit.enum.EnumLookup.get`. Not + folded -- it names an already-registered value rather than + arriving from the wire, so the caller spells it from this + module. + + Returns: + The canonical member for ``key``, or for ``default``. + + Raises: + KeyError: If no member matches ``key`` exactly or case-insensitively + and there is no usable ``default``. + ValueError: As :meth:`~pcapkit.corekit.enum.EnumLookup.get`, for a + non-``str`` key. + + """ + if isinstance(key, str) and not ( + key in cls._member_map_ or # pylint: disable=no-member + key in cls._value2member_map_ + ): + folded = key.casefold() + for name, member in cls._member_map_.items(): # pylint: disable=no-member + if name.casefold() == folded: + return member + for member in cls._value2member_map_.values(): + if member.value.casefold() == folded: + return member + return super().get(key, default) + @classmethod def _missing_(cls, value: 'str') -> 'FEATCode': """Lookup function used when value is not found. diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index 0e8ae2b21d..900b26a3d4 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -432,7 +432,10 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': unrecognised value, routing a failed *name* lookup through the constructor would let a mere ``get()`` call mint a permanent member where it previously just raised. Defensive rather than observed: of - the 125 classes that reach this method, the ``str``-valued ones + the 127 classes that reach this method -- 125 until GitHub issue #880's + own PR added ``pcapkit/const/ngap/procedure_code.py`` and + ``pcapkit/const/ngap/protocol_ie.py``, remeasured while auditing + GitHub issue #903 -- the ``str``-valued ones (:class:`~pcapkit.const.ftp.command.Command`, :class:`~pcapkit.const. ftp.command.FEATCode`, :class:`~pcapkit.const.http.method.Method`, :class:`~pcapkit.const.pcapng.option_type.OptionType`, diff --git a/pcapkit/vendor/ftp/command.py b/pcapkit/vendor/ftp/command.py index 87c6420aed..9f662965ae 100644 --- a/pcapkit/vendor/ftp/command.py +++ b/pcapkit/vendor/ftp/command.py @@ -55,10 +55,10 @@ from aenum import IntEnum, IntFlag, StrEnum, auto -from pcapkit.corekit.enum import EnumRegistry +from pcapkit.corekit.enum import NO_DEFAULT, EnumRegistry if TYPE_CHECKING: - from typing import Optional, Type + from typing import Any, Optional, Type __all__ = ['{NAME}'] @@ -103,6 +103,85 @@ class FEATCode(EnumRegistry, StrEnum): def __repr__(self) -> 'str': return f'<{{self.__class__.__name__}} [{{self._name_}}]>' + @classmethod + def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'FEATCode': + """Resolve ``key`` case-insensitively, per :rfc:`5797#section-2`. + + One of the few case-insensitive overrides the ruling on GitHub issue + #877 allows, and the registry's own defining document states the + comparison rule outright rather than leaving it to be inferred -- + :rfc:`5797#section-2`, on the ``FEAT Code`` column this class is + generated from: *"IANA maintains uniqueness of feature names (FEAT + codes) based on case-insensitive comparison."* Two FEAT codes + therefore cannot differ only by case, so folding the caller's key + cannot resolve ambiguously. + + :rfc:`2389#section-3.2`, which defines the ``FEAT`` response the + codes appear in, points the same way from the wire side: *"The + feature-label and feature-parms are nominally case sensitive, + however ... it is to be expected that those definitions will usually + specify the label and parameters in a case independent manner. Where + this is done, implementations are recommended to use upper case + letters when transmitting the feature response."* A caller feeding a + ``FEAT`` response line is therefore holding upper case by + recommendation, while 5 of this registry's 15 codes are registered in + lower case -- measured on the IANA CSV: 10 distinct all-upper-case + keywords against ``base``, ``feat``, ``hist``, ``nat6`` and ``secu``, + with none mixed. Without this override ``get('BASE')`` raised + :exc:`KeyError`, which is the defect GitHub issue #903's audit found. + + The obvious objection, answered: :rfc:`5797` uses case *presentationally* + to tell a real keyword from a placeholder -- *"defined FEAT keywords + codes are listed in all uppercase, whereas placeholder keywords ... are + listed in lowercase"* -- so folding might look like it discards that + distinction. It does not. Only the inbound ``key`` is folded; every + member keeps the registrar's own casing, per the same ruling's *"enum + should honour and keep their original writings as in the registrars"*, + so ``get('BASE').name`` is still ``'base'`` and still says placeholder. + And the uniqueness rule quoted above is what makes that safe: a real + keyword ``BASE`` could not be registered alongside the placeholder + ``base``, so there is no second member for the fold to hide. + + Folds only as a *fallback*. An exact name or value hit is delegated to + :meth:`~pcapkit.corekit.enum.EnumLookup.get` untouched, so the base's + own precedence -- name before value -- and its non-minting ``str`` + path both survive: nothing here calls ``cls(key)``, so a key matching + no member, folded or not, still raises rather than growing the + registry. ``get('ZZ-NOT-REAL')`` therefore still raises + :exc:`KeyError` while ``FEATCode('ZZ-NOT-REAL')`` still yields an + unregistered member, exactly as before. + + Args: + key: Name or value to look up. A non-``str`` key is passed + straight through, since case cannot apply to it. + default: As :meth:`~pcapkit.corekit.enum.EnumLookup.get`. Not + folded -- it names an already-registered value rather than + arriving from the wire, so the caller spells it from this + module. + + Returns: + The canonical member for ``key``, or for ``default``. + + Raises: + KeyError: If no member matches ``key`` exactly or case-insensitively + and there is no usable ``default``. + ValueError: As :meth:`~pcapkit.corekit.enum.EnumLookup.get`, for a + non-``str`` key. + + """ + if isinstance(key, str) and not ( + key in cls._member_map_ or # pylint: disable=no-member + key in cls._value2member_map_ + ): + folded = key.casefold() + for name, member in cls._member_map_.items(): # pylint: disable=no-member + if name.casefold() == folded: + return member + for member in cls._value2member_map_.values(): + if member.value.casefold() == folded: + return member + return super().get(key, default) + @classmethod def _missing_(cls, value: 'str') -> 'FEATCode': """Lookup function used when value is not found. diff --git a/tests/const/test_const_enum_no_mint.py b/tests/const/test_const_enum_no_mint.py index 4d2f151788..49d53059ee 100644 --- a/tests/const/test_const_enum_no_mint.py +++ b/tests/const/test_const_enum_no_mint.py @@ -92,8 +92,11 @@ and :class:`Method` needs ``safe``/``idempotent``, neither of which a bare wire string carries -- so both classes' own ``get()`` (a second, independent mint site bypassing ``_missing_`` entirely) converts too, alongside -``_missing_``; :class:`FEATCode` has no custom ``__new__`` and no ``get()`` -of its own, so only its one ``_missing_`` branch was in play. +``_missing_``; :class:`FEATCode` has no custom ``__new__``, and had no +``get()`` of its own at the time either, so only its one ``_missing_`` branch +was in play. (It has one now -- GitHub issue #903's audit gave it a +case-insensitive ``get`` per :rfc:`5797#section-2` -- but that override never +calls ``cls(key)``, so it added no mint site to this file's concern.) GitHub issue #860 step 2's PR 2 has now converted the last of the 9: :class:`~pcapkit.const.reg.apptype.apptype.AppType` and its four per-transport diff --git a/tests/const/test_const_ftp_featcode_case_903_unit.py b/tests/const/test_const_ftp_featcode_case_903_unit.py new file mode 100644 index 0000000000..cdb6d64091 --- /dev/null +++ b/tests/const/test_const_ftp_featcode_case_903_unit.py @@ -0,0 +1,329 @@ +# -*- coding: utf-8 -*- +"""``FEATCode.get`` must be case-insensitive, per RFC 5797 Section 2. + +GitHub issue #903, the registry-wide case-sensitivity audit. Of the 127 +registries under :mod:`pcapkit.const` and the 24 non-registry enumerations +elsewhere, :class:`~pcapkit.const.ftp.command.FEATCode` is the one the audit +found on the wrong side of the owner's ruling: it did no case folding at all, +so it ran the case-sensitive default, while the document defining the very +registry it is generated from states the *opposite* comparison rule outright. + +RFC 5797 Section 2, describing the ``FEAT Code`` column of the IANA "FTP +Commands and Extensions" registry that +:class:`pcapkit.vendor.ftp.command.Command` crawls:: + + ... but otherwise IANA maintains uniqueness of feature names (FEAT codes) + based on case-insensitive comparison. + +That is the strict limb of the criterion -- a comparison rule in the spec -- +and the lenient limb holds too. RFC 2389 Section 3.2, which defines the +``FEAT`` response these codes appear in:: + + The feature-label and feature-parms are nominally case sensitive, however + the definitions of specific labels and parameters specify the precise + interpretation, and it is to be expected that those definitions will + usually specify the label and parameters in a case independent manner. + Where this is done, implementations are recommended to use upper case + letters when transmitting the feature response. + +so a caller holding a line off a ``FEAT`` response holds upper case *by the +RFC's own recommendation*, while the registry spells 5 of its 15 codes in +lower case -- because RFC 5797 uses case presentationally, to tell a +registered keyword from a placeholder:: + + ... defined FEAT keywords codes are listed in all uppercase, whereas + placeholder keywords (henceforth called "pseudo FEAT codes") are listed + in lowercase. + +Measured on the live registry CSV while auditing: of 64 rows, 11 carry an +all-upper-case code (10 distinct -- ``AUTH``, ``HOST``, ``MDTM``, ``MLST``, +``PBSZ``, ``PROT``, ``REST``, ``SIZE``, ``TVFS``, ``UTF8``), 52 carry an +all-lower-case one (5 distinct -- ``base``, ``feat``, ``hist``, ``nat6``, +``secu``), 1 is blank, and **none** is mixed. So the two authorities +disagree about the casing of the same field, which is exactly the shape the +owner's ruling on this issue calls case-insensitive, verbatim: *"I say +lenient. TransportProtocol for example should be case-insensitive. Upper or +lower cases are being used everywhere in RFC and IANA themselves so that's an +indication of case insensitivity."* + +**Pre-change behaviour, measured before the fix** (throwaway process, no +probe that could mint; ``_member_map_`` and ``_value2member_map_`` both 15 +entries before and after):: + + FEATCode.get('base' ) -> value='' + FEATCode.get('BASE' ) -> KeyError: 'BASE' + FEATCode.get('Base' ) -> KeyError: 'Base' + FEATCode.get('') -> value='' + FEATCode.get('') -> KeyError: '' + FEATCode.get('AUTH' ) -> value='AUTH' + FEATCode.get('auth' ) -> KeyError: 'auth' + FEATCode.get('Auth' ) -> KeyError: 'Auth' + +**What the fix deliberately does not do.** It does not fold the stored +members, and it does not rename one. Every member keeps the registrar's own +casing, per the ruling on the *Registry Conventions* page -- *"enum +should honour and keep their original writings as in the registrars"* -- so +``FEATCode.get('BASE').name`` is still ``'base'`` and still says +*placeholder*. Nor does it fold the *value* a lookup resolves to, which is +what would have made the fold lossy. Only the inbound key is folded, and only +after an exact name-or-value match has already missed, so +:meth:`~pcapkit.corekit.enum.EnumLookup.get`'s own precedence (name before +value) and its non-minting ``str`` path both survive untouched. RFC 5797's +uniqueness rule is what makes the fold unambiguous rather than merely +convenient: a registered ``BASE`` cannot coexist with the placeholder +``base``, so there is no second member for the fold to hide -- pinned below +by :meth:`FEATCodeCaseFoldSafetyTests.test_no_two_members_collide_when_folded`. + +**Contrast, pinned so the default is not quietly widened.** The audit's other +``str``-valued registries stay case-sensitive and each has a reason: +:class:`~pcapkit.const.pcapng.tls_key_label.TLSKeyLabel`, whose RFC 9850 +Section 4.2 registry and live IANA CSV agree on upper case in all 10 rows +with no comparison rule stated anywhere, is measured below as the witness +that the base is untouched by this change. +""" + +import inspect +import unittest + +from pcapkit.const.ftp.command import Command, FEATCode +from pcapkit.const.pcapng.tls_key_label import TLSKeyLabel + +#: The 5 placeholder ("pseudo FEAT code") members, registered in lower case +#: per RFC 5797 Section 2, with the ``<...>`` value form the crawler emits. +PSEUDO_CODES = ('base', 'hist', 'secu', 'feat', 'nat6') + +#: The 10 genuine FEAT keywords, registered in upper case per the same section. +REAL_CODES = ('AUTH', 'HOST', 'UTF8', 'MDTM', 'MLST', 'PBSZ', 'PROT', 'REST', + 'SIZE', 'TVFS') + + +class FEATCodeCaseInsensitiveLookupTests(unittest.TestCase): + """``get`` resolves a FEAT code whatever case the caller spells it in.""" + + def test_a_lower_case_placeholder_resolves_from_upper_case(self) -> 'None': + """The issue's repro. ``get('BASE')`` used to raise ``KeyError``. + + Upper case is what RFC 2389 Section 3.2 recommends implementations + transmit, so this is the casing a caller reading a real ``FEAT`` + response is most likely to hold. + + """ + for key in ('BASE', 'Base', 'bAsE', 'base'): + with self.subTest(key=key): + self.assertIs(FEATCode.get(key), FEATCode.base) # type: ignore[attr-defined] + + def test_every_placeholder_resolves_from_either_case(self) -> 'None': + """All 5 of them, not just the one the issue names.""" + for name in PSEUDO_CODES: + member = FEATCode[name] + for key in (name, name.upper(), name.capitalize()): + with self.subTest(code=name, key=key): + self.assertIs(FEATCode.get(key), member) + + def test_an_upper_case_keyword_resolves_from_lower_case(self) -> 'None': + """The fold runs in both directions, not only lower -> upper.""" + for name in REAL_CODES: + member = FEATCode[name] + for key in (name, name.lower(), name.capitalize()): + with self.subTest(code=name, key=key): + self.assertIs(FEATCode.get(key), member) + + def test_the_value_form_folds_too(self) -> 'None': + """A placeholder's *value* carries the angle brackets, and folds. + + ``FEATCode.base`` is the member whose value is ``''`` and whose + name is ``'base'`` -- the shape + the *Registry Conventions* page uses to demonstrate the base's + name-misses-then-value-matches fall-through. Folding has to reach the + value side as well, or a caller holding the registry's own value + string in the RFC's recommended casing still fails. + + """ + for key in ('', '', ''): + with self.subTest(key=key): + self.assertIs(FEATCode.get(key), FEATCode.base) # type: ignore[attr-defined] + + def test_get_all_inherits_the_fold(self) -> 'None': + """:meth:`~pcapkit.corekit.enum.EnumLookup.get_all` resolves through + ``get``, so it gains the fold without its own override -- and still + returns the one-entry tuple a registry mapping one key to one member + should, rather than one entry per casing tried.""" + self.assertEqual(FEATCode.get_all('BASE'), + (FEATCode.base,)) # type: ignore[attr-defined] + self.assertEqual(FEATCode.get_all('auth'), + (FEATCode.AUTH,)) # type: ignore[attr-defined] + + def test_exact_hits_are_unchanged_for_every_member(self) -> 'None': + """The fold is a fallback: an exact name or value still resolves the + way it did before, through the base, for all 15 members.""" + self.assertEqual(len(FEATCode._member_names_), 15) + for member in FEATCode: + with self.subTest(member=member.name): + self.assertIs(FEATCode.get(member.name), member) + self.assertIs(FEATCode.get(member.value), member) + + +class FEATCodeCaseFoldSafetyTests(unittest.TestCase): + """The fold cannot resolve ambiguously, and cannot mint.""" + + def test_no_two_members_collide_when_folded(self) -> 'None': + """RFC 5797's uniqueness rule, checked against the generated data. + + *"IANA maintains uniqueness of feature names (FEAT codes) based on + case-insensitive comparison."* If that ever stopped holding in the + crawled registry, folding would start resolving one of the colliding + pair arbitrarily -- so it is asserted rather than assumed. Measured + across all 151 enumerations in the tree while auditing #903: + :class:`~pcapkit.const.hip.parameter.Parameter` is the *only* one with + a name-fold collision (``R1_Counter`` against ``R1_COUNTER``, both + IANA-registered), and **no** enumeration anywhere has a ``str``-value + fold collision. + + """ + names = list(FEATCode._member_map_) + folded_names = [name.casefold() for name in names] + self.assertEqual(len(set(folded_names)), len(set(names))) + + values = [member.value for member in FEATCode] + folded_values = [value.casefold() for value in values] + self.assertEqual(len(set(folded_values)), len(set(values))) + + def test_a_folded_name_never_shadows_another_members_value(self) -> 'None': + """Name-before-value precedence is safe because the two never cross. + + The base checks a ``str`` key against member names first and member + values second. The fold keeps that order, which would only be + observable if one member's folded *name* equalled a *different* + member's folded *value*. It does not, for any pair here -- so the + fold cannot change which member a key resolves to relative to the + exact-match path it falls back from. + + """ + by_folded_name = {name.casefold(): FEATCode._member_map_[name] + for name in FEATCode._member_map_} + for member in FEATCode: + folded_value = member.value.casefold() + if folded_value in by_folded_name: + with self.subTest(member=member.name): + self.assertIs(by_folded_name[folded_value], member) + + def test_an_unknown_code_still_raises_rather_than_minting(self) -> 'None': + """The asymmetry the *Registry Conventions* page records survives. + + ``get`` never calls ``cls(key)`` for a ``str``, so a key matching no + member -- in any casing -- raises instead of growing the registry, + while the *constructor* still yields an unregistered member. Folding + adds a second way to match, never a way to mint. + + """ + before_names = set(FEATCode._member_map_) + before_values = set(FEATCode._value2member_map_) + + for key in ('ZZ-NOT-REAL', 'zz-not-real', 'Zz-Not-Real'): + with self.subTest(key=key): + with self.assertRaises(KeyError): + FEATCode.get(key) + + unregistered = FEATCode('ZZ-NOT-REAL') + self.assertEqual(unregistered.value, 'ZZ-NOT-REAL') + self.assertNotIn('ZZ-NOT-REAL', FEATCode._member_map_) + + self.assertEqual(set(FEATCode._member_map_), before_names) + self.assertEqual(set(FEATCode._value2member_map_), before_values) + + def test_default_still_resolves_and_is_not_itself_folded(self) -> 'None': + """``default`` names an already-registered value from this module. + + It is spelled by the caller against the enumeration rather than + arriving off the wire, so it goes to the base untouched -- which also + keeps it on the base's non-minting ``_value2member_map_`` path. + + """ + self.assertIs(FEATCode.get('ZZ-NOT-REAL', ''), + FEATCode.base) # type: ignore[attr-defined] + with self.assertRaises(KeyError): + FEATCode.get('ZZ-NOT-REAL', '') + with self.assertRaises(KeyError): + FEATCode.get('ZZ-NOT-REAL', 'ALSO-NOT-REAL') + + def test_a_non_str_key_is_passed_straight_through(self) -> 'None': + """Case cannot apply to an :obj:`int`, so the fold must not intercept + it -- and a ``str`` registry has no member for one, so the base's + ``ValueError`` has to reach the caller.""" + with self.assertRaises(ValueError): + FEATCode.get(42) + + +class CaseSensitiveDefaultStillHoldsTests(unittest.TestCase): + """The base, and the other audited registries, are untouched.""" + + def test_tls_key_label_stays_case_sensitive(self) -> 'None': + """The audit's witness that the default is unchanged. + + :class:`~pcapkit.const.pcapng.tls_key_label.TLSKeyLabel` carries no + ``get`` of its own, so it runs + :meth:`~pcapkit.corekit.enum.EnumLookup.get` unmodified. RFC 9850 + Section 4.2's "TLS SSLKEYLOGFILE Labels" registry lists all 10 labels + in upper case, the live IANA CSV agrees on all 10, and neither states + a comparison rule -- so neither limb of the criterion is met and the + case-sensitive default is correct here. + + """ + self.assertNotIn('get', vars(TLSKeyLabel)) + self.assertIs(TLSKeyLabel.get('CLIENT_RANDOM'), + TLSKeyLabel.CLIENT_RANDOM) # type: ignore[attr-defined] + for key in ('client_random', 'Client_Random'): + with self.subTest(key=key): + with self.assertRaises(KeyError): + TLSKeyLabel.get(key) + + def test_command_keeps_its_own_rfc_959_fold(self) -> 'None': + """:class:`~pcapkit.const.ftp.command.Command` shares this module and + folds for a different reason -- RFC 959 Section 4.1's *"Upper and + lower case alphabetic characters are to be treated identically"* -- + so it must be unaffected by the sibling class changing.""" + self.assertIs(Command.get('RETR'), Command.RETR) # type: ignore[attr-defined] + self.assertIs(Command.get('retr'), Command.RETR) # type: ignore[attr-defined] + + +class VendorTemplateParityTests(unittest.TestCase): + """The fix lives in the crawler, so a regeneration cannot undo it.""" + + def test_the_crawler_template_carries_the_same_get(self) -> 'None': + """House rule: a change to a generated registry's shape belongs in the + crawler, never in the generated file alone. + + :class:`FEATCode` is written out longhand inside + :data:`pcapkit.vendor.ftp.command.LINE`, so this renders that template + and requires the generated module's own ``get`` source to appear in it + verbatim. Importing the crawler module reads its module-level + template only; the crawl itself is behind ``if __name__ == + '__main__'`` and is never run here, so this needs no network access. + + """ + from pcapkit.vendor.ftp.command import LINE + + rendered = LINE('Command', 'FTP Command', '', '', + 'pcapkit.vendor.ftp.command') + source = inspect.getsource(FEATCode.get.__func__) # type: ignore[attr-defined] + + self.assertIn('IANA maintains uniqueness of feature names', source) + self.assertIn(source.rstrip('\n'), rendered) + + def test_the_generated_module_imports_what_the_override_needs(self) -> 'None': + """``NO_DEFAULT`` is the sentinel the override's own signature carries, + so the template has to import it as well as emit the method.""" + from pcapkit.vendor.ftp.command import LINE + + rendered = LINE('Command', 'FTP Command', '', '', + 'pcapkit.vendor.ftp.command') + self.assertIn('from pcapkit.corekit.enum import NO_DEFAULT, EnumRegistry', + rendered) + + import pcapkit.const.ftp.command as generated + self.assertIn('from pcapkit.corekit.enum import NO_DEFAULT, EnumRegistry', + inspect.getsource(generated)) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/const/test_const_method_case_sensitive_896_unit.py b/tests/const/test_const_method_case_sensitive_896_unit.py index c481090c46..9b92aeb0ef 100644 --- a/tests/const/test_const_method_case_sensitive_896_unit.py +++ b/tests/const/test_const_method_case_sensitive_896_unit.py @@ -28,8 +28,11 @@ neither a member name nor an already-registered value, and with no ``default`` supplied, ``EnumRegistry.get`` raises :exc:`KeyError` -- it never builds an unregistered member the way ``Method._missing_`` does for the -*constructor* path. Confirmed on :class:`~pcapkit.const.ftp.command.FEATCode` -(no bespoke ``get`` of its own, so it already runs the base unmodified): +*constructor* path. Confirmed on :class:`~pcapkit.const.ftp.command.FEATCode`, +which ran the base unmodified when this was measured -- GitHub issue #903's +audit has since given it a case-insensitive ``get`` of its own per +:rfc:`5797#section-2`, and the measurement still stands, because that override +delegates to the base for any key it cannot match even after folding: ``FEATCode.get('totally-unknown-thing')`` raises ``KeyError``, while ``FEATCode('totally-unknown-thing')`` -- the constructor, reaching ``_missing_`` -- resolves to an unregistered member. Deleting ``Method.get``