NewBytesResponse and NewStringResponse sets ContentLength

[vrnvu]

See related issue: https://github.com/jarcoal/httpmock/pull/20#issuecomment-266199101

[maxatome]

Hello @vrnvu, in which case(s) the current behavior is problematic?

[vrnvu]

Well, the current implementation is problematic on two levels. First If you know the ContentLength the default behavior should be to set the value. It's up to the user to use this information or not. Second, there is a propagation of the header value missing in the chain. I haven't looked at the code but here is some help. According to http.Response documentation:

https://pkg.go.dev/net/http#Response

        // Header maps header keys to values. If the response had multiple
    // headers with the same key, they may be concatenated, with comma
    // delimiters.  ([RFC 7230, section 3.2.2](https://rfc-editor.org/rfc/rfc7230.html#section-3.2.2) requires that multiple headers
    // be semantically equivalent to a comma-delimited sequence.) When
    // Header values are duplicated by other fields in this struct (e.g.,
    // ContentLength, TransferEncoding, Trailer), the field values are
    // authoritative.
    //
    // Keys in the map are canonicalized (see CanonicalHeaderKey).

Where the key sentence is. The field values are authoritative. You can take a look at the stdlib or how other HTTP libraries/frameworks handle this. If we had a header for ContentLength, the value of the http.Response.ContentLength is set to this value in the chain.

Imagine the following example when testing where we set the response headers:

    httpmock.RegisterResponder("GET", "http://localhost:8000/",
        func(req *http.Request) (*http.Response, error) {
            resp := httpmock.NewStringResponse(http.StatusOK, "expected")
            resp.Header.Set(headers.ContentLength, "123")
            return resp, nil
        },
    )

The code will fail if I'm consuming http.Response.ContentLength in my production code. Because httpmock has failed to propagate this value accordingly. This means that every one that is using this library has to manually patch this bug with something like:

    httpmock.RegisterResponder("GET", "http://localhost:8000/",
        func(req *http.Request) (*http.Response, error) {
            resp := httpmock.NewStringResponse(http.StatusOK,"expected")
            resp.ContentLength = 123
            return resp, nil
        },
    )

But this is another issue. The proposal of this PR is to fix the constructors to set the actual value of the ContentLength when we know it. This way we don't need to manually set the headers (which is not working to propagate) or set the ContentLength manually ourselves. And probably fixes most (if not all) of the other related problems with ContentLength headers.

This fix doesn't break anything because on one hand If someone was not relaying on the ContentLength they have never spotted this issue and don't care if the ContentLength is set to -1 or the actual value. On the other hand, for all code relying on the actual value of ContentLength is set this fix will save the effort of manually setting the http.Response.ContentLength field.

[maxatome]

I am sorry as I didn't well express my question. Do you have a use case that the current behavior breaks?

[vrnvu]

On a second thought, after reading all the code-base I've seen that mock of http.Response isn't fully compliant on the RFC in runtime so the proposal is not safe to merge. I.e we can have a response with invalid status, transfer-encoding values, the request method is also relevant sometimes... 👍 If this is "breaking" or "problematic" up to the user I guess. It's a mock after all. I initially thought this was handled in the transport layer. It's not.

For anyone reading this issue in the future and wants to have a complex http.Response as I mentioned. The steps to proceed are basically generating your own http.Response The public API either returns a type Responder func(*http.Request) (*http.Response, error) or a *http.Response so there won't be any problem.

I wanted to fix the upstream because it was easier, I guess it's not the case. Thanks for your time. As a side note, it would be nice if the header values are propagated to the http.Response from the headers map in transport. So the code above works when setting headers.