fix: give each request configuration and headers inspection handler its own HeadersCollection - #726
Conversation
…ts own HeadersCollection RequestConfiguration.headers, HeadersInspectionHandlerOption's two collections and HeadersInspectionHandler's option object were all defaults evaluated once, so every instance built without an explicit value shared them and headers leaked between unrelated requests.
Vincent Biret (baywet)
left a comment
Vincent Biret (baywet)
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
…tory collections Same fix as the RequestConfiguration half, spelled the same way.
Vincent Biret (baywet)
left a comment
Vincent Biret (baywet)
left a comment
There was a problem hiding this comment.
Thank you for making the changes!
There was a problem hiding this comment.
🟡 Changes recommended
The new/updated test test_import_base_request_configuration_no_warning does not reliably assert “no warnings on import” as written, and the HeadersInspectionHandlerOption docstring name is inconsistent with the actual class name.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
The test reloaded nothing and len(pytest.warns()) recorded nothing, so it passed whatever the module did on import. Also name the handler correctly in the option docstring.
Head branch was pushed to by a user without write access
|
|
Vincent Biret (@baywet) small changes cause of Copilot, can you pls re-review/approve? |
Overview
RequestConfiguration.headersdefaulted to oneHeadersCollection()evaluated at class definition, so every configuration built withoutheaders=shared it and a header added for one request rode along on every other request in the process.field(default_factory=HeadersCollection)gives each instance its own, as proposed in the issue.The same shape sat two levels deeper on the http package.
HeadersInspectionHandlerOption.__init__usedHeadersCollection()as the default for bothrequest_headersandresponse_headers; theif request_headers else HeadersCollection()guard never replaced them becauseHeadersCollectionhas no__len__, so an empty collection is truthy. AndHeadersInspectionHandler.__init__usedHeadersInspectionHandlerOption()as its default, so every handler built without options shared one option object and, through it, the same two collections: the headers one client inspected showed up on, and were cleared by, another. Both defaults areNonenow and the constructor creates a fresh object.Related Issue
Fixes #507
Notes
The other middleware constructors (
RedirectHandler,RetryHandler,UserAgentHandler,UrlReplaceHandler,ParametersNameDecodingHandler) take their option object as a function default too, so all handlers of one class share one option. Those options carry configuration values rather than per-request state, so the effect is limited to mutatinghandler.optionsafter construction. Left out of this PR to keep it on #507; happy to send the sameOptional[...] = Nonechange for them separately.Testing Instructions
cd packages/abstractions && pytest tests/test_base_request_configuration.py: two configurations get distinct collections, a header added to one is not visible on the other, a fresh configuration starts empty. Fails on main.cd packages/http/httpx && pytest tests/middleware_tests/test_headers_inspection_handler.py: two options do not share request or response collections, two handlers do not share an option object. Both fail on main.