From 6133f4920b02fdad2d1fbe7d4508ac4b3e1ffc07 Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Wed, 10 Dec 2014 18:45:02 +0100 Subject: [PATCH 1/4] Cleaned up and changed promises used in the feature API --- lib/error/NameExistsError.js | 13 ++++++ lib/error/NotFoundError.js | 13 ++++++ lib/error/ValidationError.js | 13 ++++++ lib/error/requestValidator.js | 14 ++++++ lib/featureApi.js | 84 +++++++++++++++++------------------ lib/featureDb.js | 19 +++++++- test/featureDbMock.js | 3 +- 7 files changed, 113 insertions(+), 46 deletions(-) create mode 100644 lib/error/NameExistsError.js create mode 100644 lib/error/NotFoundError.js create mode 100644 lib/error/ValidationError.js create mode 100644 lib/error/requestValidator.js diff --git a/lib/error/NameExistsError.js b/lib/error/NameExistsError.js new file mode 100644 index 0000000000..ecee9feedf --- /dev/null +++ b/lib/error/NameExistsError.js @@ -0,0 +1,13 @@ +var util = require('util'); + +function NameExistsError(message) { + Error.call(this); + Error.captureStackTrace(this, this.constructor); + + this.name = this.constructor.name; + this.message = message; +} + +util.inherits(NameExistsError, Error); + +module.exports = NameExistsError; \ No newline at end of file diff --git a/lib/error/NotFoundError.js b/lib/error/NotFoundError.js new file mode 100644 index 0000000000..235cb85313 --- /dev/null +++ b/lib/error/NotFoundError.js @@ -0,0 +1,13 @@ +var util = require('util'); + +function NotFoundError(message) { + Error.call(this); + Error.captureStackTrace(this, this.constructor); + + this.name = this.constructor.name; + this.message = message; +} + +util.inherits(NotFoundError, Error); + +module.exports = NotFoundError; \ No newline at end of file diff --git a/lib/error/ValidationError.js b/lib/error/ValidationError.js new file mode 100644 index 0000000000..a1f624de49 --- /dev/null +++ b/lib/error/ValidationError.js @@ -0,0 +1,13 @@ +var util = require('util'); + +function ValidationError(message) { + Error.call(this); + Error.captureStackTrace(this, this.constructor); + + this.name = this.constructor.name; + this.message = message; +} + +util.inherits(ValidationError, Error); + +module.exports = ValidationError; \ No newline at end of file diff --git a/lib/error/requestValidator.js b/lib/error/requestValidator.js new file mode 100644 index 0000000000..58819b0730 --- /dev/null +++ b/lib/error/requestValidator.js @@ -0,0 +1,14 @@ +var Promise = require("bluebird"); +var ValidationError = require('./ValidationError'); + +function validateRequest(req) { + return new Promise(function(resolve, reject) { + if (req.validationErrors()) { + reject(new ValidationError("Invalid syntax")); + } else { + resolve(req); + } + }); +} + +module.exports = validateRequest; \ No newline at end of file diff --git a/lib/featureApi.js b/lib/featureApi.js index a9a7a10313..b34b6e9079 100644 --- a/lib/featureApi.js +++ b/lib/featureApi.js @@ -1,6 +1,11 @@ +var logger = require('./logger'); var eventStore = require('./eventStore'); var eventType = require('./eventType'); var featureDb = require('./featureDb'); +var NameExistsError = require('./error/NameExistsError'); +var NotFoundError = require('./error/NotFoundError'); +var ValidationError = require('./error/ValidationError'); +var requestValidator = require('./error/requestValidator'); module.exports = function (app) { @@ -22,62 +27,55 @@ module.exports = function (app) { req.checkBody('name', 'Name is required').notEmpty(); req.checkBody('name', 'Name must match format ^[a-zA-Z\\.\\-]+$').matches(/^[a-zA-Z\\.\\-]+$/i); - var errors = req.validationErrors(); - - if (errors) { - res.status(400).json(errors); - return; - } - - var newFeature = req.body; - - var handleFeatureExist = function() { - var errors = [ - {msg: "A feature named '" + newFeature.name + "' already exists."} - ]; - - res.status(403).json(errors).end(); - }; - - var handleCreateFeature = function () { - eventStore.create({ - type: eventType.featureCreated, - createdBy: req.connection.remoteAddress, - data: newFeature + requestValidator(req) + .then(featureDb.validateUniqueName) + .then(function() { + return eventStore.create({ + type: eventType.featureCreated, + createdBy: req.connection.remoteAddress, + data: req.body + }); }) - .then(function () { res.status(201).end(); }) - .catch(function () { res.status(500).end(); }); - }; - - featureDb.getFeature(newFeature.name) - .then(handleFeatureExist) - .catch(handleCreateFeature); - }); + .then(function () { + res.status(201).end(); + }) + .catch(NameExistsError, function() { + res.status(403).json([{msg: "A feature named '" + req.body.name + "' already exists."}]).end(); + }) + .catch(ValidationError, function() { + res.status(400).json(req.validationErrors()); + }) + .catch(function(err) { + logger.error("Could not create feature toggle", err); + res.status(500).end(); + }); + }); app.put('/features/:featureName', function (req, res) { var featureName = req.params.featureName; - var createdBy = req.connection.remoteAddress; + var userName = req.connection.remoteAddress; var updatedFeature = req.body; updatedFeature.name = featureName; - var event = { - type: eventType.featureUpdated, - createdBy: createdBy, - data: updatedFeature - }; - featureDb.getFeature(featureName) .then(function () { - eventStore - .create(event) - .then(function () { res.status(200).end(); }) - .catch(function () { res.status(500).end(); }); + return eventStore.create({ + type: eventType.featureUpdated, + createdBy: userName, + data: updatedFeature + }); }) - .catch(function () { + .then(function () { + res.status(200).end(); + }) + .catch(NotFoundError, function () { res.status(404).end(); + }) + .catch(function (err) { + logger.error("Could not update feature toggle="+featureName, err); + res.status(500).end(); }); }); - }; diff --git a/lib/featureDb.js b/lib/featureDb.js index bca0732023..3b94289b3a 100644 --- a/lib/featureDb.js +++ b/lib/featureDb.js @@ -2,6 +2,9 @@ var eventStore = require('./eventStore'); var eventType = require('./eventType'); var logger = require('./logger'); var knex = require('./dbPool'); +var Promise = require("bluebird"); +var NotFoundError = require('./error/NotFoundError'); +var NameExistsError = require('./error/NameExistsError'); var FEATURE_COLUMNS = ['name', 'description', 'enabled', 'strategy_name', 'parameters']; eventStore.on(eventType.featureCreated, function (event) { @@ -39,7 +42,7 @@ function getFeature(name) { function rowToFeature(row) { if (!row) { - throw new Error('invalid row'); + throw new NotFoundError('No feature toggle found'); } return { @@ -61,8 +64,20 @@ function eventToRow(event) { }; } +function validateUniqueName(req) { + return new Promise(function(resolve, reject) { + getFeature(req.body.name) + .then(function() { + reject(new NameExistsError("Feature name already exist")); + }, function() { + resolve(req); + }); + }); +} + module.exports = { getFeatures: getFeatures, - getFeature: getFeature + getFeature: getFeature, + validateUniqueName: validateUniqueName }; diff --git a/test/featureDbMock.js b/test/featureDbMock.js index 9ecdf12d58..fcf9082ca4 100644 --- a/test/featureDbMock.js +++ b/test/featureDbMock.js @@ -1,4 +1,5 @@ var Promise = require("bluebird"); +var NotFoundError = require('../lib/error/NotFoundError'); var features = [ { @@ -49,7 +50,7 @@ module.exports = { if(feature) { return Promise.resolve(feature); } else { - return Promise.reject("feature not found"); + return Promise.reject(new NotFoundError("feature not found")); } } }; \ No newline at end of file From 65225ffba074a64cc93feece46dc2275065939fd Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Wed, 10 Dec 2014 18:49:08 +0100 Subject: [PATCH 2/4] Validation logic should probably not be in the db-code --- lib/featureApi.js | 15 ++++++++++++++- lib/featureDb.js | 16 +--------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/lib/featureApi.js b/lib/featureApi.js index b34b6e9079..e123485816 100644 --- a/lib/featureApi.js +++ b/lib/featureApi.js @@ -5,6 +5,8 @@ var featureDb = require('./featureDb'); var NameExistsError = require('./error/NameExistsError'); var NotFoundError = require('./error/NotFoundError'); var ValidationError = require('./error/ValidationError'); +var Promise = require("bluebird"); + var requestValidator = require('./error/requestValidator'); module.exports = function (app) { @@ -28,7 +30,7 @@ module.exports = function (app) { req.checkBody('name', 'Name must match format ^[a-zA-Z\\.\\-]+$').matches(/^[a-zA-Z\\.\\-]+$/i); requestValidator(req) - .then(featureDb.validateUniqueName) + .then(validateUniqueName) .then(function() { return eventStore.create({ type: eventType.featureCreated, @@ -77,5 +79,16 @@ module.exports = function (app) { res.status(500).end(); }); }); + + function validateUniqueName(req) { + return new Promise(function(resolve, reject) { + featureDb.getFeature(req.body.name) + .then(function() { + reject(new NameExistsError("Feature name already exist")); + }, function() { + resolve(req); + }); + }); + } }; diff --git a/lib/featureDb.js b/lib/featureDb.js index 3b94289b3a..0c132fa7b4 100644 --- a/lib/featureDb.js +++ b/lib/featureDb.js @@ -2,9 +2,7 @@ var eventStore = require('./eventStore'); var eventType = require('./eventType'); var logger = require('./logger'); var knex = require('./dbPool'); -var Promise = require("bluebird"); var NotFoundError = require('./error/NotFoundError'); -var NameExistsError = require('./error/NameExistsError'); var FEATURE_COLUMNS = ['name', 'description', 'enabled', 'strategy_name', 'parameters']; eventStore.on(eventType.featureCreated, function (event) { @@ -64,20 +62,8 @@ function eventToRow(event) { }; } -function validateUniqueName(req) { - return new Promise(function(resolve, reject) { - getFeature(req.body.name) - .then(function() { - reject(new NameExistsError("Feature name already exist")); - }, function() { - resolve(req); - }); - }); -} - module.exports = { getFeatures: getFeatures, - getFeature: getFeature, - validateUniqueName: validateUniqueName + getFeature: getFeature }; From 553f20c3a60dd743662c68957955ea4bd445c005 Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Wed, 10 Dec 2014 18:51:27 +0100 Subject: [PATCH 3/4] Syntax --- lib/featureApi.js | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/featureApi.js b/lib/featureApi.js index e123485816..69bd86e70a 100644 --- a/lib/featureApi.js +++ b/lib/featureApi.js @@ -1,13 +1,12 @@ -var logger = require('./logger'); -var eventStore = require('./eventStore'); -var eventType = require('./eventType'); -var featureDb = require('./featureDb'); -var NameExistsError = require('./error/NameExistsError'); -var NotFoundError = require('./error/NotFoundError'); -var ValidationError = require('./error/ValidationError'); -var Promise = require("bluebird"); - -var requestValidator = require('./error/requestValidator'); +var Promise = require("bluebird"); +var logger = require('./logger'); +var eventStore = require('./eventStore'); +var eventType = require('./eventType'); +var featureDb = require('./featureDb'); +var NameExistsError = require('./error/NameExistsError'); +var NotFoundError = require('./error/NotFoundError'); +var ValidationError = require('./error/ValidationError'); +var requestValidator = require('./error/requestValidator'); module.exports = function (app) { From 00da8d1e3b11ef6f5b9cab3fa6525807d66bcaa1 Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Wed, 10 Dec 2014 19:11:52 +0100 Subject: [PATCH 4/4] Clean up strategyApi with chained-promises --- ...requestValidator.js => validateRequest.js} | 0 lib/featureApi.js | 4 +- lib/strategyApi.js | 67 +++++++++++-------- 3 files changed, 42 insertions(+), 29 deletions(-) rename lib/error/{requestValidator.js => validateRequest.js} (100%) diff --git a/lib/error/requestValidator.js b/lib/error/validateRequest.js similarity index 100% rename from lib/error/requestValidator.js rename to lib/error/validateRequest.js diff --git a/lib/featureApi.js b/lib/featureApi.js index 69bd86e70a..fe9bd715b8 100644 --- a/lib/featureApi.js +++ b/lib/featureApi.js @@ -6,7 +6,7 @@ var featureDb = require('./featureDb'); var NameExistsError = require('./error/NameExistsError'); var NotFoundError = require('./error/NotFoundError'); var ValidationError = require('./error/ValidationError'); -var requestValidator = require('./error/requestValidator'); +var validateRequest = require('./error/validateRequest'); module.exports = function (app) { @@ -28,7 +28,7 @@ module.exports = function (app) { req.checkBody('name', 'Name is required').notEmpty(); req.checkBody('name', 'Name must match format ^[a-zA-Z\\.\\-]+$').matches(/^[a-zA-Z\\.\\-]+$/i); - requestValidator(req) + validateRequest(req) .then(validateUniqueName) .then(function() { return eventStore.create({ diff --git a/lib/strategyApi.js b/lib/strategyApi.js index facc307ebf..6a1e469d5e 100644 --- a/lib/strategyApi.js +++ b/lib/strategyApi.js @@ -1,6 +1,11 @@ -var eventStore = require('./eventStore'); -var eventType = require('./eventType'); -var strategyDb = require('./strategyDb'); +var Promise = require("bluebird"); +var eventStore = require('./eventStore'); +var eventType = require('./eventType'); +var strategyDb = require('./strategyDb'); +var logger = require('./logger'); +var NameExistsError = require('./error/NameExistsError'); +var ValidationError = require('./error/ValidationError'); +var validateRequest = require('./error/validateRequest'); module.exports = function (app) { @@ -32,34 +37,42 @@ module.exports = function (app) { req.checkBody('name', 'Name is required').notEmpty(); req.checkBody('name', 'Name must match format ^[a-zA-Z\\.\\-]+$').matches(/^[a-zA-Z\\.\\-]+$/i); - var errors = req.validationErrors(); - - if (errors) { - res.status(400).json(errors); - return; - } - var newStrategy = req.body; - var handleStrategyExists = function() { - var errors = [{msg: "A strategy named " + newStrategy.name + " already exists."}]; - res.status(403).json(errors); - }; - - var handleCreateStrategy = function() { - eventStore.create({ - type: eventType.strategyCreated, - createdBy: req.connection.remoteAddress, - data: newStrategy + validateRequest(req) + .then(validateStrategyName) + .then(function() { + return eventStore.create({ + type: eventType.strategyCreated, + createdBy: req.connection.remoteAddress, + data: newStrategy + }); }) - .then(function () { res.status(201).end(); }) - .catch(function () { res.status(500).end(); }); - }; - - strategyDb.getStrategy(newStrategy.name) - .then(handleStrategyExists) - .catch(handleCreateStrategy); + .then(function () { + res.status(201).end(); + }) + .catch(NameExistsError, function() { + res.status(403).json([{msg: "A strategy named '" + req.body.name + "' already exists."}]).end(); + }) + .catch(ValidationError, function() { + res.status(400).json(req.validationErrors()); + }) + .catch(function(err) { + logger.error("Could not create strategy", err); + res.status(500).end(); + }); }); + function validateStrategyName(req) { + return new Promise(function(resolve, reject) { + strategyDb.getStrategy(req.body.name) + .then(function() { + reject(new NameExistsError("Feature name already exist")); + }, function() { + resolve(req); + }); + }); + } + };