Merge pull request #712 from Unleash/fix-711-conflict-on-existing-tag

Make feature-toggle-store return 409

lib/db/feature-toggle-store.js

@@ -3,6 +3,8 @@
 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 { UNIQUE_CONSTRAINT_VIOLATION } = require('../error/db-error');
 
 const FEATURE_COLUMNS = [
     'name',
@@ -240,8 +242,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'])
+            .catch(err => {
-            .ignore();
+                if (err.code === UNIQUE_CONSTRAINT_VIOLATION) {
+                    throw new FeatureHasTagError(
+                        `${featureName} already had the tag: [${tag.type}:${tag.value}]`,
+                    );
+                } else {
+                    throw err;
+                }
+            });
         stopTimer();
         return tag;
     }

lib/error/db-error.js

@@ -0,0 +1,3 @@
+module.exports = {
+    UNIQUE_CONSTRAINT_VIOLATION: '23505',
+};

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: true,
+            name: this.constructor.name,
+            details: [
+                {
+                    message: this.message,
+                },
+            ],
+        };
+        return obj;
+    }
+}
+
+module.exports = FeatureHasTagError;

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();

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",

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,86 @@ 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);
+    t.plan(3);
     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.name.firstName(), type: 'simple' };
-    const tag2 = { value: 'tagb', 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.true(res.body.features.some(f => f.name === feature1Name));
+            t.true(res.body.features.some(f => f.name === feature2Name));
         });
 });
 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 +458,14 @@ test.serial('Querying with multiple filters ANDs the filters', async t => {
         enabled: true,
         strategies: [{ name: 'default' }],
     });
-    const tag = { value: 'Crazy', type: 'simple' };
+    const tag = { value: faker.lorem.word(), type: 'simple' };
-    const tag2 = { value: 'tagb', 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 +473,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}]`,
+                );
+            });
+    },
+);

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"