Skip to content

fix(corekit): from_dict(to_dict()) loses nested values, and Info.__len__ counts hidden keys - #1334

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1290-1296-from-dict-len
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1290-1296-from-dict-len

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — ran all three on the two changed pcapkit/ files; the remaining 4 misc.py mypy and 3 infoclass.py pylint messages are identical on main
  • make test passes, and a test case covers the change — tests/corekit/* and tests/project/* one module per process, tests/protocols/{test_option_roundtrip_unit,schema/*}, plus the corekit, protocols/internet and protocols --exclude protocols/internet unittest legs
  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1290
Closes #1296

#1290. SchemaField.pack rebuilds a Mapping into self._schema before packing, so a schema flattened by to_dict and rebuilt by from_dict packs. Info.from_dict does the same for keys whose annotation resolves at runtime to an Info subclass (Optional[X] of one counts); to_dict documents that the conversion stays one-way for every other key. #1296. Info.__len__ counts only the keys __iter__ yields.

Probe, before → after: AttributeError: 'dict' object has no attribute 'pack' → b'\x07'; pcapng frame schemas failing 64/64 → 0/64; info.inner.x AttributeError → 1; len 3 ['a'] → len 1 ['a']. Over all 22 fixture captures / 1588 frames, per-Info keys and values and per-layer packed-byte digests are identical before and after; after the fix len == len(list(...)) for 13608/13608 Info objects. All 490 importable Info classes resolve and from_dict without raising.

Tests: tests/corekit/test_from_dict_roundtrip_unit.py, 14 tests + 17 subtests. Reverting both fixes fails 21 of them; restored, all pass. tests/corekit/test_infoclass.py:62 pinned the old len(bag) == 6 and is updated to 3 with a note — the only existing test changed.

Revision 2, from review:

  • The PCAP-NG case no longer reads examples/captures/many_interfaces.pcapng, which is generated and untracked, so it failed on every CI leg. It now builds 12 blocks as octets in memory, one per block schema that claims a type code (SHB with and without options, IDB ×2, EPB, SPB, NRB, ISB, DSB, CB, SJEB, and an unclaimed type). Each asserts pack() == raw as well as the round trip; verified with examples/captures/ moved aside.
  • _as_info_type guards the resolved hint with a parametrised-generic test and a TypeError fallback. Below Python 3.11 isinstance(list[Packet], type) is True while issubclass refuses it, so from_dict raised on 3.9/3.10 for 26 of 487 classes. A test pins the shape on every version by handing the resolver an object whose __class__ reports type; dropping the guard fails it here on 3.14.
  • The resolver cache moved from a module-level WeakKeyDictionary to the class's own __dict__, read back with cls.__dict__.get so no subclass inherits its parent's map. The weak map pinned self-referential classes forever (its value strongly referenced the weak key); a class attribute makes that a collectable cycle, and a gc test proves it — reinstating the weak map fails that test.
  • Accepted, with the reasons in the docstrings: a wrong-shape mapping becomes an Info carrying those keys, because validating it would reject data Info(**kwargs) has always accepted; and an unresolvable forward reference is cached as "skip", because from_dict runs per parsed packet and re-resolving every call to catch a case this package does not have costs more than it saves.

Behaviour changes: len(info) is smaller, and Info.from_dict returns Info instances where annotated. No wire format changes.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 86c1cfe7c: NEEDS CHANGES (Sonnet reviewed; the author ran on Opus). Two findings; the first is why CI is red.

1. The new test reads a capture the unit tier does not generate — CI-confirmed on every leg. tests/corekit/test_from_dict_roundtrip_unit.py::TestSchemaFromDictRoundTrip::test_pcapng_frame_schemas_round_trip fails with FileNotFound: [Errno 2] No such file or directory: 'examples/captures/many_interfaces.pcapng' on Python 3.10, 3.12, 3.13 and 3.14. It is the only failure on each: 1 failed, 2830 passed, 113 skipped on 3.10 and 1 failed, 2836 passed, 107 skipped on 3.14. examples/captures/ is generated and gitignored, so a unit-tier test cannot depend on it — build the schemas in memory, or move the case to the integration tier.

2. _nested_info_types raises a bare TypeError on Python 3.9 and 3.10 — latent, not caught by CI. A list[X] or tuple[X, ...] annotation resolves to a types.GenericAlias, for which isinstance(alias, type) is True before 3.11. The issubclass(hint, Info) then raises TypeError: issubclass() arg 1 must be a class, and that call sits outside the try, so from_dict itself raises. The reviewer measured 26 of 487 Info classes affected on 3.10 (0 on 3.14), with pcapkit.foundation.reassembly.data.ip.Packet.from_dict({'bufid': 1}) raising here and succeeding on main.

I confirmed on 3.14 that isinstance(list[int], type) is False, so the guard short-circuits there and 3.14 is genuinely unaffected — consistent with the report. The pre-3.11 half I could not re-derive locally (only 3.14 is installed here), so treat it as the reviewer's measurement plus a known CPython change rather than something I reproduced. It matters regardless: CI runs 3.10–3.14 (unit-tests.yml:57 and :137, python-compatibility.yml:36 — not lint.yml, which pins 3.14), and the 3.10 leg passing only means no test covers the path. Guard with isinstance(hint, type) and not typing.get_origin(hint), or wrap the issubclass in try/except TypeError, and add a test that would fail on 3.10.

Verified fine, so do not redo: the new module gives 69 failures / 3 passes on a copy of main. All 23 fixtures, 1605 frames and 18112 Info objects produce identical per-frame to_dict digests on both sides. The __len__ change has no consumer — static grep finds the dumpers using to_dict only, the len() hits in ipv4.py:1644 and sctp are on bytes, and ipv6.py:330's if exthdr is on a list; a runtime trace of every Info.__len__ caller across five legs found no pcapkit/ caller. The edited test_infoclass.py assertion is right: __excluded__ makes the old 6 (visible, extra, renamed items, hidden, __map__, __map_reverse__) into a correct 3. mypy's 4 errors on misc.py are present on the base file.

Three resolver findings, none blocking: a wrong-shape mapping becomes an Info with arbitrary keys and no error; a forward reference whose target does not exist at the first from_dict is cached as "skip" and never retried; and the WeakKeyDictionary leaks self-referential classes, because the cached value strongly references the key (a Node with nxt: Optional['Node'] survived gc.collect()). Worth a line in the docstring at least.

@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 7, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 7, 2026
…n__ counts hidden keys

``SchemaField.pack`` now rebuilds a mapping into its declared schema, so a
schema round-tripped through ``to_dict``/``from_dict`` packs instead of
raising ``AttributeError: 'dict' object has no attribute 'pack'`` (#1290).
``Info.from_dict`` does the same for keys whose annotation resolves at
runtime to an ``Info`` subclass, and ``to_dict`` now documents that the
conversion stays one-way for every other key.

``Info.__len__`` counts only the keys ``__iter__`` yields, so
``len(info) == len(list(info))`` (#1296).

Closes #1290
Closes #1296
@JarryShaw
JarryShaw force-pushed the fix/1290-1296-from-dict-len branch from 86c1cfe to dcb1306 Compare October 7, 2026 19:25
@JarryShaw JarryShaw added 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: needs-changes Cross-review at the current head says changes are required; see the verdict comment review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on dcb130696: GOOD TO GO (Sonnet reviewed; author Opus). Both round-1 blockers are fixed.

  1. No capture dependency. test_pcapng_frame_schemas_round_trip is replaced by test_pcapng_block_schemas_round_trip, which builds 12 PCAP-NG blocks in memory and round-trips each one through unpack/pack/from_dict(to_dict()). With examples/captures/ moved out of the way, the module gives 14 passed, 17 subtests.
  2. The pre-3.11 GenericAlias guard has two layers. get_origin is non-None for a parametrised generic, and issubclass is wrapped in try/except TypeError. A direct test with a fake that isinstance reads as a type fails when the try is removed. List[In], Dict[str, In] and 'list[In]' all resolve without raising. Only 3.14 is installed here, so the real 3.10 path is verified by reading the code and by the fake, not by execution. CI's 3.10 leg is the run that will confirm it.
  3. Resolver behaviours. Wrong-shape mappings and unretried forward references are documented accurately. The WeakKeyDictionary is gone: the map is now cached in cls.__dict__, a self-referential class is collected, and a subclass does not inherit its parent's map.

corekit leg: 607 tests, with the 5 failures being #1332's.

Nit: the docstring's "26 of 487 Info classes" does not re-derive on 3.14 (488 subclasses, 45 with a list[...]/List[...] hint). Either restate it with the interpreter and spelling it counts, or drop the number.

@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 7, 2026
@JarryShaw
JarryShaw merged commit f260f67 into main Oct 7, 2026
35 checks passed
@JarryShaw
JarryShaw deleted the fix/1290-1296-from-dict-len branch October 7, 2026 20:09
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 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(corekit): Info.__len__ counts hidden keys that iteration skips fix(schema): Schema.from_dict(to_dict()) does not rebuild nested schemas

1 participant