From 4f7fd46623909f324d4213fa3e2e2056875ef246 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Wed, 19 Apr 2023 14:10:13 +0200 Subject: [PATCH] fix: return 404 when gettings tags for a non existing feature (#3560) From the discussion here https://github.com/Unleash/unleash/pull/3496#discussion_r1163745860 This PR checks the existence of the feature before trying to get tags for the feature. Doing so by stealing the exists method from the feature store, since that's what we need to know exists, and avoiding having the feature store as a dependency to the featureTagStore seemed reasonable. --- src/lib/db/feature-tag-store.ts | 39 ++++++++++++------- src/lib/routes/admin-api/feature.ts | 2 +- .../__snapshots__/openapi.e2e.test.ts.snap | 6 +++ .../e2e/stores/feature-tag-store.e2e.test.ts | 20 +++++++++- 4 files changed, 52 insertions(+), 15 deletions(-) diff --git a/src/lib/db/feature-tag-store.ts b/src/lib/db/feature-tag-store.ts index b79d087878c..637464b934f 100644 --- a/src/lib/db/feature-tag-store.ts +++ b/src/lib/db/feature-tag-store.ts @@ -9,6 +9,7 @@ import { IFeatureTagStore, } from '../types/stores/feature-tag-store'; import { Db } from './db'; +import NotFoundError from '../error/notfound-error'; const COLUMNS = ['feature_name', 'tag_type', 'tag_value']; const TABLE = 'feature_tag'; @@ -95,12 +96,27 @@ class FeatureTagStore implements IFeatureTagStore { async getAllTagsForFeature(featureName: string): Promise { const stopTimer = this.timer('getAllForFeature'); - const rows = await this.db - .select(COLUMNS) - .from(TABLE) - .where({ feature_name: featureName }); - stopTimer(); - return rows.map(this.featureTagRowToTag); + if (await this.featureExists(featureName)) { + const rows = await this.db + .select(COLUMNS) + .from(TABLE) + .where({ feature_name: featureName }); + stopTimer(); + return rows.map(this.featureTagRowToTag); + } else { + throw new NotFoundError( + `Could not find feature with name ${featureName}`, + ); + } + } + + async featureExists(featureName: string): Promise { + const result = await this.db.raw( + 'SELECT EXISTS (SELECT 1 FROM features WHERE name = ?) AS present', + [featureName], + ); + const { present } = result.rows[0]; + return present; } async getAllByFeatures(features: string[]): Promise { @@ -187,13 +203,10 @@ class FeatureTagStore implements IFeatureTagStore { } featureTagRowToTag(row: FeatureTagTable): ITag { - if (row) { - return { - value: row.tag_value, - type: row.tag_type, - }; - } - return null; + return { + value: row.tag_value, + type: row.tag_type, + }; } rowToFeatureAndTag(row: FeatureTagTable): IFeatureAndTag { diff --git a/src/lib/routes/admin-api/feature.ts b/src/lib/routes/admin-api/feature.ts index a66596331ac..8d705a48cd6 100644 --- a/src/lib/routes/admin-api/feature.ts +++ b/src/lib/routes/admin-api/feature.ts @@ -120,7 +120,7 @@ class FeatureController extends Controller { operationId: 'listTags', responses: { 200: createResponseSchema('tagsSchema'), - ...getStandardResponses(401), + ...getStandardResponses(401, 403, 404), }, }), ], diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index 0e9ca77865c..9dffa12b311 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap @@ -6182,6 +6182,12 @@ If the provided project does not exist, the list of events will be empty.", "401": { "description": "Authorization information is missing or invalid. Provide a valid API token as the \`authorization\` header, e.g. \`authorization:*.*.my-admin-token\`.", }, + "403": { + "description": "User credentials are valid but does not have enough privileges to execute this operation", + }, + "404": { + "description": "The requested resource was not found.", + }, }, "summary": "Get all tags for a feature.", "tags": [ diff --git a/src/test/e2e/stores/feature-tag-store.e2e.test.ts b/src/test/e2e/stores/feature-tag-store.e2e.test.ts index b2e26d62e67..0eeee25be84 100644 --- a/src/test/e2e/stores/feature-tag-store.e2e.test.ts +++ b/src/test/e2e/stores/feature-tag-store.e2e.test.ts @@ -2,6 +2,7 @@ import { IFeatureTagStore } from 'lib/types/stores/feature-tag-store'; import { IFeatureToggleStore } from 'lib/types/stores/feature-toggle-store'; import dbInit from '../helpers/database-init'; import getLogger from '../../fixtures/no-logger'; +import NotFoundError from '../../../lib/error/notfound-error'; let stores; let db; @@ -43,7 +44,7 @@ test('should tag feature', async () => { expect(featureTag.tagValue).toBe(tag.value); }); -test('feature tag exits', async () => { +test('feature tag exists', async () => { await featureTagStore.tagFeature(featureName, tag); const exists = await featureTagStore.exists({ featureName, @@ -97,3 +98,20 @@ test('should import feature tags', async () => { const all = await featureTagStore.getAll(); expect(all).toHaveLength(2); }); + +test('should throw not found error if feature does not exist', async () => { + await expect(async () => + featureTagStore.getAllTagsForFeature('non.existing.toggle'), + ).rejects.toThrow( + new NotFoundError( + `Could not find feature with name non.existing.toggle`, + ), + ); +}); + +test('Returns empty tag list for existing feature with no tags', async () => { + const name = 'feature.with.no.tags'; + await featureToggleStore.create('default', { name }); + let tags = await featureTagStore.getAllTagsForFeature(name); + expect(tags).toHaveLength(0); +});