Set transfer encoding for multipart/related to chunked by default

[simov]

Fixes #1271

Also added a test for GET request.

Basically the test for this feature is

if (chunked) {
  t.ok(req.headers['transfer-encoding'] === 'chunked')
  t.notOk(req.headers['content-length'])
}

Inside the end event of the request there is no body when GET request is being used.

[simov]

@triccardi-systran can you take a look at this as well?

[nylen]

Inside the end event of the request there is no body when GET request is being used.

Why not?

[simov]

@nylen I think you are correct, at some point I didn't received anything, but I tested a few things along the way, will push.

EDIT: saw that will change it 👍

[simov]

@nylen all done, fixed my PR description as well.

[nylen]

Here is the reason (in Node http module) for Transfer-Encoding: chunked not being set on GET requests:

• when this code was first introduced
• v0.10.33: lib/http.js#L1396
• v0.11.14: lib/_http_client.js#L123

It looks to me like this is a heuristic which we can override if we assume the user knows what they're doing and really wants to send a chunked GET. Note that it applies to different methods over time (GET, HEAD at first; then CONNECT, DELETE, and OPTIONS added later on).

So, 👍 from me, but would like feedback from @triccardi-systran on whether this fixes his issue.

[thomas-riccardi]

Okay, this seems fine to me!
I confirm this fixes all the scenarios & related issues I reported in #758 and #1271.

Just one question: how do we guarantee that node.js http will not send in chunks anyway when we have chunked = false?
It seems we currently rely on the node.js http heuristics that choose well currently, but can we rely on that for the future? Is there a way to force no chunk?

[simov]

If the multipart option contains a chunked key explicitly set to false then the multipart/related parts are pushed into array and assigned to self.body inside the multipart method.

Then here self.body is an Array, and therefore the content-length header is always set.

Having the content-length header set means that you can't have a chunked encoding at the same time (as far as I understand the spec)

[nylen]

Here's the relevant core code: lib/http.js#L630

@simov do we have a test to make sure there is only one of content-length and transfer-encoding headers, not both?

[simov]

@nylen you refer to the the scenario where the user sets the content-length header and at the same time doesn't set the chunked key to false explicitly? If that's the scenario you are referring to, then no, currently there is no such check. And I can't think of any other possible scenario.

[nylen]

Ah, it's in there. I was thinking of this: https://github.com/simov/request/blob/multipart-get/tests/test-multipart.js#L28

[nylen]

Thanks @triccardi-systran and @simov for your work on this!

[thomas-riccardi]

@simov I tested this case, and since request always adds the chunked header, node http will encode in chunks, and we will get a HTTP request chunked encoded that also has a content-length header. This is an invalid HTTP request according to rfc2616 section 4.4.
Maybe we should check for content-length header before forcing chunked encoding.

[nylen]

I don't think we can or should guard against every combination of impossible settings, this one in particular is obviously user error.

[thomas-riccardi]

This is not obviously a user error since the user doesn't know that multipart is chunked encoded by default (since it's a default).
However, in this particular case, the user cannot know the real content-length since it's request that generates the boundary for the multipart, so we can assume it's indeed invalid.