Multipart Improvements

[simov]

After I changed the behavior of the multipart API in #1185 a few issues popped out because of it: #1243 fixed here #1253, then #1271 fixed here #1276 so I thought we could handle that in a better way.

Detect body streams

This one overrides the rest of the options. In case the user set a stream as the body value, then chunked encoding will be used and all of the data will be piped with combined-stream.

The detection for a stream is here https://github.com/simov/request/blob/e74000d7ed82e293e46e7a8bce167eae144873ca/request.js#L1442-L1444 - no idea if that's a reliable solution or not.

Essentially this case will allow existing clients to continue to work without any changes, in other words this fixes the introduced breaking change for CouchDB users and possibly others as well.

Always chunked

By setting the transfer-encoding:'chunked' header explicitly you get the expected behavior from it - request will be chunked and body parts will be piped always.

Using multipart object options

Having the object option here is good in case we want to add more features to it, or extract this API into a separate module. The chunked option here overrides the transfer-encoding:'chunked' header (if set), also the detection of body streams is still viable here.

[simov]

Summoning @dscape @triccardi-systran @nylen @FredKSchott @tbuchok and everyone else.

[thomas-riccardi]

I haven't tested the PR.

For the stream detection, maybe use this: https://github.com/rvagg/isstream

With your proposal we loose non-chunked encoding for streams we know the size of (this is the case for fs.createReadStream()). It may be an issue if the receiving server doesn't handles correctly chunked encoding (like couchdb seem to do, although it seems also related to multipart).
Maybe add an option to force the non-chunked encoding by pre-setting the size (or just use Content-Length header).

I'm not sure it is really useful, but in any case we should have a sane behavior if the Content-Lengtt header is set by the user with a stream body. Indeed the user explicitly sets the content-length, and would with your path implicitly set chunked encoding. This would create an invalid HTTP request, and I think it's the implicit behavior that is wrong in this case.

So I propose, in order to solve this issue, to disable auto chunked encoding (by detecting stream body) if Content-Length is set.

If the user explicitly sets inconsistent headers (transfer-encoding: chunked + content-length), then it's its falt: at most request could raise an error, or just send the invalid request, as is was explicitly specified by the user.

The last remaining case is if multipart: {chunked: true} is set with Content-Length. I think we should apply the same solution as the previous point.

[simov]

Thanks for the heads up, I really haven't thought about that. Will do a bit more testing and fix the solution according to your notes.

[simov]

@triccardi-systran this actually can't happen that way. Content length should be set for each multipart individually much like form-data is doing it through the knownLength option. So that could be possible only through the multipart object case.

As for detecting streams, probably https://github.com/rvagg/isstream would be better to use.

[thomas-riccardi]

@simov my bad, I wrote my previous commend under the assumption that the chunked variable applied to all requests, including non-multipart ones.
Indeed, in case of multipart, the user cannot set the global Content-Length itself: it doesn't know the length of the boundary, so it cannot know the length of the whole body.

However, we could handle the case where all stream parts have their Content-Length part header set: in this case we don't really need chunked encoding.

Another corner-case scenario: the user explicitly sets multipart: { chunked: false}, with stream parts. Your automatic chunk choice on streams parts will silently override the explicit user choice. This may not be a good idea. Maybe throw an error in this case?
I don't know what is the usual behavior of request in such case, do we have the notion of user error? If not it may be a good feature to add.

Finally, more and more this multipart/related feature converges toward what node-form-data does for multipart/form-data. We could probably merge the two and just add non multipart/form-data mimetypes support to node-form-data (and maybe rename it..).

[simov]

Yes, re-using most of the form-data logic would be great (probably removing the submit part), then adding support for multipart/related as both are very similar.

A couple of problems here: currently I don't think I'm going to do it, having our own generic multipart module means we'll have to support it.

As for the per multipart body content length, as I said that would be possible only through the object option, as the multipart/related body is not bound to any specific syntax.

For the

multipart: { chunked: false}, with stream parts

that would throw an error anyway, if we don't try to send it as chunked under the hood, so it would be wrong in any case. I just try to make it work.

[simov]

Just because it might be lost in translation: the goal of this PR is to both fix the breaking change introduced in #1185 and at the same time satisfy its requirements.

Again, the main reason for this PR is to fix the API (the multipart option) to work without changes for both existing users, and those who want body streams.

It doesn't change or add anything else, so if https://github.com/rvagg/isstream is better option for detecting stream objects, I'm fine with adding it. Otherwise this PR should be good for merging. It doesn't cover each and every possible input combination, but at least it's more flexible.

[thomas-riccardi]

@simov I think it's necessary to use isstream; and with that your PR will be complete and good to merge!

[simov]

@triccardi-systran thanks for your feedback. @ALL added https://github.com/rvagg/isstream as a module dependency. In case there are no objections I'll merge this PR in a few days.