Fix GZip handling for requests #4165
Conversation
tools/test-proxy/Azure.Sdk.Tools.TestProxy/Properties/launchSettings.json
Show resolved
Hide resolved
| { | ||
| var upstreamRequest = new HttpRequestMessage(); | ||
| upstreamRequest.RequestUri = GetRequestUri(incomingRequest); | ||
| upstreamRequest.Method = new HttpMethod(incomingRequest.Method); | ||
| upstreamRequest.Content = new ReadOnlyMemoryContent(incomingBody); | ||
| upstreamRequest.Content = new StreamContent(incomingRequest.Body); |
There was a problem hiding this comment.
This change was made because we now use the body directly from the incoming request, rather than the body from the RecordEntry, as the RecordEntry body would be decompressed.
There was a problem hiding this comment.
@JoshLove-msft there is something going wrong here. I hit issues with the JS test-utils. This is the raw request that is generated:
POST http://127.0.0.1:8080/sample_response/abcdef HTTP/1.1
Host: 127.0.0.1:8080
Connection: keep-alive
User-Agent: core-rest-pipeline/1.9.3 Node/v16.11.1 OS/(x64-Windows_NT-10.0.19044)
Accept-Encoding: gzip,deflate
x-ms-client-request-id: 9cfe9d23-0d2a-4438-8905-f568295da7ae
traceparent: 00-7ad45034777edc7bc1e007d10955a25b-521a4c601c0706e7-00
Content-Type: text/plain
Content-Length: 6
And we expect the body to contain abcdef which is why the Content-Length is 6. For some reason that's not what we're sending.
There was a problem hiding this comment.
Aha. I don't think we reset body position, and DecompressBody moves it forward. So when we pass the stream to StreamContent is doesn't get reset.
There was a problem hiding this comment.
Unfortunately, I think we're going to have to use the previous methodology. I don't really want to enable buffering for perf reasons.
There was a problem hiding this comment.
Ah I see. I will push out a fix, and hopefully a test.
There was a problem hiding this comment.
Yeah this is exactly what it was. Unfortunately, during record, we effectively never send a request body. We still have the headers specifying content-length, but because we have exhausted the stream to read it into the entry, when we pass the stream over to the response body, it just writes 0 bytes.
I have pushed an update. Your refactor of the GZip utility worked out great for this, even if we do a tiny bit of additional work during recording.
There was a problem hiding this comment.
Thanks, I can update the MockHttpHandler so we have some coverage of this scenario.
tools/test-proxy/Azure.Sdk.Tools.TestProxy/Common/GZipUtilities.cs
Outdated
Show resolved
Hide resolved
...zure.Sdk.Tools.TestProxy.Tests/Test.RecordEntries/request_response_with_gzipped_content.json
Show resolved
Hide resolved
There was a problem hiding this comment.
@JoshLove-msft I really appreciate this PR. I don't see any issues jump out at me. I think for the most part where people ran into this bug, they ended up disabling body matching entirely. This means that this change should be backwards compatible, but I need to actually confirm that via an eng/common update before we merge this.
I'll kick an internal build of this to get a published image and run a couple smoke-test runs of different suites that I know involve gzipped content.
Thank you! Yes, testing against the other repos first is much appreciated! |
|
/check-enforcer override |
|
Thanks @scbedd, @JoshLove-msft. |
Fixes #4103
The change here is to make sure that gzipped request bodies are stored decompressed (matching the behavior for response bodies). We also will decompress the body in playback mode before sanitization/matching.