From 8b8309fe0f4deceaf73d846f838f8b0aa95a021c Mon Sep 17 00:00:00 2001 From: David Frank Date: Thu, 26 May 2016 00:46:11 +0800 Subject: [PATCH 1/7] handle gzip encoding with 204 and 304 responses --- index.js | 11 +++++++---- test/server.js | 19 ++++++++++++++++++- test/test.js | 45 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/index.js b/index.js index ade4d51..cad8394 100644 --- a/index.js +++ b/index.js @@ -167,10 +167,13 @@ function Fetch(url, opts) { if (options.compress && headers.has('content-encoding')) { var name = headers.get('content-encoding'); - if (name == 'gzip' || name == 'x-gzip') { - body = body.pipe(zlib.createGunzip()); - } else if (name == 'deflate' || name == 'x-deflate') { - body = body.pipe(zlib.createInflate()); + // no need to pipe no content and not modified response body + if (res.statusCode !== 204 && res.statusCode !== 304) { + if (name == 'gzip' || name == 'x-gzip') { + body = body.pipe(zlib.createGunzip()); + } else if (name == 'deflate' || name == 'x-deflate') { + body = body.pipe(zlib.createInflate()); + } } } diff --git a/test/server.js b/test/server.js index c4b73df..c57cfb0 100644 --- a/test/server.js +++ b/test/server.js @@ -264,11 +264,28 @@ TestServer.prototype.router = function(req, res) { res.end('invalid json'); } - if (p === '/empty') { + if (p === '/no-content') { res.statusCode = 204; res.end(); } + if (p === '/no-content/gzip') { + res.statusCode = 204; + res.setHeader('Content-Encoding', 'gzip'); + res.end(); + } + + if (p === '/not-modified') { + res.statusCode = 304; + res.end(); + } + + if (p === '/not-modified/gzip') { + res.statusCode = 304; + res.setHeader('Content-Encoding', 'gzip'); + res.end(); + } + if (p === '/inspect') { res.statusCode = 200; res.setHeader('Content-Type', 'application/json'); diff --git a/test/test.js b/test/test.js index e13e713..fa559cb 100644 --- a/test/test.js +++ b/test/test.js @@ -416,8 +416,8 @@ describe('node-fetch', function() { }); }); - it('should handle empty response', function() { - url = base + '/empty'; + it('should handle no content response', function() { + url = base + '/no-content'; return fetch(url).then(function(res) { expect(res.status).to.equal(204); expect(res.statusText).to.equal('No Content'); @@ -429,6 +429,47 @@ describe('node-fetch', function() { }); }); + it('should handle no content response with gzip encoding', function() { + url = base + '/no-content/gzip'; + return fetch(url).then(function(res) { + expect(res.status).to.equal(204); + expect(res.statusText).to.equal('No Content'); + expect(res.headers.get('content-encoding')).to.equal('gzip'); + expect(res.ok).to.be.true; + return res.text().then(function(result) { + expect(result).to.be.a('string'); + expect(result).to.be.empty; + }); + }); + }); + + it('should handle not modified response', function() { + url = base + '/not-modified'; + return fetch(url).then(function(res) { + expect(res.status).to.equal(304); + expect(res.statusText).to.equal('Not Modified'); + expect(res.ok).to.be.false; + return res.text().then(function(result) { + expect(result).to.be.a('string'); + expect(result).to.be.empty; + }); + }); + }); + + it('should handle not modified response with gzip encoding', function() { + url = base + '/not-modified/gzip'; + return fetch(url).then(function(res) { + expect(res.status).to.equal(304); + expect(res.statusText).to.equal('Not Modified'); + expect(res.headers.get('content-encoding')).to.equal('gzip'); + expect(res.ok).to.be.false; + return res.text().then(function(result) { + expect(result).to.be.a('string'); + expect(result).to.be.empty; + }); + }); + }); + it('should decompress gzip response', function() { url = base + '/gzip'; return fetch(url).then(function(res) { From 419597fe131cec0552d5d907f145032df485a2d6 Mon Sep 17 00:00:00 2001 From: David Frank Date: Thu, 26 May 2016 01:01:56 +0800 Subject: [PATCH 2/7] user should be able to resolve cloned body before original body --- lib/body.js | 13 +++++++++---- test/test.js | 13 +++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/lib/body.js b/lib/body.js index 44f4400..8e69b5d 100644 --- a/lib/body.js +++ b/lib/body.js @@ -205,7 +205,7 @@ Body.prototype._convert = function(encoding) { * @return Mixed */ Body.prototype._clone = function(instance) { - var pass; + var p1, p2; var body = instance.body; // don't allow cloning a used body @@ -216,9 +216,14 @@ Body.prototype._clone = function(instance) { // check that body is a stream and not form-data object // note: we can't clone the form-data object without having it as a dependency if (bodyStream(body) && typeof body.getBoundary !== 'function') { - pass = new PassThrough(); - body.pipe(pass); - body = pass; + // tee instance body + p1 = new PassThrough(); + p2 = new PassThrough(); + body.pipe(p1); + body.pipe(p2); + // set instance body to teed body and return the other teed body + instance.body = p1; + body = p2; } return body; diff --git a/test/test.js b/test/test.js index fa559cb..673285b 100644 --- a/test/test.js +++ b/test/test.js @@ -939,6 +939,19 @@ describe('node-fetch', function() { }); }); + it('should allow cloning a json response, first log as text response, then return json object', function() { + url = base + '/json'; + return fetch(url).then(function(res) { + var r1 = res.clone(); + return r1.text().then(function(result) { + expect(result).to.equal('{"name":"value"}'); + return res.json().then(function(result) { + expect(result).to.deep.equal({name: 'value'}); + }); + }); + }); + }); + it('should not allow cloning a response after its been used', function() { url = base + '/hello'; return fetch(url).then(function(res) { From 8a6213198b286cf7dcaf09bb49f183ea490374e4 Mon Sep 17 00:00:00 2001 From: David Frank Date: Thu, 26 May 2016 01:37:36 +0800 Subject: [PATCH 3/7] avoid calling formdata getLengthSync when it has stream as fields --- index.js | 2 +- test/dummy.txt | 1 + test/test.js | 26 +++++++++++++++++++++++++- 3 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 test/dummy.txt diff --git a/index.js b/index.js index cad8394..03e9fe7 100644 --- a/index.js +++ b/index.js @@ -92,7 +92,7 @@ function Fetch(url, opts) { if (typeof options.body === 'string') { headers.set('content-length', Buffer.byteLength(options.body)); // detect form data input from form-data module, this hack avoid the need to add content-length header manually - } else if (options.body && typeof options.body.getLengthSync === 'function') { + } else if (options.body && typeof options.body.getLengthSync === 'function' && options.body._lengthRetrievers.length == 0) { headers.set('content-length', options.body.getLengthSync().toString()); // this is only necessary for older nodejs releases (before iojs merge) } else if (options.body === undefined || options.body === null) { diff --git a/test/dummy.txt b/test/dummy.txt new file mode 100644 index 0000000..5ca5191 --- /dev/null +++ b/test/dummy.txt @@ -0,0 +1 @@ +i am a dummy \ No newline at end of file diff --git a/test/test.js b/test/test.js index 673285b..508abd6 100644 --- a/test/test.js +++ b/test/test.js @@ -11,6 +11,7 @@ var stream = require('stream'); var resumer = require('resumer'); var FormData = require('form-data'); var http = require('http'); +var fs = require('fs'); var TestServer = require('./server'); @@ -607,10 +608,13 @@ describe('node-fetch', function() { }); it('should allow POST request with readable stream as body', function() { + var body = resumer().queue('a=1').end(); + body = body.pipe(new stream.PassThrough()); + url = base + '/inspect'; opts = { method: 'POST' - , body: resumer().queue('a=1').end() + , body: body }; return fetch(url, opts).then(function(res) { return res.json(); @@ -641,6 +645,26 @@ describe('node-fetch', function() { }); }); + it('should allow POST request with form-data using stream as body', function() { + var form = new FormData(); + form.append('my_field', fs.createReadStream('test/dummy.txt')); + + url = base + '/multipart'; + opts = { + method: 'POST' + , body: form + }; + + return fetch(url, opts).then(function(res) { + return res.json(); + }).then(function(res) { + expect(res.method).to.equal('POST'); + expect(res.headers['content-type']).to.contain('multipart/form-data'); + expect(res.headers['content-length']).to.be.undefined; + expect(res.body).to.contain('my_field='); + }); + }); + it('should allow POST request with form-data as body and custom headers', function() { var form = new FormData(); form.append('a','1'); From a2607719ce0ca427462251f812982992a94b1119 Mon Sep 17 00:00:00 2001 From: David Frank Date: Thu, 26 May 2016 02:00:25 +0800 Subject: [PATCH 4/7] send delete body with content-length --- index.js | 4 ++-- test/test.js | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 03e9fe7..9c45cae 100644 --- a/index.js +++ b/index.js @@ -87,8 +87,8 @@ function Fetch(url, opts) { headers.set('content-type', 'multipart/form-data; boundary=' + options.body.getBoundary()); } - // bring node-fetch closer to browser behavior by setting content-length automatically for POST, PUT, PATCH requests when body is empty or string - if (!headers.has('content-length') && options.method.substr(0, 1).toUpperCase() === 'P') { + // bring node-fetch closer to browser behavior by setting content-length automatically + if (!headers.has('content-length') && /post|put|patch|delete/i.test(options.method)) { if (typeof options.body === 'string') { headers.set('content-length', Buffer.byteLength(options.body)); // detect form data input from form-data module, this hack avoid the need to add content-length header manually diff --git a/test/test.js b/test/test.js index 508abd6..4113eb6 100644 --- a/test/test.js +++ b/test/test.js @@ -715,6 +715,38 @@ describe('node-fetch', function() { }); }); + it('should allow POST request with string body', function() { + url = base + '/inspect'; + opts = { + method: 'POST' + , body: 'a=1' + }; + return fetch(url, opts).then(function(res) { + return res.json(); + }).then(function(res) { + expect(res.method).to.equal('POST'); + expect(res.body).to.equal('a=1'); + expect(res.headers['transfer-encoding']).to.be.undefined; + expect(res.headers['content-length']).to.equal('3'); + }); + }); + + it('should allow DELETE request with string body', function() { + url = base + '/inspect'; + opts = { + method: 'DELETE' + , body: 'a=1' + }; + return fetch(url, opts).then(function(res) { + return res.json(); + }).then(function(res) { + expect(res.method).to.equal('DELETE'); + expect(res.body).to.equal('a=1'); + expect(res.headers['transfer-encoding']).to.be.undefined; + expect(res.headers['content-length']).to.equal('3'); + }); + }); + it('should allow PATCH request', function() { url = base + '/inspect'; opts = { From 2a6e656c1dafc0ccbbacf2e9836f312e093c6b7c Mon Sep 17 00:00:00 2001 From: David Frank Date: Thu, 26 May 2016 02:19:16 +0800 Subject: [PATCH 5/7] allow any url for new request, but still reject non-http url in fetch --- index.js | 8 ++++++++ lib/request.js | 8 -------- test/test.js | 6 ++++++ 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/index.js b/index.js index 9c45cae..f633f33 100644 --- a/index.js +++ b/index.js @@ -56,6 +56,14 @@ function Fetch(url, opts) { return; } + if (!options.protocol || !options.hostname) { + throw new Error('only absolute urls are supported'); + } + + if (options.protocol !== 'http:' && options.protocol !== 'https:') { + throw new Error('only http(s) protocols are supported'); + } + var send; if (options.protocol === 'https:') { send = https.request; diff --git a/lib/request.js b/lib/request.js index ed1e8db..9d1e25a 100644 --- a/lib/request.js +++ b/lib/request.js @@ -31,14 +31,6 @@ function Request(input, init) { url_parsed = parse_url(url); } - if (!url_parsed.protocol || !url_parsed.hostname) { - throw new Error('only absolute urls are supported'); - } - - if (url_parsed.protocol !== 'http:' && url_parsed.protocol !== 'https:') { - throw new Error('only http(s) protocols are supported'); - } - // normalize init init = init || {}; diff --git a/test/test.js b/test/test.js index 4113eb6..2251023 100644 --- a/test/test.js +++ b/test/test.js @@ -1307,6 +1307,12 @@ describe('node-fetch', function() { }); });
 + it('should support arbitrary url in Request constructor', function() { + url = 'anything'; + var req = new Request(url); + expect(req.url).to.equal('anything'); + }); + it('should support clone() method in Request constructor', function() { url = base; var body = resumer().queue('a=1').end(); From 30b95dc6b85298a07bdb9f35d4a15c9f2ad37a82 Mon Sep 17 00:00:00 2001 From: David Frank Date: Thu, 26 May 2016 02:20:38 +0800 Subject: [PATCH 6/7] request no longer throw errors --- index.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/index.js b/index.js index f633f33..10fb8ff 100644 --- a/index.js +++ b/index.js @@ -48,13 +48,7 @@ function Fetch(url, opts) { // wrap http.request into fetch return new Fetch.Promise(function(resolve, reject) { // build request object - var options; - try { - options = new Request(url, opts); - } catch (err) { - reject(err); - return; - } + var options = new Request(url, opts); if (!options.protocol || !options.hostname) { throw new Error('only absolute urls are supported'); From 21e1d81ff0066545813fab123cd0215c6115cd8f Mon Sep 17 00:00:00 2001 From: David Frank Date: Thu, 26 May 2016 02:27:25 +0800 Subject: [PATCH 7/7] changelog update --- CHANGELOG.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20ae51e..06be686 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,15 @@ Changelog # 1.x release -## v1.5.2 (master) +## v1.5.3 (master) + +- Fix: handles 204 and 304 responses when body is empty but content-encoding is gzip/deflate +- Fix: allow resolving response and cloned response in any order +- Fix: avoid setting content-length when form-data body use streams +- Fix: send DELETE request with content-length when body is present +- Fix: allow any url when calling new Request, but still reject non-http(s) url in fetch + +## v1.5.2 - Fix: allow node.js core to handle keep-alive connection pool when passing a custom agent