diff --git a/CHANGELOG.md b/CHANGELOG.md index 22b162cd53..b67c26c35d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -457,6 +457,7 @@ This resolves [#548](https://github.com/JarryShaw/PyPCAPKit/issues/548), which r - the generated const comments render as written: a URL reference followed by another bracket no longer autolinks into it, a leading enumerator such as `(S)` is escaped, and a hyphenated word is no longer split across lines ([#1846](https://github.com/JarryShaw/PyPCAPKit/issues/1846), [#1847](https://github.com/JarryShaw/PyPCAPKit/issues/1847), [#1871](https://github.com/JarryShaw/PyPCAPKit/pull/1871)). - without the `vendor` extra, `pcapkit-vendor` and `python -m pcapkit.vendor` exit 1 with one line naming the missing packages and `pypcapkit[vendor]`, where they died with a traceback, and `--help` and `--version` still work. Importing a crawler then raises `ModuleNotFoundError` ([#1850](https://github.com/JarryShaw/PyPCAPKit/issues/1850), [#1875](https://github.com/JarryShaw/PyPCAPKit/pull/1875)). - the ESP cipher and integrity crawlers strip the brackets off IANA's `[UNSPECIFIED]` "no document" cell before testing it against the unknown placeholder, so `pcapkit.const.esp.integrity` no longer reads `#: AUTH_DES_MAC [UNSPECIFIED] (DEPRECATED ...)` for `AUTH_DES_MAC` and `AUTH_KPDK_MD5` ([#2294](https://github.com/JarryShaw/PyPCAPKit/issues/2294), [#2371](https://github.com/JarryShaw/PyPCAPKit/pull/2371)). +- some dict-based crawlers' `process(self, data)` ignored the `data` argument and read the module-level `DATA` literal instead, in `ipv4/{qs_function,tos_del,tos_ecn,tos_pre,tos_rel,tos_thr,ts_flag}.py`, `ipv6/{option_action,qs_function,smf_dpd_mode}.py`, `mh/{binding_error,lla_code}.py` and `pcapng/{filter_type,hash_algorithm,secrets_type,verdict_type}.py`. Harmless today, since every real call site passes `request()`'s own `DATA`-returning value, so `data is DATA` always held in practice -- but a caller passing different data, such as a test fixture or a future `LINK`-based fetch, got the module's table back regardless, with no error. The dict-based crawlers are hand-written rather than generated, so the fix needs no `const/` regeneration ([#2429](https://github.com/JarryShaw/PyPCAPKit/issues/2429), [#2430](https://github.com/JarryShaw/PyPCAPKit/pull/2430)). ### Project infrastructure diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 758edb496a..c7864095e2 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -3350,6 +3350,18 @@ Fixed placeholder, so :mod:`pcapkit.const.esp.integrity` no longer reads ``#: AUTH_DES_MAC [UNSPECIFIED] (DEPRECATED ...)`` for ``AUTH_DES_MAC`` and ``AUTH_KPDK_MD5`` (:issue:`2294`, :pr:`2371`). +* some dict-based crawlers' ``process(self, data)`` ignored the ``data`` + argument and read the module-level ``DATA`` literal instead, in + ``ipv4/{qs_function,tos_del,tos_ecn,tos_pre,tos_rel,tos_thr,ts_flag}.py``, + ``ipv6/{option_action,qs_function,smf_dpd_mode}.py``, + ``mh/{binding_error,lla_code}.py`` and + ``pcapng/{filter_type,hash_algorithm,secrets_type,verdict_type}.py``. Harmless + today, since every real call site passes ``request()``'s own + ``DATA``-returning value, so ``data is DATA`` always held in practice -- but a + caller passing different data, such as a test fixture or a future ``LINK``-based + fetch, got the module's table back regardless, with no error. The dict-based + crawlers are hand-written rather than generated, so the fix needs no ``const/`` + regeneration (:issue:`2429`, :pr:`2430`). Project infrastructure ---------------------- diff --git a/pcapkit/vendor/ipv4/qs_function.py b/pcapkit/vendor/ipv4/qs_function.py index b8d8edf6a9..d472a37404 100644 --- a/pcapkit/vendor/ipv4/qs_function.py +++ b/pcapkit/vendor/ipv4/qs_function.py @@ -68,7 +68,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/ipv4/tos_del.py b/pcapkit/vendor/ipv4/tos_del.py index 8c5a27460c..dc045afd13 100644 --- a/pcapkit/vendor/ipv4/tos_del.py +++ b/pcapkit/vendor/ipv4/tos_del.py @@ -68,7 +68,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code).upper() # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/ipv4/tos_ecn.py b/pcapkit/vendor/ipv4/tos_ecn.py index 3be47d7d66..2eefee1c54 100644 --- a/pcapkit/vendor/ipv4/tos_ecn.py +++ b/pcapkit/vendor/ipv4/tos_ecn.py @@ -85,7 +85,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) enum.append(f"{renm} = 0b{bin(code)[2:].zfill(2)}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/ipv4/tos_pre.py b/pcapkit/vendor/ipv4/tos_pre.py index 1d64ff8d8b..513ef68ab6 100644 --- a/pcapkit/vendor/ipv4/tos_pre.py +++ b/pcapkit/vendor/ipv4/tos_pre.py @@ -74,7 +74,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/ipv4/tos_rel.py b/pcapkit/vendor/ipv4/tos_rel.py index 62021f179e..bc0bf7f6a1 100644 --- a/pcapkit/vendor/ipv4/tos_rel.py +++ b/pcapkit/vendor/ipv4/tos_rel.py @@ -68,7 +68,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code).upper() # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/ipv4/tos_thr.py b/pcapkit/vendor/ipv4/tos_thr.py index 7af2e108d7..03e0d34825 100644 --- a/pcapkit/vendor/ipv4/tos_thr.py +++ b/pcapkit/vendor/ipv4/tos_thr.py @@ -68,7 +68,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code).upper() # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/ipv4/ts_flag.py b/pcapkit/vendor/ipv4/ts_flag.py index 971f788de6..5eb0fbe5b3 100644 --- a/pcapkit/vendor/ipv4/ts_flag.py +++ b/pcapkit/vendor/ipv4/ts_flag.py @@ -69,7 +69,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/ipv6/option_action.py b/pcapkit/vendor/ipv6/option_action.py index 7d22c01ae5..e522520b7a 100644 --- a/pcapkit/vendor/ipv6/option_action.py +++ b/pcapkit/vendor/ipv6/option_action.py @@ -80,7 +80,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/ipv6/qs_function.py b/pcapkit/vendor/ipv6/qs_function.py index 90a8d4bb00..02cc4e4ad8 100644 --- a/pcapkit/vendor/ipv6/qs_function.py +++ b/pcapkit/vendor/ipv6/qs_function.py @@ -68,7 +68,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/ipv6/smf_dpd_mode.py b/pcapkit/vendor/ipv6/smf_dpd_mode.py index d7fddf5d0a..579d70b391 100644 --- a/pcapkit/vendor/ipv6/smf_dpd_mode.py +++ b/pcapkit/vendor/ipv6/smf_dpd_mode.py @@ -68,7 +68,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/mh/binding_error.py b/pcapkit/vendor/mh/binding_error.py index d862d6a55e..bb3405ad7b 100644 --- a/pcapkit/vendor/mh/binding_error.py +++ b/pcapkit/vendor/mh/binding_error.py @@ -68,7 +68,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/mh/lla_code.py b/pcapkit/vendor/mh/lla_code.py index 81bbed9713..55f68ad880 100644 --- a/pcapkit/vendor/mh/lla_code.py +++ b/pcapkit/vendor/mh/lla_code.py @@ -87,7 +87,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/pcapng/filter_type.py b/pcapkit/vendor/pcapng/filter_type.py index ad294da814..28672ef26a 100644 --- a/pcapkit/vendor/pcapng/filter_type.py +++ b/pcapkit/vendor/pcapng/filter_type.py @@ -67,7 +67,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/pcapng/hash_algorithm.py b/pcapkit/vendor/pcapng/hash_algorithm.py index 5cb9131048..e4776da44a 100644 --- a/pcapkit/vendor/pcapng/hash_algorithm.py +++ b/pcapkit/vendor/pcapng/hash_algorithm.py @@ -72,7 +72,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/pcapng/secrets_type.py b/pcapkit/vendor/pcapng/secrets_type.py index e352912667..41b2dfb289 100644 --- a/pcapkit/vendor/pcapng/secrets_type.py +++ b/pcapkit/vendor/pcapng/secrets_type.py @@ -70,7 +70,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = 0x{code:08x}".ljust(76)) return enum, miss diff --git a/pcapkit/vendor/pcapng/verdict_type.py b/pcapkit/vendor/pcapng/verdict_type.py index 155a6dc3d8..fa69bc7135 100644 --- a/pcapkit/vendor/pcapng/verdict_type.py +++ b/pcapkit/vendor/pcapng/verdict_type.py @@ -69,7 +69,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t miss = [ "return cls._unregistered_member(value, 'Unassigned')", ] - for code, name in DATA.items(): + for code, name in data.items(): renm = self.rename(name, code) # type: ignore[arg-type] enum.append(f"{renm} = {code}".ljust(76)) return enum, miss diff --git a/tests/vendor/test_vendor_process_uses_passed_data_2429_unit.py b/tests/vendor/test_vendor_process_uses_passed_data_2429_unit.py new file mode 100644 index 0000000000..faefa18edf --- /dev/null +++ b/tests/vendor/test_vendor_process_uses_passed_data_2429_unit.py @@ -0,0 +1,154 @@ +# -*- coding: utf-8 -*- +"""Some dict-based crawlers' ``process`` ignored their own ``data`` argument (#2429). + +:meth:`~pcapkit.vendor.default.Vendor.process` on the dict-based crawlers listed +in :data:`TARGETS` is meant to render an enumeration from whatever registry +mapping its caller hands it, mirroring :meth:`~pcapkit.vendor.default.Vendor.count`, +which both takes and uses its own ``data``. Those crawlers instead read the +module-level ``DATA`` literal straight out of the closure, silently discarding +the argument. Harmless today, since every real call site passes +``request()``'s own ``DATA``-returning value, so ``data is DATA`` always holds +in practice -- but a caller that ever passes different data would get the +module's table back regardless, with no error. + +Each test below builds a crawler with :meth:`~pcapkit.vendor.default.Vendor.count` +and :meth:`~pcapkit.vendor.default.Vendor.process` driven on a fabricated +mapping whose codes and names are both disjoint from the module's own +``DATA`` (see :meth:`ProcessUsesPassedDataTests._vendor`), the same +``cls.__new__(cls)`` bypass +:file:`tests/vendor/test_vendor_mh_lla_code_1848_unit.py` uses to skip +:meth:`Vendor.__init__`'s network fetch and file write. A fixed crawler's +rendered enumeration carries the fabricated names and none of the module's +real ones; the unfixed crawler ignores the argument and renders the module's +real ``DATA`` instead. + +""" +from __future__ import annotations + +import importlib +import importlib.util +import unittest +from typing import TYPE_CHECKING + +from tests._support import reimport_once_per_class + +if TYPE_CHECKING: + from typing import Any + +#: Test tier (#1958): drives the vendor crawlers directly, the same reasoning +#: as tests/vendor/test_vendor_mh_lla_code_1848_unit.py. +__tier__ = 'LATEST' + +#: Every distribution importing :mod:`pcapkit.vendor` needs, as +#: :file:`tests/vendor/test_user_agent_unit.py` explains. +VENDOR_DEPS = ('requests', 'bs4', 'html5lib') + +#: Whether the crawlers are importable at all. +HAS_VENDOR_DEPS = all(importlib.util.find_spec(name) is not None for name in VENDOR_DEPS) + +#: ``module name -> class name``, the crawlers GitHub issue #2429 found +#: reading the module-level ``DATA`` literal instead of their own ``data`` +#: argument in :meth:`~pcapkit.vendor.default.Vendor.process`. +TARGETS = { + 'pcapkit.vendor.ipv4.qs_function': 'QSFunction', + 'pcapkit.vendor.ipv4.tos_del': 'ToSDelay', + 'pcapkit.vendor.ipv4.tos_ecn': 'ToSECN', + 'pcapkit.vendor.ipv4.tos_pre': 'ToSPrecedence', + 'pcapkit.vendor.ipv4.tos_rel': 'ToSReliability', + 'pcapkit.vendor.ipv4.tos_thr': 'ToSThroughput', + 'pcapkit.vendor.ipv4.ts_flag': 'TSFlag', + 'pcapkit.vendor.ipv6.option_action': 'OptionAction', + 'pcapkit.vendor.ipv6.qs_function': 'QSFunction', + 'pcapkit.vendor.ipv6.smf_dpd_mode': 'SMFDPDMode', + 'pcapkit.vendor.mh.binding_error': 'BindingError', + 'pcapkit.vendor.mh.lla_code': 'LLACode', + 'pcapkit.vendor.pcapng.filter_type': 'FilterType', + 'pcapkit.vendor.pcapng.hash_algorithm': 'HashAlgorithm', + 'pcapkit.vendor.pcapng.secrets_type': 'SecretsType', + 'pcapkit.vendor.pcapng.verdict_type': 'VerdictType', +} + + +@unittest.skipUnless(HAS_VENDOR_DEPS, f'vendor extra not installed ({", ".join(VENDOR_DEPS)})') +class ProcessUsesPassedDataTests(unittest.TestCase): + """Each crawler's :meth:`process` renders what it was called with.""" + + if TYPE_CHECKING: + module_cache: 'dict[str, Any]' + + def setUp(self) -> 'None': + reimport_once_per_class(self) + self.module_cache = {} + + def _vendor(self, module_name: 'str', class_name: 'str') -> 'tuple[Any, dict[int, str], dict[int, str]]': + """A crawler with the attributes ``__init__`` sets, but none of its I/O. + + The fabricated registry uses codes well outside any real registry's + range (the dict-based crawlers here enumerate small bit flags or IANA + codes, never six-digit values) so it can never coincide with the + module's own ``DATA``, even for a crawler whose table is empty. + + Returns: + The crawler, its module's real registry, and a fabricated one. + + """ + module = self.module_cache.get(module_name) + if module is None: + module = importlib.import_module(module_name) + self.module_cache[module_name] = module + + cls = getattr(module, class_name) + vendor = cls.__new__(cls) + vendor.NAME = cls.__name__ + vendor.DOCS = cls.__doc__ + + real_data = vendor.request() + fake_data = {900001: 'Sentinel2429MarkerAlpha', 900002: 'Sentinel2429MarkerBeta'} + return vendor, real_data, fake_data + + def test_process_renders_the_data_it_is_given(self) -> 'None': + for module_name, class_name in TARGETS.items(): + with self.subTest(module=module_name, cls=class_name): + vendor, real_data, fake_data = self._vendor(module_name, class_name) + + vendor.record = vendor.count(fake_data) + enum, _ = vendor.process(fake_data) + rendered = '\n'.join(enum).upper() + + for name in fake_data.values(): + with self.subTest(fake_name=name): + self.assertIn(name.upper(), rendered) + + for name in real_data.values(): + safe = vendor.safe_name(name) + if safe.upper() in ('RESERVED', 'UNASSIGNED'): + continue + with self.subTest(real_name=name): + self.assertNotIn(safe.upper(), rendered) + + def test_process_does_not_fall_back_to_the_module_table(self) -> 'None': + # The sharpest form of the regression: process() called with the real + # DATA produces the committed enumeration, but process() called with + # anything else must not silently produce that same enumeration too. + # + # vendor.record is set once, from the real registry, and left alone + # across both calls -- the way Vendor.__init__ computes it once from + # request()'s own return before calling process(). Recomputing it + # between the two calls would let a crawler whose own rename() keys + # off self.record (ipv4/tos_ecn folds 'ECT(1)' and 'ECT(0)' to the + # same safe name and only disambiguates when their count collides) + # diverge for that reason alone, which would pass this assertion + # whether or not process() is reading its own data argument. + for module_name, class_name in TARGETS.items(): + with self.subTest(module=module_name, cls=class_name): + vendor, real_data, fake_data = self._vendor(module_name, class_name) + vendor.record = vendor.count(real_data) + + enum_real, _ = vendor.process(real_data) + enum_fake, _ = vendor.process(fake_data) + + self.assertNotEqual(enum_real, enum_fake) + + +if __name__ == '__main__': + unittest.main()