Gracefully handle a request body that can't be deepcopied - #1024
Open
agu2347 wants to merge 1 commit into
Open
Conversation
before_record_request() unconditionally called copy.deepcopy(request)
before running any filter functions. A request's body can contain
something that isn't deepcopy-able, most commonly a raw, open file
handle -- e.g. aiohttp file uploads, which are commonly passed
directly as part of the request body (data={"file": open(path, "rb")})
rather than pre-read into bytes. deepcopy()ing an open file handle
raises TypeError: cannot pickle '...' instances, crashing the entire
recording attempt for any request containing one, exactly as
described in the issue.
The `request` object here is VCR's own internal representation
(named vcr_request throughout the aiohttp stub, for example), not a
reference to the live, actual network request -- so if the deepcopy
fails, falling back to using the original object directly (letting
filter functions mutate it in place instead of a copy) is safe. It's
still far better than crashing and preventing the upload/recording
from working at all.
Verified directly: confirmed deepcopy() on a real open file handle
reproduces the exact reported TypeError, and confirmed
_build_before_record_request()'s returned function crashes on a
request whose body contains one, with the original code. With the
fix, the same request is handled correctly, and header filtering
still takes effect. Also verified the normal case (a request with an
ordinary bytes/str body) is completely unaffected: it's still
properly deep-copied and isolated from the original request object,
exactly as before.
Added a regression test using a real open file handle as part of the
request body, going through the actual use_cassette()/filter_request()
path. Confirmed the test fails with the original code (raises
TypeError) and passes with the fix. Ran the full existing
test_vcr.py suite (26 passed: 25 baseline + 1 new), no regressions.
Fixes kevin1024#737
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #737.
before_record_request()unconditionally calledcopy.deepcopy(request)before running any filter functions. A request's body can contain something that isn't deepcopy-able, most commonly a raw, open file handle -- e.g. aiohttp file uploads, which are commonly passed directly as part of the request body (data={"file": open(path, "rb")}) rather than pre-read into bytes.deepcopy()ing an open file handle raisesTypeError: cannot pickle '...' instances, crashing the entire recording attempt for any request containing one, exactly as described in the issue.The
requestobject here is VCR's own internal representation (namedvcr_requestthroughout the aiohttp stub, for example), not a reference to the live, actual network request -- so if the deepcopy fails, falling back to using the original object directly (letting filter functions mutate it in place instead of a copy) is safe. It's still far better than crashing and preventing the upload/recording from working at all.Testing: verified directly: confirmed
deepcopy()on a real open file handle reproduces the exact reportedTypeError, and confirmed_build_before_record_request()'s returned function crashes on a request whose body contains one, with the original code. With the fix, the same request is handled correctly, and header filtering still takes effect. Also verified the normal case (a request with an ordinary bytes/str body) is completely unaffected: it's still properly deep-copied and isolated from the original request object, exactly as before.Added a regression test using a real open file handle as part of the request body, going through the actual
use_cassette()/filter_request()path. I confirmed the test fails with the original code (raisesTypeError) and passes with the fix. Ran the full existingtest_vcr.pysuite (26 passed: 25 baseline + 1 new), no regressions.