Fix: Handling of empty header values and field data

This patch introduces two new events to the multipart parser:
'onHeaderEnd' and 'onHeadersEnd'. These make it much easier to properly
handle the output of the parser.

Another nice addition is a system test that verifies a complete upload
from start to finish.

Makefile

@@ -1,4 +1,4 @@
 test:
-	@find test/{simple,integration}/test-*.js | xargs -n 1 -t node
+	@find test/{simple,integration,system}/test-*.js | xargs -n 1 -t node
 
 .PHONY: test

lib/formidable/incoming_form.js

@@ -150,9 +150,7 @@ IncomingForm.prototype.handlePart = function(part) {
     });
 
     part.addListener('end', function() {
-      if (value) {
-        self.emit('field', part.name, value);
-      }
+      self.emit('field', part.name, value);
     });
     return;
   }
@@ -233,29 +231,9 @@ IncomingForm.prototype._initMultipart = function(boundary) {
 
   var parser = new MultipartParser()
     , self = this
-    , headerField = ''
-    , headerValue = ''
-    , part
-    , addHeader = function() {
-        headerField = headerField.toLowerCase();
-        part.headers[headerField] = headerValue;
-
-        var m;
-        if (headerField == 'content-disposition') {
-          if (m = headerValue.match(/name="([^"]+)"/i)) {
-            part.name = m[1];
-          }
-
-          if (m = headerValue.match(/filename="([^"]+)"/i)) {
-            part.filename = m[1].substr(m[1].lastIndexOf('\\') + 1);
-          }
-        } else if (headerField == 'content-type') {
-          part.mime = headerValue;
-        }
-
-        headerField = '';
-        headerValue = '';
-      };
+    , headerField
+    , headerValue
+    , part;
 
   parser.initWithBoundary(boundary);
 
@@ -265,25 +243,44 @@ IncomingForm.prototype._initMultipart = function(boundary) {
     part.name = null;
     part.filename = null;
     part.mime = null;
+    headerField = '';
+    headerValue = '';
   };
 
   parser.onHeaderField = function(b, start, end) {
-    if (headerValue) {
-      addHeader();
-    }
     headerField += b.toString(self.encoding, start, end);
   };
 
   parser.onHeaderValue = function(b, start, end) {
     headerValue += b.toString(self.encoding, start, end);
   };
 
-  parser.onPartData = function(b, start, end) {
-    if (headerValue) {
-      addHeader();
-      self.onPart(part);
+  parser.onHeaderEnd = function() {
+    headerField = headerField.toLowerCase();
+    part.headers[headerField] = headerValue;
+
+    var m;
+    if (headerField == 'content-disposition') {
+      if (m = headerValue.match(/name="([^"]+)"/i)) {
+        part.name = m[1];
+      }
+
+      if (m = headerValue.match(/filename="([^"]+)"/i)) {
+        part.filename = m[1].substr(m[1].lastIndexOf('\\') + 1);
+      }
+    } else if (headerField == 'content-type') {
+      part.mime = headerValue;
     }
 
+    headerField = '';
+    headerValue = '';
+  };
+
+  parser.onHeadersEnd = function() {
+    self.onPart(part);
+  };
+
+  parser.onPartData = function(b, start, end) {
     part.emit('data', b.slice(start, end));
   };
 

lib/formidable/multipart_parser.js

@@ -181,6 +181,7 @@ MultipartParser.prototype.write = function(buffer) {
       case S.HEADER_VALUE:
         if (c == CR) {
           dataCallback('headerValue', true);
+          callback('headerEnd');
           state = S.HEADER_VALUE_ALMOST_DONE;
         }
         break;
@@ -195,6 +196,7 @@ MultipartParser.prototype.write = function(buffer) {
           return i;
         }
 
+        callback('headersEnd');
         state = S.PART_DATA_START;
         break;
       case S.PART_DATA_START:

test/integration/test-multipart-parser.js

@@ -15,9 +15,9 @@ Object.keys(fixtures).forEach(function(name) {
 
     , parts = []
     , part = null
-    , headerField = null
-    , headerValue = null
-    , endCalled = false;
+    , headerField
+    , headerValue
+    , endCalled = '';
 
   parser.initWithBoundary(fixture.boundary);
   parser.onPartBegin = function() {
@@ -28,27 +28,21 @@ Object.keys(fixtures).forEach(function(name) {
   };
 
   parser.onHeaderField = function(b, start, end) {
-    var str = b.toString('ascii', start, end);
-    if (headerValue) {
-      part.headers[headerField] = headerValue;
-      headerField = '';
-      headerValue = '';      
-    }
-    headerField += str;
+    headerField += b.toString('ascii', start, end);
   };
 
   parser.onHeaderValue = function(b, start, end) {
-    var str = b.toString('ascii', start, end);
-    headerValue += str;
+    headerValue += b.toString('ascii', start, end);
   }
 
+  parser.onHeaderEnd = function() {
+    part.headers[headerField] = headerValue;
+    headerField = '';
+    headerValue = '';
+  };
+
   parser.onPartData = function(b, start, end) {
     var str = b.toString('ascii', start, end);
-    if (headerField) {
-      part.headers[headerField] = headerValue;
-      headerValue = '';
-      headerField = '';
-    }
     part.data += b.binarySlice(start, end);
   }
 

test/simple/test-incoming-form.js

@@ -401,8 +401,11 @@ test(function _initMultipart() {
     PARSER.onHeaderField(new Buffer('content-disposition'), 10, 19);
     PARSER.onHeaderValue(new Buffer('form-data; name="field1"'), 0, 14);
     PARSER.onHeaderValue(new Buffer('form-data; name="field1"'), 14, 24);
+    PARSER.onHeaderEnd();
     PARSER.onHeaderField(new Buffer('foo'), 0, 3);
     PARSER.onHeaderValue(new Buffer('bar'), 0, 3);
+    PARSER.onHeaderEnd();
+    PARSER.onHeadersEnd();
     PARSER.onPartData(new Buffer('hello world'), 0, 5);
     PARSER.onPartData(new Buffer('hello world'), 5, 11);
     PARSER.onPartEnd();
@@ -438,8 +441,11 @@ test(function _initMultipart() {
     PARSER.onPartBegin();
     PARSER.onHeaderField(new Buffer('content-disposition'), 0, 19);
     PARSER.onHeaderValue(new Buffer('form-data; name="field2"; filename="C:\\Documents and Settings\\IE\\Must\\Die\\Sunset.jpg"'), 0, 85);
+    PARSER.onHeaderEnd();
     PARSER.onHeaderField(new Buffer('Content-Type'), 0, 12);
     PARSER.onHeaderValue(new Buffer('text/plain'), 0, 10);
+    PARSER.onHeaderEnd();
+    PARSER.onHeadersEnd();
     PARSER.onPartData(new Buffer('... contents of file1.txt ...'), 0, 29);
     PARSER.onPartEnd();
   })();

test/system/test-multi-video-upload.js

@@ -0,0 +1,52 @@
+require('../common');
+var BOUNDARY = '---------------------------10102754414578508781458777923'
+  , FIXTURE = TEST_FIXTURES+'/multi_video.upload'
+  , fs = require('fs')
+  , sys = require('sys')
+  , http = require('http')
+  , formidable = require('formidable')
+  , server = http.createServer();
+
+server.addListener('request', function(req, res) {
+  var form = new formidable.IncomingForm()
+    , files = {};
+
+  form.uploadDir = TEST_TMP;
+  form.parse(req);
+
+  form
+    .addListener('field', function(field, value) {
+      assert.equal(field, 'title');
+      assert.equal(value, '');
+    })
+    .addListener('file', function(field, file) {
+      assert.equal(field, 'upload');
+      files[file.filename] = true;
+    })
+    .addListener('end', function() {
+      assert.deepEqual
+        ( files
+        , { 'shortest_video.flv': true
+          , 'shortest_video.mp4' :true
+          }
+        );
+      server.close();
+      res.writeHead(200);
+      res.end('good');
+    });
+});
+
+server.listen(TEST_PORT, function() {
+  var client = http.createClient(TEST_PORT)
+    , headers = {'content-type': 'multipart/form-data; boundary='+BOUNDARY}
+    , request = client.request('POST', '/', headers)
+    , fixture = new fs.ReadStream(FIXTURE);
+
+  fixture
+    .addListener('data', function(b) {;
+      request.write(b);
+    })
+    .addListener('end', function() {
+      request.end();
+    });
+});