Skip to content

refactor(protocols): every versioned family base dispatches on read, make and from_data - #2639

Merged
JarryShaw merged 6 commits into
mainfrom
refactor/2574-family-dispatch
Nov 2, 2026
Merged

refactor(protocols): every versioned family base dispatches on read, make and from_data#2639
JarryShaw merged 6 commits into
mainfrom
refactor/2574-family-dispatch

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Nov 2, 2026

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • refactor — changes neither behaviour nor performance

Description of your pull request and other information

The versioned family bases — IGMP, IP, ICMP, L2TP and LinuxSLL — now dispatch on read, make and from_data, under the owner's rulings on #2574. The series is one commit per family, plus the conventions page.

  • version= picks the member. The member is the one version= names. Otherwise it is the one the octets carry (IP, IGMP, L2TP) or the data model carries; IGMP's data models gain a version attribute for this. A field never picks the member.
  • No default member. When nothing determines the member, construction raises ProtocolError and names version=. So do versionless ICMP/LinuxSLL octets, and a non-int version.
  • Clear errors for the wrong version. IPv4 and IPv6 check the Version nibble before parsing the schema, so IP(<v6 octets>, version=4) raises [IPv4] invalid version: 6.
  • Subclasses. A subclass of a base is constructed as itself. When building, version= must be the class's own version.

All 23 sample captures parse identically, and so do synthetic 6in4, IGMP-in-IPv4, L2TP and SLL frames. The bases raised TypeError on all three paths on main. HTTP still predates the rule; #2628 follows up.

A Sonnet cross-review returned GOOD TO GO, and its notes are folded in.

Closes #2574

@JarryShaw JarryShaw added this to the PyPCAPKit v1.6 milestone Nov 2, 2026
@JarryShaw JarryShaw added refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix) review: good-to-go Cross-review at the current head says ready; CI state is separate labels Nov 2, 2026
@JarryShaw

JarryShaw commented Nov 2, 2026

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 9cb46c434. A Sonnet cross-review checked each family on fields, octets and from_data. Its parsing differential found zero differences over the sample captures and 51 synthetic frames, which cover 6in4, IGMP in IPv4, L2TP and SLL. Every new test fails on main.

The review's notes are folded in:

  • non-int versions are refused;
  • IPv4 and IPv6 check the version nibble before parsing their schema;
  • the conventions lead sentence is reworded;
  • subclasses follow one rule.

The owner's later ruling is in too: IGMP's data model carries version, so from_data no longer reads field values.

@JarryShaw JarryShaw moved this to In review in PyPCAPKit Nov 2, 2026
@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Nov 2, 2026
@JarryShaw

JarryShaw commented Nov 2, 2026

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES at 9cb46c434: CI on Python 3.15 found a regression the review missed.

  • test_rfc_constants_one_home_1739_unit (TestNotReinlined.test_sweep) flags the new version == 6 / version != 6 literals in icmp.py, ip.py and ipv6.py as copies of a homed constant.
  • The DTLSCIDCiphertext failures on the same leg (schema round trip, re-exports) come from main, not from this PR. fix(protocols): re-export the DTLS connection-ID data and schema names #2641 fixes the re-exports.

A fix round, rebased onto main, is in progress. CI on this head is cancelled until then.

@JarryShaw
refactor(internet): dispatch the IGMP base on read, make and from_data (
250cb4f 
#2574)

- IGMP.__new__ gives the version class: version= names it on either path;
  without it, octets go by _dispatch and fields raise ProtocolError. A
  subclass of the base is created as itself.
- IGMP.__init__ takes version=: building, it must be the class's own
  version; parsing, it is handed on, as IP passes its own under that name.
- The IGMPv1/v2 and IGMPv3 data models gain version, on no octet: read
  sets it, _make_data carries it, and IGMP.from_data dispatches on it.
  Data without it raises ProtocolError.
- _member_of_version maps the int 1, 2 or 3, refusing a bool or a float;
  _dispatch uses it.
- Octets of no version raise ProtocolError('IGMP: unknown version').
- Tests: version= on both paths, fields without it, octets by their own
  version, version in the data model only, from_data, subclasses,
  make->parse round trips; the abstract-base tests check object.__new__.
@JarryShaw
refactor(internet): dispatch the IP base on read, make and from_data (#…
341736b 
…2574)

- IP.__new__ gives IPv4 or IPv6: version= names it on either path;
  without it, octets go by their Version nibble and fields raise
  ProtocolError. A subclass of the base is created as itself.
- IP.__init__ takes version=: building, it must be the class's own, which
  a subclass outside the members lacks; parsing, it is handed on, as the
  enclosing IP layer passes its own under that name.
- IPv4.unpack and IPv6.unpack check the Version nibble before the header
  schema, so another version's octets raise "[IPv4] invalid version: 6".
- IP.from_data picks the member the data's version names.
- Empty octets, or a version other than the int 4 or 6, raise ProtocolError.
- The RFC-constants sweep lists the IP version 6 in ip.py and ipv6.py as
  no copy of the IPv6 Next Header offset.
- Tests: version= on both paths, fields without it, octets by their own
  version, the Version check, from_data, subclasses, make->parse round trips.
@JarryShaw
refactor(internet): dispatch the ICMP base on read, make and from_data (
b887c47 
#2574)

- ICMP.__new__ gives the member version= names, from octets or fields.
  Without it both raise ProtocolError: an ICMP message carries no
  version, and no field picks a member. A subclass is created as itself.
- ICMP.__init__ takes version=: building, it must be the class's own, which
  a subclass outside the members lacks; parsing, it is handed on, as the IP
  layer passes its own under that name.
- ICMP.from_data picks the member whose data model it is handed; a dict,
  and a version other than the int 4 or 6, raise ProtocolError.
- The RFC-constants sweep lists ICMPv6's version 6 as no copy of the IPv6
  Next Header offset.
- Tests: version= on both paths, octets and fields without it, from_data,
  subclasses, make->parse round trips.
@JarryShaw
refactor(link): dispatch the L2TP base on read, make and from_data (#…
fa66be0 
…2574)

- L2TP.__new__ gives the member the version field names, on either path;
  without it, octets go by the nibble in bits 12-15 and fields raise
  ProtocolError. A subclass of the base is created as itself.
- L2TP.from_data picks the member the data's version names.
- _member_of_version maps the int 2 to L2TPv2; any other version, a bool
  or a float, and octets shorter than the first word raise ProtocolError.
- Module and API page prose: the base switches on version; UDP 1701 binds
  L2TPv2.
- Tests: version on both paths, fields without it, octets by their own
  nibble, from_data, a subclass, make->parse round trips.
@JarryShaw
refactor(link): dispatch the LinuxSLL base on read, make and from_data (
0f7ef36 
#2574)

- LinuxSLL.__new__ gives the member version= names, from octets or fields.
  Without it both raise ProtocolError: neither header carries a version,
  and no field picks a member. A subclass is created as itself.
- LinuxSLL.__init__ takes version=: building, it must be the class's own,
  which a subclass outside the members lacks; parsing, it is handed on.
- LinuxSLL.from_data picks the member whose data model it is handed; a
  dict, and a version other than the int 1 or 2, raise ProtocolError.
- Tests: version= on both paths, octets and fields without it, from_data,
  subclasses, make->parse round trips.
@JarryShaw
docs(conventions): list each family base's dispatch in Dispatching Ba…
fef0cb5 
…ses (#2574)

- Record the ruling: the member is the one version= names, or else the
  one the octets or the data model carry; no field picks one and no
  member is a default.
- A table gives each base's versions and what it reads from octets
  without version= and in from_data; HTTP, which dispatches otherwise,
  is described apart.
- ICMP and LinuxSLL octets and dicts, and a version a family has no member
  for, are refused with ProtocolError.
@JarryShaw
JarryShaw force-pushed the refactor/2574-family-dispatch branch from 9cb46c4 to fef0cb5 Compare November 2, 2026 07:42
@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 Nov 2, 2026
@JarryShaw

JarryShaw commented Nov 2, 2026

Copy link
Copy Markdown
Owner Author

Fix round and rebase: 9cb46c434 → fef0cb578, now on 18544c3c9. Only the IP and ICMP commits change; the other four are identical by range-diff.

  • The (4, 6) checks in the IP and ICMP bases become isinstance checks against the member classes, so they hold no literal.
  • The remaining version == 6 / != 6 comparisons are the IP version number, so they gain NOT_A_COPY entries in the constants sweep, matching the existing ones for frame.py and pcapng.py.

The patch-id changed, so the label is review: running while a Sonnet re-review checks the delta.

@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 Nov 2, 2026
@JarryShaw

JarryShaw commented Nov 2, 2026

Copy link
Copy Markdown
Owner Author

GOOD TO GO at fef0cb578, from a Sonnet re-review of the fix round.

  • isinstance rewrite: the old and new heads gave byte-identical output across a differential over IP, ICMP, member subclasses, outsiders and odd version values. Eight tuple mutants all fail the dispatch tests. No import cycle, and the apptype modules stay lazy.
  • NOT_A_COPY entries: each literal is the IP version number, and none copies IPV6_NEXT_HEADER_OFFSET. The table's preface describes owning modules, which ip.py and icmp.py are not. The per-expression entries are stricter than a module-wide UNRELATED, so they stay.
  • The sweep passes at every commit in the stack.

@JarryShaw

JarryShaw commented Nov 2, 2026

Copy link
Copy Markdown
Owner Author

The red Python 3.15 leg comes from this PR's base, not its change. It fails only the DTLSCIDCiphertext schema round trip, which main fixed in 16ccc4373 (#2645), and the re-export test that #2641 fixes. I'll rebase once #2641 merges. The verdict stays good to go.

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Coverage: 94.11% -- tests failure, so this is partial (unit tier, Python 3.15, fef0cb578, Unit Tests run failure)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 20561 177 2832 158 98.53%
pcapkit/corekit 2697 74 854 37 96.65%
pcapkit/dumpkit 281 3 100 3 98.43%
pcapkit/foundation 3475 27 1206 32 98.74%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 21633 219 5778 197 98.47%
pcapkit/toolkit 961 28 342 10 96.32%
pcapkit/utilities 512 7 144 5 98.17%
pcapkit/vendor 5256 2260 1198 194 53.61%

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

@JarryShaw
JarryShaw merged commit e87eb83 into main Nov 2, 2026
63 of 65 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Nov 2, 2026
@JarryShaw
JarryShaw deleted the refactor/2574-family-dispatch branch November 2, 2026 14:38
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Nov 2, 2026
This was referenced Nov 2, 2026
Closed
Closed
JarryShaw added a commit that referenced this pull request Nov 2, 2026
@JarryShaw
docs(changelog): fold NoPayload's move, family-base dispatch and the …
8ab5026 
…shared value helpers into 1.6.0 (#2449)

- Breaking: NoPayload and its data model and schema move from misc.null to
  misc.no_payload with no re-export, so a frame pickled by 1.5.x fails to
  load (#2636). 1.5.0's three NoPayload roles and 1.6.0's own point at
  misc.no_payload, 1.5.0's text unchanged.
- Added: the IP, L2TP, ICMP, IGMP and LinuxSLL bases give a member on read,
  make and from_data, version= naming it, with the refusals and IPv4's
  Version check ahead of the header (#2639).
- Changed: SCTP's chunk-flag and Invalid Stream Identifier reserved-octet
  messages (#2638). Folded into the TLS/DTLS entry: make() raises
  StructError for a value too wide for its field (#2638), and the
  DTLS_CIDCiphertext and TLS_ConnectionId package exports (#2641).
- The conventions entry records the version= dispatch rule (#2639) and ARP
  as the member-order reference (#2638).
- No entry for #2638's private value helpers or the Ethernet and PCAP-NG
  PAT_MAC_ADDR, as for #1844's OSPF copy; #2640's docstring fixes; or the
  test-only #2620, #2637, #2645 and #2646.
- CHANGELOG.md regenerated; util/changelog_md.py --check passes.
This was referenced Nov 2, 2026
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

Projects

PyPCAPKit
Status: Done

Milestone

PyPCAPKit v1.6

Development

Successfully merging this pull request may close these issues.

refactor(protocols): every versioned family base dispatches on read and make (IGMP, IP, ICMP, L2TP, LinuxSLL)

1 participant

@JarryShaw