From e34eed6ecf77c9a0f77be239657735f23b692b14 Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Mon, 12 Dec 2016 16:52:20 +0100 Subject: [PATCH 1/6] Strategy should use better param description Adds support for more fields sucha as description, required, etc. relates to #182 --- docs/developer-guide.md | 11 +++- ...9-better-strategy-parameter-definitions.js | 61 +++++++++++++++++++ package.json | 3 +- 3 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 migrations/20161212101749-better-strategy-parameter-definitions.js diff --git a/docs/developer-guide.md b/docs/developer-guide.md index a5ff88d2ba..505adf55cd 100644 --- a/docs/developer-guide.md +++ b/docs/developer-guide.md @@ -59,11 +59,14 @@ npm test We use database migrations to track database changes. ### Making a schema change +In order to run migrations you will set the environment variable for DATABASE_URL + +`export DATABASE_URL=postgres://unleash_user:passord@localhost:5432/unleash` Use db-migrate to create new migrations file. ```bash -> ./node_modules/.bin/db-migrate create your-migration-name +> npm run db-migrate -- create YOUR-MIGRATION-NAME ``` All migrations requires on `up` and one `down` method. @@ -86,6 +89,12 @@ exports.down = function (db, cb) { }; ``` +Test your migrations: + +```bash +> npm run db-migrate -- up +> npm run db-migrate -- down +``` ## Publishing / Releasing new packages diff --git a/migrations/20161212101749-better-strategy-parameter-definitions.js b/migrations/20161212101749-better-strategy-parameter-definitions.js new file mode 100644 index 0000000000..fdb98656a8 --- /dev/null +++ b/migrations/20161212101749-better-strategy-parameter-definitions.js @@ -0,0 +1,61 @@ +/* eslint camelcase: "off" */ +'use strict'; + +const async = require('async'); + +exports.up = function (db, callback) { + const populateNewData = (cb) => { + db.all('select name, parameters_template from strategies', (err, results) => { + const updateSQL = results + .map(({ name, parameters_template }) => { + const parameters = []; + Object.keys(parameters_template || {}).forEach(p => { + parameters.push({ name: p, type: parameters_template[p], description: '' }); + }); + return { name, parameters }; + }) + .map(strategy => ` + UPDATE strategies + SET parameters='${JSON.stringify(strategy.parameters)}' + WHERE name='${strategy.name}';`) + .join('\n'); + + db.runSql(updateSQL, cb); + }); + }; + + async.series([ + db.addColumn.bind(db, 'strategies', 'parameters', { type: 'json' }), + populateNewData.bind(db), + db.removeColumn.bind(db, 'strategies', 'parameters_template'), + ], callback); +}; + +exports.down = function (db, callback) { + const populateOldData = (cb) => { + db.all('select name, parameters from strategies', (err, results) => { + const updateSQL = results + .map(({ name, parameters }) => { + const parameters_template = {}; + parameters.forEach(p => { + parameters_template[p.name] = p.type; + }); + + return { name, parameters_template }; + }) + .map(strategy => ` + UPDATE strategies + SET parameters_template='${JSON.stringify(strategy.parameters_template)}' + WHERE name='${strategy.name}';`) + .join('\n'); + + db.runSql(updateSQL, cb); + }); + }; + + async.series([ + db.addColumn.bind(db, 'strategies', 'parameters_template', { type: 'json' }), + populateOldData.bind(db), + db.removeColumn.bind(db, 'strategies', 'parameters'), + ], callback); +}; diff --git a/package.json b/package.json index a0e1aaf943..6caf357099 100644 --- a/package.json +++ b/package.json @@ -37,8 +37,7 @@ "start:dev": "NODE_ENV=development supervisor --ignore ./node_modules/ server.js", "start:dev:pg": "pg_virtualenv npm run start:dev:pg-chain", "start:dev:pg-chain": "export DATABASE_URL=postgres://$PGUSER:$PGPASSWORD@localhost:$PGPORT/postgres ; db-migrate up && npm run start:dev", - "db-migrate": "db-migrate up", - "db-migrate:down": "db-migrate down", + "db-migrate": "db-migrate", "lint": "eslint lib", "pretest": "npm run lint", "test": "PORT=4243 ava test lib/*.test.js lib/**/*.test.js", From 8c869cc5c288c8198ace3ca8db16b889d04a8559 Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Mon, 12 Dec 2016 16:56:52 +0100 Subject: [PATCH 2/6] Add required flag to strategy parameters --- .../20161212101749-better-strategy-parameter-definitions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migrations/20161212101749-better-strategy-parameter-definitions.js b/migrations/20161212101749-better-strategy-parameter-definitions.js index fdb98656a8..7762fc0944 100644 --- a/migrations/20161212101749-better-strategy-parameter-definitions.js +++ b/migrations/20161212101749-better-strategy-parameter-definitions.js @@ -10,7 +10,7 @@ exports.up = function (db, callback) { .map(({ name, parameters_template }) => { const parameters = []; Object.keys(parameters_template || {}).forEach(p => { - parameters.push({ name: p, type: parameters_template[p], description: '' }); + parameters.push({ name: p, type: parameters_template[p], description: '', required: false }); }); return { name, parameters }; }) From e60c7c5cfcf0aa161da5b003ffac17d0fde620ad Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Mon, 12 Dec 2016 17:09:44 +0100 Subject: [PATCH 3/6] Fix failing tests --- lib/db/strategy-store.js | 6 +++--- test/e2e/helpers/test-helper.js | 8 ++++---- test/e2e/strategy-api.test.js | 3 +++ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/db/strategy-store.js b/lib/db/strategy-store.js index ca6ad9c2b3..ac13132932 100644 --- a/lib/db/strategy-store.js +++ b/lib/db/strategy-store.js @@ -3,7 +3,7 @@ const { STRATEGY_CREATED, STRATEGY_DELETED } = require('../event-type'); const logger = require('../logger'); const NotFoundError = require('../error/notfound-error'); -const STRATEGY_COLUMNS = ['name', 'description', 'parameters_template']; +const STRATEGY_COLUMNS = ['name', 'description', 'parameters']; const TABLE = 'strategies'; class StrategyStore { @@ -44,7 +44,7 @@ class StrategyStore { return { name: row.name, description: row.description, - parametersTemplate: row.parameters_template, + parameters: row.parameters, }; } @@ -52,7 +52,7 @@ class StrategyStore { return { name: data.name, description: data.description, - parameters_template: data.parametersTemplate // eslint-disable-line + parameters: JSON.stringify(data.parameters), }; } diff --git a/test/e2e/helpers/test-helper.js b/test/e2e/helpers/test-helper.js index 958e49ddb3..d84b5b1075 100644 --- a/test/e2e/helpers/test-helper.js +++ b/test/e2e/helpers/test-helper.js @@ -48,14 +48,14 @@ function createStrategies (stores) { { name: 'default', description: 'Default on or off Strategy.', - parametersTemplate: {}, + parameters: [], }, { name: 'usersWithEmail', description: 'Active for users defined in the comma-separated emails-parameter.', - parametersTemplate: { - emails: 'String', - }, + parameters: [ + { name: 'emails', type: 'string' }, + ], }, ].map(strategy => stores.strategyStore._createStrategy(strategy)); } diff --git a/test/e2e/strategy-api.test.js b/test/e2e/strategy-api.test.js index 9eee739d32..40aa3deb0e 100644 --- a/test/e2e/strategy-api.test.js +++ b/test/e2e/strategy-api.test.js @@ -14,6 +14,9 @@ test.serial('gets all strategies', async (t) => { .get('/api/strategies') .expect('Content-Type', /json/) .expect(200) + .expect((res) => { + t.true(res.body.strategies.length === 2, 'expected to have two strategies'); + }) .then(destroy); }); From bc82aa6e77e0fb7ae9e34321c97513fbedd9cf66 Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Mon, 12 Dec 2016 21:44:21 +0100 Subject: [PATCH 4/6] Add schema validation for strategies --- lib/routes/strategy-schema.js | 20 +++++++++++++++++ lib/routes/strategy.js | 42 ++++++++++++++++++++--------------- migrator.js | 1 + test/e2e/strategy-api.test.js | 4 ++-- 4 files changed, 47 insertions(+), 20 deletions(-) create mode 100644 lib/routes/strategy-schema.js diff --git a/lib/routes/strategy-schema.js b/lib/routes/strategy-schema.js new file mode 100644 index 0000000000..9241c79737 --- /dev/null +++ b/lib/routes/strategy-schema.js @@ -0,0 +1,20 @@ +'use strict'; + +const joi = require('joi'); + +const strategySchema = joi.object().keys({ + name: joi.string() + .regex(/^[a-zA-Z0-9\\.\\-]{3,30}$/) + .required(), + description: joi.string(), + parameters: joi.array() + .required() + .items(joi.object().keys({ + name: joi.string().required(), + type: joi.string().required(), + description: joi.string(), + required: joi.boolean(), + })), +}); + +module.exports = strategySchema; diff --git a/lib/routes/strategy.js b/lib/routes/strategy.js index cad1d5ccd7..9647a8adbb 100644 --- a/lib/routes/strategy.js +++ b/lib/routes/strategy.js @@ -1,29 +1,28 @@ 'use strict'; +const joi = require('joi'); const eventType = require('../event-type'); const logger = require('../logger'); const NameExistsError = require('../error/name-exists-error'); -const ValidationError = require('../error/validation-error.js'); -const NotFoundError = require('../error/notfound-error'); -const validateRequest = require('../error/validate-request'); const extractUser = require('../extract-user'); +const strategySchema = require('./strategy-schema'); const version = 1; const handleError = (req, res, error) => { - switch (error.constructor) { - case NotFoundError: + switch (error.name) { + case 'NotFoundError': return res .status(404) .end(); - case NameExistsError: + case 'NameExistsError': return res .status(403) .json([{ msg: `A strategy named '${req.body.name}' already exists.` }]) .end(); - case ValidationError: + case 'ValidationError': return res .status(400) - .json(req.validationErrors()) + .json(error) .end(); default: logger.error('Could perfom operation', error); @@ -64,14 +63,10 @@ module.exports = function (app, config) { }); app.post('/strategies', (req, res) => { - req.checkBody('name', 'Name is required').notEmpty(); - req.checkBody('name', 'Name must match format ^[0-9a-zA-Z\\.\\-]+$').matches(/^[0-9a-zA-Z\\.\\-]+$/i); - - const newStrategy = req.body; - - validateRequest(req) + const data = req.body; + validateInput(data) .then(validateStrategyName) - .then(() => eventStore.store({ + .then((newStrategy) => eventStore.store({ type: eventType.STRATEGY_CREATED, createdBy: extractUser(req), data: newStrategy, @@ -80,11 +75,22 @@ module.exports = function (app, config) { .catch(error => handleError(req, res, error)); }); - function validateStrategyName (req) { + function validateStrategyName (data) { return new Promise((resolve, reject) => { - strategyStore.getStrategy(req.body.name) + strategyStore.getStrategy(data.name) .then(() => reject(new NameExistsError('Feature name already exist'))) - .catch(() => resolve(req)); + .catch(() => resolve(data)); + }); + } + + function validateInput (data) { + return new Promise((resolve, reject) => { + joi.validate(data, strategySchema, (err, cleaned) => { + if (err) { + return reject(err); + } + return resolve(cleaned); + }); }); } }; diff --git a/migrator.js b/migrator.js index 6c458bd5af..1b21e789ea 100644 --- a/migrator.js +++ b/migrator.js @@ -2,6 +2,7 @@ const { getInstance } = require('db-migrate'); const parseDbUrl = require('parse-database-url'); +require('db-migrate-shared').log.silence(true); function migrateDb ({ databaseUrl, databaseSchema = 'public' }) { const custom = parseDbUrl(databaseUrl); diff --git a/test/e2e/strategy-api.test.js b/test/e2e/strategy-api.test.js index 40aa3deb0e..8f47201a6d 100644 --- a/test/e2e/strategy-api.test.js +++ b/test/e2e/strategy-api.test.js @@ -42,7 +42,7 @@ test.serial('creates a new strategy', async (t) => { const { request, destroy } = await setupApp('strategy_api_serial'); return request .post('/api/strategies') - .send({ name: 'myCustomStrategy', description: 'Best strategy ever.' }) + .send({ name: 'myCustomStrategy', description: 'Best strategy ever.', parameters: [] }) .set('Content-Type', 'application/json') .expect(201) .then(destroy); @@ -62,7 +62,7 @@ test.serial('refuses to create a strategy with an existing name', async (t) => { const { request, destroy } = await setupApp('strategy_api_serial'); return request .post('/api/strategies') - .send({ name: 'default' }) + .send({ name: 'default', parameters: [] }) .set('Content-Type', 'application/json') .expect(403) .then(destroy); From 9d1f1e5639831e3ebceb3fc08664042c1516441b Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Tue, 13 Dec 2016 13:59:52 +0100 Subject: [PATCH 5/6] Unit tests for strategy-api --- migrator.js | 3 +- .../routes/fixtures/fake-strategies-store.js | 9 +++ test/unit/routes/strategies.test.js | 65 ++++++++++++++++--- 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/migrator.js b/migrator.js index 1b21e789ea..a3f4b91d72 100644 --- a/migrator.js +++ b/migrator.js @@ -1,8 +1,9 @@ 'use strict'; +require('db-migrate-shared').log.setLogLevel('error'); + const { getInstance } = require('db-migrate'); const parseDbUrl = require('parse-database-url'); -require('db-migrate-shared').log.silence(true); function migrateDb ({ databaseUrl, databaseSchema = 'public' }) { const custom = parseDbUrl(databaseUrl); diff --git a/test/unit/routes/fixtures/fake-strategies-store.js b/test/unit/routes/fixtures/fake-strategies-store.js index 9a11dc3433..a08fdc4041 100644 --- a/test/unit/routes/fixtures/fake-strategies-store.js +++ b/test/unit/routes/fixtures/fake-strategies-store.js @@ -1,5 +1,6 @@ 'use strict'; +const NotFoundError = require('../../../../lib/error/notfound-error'); @@ -8,6 +9,14 @@ module.exports = () => { return { getStrategies: () => Promise.resolve(_strategies), + getStrategy: (name) => { + const strategy = _strategies.find(s => s.name === name); + if (strategy) { + return Promise.resolve(strategy); + } else { + return Promise.reject(new NotFoundError('Not found!')); + } + }, addStrategy: (strat) => _strategies.push(strat), }; }; diff --git a/test/unit/routes/strategies.test.js b/test/unit/routes/strategies.test.js index 219854e6cf..fad6372661 100644 --- a/test/unit/routes/strategies.test.js +++ b/test/unit/routes/strategies.test.js @@ -3,31 +3,78 @@ const test = require('ava'); const store = require('./fixtures/store'); const supertest = require('supertest'); -const logger = require('../../../lib/logger'); const getApp = require('../../../lib/app'); const { EventEmitter } = require('events'); const eventBus = new EventEmitter(); -test.beforeEach(() => { - logger.setLevel('FATAL'); -}); - -test('should add version numbers for /stategies', t => { +function getSetup () { + const base = `/random${Math.round(Math.random() * 1000)}`; const stores = store.createStores(); const app = getApp({ - baseUriPath: '', + baseUriPath: base, stores, eventBus, }); - const request = supertest(app); + return { + base, + strategyStore: stores.strategyStore, + request: supertest(app), + }; +} + +test('should add version numbers for /stategies', t => { + const { request, base } = getSetup(); return request - .get('/api/strategies') + .get(`${base}/api/strategies`) .expect('Content-Type', /json/) .expect(200) .expect((res) => { t.true(res.body.version === 1); }); }); + +test('should require a name when creating a new stratey', t => { + const { request, base } = getSetup(); + + return request + .post(`${base}/api/strategies`) + .send({}) + .expect(400) + .expect((res) => { + t.true(res.body.name === 'ValidationError'); + }); +}); + +test('should require parameters array when creating a new stratey', t => { + const { request, base } = getSetup(); + + return request + .post(`${base}/api/strategies`) + .send({ name: 'TestStrat' }) + .expect(400) + .expect((res) => { + t.true(res.body.name === 'ValidationError'); + }); +}); + +test('should create a new stratey with empty parameters', () => { + const { request, base } = getSetup(); + + return request + .post(`${base}/api/strategies`) + .send({ name: 'TestStrat', parameters: [] }) + .expect(201); +}); + +test('should not be possible to override name', () => { + const { request, base, strategyStore } = getSetup(); + strategyStore.addStrategy({ name: 'Testing', parameters: [] }); + + return request + .post(`${base}/api/strategies`) + .send({ name: 'Testing', parameters: [] }) + .expect(403); +}); From cea7b45b1c3bf22b3f24a1d18a5b86ea7b47cc48 Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Tue, 13 Dec 2016 14:07:55 +0100 Subject: [PATCH 6/6] Update strategy-api docs --- docs/api/strategies-api.md | 71 ++++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 22 deletions(-) diff --git a/docs/api/strategies-api.md b/docs/api/strategies-api.md index 86dd87bd35..906996cbbf 100644 --- a/docs/api/strategies-api.md +++ b/docs/api/strategies-api.md @@ -12,25 +12,41 @@ Used to fetch all defined strategies and their defined paramters. "version": 1, "strategies": [ { - "name": "default", - "description": "Default on/off strategy.", - "parametersTemplate": null + "name": "default", + "description": "Default on/off strategy.", + "parameters": [] }, { - "name": "ActiveForUserWithEmail", - "description": "A comma separated list of email adresses this feature should be active for.", - "parametersTemplate": { - "emails": "string" - } + "name": "userWithId", + "description": "Active for userId specified in the comma seperated 'userIds' parameter.", + "parameters": [ + { + "name": "userIds", + "type": "list", + "description": "List of unique userIds the feature should be active for.", + "required": true + } + ] }, { - "name": "Accounts", - "description": "Enable for user accounts", - "parametersTemplate": { - "Accountname": "string" - } - } -]} + "name": "gradualRollout", + "description": "Gradual rollout to logged in users", + "parameters": [ + { + "name": "percentage", + "type": "percentage", + "description": "How many percent should the new feature be active for.", + "required": false + }, + { + "name": "group", + "type": "string", + "description": "Group key to use when hasing the userId. Makes sure that the same user get different value for different groups", + "required": false + } + ] + }, + ]} ``` ### Create strategy @@ -41,12 +57,23 @@ Used to fetch all defined strategies and their defined paramters. ```json { - "name": "ActiveForUserWithEmail", - "description": "A comma separated list of email adresses this feature should be active for.", - "parametersTemplate": { - "emails": "string" - } -} + "name": "gradualRollout", + "description": "Gradual rollout to logged in users", + "parameters": [ + { + "name": "percentage", + "type": "percentage", + "description": "How many percent should the new feature be active for.", + "required": false + }, + { + "name": "group", + "type": "string", + "description": "Group key to use when hasing the userId. Makes sure that the same user get different value for different groups", + "required": false + } + ] +}, ``` -Used to create a new Strategy. Name must be unique. \ No newline at end of file +Used to create a new Strategy. Name is required and must be unique. It is also required to have a parameters array, but it can be empty. \ No newline at end of file