Merge pull request #183 from Unleash/strategy_def_format

Strategy should use better param description

docs/api/strategies-api.md

@@ -12,25 +12,41 @@ Used to fetch all defined strategies and their defined paramters.
     "version": 1,
     "strategies": [
         {
-        "name": "default",
+            "name": "default",
-        "description": "Default on/off strategy.",
+            "description": "Default on/off strategy.",
-            "parametersTemplate": null
+            "parameters": []
         },
         {
-            "name": "ActiveForUserWithEmail",
+            "name": "userWithId",
-            "description": "A comma separated list of email adresses this feature should be active for.",
+            "description": "Active for userId specified in the comma seperated 'userIds' parameter.",
-            "parametersTemplate": {
+            "parameters": [
-                "emails": "string"
+                {
-            }
+                    "name": "userIds",
+                    "type": "list",
+                    "description": "List of unique userIds the feature should be active for.",
+                    "required": true
+                }
+            ]
         },
         {
-            "name": "Accounts",
+            "name": "gradualRollout",
-            "description": "Enable for user accounts",
+            "description": "Gradual rollout to logged in users",
-            "parametersTemplate": {
+            "parameters": [
-                "Accountname": "string"
+                {
-            }
+                    "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",
+    "name": "gradualRollout",
-    "description": "A comma separated list of email adresses this feature should be active for.",
+    "description": "Gradual rollout to logged in users",
-    "parametersTemplate": {
+    "parameters": [
-        "emails": "string"
+        {
-    }
+            "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. 
+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. 

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

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),
         };
     }
 

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;

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) {
+    switch (error.name) {
-        case NotFoundError:
+        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();
+        const data = req.body;
-        req.checkBody('name', 'Name must match format ^[0-9a-zA-Z\\.\\-]+$').matches(/^[0-9a-zA-Z\\.\\-]+$/i);
+        validateInput(data)
-
-        const newStrategy = req.body;
-
-        validateRequest(req)
             .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);
+            });
         });
     }
 };

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);
+};

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

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": "db-migrate",
-    "db-migrate:down": "db-migrate down",
     "lint": "eslint lib",
     "pretest": "npm run lint",
     "test": "PORT=4243 ava test lib/*.test.js lib/**/*.test.js",

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: {
+            parameters: [
-                emails: 'String',
+                { name: 'emails', type: 'string' },
-            },
+            ],
         },
     ].map(strategy => stores.strategyStore._createStrategy(strategy));
 }

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

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),
     };
 };

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(() =>  {
+function getSetup () {
-    logger.setLevel('FATAL');
+    const base = `/random${Math.round(Math.random() * 1000)}`;
-});
-
-test('should add version numbers for /stategies', t => {
     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);
+});