fix: remove encoding from response

[rgrinberg]

one more @mefyl

[mefyl]

Same thing, good cleanup 👍

[mefyl]

It breaks two unit tests though, taking a look.

[mefyl]

While this part definitively requires cleanup, this change outputs responses with no transfer-encoding nor content-size headers, which is a problem (the only way to end the body is then to close the connection).

I think we should clean it up even further: I think there is no point in letting the user force the encoding, as she could for instance specify an incorrect content-length and ruin the protocol. This should IMO be an implementation detail entirely handled by cohttp. The only encoding feature that could arguably exist is forcing chunked encoding despite knowing the body length, which seems a bit wasteful but not incorrect per se. Thus I would suggest we

• Either entirely drop the ?encoding parameter when building responses, and just pick the best encoding when writing the request.
• Either provide a ?(force_chunked_encoding = false) parameter, which would only prevent the implementation from picking a content-length style transmission.

There is still the issue that the user can specify headers manually and thus forcefully insert erroneous headers, but I blame HTTP 1 for this, as it mixes up transport and application layers in the headers. We could either manually fix these headers, or decline any responsibility if they are set manually.

[mefyl]

Happy working on this if we agree on the desired result. It would definitely be a good thing to clean this up before 6.0.

[mefyl]

Here's already a small fix that restore the previous behavior and thus fixes the test suite : https://github.com/mefyl/ocaml-cohttp/tree/fix/remove-encoding-from-response . The main difference is that encoding is set to Chunked by default. I changed one test expectation because the server now return content-length: 0 where it was previously omitted, which is I think strictly an improvement.

I'm still a bit uneasy with the way encoding is determined, but with this on top I think this MR is already a good step forward.

[rgrinberg]

Thanks for the response. I think there's two separate concerns:

1. Providing types that reflect HTTP requests and responses as accurately as possible
2. Allowing users to construct only correct HTTP requests or responses.

My PR attempts to follow 1, while your commit helps out with 2. Looking at your commit, it seems like a good approach to me.

Your proposal to make it easier to construct correct responses with bodies seems good as well. However, I think we should still allow the users to construct incorrect requests/responses in some lower level layer. In the real world, there's plenty of situations where one has to deal with non spec compliant clients and servers.

[rgrinberg]

I've included your commit but adjusted it to not to overwrite user set headers.

[mefyl]

I agree, and I think this should be split across libraries:

• http should be an entirely agnostic tool to read and write http requests. The user should be able to forge any grammatically valid request, even semantically invalid ones.
• Starting from cohttp, we can start being opinionated and prevent semantically invalid requests, or even decide to take the encoding configuration entirely out of the user hands.