schema: install the generated typed __init__, so construction runs __post_init__ - #430
Merged
Merged
Conversation
…`__post_init__` (#422) `schema_final` guarded the typed `__init__` it builds on `hasattr(cls, '__init__')`, which every class satisfies through `object`, so the method was never installed and `Schema.__init__` stayed bound to `__update__`. Constructing a schema therefore never reached `__post_init__`. `info_final` spells the same test correctly, on `cls.__dict__`, and did so a day later than this one was written (`43cf904ff`, then `aeb72729b`); this is the fix that never came back to `schema.py`. The consequence was not merely dead code. A schema built from a subset of its fields could not be packed at all: >>> from pcapkit.protocols.schema.transport.udp import UDP >>> bytes(UDP(srcport=53, dstport=5353)) TypeError: unsupported operand type(s) for &: 'UInt16Field' and 'int' which now returns `b'\x00\x35\x14\xe9\x00\x00\x00\x00'`. * `schema_final`: test `'__init__' not in cls.__dict__`. `pcapkit.protocols.schema.misc.null.NoPayload` is what that protects, and says so in its own comment. Forward `**kwargs` as well, so a keyword naming something other than a field keeps drawing `__update__`'s `UnknownFieldWarning` instead of a `TypeError`: the Multipath TCP options are constructed with a `kind` and a `length` that the enclosing option owns, and `MPTCP` declares them for the type checker only. * `Schema.pack`: read each field's value from the instance rather than with `getattr`, which finds the class attribute when the instance has none -- and a schema's class attribute for a field is the `FieldBase` object. That is where the error above came from, and why it named a field class rather than the field that had been left out. It is also why the `data is None` branches could never fire. * `Schema.__post_init__`: fill a field left unset from its declared default; where it declares none, drop the `NoValue` the generated `__init__` seeded, since `NoValue` is a field sentinel and not a value a schema may hold. A `None` the caller passed is kept, being a chosen value on an optional field. * `Schema.__post_init__`: pack only when a packet context was given. A schema is not in general packable from its own fields alone -- a field callback may read a key the enclosing layer owns, as `MPTCPCapable.rkey` does with the TCP option's `length` -- and `__updated__` is still set, so one left unpacked here is packed by `__bytes__` on first use, which is where its octets came from before. Construction therefore performs exactly as many `pack` calls as it did before this change, measured per schema instance. * `FieldBase._default`: declare on the class. A field class may replace `__init__` without chaining to `FieldBase`'s, as `ListField` does since a list of fields takes no default of its own, and reading `default` off one of those raised `AttributeError` for a private attribute. `Schema.from_dict` is brought into line by the same `__post_init__` change: it seeds only the keys its argument carries, so a partial dict used to raise `KeyError` naming the first field left out. Verified: every capture in `examples/captures` serialised to `tree` and `json` with `ip=True, tcp=True, reassembly=True` is byte-identical, as it must be -- parsing goes through `unpack`, which never calls `__post_init__`; extraction of `http.pcap` is unchanged at 688 ms against 692 ms. Twelve protocols `make`d and re-parsed give identical octets and identical parsed data, and `examples/generators/make_samples.py` regenerates all thirteen captures byte-identically. Bare schema construction costs 1.2 us more, 3.0 us to 4.2 us, for the `__post_init__` pass over the fields; a whole `Protocol` construction, which makes, packs and re-parses, is 2% to 6% slower on that. `tests/protocols/schema/test_schema_unit.py`: the new `test_generated_init_is_installed_and_runs_post_init` pins all of the above, and fails on a pristine tree at `assertIsNot(HeaderSchema.__init__, Schema.__update__)`. `test_schema_final_generated_init_and_legacy_version_branch` was reaching the dead branch by mocking `builtins.hasattr` to return `False`, which is no longer needed and would no longer describe anything; it now finalises the two schemas the ordinary way, and fails on a pristine tree.
There was a problem hiding this comment.
🟡 Changes recommended
Schema.__post_init__ currently overwrites an explicitly provided None with the field default, contradicting the new documented behavior and preventing callers from intentionally preserving None for conditional packing logic.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR fixes schema_final so the generated typed __init__ is actually installed, ensuring Schema.__post_init__ runs on construction, and corrects packing to read values from the instance (not class-level field objects) for unset fields.
Changes:
- Fix
schema_final’s__init__installation guard and ensure generated__init__calls__post_init__(while preserving unknown-keyword warning behavior). - Update
Schema.__post_init__andSchema.packto handle unset fields correctly (instance dict lookup; defaults and missing-value behavior). - Add/adjust unit tests to cover the previously-dead generated
__init__path and the corrected packing behavior.
File summaries
| File | Description |
|---|---|
pcapkit/protocols/schema/schema.py |
Fixes generated __init__ installation and corrects default-filling + packing behavior for unset fields. |
pcapkit/corekit/fields/field.py |
Ensures all fields have a well-defined _default so default access is reliable even when subclasses override __init__. |
tests/protocols/schema/test_schema_unit.py |
Updates legacy test scaffolding and adds a new regression test to ensure generated __init__ is installed and runs __post_init__. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
`__post_init__` guarded on `value is not NoValue and value is not None`, so a
field the caller had explicitly set to `None` fell through and was overwritten
with the field's declared default. The comment four lines below claimed the
opposite -- that such a `None` was kept -- which held only for a field declaring
no default, the one branch an explicit `None` could never reach.
The code is what was wrong, not the comment. `unpack` stores `None` for a
`ConditionalField` whose test fails, *including* one that declares a default of
its own, so substituting the default left a constructed schema disagreeing with
a parsed one about the same packet. Measured on the tree as it stood, with a
conditional `maybe` defaulting to `0xCC`:
parsed = Probe.unpack(b'\x01zz', 3, None)
parsed.to_dict() # {'kind': 1, 'maybe': None, ...}
Probe.from_dict(parsed.to_dict()).to_dict() # {'kind': 1, 'maybe': 204, ...}
`to_dict` did not survive `from_dict`. Telling "unset" from "set to `None`" is
what `NoValue` is for, and the information was already there; the guard now tests
`NoValue` alone, which also collapses the `elif` below into an `else`.
Nothing relied on the substitution. It takes a field with a real default to see
at all, which among the option schemas means only `len` on `CALIPSOOption`,
`HomeAddressOption`, `ILNPOption` and `IPDFFOption`, all defaulting to 0 -- and
every `_make_*` that builds those passes a computed integer (`8 + cmpt_len`,
`math.ceil(...)`, `16`, `2`), never `None`. Nor is there anything in the schema
package for which the substitution could have mattered on the wire: no field
callback anywhere compares a packet value against `None`, and `FieldBase.pack`
resolves a `None` from the field's own default regardless, so the octets are the
same either way. `HomeAddressOption(len=None)` and `IPDFFOption(len=None)` pack
byte-for-byte as before.
Verified: every capture in `examples/captures` serialised to `tree` and `json`
with `ip=True, tcp=True, reassembly=True` is byte-identical both to `origin/main`
and to the previous commit; the fourteen `make`-and-re-parse cases are identical
to the previous commit; `examples/generators/make_samples.py` regenerates all
thirteen captures byte-identically.
`tests/protocols/schema/test_schema_unit.py`: the new
`test_post_init_fills_the_unset_and_keeps_an_explicit_none` pins both halves --
an omitted field takes its default, an explicit `None` does not -- along with the
octets agreeing either way and the `from_dict(parsed.to_dict())` round trip. It
fails on the previous commit at `{'maybe': 204} != {'maybe': None}`.
There was a problem hiding this comment.
🟢 Approval recommended
The fix is targeted, internally consistent, and is backed by new/updated tests that directly pin the previously-dead __init__/__post_init__ path and the corrected packing behavior.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
JarryShaw
added a commit
that referenced
this pull request
Sep 17, 2026
Brings in #427 (the option-parse shortcut), #428 (dispatch no longer writing to shared registries) and #430 (the generated typed `__init__`). One conflict, in `tests/protocols/transport/test_tcp_udp_unit.py`, where both sides appended test methods to the end of `TCPUDPUnitTests`. Both kept. `pcapkit/corekit/fields/collections.py` auto-merged, which is the file worth checking by hand rather than trusting: #427 rewrote the head of the `OptionField.unpack` loop and this branch rewrote its tail, so the two touch the same function without overlapping. The merged loop reads the type field through #427's shortcut, rewinds by its `consumed`, then subtracts `len(data)`, breaks on the end-of-option-list code, and only then checks that the option moved the stream. `git diff origin/main -- pcapkit/corekit/fields/collections.py` is additions only -- none of #427's work is lost, and the guard is still after the `eool` break, which is where #431's last commit had to move it.
This was referenced Sep 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #422.
The guard could never pass
pcapkit/protocols/schema/schema.py:77readif not hasattr(cls, '__init__'):, and every class inherits__init__fromobject, so the typed__init__built just below it was never installed.Schema.__init__stayed as__update__, which fills fields but never calls__post_init__.The archaeology is decisive:
schema.py'shasattrguard is43cf904ff(2023-05-13);infoclass.py's correct'__init__' not in cls.__dict__ and cls is not Infoisaeb72729b(2023-05-14) — the next day. The pattern was fixed once and the schema copy never caught up.I had briefed this as "
infoclass.pyinstalls unconditionally, so do not touch it". That was wrong: it already carries exactly the protection, and of 398Infosubclasses none hand-writes an__init__.What it broke
Measured on
origin/mainversus this branch:So a schema built from a subset of its fields could not be packed at all, and the error named a field class rather than the missing field.
The confusing error had a separate root cause, now fixed properly:
Schema.packusedgetattr(self, field.name), which finds the class attribute — theFieldBaseobject — for an unset field.getattr(self, name, None)could therefore never returnNone, so everydata is Nonebranch was unreachable. It now reads from the instance dict.One correction to my own brief:
TCP(srcport=80, dstport=443)still cannot be packed, and should not be — the option-area length islambda pkt: pkt['offset']['offset'] * 4 - 20, sooffsetis genuinely required. It now raisesKeyError: 'offset', naming the field. Any tolerant default would emit an invalid header, since offset must be ≥ 5.Why the pack is conditional, not eager
The substantive finding. Packing unconditionally at construction breaks 39 tests across 8 files, because many schemas are not packable from their own fields alone —
MPTCPCapable.rkey's condition islambda pkt: pkt['length'] != 32, andlengthbelongs to the enclosing TCP option. The implicated modules are exactlytcp.py,mh.py,hopopt.py,ipv6_opts.py,pcapng.py,sctp.py.So
__post_init__packs only when given a packet context; otherwise__updated__stays set and__bytes__packs on first use with the enclosing context, which is where the octets came from before. Zero failures.pack()is idempotent — three consecutive calls give identical octets across six protocols — and nothing double-packs: pack calls per schema instance are identical to baseline.Verification
tree+jsonwithip/tcp/reassembly,diff -rqclean. Parsing goes throughunpack, which never calls__post_init__.make_samples.pyregenerates all 13 captures byte-identically.make+re-parse cases — same octets, same parsedinfo, same warning categories. Two fail identically on both trees (ipv4_opt_rr,ipv4_opt_ts, pre-existing).http.pcap691.9 → 688.3 ms). Bare schema construction +1.2 µs for the defaults pass; fullProtocolconstruction +2–6%. Nothing given back from perf: cut extraction time by ~46% on an HTTP capture, with byte-identical output #420 or perf: stop the flow dumper re-dissecting every frame, and options being parsed twice #427.Tests
test_generated_init_is_installed_and_runs_post_initis new and fails on a pristine tree.test_schema_final_generated_init_and_legacy_version_branchwas mockingbuiltins.hasattrto returnFalsepurely to reach the dead branch — that scaffolding is gone, and it also fails on a pristine tree.Three pre-existing defects found, not fixed
ipv4.py:1480—_make_opt_tspassesdata=ts_listwhere the field ists_data, so the IPv4 Timestamp option cannot be constructed on either tree;data=was silently dropped with anUnknownFieldWarning.tcp.py:2604–2996, 11 sites — every_make_mptcp_*passeskind=/length=, which are notMPTCPfields (declared underTYPE_CHECKINGonly), so a made MPTCP option omits its leading kind/length octets.tcp.pywas held by other work.hip.py:3445—EncryptedParameter(cipher=...);cipheris not a field.Because mypy believes those keywords are valid — the
TYPE_CHECKINGstubs declare them — the generated__init__forwards**kwargsto__update__rather than rejecting unknown keywords. Tightening that would turn all 13 sites intoTypeErrors, so it wants its own change.