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 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/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/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/migrations/20161212101749-better-strategy-parameter-definitions.js b/migrations/20161212101749-better-strategy-parameter-definitions.js new file mode 100644 index 0000000000..7762fc0944 --- /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: '', required: false }); + }); + 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/migrator.js b/migrator.js index 6c458bd5af..a3f4b91d72 100644 --- a/migrator.js +++ b/migrator.js @@ -1,5 +1,7 @@ 'use strict'; +require('db-migrate-shared').log.setLogLevel('error'); + const { getInstance } = require('db-migrate'); const parseDbUrl = require('parse-database-url'); 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", 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..8f47201a6d 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); }); @@ -39,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); @@ -59,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); 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); +});