Skip to content

refactor(corekit)!: Info.to_dict() returns an OrderedMultiDict; add MultiInfo and OrderedMultiInfo (#1484) - #1523

Merged
JarryShaw merged 1 commit into
mainfrom
refactor/1484-multiinfo
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
refactor/1484-multiinfo

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

  • MultiInfo / OrderedMultiInfo (corekit/infoclass.py) are Info subclasses that also derive from MultiDict / OrderedMultiDict, via a private _MultiInfo base. They inherit from_dict, __init_subclass__ and the finalisation rules; the mapping protocol is bound back to the multi-mapping's own, since Info and Mapping precede it in the MRO. Mutators raise UnsupportedCall, keys read as attributes, repeated keys keep every value, and listvalues() hands out copies. The carve-out stays in those classes: __new__/__update__ fill the multi-mapping, __setattr__ admits only its bookkeeping, and info_final generates no __init__ for them.
  • Finalisation: Info stores every MultiDict / OrderedMultiDict value as one of them; PCAP-NG's journal and TLS entries are frozen where built; HTTP/2 Settings is an OrderedMultiInfo. In protocols/data, the 54 attribute annotations and Settings' base are OrderedMultiInfo and the 51 __init__ stubs keep OrderedMultiDict; 2 MultiDict[...] attributes are MultiInfo.
  • to_dict() returns an OrderedMultiDict at every level, as the private subclass _OrderedMultiDict, whose != negates == (the owner's option 3; multidict.py stays verbatim Werkzeug). OrderedMultiInfo gets the same __ne__, and MultiInfo's ==/!= defer to an ordered operand, so == is symmetric and != its negation across every kind (bar a bare Werkzeug OrderedMultiDict on the left of !=, or right of a bare MultiDict); copies, deep copies and pickles keep the class. from_dict() restores the *Info types. Dumpers write a repeated field as a list; the PCAP / PCAP-NG engines hand them the Info.

Breaking. InfoDict (which compared like a dict) is gone:

  • to_dict() == {...} is False and != True; dict(d), {**d} and json.dumps(d) keep only the first value of a repeated key. type(d) is OrderedMultiDict is False (isinstance holds).
  • Option lists, and their .copy(), copy.copy and deepcopy, are immutable: use OrderedMultiDict(opts) or info.to_dict(). type(opts) is OrderedMultiDict is False, and isinstance(opts, Info) is now True.
  • opts.to_dict() returns the plain multi-mapping (to_dict(flat=True) is the old dict); Settings().add() raises.
  • A writer handed to_dict() instead of the Info writes nested fields as lists of single-key mappings.

Ran one at a time: the #1484 modules, tests/corekit, tests/dumpkit, tests/protocols/{misc,transport,internet}, the capture round trip and tests/test_final_enforcement.py on 3.14, the last plus the #1484 modules on 3.11 — all green. ==/!= are pinned across all 64 pairs of kinds, equal and reordered, both ways round. 6,823 layers across 23 captures rebuild byte-exactly from info and info.to_dict(); 277 frames and 261 option lists compare != False; canonical to_dict() output and all 69 dumps match main.

Closes #1484

@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the refactor/1484-multiinfo branch from 1f9d1e1 to e2f35e7 Compare October 9, 2026 22:04
@JarryShaw JarryShaw added 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.66% (unit tier, Python 3.14, 94c3545bd, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18858 957 2366 871 91.03%
pcapkit/corekit 2334 67 746 35 96.43%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2766 129 980 47 94.23%
pcapkit/interface 108 7 36 5 91.67%
pcapkit/protocols 16991 230 4544 191 98.02%
pcapkit/toolkit 697 100 232 7 84.61%
pcapkit/utilities 431 4 124 4 98.56%
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 e2f35e7b2. Cross-reviewed on Sonnet; the author ran Opus.

Blocking:

  • CI is red on Python 3.11 and 3.12. tests/test_final_enforcement.py has 8 failures, all of them OrderedMultiDict([('x', 1)]) != {'x': 1}. The worker's selection never ran the top-level tests/test_*.py files.
  • The PR body is wrong about dict(d) and json.dumps(d). They do not expose the internal buckets: they keep the first value of a repeated key and silently drop the rest. Measured on 3.14. The to_dict docstring repeats the same claim.
  • Breaking-change list:
    • to_dict() == {...} is now False (on main the removed InfoDict defined __eq__/__ne__).
    • .copy(), copy.copy and deepcopy of an option list stay immutable.
    • HTTP/2 Settings is immutable.
    • type(opts) is OrderedMultiDict is False.
  • Annotations not retyped: protocols/data/misc/pcapng.py:159 and :359 still say OrderedMultiDict.

Confirmed:

A question for the owner. OrderedMultiDict defines __eq__ but not __ne__, so != falls back to dict.__ne__. Two equal exports then give == True and != True, and assertEqual on nested exports fails. Upstream werkzeug 2.3.8 has the same gap (structures.py:492), so this is a faithful copy.

  1. Accept it as werkzeug's behaviour and document it.
  2. Add the missing __ne__ to multidict.py, a deliberate one-method deviation from upstream.

I recommend 2: it is a one-line correctness fix, and without it equality on exports is unreliable.

@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
@JarryShaw
JarryShaw force-pushed the refactor/1484-multiinfo branch from e2f35e7 to 2cd7aee Compare October 9, 2026 23:04
@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 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 2cd7aeedf. This was a Sonnet delta review; the author ran Opus.

Still open for the owner: the __ne__ question above. If the answer is to add __ne__ to multidict.py, this PR needs one more push; otherwise it is ready as is.

@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

Copy link
Copy Markdown
Owner Author

Seems like we're not following Info subclass for the two next tho.

@JarryShaw

Copy link
Copy Markdown
Owner Author

You're right, they don't. I measured at 2cd7aeedf:

  • MultiInfo resolves as MultiInfo → _MultiInfoBase → MultiDict → dict, and OrderedMultiInfo adds OrderedMultiDict before MultiDict.
  • issubclass(MultiInfo, Info) and issubclass(OrderedMultiInfo, Info) are both False.
  • Compared with Info, they lack from_dict, __additional__, __excluded__ and Info's ABC/finalisation machinery. They do have to_dict and attribute access.

Which do you mean?

  1. Make them real Info subclasses, e.g. class MultiInfo(Info, MultiDict). Then isinstance(x, Info) holds and they inherit from_dict and the finalisation rules. The catch: Info stores declared fields, while these hold arbitrary repeated keys, so Info.__init__ and __init_subclass__ would need a carve-out.
  2. Keep the MultiDict base, but follow Info's contract: from_dict, the same immutability errors, Info-style repr, and optionally Info.register(MultiInfo) so isinstance holds. No change to Info itself.

I recommend 2. It keeps your earlier ruling that they extend werkzeug's MultiDict, and avoids special-casing Info. #1523 is on hold until you answer.

@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 Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

i prefer option 1

@JarryShaw

Copy link
Copy Markdown
Owner Author

Recorded: the owner chose option 1 on the question above. MultiInfo and OrderedMultiInfo become real Info subclasses, keeping the MultiDict/OrderedMultiDict bases, so isinstance(x, Info) holds and both inherit from_dict and Info's finalisation rules. Info.__init__ and __init_subclass__ get a carve-out for classes that hold arbitrary repeated keys rather than declared fields. The worker is starting on this branch.

One check: I've read "option 1" as answering the 00:17Z Info-subclass question. The earlier __ne__ question is still open:

  1. Accept werkzeug's behaviour and document it.
  2. Add __ne__ to corekit/multidict.py.

Your #1516 ruling not to touch corekit.multidict points at 1. Tell me if "option 1" meant that question instead, and I'll stop the worker.

@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 7b7f2c157 on the redesign. Before merging, please answer the __ne__ question; it now has a measured cost. Cross-reviewed on Sonnet; the author ran Opus.

  • Option 1 as built: MultiInfo and OrderedMultiInfo are real Info subclasses. isinstance(x, Info) holds, and from_dict(to_dict()) == x works with repeated keys. All 15 mutators raise. copy, deepcopy and pickle keep type, equality and immutability. The carve-out doesn't leak: four ordinary Info classes are md5-identical to main.
  • Round trips: 6,826 layers across 23 captures rebuild byte-exact, and the canonical to_dict() hash and the 21 json/plist/tree dumps match main.
  • Tests: green on 3.14 and 3.11.

The __ne__ cost, measured by me. On this PR, f.info.to_dict() != f.info.to_dict() is True for every frame: 5/5 on test.pcapng, 29/29 on options-tcp.pcap. On main it is 0/34. The cause is that to_dict() now returns an OrderedMultiDict, which defines __eq__ but no __ne__. Your options:

  1. Accept it and document it.
  2. Add __ne__ to corekit/multidict.py. That lifts your don't-touch rule for one method.
  3. Leave multidict.py verbatim, and give the export a thin OrderedMultiDict subclass in infoclass.py that adds __ne__.

I recommend 3: it fixes != without touching the werkzeug port.

Not blocking: MultiInfo.listvalues() hands out werkzeug's internal lists, so next(iter(m.listvalues())).append(99) mutates it (I reproduced this). OrderedMultiInfo is unaffected.

@JarryShaw JarryShaw added needs: decision Waiting on the maintainer to decide — not blocked by other work 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 10, 2026
@JarryShaw JarryShaw moved this from In review to Needs decision in PyPCAPKit Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

take option 3

@JarryShaw

Copy link
Copy Markdown
Owner Author

Recorded: you chose option 3 for __ne__. corekit/multidict.py stays a verbatim werkzeug port. infoclass.py gains a thin OrderedMultiDict subclass that adds __ne__, and to_dict() returns that subclass, so two equal exports compare == True and != False. I've read this, together with your go-ahead on the redesign, as confirming that "option 1" meant the Info-subclass question.

The worker is adding it to this branch. The change includes:

  • a test that x.to_dict() != x.to_dict() is False on every frame, which is 34/34 True on the current head;
  • a re-run of the round-trip and dump checks.

The same push fixes MultiInfo.listvalues() so it no longer hands out mutable internal lists.

@JarryShaw JarryShaw removed the needs: decision Waiting on the maintainer to decide — not blocked by other work label Oct 10, 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 Oct 10, 2026
@JarryShaw
JarryShaw force-pushed the refactor/1484-multiinfo branch 2 times, most recently from c75108e to 3c1f9b8 Compare October 10, 2026 04:18
@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 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 3c1f9b8ce. This was a Sonnet delta review; the author ran Opus. I re-measured the one remaining defect myself.

== is asymmetric between MultiInfo and the ordered classes. With p = [('x', 1)]:

a == b b == a
MultiInfo(p) vs OrderedMultiInfo(p) False True
MultiInfo(p) vs OrderedMultiDict(p) False True
bare MultiDict(p) vs OrderedMultiDict(p), on main too True True
  • Why: MultiInfo.__eq__ is bound to MultiDict.__eq__ (infoclass.py:875), which is dict's. That compares the raw storage and returns False instead of NotImplemented. A bare MultiDict avoids this only because OrderedMultiDict subclasses it, so Python tries the reflected __eq__ first. MultiInfo is new in this PR, so the asymmetry is too.
  • Suggested fix: return NotImplemented from MultiInfo.__eq__ for an OrderedMultiDict, so the ordered side decides, and bind __ne__ = _ne. Pin both operand orders in a test.

Confirmed by the reviewer:

@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 10, 2026
…ultiInfo and OrderedMultiInfo (#1484)

- corekit/infoclass: add MultiInfo and OrderedMultiInfo, Info subclasses
  that also derive from MultiDict/OrderedMultiDict. They inherit from_dict
  and Info's finalisation, keep the multi-mapping protocol, refuse every
  mutation, read keys as attributes and keep every value of a repeated key.
  Info stores every MultiDict value as one of them, so a finalised Info
  holds no mutable multi-mapping.
- Info.to_dict() returns an OrderedMultiDict at every level, and a
  MultiInfo/OrderedMultiInfo exports its plain multi-mapping; from_dict()
  restores them, including an annotated subclass (HTTP/2 Settings, now an
  OrderedMultiInfo). The OrderedMultiDict exports are a private subclass
  whose != negates ==, as OrderedMultiInfo's does, so two equal exports or
  option lists no longer compare unequal.
- corekit/multidict: drop InfoDict; the module is Werkzeug's classes only.
- dumpkit: write an Info's fields with every value of a repeated field, as a
  list; the PCAP and PCAP-NG engines hand the writers the Info itself.
- protocols/data: retype the attribute annotations to OrderedMultiInfo and
  MultiInfo; PCAP-NG freezes the entries it holds in a tuple or dict.
- tests: move the to_dict() assertions to the OrderedMultiDict contract.

Every test directory and top-level test module passes, run one at a time.
@JarryShaw
JarryShaw force-pushed the refactor/1484-multiinfo branch from 3c1f9b8 to 94c3545 Compare October 10, 2026 04: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 Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 94c3545bd. This was a Sonnet delta review; the author ran Opus.

  • Symmetry: across 27,556 operand pairs there are 0 asymmetric == results and 0 exceptions. The pairs covered all 8 kinds plus subclasses, copies, OrderedDict, mappingproxy, nested values, multi-value keys and unrelated objects. Every failing != is one of the two verbatim-werkzeug cases the test excludes. The same matrix on 3c1f9b8ce finds 24 asymmetric kind pairs, so the test can catch a regression. I checked symmetry myself on 1-key, 2-key and reordered values.
  • Semantics: these match the ported MultiDict/OrderedMultiDict. Key order is ignored on the unordered side, and value order within a key is not.
  • Exclusions: both are out of reach without editing multidict.py. Instrumenting showed MultiInfo's methods are never consulted for them. multidict.py is byte-identical to its state before fix(corekit)!: to_dict() returns an InfoDict, so a repeated IPv6 extension header survives the dict rebuild (#1453) #1466.
  • Hashing: __ne__ returns NotImplemented for non-mappings. hash() raises on both heads, which is unchanged.
  • Fail-before: 17 failed on 3c1f9b8ce.
  • Tests: these pass at the head: tests/corekit (564), test_final_enforcement.py (31), the capture round trip (20,492 subtests) and the Settings modules (67). Merged into main (a3da376dd), the merge is clean and the refactor(corekit)!: Info.to_dict() returns an OrderedMultiDict; add MultiInfo and OrderedMultiInfo to infoclass #1484 modules plus the capture round trip pass.

Non-blocking nits:

  • The test's exclusion is wider than it needs to be. A bare OrderedMultiDict != OrderedMultiInfo/Settings is correct, because Python asks the subclass first, so those pairs could be asserted.
  • The Info.to_dict note says an export is "never" equal to a plain dict. I measured that this holds except when both are empty: OrderedMultiInfo().to_dict() == {} is True.

@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 10, 2026
@JarryShaw
JarryShaw merged commit 2735d7d into main Oct 10, 2026
39 checks passed
@JarryShaw
JarryShaw deleted the refactor/1484-multiinfo branch October 10, 2026 05:17
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 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

breaking Breaks public-facing behaviour or API (apply alongside the type label) refactor Restructuring for its own sake — neither a fix nor a new capability (refactor: prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

refactor(corekit)!: Info.to_dict() returns an OrderedMultiDict; add MultiInfo and OrderedMultiInfo to infoclass

1 participant