Add generic execute Request.t function to Cohttp_async.Client

[trevorsummerssmith]

The current client interface does not allow one to exercise full control over the headers that are sent. This is limiting. For example, if one wants to send requests to the AWS S3 API one must create a signed hash the contains all headers prior to transmission. It is not possible to do this with the current, Client interface, because headers are added after the user's call. I could imagine other situations where it is desirable to construct a Request.t and then send it.

I would propose a call that takes a Request.t and executes it. I am happy to submit a pull request for this if you all like this idea. Suggested signature:

val call :
   ?interrupt:unit Deferred.t ->
   ?body:Body.t ->
   Request.t ->
   (Response.t * Body.t) Deferred.t

[rgrinberg]

I'm in favor of this feature but I'd like to discuss the implementation a little bit more.

I think you forgot a method argument in that function. I presume that the content encoding will be inferred from the headers provided.

Also, a PR would be great!

[trevorsummerssmith]

I was thinking the method will be the Request's method. Will that work?

Yes to your point about content-encoding. To be clear the the call method will do no 'inferring' itself. It will pass along the request and body as given.

Great, I can send a pull request soon.

[rgrinberg]

Yep, the method on the request should work and I look forward to reviewing your PR.

[fxfactorial]

+1 on this issue. I'm writing an aws client and this is getting in the way. Ideally I'd like something to just take a Request.t and give back a (Response.t * Body.t) Lwt.t

[avsm]

That S3 signing is an interesting non-proxy-friendly API I hadn't seen before. Completely agreed that this should be in 0.16.0. If you think it'll take more than a few days to get a pull request then I'm happy to take a shot at a patch, @trevorsummerssmith

[trevorsummerssmith]

@avsm Pushed a pull request.
@fxfactorial the ticket I made here, and my pull request are only for Cohttp_async.

It seems another discussion #200 that the contributors you all are already having about better unifying the lwt and async interfaces? Perhaps this goes int there?

[avsm]

I've just had a chance to look closer at the S3 issue, since I found it very odd that all headers have to be signed (which would make the API very brittle to middleboxes). It turns out that there is a minimum required set:

HTTP host header
If the Content-Type header is present in the request, it must be added to the CanonicalHeaders list.
Any x-amz-* headers that you plan to include in your request must also be added. For example, if you are using temporary security credentials, you will include x-amz-security-token in your request. You must add this header in the list of CanonicalHeaders.

The headers above should never be tampered with by the call, so it should be possible to build an S3 client on the existing released Cohttp. Could you confirm this @trevorsummerssmith? Having an S3 test case (with a nocrypto SHA256 dependency) would be fantastic.

For the 0.16.0dev changes, I like the idea of having two calls with the following semantics:

• Client.request will take a raw Request.t, send it, and get the response, with no safety checks whatsoever.
• Client.call will be the existing interface, with the guarantee that any caller-supplied headers will never be overridden. So if you (for example) supply a transfer-encoding, it will be respected. This is important since the higher level users of call (the commands like get or post) need to add more API invariants such as get never sending a body.

So you should be able to sign all request headers by using Client.request (carefully), or a known subset of headers via Client.call. How does this sound?

[trevorsummerssmith]

@avsm That all sounds good. I should be able to get to testing the example today but your read looks correct. Will update later today.

[rgrinberg]

@avsm that sounds good although this sounds identical to what we've agreed on in a previous discussion

@trevorsummerssmith can you update #284 accordingly. (by creating request)

[avsm]

@rgrinberg Yep it mostly is, except I wanted to clarify the names and the semantics in one place (before sticking it in the ocamldoc ofc).