From b4dc2b3e665066bef604a885cad5b1ddb0ee0f40 Mon Sep 17 00:00:00 2001 From: Ivar Date: Sat, 2 Jul 2016 16:25:30 +0200 Subject: [PATCH] Add support for aggregate_strategies in the API. This commit changes the features-tables: - drop columns 'strategy' and 'parameters' - add column 'strategies' of type json. - migrates existing strategy-mappings in to the new format. The idea is that the 'strategies' column should contain a json-array of strategy-configuration for the toggle: ``` [{ "name" : "strategy1", "parameters": { "name": "vale" } }] ``` To make sure to not break exiting clients the api is extended with a mapping layer (adding old fields to the json-respons, and mapping to the new format on create/update a feature toggle. this commit is first step in solving #102 --- packages/unleash-api/lib/db/feature.js | 11 ++- .../lib/helper/legacy-feature-mapper.js | 31 ++++++++ packages/unleash-api/lib/routes/feature.js | 52 +++++--------- ...160618193924-add-strategies-to-features.js | 2 + .../007-add-strategies-to-features.down.sql | 13 ++++ .../sql/007-add-strategies-to-features.up.sql | 12 ++++ packages/unleash-api/notes/schema.md | 71 +++++++++++++++++++ packages/unleash-api/test/featureApiSpec.js | 31 ++++++++ packages/unleash-api/test/specHelper.js | 44 +++++++----- .../unit/helper/legacy-feature-mapper.test.js | 48 +++++++++++++ 10 files changed, 257 insertions(+), 58 deletions(-) create mode 100644 packages/unleash-api/lib/helper/legacy-feature-mapper.js create mode 100644 packages/unleash-api/migrations/20160618193924-add-strategies-to-features.js create mode 100644 packages/unleash-api/migrations/sql/007-add-strategies-to-features.down.sql create mode 100644 packages/unleash-api/migrations/sql/007-add-strategies-to-features.up.sql create mode 100644 packages/unleash-api/notes/schema.md create mode 100644 packages/unleash-api/test/unit/helper/legacy-feature-mapper.test.js diff --git a/packages/unleash-api/lib/db/feature.js b/packages/unleash-api/lib/db/feature.js index e9325df88302..04304215692b 100644 --- a/packages/unleash-api/lib/db/feature.js +++ b/packages/unleash-api/lib/db/feature.js @@ -2,7 +2,7 @@ const eventType = require('../eventType'); const logger = require('../logger'); const NotFoundError = require('../error/NotFoundError'); -const FEATURE_COLUMNS = ['name', 'description', 'enabled', 'strategy_name', 'parameters']; +const FEATURE_COLUMNS = ['name', 'description', 'enabled', 'strategies']; module.exports = function (db, eventStore) { eventStore.on(eventType.featureCreated, event => createFeature(event.data)); @@ -39,18 +39,15 @@ module.exports = function (db, eventStore) { .map(rowToFeature); } - function rowToFeature (row) { if (!row) { throw new NotFoundError('No feature toggle found'); } - return { name: row.name, description: row.description, enabled: row.enabled > 0, - strategy: row.strategy_name, // eslint-disable-line - parameters: row.parameters, + strategies: row.strategies, }; } @@ -60,8 +57,7 @@ module.exports = function (db, eventStore) { description: data.description, enabled: data.enabled ? 1 : 0, archived: data.archived ? 1 : 0, - strategy_name: data.strategy, // eslint-disable-line - parameters: data.parameters, + strategies: JSON.stringify(data.strategies), }; } @@ -74,6 +70,7 @@ module.exports = function (db, eventStore) { } function updateFeature (data) { + console.log(data); return db('features') .where({ name: data.name }) .update(eventDataToRow(data)) diff --git a/packages/unleash-api/lib/helper/legacy-feature-mapper.js b/packages/unleash-api/lib/helper/legacy-feature-mapper.js new file mode 100644 index 000000000000..7a0844287c57 --- /dev/null +++ b/packages/unleash-api/lib/helper/legacy-feature-mapper.js @@ -0,0 +1,31 @@ +'use strict'; + +function addOldFields (feature) { + let modifiedFeature = Object.assign({}, feature); + modifiedFeature.strategy = feature.strategies[0].name; + modifiedFeature.parameters = Object.assign({}, feature.strategies[0].parameters); + return modifiedFeature; +} + +function isOldFomrat (feature) { + return feature.strategy && !feature.strategies; +} + +function toNewFormat (feature) { + if (isOldFomrat(feature)) { + return { + name: feature.name, + description: feature.description, + enabled: feature.enabled, + strategies: [ + { + name: feature.strategy, + parameters: Object.assign({}, feature.parameters), + }, + ], + }; + } + return feature; +} + +module.exports = { addOldFields, toNewFormat }; diff --git a/packages/unleash-api/lib/routes/feature.js b/packages/unleash-api/lib/routes/feature.js index 96ffb5171727..38d2cd172262 100644 --- a/packages/unleash-api/lib/routes/feature.js +++ b/packages/unleash-api/lib/routes/feature.js @@ -8,24 +8,23 @@ const ValidationError = require('../error/ValidationError'); const validateRequest = require('../error/validateRequest'); const extractUser = require('../extractUser'); +const legacyFeatureMapper = require('../helper/legacy-feature-mapper'); + module.exports = function (app, config) { const featureDb = config.featureDb; const eventStore = config.eventStore; app.get('/features', (req, res) => { - featureDb.getFeatures().then(features => { - res.json({ features }); - }); + featureDb.getFeatures() + .then((features) => features.map(legacyFeatureMapper.addOldFields)) + .then(features => res.json({ features })); }); app.get('/features/:featureName', (req, res) => { featureDb.getFeature(req.params.featureName) - .then(feature => { - res.json(feature); - }) - .catch(() => { - res.status(404).json({ error: 'Could not find feature' }); - }); + .then(legacyFeatureMapper.addOldFields) + .then(feature => res.json(feature).end()) + .catch(() => res.status(404).json({ error: 'Could not find feature' })); }); app.post('/features', (req, res) => { @@ -37,20 +36,15 @@ module.exports = function (app, config) { .then(() => eventStore.create({ type: eventType.featureCreated, createdBy: extractUser(req), - data: req.body, + data: legacyFeatureMapper.toNewFormat(req.body), })) - .then(() => { - res.status(201).end(); - }) + .then(() => res.status(201).end()) .catch(NameExistsError, () => { - res.status(403).json([{ - msg: `A feature named '${req.body.name}' already exists. It could be archived.`, - }]) - .end(); - }) - .catch(ValidationError, () => { - res.status(400).json(req.validationErrors()); + res.status(403) + .json([{ msg: `A feature named '${req.body.name}' already exists.` }]) + .end(); }) + .catch(ValidationError, () => res.status(400).json(req.validationErrors())) .catch(err => { logger.error('Could not create feature toggle', err); res.status(500).end(); @@ -60,7 +54,7 @@ module.exports = function (app, config) { app.put('/features/:featureName', (req, res) => { const featureName = req.params.featureName; const userName = extractUser(req); - const updatedFeature = req.body; + const updatedFeature = legacyFeatureMapper.toNewFormat(req.body); updatedFeature.name = featureName; @@ -70,12 +64,8 @@ module.exports = function (app, config) { createdBy: userName, data: updatedFeature, })) - .then(() => { - res.status(200).end(); - }) - .catch(NotFoundError, () => { - res.status(404).end(); - }) + .then(() => res.status(200).end()) + .catch(NotFoundError, () => res.status(404).end()) .catch(err => { logger.error(`Could not update feature toggle=${featureName}`, err); res.status(500).end(); @@ -94,12 +84,8 @@ module.exports = function (app, config) { name: featureName, }, })) - .then(() => { - res.status(200).end(); - }) - .catch(NotFoundError, () => { - res.status(404).end(); - }) + .then(() => res.status(200).end()) + .catch(NotFoundError, () => res.status(404).end()) .catch(err => { logger.error(`Could not archive feature=${featureName}`, err); res.status(500).end(); diff --git a/packages/unleash-api/migrations/20160618193924-add-strategies-to-features.js b/packages/unleash-api/migrations/20160618193924-add-strategies-to-features.js new file mode 100644 index 000000000000..82e87ab8aaf7 --- /dev/null +++ b/packages/unleash-api/migrations/20160618193924-add-strategies-to-features.js @@ -0,0 +1,2 @@ +'use strict'; +module.exports = require('../scripts/migration-runner').create('007-add-strategies-to-features'); diff --git a/packages/unleash-api/migrations/sql/007-add-strategies-to-features.down.sql b/packages/unleash-api/migrations/sql/007-add-strategies-to-features.down.sql new file mode 100644 index 000000000000..45adc8d9d470 --- /dev/null +++ b/packages/unleash-api/migrations/sql/007-add-strategies-to-features.down.sql @@ -0,0 +1,13 @@ +--create old columns +ALTER TABLE features ADD "parameters" json; +ALTER TABLE features ADD "strategy_name" varchar(255); + +--populate old columns +UPDATE features +SET strategy_name = f.strategies->0->>'name', + parameters = f.strategies->0->'parameters' +FROM features as f +WHERE f.name = features.name; + +--drop new column +ALTER TABLE features DROP COLUMN "strategies"; diff --git a/packages/unleash-api/migrations/sql/007-add-strategies-to-features.up.sql b/packages/unleash-api/migrations/sql/007-add-strategies-to-features.up.sql new file mode 100644 index 000000000000..ea01049bd36c --- /dev/null +++ b/packages/unleash-api/migrations/sql/007-add-strategies-to-features.up.sql @@ -0,0 +1,12 @@ +--create new strategies-column +ALTER TABLE features ADD "strategies" json; + +--populate the strategies column +UPDATE features +SET strategies = ('[{"name":"'||f.strategy_name||'","parameters":'||f.parameters||'}]')::json +FROM features as f +WHERE f.name = features.name; + +--delete old strategy-columns +ALTER TABLE features DROP COLUMN "strategy_name"; +ALTER TABLE features DROP COLUMN "parameters"; diff --git a/packages/unleash-api/notes/schema.md b/packages/unleash-api/notes/schema.md new file mode 100644 index 000000000000..af6fb0c650fb --- /dev/null +++ b/packages/unleash-api/notes/schema.md @@ -0,0 +1,71 @@ +# Schema + +## Table: _migrations_ + +Used by db-migrate module to keep track of migrations. + +| NAME | TYPE | SIZE | NULLABLE | COLUMN_DEF | +| ----------- | --------- | ----------- | -------- | -------------------------------------- | +| id | serial | 10 | 0 | nextval('migrations_id_seq'::regclass) | +| name | varchar | 255 | 0 | (null) | +| run_on | timestamp | 29 | 0 | (null) | + + + +## Table: _events_ +| NAME | TYPE | SIZE | NULLABLE | COLUMN_DEF | +| ----------- | --------- | ----------- | -------- | ---------------------------------- | +| id | serial | 10 | 0 | nextval('events_id_seq'::regclass) | +| created_at | timestamp | 29 | 1 | now() | +| type | varchar | 255 | 0 | (null) | +| created_by | varchar | 255 | 0 | (null) | +| data | json | 2147483647 | 1 | (null) | + + +## Table: _strategies_loc +| NAME | TYPE | SIZE | NULLABLE | COLUMN_DEF | +| ------------------- | --------- | ----------- | -------- | ---------- | +| created_at | timestamp | 29 | 1 | now() | +| name | varchar | 255 | 0 | (null) | +| description | text | 2147483647 | 1 | (null) | +| parameters_template | json | 2147483647 | 1 | (null) | + + +## Table: _features_ + +| **NAME** | **TYPE** | **SIZE** | **NULLABLE** | **COLUMN_DEF** | **COMMENT** | +| ------------- | --------- | ----------- | ------------ | -------------- | ----------- | +| created_at | timestamp | 29 | 1 | now() | | +| name | varchar | 255 | 0 | (null) | | +| enabled | int4 | 10 | 1 | 0 | | +| description | text | 2147483647 | 1 | (null) | | +| archived | int4 | 10 | 1 | 0 | | +| parameters | json | 2147483647 | 1 | (null) | deprecated (*) | +| strategy_name | varchar | 255 | 1 | (null) | deprecated (*) | +| strategies | json | 2147483647 | 1 | (null) | | + +(*) we migrated from `parmaters` and `strategy_name` to `strategies` which should contain an array of these. + +For [aggregate strategies](https://github.com/finn-no/unleash/issues/102) we had the following sql to migrate to the strategies column: + +```sql +ALTER TABLE features ADD "strategies" json; + +--populate the strategies column +UPDATE features +SET strategies = ('[{"name":"'||f.strategy_name||'","parameters":'||f.parameters||'}]')::json +FROM features as f +WHERE f.name = features.name; +``` + +In order to migrate back, one can use the following sql (it will loose all, but the first activation strategy): + +```sql +UPDATE features +SET strategy_name = f.strategies->0->>'name', + parameters = f.strategies->0->'parameters' +FROM features as f +WHERE f.name = features.name; + +ALTER TABLE features DROP COLUMN "strategies"; +``` diff --git a/packages/unleash-api/test/featureApiSpec.js b/packages/unleash-api/test/featureApiSpec.js index 4dd03a016594..4f348b7bfdaa 100644 --- a/packages/unleash-api/test/featureApiSpec.js +++ b/packages/unleash-api/test/featureApiSpec.js @@ -111,4 +111,35 @@ describe('The features api', () => { .set('Content-Type', 'application/json') .expect(403, done); }); + + describe('new strategies api', function () { + it('automatically map existing strategy to strategies array', function (done) { + request + .get('/features/featureY') + .expect('Content-Type', /json/) + .end(function (err, res) { + assert.equal(res.body.strategies.length, 1, 'expected strategy added to strategies'); + assert.equal(res.body.strategy, res.body.strategies[0].name); + assert.deepEqual(res.body.parameters, res.body.strategies[0].parameters); + done(); + }); + }); + + it('can add two strategies to a feature toggle', function (done) { + request + .put('/features/featureY') + .send({ + name: 'featureY', + description: 'soon to be the #14 feature', + enabled: false, + strategies: [ + { + name: 'baz', + parameters: { foo: 'bar' }, + }, + ] }) + .set('Content-Type', 'application/json') + .expect(200, done); + }); + }); }); diff --git a/packages/unleash-api/test/specHelper.js b/packages/unleash-api/test/specHelper.js index ebbd17abba6d..408f47f5013f 100644 --- a/packages/unleash-api/test/specHelper.js +++ b/packages/unleash-api/test/specHelper.js @@ -46,52 +46,60 @@ function createFeatures () { name: 'featureX', description: 'the #1 feature', enabled: true, - strategy: 'default', + strategies: [{ name: 'default', parameters: {} }], }, { name: 'featureY', description: 'soon to be the #1 feature', enabled: false, - strategy: 'baz', - parameters: { - foo: 'bar', - }, + strategies: [{ + name: 'baz', + parameters: { + foo: 'bar', + }, + }], }, { name: 'featureZ', description: 'terrible feature', enabled: true, - strategy: 'baz', - parameters: { - foo: 'rab', - }, + strategies: [{ + name: 'baz', + parameters: { + foo: 'rab', + }, + }], }, { name: 'featureArchivedX', description: 'the #1 feature', enabled: true, archived: true, - strategy: 'default', + strategies: [{ name: 'default', parameters: {} }], }, { name: 'featureArchivedY', description: 'soon to be the #1 feature', enabled: false, archived: true, - strategy: 'baz', - parameters: { - foo: 'bar', - }, + strategies: [{ + name: 'baz', + parameters: { + foo: 'bar', + }, + }], }, { name: 'featureArchivedZ', description: 'terrible feature', enabled: true, archived: true, - strategy: 'baz', - parameters: { - foo: 'rab', - }, + strategies: [{ + name: 'baz', + parameters: { + foo: 'rab', + }, + }], }, ], feature => featureDb._createFeature(feature)); } diff --git a/packages/unleash-api/test/unit/helper/legacy-feature-mapper.test.js b/packages/unleash-api/test/unit/helper/legacy-feature-mapper.test.js new file mode 100644 index 000000000000..ad0daf8c8798 --- /dev/null +++ b/packages/unleash-api/test/unit/helper/legacy-feature-mapper.test.js @@ -0,0 +1,48 @@ +'use strict'; +const assert = require('assert'); + +const mapper = require('../../../lib/helper/legacy-feature-mapper'); + +describe('legacy-feature-mapper', () => { + it('adds old fields to feature', () => { + const feature = { + name: 'test', + enabled: 0, + strategies: [{ + name: 'default', + parameters: { + val: 'bar', + }, + }], + }; + + let mappedFeature = mapper.addOldFields(feature); + + assert.equal(mappedFeature.name, feature.name); + assert.equal(mappedFeature.enabled, feature.enabled); + assert.equal(mappedFeature.strategy, feature.strategies[0].name); + assert.notEqual(mappedFeature.parameters, feature.strategies[0].parameters); + assert.deepEqual(mappedFeature.parameters, feature.strategies[0].parameters); + }); + + it('transforms fields to new format', () => { + const feature = { + name: 'test', + enabled: 0, + strategy: 'default', + parameters: { + val: 'bar', + }, + }; + + const mappedFeature = mapper.toNewFormat(feature); + + assert.equal(mappedFeature.name, feature.name); + assert.equal(mappedFeature.enabled, feature.enabled); + assert.equal(mappedFeature.strategies.length, 1); + assert.equal(mappedFeature.strategies[0].name, feature.strategy); + assert.deepEqual(mappedFeature.strategies[0].parameters, feature.parameters); + assert(mappedFeature.strategy === undefined); + assert(mappedFeature.parameters === undefined); + }); +});