Repository navigation
fix(const,vendor): make Method.get case-sensitive, per RFC 9110 (#896) - #907
Conversation
Method.get upper-cased its key before matching, so Method.get('get')
resolved to the standardised Method.GET member -- RFC 9110 Section 9.1
makes the HTTP method token case-sensitive, so a lower-case get is a
distinct, non-standardised method, not an alias of GET.
- pcapkit/vendor/http/method.py: the get() override in this crawler's
own LINE template now matches against `key` itself rather than
`key.upper()`, and falls through to build an unregistered member
(preserving the caller's own casing) exactly as before on a miss.
Command.get (FTP) is untouched: RFC 959 Section 4.1 makes FTP
command codes case-insensitive, so its equivalent override stays.
- pcapkit/const/http/method.py regenerated from that template; the
diff is confined to get()'s body and docstring. Verified
byte-reproducible across two independent offline regenerations fed
from a CSV fixture reconstructed from the committed file (no live
IANA crawl).
- Updated three pre-existing tests that pinned the old
case-insensitive Method.get behaviour (#582/#583), and
docs/source/conventions.rst's illustration of the same. Added
tests/const/test_const_method_case_sensitive_896_unit.py for the
direct repro, the unregistered-member casing guarantee, and the
crawler/generated-file parity check.
- Did not delete the override in favour of EnumRegistry.get, as the
issue suggested: measured directly, the base's get() raises
KeyError for an unmatched str key with no default, rather than
building an unregistered member -- deleting it would have made
Method.get('get') raise instead of resolving to a pseudo-member
preserving 'get', which is what issue #896 and the tests both
require.
Build/tests: `coverage run -m pytest tests/const/ tests/protocols/application/test_http_unit.py`
green (264 tests / 40325 subtests in tests/const/, 60 tests / 387
subtests in test_http_unit.py); pylint and mypy report no new
findings against pcapkit/const/http/method.py and
pcapkit/vendor/http/method.py.
JarryShaw
added
fix
|
GOOD TO GO on The base- The base never builds an unregistered member for a Verified: case sensitivity holds with caller casing preserved on the value and the name canonicalised, no minting, membership snapshot-checked at 40 unchanged. A live defect it uncovered, filed as #908. Same for The reviewer's third option would fix it for free, and is worth noting for #908: One inconsistency inside a file this PR edits, not blocking because I scoped it out myself. A merge-order hazard I own, and git will not flag it. This PR adds three Ready for you to merge. I am not merging. |
JarryShaw
added
review: good-to-go
JarryShaw
removed
the
review: good-to-go
…ONTROL resolve (#908) `Method.get` checked only `_member_map_` (names), never `_value2member_map_` (values), so `BASELINE_CONTROL`/`'BASELINE-CONTROL'` and `VERSION_CONTROL`/`'VERSION-CONTROL'` -- whose name and value differ, a hyphen being unusable in an identifier -- fell through to an unregistered member with the wrong `safe`/`idempotent`. `httpv1.py`'s permissive method regex lets a real capture reach this. - Delegate to the base `EnumLookup.get`, keeping the override only for the unregistered fallback; moved `staticmethod` -> `classmethod` since zero-argument `super()` needs `cls` to bind. - Kept `default`'s existing meaning (the fallback member's value) rather than the base's `NO_DEFAULT`/registered-value-only semantics, which would be caller-visible. - Case-sensitivity (#896/#907) and caller's-own-casing (#860) are unchanged, each pinned by its own test; fixed in the crawler template and the generated file together, verified byte-identical. Added an end-to-end `httpv1` test and a template/generated-file parity test. Build: `coverage run -m unittest` over `tests/const/` and `test_http_unit.py`, all passing. Document and pin the non-`str` surface, which this fix moves as a side effect. Delegating to the base routes a non-`str` key through `cls(key)` and so `_missing_`, which raises `ValueError`; `except KeyError` catches only a failed *name* lookup, so that `ValueError` reaches the caller. Measured before and after: get(42) AttributeError: 'int' object has no attribute 'upper' -> ValueError: 42 is not a valid Method get(None) AttributeError -> ValueError get(b'GET') RETURNED name=b'GET' value=b'GET' -> ValueError: b'GET' is not a valid Method The bytes row is a return-to-raise change, and bytes is the plausible mistake: `_RE_METHOD` is a bytes pattern and `httpv1` is bytes throughout, so `httpv1.py:434` is safe only because it wraps the match in `self.decode(...)`. Raising is the intended behaviour -- a bytes-valued `Method` is not something this registry should hand back -- so the fix is a `Raises:` clause plus four pins in a new `NonStrKeyTests`, all four of which fail against unmodified main. Added to the crawler template as well as the generated module, so regeneration cannot drop it.
…ONTROL resolve (#908) `Method.get` checked only `_member_map_` (names), never `_value2member_map_` (values), so `BASELINE_CONTROL`/`'BASELINE-CONTROL'` and `VERSION_CONTROL`/`'VERSION-CONTROL'` -- whose name and value differ, a hyphen being unusable in an identifier -- fell through to an unregistered member with the wrong `safe`/`idempotent`. `httpv1.py`'s permissive method regex lets a real capture reach this. - Delegate to the base `EnumLookup.get`, keeping the override only for the unregistered fallback; moved `staticmethod` -> `classmethod` since zero-argument `super()` needs `cls` to bind. - Kept `default`'s existing meaning (the fallback member's value) rather than the base's `NO_DEFAULT`/registered-value-only semantics, which would be caller-visible. - Case-sensitivity (#896/#907) and caller's-own-casing (#860) are unchanged, each pinned by its own test; fixed in the crawler template and the generated file together, verified byte-identical. Added an end-to-end `httpv1` test and a template/generated-file parity test. Build: `coverage run -m unittest` over `tests/const/` and `test_http_unit.py`, all passing. Document and pin the non-`str` surface, which this fix moves as a side effect. Delegating to the base routes a non-`str` key through `cls(key)` and so `_missing_`, which raises `ValueError`; `except KeyError` catches only a failed *name* lookup, so that `ValueError` reaches the caller. Measured before and after: get(42) AttributeError: 'int' object has no attribute 'upper' -> ValueError: 42 is not a valid Method get(None) AttributeError -> ValueError get(b'GET') RETURNED name=b'GET' value=b'GET' -> ValueError: b'GET' is not a valid Method The bytes row is a return-to-raise change, and bytes is the plausible mistake: `_RE_METHOD` is a bytes pattern and `httpv1` is bytes throughout, so `httpv1.py:434` is safe only because it wraps the match in `self.decode(...)`. Raising is the intended behaviour -- a bytes-valued `Method` is not something this registry should hand back -- so the fix is a `Raises:` clause plus four pins in a new `NonStrKeyTests`, all four of which fail against unmodified main (failures=2, errors=2; four distinct method names). The docstring pin reads the source via `inspect.getsource(Method.get)` rather than `Method.get.__func__`, which a `staticmethod` does not carry -- against the pre-fix module the `__func__` form died with `AttributeError` before any assertion ran, so it pinned `classmethod`-ness rather than the clause it is named for. Added to the crawler template as well as the generated module, so regeneration cannot drop it.
…ONTROL resolve (#908) `Method.get` checked only `_member_map_` (names), never `_value2member_map_` (values), so `BASELINE_CONTROL`/`'BASELINE-CONTROL'` and `VERSION_CONTROL`/`'VERSION-CONTROL'` -- whose name and value differ, a hyphen being unusable in an identifier -- fell through to an unregistered member with the wrong `safe`/`idempotent`. `httpv1.py`'s permissive method regex lets a real capture reach this. - Delegate to the base `EnumLookup.get`, keeping the override only for the unregistered fallback; moved `staticmethod` -> `classmethod` since zero-argument `super()` needs `cls` to bind. - Kept `default`'s existing meaning (the fallback member's value) rather than the base's `NO_DEFAULT`/registered-value-only semantics, which would be caller-visible. - Case-sensitivity (#896/#907) and caller's-own-casing (#860) are unchanged, each pinned by its own test; fixed in the crawler template and the generated file together, verified byte-identical. Added an end-to-end `httpv1` test and a template/generated-file parity test. Build: `coverage run -m unittest` over `tests/const/` and `test_http_unit.py`, all passing. Document and pin the non-`str` surface, which this fix moves as a side effect. Delegating to the base routes a non-`str` key through `cls(key)` and so `_missing_`, which raises `ValueError`; `except KeyError` catches only a failed *name* lookup, so that `ValueError` reaches the caller. Measured before and after: get(42) AttributeError: 'int' object has no attribute 'upper' -> ValueError: 42 is not a valid Method get(None) AttributeError -> ValueError get(b'GET') RETURNED name=b'GET' value=b'GET' -> ValueError: b'GET' is not a valid Method The bytes row is a return-to-raise change, and bytes is the plausible mistake: `_RE_METHOD` is a bytes pattern and `httpv1` is bytes throughout, so `httpv1.py:434` is safe only because it wraps the match in `self.decode(...)`. Raising is the intended behaviour -- a bytes-valued `Method` is not something this registry should hand back -- so the fix is a `Raises:` clause plus four pins in a new `NonStrKeyTests`, all four of which fail against unmodified main (failures=2, errors=2; four distinct method names). The docstring pin reads the source via `inspect.getsource(Method.get)` rather than `Method.get.__func__`, which a `staticmethod` does not carry -- against the pre-fix module the `__func__` form died with `AttributeError` before any assertion ran, so it pinned `classmethod`-ness rather than the clause it is named for. Added to the crawler template as well as the generated module, so regeneration cannot drop it.
1 participant
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/and regeneratedCHANGELOG.md, if the change is user-visible -- N/A -- changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657What is the purpose of your pull request?
fix— corrects a defectDescription of your pull request and other information
Closes #896.
Method.getupper-cased its key before matching, soMethod.get('get')resolved toMethod.GET-- RFC 9110 §9.1 makes the HTTP method token case-sensitive, sogetis a distinct, non-standardised method.Fixed in
pcapkit/vendor/http/method.py's ownget()template (this crawler renders its own, not the shared default one) and regeneratedpcapkit/const/http/method.py-- verified byte-reproducible offline from a CSV fixture reconstructed from the committed file, no live IANA crawl.Command.get(FTP) is untouched: RFC 959 §4.1 makes FTP command codes case-insensitive, so its equivalent override stays as-is.Did not delete the override in favour of the base
EnumRegistry.get, as the issue's suggested mechanism proposed: measured directly, the base raisesKeyErrorfor an unmatched string key with no default rather than building an unregistered member, so deleting it would have madeMethod.get('get')raise instead of resolving to a pseudo-member preserving'get''s casing -- the behaviour the issue and this PR's own tests require.Updated three pre-existing tests that pinned the old case-insensitive behaviour (#582/#583) and
docs/source/conventions.rst's illustration of it, and added a dedicated test module for the direct repro, the unregistered-member casing guarantee, and crawler/generated-file parity.