Multipart Improvements

README.md

@@ -545,8 +545,7 @@ The first argument can be either a `url` or an `options` object. The only requir
   * Alternatively you can pass in an object `{chunked: false, data: []}` where
     `chunked` is used to specify whether the request is sent in
     [chunked transfer encoding](https://en.wikipedia.org/wiki/Chunked_transfer_encoding)
-    (the default is `chunked: true`).  In non-chunked requests, data items with
-    body streams are not allowed.
+    In non-chunked requests, data items with body streams are not allowed.
 * `auth` - A hash containing values `user` || `username`, `pass` || `password`, and `sendImmediately` (optional).  See documentation above.
 * `json` - sets `body` but to JSON representation of value and adds `Content-type: application/json` header.  Additionally, parses the response body as JSON.
 * `jsonReviver` - a [reviver function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse) that will be passed to `JSON.parse()` when parsing a JSON response body.

package.json

@@ -37,7 +37,8 @@
     "hawk": "1.1.1",
     "aws-sign2": "~0.5.0",
     "stringstream": "~0.0.4",
-    "combined-stream": "~0.0.5"
+    "combined-stream": "~0.0.5",
+    "isstream": "~0.1.1"
   },
   "scripts": {
     "test": "npm run lint && node node_modules/.bin/taper tests/test-*.js && npm run test-browser",

request.js

@@ -26,6 +26,7 @@ var http = require('http')
   , debug = require('./lib/debug')
   , net = require('net')
   , CombinedStream = require('combined-stream')
+  , isstream = require('isstream')
 
 var safeStringify = helpers.safeStringify
   , md5 = helpers.md5
@@ -1422,15 +1423,31 @@ Request.prototype.form = function (form) {
 Request.prototype.multipart = function (multipart) {
   var self = this
 
-  var chunked = (multipart instanceof Array) || (multipart.chunked === undefined) || multipart.chunked
-  multipart = multipart.data || multipart
+  var chunked = false
+  var _multipart = multipart.data || multipart
 
-  var items = chunked ? new CombinedStream() : []
-  function add (part) {
-    return chunked ? items.append(part) : items.push(new Buffer(part))
+  if (!_multipart.forEach) {
+    throw new Error('Argument error, options.multipart.')
+  }
+
+  if (self.getHeader('transfer-encoding') === 'chunked') {
+    chunked = true
+  }
+  if (multipart.chunked !== undefined) {
+    chunked = multipart.chunked
+  }
+  if (!chunked) {
+    _multipart.forEach(function (part) {
+      if(typeof part.body === 'undefined') {
+        throw new Error('Body attribute missing in multipart.')
+      }
+      if (isstream(part.body)) {
+        chunked = true
+      }
+    })
   }
 
-  if (chunked) {
+  if (chunked && !self.hasHeader('transfer-encoding')) {
     self.setHeader('transfer-encoding', 'chunked')
   }
 
@@ -1441,19 +1458,17 @@ Request.prototype.multipart = function (multipart) {
     self.setHeader(headerName, self.headers[headerName].split(';')[0] + '; boundary=' + self.boundary)
   }
 
-  if (!multipart.forEach) {
-    throw new Error('Argument error, options.multipart.')
+  var parts = chunked ? new CombinedStream() : []
+  function add (part) {
+    return chunked ? parts.append(part) : parts.push(new Buffer(part))
   }
 
   if (self.preambleCRLF) {
     add('\r\n')
   }
 
-  multipart.forEach(function (part) {
+  _multipart.forEach(function (part) {
     var body = part.body
-    if(typeof body === 'undefined') {
-      throw new Error('Body attribute missing in multipart.')
-    }
     var preamble = '--' + self.boundary + '\r\n'
     Object.keys(part).forEach(function (key) {
       if (key === 'body') { return }
@@ -1470,7 +1485,7 @@ Request.prototype.multipart = function (multipart) {
     add('\r\n')
   }
 
-  self[chunked ? '_multipart' : 'body'] = items
+  self[chunked ? '_multipart' : 'body'] = parts
   return self
 }
 Request.prototype.json = function (val) {

tests/test-multipart.js

@@ -10,7 +10,7 @@ function runTest(t, a) {
   var remoteFile = path.join(__dirname, 'googledoodle.jpg')
     , localFile = path.join(__dirname, 'unicycle.jpg')
     , multipartData = []
-    , chunked = a.array || (a.chunked === undefined) || a.chunked
+    , chunked = a.stream || a.chunked || a.encoding
 
   var server = http.createServer(function(req, res) {
     if (req.url === '/file') {
@@ -19,7 +19,7 @@ function runTest(t, a) {
       return
     }
 
-    if (a.headers) {
+    if (a.mixed) {
       t.ok(req.headers['content-type'].match(/multipart\/mixed/))
     } else {
       t.ok(req.headers['content-type'].match(/multipart\/related/))
@@ -86,7 +86,16 @@ function runTest(t, a) {
 
     var reqOptions = {
       url: 'http://localhost:8080/upload',
-      headers: (a.headers ? {'content-type': 'multipart/mixed'} : undefined),
+      headers: (function () {
+        var headers = {}
+        if (a.mixed) {
+          headers['content-type'] = 'multipart/mixed'
+        }
+        if (a.encoding) {
+          headers['transfer-encoding'] = 'chunked'
+        }
+        return headers
+      }()),
       multipart: a.array
         ? multipartData
         : {chunked: a.chunked, data: multipartData}
@@ -105,34 +114,57 @@ function runTest(t, a) {
   })
 }
 
-var methods = ['post', 'get']
+// array - multipart option is array
+// object - multipart option is object
+// encoding -  headers option have transfer-encoding set to chunked
+// mixed - headers option have content-type set to something different than multipart/related
+// json - json option
+// stream - body contains streams or not
+// chunked - chunked is set when multipart is object
+
+// var methods = ['post', 'get']
 var cases = [
-  {name: '-json +array',   args: {json: false, array: true}},
-  {name: '-json -array',   args: {json: false, array: false}},
-  {name: '-json +chunked', args: {json: false, array: false, chunked: true}},
-  {name: '-json -chunked', args: {json: false, array: false, chunked: false}},
-
-  {name: '-json +headers +array',   args: {json: false, headers: true, array: true}},
-  {name: '-json +headers -array',   args: {json: false, headers: true, array: false}},
-  {name: '-json +headers +chunked', args: {json: false, headers: true, array: false, chunked: true}},
-  {name: '-json +headers -chunked', args: {json: false, headers: true, array: false, chunked: false}},
-
-  {name: '+json +array',   args: {json: true, array: true}},
-  {name: '+json -array',   args: {json: true, array: false}},
-  {name: '+json +chunked', args: {json: true, array: false, chunked: true}},
-  {name: '+json -chunked', args: {json: true, array: false, chunked: false}},
-
-  {name: '+json +headers +array',   args: {json: true, headers: true, array: true}},
-  {name: '+json +headers -array',   args: {json: true, headers: true, array: false}},
-  {name: '+json +headers +chunked', args: {json: true, headers: true, array: false, chunked: true}},
-  {name: '+json +headers -chunked', args: {json: true, headers: true, array: false, chunked: false}}
+  // based on body type
+  {name: '+array -stream',   args: {array: true, encoding: false, stream: false}},
+  {name: '+array +stream',   args: {array: true, encoding: false, stream: true}},
+  // encoding overrides stream
+  {name: '+array +encoding',   args: {array: true, encoding: true, stream: false}},
+
+  // based on body type
+  {name: '+object -stream',   args: {object: true, encoding: false, stream: false}},
+  {name: '+object +stream',   args: {object: true, encoding: false, stream: true}},
+  // encoding overrides stream
+  {name: '+object +encoding',   args: {object: true, encoding: true, stream: false}},
+
+  // based on body type
+  {name: '+object -chunked -stream',   args: {object: true, encoding: false, chunked: false, stream: false}},
+  {name: '+object -chunked +stream',   args: {object: true, encoding: false, chunked: false, stream: true}},
+  // chunked overrides stream
+  {name: '+object +chunked -stream',   args: {object: true, encoding: false, chunked: true, stream: false}},
+  // chunked overrides encoding
+  {name: '+object +encoding -chunked',   args: {object: true, encoding: true, chunked: false, stream: false}},
+  // stream overrides chunked
+  {name: '+object +encoding -chunked +stream',   args: {object: true, encoding: true, chunked: false, stream: true}}
 ]
 
-methods.forEach(function(method) {
-  cases.forEach(function (test) {
-    tape('multipart related ' + method + ' ' + test.name, function(t) {
-      test.args.method = method
-      runTest(t, test.args)
+var suite = ['post', 'get'].forEach(function(method) {
+  [true, false].forEach(function(json) {
+    [true, false].forEach(function(mixed) {
+      cases.forEach(function (test) {
+        var name = [
+          'multipart related', method,
+          (json ? '+' : '-') + 'json',
+          (mixed ? '+' : '-') + 'mixed',
+          test.name
+        ].join(' ')
+
+        tape(name, function(t) {
+          test.args.method = method
+          test.args.json = json
+          test.args.mixed = mixed
+          runTest(t, test.args)
+        })
+      })
     })
   })
 })