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"