From c22bfa81892b6fcacc8e132c0fc5801d9c52c0d9 Mon Sep 17 00:00:00 2001 From: David Frank Date: Mon, 28 Sep 2015 17:40:58 +0800 Subject: [PATCH 1/8] auto add content-length on for string body --- index.js | 5 +++++ test/test.js | 54 ++++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/index.js b/index.js index cdc2444..8d94eaf 100644 --- a/index.js +++ b/index.js @@ -82,6 +82,11 @@ function Fetch(url, opts) { headers.set('content-type', 'multipart/form-data; boundary=' + options.body.getBoundary()); } + // bring node-fetch closer to browser fetch behavior by setting content-length automatically for post, put, patch requests when body is string + if (options.method.substr(0, 1).toUpperCase() === 'P' && typeof options.body === 'string') { + headers.set('content-length', Buffer.byteLength(options.body)); + } + options.headers = headers.raw(); // http.request only support string as host header, this hack make custom host header possible diff --git a/test/test.js b/test/test.js index fb4bb08..f3abfb8 100644 --- a/test/test.js +++ b/test/test.js @@ -416,6 +416,8 @@ describe('node-fetch', function() { return res.json(); }).then(function(res) { expect(res.method).to.equal('POST'); + expect(res.headers['transfer-encoding']).to.be.undefined; + expect(res.headers['content-length']).to.equal('0'); }); }); @@ -430,6 +432,8 @@ describe('node-fetch', function() { }).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'); }); }); @@ -444,6 +448,8 @@ describe('node-fetch', function() { }).then(function(res) { expect(res.method).to.equal('POST'); expect(res.body).to.equal('a=1'); + expect(res.headers['transfer-encoding']).to.equal('chunked'); + expect(res.headers['content-length']).to.be.undefined; }); }); @@ -541,6 +547,20 @@ describe('node-fetch', function() { }); }); + it('should set request content-length when sending string as 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'); + + }); + }); + it('should reject decoding body twice', function() { url = base + '/plain'; return fetch(url).then(function(res) { @@ -796,6 +816,26 @@ describe('node-fetch', function() { }); }); + it('should support parsing headers in Response constructor', function() { + url = base; + var res = new Response(url, { + headers: { + a: '1' + } + }); + expect(res.headers.get('a')).to.equal('1'); + }); + + it('should support parsing headers in Request constructor', function() { + url = base; + var req = new Request(url, { + headers: { + a: '1' + } + }); + expect(req.headers.get('a')).to.equal('1'); + }); + it('should support https request', function() { this.timeout(5000); url = 'https://github.com/'; @@ -808,18 +848,4 @@ describe('node-fetch', function() { }); }); - it('should support parsing headers in Response constructor', function(){ - - var r = new Response(null, {headers: {'foo': 'bar'}}); - expect(r.headers.get('foo')).to.equal('bar'); - - }); - - it('should support parsing headers in Request constructor', function(){ - - var r = new Request('http://foo', {headers: {'foo': 'bar'}}); - expect(r.headers.get('foo')).to.equal('bar'); - - }); - }); From cfe98cdd35d4cb24e852503446bbfe6f16250377 Mon Sep 17 00:00:00 2001 From: David Frank Date: Mon, 28 Sep 2015 19:27:57 +0800 Subject: [PATCH 2/8] make sure we don't overwrite existing content-length --- index.js | 4 ++-- test/test.js | 14 -------------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/index.js b/index.js index 8d94eaf..b336c47 100644 --- a/index.js +++ b/index.js @@ -82,8 +82,8 @@ function Fetch(url, opts) { headers.set('content-type', 'multipart/form-data; boundary=' + options.body.getBoundary()); } - // bring node-fetch closer to browser fetch behavior by setting content-length automatically for post, put, patch requests when body is string - if (options.method.substr(0, 1).toUpperCase() === 'P' && typeof options.body === 'string') { + // bring node-fetch closer to browser behavior by setting content-length automatically for POST, PUT, PATCH requests when body is string + if (!headers.has('content-length') && typeof options.body === 'string' && options.method.substr(0, 1).toUpperCase() === 'P') { headers.set('content-length', Buffer.byteLength(options.body)); } diff --git a/test/test.js b/test/test.js index f3abfb8..e729f0b 100644 --- a/test/test.js +++ b/test/test.js @@ -547,20 +547,6 @@ describe('node-fetch', function() { }); }); - it('should set request content-length when sending string as 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'); - - }); - }); - it('should reject decoding body twice', function() { url = base + '/plain'; return fetch(url).then(function(res) { From a3e14b503a2ed9c91e7af1cc0c9cebe08061012d Mon Sep 17 00:00:00 2001 From: David Frank Date: Mon, 28 Sep 2015 20:46:39 +0800 Subject: [PATCH 3/8] make sure we support latest node and legacy node --- .travis.yml | 1 + index.js | 11 ++++++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 5a5a081..692c2d6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,5 +3,6 @@ node_js: - "0.10" - "0.12" - "iojs" + - "node" before_install: npm install -g npm script: npm run coverage \ No newline at end of file diff --git a/index.js b/index.js index b336c47..92a35e2 100644 --- a/index.js +++ b/index.js @@ -82,9 +82,14 @@ 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 string - if (!headers.has('content-length') && typeof options.body === 'string' && options.method.substr(0, 1).toUpperCase() === 'P') { - headers.set('content-length', Buffer.byteLength(options.body)); + // 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') { + if (typeof options.body === 'string') { + headers.set('content-length', Buffer.byteLength(options.body)); + // this is only necessary for older nodejs releases (before iojs merge) + } else if (options.body === undefined || options.body === null) { + headers.set('content-length', '0'); + } } options.headers = headers.raw(); From 8f02a2b77ceff713c227b8ff382ba778cbe4f361 Mon Sep 17 00:00:00 2001 From: David Frank Date: Mon, 28 Sep 2015 21:58:45 +0800 Subject: [PATCH 4/8] handle body stream error --- lib/response.js | 5 +++++ test/server.js | 7 +++++++ test/test.js | 8 ++++++++ 3 files changed, 20 insertions(+) diff --git a/lib/response.js b/lib/response.js index d261b09..4512fd2 100644 --- a/lib/response.js +++ b/lib/response.js @@ -87,6 +87,11 @@ Response.prototype._decode = function() { }, self.timeout); } + // handle stream error, such as incorrect content-encoding + self.body.on('error', function(err) { + reject(new Error('invalid response body at: ' + self.url + ' reason: ' + err.message)); + }); + self.body.on('data', function(chunk) { if (self._abort || chunk === null) { return; diff --git a/test/server.js b/test/server.js index e18a665..7403922 100644 --- a/test/server.js +++ b/test/server.js @@ -80,6 +80,13 @@ TestServer.prototype.router = function(req, res) { res.end('fake sdch string'); } + if (p === '/invalid-content-encoding') { + res.statusCode = 200; + res.setHeader('Content-Type', 'text/plain'); + res.setHeader('Content-Encoding', 'gzip'); + res.end('fake gzip string'); + } + if (p === '/timeout') { setTimeout(function() { res.statusCode = 200; diff --git a/test/test.js b/test/test.js index e729f0b..8140ec1 100644 --- a/test/test.js +++ b/test/test.js @@ -348,6 +348,14 @@ describe('node-fetch', function() { }); }); + it('should reject if response compression is invalid', function() { + url = base + '/invalid-content-encoding'; + return fetch(url).then(function(res) { + expect(res.headers.get('content-type')).to.equal('text/plain'); + return expect(res.text()).to.eventually.be.rejectedWith(Error); + }); + }); + it('should allow disabling auto decompression', function() { url = base + '/gzip'; opts = { From b48757013a121a52a22288ef5e471914b0b38dff Mon Sep 17 00:00:00 2001 From: David Frank Date: Mon, 28 Sep 2015 22:30:41 +0800 Subject: [PATCH 5/8] per spec, follow redirect with GET request --- index.js | 9 +++++++++ test/test.js | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 92a35e2..ca2b452 100644 --- a/index.js +++ b/index.js @@ -132,6 +132,15 @@ function Fetch(url, opts) { return; } + // per fetch spec, for POST request with 301/302 response, or any request with 303 response, use GET when following redirect + if (res.statusCode === 303 + || ((res.statusCode === 301 || res.statusCode === 302) && options.method === 'POST')) + { + options.method = 'GET'; + delete options.body; + delete options.headers['content-length']; + } + options.counter++; resolve(Fetch(resolve_url(options.url, res.headers.location), options)); diff --git a/test/test.js b/test/test.js index 8140ec1..4a58661 100644 --- a/test/test.js +++ b/test/test.js @@ -225,6 +225,54 @@ describe('node-fetch', function() { }); }); + it('should follow POST request redirect code 301 with GET', function() { + url = base + '/redirect/301'; + opts = { + method: 'POST' + , body: 'a=1' + }; + return fetch(url, opts).then(function(res) { + expect(res.url).to.equal(base + '/inspect'); + expect(res.status).to.equal(200); + return res.json().then(function(result) { + expect(result.method).to.equal('GET'); + expect(result.body).to.equal(''); + }); + }); + }); + + it('should follow POST request redirect code 302 with GET', function() { + url = base + '/redirect/302'; + opts = { + method: 'POST' + , body: 'a=1' + }; + return fetch(url, opts).then(function(res) { + expect(res.url).to.equal(base + '/inspect'); + expect(res.status).to.equal(200); + return res.json().then(function(result) { + expect(result.method).to.equal('GET'); + expect(result.body).to.equal(''); + }); + }); + }); + + it('should follow redirect code 303 with GET', function() { + url = base + '/redirect/303'; + opts = { + method: 'PUT' + , body: 'a=1' + }; + return fetch(url, opts).then(function(res) { + expect(res.url).to.equal(base + '/inspect'); + expect(res.status).to.equal(200); + return res.json().then(function(result) { + expect(result.method).to.equal('GET'); + expect(result.body).to.equal(''); + }); + }); + }); + it('should obey maximum redirect', function() { url = base + '/redirect/chain'; opts = { @@ -830,7 +878,7 @@ describe('node-fetch', function() { expect(req.headers.get('a')).to.equal('1'); }); - it('should support https request', function() { + it.skip('should support https request', function() { this.timeout(5000); url = 'https://github.com/'; opts = { From 6072402eb6134beccec30420eb2fef966c293a2c Mon Sep 17 00:00:00 2001 From: David Frank Date: Mon, 28 Sep 2015 22:39:53 +0800 Subject: [PATCH 6/8] test case for Response ctor --- test/test.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/test/test.js b/test/test.js index 4a58661..6d880a5 100644 --- a/test/test.js +++ b/test/test.js @@ -858,14 +858,25 @@ describe('node-fetch', function() { }); }); + it('should support empty options in Response constructor', function() { + var body = resumer().queue('a=1').end(); + var res = new Response(body); + return res.text().then(function(result) { + expect(result).to.equal('a=1'); + }); + }); + it('should support parsing headers in Response constructor', function() { - url = base; - var res = new Response(url, { + var body = resumer().queue('a=1').end(); + var res = new Response(body, { headers: { a: '1' } }); expect(res.headers.get('a')).to.equal('1'); + return res.text().then(function(result) { + expect(result).to.equal('a=1'); + }); }); it('should support parsing headers in Request constructor', function() { @@ -875,10 +886,11 @@ describe('node-fetch', function() { a: '1' } }); + expect(req.url).to.equal(url); expect(req.headers.get('a')).to.equal('1'); }); - it.skip('should support https request', function() { + it('should support https request', function() { this.timeout(5000); url = 'https://github.com/'; opts = { From a7bb74b040ce09fafc038d9cb19a093ffa66c983 Mon Sep 17 00:00:00 2001 From: David Frank Date: Mon, 28 Sep 2015 22:48:11 +0800 Subject: [PATCH 7/8] update changelog --- CHANGELOG.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index baea6ab..337bd7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,14 @@ Changelog # 1.x release -## v1.3.2 (master) +## v1.3.3 (master) + +- Fix: make sure `Content-Length` header is set when body is string for POST/PUT/PATCH requests +- Fix: handle body stream error, for cases such as incorrect `Content-Encoding` header +- Fix: when following certain redirects, use `GET` on subsequent request per Fetch Spec +- Fix: `Request` and `Response` constructors now parse headers input using `Headers` + +## v1.3.2 - Enhance: allow auto detect of form-data input (no `FormData` spec on node.js, this is form-data specific feature) From cf128ddfb87001fbf46916cfd4b50a0b7488d4ab Mon Sep 17 00:00:00 2001 From: David Frank Date: Mon, 28 Sep 2015 23:04:14 +0800 Subject: [PATCH 8/8] make sure we test with transform stream --- test/test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/test.js b/test/test.js index 6d880a5..14eaa9b 100644 --- a/test/test.js +++ b/test/test.js @@ -860,6 +860,7 @@ describe('node-fetch', function() { it('should support empty options in Response constructor', function() { var body = resumer().queue('a=1').end(); + body = body.pipe(new stream.PassThrough()); var res = new Response(body); return res.text().then(function(result) { expect(result).to.equal('a=1'); @@ -868,6 +869,7 @@ describe('node-fetch', function() { it('should support parsing headers in Response constructor', function() { var body = resumer().queue('a=1').end(); + body = body.pipe(new stream.PassThrough()); var res = new Response(body, { headers: { a: '1'