From 5565dd8c4b69a9adec8fc896a925f662511fd47a Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Thu, 6 May 2021 14:11:56 +0200 Subject: [PATCH] chore: expose an endpoint to really delete a toggle (#808) * chore: expose an endpoint to really delete a toggle - To provide a way to run end-to-end tests without cluttering our demo instance with way too many feature-toggles, making this endpoint available will allow end-to-end tests to clean up properly after themselves --- src/lib/db/feature-toggle-store.js | 6 ++ src/lib/routes/admin-api/archive.test.js | 17 ++++++ src/lib/routes/admin-api/archive.ts | 29 ++++++++-- src/lib/routes/admin-api/feature.ts | 7 +-- src/lib/services/feature-toggle-service.js | 4 ++ .../e2e/api/admin/feature-archive.e2e.test.js | 56 +++++++++++++++++++ .../fixtures/fake-feature-toggle-store.js | 7 +++ 7 files changed, 117 insertions(+), 9 deletions(-) diff --git a/src/lib/db/feature-toggle-store.js b/src/lib/db/feature-toggle-store.js index d7c9e12583..fe9d920111 100644 --- a/src/lib/db/feature-toggle-store.js +++ b/src/lib/db/feature-toggle-store.js @@ -222,6 +222,12 @@ class FeatureToggleStore { } } + async deleteFeature(name) { + await this.db(TABLE) + .where({ name, archived: 1 }) // Feature toggle must be archived to allow deletion + .del(); + } + async reviveFeature({ name }) { try { await this.db(TABLE) diff --git a/src/lib/routes/admin-api/archive.test.js b/src/lib/routes/admin-api/archive.test.js index eee9cb7526..16873929f8 100644 --- a/src/lib/routes/admin-api/archive.test.js +++ b/src/lib/routes/admin-api/archive.test.js @@ -44,6 +44,23 @@ test('should get empty getFeatures via admin', t => { }); }); +test('should be allowed to reuse deleted toggle name', async t => { + t.plan(0); + const { request, archiveStore, base } = getSetup(); + await archiveStore.createFeature({ + name: 'ts.really.delete', + strategies: [{ name: 'default' }], + }); + await request + .delete(`${base}/api/admin/archive/ts.really.delete`) + .expect(200); + return request + .post(`${base}/api/admin/features/validate`) + .send({ name: 'ts.really.delete' }) + .set('Content-Type', 'application/json') + .expect(409); +}); + test('should get archived toggles via admin', t => { t.plan(1); const { request, base, archiveStore } = getSetup(); diff --git a/src/lib/routes/admin-api/archive.ts b/src/lib/routes/admin-api/archive.ts index d5eeb0cf93..ba259b00de 100644 --- a/src/lib/routes/admin-api/archive.ts +++ b/src/lib/routes/admin-api/archive.ts @@ -1,15 +1,20 @@ -'use strict'; - +import { Request, Response } from 'express'; import { handleErrors } from './util'; import { IUnleashConfig } from '../../types/option'; import { IUnleashServices } from '../../types/services'; +import { Logger } from '../../logger'; -const Controller = require('../controller'); +import Controller from '../controller'; -const extractUser = require('../../extract-user'); -const { UPDATE_FEATURE } = require('../../types/permissions'); +import extractUser from '../../extract-user'; +import { UPDATE_FEATURE, DELETE_FEATURE } from '../../types/permissions'; +import FeatureToggleService from '../../services/feature-toggle-service'; export default class ArchiveController extends Controller { + private readonly logger: Logger; + + private featureService: FeatureToggleService; + constructor( config: IUnleashConfig, { @@ -21,6 +26,7 @@ export default class ArchiveController extends Controller { this.featureService = featureToggleService; this.get('/features', this.getArchivedFeatures); + this.delete('/:featureName', this.deleteFeature, DELETE_FEATURE); this.post( '/revive/:featureName', this.reviveFeatureToggle, @@ -38,6 +44,19 @@ export default class ArchiveController extends Controller { } } + async deleteFeature( + req: Request, + res: Response, + ): Promise { + const { featureName } = req.params; + try { + await this.featureService.deleteFeature(featureName); + res.status(200).end(); + } catch (error) { + handleErrors(res, this.logger, error); + } + } + // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types async reviveFeatureToggle(req, res): Promise { const userName = extractUser(req); diff --git a/src/lib/routes/admin-api/feature.ts b/src/lib/routes/admin-api/feature.ts index a9f30838de..9ccf85c8c5 100644 --- a/src/lib/routes/admin-api/feature.ts +++ b/src/lib/routes/admin-api/feature.ts @@ -47,7 +47,7 @@ class FeatureController extends Controller { this.post('/', this.createToggle, CREATE_FEATURE); this.get('/:featureName', this.getToggle); this.put('/:featureName', this.updateToggle, UPDATE_FEATURE); - this.delete('/:featureName', this.deleteToggle, DELETE_FEATURE); + this.delete('/:featureName', this.archiveToggle, DELETE_FEATURE); this.post('/validate', this.validate); this.post('/:featureName/toggle', this.toggle, UPDATE_FEATURE); this.post('/:featureName/toggle/on', this.toggleOn, UPDATE_FEATURE); @@ -233,12 +233,11 @@ class FeatureController extends Controller { } } - async deleteToggle(req: Request, res: Response): Promise { + async archiveToggle(req: Request, res: Response): Promise { const { featureName } = req.params; - const userName = extractUser(req); try { - await this.featureService.archiveToggle(featureName, userName); + await this.featureService.archiveToggle(featureName); res.status(200).end(); } catch (error) { handleErrors(res, this.logger, error); diff --git a/src/lib/services/feature-toggle-service.js b/src/lib/services/feature-toggle-service.js index 97369539bc..40442d9717 100644 --- a/src/lib/services/feature-toggle-service.js +++ b/src/lib/services/feature-toggle-service.js @@ -58,6 +58,10 @@ export default class FeatureToggleService { await this.featureToggleStore.addArchivedFeature(feature); } + async deleteFeature(name) { + await this.featureToggleStore.deleteFeature(name); + } + async getFeature(name) { return this.featureToggleStore.getFeature(name); } diff --git a/src/test/e2e/api/admin/feature-archive.e2e.test.js b/src/test/e2e/api/admin/feature-archive.e2e.test.js index bb9ab60c2f..46baa65005 100644 --- a/src/test/e2e/api/admin/feature-archive.e2e.test.js +++ b/src/test/e2e/api/admin/feature-archive.e2e.test.js @@ -56,3 +56,59 @@ test.serial('must set name when reviving toggle', async t => { const request = await setupApp(stores); return request.post('/api/admin/archive/revive/').expect(404); }); + +test.serial('should be allowed to reuse deleted toggle name', async t => { + t.plan(3); + const request = await setupApp(stores); + await request + .post('/api/admin/features') + .send({ + name: 'really.delete.feature', + enabled: false, + strategies: [{ name: 'default' }], + }) + .set('Content-Type', 'application/json') + .expect(201) + .expect(res => { + t.is(res.body.name, 'really.delete.feature'); + t.is(res.body.enabled, false); + t.truthy(res.body.createdAt); + }); + await request + .delete(`/api/admin/features/really.delete.feature`) + .expect(200); + await request + .delete(`/api/admin/archive/really.delete.feature`) + .expect(200); + return request + .post(`/api/admin/features/validate`) + .send({ name: 'really.delete.feature' }) + .set('Content-Type', 'application/json') + .expect(200); +}); +test.serial('Deleting an unarchived toggle should not take effect', async t => { + t.plan(3); + const request = await setupApp(stores); + await request + .post('/api/admin/features') + .send({ + name: 'really.delete.feature', + enabled: false, + strategies: [{ name: 'default' }], + }) + .set('Content-Type', 'application/json') + .expect(201) + .expect(res => { + t.is(res.body.name, 'really.delete.feature'); + t.is(res.body.enabled, false); + t.truthy(res.body.createdAt); + }); + await request + .delete(`/api/admin/archive/really.delete.feature`) + .expect(200); + return request + .post(`/api/admin/features/validate`) + .send({ name: 'really.delete.feature' }) + .set('Content-Type', 'application/json') + .expect(409); // because it still exists +}); diff --git a/src/test/fixtures/fake-feature-toggle-store.js b/src/test/fixtures/fake-feature-toggle-store.js index dd71c1b498..7cbffe316a 100644 --- a/src/test/fixtures/fake-feature-toggle-store.js +++ b/src/test/fixtures/fake-feature-toggle-store.js @@ -64,6 +64,13 @@ module.exports = (databaseIsUp = true) => { _features.splice(0, _features.length); _archive.splice(0, _archive.length); }, + deleteFeature: featureName => { + const archivedIdx = _archive.findIndex(f => f.name === featureName); + if (archivedIdx > -1) { + _archive.splice(archivedIdx, 1); + } + return Promise.resolve(); + }, importFeature: feat => Promise.resolve(_features.push(feat)), getFeatures: query => { if (!databaseIsUp) {