Skip to content

fix(protocols): rebuild the payload from info.to_dict() in from_data (#1447) - #1452

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1447-to-dict-payload-roundtrip
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1447-to-dict-payload-roundtrip

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style — mypy on the 3 modules: no new errors vs main; pylint: no new findings in the changed source; isort not run with the repo config
  • make test passes, and a test case covers the change — ordering legs foundation 528 OK, dumpkit 227 OK, protocols/internet 601 OK, protocols (rest) 1126 OK, project 606 OK; test_capture_roundtrip_runtime.py 4 passed, 20501 subtests
  • Changelog entry — N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1447

to_dict() is left unchanged, because the dumps are written from it. Instead, _make_payload falls back to the dict's structure when __next_type__ and __next_name__ are absent. The payload is the last key the data model does not declare whose value is a mapping. It is matched against the info_name of each protocol in __proto__ and its fallback, not the class name, so L2TPv2's l2tp resolves. IPv6 recovers __exthdr__ by walking next over the extension-header keys. HTTP had a separate cause: its dispatcher's data model is Raw, so HTTP.from_data now picks the HTTP/1.* data model if the dict has receipt, or HTTP/2 if it has sid.

Over every sample capture, the dict-form rebuild is now byte-exact for Frame, Ethernet, ARP, IPv4, IPv6, TCP, UDP and HTTP. Ethernet goes from 22582 to 243047 of 243047 octets, and ipv4.pcap #1 from 14 to 1870 of 1870. This PR drops the #1447 rows from DICT_GAPS in test_capture_roundtrip_runtime.py, which leaves the table empty. Repeated IPv6 extension headers colliding in to_dict() keys are #1453, which this PR does not fix.

New tests/protocols/test_to_dict_payload_roundtrip_1447_unit.py builds frames in memory, including UDP over L2TPv2. With the sources from main, 54 cases fail across 5 of its 7 tests. The other 2 pin that to_dict is unchanged and that a dict without a payload key yields NoPayload.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.51% (unit tier, Python 3.14, c5c00240d, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 968 2356 869 90.90%
pcapkit/corekit 2043 89 636 25 94.77%
pcapkit/dumpkit 234 2 84 2 98.74%
pcapkit/foundation 2648 98 930 48 94.80%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16875 225 4554 189 98.03%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 2a6d4fba5 (Sonnet cross-review; the author ran on Opus).

Blocking: a UDP datagram carrying L2TPv2 (port 1701) still loses its payload in the dict rebuild. _lookup_payload (pcapkit/protocols/protocol.py:1991) matches the payload key against the lowercased class name, l2tpv2. But L2TP.info_name (pcapkit/protocols/link/l2tp.py:136) returns l2tp for every version, so the key is never found. I reproduced it on this head:

  • dict rebuild: UDP.from_data(udp.info.to_dict()).data gives 8 of 20 octets
  • object rebuild: UDP.from_data(udp.info) gives 20 of 20

The layers above (IPv4, Ethernet) then rebuild inexactly as well.

Fix: match on info_name (or every class in the MRO), not __name__.lower(). Correct the docstring that says info_name is always the lowercased class name, and add a UDP-over-L2TPv2 case to the new test.

Everything else holds:

  • All 1616 capture frames rebuild exactly from the dict for every layer.
  • 129 dispatch-generator envelopes rebuild exactly, except the 3 under UDP/1701.
  • HTTP/1 and all 14 HTTP/2 frame types are exact.
  • The object-form rebuild and the json/tree/plist dumps are identical to main.
  • Tests fail before the fix: 45.
  • All legs are clean.

Nit: inspect.get_annotations is Python 3.10+. CI only covers 3.10 and later, but the code otherwise runs on 3.8, so klass.__dict__.get('__annotations__', {}) is a safer fallback.

Separate issue, not this PR: repeated IPv6 extension headers collide in the to_dict() keys. The Info object is exact; only the dict collapses them. Filed as #1453.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
…o_dict omits __next_type__/__next_name__ (#1447)

to_dict() leaves out the dispatch keys, so _make_payload fell back to
NoPayload and a layer rebuilt from its dict lost everything past its own
header. The dict is left as it is, since the dumps are written from it;
the payload is found from its structure instead.

- ProtocolBase._lookup_payload: the payload is the last key the data
  model does not declare whose value is a mapping, matched against the
  info_name of each protocol in __proto__ and its fallback.
- IPv6._lookup_exthdr: recover the extension header chain by walking
  next from the keys the headers were written under.
- HTTP.from_data: rebuild a dict into the HTTP/1.* or HTTP/2 data model
  by its keys, since the dispatcher's own model carries no version.
- test_capture_roundtrip_runtime: drop the #1447 DICT_GAPS rows.
@JarryShaw
JarryShaw force-pushed the fix/1447-to-dict-payload-roundtrip branch from 2a6d4fb to c5c0024 Compare October 9, 2026 02:23
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at c5c00240d (round 2, Sonnet cross-review; the author ran on Opus).

  • L2TPv2 fixed. The payload is now matched by info_name. UDP-over-L2TPv2 rebuilds from the dict at 20 of 20 octets, which I re-ran myself on this head. IPv4 and Ethernet over it are also byte-exact.
  • proto.__new__(proto) is safe. No class overrides __new__, and the metaclass hooks leave the registries alone. None of the 44 ProtocolBase subclasses raises, and all 27 registry-reachable classes resolve. Only L2TPv2 and HTTP have an info_name that differs from their class name.
  • Every dict rebuild is exact. That holds for every layer of all 1616 capture frames (main: 5446 bad) and for all 129 dispatch envelopes.
  • Nothing else moved. The dumps (24 files) and the object-form rebuild are identical to main.
  • Tests. The new module, re-run by me, gives 7 passed, 89 subtests. With main's sources it gives 54 failures. The test: capture-driven parse→rebuild and dumper round trips (#1202) #1445 module passes after the DICT_GAPS rows are removed.
  • Legs. Foundation 528, dumpkit 227, internet 601, rest 1126 and project 606, all OK.
  • Scope. fix(ipv6): to_dict() collapses repeated extension headers, so the dict rebuild silently shortens the packet #1453 is correctly listed as not fixed here.

Nit: the get_annotations fallback is untested on Python versions before 3.10. CI starts at 3.10.

Separately, unrelated to this PR: PCAPNG block layers need constructor keywords to rebuild, in both the dict and object form. That predates this PR, and the PR doesn't claim it.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@JarryShaw
JarryShaw merged commit ca5f6db into main Oct 9, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1447-to-dict-payload-roundtrip branch October 9, 2026 02:41
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(protocols): from_data(info.to_dict()) drops the payload because to_dict omits __next_type__/__next_name__

1 participant