From 65e62c64b805b7e61aa999afc57ddc1cb8200d38 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Wed, 23 Aug 2023 11:11:16 +0200 Subject: [PATCH] fix: import duplicate features (#4550) --- .../src/hooks/api/actions/useApi/useApi.ts | 4 +++ .../api/actions/useImportApi/useImportApi.ts | 5 +++ src/lib/db/feature-toggle-store.ts | 11 +++++- .../export-import-service.ts | 34 ++++++++++++------- .../export-import.e2e.test.ts | 32 +++++++++++++++-- .../import-context-validation.ts | 6 ++-- src/lib/types/stores/context-field-store.ts | 6 ++-- 7 files changed, 77 insertions(+), 21 deletions(-) diff --git a/frontend/src/hooks/api/actions/useApi/useApi.ts b/frontend/src/hooks/api/actions/useApi/useApi.ts index 256aee594d..3383e6a795 100644 --- a/frontend/src/hooks/api/actions/useApi/useApi.ts +++ b/frontend/src/hooks/api/actions/useApi/useApi.ts @@ -108,6 +108,10 @@ const useAPI = ({ const response = await res.json(); if (response?.details?.length > 0 && propagateErrors) { const error = response.details[0]; + setErrors(prev => ({ + ...prev, + unknown: error, + })); if (propagateErrors) { throw new Error(error.message || error.msg); } diff --git a/frontend/src/hooks/api/actions/useImportApi/useImportApi.ts b/frontend/src/hooks/api/actions/useImportApi/useImportApi.ts index 383a07eced..c29928528f 100644 --- a/frontend/src/hooks/api/actions/useImportApi/useImportApi.ts +++ b/frontend/src/hooks/api/actions/useImportApi/useImportApi.ts @@ -29,6 +29,11 @@ export const useImportApi = () => { }); return res; } catch (e) { + trackEvent('export_import', { + props: { + eventType: `features import failed`, + }, + }); throw e; } }; diff --git a/src/lib/db/feature-toggle-store.ts b/src/lib/db/feature-toggle-store.ts index 64ab1278ff..58dce8b9d7 100644 --- a/src/lib/db/feature-toggle-store.ts +++ b/src/lib/db/feature-toggle-store.ts @@ -8,6 +8,7 @@ import { FeatureToggle, FeatureToggleDTO, IVariant } from '../types/model'; import { IFeatureToggleStore } from '../types/stores/feature-toggle-store'; import { Db } from './db'; import { LastSeenInput } from '../services/client-metrics/last-seen-service'; +import { NameExistsError } from '../error'; export type EnvironmentFeatureNames = { [key: string]: string[] }; @@ -289,8 +290,16 @@ export default class FeatureToggleStore implements IFeatureToggleStore { return this.rowToFeature(row[0]); } catch (err) { this.logger.error('Could not insert feature, error: ', err); + if ( + typeof err.detail === 'string' && + err.detail.includes('already exists') + ) { + throw new NameExistsError( + `Feature ${data.name} already exists`, + ); + } + throw err; } - return undefined; } async update( diff --git a/src/lib/features/export-import-toggles/export-import-service.ts b/src/lib/features/export-import-toggles/export-import-service.ts index 3e553acefc..cd70e7886f 100644 --- a/src/lib/features/export-import-toggles/export-import-service.ts +++ b/src/lib/features/export-import-toggles/export-import-service.ts @@ -205,7 +205,7 @@ export default class ExportImportService { ), this.verifyFeatures(dto), ]); - await this.createToggles(cleanedDto, user); + await this.createOrUpdateToggles(cleanedDto, user); await this.importToggleVariants(dto, user); await this.importTagTypes(cleanedDto, user); await this.importTags(cleanedDto, user); @@ -348,10 +348,21 @@ export default class ExportImportService { ); } - private async createToggles(dto: ImportTogglesSchema, user: User) { + private async createOrUpdateToggles(dto: ImportTogglesSchema, user: User) { + const existingFeatures = await this.getExistingProjectFeatures(dto); + const username = extractUsernameFromUser(user); await Promise.all( - dto.data.features.map((feature) => - this.featureToggleService + dto.data.features.map((feature) => { + if (existingFeatures.includes(feature.name)) { + const { archivedAt, createdAt, ...rest } = feature; + return this.featureToggleService.updateFeatureToggle( + dto.project, + rest as FeatureToggleDTO, + username, + feature.name, + ); + } + return this.featureToggleService .validateName(feature.name) .then(() => { const { archivedAt, createdAt, ...rest } = feature; @@ -360,9 +371,8 @@ export default class ExportImportService { rest as FeatureToggleDTO, extractUsernameFromUser(user), ); - }) - .catch(() => {}), - ), + }); + }), ); } @@ -533,12 +543,10 @@ export default class ExportImportService { } private async getExistingProjectFeatures(dto: ImportTogglesSchema) { - const existingProjectsFeatures = - await this.importTogglesStore.getFeaturesInProject( - dto.data.features.map((feature) => feature.name), - dto.project, - ); - return existingProjectsFeatures; + return this.importTogglesStore.getFeaturesInProject( + dto.data.features.map((feature) => feature.name), + dto.project, + ); } private async getNewTagTypes(dto: ImportTogglesSchema) { diff --git a/src/lib/features/export-import-toggles/export-import.e2e.test.ts b/src/lib/features/export-import-toggles/export-import.e2e.test.ts index 3d984f92db..3f867bd195 100644 --- a/src/lib/features/export-import-toggles/export-import.e2e.test.ts +++ b/src/lib/features/export-import-toggles/export-import.e2e.test.ts @@ -547,6 +547,7 @@ const variants: VariantsSchema = [ const exportedFeature: ImportTogglesSchema['data']['features'][0] = { project: 'old_project', name: 'first_feature', + type: 'release', }; const anotherExportedFeature: ImportTogglesSchema['data']['features'][0] = { project: 'old_project', @@ -723,15 +724,22 @@ test('import multiple features with same tag', async () => { }); }); -test('importing same JSON should work multiple times in a row', async () => { +test('can update toggles on subsequent import', async () => { await createProjects(); await app.importToggles(defaultImportPayload); - await app.importToggles(defaultImportPayload); + await app.importToggles({ + ...defaultImportPayload, + data: { + ...defaultImportPayload.data, + features: [{ ...exportedFeature, type: 'operational' }], + }, + }); const { body: importedFeature } = await getFeature(defaultFeature); expect(importedFeature).toMatchObject({ name: 'first_feature', project: DEFAULT_PROJECT, + type: 'operational', variants, }); @@ -803,6 +811,26 @@ test('reject import with unsupported strategies', async () => { expect(body.details[0].description).toMatch(/\bcustomStrategy\b/); }); +test('reject import with duplicate features', async () => { + await createProjects(); + const importPayloadWithContextFields: ImportTogglesSchema = { + ...defaultImportPayload, + data: { + ...defaultImportPayload.data, + features: [exportedFeature, exportedFeature], + }, + }; + + const { body } = await app.importToggles( + importPayloadWithContextFields, + 409, + ); + + expect(body.details[0].description).toBe( + 'Feature first_feature already exists', + ); +}); + test('validate import data', async () => { await createProjects(); const contextField: IContextFieldDto = { diff --git a/src/lib/features/export-import-toggles/import-context-validation.ts b/src/lib/features/export-import-toggles/import-context-validation.ts index 04b8460518..718c58df38 100644 --- a/src/lib/features/export-import-toggles/import-context-validation.ts +++ b/src/lib/features/export-import-toggles/import-context-validation.ts @@ -10,7 +10,9 @@ export const isValidField = ( if (!matchingExistingField) { return true; } - return importedField.legalValues.every((value) => - matchingExistingField.legalValues.find((v) => v.value === value.value), + return (importedField.legalValues || []).every((value) => + (matchingExistingField.legalValues || []).find( + (v) => v.value === value.value, + ), ); }; diff --git a/src/lib/types/stores/context-field-store.ts b/src/lib/types/stores/context-field-store.ts index 10865fb151..8e19e6dff3 100644 --- a/src/lib/types/stores/context-field-store.ts +++ b/src/lib/types/stores/context-field-store.ts @@ -2,11 +2,11 @@ import { Store } from './store'; export interface IContextFieldDto { name: string; - description?: string; + description?: string | null; stickiness?: boolean; sortOrder?: number; - usedInProjects?: number; - usedInFeatures?: number; + usedInProjects?: number | null; + usedInFeatures?: number | null; legalValues?: ILegalValue[]; }