From 063d3f0e4a2c00310364f44adffd1e77aa4ffa35 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Mon, 8 Feb 2021 16:22:15 +0100 Subject: [PATCH] Make feature-toggle-store return 409 - After seeing frontend behaviour where the user could add the same tag multiple times, and not get errors or be stopped doing so, we'll change the backend to return a 409 if you tag a feature with a tag it already has. - Previous to this commit, the setup was to do `onConflict().ignore()` which caused the frontend to not get any help from the backend as to whether or not the operation was allowed - This fix adds a custom error and adds a branch to the handleError util method for handling just that error type with a 409. - This caused a couple of tests to receive 409, probably due to insufficient cleanup between tests. Adding faker as a dev-dependency and randomising toggle names and tag values for each test reduces the chance that we'll run into duplicate issues in the future for the tests that touches this problem fixes: #711 --- lib/db/feature-toggle-store.js | 12 +++- lib/error/feature-has-tag-error.js | 26 +++++++ lib/routes/admin-api/util.js | 5 ++ package.json | 1 + test/e2e/api/admin/feature.e2e.test.js | 99 ++++++++++++++++++-------- yarn.lock | 5 ++ 6 files changed, 118 insertions(+), 30 deletions(-) create mode 100644 lib/error/feature-has-tag-error.js diff --git a/lib/db/feature-toggle-store.js b/lib/db/feature-toggle-store.js index 6e12697210..3ac028d183 100644 --- a/lib/db/feature-toggle-store.js +++ b/lib/db/feature-toggle-store.js @@ -3,6 +3,7 @@ const metricsHelper = require('../metrics-helper'); const { DB_TIME } = require('../events'); const NotFoundError = require('../error/notfound-error'); +const FeatureHasTagError = require('../error/feature-has-tag-error'); const FEATURE_COLUMNS = [ 'name', @@ -240,8 +241,15 @@ class FeatureToggleStore { const stopTimer = this.timer('tagFeature'); await this.db(FEATURE_TAG_TABLE) .insert(this.featureAndTagToRow(featureName, tag)) - .onConflict(['feature_name', 'tag_type', 'tag_value']) - .ignore(); + .catch(err => { + if (err.code === '23505') { + throw new FeatureHasTagError( + `${featureName} already had the tag: [${tag.type}:${tag.value}]`, + ); + } else { + throw err; + } + }); stopTimer(); return tag; } diff --git a/lib/error/feature-has-tag-error.js b/lib/error/feature-has-tag-error.js new file mode 100644 index 0000000000..8180773be2 --- /dev/null +++ b/lib/error/feature-has-tag-error.js @@ -0,0 +1,26 @@ +'use strict'; + +class FeatureHasTagError extends Error { + constructor(message) { + super(); + Error.captureStackTrace(this, this.constructor); + + this.name = this.constructor.name; + this.message = message; + } + + toJSON() { + const obj = { + isJoi: false, + name: this.constructor.name, + details: [ + { + message: this.message, + }, + ], + }; + return obj; + } +} + +module.exports = FeatureHasTagError; diff --git a/lib/routes/admin-api/util.js b/lib/routes/admin-api/util.js index d9bbd5fc91..5e8608d301 100644 --- a/lib/routes/admin-api/util.js +++ b/lib/routes/admin-api/util.js @@ -42,6 +42,11 @@ const handleErrors = (res, logger, error) => { .status(400) .json(error) .end(); + case 'FeatureHasTagError': + return res + .status(409) + .json(error) + .end(); default: logger.error('Server failed executing request', error); return res.status(500).end(); diff --git a/package.json b/package.json index 02be0af759..2d7fe6a74d 100644 --- a/package.json +++ b/package.json @@ -101,6 +101,7 @@ "eslint-config-prettier": "^6.10.1", "eslint-plugin-import": "^2.20.2", "eslint-plugin-prettier": "^3.1.3", + "faker": "^5.3.1", "fetch-mock": "^9.11.0", "husky": "^4.2.3", "lint-staged": "^10.0.7", diff --git a/test/e2e/api/admin/feature.e2e.test.js b/test/e2e/api/admin/feature.e2e.test.js index dce380f733..e19f6a66af 100644 --- a/test/e2e/api/admin/feature.e2e.test.js +++ b/test/e2e/api/admin/feature.e2e.test.js @@ -1,6 +1,7 @@ 'use strict'; const test = require('ava'); +const faker = require('faker'); const dbInit = require('../../helpers/database-init'); const { setupApp } = require('../../helpers/test-helper'); const getLogger = require('../../../fixtures/no-logger'); @@ -338,24 +339,25 @@ test.serial( test.serial('can untag feature', async t => { t.plan(1); const request = await setupApp(stores); + const feature1Name = faker.helpers.slugify(faker.lorem.words(3)); await request.post('/api/admin/features').send({ - name: 'test.feature', + name: feature1Name, type: 'killswitch', enabled: true, strategies: [{ name: 'default' }], }); - const tag = { value: 'TeamGreen', type: 'simple' }; + const tag = { value: faker.lorem.word(), type: 'simple' }; await request - .post('/api/admin/features/test.feature/tags') + .post(`/api/admin/features/${feature1Name}/tags`) .send(tag) .expect(201); await request .delete( - `/api/admin/features/test.feature/tags/${tag.type}/${tag.value}`, + `/api/admin/features/${feature1Name}/tags/${tag.type}/${tag.value}`, ) .expect(200); return request - .get('/api/admin/features/test.feature/tags') + .get(`/api/admin/features/${feature1Name}/tags`) .expect('Content-Type', /json/) .expect(200) .expect(res => { @@ -366,76 +368,85 @@ test.serial('can untag feature', async t => { test.serial('Can get features tagged by tag', async t => { t.plan(2); const request = await setupApp(stores); + const feature1Name = faker.helpers.slugify(faker.lorem.words(3)); + const feature2Name = faker.helpers.slugify(faker.lorem.words(3)); await request.post('/api/admin/features').send({ - name: 'test.feature', + name: feature1Name, type: 'killswitch', enabled: true, strategies: [{ name: 'default' }], }); await request.post('/api/admin/features').send({ - name: 'test.feature2', + name: feature2Name, type: 'killswitch', enabled: true, strategies: [{ name: 'default' }], }); - const tag = { value: 'Crazy', type: 'simple' }; + const tag = { value: faker.lorem.word(), type: 'simple' }; await request - .post('/api/admin/features/test.feature/tags') + .post(`/api/admin/features/${feature1Name}/tags`) .send(tag) .expect(201); return request - .get('/api/admin/features?tag=simple:Crazy') + .get(`/api/admin/features?tag=${tag.type}:${tag.value}`) .expect('Content-Type', /json/) .expect(200) .expect(res => { t.is(res.body.features.length, 1); - t.is(res.body.features[0].name, 'test.feature'); + t.is(res.body.features[0].name, feature1Name); }); }); test.serial('Can query for multiple tags using OR', async t => { t.plan(2); const request = await setupApp(stores); + const feature1Name = faker.helpers.slugify(faker.lorem.words(3)); + const feature2Name = faker.helpers.slugify(faker.lorem.words(3)); await request.post('/api/admin/features').send({ - name: 'test.feature', + name: feature1Name, type: 'killswitch', enabled: true, strategies: [{ name: 'default' }], }); await request.post('/api/admin/features').send({ - name: 'test.feature2', + name: feature2Name, type: 'killswitch', enabled: true, strategies: [{ name: 'default' }], }); - const tag = { value: 'Crazy', type: 'simple' }; - const tag2 = { value: 'tagb', type: 'simple' }; + const tag = { value: faker.name.firstName(), type: 'simple' }; + const tag2 = { value: faker.name.firstName(), type: 'simple' }; await request - .post('/api/admin/features/test.feature/tags') + .post(`/api/admin/features/${feature1Name}/tags`) .send(tag) .expect(201); await request - .post('/api/admin/features/test.feature2/tags') + .post(`/api/admin/features/${feature2Name}/tags`) .send(tag2) .expect(201); return request - .get('/api/admin/features?tag[]=simple:Crazy&tag[]=simple:tagb') + .get( + `/api/admin/features?tag[]=${tag.type}:${tag.value}&tag[]=${tag2.type}:${tag2.value}`, + ) .expect('Content-Type', /json/) .expect(200) .expect(res => { t.is(res.body.features.length, 2); - t.is(res.body.features[0].name, 'test.feature'); + t.is(res.body.features[0].name, feature1Name); }); }); test.serial('Querying with multiple filters ANDs the filters', async t => { const request = await setupApp(stores); + const feature1Name = `test.${faker.helpers.slugify(faker.hacker.phrase())}`; + const feature2Name = faker.helpers.slugify(faker.lorem.words()); + await request.post('/api/admin/features').send({ - name: 'test.feature', + name: feature1Name, type: 'killswitch', enabled: true, strategies: [{ name: 'default' }], }); await request.post('/api/admin/features').send({ - name: 'test.feature2', + name: feature2Name, type: 'killswitch', enabled: true, strategies: [{ name: 'default' }], @@ -446,14 +457,14 @@ test.serial('Querying with multiple filters ANDs the filters', async t => { enabled: true, strategies: [{ name: 'default' }], }); - const tag = { value: 'Crazy', type: 'simple' }; - const tag2 = { value: 'tagb', type: 'simple' }; + const tag = { value: faker.lorem.word(), type: 'simple' }; + const tag2 = { value: faker.name.firstName(), type: 'simple' }; await request - .post('/api/admin/features/test.feature/tags') + .post(`/api/admin/features/${feature1Name}/tags`) .send(tag) .expect(201); await request - .post('/api/admin/features/test.feature2/tags') + .post(`/api/admin/features/${feature2Name}/tags`) .send(tag2) .expect(201); await request @@ -461,16 +472,48 @@ test.serial('Querying with multiple filters ANDs the filters', async t => { .send(tag) .expect(201); await request - .get('/api/admin/features?tag=simple:Crazy') + .get(`/api/admin/features?tag=${tag.type}:${tag.value}`) .expect('Content-Type', /json/) .expect(200) .expect(res => t.is(res.body.features.length, 2)); await request - .get('/api/admin/features?namePrefix=test&tag=simple:Crazy') + .get(`/api/admin/features?namePrefix=test&tag=${tag.type}:${tag.value}`) .expect('Content-Type', /json/) .expect(200) .expect(res => { t.is(res.body.features.length, 1); - t.is(res.body.features[0].name, 'test.feature'); + t.is(res.body.features[0].name, feature1Name); }); }); + +test.serial( + 'Tagging a feature with a tag it already has should return 409', + async t => { + const request = await setupApp(stores); + const feature1Name = `test.${faker.helpers.slugify( + faker.lorem.words(3), + )}`; + await request.post('/api/admin/features').send({ + name: feature1Name, + type: 'killswitch', + enabled: true, + strategies: [{ name: 'default' }], + }); + + const tag = { value: faker.lorem.word(), type: 'simple' }; + await request + .post(`/api/admin/features/${feature1Name}/tags`) + .send(tag) + .expect(201); + return request + .post(`/api/admin/features/${feature1Name}/tags`) + .send(tag) + .expect(409) + .expect(res => { + t.is( + res.body.details[0].message, + `${feature1Name} already had the tag: [${tag.type}:${tag.value}]`, + ); + }); + }, +); diff --git a/yarn.lock b/yarn.lock index 1f890011ae..046359aab9 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2284,6 +2284,11 @@ eyes@0.1.x: resolved "https://registry.npmjs.org/eyes/-/eyes-0.1.8.tgz" integrity sha1-Ys8SAjTGg3hdkCNIqADvPgzCC8A= +faker@^5.3.1: + version "5.3.1" + resolved "https://registry.yarnpkg.com/faker/-/faker-5.3.1.tgz#67f8f5c170b97a76b875389e0e8b9155da7b4853" + integrity sha512-sVdoApX/awJHO9DZHZsHVaJBNFiJW0n3lPs0q/nFxp/Mtya1dr2sCMktST3mdxNMHjkvKTTMAW488E+jH1eSbg== + fast-deep-equal@^3.1.1: version "3.1.3" resolved "https://registry.npmjs.org/fast-deep-equal/-/fast-deep-equal-3.1.3.tgz"