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 force-pushed the refactor/1484-multiinfo branch from 2cd7aee to 7b7f2c1 Compare October 10, 2026 03:05
@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 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

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
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