diff --git a/CHANGELOG.md b/CHANGELOG.md index b21e7679ba..8d24da17ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ - feat: add tags (#655) - feat: add tag-types (#655) - feat: Added servicelayer (#685) +- feat: Allow deprecation of strategies (#682) - fix: upgrade knex to 0.21.15 - fix: Updated docs about event-types (#684) - fix: Add application-created event (#595) diff --git a/docs/api/admin/strategies-api.md b/docs/api/admin/strategies-api.md index ae5a15969d..cb5f24a116 100644 --- a/docs/api/admin/strategies-api.md +++ b/docs/api/admin/strategies-api.md @@ -113,3 +113,23 @@ Used to create a new Strategy. Name is required and must be unique. It is also r ``` Used to update a Strategy definition. Name can't be changed. **PS! I can be dangerous to change a implemnted strategy as the implementation also might need to be changed** + +### Deprecate strategy + +`POST: https://unleash.host.com/api/admin/strategies/:name/deprecate` + +Used to deprecate a strategy definition. This will set the deprecated flag to true. If the strategy is already deprecated, this will be a noop. + +#### Errors + +_404 NOT FOUND_ - if `:name` does not exist + +### Reactivate strategy + +`POST: https://unleash.host.com/api/admin/strategies/:name/reactivate` + +Used to reactivate a deprecated strategy defintion. This will set the deprecated flag back to false. If the strategy is not deprecated this is a noop and will still return 200. + +#### Errors + +_404 NOT FOUND_ - if if `:name` does not exist diff --git a/lib/db/strategy-store.js b/lib/db/strategy-store.js index 20d40f64c0..6d6964ee9b 100644 --- a/lib/db/strategy-store.js +++ b/lib/db/strategy-store.js @@ -2,7 +2,13 @@ const NotFoundError = require('../error/notfound-error'); -const STRATEGY_COLUMNS = ['name', 'description', 'parameters', 'built_in']; +const STRATEGY_COLUMNS = [ + 'name', + 'description', + 'parameters', + 'built_in', + 'deprecated', +]; const TABLE = 'strategies'; class StrategyStore { @@ -46,6 +52,7 @@ class StrategyStore { editable: row.built_in !== 1, description: row.description, parameters: row.parameters, + deprecated: row.deprecated, }; } @@ -57,6 +64,7 @@ class StrategyStore { name: row.name, description: row.description, parameters: row.parameters, + deprecated: row.deprecated, }; } @@ -85,6 +93,27 @@ class StrategyStore { ); } + async deprecateStrategy({ name }) { + this.db(TABLE) + .where({ name }) + .update({ deprecated: true }) + .catch(err => + this.logger.error('Could not deprecate strategy, error: ', err), + ); + } + + async reactivateStrategy({ name }) { + this.db(TABLE) + .where({ name }) + .update({ deprecated: false }) + .catch(err => + this.logger.error( + 'Could not reactivate strategy, error: ', + err, + ), + ); + } + async deleteStrategy({ name }) { return this.db(TABLE) .where({ name }) diff --git a/lib/event-differ.js b/lib/event-differ.js index 0a294bc636..be33d0c07c 100644 --- a/lib/event-differ.js +++ b/lib/event-differ.js @@ -8,6 +8,8 @@ const { STRATEGY_DELETED, STRATEGY_UPDATED, STRATEGY_IMPORT, + STRATEGY_DEPRECATED, + STRATEGY_REACTIVATED, DROP_STRATEGIES, FEATURE_CREATED, FEATURE_UPDATED, @@ -35,6 +37,8 @@ const strategyTypes = [ STRATEGY_DELETED, STRATEGY_UPDATED, STRATEGY_IMPORT, + STRATEGY_DEPRECATED, + STRATEGY_REACTIVATED, DROP_STRATEGIES, ]; diff --git a/lib/event-type.js b/lib/event-type.js index d74a634c40..9544be6d6e 100644 --- a/lib/event-type.js +++ b/lib/event-type.js @@ -12,6 +12,8 @@ module.exports = { DROP_FEATURES: 'drop-features', STRATEGY_CREATED: 'strategy-created', STRATEGY_DELETED: 'strategy-deleted', + STRATEGY_DEPRECATED: 'strategy-deprecated', + STRATEGY_REACTIVATED: 'strategy-reactivated', STRATEGY_UPDATED: 'strategy-updated', STRATEGY_IMPORT: 'strategy-import', DROP_STRATEGIES: 'drop-strategies', diff --git a/lib/routes/admin-api/strategy.js b/lib/routes/admin-api/strategy.js index e28a770b1e..0bb8d7b7fc 100644 --- a/lib/routes/admin-api/strategy.js +++ b/lib/routes/admin-api/strategy.js @@ -23,6 +23,16 @@ class StrategyController extends Controller { this.delete('/:name', this.removeStrategy, DELETE_STRATEGY); this.post('/', this.createStrategy, CREATE_STRATEGY); this.put('/:strategyName', this.updateStrategy, UPDATE_STRATEGY); + this.post( + '/:strategyName/deprecate', + this.deprecateStrategy, + UPDATE_STRATEGY, + ); + this.post( + '/:strategyName/reactivate', + this.reactivateStrategy, + UPDATE_STRATEGY, + ); } async getAllStratgies(req, res) { @@ -71,6 +81,38 @@ class StrategyController extends Controller { handleErrors(res, this.logger, error); } } + + async deprecateStrategy(req, res) { + const userName = extractUser(req); + const { strategyName } = req.params; + if (strategyName === 'default') { + res.status(403).end(); + } else { + try { + await this.strategyService.deprecateStrategy( + strategyName, + userName, + ); + res.status(200).end(); + } catch (error) { + handleErrors(res, this.logger, error); + } + } + } + + async reactivateStrategy(req, res) { + const userName = extractUser(req); + const { strategyName } = req.params; + try { + await this.strategyService.reactivateStrategy( + strategyName, + userName, + ); + res.status(200).end(); + } catch (error) { + handleErrors(res, this.logger, error); + } + } } module.exports = StrategyController; diff --git a/lib/routes/admin-api/strategy.test.js b/lib/routes/admin-api/strategy.test.js index ffe50fd4f5..a99677ef51 100644 --- a/lib/routes/admin-api/strategy.test.js +++ b/lib/routes/admin-api/strategy.test.js @@ -196,3 +196,63 @@ test('editable=true will allow edit request', t => { .send({ name, parameters: [] }) .expect(200); }); + +test('deprecating a strategy works', async t => { + t.plan(1); + const name = 'editStrat'; + const { request, base, strategyStore, perms } = getSetup(); + perms.withPermissions(UPDATE_STRATEGY); + strategyStore.createStrategy({ name, parameters: [] }); + + await request + .post(`${base}/api/admin/strategies/${name}/deprecate`) + .send() + .expect(200); + return request + .get(`${base}/api/admin/strategies/${name}`) + .expect(200) + .expect(res => t.is(res.body.deprecated, true)); +}); + +test('deprecating a non-existent strategy yields 404', t => { + t.plan(0); + const { request, base, perms } = getSetup(); + perms.withPermissions(UPDATE_STRATEGY); + return request + .post(`${base}/api/admin/strategies/non-existent-strategy/deprecate`) + .expect(404); +}); + +test('reactivating a strategy works', async t => { + t.plan(1); + const name = 'editStrat'; + const { request, base, strategyStore, perms } = getSetup(); + perms.withPermissions(UPDATE_STRATEGY); + strategyStore.createStrategy({ name, parameters: [] }); + + await request + .post(`${base}/api/admin/strategies/${name}/reactivate`) + .send() + .expect(200); + return request + .get(`${base}/api/admin/strategies/${name}`) + .expect(200) + .expect(res => t.is(res.body.deprecated, false)); +}); + +test('reactivating a non-existent strategy yields 404', t => { + t.plan(0); + const { request, base, perms } = getSetup(); + perms.withPermissions(UPDATE_STRATEGY); + return request + .post(`${base}/api/admin/strategies/non-existent-strategy/reactivate`) + .expect(404); +}); +test(`deprecating 'default' strategy will yield 403`, t => { + t.plan(0); + const { request, base, perms } = getSetup(); + perms.withPermissions(UPDATE_STRATEGY); + return request + .post(`${base}/api/admin/strategies/default/deprecate`) + .expect(403); +}); diff --git a/lib/routes/client-api/feature.js b/lib/routes/client-api/feature.js index 8663fab2a1..0cfed78151 100644 --- a/lib/routes/client-api/feature.js +++ b/lib/routes/client-api/feature.js @@ -6,10 +6,10 @@ const { filter } = require('./util'); const version = 1; class FeatureController extends Controller { - constructor({ featureToggleService }) { + constructor({ featureToggleService }, getLogger) { super(); this.toggleService = featureToggleService; - + this.logger = getLogger('client-api/feature.js'); this.get('/', this.getAll); this.get('/:featureName', this.getFeatureToggle); } diff --git a/lib/routes/client-api/metrics.js b/lib/routes/client-api/metrics.js index 445bc96a80..cfc691130c 100644 --- a/lib/routes/client-api/metrics.js +++ b/lib/routes/client-api/metrics.js @@ -19,7 +19,7 @@ class ClientMetricsController extends Controller { await this.metrics.registerClientMetrics(data, clientIp); return res.status(202).end(); } catch (e) { - this.logger.error('Failed to store metrics', e); + this.logger.warn('Failed to store metrics', e); switch (e.name) { case 'ValidationError': return res.status(400).end(); diff --git a/lib/routes/client-api/register.js b/lib/routes/client-api/register.js index 681ee5d17f..fbced5d943 100644 --- a/lib/routes/client-api/register.js +++ b/lib/routes/client-api/register.js @@ -17,7 +17,7 @@ class RegisterController extends Controller { await this.metrics.registerClient(data, clientIp); return res.status(202).end(); } catch (err) { - this.logger.error('failed to register client', err); + this.logger.warn('failed to register client', err); switch (err.name) { case 'ValidationError': return res.status(400).end(); diff --git a/lib/services/strategy-service.js b/lib/services/strategy-service.js index 951cbdee49..bc81cc305c 100644 --- a/lib/services/strategy-service.js +++ b/lib/services/strategy-service.js @@ -3,6 +3,8 @@ const NameExistsError = require('../error/name-exists-error'); const { STRATEGY_CREATED, STRATEGY_DELETED, + STRATEGY_DEPRECATED, + STRATEGY_REACTIVATED, STRATEGY_UPDATED, } = require('../event-type'); @@ -34,8 +36,33 @@ class StrategyService { }); } + async deprecateStrategy(strategyName, userName) { + await this.strategyStore.getStrategy(strategyName); // Check existence + await this.strategyStore.deprecateStrategy({ name: strategyName }); + await this.eventStore.store({ + type: STRATEGY_DEPRECATED, + createdBy: userName, + data: { + name: strategyName, + }, + }); + } + + async reactivateStrategy(strategyName, userName) { + await this.strategyStore.getStrategy(strategyName); // Check existence + await this.strategyStore.reactivateStrategy({ name: strategyName }); + await this.eventStore.store({ + type: STRATEGY_REACTIVATED, + createdBy: userName, + data: { + name: strategyName, + }, + }); + } + async createStrategy(value, userName) { const strategy = await strategySchema.validateAsync(value); + strategy.deprecated = false; await this._validateStrategyName(strategy); await this.strategyStore.createStrategy(strategy); await this.eventStore.store({ diff --git a/migrations/20210121115438-add-deprecated-column-to-strategies.js b/migrations/20210121115438-add-deprecated-column-to-strategies.js new file mode 100644 index 0000000000..313a5fc279 --- /dev/null +++ b/migrations/20210121115438-add-deprecated-column-to-strategies.js @@ -0,0 +1,18 @@ +'use strict'; + +exports.up = function(db, cb) { + db.runSql( + ` + ALTER TABLE strategies ADD COLUMN deprecated boolean default false + `, + cb, + ); +}; + +exports.down = function(db, cb) { + db.runSql(`ALTER TABLE strategies DROP COLUMN deprecated`, cb); +}; + +exports._meta = { + version: 1, +}; diff --git a/test/e2e/api/admin/strategy.e2e.test.js b/test/e2e/api/admin/strategy.e2e.test.js index ebd934896e..d522928c16 100644 --- a/test/e2e/api/admin/strategy.e2e.test.js +++ b/test/e2e/api/admin/strategy.e2e.test.js @@ -119,3 +119,56 @@ test.serial('cant update a unknown strategy', async t => { .set('Content-Type', 'application/json') .expect(404); }); + +test.serial('deprecating a strategy works', async t => { + const request = await setupApp(stores); + const name = 'deprecate'; + await request + .post('/api/admin/strategies') + .send({ name, description: 'Should deprecate this', parameters: [] }) + .set('Content-Type', 'application/json') + .expect(201); + await request + .post(`/api/admin/strategies/${name}/deprecate`) + .send() + .expect(200); + await request + .get(`/api/admin/strategies/${name}`) + .expect('Content-Type', /json/) + .expect(200) + .expect(res => t.is(res.body.deprecated, true)); +}); + +test.serial('can reactivate a deprecated strategy', async t => { + const request = await setupApp(stores); + const name = 'reactivate'; + await request + .post('/api/admin/strategies') + .send({ name, description: 'Should deprecate this', parameters: [] }) + .set('Content-Type', 'application/json') + .expect(201); + await request + .post(`/api/admin/strategies/${name}/deprecate`) + .send() + .expect(200); + await request + .get(`/api/admin/strategies/${name}`) + .expect('Content-Type', /json/) + .expect(200) + .expect(res => t.is(res.body.deprecated, true)); + await request + .post(`/api/admin/strategies/${name}/reactivate`) + .send() + .expect(200); + await request + .get(`/api/admin/strategies/${name}`) + .expect('Content-Type', /json/) + .expect(200) + .expect(res => t.is(res.body.deprecated, false)); +}); + +test.serial('cannot deprecate default strategy', async t => { + t.plan(0); + const request = await setupApp(stores); + await request.post('/api/admin/strategies/default/deprecate').expect(403); +}); diff --git a/test/fixtures/fake-strategies-store.js b/test/fixtures/fake-strategies-store.js index 1abb9f2a66..b8a925a304 100644 --- a/test/fixtures/fake-strategies-store.js +++ b/test/fixtures/fake-strategies-store.js @@ -3,7 +3,9 @@ const NotFoundError = require('../../lib/error/notfound-error'); module.exports = () => { - const _strategies = [{ name: 'default', editable: false, parameters: {} }]; + const _strategies = [ + { name: 'default', editable: false, parameters: {}, deprecated: false }, + ]; return { getStrategies: () => Promise.resolve(_strategies), @@ -31,5 +33,23 @@ module.exports = () => { _strategies.indexOf(({ name }) => name === strat.name), 1, ), + deprecateStrategy: ({ name }) => { + const deprecatedStrat = _strategies.find(s => s.name === name); + deprecatedStrat.deprecated = true; + _strategies.splice( + _strategies.indexOf(s => name === s.name), + 1, + ); + _strategies.push(deprecatedStrat); + }, + reactivateStrategy: ({ name }) => { + const reactivatedStrat = _strategies.find(s => s.name === name); + reactivatedStrat.deprecated = false; + _strategies.splice( + _strategies.indexOf(s => name === s.name), + 1, + ); + _strategies.push(reactivatedStrat); + }, }; };