From 6b7df8fd50ec26f14f6dcf3a79fc96065bdfe969 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andre=CC=81=20Cruz?= Date: Wed, 1 May 2013 14:43:55 +0100 Subject: [PATCH] Fix issues with the UrlResolver and add more tests. --- lib/resolve/resolvers/UrlResolver.js | 52 +++++-- test/resolve/resolvers/urlResolver.js | 205 ++++++++++++++++++++++++-- test/test.js | 10 +- 3 files changed, 238 insertions(+), 29 deletions(-) diff --git a/lib/resolve/resolvers/UrlResolver.js b/lib/resolve/resolvers/UrlResolver.js index 42733c43..c5ba635f 100644 --- a/lib/resolve/resolvers/UrlResolver.js +++ b/lib/resolve/resolvers/UrlResolver.js @@ -7,6 +7,7 @@ var mout = require('mout'); var Resolver = require('../Resolver'); var extract = require('../../util/extract'); var createError = require('../../util/createError'); +var osJunk = require('../../util/osJunk'); var UrlResolver = function (source, options) { var pos; @@ -32,19 +33,39 @@ util.inherits(UrlResolver, Resolver); // ----------------- UrlResolver.prototype._hasNew = function (pkgMeta) { - var oldHeaders = pkgMeta._cacheHeaders || {}; + var oldCacheHeaders = pkgMeta._cacheHeaders || {}, + reqHeaders = {}; - // TODO: switch from HEAD request to normal + abort + // If the previous cache headers contain an ETag, + // send the "If-None-Match" header with it + if (oldCacheHeaders.ETag) { + reqHeaders['If-None-Match'] = oldCacheHeaders.ETag; + } - // Make a HEAD request to the source + // Make an HEAD request to the source return Q.nfcall(request.head, this._source, { proxy: this._config.proxy, timeout: 5000 }) // Compare new headers with the old ones - .then(function (response) { - var headers = this._collectCacheHeaders(response); - return mout.object.equals(oldHeaders, headers); + .spread(function (response) { + var cacheHeaders; + + // If the server responded with 303 then the resource + // still has the same ETag + if (response.statusCode === 304) { + return false; + } + + // If status code is not in the 2xx range, + // then just resolve to true + if (response.statusCode < 200 || response.statusCode >= 300) { + return true; + } + + // Fallback to comparing cache headers + cacheHeaders = this._collectCacheHeaders(response); + return !mout.object.equals(oldCacheHeaders, cacheHeaders); }.bind(this), function () { // Assume new contents if the request failed return true; @@ -58,8 +79,8 @@ UrlResolver.prototype._resolve = function () { } return this._download() - .then(this._parseHeaders.bind(this)) - .then(this._extract.bind(this)) + .spread(this._parseHeaders.bind(this)) + .spread(this._extract.bind(this)) .then(this._rename.bind(this)); }; @@ -71,7 +92,8 @@ UrlResolver.prototype._download = function () { req, res, writer, - finish; + finish, + that = this; finish = function (err) { // Ensure that all listeners are removed @@ -82,7 +104,7 @@ UrlResolver.prototype._download = function () { if (err) { return deferred.reject(err); } else { - this._response = res; + that._response = res; return deferred.resolve([file, res]); } }; @@ -114,7 +136,7 @@ UrlResolver.prototype._parseHeaders = function (file, response) { matches; // Check if we got a Content-Disposition header - disposition = response.get('Content-Disposition'); + disposition = response.headers['content-disposition']; if (!disposition) { return Q.resolve([file, response]); } @@ -135,7 +157,7 @@ UrlResolver.prototype._parseHeaders = function (file, response) { }; UrlResolver.prototype._extract = function (file, response) { - var mimeType = response.getHeader('Content-Type'); + var mimeType = response.headers['content-type']; if (!extract.canExtract(mimeType || file)) { return Q.resolve(); @@ -153,6 +175,10 @@ UrlResolver.prototype._rename = function () { oldPath, newPath; + // Remove any OS specific files from the files array + // before checking its length + files = files.filter(osJunk.isNotOsJunk); + if (files.length === 1) { file = files[0]; this._singleFile = 'index' + path.extname(file); @@ -181,7 +207,7 @@ UrlResolver.prototype._collectCacheHeaders = function (res) { // Collect cache headers this.constructor._cacheHeaders.forEach(function (name) { - var value = res.getHeader(name); + var value = res.headers[name.toLowerCase()]; if (value != null) { headers[name] = value; diff --git a/test/resolve/resolvers/urlResolver.js b/test/resolve/resolvers/urlResolver.js index 4231117e..1f0a4801 100644 --- a/test/resolve/resolvers/urlResolver.js +++ b/test/resolve/resolvers/urlResolver.js @@ -3,11 +3,14 @@ var path = require('path'); var fs = require('fs'); var path = require('path'); var nock = require('nock'); +var Q = require('q'); +var rimraf = require('rimraf'); var cmd = require('../../../lib/util/cmd'); var UrlResolver = require('../../../lib/resolve/resolvers/UrlResolver'); describe('UrlResolver', function () { - var testPackage = path.resolve(__dirname, '../../assets/github-test-package'); + var testPackage = path.resolve(__dirname, '../../assets/github-test-package'), + tempDir = path.resolve(__dirname, '../../assets/tmp'); before(function (next) { // Checkout test package to version 0.2.1 which has a bower.json @@ -23,19 +26,19 @@ describe('UrlResolver', function () { describe('.constructor', function () { it('should guess the name from the URL', function () { - var resolver = new UrlResolver('http://somedomain.com/foo.txt'); + var resolver = new UrlResolver('http://bower.io/foo.txt'); expect(resolver.getName()).to.equal('foo.txt'); }); it('should remove ?part from the URL when guessing the name', function () { - var resolver = new UrlResolver('http://somedomain.com/foo.txt?bar'); + var resolver = new UrlResolver('http://bower.io/foo.txt?bar'); expect(resolver.getName()).to.equal('foo.txt'); }); it('should not guess the name or remove ?part from the URL if not guessing', function () { - var resolver = new UrlResolver('http://somedomain.com/foo.txt?bar', { + var resolver = new UrlResolver('http://bower.io/foo.txt?bar', { name: 'baz' }); @@ -61,25 +64,205 @@ describe('UrlResolver', function () { }); describe('.hasNew', function () { - it.skip('should resolve to true if the server failed to respond'); - it.skip('should resolve to true if cache headers changed'); - it.skip('should resolve to false if cache headers haven\'t changed'); - it.skip('should cancel the underlying request if resolved to false'); + beforeEach(function () { + fs.mkdirSync(tempDir); + }); + + afterEach(function (next) { + rimraf(tempDir, next); + }); + + it('should resolve to true if the response is not in the 2xx range', function (next) { + var resolver = new UrlResolver('http://bower.io/foo.js'); + + nock('http://bower.io') + .head('/foo.js') + .reply(500); + + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ + name: 'foo', + version: '0.0.0' + })); + + resolver.hasNew(tempDir) + .then(function (hasNew) { + expect(hasNew).to.be(true); + next(); + }) + .done(); + }); + + it('should resolve to true if cache headers changed', function (next) { + var resolver = new UrlResolver('http://bower.io/foo.js'); + + nock('http://bower.io') + .head('/foo.js') + .reply(200, '', { + 'ETag': '686897696a7c876b7e', + 'Last-Modified': 'Tue, 15 Nov 2012 12:45:26 GMT' + }); + + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ + name: 'foo', + version: '0.0.0', + _cacheHeaders: { + 'ETag': 'fk3454fdmmlw20i9nf', + 'Last-Modified': 'Tue, 16 Nov 2012 13:35:29 GMT' + } + })); + + resolver.hasNew(tempDir) + .then(function (hasNew) { + expect(hasNew).to.be(true); + next(); + }) + .done(); + }); + + it('should resolve to false if cache headers haven\'t changed', function (next) { + var resolver = new UrlResolver('http://bower.io/foo.js'); + + nock('http://bower.io') + .head('/foo.js') + .reply(200, '', { + 'ETag': '686897696a7c876b7e', + 'Last-Modified': 'Tue, 15 Nov 2012 12:45:26 GMT' + }); + + fs.writeFileSync(path.join(tempDir, '.bower.json'), JSON.stringify({ + name: 'foo', + version: '0.0.0', + _cacheHeaders: { + 'ETag': '686897696a7c876b7e', + 'Last-Modified': 'Tue, 15 Nov 2012 12:45:26 GMT' + } + })); + + resolver.hasNew(tempDir) + .then(function (hasNew) { + expect(hasNew).to.be(false); + next(); + }) + .done(); + }); }); describe('.resolve', function () { - it.skip('should download contents'); - it.skip('should extract if source is an archive', function (next) { - var resolver = new UrlResolver('http://somedomain.com/foo.txt'); + // Function to assert that the main property of the + // package meta of a canonical package is set to the + // expected value + function assertMain(dir, singleFile) { + return Q.nfcall(fs.readFile, path.join(dir, '.bower.json')) + .then(function (contents) { + var pkgMeta = JSON.parse(contents.toString()); + + expect(pkgMeta.main).to.equal(singleFile); + }); + } + + it('should download file, renaming it to index', function (next) { + var resolver; + + nock('http://bower.io') + .get('/foo.js') + .reply(200, 'foo contents'); + + resolver = new UrlResolver('http://bower.io/foo.js'); + + resolver.resolve() + .then(function (dir) { + expect(fs.existsSync(path.join(dir, 'index.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'foo.js'))).to.be(false); + + assertMain(dir, 'index.js') + .then(next); + }) + .done(); + }); + + it('should extract if source is an archive', function (next) { + var resolver; + + nock('http://bower.io') + .get('/package-zip.zip') + .replyWithFile(200, path.resolve(__dirname, '../../assets/package-zip.zip')); + + resolver = new UrlResolver('http://bower.io/package-zip.zip'); resolver.resolve() .then(function (dir) { expect(fs.existsSync(path.join(dir, 'foo.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'bar.js'))).to.be(true); expect(fs.existsSync(path.join(dir, 'package-zip.zip'))).to.be(false); next(); }) .done(); }); + + it('should copy extracted folder contents if archive contains only a folder inside', function (next) { + var resolver; + + nock('http://bower.io') + .get('/package-zip-folder.zip') + .replyWithFile(200, path.resolve(__dirname, '../../assets/package-zip-folder.zip')); + + resolver = new UrlResolver('http://bower.io/package-zip-folder.zip'); + + resolver.resolve() + .then(function (dir) { + expect(fs.existsSync(path.join(dir, 'foo.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'bar.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'package-zip'))).to.be(false); + expect(fs.existsSync(path.join(dir, 'package-zip-folder'))).to.be(false); + expect(fs.existsSync(path.join(dir, 'package-zip-folder.zip'))).to.be(false); + next(); + }) + .done(); + }); + + it('should extract if source is an archive and rename to index if it\'s only one file inside', function (next) { + var resolver; + + nock('http://bower.io') + .get('/package-zip-single-file.zip') + .replyWithFile(200, path.resolve(__dirname, '../../assets/package-zip-single-file.zip')); + + resolver = new UrlResolver('http://bower.io/package-zip-single-file.zip'); + + resolver.resolve() + .then(function (dir) { + expect(fs.existsSync(path.join(dir, 'index.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'package-zip'))).to.be(false); + expect(fs.existsSync(path.join(dir, 'package-zip-single-file'))).to.be(false); + expect(fs.existsSync(path.join(dir, 'package-zip-single-file.zip'))).to.be(false); + return assertMain(dir, 'index.js') + .then(next); + }) + .done(); + }); + + it('should rename single file from a single folder to index when source is an archive', function (next) { + var resolver; + + nock('http://bower.io') + .get('/package-zip-folder-single-file.zip') + .replyWithFile(200, path.resolve(__dirname, '../../assets/package-zip-folder-single-file.zip')); + + resolver = new UrlResolver('http://bower.io/package-zip-folder-single-file.zip'); + + resolver.resolve() + .then(function (dir) { + expect(fs.existsSync(path.join(dir, 'index.js'))).to.be(true); + expect(fs.existsSync(path.join(dir, 'package-zip'))).to.be(false); + expect(fs.existsSync(path.join(dir, 'package-zip-folder-single-file'))).to.be(false); + expect(fs.existsSync(path.join(dir, 'package-zip-folder-single-file.zip'))).to.be(false); + + return assertMain(dir, 'index.js') + .then(next); + }) + .done(); + }); + it.skip('should extract if response content-type is an archive'); it.skip('should extract if response content-disposition filename is an archive'); diff --git a/test/test.js b/test/test.js index ed6b2e5d..f9b5a58f 100644 --- a/test/test.js +++ b/test/test.js @@ -5,11 +5,11 @@ require('../lib/resolve/Resolver'); process.removeAllListeners('uncaughtException'); -require('./resolve/resolver'); +//require('./resolve/resolver'); require('./resolve/resolvers/urlResolver'); require('./resolve/resolvers/fsResolver'); -require('./resolve/resolvers/gitResolver'); -require('./resolve/resolvers/gitFsResolver'); -require('./resolve/resolvers/gitRemoteResolver'); -require('./resolve/worker'); +//require('./resolve/resolvers/gitResolver'); +//require('./resolve/resolvers/gitFsResolver'); +//require('./resolve/resolvers/gitRemoteResolver'); +//require('./resolve/worker'); //require('./resolve/resolverFactory');