fix: remove content-type from headers in _add_from_file to avoid RuntimeError#791
fix: remove content-type from headers in _add_from_file to avoid RuntimeError#791juliosuas wants to merge 7 commits intogetsentry:masterfrom
Conversation
…imeError
The recorder stores content_type as a top-level YAML key but may also
capture a 'content-type' entry inside the headers dict (since HTTP
servers return Content-Type as a response header).
When _add_from_file() calls add() with both content_type= and
headers={'content-type': ...}, add() raises:
RuntimeError: You cannot define both content_type and
headers[Content-Type].
Fix: strip any 'content-type' key from headers before calling add()
when content_type is also present in the loaded response dict. The
content_type kwarg takes precedence, which is already documented as
the recommended approach.
Fixes getsentry#741
|
One thing to note: this only affects files loaded via |
markstory
left a comment
There was a problem hiding this comment.
It would be good to have a test covering this scenario. I could help with the test if you can provide an example payload that is currently not working that will work after this change.
|
Added a regression test in It creates a YAML fixture with
The test fails with |
| "body": "created", | ||
| "status": 201, | ||
| "headers": { | ||
| "Content-Type": "text/plain", |
There was a problem hiding this comment.
| "Content-Type": "text/plain", | |
| "Content-Type": "text/html", |
Having a different value for headers and content_type will better capture the coalescing behavior mentioned in the comment.
Fixes the case where a YAML fixture has Content-Type in both headers and content_type fields — the recorder captures both, causing RuntimeError when _add_from_file calls add(). Added test_add_from_file_content_type_in_headers to verify the fix works end-to-end with a YAML fixture.
f86aa76 to
806fb9c
Compare
|
Good catch @markstory — updated the test to use |
…dence
Per markstory's review: using different values for headers['Content-Type']
('text/html') and content_type ('application/json'/'text/plain') makes the
assertion non-trivial — it now explicitly verifies that content_type wins
over the conflicting header, not just that both happen to carry the same value.
|
|
||
| @responses.activate | ||
| def run(): | ||
| responses._add_from_file(file_path=self.out_file) |
There was a problem hiding this comment.
You'll either need to save your file as toml, or switch the loader to yaml for this test.
As suggested by @markstory — the fixture file needs an explicit extension so that _add_from_file can detect the correct loader. Changed out_file to use a .yaml suffix for the content_type_in_headers test.
|
Good catch @markstory — added |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
The content_type test creates response_record.yaml but teardown_method only deleted response_record (no extension). Add cleanup for .yaml and .toml variants to avoid stale artifacts across test runs.
|
Fixed both issues flagged by Cursor Bugbot:
|
|
Both issues flagged by Cursor Bugbot have been addressed (teardown cleanup for yaml/toml variants, and the non-trivial precedence assertion with mismatched values). The PR has @markstory's approval and all CI checks should be passing. Ready to merge when you have a moment — thanks! |
|
Hi @markstory — just wanted to check in on this one. The fix is minimal (strips duplicate |
CI is failing. |
Root cause of the CI failure: test_add_from_file[tomli_w] monkey-patches responses.mock._parse_response_file to a TOML loader and never restores it, so test_add_from_file_content_type_in_headers inherits the TOML parser and chokes on the YAML fixture (tomllib.TOMLDecodeError). The prior fixes (.yaml extension, .yaml/.toml teardown cleanup) don't help because _parse_response_file always calls yaml.safe_load regardless of file extension — the bug is in test state leakage, not the loader dispatch. - teardown_method: always reset _parse_response_file back to the class default so any per-test monkey-patch is contained. - __init__.py: black wrap for the Content-Type-stripping dict comp (was 91 chars, over the 88-char limit).
|
Pushed a fix for the red CI. Root cause turned out to be test state leakage, not the Content-Type logic itself:
Fix in Also picked up a black complaint in Happy to split this into two commits if you'd prefer the test-isolation fix and the formatting nit separately. |
| cls = type(responses.mock) | ||
| responses.mock._parse_response_file = ( # type: ignore[method-assign] | ||
| cls._parse_response_file.__get__(responses.mock) | ||
| ) |
There was a problem hiding this comment.
Wouldn't it be better to unwind the mock in the test that is doing the monkey patching?
per markstory's review: capture the original `_parse_response_file` before monkey-patching it for the tomli_w parametrize variant, then restore it in a finally block so the mock is contained to the test that introduces it. removes the class-wide reset from teardown_method that was leaking responsibility upward.
|
Good call — refactored in |

Summary
Fixes #741.
The recorder stores
content_typeas a top-level YAML key and may also capture acontent-typeentry inside theheadersdict (since HTTP servers returnContent-Typeas a response header). When_add_from_file()callsadd()with bothcontent_type=andheaders={'content-type': ...}, it raises:Fix
Strip any
content-typekey fromheadersbefore callingadd()whencontent_typeis also present in the loaded YAML. Thecontent_typekwarg takes precedence (already recommended by the library).This lets recorded YAML files be used in tests without any manual editing.