Skip to content

Commit 347c88e

Browse files
alexindigoalexindigo
committed
Second iteration of cleanup.
1 parent 7c06ae6 commit 347c88e

7 files changed

Lines changed: 126 additions & 74 deletions

File tree

‎.eslintignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
node_modules/*

‎lib/form_data.js‎

Lines changed: 67 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ var parseUrl = require('url').parse;
77
var fs = require('fs');
88
var mime = require('mime-types');
99
var async = require('async');
10+
var populate = require('./populate.js');
1011

1112
module.exports = FormData;
1213
function FormData() {
@@ -26,9 +27,13 @@ FormData.LINE_BREAK = '\r\n';
2627
FormData.DEFAULT_CONTENT_TYPE = 'application/octet-stream';
2728

2829
FormData.prototype.append = function(field, value, options) {
29-
options = (typeof options === 'string')
30-
? { filename: options }
31-
: options || {};
30+
31+
options = options || {};
32+
33+
// allow filename as single option
34+
if (typeof options == 'string') {
35+
options = {filename: options};
36+
}
3237

3338
var append = CombinedStream.prototype.append.bind(this);
3439

@@ -142,51 +147,26 @@ FormData.prototype._trackLength = function(header, value, options) {
142147
}
143148
};
144149

150+
// TODO: Use request's response mime-type
145151
FormData.prototype._multiPartHeader = function(field, value, options) {
146152
// custom header specified (as string)?
147153
// it becomes responsible for boundary
148154
// (e.g. to handle extra CRLFs on .NET servers)
149-
if (options.header != null) {
155+
if (options.header) {
150156
return options.header;
151157
}
152158

159+
var contentDisposition = this._getContentDisposition(value, options);
160+
var contentType = this._getContentType(value, options);
161+
153162
var contents = '';
154163
var headers = {
155-
'Content-Disposition': ['form-data', 'name="' + field + '"'],
156-
'Content-Type': []
164+
// add custom disposition as third element or keep it two elements if not
165+
'Content-Disposition': ['form-data', 'name="' + field + '"'].concat(contentDisposition || []),
166+
// if no content type. allow it to be empty array
167+
'Content-Type': [].concat(contentType || [])
157168
};
158169

159-
// fs- and request- streams have path property
160-
// or use custom filename and/or contentType
161-
// TODO: Use request's response mime-type
162-
if (options.filename || value.path) {
163-
headers['Content-Disposition'].push(
164-
'filename="' + path.basename(options.filename || value.path) + '"'
165-
);
166-
headers['Content-Type'].push(
167-
options.contentType ||
168-
mime.lookup(options.filename || value.path) ||
169-
FormData.DEFAULT_CONTENT_TYPE
170-
);
171-
// http response has not
172-
} else if (value.readable && value.hasOwnProperty('httpVersion')) {
173-
headers['Content-Disposition'].push(
174-
'filename="' + path.basename(value.client._httpMessage.path) + '"'
175-
);
176-
headers['Content-Type'].push(
177-
options.contentType ||
178-
value.headers['content-type'] ||
179-
FormData.DEFAULT_CONTENT_TYPE
180-
);
181-
} else if (Buffer.isBuffer(value)) {
182-
headers['Content-Type'].push(
183-
options.contentType ||
184-
FormData.DEFAULT_CONTENT_TYPE
185-
);
186-
} else if (options.contentType) {
187-
headers['Content-Type'].push(options.contentType);
188-
}
189-
190170
for (var prop in headers) {
191171
if (headers[prop].length) {
192172
contents += prop + ': ' + headers[prop].join('; ') + FormData.LINE_BREAK;
@@ -196,6 +176,56 @@ FormData.prototype._multiPartHeader = function(field, value, options) {
196176
return '--' + this.getBoundary() + FormData.LINE_BREAK + contents + FormData.LINE_BREAK;
197177
};
198178

179+
FormData.prototype._getContentDisposition = function(value, options) {
180+
181+
var contentDisposition;
182+
183+
// custom filename takes precedence
184+
// fs- and request- streams have path property
185+
var filename = options.filename || value.path;
186+
187+
// or try http response
188+
if (!filename && value.readable && value.hasOwnProperty('httpVersion')) {
189+
filename = value.client._httpMessage.path;
190+
}
191+
192+
if (filename) {
193+
contentDisposition = 'filename="' + path.basename(filename) + '"';
194+
}
195+
196+
return contentDisposition;
197+
};
198+
199+
// TODO: Streamline logic here
200+
// maybe have content-type always present
201+
FormData.prototype._getContentType = function(value, options) {
202+
203+
// use custom content-type above all
204+
var contentType = options.contentType;
205+
206+
// or try `path` from fs-, request- streams
207+
if (!contentType && value.path) {
208+
contentType = mime.lookup(value.path);
209+
}
210+
211+
// or if it's http-reponse
212+
if (!contentType && value.readable && value.hasOwnProperty('httpVersion')) {
213+
contentType = value.headers['content-type'];
214+
}
215+
216+
// or guess it from the filename
217+
if (!contentType && options.filename) {
218+
contentType = mime.lookup(options.filename);
219+
}
220+
221+
// fallback to the default content type if `value` is not simple value
222+
if (!contentType && typeof value == 'object') {
223+
contentType = FormData.DEFAULT_CONTENT_TYPE;
224+
}
225+
226+
return contentType;
227+
};
228+
199229
FormData.prototype._multiPartFooter = function() {
200230
return function(next) {
201231
var footer = FormData.LINE_BREAK;
@@ -374,13 +404,3 @@ FormData.prototype._error = function(err) {
374404
this.emit('error', err);
375405
}
376406
};
377-
378-
// populates missing values
379-
function populate(dst, src) {
380-
for (var prop in src) {
381-
if (src.hasOwnProperty(prop) && !dst[prop]) {
382-
dst[prop] = src[prop];
383-
}
384-
}
385-
return dst;
386-
}

‎lib/populate.js‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// populates missing values
2+
module.exports = populate;
3+
4+
function populate(dst, src) {
5+
for (var prop in src) {
6+
if (src.hasOwnProperty(prop) && !dst[prop]) {
7+
dst[prop] = src[prop];
8+
}
9+
}
10+
return dst;
11+
}

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
"browser": "./lib/browser",
1212
"scripts": {
1313
"test": "./test/run.js",
14-
"lint": "eslint -c .eslintrc {lib/*,test/**/*}.js"
14+
"lint": "eslint lib/*.js test/*.js test/**/*.js"
1515
},
1616
"pre-commit": [
1717
"lint",

‎test/integration/test-custom-content-type.js‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,13 @@ var FIELDS = {
2626
'implicit_type': {
2727
expectedType: mime.lookup(common.dir.fixture + '/unicycle.jpg'),
2828
value: function(){ return fs.createReadStream(common.dir.fixture + '/unicycle.jpg'); }
29+
},
30+
'overridden_type': {
31+
expectedType: 'image/png',
32+
options: {
33+
contentType: 'image/png'
34+
},
35+
value: function(){ return fs.createReadStream(common.dir.fixture + '/unicycle.jpg'); }
2936
}
3037
};
3138
var fieldsPassed = false;
@@ -38,20 +45,19 @@ var server = http.createServer(function(req, res) {
3845
req.on('end', function () {
3946
// Separate body into individual files/fields and remove leading and trailing content.
4047
var fields = body.split(boundry).slice(1, -1);
48+
var fieldNames = Object.keys(FIELDS);
4149

42-
assert.ok(fields.length === 4);
43-
44-
assert.ok(fields[0].indexOf('name="no_type"') > -1);
45-
assert.ok(fields[0].indexOf('Content-Type"') === -1);
50+
assert.ok(fields.length === fieldNames.length);
4651

47-
assert.ok(fields[1].indexOf('name="custom_type"') > -1);
48-
assert.ok(fields[1].indexOf('Content-Type: ' + FIELDS.custom_type.expectedType) > -1);
52+
for (var i = 0; i < fieldNames.length; i++) {
53+
assert.ok(fields[i].indexOf('name="' + fieldNames[i] + '"') > -1);
4954

50-
assert.ok(fields[2].indexOf('name="default_type"') > -1);
51-
assert.ok(fields[2].indexOf('Content-Type: ' + FIELDS.default_type.expectedType) > -1);
52-
53-
assert.ok(fields[3].indexOf('name="implicit_type"') > -1);
54-
assert.ok(fields[3].indexOf('Content-Type: ' + FIELDS.implicit_type.expectedType) > -1);
55+
if (!FIELDS[fieldNames[i]].expectedType) {
56+
assert.equal(fields[i].indexOf('Content-Type'), -1, 'Expecting ' + fieldNames[i] + ' not to have Content-Type');
57+
} else {
58+
assert.ok(fields[i].indexOf('Content-Type: ' + FIELDS[fieldNames[i]].expectedType) > -1, 'Expecting ' + fieldNames[i] + ' to have Content-Type ' + FIELDS[fieldNames[i]].expectedType);
59+
}
60+
}
5561

5662
fieldsPassed = true;
5763
res.end();

‎test/integration/test-custom-filename.js‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ var fs = require('fs');
1212
var FormData = require(common.dir.lib + '/form_data');
1313
var IncomingForm = require('formidable').IncomingForm;
1414

15+
var knownFile = common.dir.fixture + '/unicycle.jpg';
16+
var unknownFile = common.dir.fixture + '/unknown_file_type';
17+
1518
var options = {
1619
filename: 'test.png',
1720
contentType: 'image/gif'
@@ -24,16 +27,24 @@ var server = http.createServer(function(req, res) {
2427
form.parse(req, function (err, fields, files) {
2528
assert(!err);
2629

27-
assert('my_file1' in files);
28-
assert.strictEqual(files['my_file1'].name, options.filename);
29-
assert.strictEqual(files['my_file1'].type, options.contentType);
30+
assert('custom_everything' in files);
31+
assert.strictEqual(files['custom_everything'].name, options.filename, 'Expects custom filename');
32+
assert.strictEqual(files['custom_everything'].type, options.contentType, 'Expects custom content-type');
33+
34+
assert('custom_filename' in files);
35+
assert.strictEqual(files['custom_filename'].name, options.filename, 'Expects custom filename');
36+
assert.strictEqual(files['custom_filename'].type, mime.lookup(knownFile), 'Expects original content-type');
37+
38+
assert('unknown_with_filename' in files);
39+
assert.strictEqual(files['unknown_with_filename'].name, options.filename, 'Expects custom filename');
40+
assert.strictEqual(files['unknown_with_filename'].type, mime.lookup(options.filename), 'Expects filename-derived content-type');
3041

31-
assert('my_file2' in files);
32-
assert.strictEqual(files['my_file2'].name, options.filename);
33-
assert.strictEqual(files['my_file2'].type, mime.lookup(options.filename));
42+
assert('unknown_with_filename_as_object' in files);
43+
assert.strictEqual(files['unknown_with_filename_as_object'].name, options.filename, 'Expects custom filename');
44+
assert.strictEqual(files['unknown_with_filename_as_object'].type, mime.lookup(options.filename), 'Expects filename-derived content-type');
3445

35-
assert('my_file3' in files);
36-
assert.strictEqual(files['my_file3'].type, FormData.DEFAULT_CONTENT_TYPE);
46+
assert('unknown_everything' in files);
47+
assert.strictEqual(files['unknown_everything'].type, FormData.DEFAULT_CONTENT_TYPE, 'Expects default content-type');
3748

3849
res.writeHead(200);
3950
res.end('done');
@@ -45,11 +56,15 @@ server.listen(common.port, function() {
4556
var form = new FormData();
4657

4758
// Explicit contentType and filename.
48-
form.append('my_file1', fs.createReadStream(common.dir.fixture + '/unicycle.jpg'), options);
49-
// Filename only.
50-
form.append('my_file2', fs.createReadStream(common.dir.fixture + '/unicycle.jpg'), options.filename);
59+
form.append('custom_everything', fs.createReadStream(knownFile), options);
60+
// Filename only with real file
61+
form.append('custom_filename', fs.createReadStream(knownFile), options.filename);
62+
// Filename only with unknown file
63+
form.append('unknown_with_filename', fs.createReadStream(unknownFile), options.filename);
64+
// Filename only with unknown file
65+
form.append('unknown_with_filename_as_object', fs.createReadStream(unknownFile), {filename: options.filename});
5166
// No options or implicit file type from extension.
52-
form.append('my_file3', fs.createReadStream(common.dir.fixture + '/unknown_file_type'));
67+
form.append('unknown_everything', fs.createReadStream(unknownFile));
5368

5469
form.submit('http://localhost:' + common.port + '/', function(err, res) {
5570
if (err) {

‎test/integration/test-submit-multi.js‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ var assert = common.assert;
33
var http = require('http');
44
var FormData = require(common.dir.lib + '/form_data');
55
var IncomingForm = require('formidable').IncomingForm;
6-
76
var times = 10;
7+
var server;
88

99
function submitForm() {
1010
var form = new FormData();
@@ -27,10 +27,9 @@ function submitForm() {
2727
server.close();
2828
}
2929
});
30-
3130
}
3231

33-
var server = http.createServer(function(req, res) {
32+
server = http.createServer(function(req, res) {
3433

3534
// no need to have tmp dir here, since no files being uploaded
3635
// but formidable would fail in 0.6 otherwise

0 commit comments

Comments
 (0)