Fix issues with the UrlResolver and add more tests.

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;

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');
 

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');