Skip to content

fix(httpv2): give make() a usable default for frame types with a fixed-size payload - #2395

Open
JarryShaw wants to merge 1 commit into
mainfrom
fix/2274-httpv2-frame-default-v2
Open

JarryShaw wants to merge 1 commit into
mainfrom
fix/2274-httpv2-frame-default-v2

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — bug fix

Description

Closes #2274.

  • Problem: HTTP.make defaulted frame to b'', packed verbatim with Length 0, so PRIORITY, RST_STREAM, PUSH_PROMISE, PING, GOAWAY and WINDOW_UPDATE could not be read back and HTTP(type=<T>, sid=...) raised. _make_http_settings also raised on settings=None, though RFC 9113 §6.5 permits a SETTINGS frame carrying no parameters.
  • Fix: frame now defaults to None, dispatching to the frame type's own _make_http_* constructor — the same as frame={} — while an explicit b'' still packs raw, unchanged. _make_http_settings treats settings=None as no parameters.
  • Judgment call: chose dispatching over raising a clearer error on the default call. Every other make default already produces a valid packet, so a well-worded exception would still be an API wart; dispatching costs nothing since each constructor already has working per-field defaults, and it makes the default and frame={} byte-identical for every frame type.
  • Test: test_httpv2_frame_default_2274_unit.py pins the octets all ten frame types emit by default, that they match frame={} and round-trip, that explicit bytes stay raw, and both empty and populated SETTINGS frames. test_http_unit.py's constructor sweep now pins settings=None building rather than raising; test_http_message_prefix_2298_unit.py and test_httpv2_error_default_2311_unit.py updated for the prose/cases that pinned the old behaviour.
  • Fail-without-fix: 23 failed / 8 passed on the new test file against unfixed source; all pass with the fix.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) bug Issues reporting a defect (set by the bug report template; a default, not an assessment) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 27, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Review: NEEDS CHANGES at b116d3b1a (Sonnet). Core change confirmed correct in full: bug reproduces on origin/main for all 6 affected types, frame=None/omitted/{} agree byte-exact across 1,224 cases, b'' behavior preserved across 2,856 cases, 270 passed/3055 subtests matching exactly, mypy/pylint unchanged. One real gap: no test pins that an explicit frame=b'' stays raw on a fixed-size type (e.g. PING) rather than being swept into the new None-dispatch path — the reviewer confirmed a mutation that breaks this (frame is None or (isinstance(frame, bytes) and not frame)) still passes all 270 existing tests. Adding that one assertion now.

@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 27, 2026
…d-size payload (#2274)

HTTP.make defaulted frame to b'', packed verbatim with Length 0, so PRIORITY,
RST_STREAM, PUSH_PROMISE, PING, GOAWAY and WINDOW_UPDATE could not be read
back and HTTP(type=<T>, sid=...) raised. HTTP._make_http_settings also
raised on settings=None, though RFC 9113 section 6.5 permits a SETTINGS
frame carrying no parameters.

frame now defaults to None, dispatching to the frame type's own
_make_http_* constructor -- the same as passing frame={} -- while an
explicit b'' still packs raw, unchanged. _make_http_settings treats
settings=None as no parameters rather than raising.

Chosen over raising a clearer error: every other make default already
produces a valid packet, so a well-worded exception on the default call
would still have been an API wart dressed up as a diagnostic. Dispatching
makes the default and frame={} byte-identical for every frame type, at no
extra cost since each constructor already has working per-field defaults.

Tests: test_httpv2_frame_default_2274_unit.py pins the octets all ten
frame types emit by default, that they match frame={} and round-trip,
that explicit bytes stay raw, and both empty and populated SETTINGS
frames. test_http_unit.py's constructor sweep now pins settings=None
building rather than raising, and test_http_message_prefix_2298_unit.py
and test_httpv2_error_default_2311_unit.py are updated for prose and
cases that pinned the old behaviour.
@JarryShaw
JarryShaw force-pushed the fix/2274-httpv2-frame-default-v2 branch from b116d3b to 30a6030 Compare October 27, 2026 18:12
@JarryShaw

Copy link
Copy Markdown
Owner Author

Amended b116d3b1a → 30a60304e: added test_explicit_empty_bytes_stay_raw_on_a_fixed_size_type, calling .make() directly on Frame.PING with frame=b'' (bypassing the full round-trip constructor, since a length-0 PING would legitimately fail parse — PING's payload is a fixed 8 octets; the invariant under test is build-time raw packing, not parse validity). Confirmed it fails under the exact mutation the review identified (frame is None or (isinstance(frame, bytes) and not frame)) and passes on the real fix. 271 passed / 3055 subtests, +1 from the new test.

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

Copy link
Copy Markdown
Owner Author

Review: GOOD TO GO at 30a60304e (Sonnet, round 2). Confirmed the new test's mutation-kill independently in a scratch export: the reviewer's proposed mutation fails only this one test (1 failed/270 passed), reverting restores 271/0. HTTP.__new__(HTTP).make(...) has precedent elsewhere in the test suite. All clear.

@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 27, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 27, 2026
@github-actions github-actions Bot added this to the PyPCAPKit v1.5 milestone Oct 27, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix) review: good-to-go Cross-review at the current head says ready; CI state is separate

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

fix(httpv2): make's default frame=b'' cannot build frame types with a fixed-size payload

1 participant