From f4d82cc8d1aa63f2d05e852db91798adbad1a241 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 28 Sep 2026 08:46:45 -0400 Subject: [PATCH] fix(const,vendor): give Method's registered members their str payload (#870) `Method.__new__` built every registered member with `str.__new__(cls)` (no argument), so the underlying str payload was permanently empty regardless of the declared value -- `str(Method.GET) == ''` and `Method.GET == 'GET'` was False for all 40 members, even though `_value_`/`.value` were always correct. #869 fixed the same class's *unregistered* path only, which exposed the inconsistency: after #869, `str(Method('frob')) == 'frob'` but `str(Method.GET) == ''`. - Fix `__new__` to `str.__new__(cls, value)`, mirroring `Command.__new__`, which never had this defect. Fixed in both the generator (`pcapkit/vendor/http/method.py`) and the generated module (`pcapkit/const/http/method.py`); a second regeneration is byte-identical. - Surveyed every other hand-rolled `__new__` under `pcapkit/const/`: `Command` already passes its value, `FEATCode` has no custom `__new__`, and `OptionType`/`AppType` deliberately store a formatted display string as their value -- none share this defect. - Added `tests/const/test_const_str_payload_870_unit.py`: the exact repro, a sweep of all 40 `Method` members, and a registry-wide sweep of `Command`/`FEATCode`/`Method` (`OptionType` explicitly exempted, with a comment, since its value already *is* the display string). Verified each new assertion fails on unfixed `__new__` and passes with the fix. - Updated `tests/const/test_const_enum_no_mint.py`, which had pinned the old empty-payload behavior as an accepted-but-unfixed asymmetry; it now expects the fixed behavior. Behaviour change: `Method.GET == 'GET'` is now True (was False) for all 40 public members; no code under `pcapkit/protocols` compared a Method member this way. Build and targeted tests green (315 methods across const/http/ftp suites). --- pcapkit/const/http/method.py | 29 +-- pcapkit/vendor/http/method.py | 29 +-- tests/const/test_const_enum_no_mint.py | 33 ++- .../const/test_const_str_payload_870_unit.py | 191 ++++++++++++++++++ 4 files changed, 237 insertions(+), 45 deletions(-) create mode 100644 tests/const/test_const_str_payload_870_unit.py diff --git a/pcapkit/const/http/method.py b/pcapkit/const/http/method.py index 5b0a5780d3..434ab0450e 100644 --- a/pcapkit/const/http/method.py +++ b/pcapkit/const/http/method.py @@ -45,7 +45,7 @@ class Method(EnumRegistry, StrEnum): def __new__(cls, value: 'str', safe: 'bool' = False, idempotent: 'bool' = False) -> 'Type[Method]': - obj = str.__new__(cls) + obj = str.__new__(cls, value) obj._value_ = value obj.safe = safe @@ -201,20 +201,21 @@ def _unregistered_member(cls, value: 'str', name: 'str') -> 'Method': declared, never reformatted. Only :attr:`name` -- the identifier, not the value -- is canonicalised. - This is deliberately *not* the same as what a *registered* - member of this class carries, though, and that asymmetry is - left as-is here rather than fixed: :meth:`__new__` itself is - untouched, and it calls ``str.__new__(cls)`` with no + A *registered* member of this class used to carry something + different here: GitHub issue #870 found that + :meth:`__new__` called ``str.__new__(cls)`` with no argument at all, so every one of the 40 declared members' - underlying :class:`str` payload is permanently empty - regardless of ``value`` (``str(Method.GET) == ''``, and - ``Method.GET == 'GET'`` is :obj:`False`) -- true on ``main`` - as well as here. An *unregistered* member built through this - method, by contrast, now carries real content - (``str(Method('frob')) == 'frob'``). Tracked as GitHub issue - #870 rather than fixed in this PR: changing :meth:`__new__` - changes what all 40 public members compare equal to, which - is its own review. + own :class:`str` payload was permanently empty regardless + of ``value`` (``str(Method.GET) == ''``, and + ``Method.GET == 'GET'`` was :obj:`False`) -- true on + ``main`` at ``60b85e3a4`` as well as when this docstring + was first written. #870 fixed :meth:`__new__` to + ``str.__new__(cls, value)``, mirroring + :class:`~pcapkit.const.ftp.command.Command`'s own + ``__new__``, so a registered member's payload now agrees + with an *unregistered* member built through this method: + both carry their own value as real :class:`str` content + (``str(Method('frob')) == 'frob'``). name: Bare label for the unregistered member -- here, the canonical upper-case form of ``value``, matching the name every *registered* member of this class is looked up by, diff --git a/pcapkit/vendor/http/method.py b/pcapkit/vendor/http/method.py index f8121500bc..f4a83c0c01 100644 --- a/pcapkit/vendor/http/method.py +++ b/pcapkit/vendor/http/method.py @@ -69,7 +69,7 @@ class {NAME}(EnumRegistry, StrEnum): def __new__(cls, value: 'str', safe: 'bool' = False, idempotent: 'bool' = False) -> 'Type[{NAME}]': - obj = str.__new__(cls) + obj = str.__new__(cls, value) obj._value_ = value obj.safe = safe @@ -106,20 +106,21 @@ def _unregistered_member(cls, value: 'str', name: 'str') -> '{NAME}': declared, never reformatted. Only :attr:`name` -- the identifier, not the value -- is canonicalised. - This is deliberately *not* the same as what a *registered* - member of this class carries, though, and that asymmetry is - left as-is here rather than fixed: :meth:`__new__` itself is - untouched, and it calls ``str.__new__(cls)`` with no + A *registered* member of this class used to carry something + different here: GitHub issue #870 found that + :meth:`__new__` called ``str.__new__(cls)`` with no argument at all, so every one of the 40 declared members' - underlying :class:`str` payload is permanently empty - regardless of ``value`` (``str(Method.GET) == ''``, and - ``Method.GET == 'GET'`` is :obj:`False`) -- true on ``main`` - as well as here. An *unregistered* member built through this - method, by contrast, now carries real content - (``str(Method('frob')) == 'frob'``). Tracked as GitHub issue - #870 rather than fixed in this PR: changing :meth:`__new__` - changes what all 40 public members compare equal to, which - is its own review. + own :class:`str` payload was permanently empty regardless + of ``value`` (``str(Method.GET) == ''``, and + ``Method.GET == 'GET'`` was :obj:`False`) -- true on + ``main`` at ``60b85e3a4`` as well as when this docstring + was first written. #870 fixed :meth:`__new__` to + ``str.__new__(cls, value)``, mirroring + :class:`~pcapkit.const.ftp.command.Command`'s own + ``__new__``, so a registered member's payload now agrees + with an *unregistered* member built through this method: + both carry their own value as real :class:`str` content + (``str(Method('frob')) == 'frob'``). name: Bare label for the unregistered member -- here, the canonical upper-case form of ``value``, matching the name every *registered* member of this class is looked up by, diff --git a/tests/const/test_const_enum_no_mint.py b/tests/const/test_const_enum_no_mint.py index ebf194104a..bbe0aaf8db 100644 --- a/tests/const/test_const_enum_no_mint.py +++ b/tests/const/test_const_enum_no_mint.py @@ -1829,25 +1829,24 @@ def test_method_missing_no_longer_mints(self) -> None: """Same convention as :class:`~pcapkit.const.ftp.command.FEATCode`'s and :class:`~pcapkit.const.ftp.command.Command`'s: an *unregistered* member's value is the caller's own casing, with real :class:`str` - content. That leaves a genuine asymmetry with every *registered* - member of this class, tracked as GitHub issue #870 rather than - fixed here: :meth:`Method.__new__` is untouched, and it calls - ``str.__new__(cls)`` with no argument at all, so all 40 declared - members' own :class:`str` payload is permanently empty regardless - of value (``str(Method.GET) == ''``, ``Method.GET == 'GET'`` is - :obj:`False`) -- on ``main`` as well as here. This test is about - the *unregistered* path only, which -- because it bypasses - ``__new__`` entirely rather than being routed through its bug -- - gets real content where a *minted* lookup of the same unrecognised - word used to get the same permanently-empty payload on ``main`` - too.""" + content. This test used to also pin a genuine asymmetry with every + *registered* member of this class, tracked as GitHub issue #870: + :meth:`Method.__new__` called ``str.__new__(cls)`` with no argument + at all, so all 40 declared members' own :class:`str` payload was + permanently empty regardless of value (``str(Method.GET) == ''``, + ``Method.GET == 'GET'`` was :obj:`False`) -- true on ``main`` at + ``60b85e3a4`` as well as when this test was first written. #870 + fixed :meth:`Method.__new__` to ``str.__new__(cls, value)``, + mirroring :class:`~pcapkit.const.ftp.command.Command`'s own + ``__new__``, so a registered member now carries its value as its + :class:`str` payload too -- see + :mod:`tests.const.test_const_str_payload_870_unit` for the direct + pin of that fix, registry-wide. What is left here is what this + test was always really about: the *unregistered* path, which + already carried real content before #870 (it bypasses ``__new__`` + entirely) and is unaffected by that fix.""" from pcapkit.const.http.method import Method - # The asymmetry itself, pinned directly: a registered member's - # payload is still empty, unchanged and not addressed by this PR. - self.assertEqual(str(Method.GET), '') - self.assertNotEqual(Method.GET, 'GET') - before = len(Method.__members__) first = Method('pypcapkit860probe') after = len(Method.__members__) diff --git a/tests/const/test_const_str_payload_870_unit.py b/tests/const/test_const_str_payload_870_unit.py new file mode 100644 index 0000000000..b550018f9a --- /dev/null +++ b/tests/const/test_const_str_payload_870_unit.py @@ -0,0 +1,191 @@ +# -*- coding: utf-8 -*- +"""A registered ``StrEnum`` member's :class:`str` payload must be its value. + +GitHub issue #870: :meth:`pcapkit.const.http.method.Method.__new__` built every +registered member with ``obj = str.__new__(cls)`` -- no second argument -- so +the underlying :class:`str` content was permanently empty regardless of the +member's own declared value. ``_value_`` was set correctly (``Method.GET.value +== 'GET'``), but the :class:`str` the member itself *is* was not: +``str(Method.GET) == ''``, ``len(str(Method.GET)) == 0`` and, most visibly, +``Method.GET == 'GET'`` was :data:`False` for every one of the 40 declared +members. True on ``main`` at ``60b85e3a4`` (measured), and true since the class +was first written -- GitHub issue #869 fixed the same inconsistency for an +*unregistered* member's own str payload +(:meth:`~pcapkit.const.http.method.Method._unregistered_member` now calls the +base's :meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member`, which +does pass the value along), which is what exposed this half: after #869, +``str(Method('frob')) == 'frob'`` while ``str(Method.GET) == ''`` -- a +registered member and an unregistered one disagreeing about the one thing +they are both supposed to be, a :class:`str`. + +The fix is ``obj = str.__new__(cls, value)``, mirroring +:meth:`pcapkit.const.ftp.command.Command.__new__` +(``obj = str.__new__(cls, name)``), which never had this defect. Applied to +both :mod:`pcapkit.const.http.method` (the generated module) and +:mod:`pcapkit.vendor.http.method` (the generator template that produces it), +since a fix only in the former is silently reverted by the next crawl. + +This is a **behaviour change to all 40 public members**: every equality +comparison of a :class:`~pcapkit.const.http.method.Method` member against its +own wire string now succeeds where it used to fail silently (no exception -- +just an unexpected :data:`False`). Nothing under :mod:`pcapkit.protocols` +compared a member this way before the fix (the one caller, +:meth:`pcapkit.protocols.application.httpv1.HTTP.read_http_header`, only ever +reads ``.value`` indirectly through :meth:`~pcapkit.const.http.method.Method. +get`, never ``str()`` or ``==`` against the member itself), so the change is +observable only to external callers and to this module's own dunders +(``__repr__``, which reads ``_value_`` and was already correct). + +The wider sweep below -- :class:`StrValuedRegistryPayloadTests` -- checks the +same property, ``str(member) == member.value``, for every ``EnumRegistry`` ++ ``StrEnum`` registry under :mod:`pcapkit.const` with a hand-rolled +``__new__``: :class:`~pcapkit.const.ftp.command.Command`, +:class:`~pcapkit.const.ftp.command.FEATCode`, +:class:`~pcapkit.const.http.method.Method` and +:class:`~pcapkit.const.pcapng.option_type.OptionType`. ``Command`` already +passed ``name`` to ``str.__new__`` and ``FEATCode`` has no custom ``__new__`` +at all (the base :class:`~aenum.StrEnum` machinery handles it), so neither +carried this defect -- only ``Method`` did. ``OptionType`` is deliberately +*not* swept the same way: its own :attr:`~pcapkit.const.pcapng.option_type. +OptionType.value` is *itself* the formatted display string +(``'%s [%d]' % (opt_name, opt_value)``, set by its own ``__new__``), not the +raw wire value (that lives in :attr:`~pcapkit.const.pcapng.option_type. +OptionType.opt_value`) -- so ``str(member) == member.value`` holds for it +trivially and proves nothing about the property the other three are being +checked for. It gets its own, separate assertion instead, pinning its actual +contract rather than silently dropping it from the sweep. + +""" +from __future__ import annotations + +import unittest + +from tests._support import purge_modules + + +class MethodStrPayload870RegressionTests(unittest.TestCase): + """The exact repro from GitHub issue #870, and the full sweep of Method's own 40 members.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_the_reported_case(self) -> None: + """``Method.GET``, byte for byte, as the issue measured it. + + Fails against ``main`` at ``60b85e3a4``: ``str(Method.GET)`` there is + ``''`` (``len`` 0) and ``Method.GET == 'GET'`` is :data:`False`. + """ + from pcapkit.const.http.method import Method + + self.assertEqual(Method.GET.value, 'GET') + self.assertEqual(str(Method.GET), 'GET') + self.assertEqual(len(str(Method.GET)), 3) + self.assertEqual(Method.GET, 'GET') + self.assertTrue(Method.GET == 'GET') # pylint: disable=singleton-comparison + + def test_every_one_of_the_40_registered_members_carries_its_value_as_str_payload(self) -> None: + """Swept, not just ``GET`` -- the defect was in ``__new__``, so every + member built through it was affected identically.""" + from pcapkit.const.http.method import Method + + members = list(Method) + self.assertEqual(len(members), 40, 'Method no longer declares 40 members; a fresh look is needed') + + for member in members: + with self.subTest(member=member.name): + self.assertEqual(str(member), member.value) + self.assertEqual(member, member.value) + self.assertEqual(len(str(member)), len(member.value)) + + def test_registered_and_unregistered_members_are_now_consistent(self) -> None: + """The inconsistency #870 reported: #869 fixed the unregistered half + only, so ``str(Method('frob')) == 'frob'`` while ``str(Method.GET) == + ''`` on ``main`` at ``60b85e3a4``. Both now carry real content.""" + from pcapkit.const.http.method import Method + + registered = Method.GET + unregistered = Method('frob') + + self.assertEqual(str(registered), 'GET') + self.assertEqual(str(unregistered), 'frob') + self.assertNotEqual(str(registered), '') + self.assertNotEqual(str(unregistered), '') + + def test_repr_and_the_extra_attributes_are_unaffected(self) -> None: + """The fix touches only the :class:`str` payload -- ``__repr__`` (which + reads ``_value_``, already correct) and the ``safe``/``idempotent`` + attributes :meth:`__new__` also sets must be untouched.""" + from pcapkit.const.http.method import Method + + self.assertEqual(repr(Method.GET), '') + self.assertTrue(Method.GET.safe) + self.assertTrue(Method.GET.idempotent) + self.assertFalse(Method.POST.safe) + self.assertFalse(Method.POST.idempotent) + + +class StrValuedRegistryPayloadTests(unittest.TestCase): + """The registry-wide form: every ``EnumRegistry`` + ``StrEnum`` member's + :class:`str` payload must equal its own declared value. + + :class:`~pcapkit.const.pcapng.option_type.OptionType` is deliberately + excluded from :data:`SWEPT_REGISTRIES` -- see the module docstring for why + sweeping it the same way would prove nothing -- and gets its own + :meth:`test_optiontype_is_exempt_but_its_own_contract_still_holds` instead. + """ + + #: The three registries this sweep actually checks. ``Command`` and + #: ``FEATCode`` never carried GitHub issue #870's defect (see the module + #: docstring's sibling survey); they are swept anyway so a regression in + #: either would be caught here too, not only in ``Method``. + SWEPT_REGISTRIES = ('Command', 'FEATCode', 'Method') + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_every_member_of_every_swept_registry_matches_its_value(self) -> None: + from pcapkit.const.ftp.command import Command, FEATCode + from pcapkit.const.http.method import Method + + registries = {'Command': Command, 'FEATCode': FEATCode, 'Method': Method} + self.assertEqual(set(registries), set(self.SWEPT_REGISTRIES)) + + checked = 0 + for name in self.SWEPT_REGISTRIES: + cls = registries[name] + for member in cls: + checked += 1 + with self.subTest(registry=name, member=member.name): + self.assertEqual( + str(member), member.value, + f'{name}.{member.name}: str(member) != member.value; ' + f'see GitHub issue #870') + + # Command (60) + FEATCode (15) + Method (40), pinned so a registry + # gaining or losing members gets a fresh look rather than a silent pass. + self.assertEqual(checked, 115) + + def test_optiontype_is_exempt_but_its_own_contract_still_holds(self) -> None: + """Not swept above because ``OptionType.value`` *is* the formatted + display string already -- ``str(member) == member.value`` would pass + trivially and check nothing about a raw wire payload, unlike the + other three. This pins what its ``__new__``/``__str__`` actually + promise instead: the value equals the display string built from + ``opt_name``/``opt_value``, and the wire-facing ``opt_value`` is + reachable separately via :attr:`~pcapkit.const.pcapng.option_type. + OptionType.opt_value`, not through :class:`str` at all. + """ + from pcapkit.const.pcapng.option_type import OptionType + + for member in OptionType: + with self.subTest(member=member.name): + expected = '%s [%d]' % (member.opt_name, member.opt_value) # pylint: disable=consider-using-f-string + self.assertEqual(str(member), expected) + self.assertEqual(member.value, expected) + # The raw wire value is opt_value, an int -- not derivable + # from str(member) the way it is for Command/FEATCode/Method. + self.assertIsInstance(member.opt_value, int) + + +if __name__ == '__main__': + unittest.main()