recorder: use a case-insensitive HTTP headers check#728
Draft
ptalbert wants to merge 2 commits intogetsentry:masterfrom
Draft
recorder: use a case-insensitive HTTP headers check#728ptalbert wants to merge 2 commits intogetsentry:masterfrom
ptalbert wants to merge 2 commits intogetsentry:masterfrom
Conversation
_recorder._remove_default_headers() does a case-sensitive check for HTTP header field names are case insensitive. The result is that for example, a lower-case 'content-type' field will not be removed. Later, when _add_from_file tries to load the data using RequestsMock.add(), a RuntimeError exception will be raised when it finds the 'content_type' kwarg was passed and 'content-type' is in the 'headers' kwargs. Fix this by simply having _remove_default_headers() do a case-insensitive check. Signed-off-by: Patrick Talbert <ptalbert@redhat.com>
Recorder._on_request does not set the content_type parameter when creating a new Response instance. When content_type is not set the Response.__init__ will always set the value to "text/plain". This means the recording file is always written with that content type and when responses are replayed they may not have their original content-type header value. For example, the python-gitlab package expects the response header content type to be 'application/json' but this information is in the recording and playing it back will fail. Fix this by having _on_request() pass in the content_type of the requests_response.header. Signed-off-by: Patrick Talbert <ptalbert@redhat.com>
markstory
reviewed
Jul 12, 2024
Comment on lines
+157
to
+158
| if content_type := requests_response.headers.get("content-type"): | ||
| response_params["content_type"] = content_type |
Member
There was a problem hiding this comment.
It would be good to have a test covering this change and validating that the Response that is created from the recording has the right parameters/headers.
|
This should also resolve #724 |
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
3 participants
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.
_recorder._remove_default_headers() does a case-sensitive check for HTTP header field names are case insensitive. The result is that for example, a lower-case 'content-type' field will not be removed. Later, when _add_from_file tries to load the data using RequestsMock.add(), a RuntimeError exception will be raised when it finds the 'content_type' kwarg was passed and 'content-type' is in the 'headers' kwargs.
Fix this by simply having _remove_default_headers() do a case-insensitive check.