From 4ab3528152cef83398df063967b990a3fd29c9f9 Mon Sep 17 00:00:00 2001 From: Matteo Figus Date: Thu, 20 Oct 2016 16:48:03 +0100 Subject: [PATCH 1/6] Refactoring --- src/registry/index.js | 72 ++++++++++---------------------- src/registry/middleware/index.js | 46 ++++++++++++++++++++ 2 files changed, 68 insertions(+), 50 deletions(-) create mode 100644 src/registry/middleware/index.js diff --git a/src/registry/index.js b/src/registry/index.js index cf8e1c916..b694432e1 100644 --- a/src/registry/index.js +++ b/src/registry/index.js @@ -4,18 +4,12 @@ var colors = require('colors'); var express = require('express'); var format = require('stringformat'); var http = require('http'); -var path = require('path'); var _ = require('underscore'); var appStart = require('./app-start'); -var baseUrlHandler = require('./middleware/base-url-handler'); -var cors = require('./middleware/cors'); -var discoveryHandler = require('./middleware/discovery-handler'); -var eventsHandler = require('./domain/events-handler'); -var fileUploads = require('./middleware/file-uploads'); +var middleware = require('./middleware'); var pluginsInitialiser = require('./domain/plugins-initialiser'); var Repository = require('./domain/repository'); -var requestHandler = require('./middleware/request-handler'); var Router = require('./router'); var sanitiseOptions = require('./domain/options-sanitiser'); var settings = require('../resources/settings'); @@ -45,41 +39,9 @@ module.exports = function(options){ } }; - this.init = function(callback){ - var app = express(); - + this.init = function(){ + self.app = middleware.bind(express(), options); repository = new Repository(options); - - // middleware - app.set('port', process.env.PORT || options.port); - app.set('json spaces', 0); - - app.use(function(req, res, next){ - res.conf = options; - next(); - }); - - app.use(requestHandler()); - app.use(express.json()); - app.use(express.urlencoded()); - app.use(cors); - app.use(fileUploads); - app.use(baseUrlHandler); - app.use(discoveryHandler); - - app.set('views', path.join(__dirname, 'views')); - app.set('view engine', 'jade'); - app.set('view cache', true); - - if(withLogging){ - app.use(express.logger('dev')); - } - - if(options.local){ - app.use(express.errorHandler({ dumpExceptions: true, showStack: true })); - } - - self.app = app; }; this.register = function(plugin, cb){ @@ -133,33 +95,43 @@ module.exports = function(options){ app.set('etag', 'strong'); pluginsInitialiser.init(plugins, function(err, plugins){ + if(!!err){ return callback(err); } + options.plugins = plugins; repository.init(function(err, componentsInfo){ + + if(!!err){ return callback(err); } + appStart(repository, options, function(err, res){ if(!!err){ return callback(err.msg); } server = http.createServer(self.app); - server.listen(self.app.get('port'), function(){ + server.listen(self.app.get('port'), function(err){ + + if(!!err){ return callback(err); } + eventsHandler.fire('start', {}); + if(withLogging){ + console.log(format('Registry started at port {0}'.green, self.app.get('port'))); + if(_.isObject(componentsInfo)){ - var componentsNumber = _.keys(componentsInfo.components).length; - var componentsReleases = _.reduce(componentsInfo.components, function(memo, component){ - return (parseInt(memo, 10) + component.length); - }); + + var componentsNumber = _.keys(componentsInfo.components).length, + componentsReleases = _.reduce(componentsInfo.components, function(memo, component){ + return (parseInt(memo, 10) + component.length); + }); console.log(format('Registry serving {0} components for a total of {1} releases.', componentsNumber, componentsReleases).green); } } - callback(null, { - app: self.app, - server: server - }); + + callback(null, { app: self.app, server: server }); }); server.on('error', function(e){ diff --git a/src/registry/middleware/index.js b/src/registry/middleware/index.js new file mode 100644 index 000000000..4a769c8c3 --- /dev/null +++ b/src/registry/middleware/index.js @@ -0,0 +1,46 @@ +'use strict'; + +var express = require('express'); +var path = require('path'); +var _ = require('underscore'); + +var baseUrlHandler = require('./base-url-handler'); +var cors = require('./cors'); +var discoveryHandler = require('./discovery-handler'); +var fileUploads = require('./file-uploads'); +var requestHandler = require('./request-handler'); + +module.exports.bind = function(app, options){ + + var withLogging = !_.has(options, 'verbosity') || options.verbosity > 0; + + app.set('port', process.env.PORT || options.port); + app.set('json spaces', 0); + + app.use(function(req, res, next){ + res.conf = options; + next(); + }); + + app.use(requestHandler()); + app.use(express.json()); + app.use(express.urlencoded()); + app.use(cors); + app.use(fileUploads); + app.use(baseUrlHandler); + app.use(discoveryHandler); + + app.set('views', path.join(__dirname, '../views')); + app.set('view engine', 'jade'); + app.set('view cache', true); + + if(withLogging){ + app.use(express.logger('dev')); + } + + if(options.local){ + app.use(express.errorHandler({ dumpExceptions: true, showStack: true })); + } + + return app; +}; \ No newline at end of file From a9d30c47a2c853de22ed9a964c42bf659d51b803 Mon Sep 17 00:00:00 2001 From: Matteo Figus Date: Thu, 20 Oct 2016 17:05:45 +0100 Subject: [PATCH 2/6] More refactoring --- src/registry/index.js | 44 +++----------------------------------- src/registry/router.js | 48 +++++++++++++++++++++++++++++++++++++++--- 2 files changed, 48 insertions(+), 44 deletions(-) diff --git a/src/registry/index.js b/src/registry/index.js index b694432e1..3fbc4540f 100644 --- a/src/registry/index.js +++ b/src/registry/index.js @@ -7,12 +7,12 @@ var http = require('http'); var _ = require('underscore'); var appStart = require('./app-start'); +var eventsHandler = require('./domain/events-handler'); var middleware = require('./middleware'); var pluginsInitialiser = require('./domain/plugins-initialiser'); var Repository = require('./domain/repository'); -var Router = require('./router'); +var router = require('./router'); var sanitiseOptions = require('./domain/options-sanitiser'); -var settings = require('../resources/settings'); var validator = require('./domain/validators'); module.exports = function(options){ @@ -54,45 +54,7 @@ module.exports = function(options){ callback = _.noop; } - var app = this.app; - - // routes - app.use(app.router); - var router = new Router(options, repository); - - if(options.prefix !== '/'){ - app.get('/', function(req, res){ res.redirect(options.prefix); }); - app.get(options.prefix.substr(0, options.prefix.length - 1), router.listComponents); - } - - app.get(options.prefix + 'oc-client/client.js', router.staticRedirector); - app.get(options.prefix + 'oc-client/oc-client.min.map', router.staticRedirector); - - if(options.local){ - app.get(format('{0}:componentName/:componentVersion/{1}*', options.prefix, settings.registry.localStaticRedirectorPath), router.staticRedirector); - } else { - app.put(options.prefix + ':componentName/:componentVersion', options.beforePublish, router.publish); - } - - app.get(options.prefix, router.listComponents); - app.post(options.prefix, router.components); - - app.get(format('{0}:componentName/:componentVersion{1}', options.prefix, settings.registry.componentInfoPath), router.componentInfo); - app.get(format('{0}:componentName{1}', options.prefix, settings.registry.componentInfoPath), router.componentInfo); - - app.get(format('{0}:componentName/:componentVersion{1}', options.prefix, settings.registry.componentPreviewPath), router.componentPreview); - app.get(format('{0}:componentName{1}', options.prefix, settings.registry.componentPreviewPath), router.componentPreview); - - app.get(options.prefix + ':componentName/:componentVersion', router.component); - app.get(options.prefix + ':componentName', router.component); - - if(!!options.routes){ - _.forEach(options.routes, function(route){ - app[route.method.toLowerCase()](route.route, route.handler); - }); - } - - app.set('etag', 'strong'); + router.create(this.app, options, repository); pluginsInitialiser.init(plugins, function(err, plugins){ diff --git a/src/registry/router.js b/src/registry/router.js index 6a8444752..3ad3215a9 100644 --- a/src/registry/router.js +++ b/src/registry/router.js @@ -1,15 +1,19 @@ 'use strict'; +var format = require('stringformat'); +var _ = require('underscore'); + var ComponentRoute = require('./routes/component'); var ComponentsRoute = require('./routes/components'); var ComponentInfoRoute = require('./routes/component-info'); var ComponentPreviewRoute = require('./routes/component-preview'); var ListComponentsRoute = require('./routes/list-components'); var PublishRoute = require('./routes/publish'); +var settings = require('../resources/settings'); var StaticRedirectorRoute = require('./routes/static-redirector'); -module.exports = function(conf, repository){ - return { +module.exports.create = function(app, conf, repository){ + var routes = { component: new ComponentRoute(conf, repository), components: new ComponentsRoute(conf, repository), componentInfo: new ComponentInfoRoute(repository), @@ -18,4 +22,42 @@ module.exports = function(conf, repository){ publish: new PublishRoute(repository), staticRedirector: new StaticRedirectorRoute(repository) }; -}; \ No newline at end of file + + app.use(app.router); + + if(conf.prefix !== '/'){ + app.get('/', function(req, res){ res.redirect(conf.prefix); }); + app.get(conf.prefix.substr(0, conf.prefix.length - 1), routes.listComponents); + } + + app.get(conf.prefix + 'oc-client/client.js', routes.staticRedirector); + app.get(conf.prefix + 'oc-client/oc-client.min.map', routes.staticRedirector); + + if(conf.local){ + app.get(format('{0}:componentName/:componentVersion/{1}*', conf.prefix, settings.registry.localStaticRedirectorPath), routes.staticRedirector); + } else { + app.put(conf.prefix + ':componentName/:componentVersion', conf.beforePublish, routes.publish); + } + + app.get(conf.prefix, routes.listComponents); + app.post(conf.prefix, routes.components); + + app.get(format('{0}:componentName/:componentVersion{1}', conf.prefix, settings.registry.componentInfoPath), routes.componentInfo); + app.get(format('{0}:componentName{1}', conf.prefix, settings.registry.componentInfoPath), routes.componentInfo); + + app.get(format('{0}:componentName/:componentVersion{1}', conf.prefix, settings.registry.componentPreviewPath), routes.componentPreview); + app.get(format('{0}:componentName{1}', conf.prefix, settings.registry.componentPreviewPath), routes.componentPreview); + + app.get(conf.prefix + ':componentName/:componentVersion', routes.component); + app.get(conf.prefix + ':componentName', routes.component); + + if(!!conf.routes){ + _.forEach(conf.routes, function(route){ + app[route.method.toLowerCase()](route.route, route.handler); + }); + } + + app.set('etag', 'strong'); + + return app; +}; From 38a70a7b4906345de828c04343bb53b4f09dd8f0 Mon Sep 17 00:00:00 2001 From: Matteo Figus Date: Thu, 20 Oct 2016 17:09:49 +0100 Subject: [PATCH 3/6] Refactoring --- src/registry/domain/options-sanitiser.js | 4 ++++ src/registry/index.js | 3 +-- src/registry/middleware/index.js | 4 +--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/registry/domain/options-sanitiser.js b/src/registry/domain/options-sanitiser.js index 29e7fb50b..84f36f226 100644 --- a/src/registry/domain/options-sanitiser.js +++ b/src/registry/domain/options-sanitiser.js @@ -38,5 +38,9 @@ module.exports = function(input){ options.hotReloading = !!options.local; } + if(!_.isUndefined(options.verbosity)){ + options.verbosity = 0; + } + return options; }; diff --git a/src/registry/index.js b/src/registry/index.js index 3fbc4540f..6c6f2e5be 100644 --- a/src/registry/index.js +++ b/src/registry/index.js @@ -20,7 +20,6 @@ module.exports = function(options){ var repository, self = this, server, - withLogging = !_.has(options, 'verbosity') || options.verbosity > 0, validationResult = validator.validateRegistryConfiguration(options), plugins = []; @@ -78,7 +77,7 @@ module.exports = function(options){ eventsHandler.fire('start', {}); - if(withLogging){ + if(!!options.verbosity){ console.log(format('Registry started at port {0}'.green, self.app.get('port'))); diff --git a/src/registry/middleware/index.js b/src/registry/middleware/index.js index 4a769c8c3..fc1738d45 100644 --- a/src/registry/middleware/index.js +++ b/src/registry/middleware/index.js @@ -12,8 +12,6 @@ var requestHandler = require('./request-handler'); module.exports.bind = function(app, options){ - var withLogging = !_.has(options, 'verbosity') || options.verbosity > 0; - app.set('port', process.env.PORT || options.port); app.set('json spaces', 0); @@ -34,7 +32,7 @@ module.exports.bind = function(app, options){ app.set('view engine', 'jade'); app.set('view cache', true); - if(withLogging){ + if(!!options.verbosity){ app.use(express.logger('dev')); } From 5ac30fc92dd69b2b2ffee4e241628b218eba0c89 Mon Sep 17 00:00:00 2001 From: Matteo Figus Date: Thu, 20 Oct 2016 18:13:34 +0100 Subject: [PATCH 4/6] Refactoring --- src/registry/domain/options-sanitiser.js | 2 ++ src/registry/index.js | 2 +- src/registry/middleware/index.js | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/registry/domain/options-sanitiser.js b/src/registry/domain/options-sanitiser.js index 84f36f226..f9cefc9ce 100644 --- a/src/registry/domain/options-sanitiser.js +++ b/src/registry/domain/options-sanitiser.js @@ -42,5 +42,7 @@ module.exports = function(input){ options.verbosity = 0; } + options.port = process.env.PORT || options.port; + return options; }; diff --git a/src/registry/index.js b/src/registry/index.js index 6c6f2e5be..31aabfca9 100644 --- a/src/registry/index.js +++ b/src/registry/index.js @@ -71,7 +71,7 @@ module.exports = function(options){ server = http.createServer(self.app); - server.listen(self.app.get('port'), function(err){ + server.listen(options.port, function(err){ if(!!err){ return callback(err); } diff --git a/src/registry/middleware/index.js b/src/registry/middleware/index.js index fc1738d45..37df9ca1e 100644 --- a/src/registry/middleware/index.js +++ b/src/registry/middleware/index.js @@ -12,7 +12,7 @@ var requestHandler = require('./request-handler'); module.exports.bind = function(app, options){ - app.set('port', process.env.PORT || options.port); + app.set('port', options.port); app.set('json spaces', 0); app.use(function(req, res, next){ From 3f77b2d32e0ad471d3b99e0cda5388e3ef2a6e30 Mon Sep 17 00:00:00 2001 From: Matteo Figus Date: Thu, 20 Oct 2016 19:08:21 +0100 Subject: [PATCH 5/6] Added unit tests to registry initialisation --- test/unit/registry.js | 229 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 229 insertions(+) create mode 100644 test/unit/registry.js diff --git a/test/unit/registry.js b/test/unit/registry.js new file mode 100644 index 000000000..f958cd555 --- /dev/null +++ b/test/unit/registry.js @@ -0,0 +1,229 @@ +'use strict'; + +var expect = require('chai').expect; +var injectr = require('injectr'); +var sinon = require('sinon'); + +describe('registry', function(){ + + var repositoryInitStub = sinon.stub(); + + var deps = { + './app-start': sinon.stub(), + './domain/events-handler': {}, + 'express': sinon.stub(), + 'http': { + createServer: sinon.stub() + }, + './middleware': { bind: sinon.stub() }, + './domain/plugins-initialiser': { init: sinon.stub() }, + './domain/repository': sinon.stub().returns({ + init: repositoryInitStub + }), + './router': sinon.stub(), + './domain/options-sanitiser': sinon.stub(), + './domain/validators': { + validateRegistryConfiguration: sinon.stub() + } + }; + + var Registry = injectr('../../src/registry/index.js', deps); + + describe('when instanciated', function(){ + + describe('when options are not valid', function(){ + + var init; + beforeEach(function(){ + deps['./domain/validators'].validateRegistryConfiguration.returns({ isValid: false, message: 'blargh' }); + init = function(){ var registry = new Registry({}); }; + }); + + it('should throw an error', function(){ + expect(init).to.throw('blargh'); + }); + }); + + describe('when options are valid', function(){ + + var registry; + beforeEach(function(){ + deps['./domain/validators'].validateRegistryConfiguration.returns({ isValid: true }); + deps.express.returns('express instance'); + deps['./domain/options-sanitiser'].returns({ port: 3000 }); + registry = new Registry({}); + }); + + it('should instantiate express', function(){ + expect(deps.express.called).to.be.true; + }); + + it('should bind the middleware', function(){ + var bind = deps['./middleware'].bind; + expect(bind.called).to.be.true; + expect(bind.args[0][0]).to.equal('express instance'); + expect(bind.args[0][1]).to.eql({ port: 3000 }); + }); + + it('should instanciate the repository', function(){ + expect(deps['./domain/repository'].called).to.be.true; + }); + + describe('when starting it', function(){ + + describe('when plugins initialiser fails', function(){ + + var error; + beforeEach(function(done){ + deps['./domain/plugins-initialiser'].init.yields('error!'); + registry.start(function(err){ + error = err; + done(); + }); + }); + + it('should fail with error', function(){ + expect(error).to.equal('error!'); + }); + }); + + describe('when plugins initialiser succeeds', function(){ + + describe('when repository initialisation fails', function(){ + + var error; + beforeEach(function(done){ + deps['./domain/plugins-initialiser'].init.yields(null, 'ok'); + repositoryInitStub.yields('nope'); + + registry.start(function(err){ + error = err; + done(); + }); + }); + + it('should fail with error', function(){ + expect(error).to.equal('nope'); + }); + }); + + describe('when repository initialisation succeeds', function(){ + + describe('when app fails to start', function(){ + + var error; + beforeEach(function(done){ + deps['./domain/plugins-initialiser'].init.yields(null, 'ok'); + repositoryInitStub.yields(null, 'ok'); + deps['./app-start'].yields({ msg: 'I got a problem'}); + + registry.start(function(err){ + error = err; + done(); + }); + }); + + it('should fail with error', function(){ + expect(error).to.equal('I got a problem'); + }); + }); + + describe('when app starts', function(){ + + describe('when http listener errors', function(){ + + var error; + beforeEach(function(done){ + deps['./domain/plugins-initialiser'].init.yields(null, 'ok'); + repositoryInitStub.yields(null, 'ok'); + deps['./app-start'].yields(null, 'ok'); + + deps['http'].createServer.returns({ + listen: sinon.stub().yields('Port is already used'), + on: sinon.stub() + }); + + registry.start(function(err){ + error = err; + done(); + }); + }); + + it('should fail with error', function(){ + expect(error).to.equal('Port is already used'); + }); + }); + + describe('when http listener succeeds', function(){ + + var error, result; + beforeEach(function(done){ + deps['./domain/plugins-initialiser'].init.yields(null, 'ok'); + repositoryInitStub.yields(null, 'ok'); + deps['./app-start'].yields(null, 'ok'); + deps['./domain/events-handler'].fire = sinon.stub(); + + deps['http'].createServer.returns({ + listen: sinon.stub().yields(null, 'ok'), + on: sinon.stub() + }); + + registry.start(function(err, res){ + error = err; + result = res; + done(); + }); + }); + + it('should not return error', function(){ + expect(error).to.be.null; + }); + + it('should return the server instance', function(){ + expect(result.app).to.not.be.null; + expect(result.server).to.not.be.null; + }); + + it('should emit a start event', function(){ + expect(deps['./domain/events-handler'].fire.args[0]).to.eql(['start', {}]); + }); + }); + + describe('when http listener emits an error before the listener to start', function(){ + + var error; + beforeEach(function(done){ + deps['./domain/plugins-initialiser'].init.yields(null, 'ok'); + repositoryInitStub.yields(null, 'ok'); + deps['./app-start'].yields(null, 'ok'); + deps['./domain/events-handler'].fire = sinon.stub(); + + deps['http'].createServer.returns({ + listen: sinon.stub(), + on: sinon.stub().yields('I failed for some reason') + }); + + registry.start(function(err, res){ + error = err; + done(); + }); + }); + + it('should return error', function(){ + expect(error).to.be.equal('I failed for some reason'); + }); + + it('should emit an error event', function(){ + expect(deps['./domain/events-handler'].fire.args[0]).to.eql(['error', { + code: 'EXPRESS_ERROR', + message: 'I failed for some reason' + }]); + }); + }); + }); + }); + }); + }); + }); + }); +}); \ No newline at end of file From 005fe49392f7be66ea541a44e3355d22c631f10f Mon Sep 17 00:00:00 2001 From: Matteo Figus Date: Thu, 20 Oct 2016 19:43:50 +0100 Subject: [PATCH 6/6] Refactoring --- src/registry/index.js | 72 ++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/src/registry/index.js b/src/registry/index.js index 31aabfca9..b13798755 100644 --- a/src/registry/index.js +++ b/src/registry/index.js @@ -1,5 +1,6 @@ 'use strict'; +var async = require('async'); var colors = require('colors'); var express = require('express'); var format = require('stringformat'); @@ -55,51 +56,52 @@ module.exports = function(options){ router.create(this.app, options, repository); - pluginsInitialiser.init(plugins, function(err, plugins){ - + async.waterfall([ + function(cb){ + pluginsInitialiser.init(plugins, cb); + }, + function(plugins, cb){ + options.plugins = plugins; + repository.init(cb); + }, + function(componentsInfo, cb){ + appStart(repository, options, function(err){ + cb(!!err ? err.msg : null, componentsInfo); + }); + } + ], + function(err, componentsInfo){ if(!!err){ return callback(err); } - - options.plugins = plugins; - repository.init(function(err, componentsInfo){ + server = http.createServer(self.app); + server.listen(options.port, function(err){ + if(!!err){ return callback(err); } - appStart(repository, options, function(err, res){ - - if(!!err){ return callback(err.msg); } - - server = http.createServer(self.app); + eventsHandler.fire('start', {}); + + if(!!options.verbosity){ - server.listen(options.port, function(err){ - - if(!!err){ return callback(err); } + console.log(format('Registry started at port {0}'.green, self.app.get('port'))); + + if(_.isObject(componentsInfo)){ - eventsHandler.fire('start', {}); - - if(!!options.verbosity){ + var componentsNumber = _.keys(componentsInfo.components).length, + componentsReleases = _.reduce(componentsInfo.components, function(memo, component){ + return (parseInt(memo, 10) + component.length); + }); - console.log(format('Registry started at port {0}'.green, self.app.get('port'))); - - if(_.isObject(componentsInfo)){ + console.log(format('Registry serving {0} components for a total of {1} releases.', componentsNumber, componentsReleases).green); + } + } - var componentsNumber = _.keys(componentsInfo.components).length, - componentsReleases = _.reduce(componentsInfo.components, function(memo, component){ - return (parseInt(memo, 10) + component.length); - }); - - console.log(format('Registry serving {0} components for a total of {1} releases.', componentsNumber, componentsReleases).green); - } - } - - callback(null, { app: self.app, server: server }); - }); + callback(null, { app: self.app, server: server }); + }); - server.on('error', function(e){ - eventsHandler.fire('error', { code: 'EXPRESS_ERROR', message: e }); - callback(e); - }); - }); + server.on('error', function(e){ + eventsHandler.fire('error', { code: 'EXPRESS_ERROR', message: e }); + callback(e); }); }); };