fix(generators): correct stale options.py comments and the code they justified (#1337)

examples/generators/options.py

@@ -525,8 +525,8 @@ def _ipv4_parse(octets: 'bytes') -> 'Any':
 def _ipv4_extract(parsed: 'Any') -> 'Any':
     from pcapkit.corekit.multidict import OrderedMultiDict
 
-    # EOOL and NOP are dropped by ``_make_ipv4_options`` as padding, so the
-    # header they produce has no options at all and ``info`` carries no
+    # ``_make_ipv4_options`` drops EOOL and NOP from a fresh option list as
+    # padding, so the header a case builds from either has no options at all and ``info`` carries no
     # ``options`` attribute. An empty collection is the honest reading of that,
     # and it keeps the two padding codes from being reported as parse failures.
     return getattr(parsed.info, 'options', OrderedMultiDict())
@@ -558,9 +558,10 @@ def _ipv6_option_overrides() -> 'dict[Any, dict[str, Any]]':
     # HOPOPT and IPv6-Opts key on the same enumeration and behave identically
     # on every code, so one table serves both.
     return {
-        # ``nonce=0`` gives a zero-octet nonce. Only widths 3, 5, 6 and 7 pack
-        # at all, because a callable-length NumberField never clears the flag
-        # that says "hand struct a bytes"; 0xFFFFFF is three octets.
+        # The nonce is sized to its bit length rounded up to whole octets, at
+        # least one, so the default ``nonce=0`` is a one-octet zero. A
+        # three-octet value exercises a multi-octet width; every width from 1
+        # to 16 octets builds and round-trips.
         Enum_Option.ILNP_Nonce: {'nonce': 0xFFFFFF},
         Enum_Option.Line_Identification_Option: {'id': b'line-1'},
     }
@@ -1163,10 +1164,14 @@ def _httpv2_extract(parsed: 'Any') -> 'Any':
 def _httpv2_rebuild(frame: 'Any') -> 'Any':
     from pcapkit.protocols.application.httpv2 import HTTP
 
-    # ``flags`` is deliberately not passed on: ``HTTP.make`` rebinds it from
-    # the ``_make_http_*`` return whenever ``frame`` is a mapping or a Data
-    # object, so passing it would be silently ignored.
-    return HTTP(type=frame.type, sid=frame.sid, frame=frame)
+    # ``HTTP.make`` ORs ``flags`` into the bits the ``_make_http_*`` constructor
+    # derives from ``frame``, and that constructor knows only the flags its
+    # frame type defines. So the raw flags octet is passed on, as
+    # ``HTTP._make_data`` does, or a bit the type leaves undefined is dropped.
+    # ``reserved`` is passed on for the same reason: ``make`` defaults it to 0.
+    return HTTP(type=frame.type, sid=frame.sid, reserved=frame.reserved,
+                flags=frame.flags.__value__ if frame.flags is not None else 0,
+                frame=frame)
 
 
 ###############################################################################
@@ -1625,21 +1630,17 @@ def _pcapng_secrets_build(code: 'Any', kwargs: 'dict[str, Any]') -> 'Any':
 
 #: Codes deliberately left out of the case list, with the reason.
 #:
-#: The padding options are dropped by every ``_make_*_options`` in the tree --
-#: it discards them and inserts its own padding to reach the alignment the
-#: protocol wants -- so a case for one would assert only that construction
-#: ignores its own input, which it does. They are kept for TCP, where the
-#: dropping is symmetric and the case is a genuine round trip of an
-#: option-less header, and skipped where a second padding code would merely
-#: duplicate the first.
+#: Every padding option has a case except PadN in HOPOPT and IPv6-Opts. Their
+#: ``_make_*_options`` keep the padding of parsed options in place, but drop
+#: any in a list of fresh options and insert their own to reach 8-octet
+#: alignment. A case is built from such a list, so the Pad1 and PadN cases
+#: would both emit the same option-less header; Pad1 is kept, PadN skipped.
+#: ``_make_tcp_options`` emits ``EOOL`` and ``NOP`` as given, so the TCP
+#: padding cases are distinct headers and both round-trip.
 SKIP = {
-    ('tcp-option', 'End_of_Option_List'): 'padding, dropped by _make_tcp_options',
-    ('tcp-option', 'No_Operation'): 'padding, dropped by _make_tcp_options',
     # Multipath TCP is the envelope for the whole tcp-mptcp family, so a case
     # here would silently duplicate whichever subtype it defaulted to.
     ('tcp-option', 'Multipath_TCP'): 'envelope for the tcp-mptcp family',
-    # Pad1 and PadN are both discarded by _make_hopopt_options, so the two
-    # cases emit byte-identical option-less headers.
     ('hopopt-option', 'PadN'): 'padding, indistinguishable from Pad1 here',
     ('ipv6-opts-option', 'PadN'): 'padding, indistinguishable from Pad1 here',
 }

tests/protocols/test_option_generator_1337_unit.py

@@ -0,0 +1,120 @@
+# -*- coding: utf-8 -*-
+"""What #1337 changed in :file:`examples/generators/options.py`.
+
+Two comments there justified code by limitations that no longer hold.
+
+* ``_httpv2_rebuild`` withheld ``flags``, on the grounds that ``HTTP.make``
+  ignored it. Since #1341 ``make`` ORs ``flags`` into the bits the frame
+  constructor derives, and that constructor knows only the bits its frame type
+  defines. So a parsed frame with an undefined flag bit, or the reserved bit,
+  lost it on the generator's rebuild although ``HTTP.from_data`` keeps it.
+* ``SKIP`` left out TCP ``EOOL`` and ``NOP`` as "dropped by
+  ``_make_tcp_options``". Since #1163 that maker emits them as given, so each is
+  a distinct header that round-trips, and both now have a case.
+
+This module is unit tier: it builds everything in memory and reads no capture.
+
+"""
+
+from __future__ import annotations
+
+import importlib.util
+import sys
+import types
+import unittest
+import warnings
+
+from tests._support import time_limit
+from tests._tiers import ROOT
+
+RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper')
+HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS)
+
+#: Parsed HTTP/2 frames whose flags octet or reserved bit the frame constructor
+#: cannot derive. Each is ``(label, frame octets)``.
+HTTPV2_FRAMES = (
+    # DATA defines END_STREAM (0x01) and PADDED (0x08) only.
+    ('DATA, undefined flag 0x02', '000003000200000001616263'),
+    ('DATA, undefined flag 0x80', '000003008000000001616263'),
+    # PING defines ACK (0x01) only.
+    ('PING, undefined flag 0x40', '0000080640000000000000000000000000'),
+    # WINDOW_UPDATE defines no flags at all.
+    ('WINDOW_UPDATE, flag 0x02', '00000408020000000100000000'),
+    # The reserved bit is the top bit of the stream identifier.
+    ('DATA, reserved bit set', '000003000080000001616263'),
+)
+
+#: TCP header octets a TCP padding case must carry after the fixed 20-octet
+#: header: the option as given, then alignment to 32 bits.
+TCP_PADDING = {
+    'End_of_Option_List': '00000000',
+    'No_Operation': '01010100',
+}
+
+
+def _load_generator() -> 'types.ModuleType':
+    """Load :file:`examples/generators/options.py` by path, under a name of its own.
+
+    Returns:
+        The generator module.
+
+    Raises:
+        RuntimeError: If the module cannot be found or loaded.
+
+    """
+    path = ROOT / 'examples' / 'generators' / 'options.py'
+    spec = importlib.util.spec_from_file_location('pcapkit_samples_options_1337', path)
+    if spec is None or spec.loader is None:  # pragma: no cover
+        raise RuntimeError(f'cannot load the option generator from {path}')
+
+    module = importlib.util.module_from_spec(spec)
+    sys.modules[spec.name] = module
+    spec.loader.exec_module(module)
+    return module
+
+
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
+class OptionGenerator1337Tests(unittest.TestCase):
+    """The generator code the stale #1337 comments used to justify."""
+
+    options = None  # type: types.ModuleType
+
+    @classmethod
+    def setUpClass(cls) -> None:
+        cls.options = _load_generator()
+
+    def test_httpv2_rebuild_keeps_the_flags_octet_and_reserved_bit(self) -> None:
+        """A parsed frame rebuilds byte-exact through ``_httpv2_rebuild``."""
+        family = self.options.FAMILY_MAP['httpv2-frame']
+        for label, hexstr in HTTPV2_FRAMES:
+            with self.subTest(frame=label):
+                octets = bytes.fromhex(hexstr)
+                with warnings.catch_warnings():
+                    warnings.simplefilter('ignore')
+                    with time_limit():
+                        info = family.extract(family.parse(octets))
+                        rebuilt = family.rebuild(info).data
+                self.assertEqual(rebuilt.hex(), hexstr)
+
+    def test_tcp_padding_codes_have_cases(self) -> None:
+        """``EOOL`` and ``NOP`` are cases, not ``SKIP`` entries."""
+        for name in TCP_PADDING:
+            with self.subTest(code=name):
+                self.assertNotIn(('tcp-option', name), self.options.SKIP)
+
+    def test_tcp_padding_cases_round_trip_as_distinct_headers(self) -> None:
+        """Each TCP padding case is ``OK`` and carries its own option octets."""
+        family = self.options.FAMILY_MAP['tcp-option']
+        cases = {case.name: case for case in self.options.cases((family,))}
+        for name, tail in TCP_PADDING.items():
+            with self.subTest(code=name):
+                self.assertIn(name, cases)
+                with warnings.catch_warnings():
+                    warnings.simplefilter('ignore')
+                    outcome = self.options.roundtrip(cases[name])
+                self.assertEqual(outcome.status, 'OK', outcome.detail)
+                self.assertEqual(outcome.octets[20:].hex(), tail)
+
+
+if __name__ == '__main__':
+    unittest.main()