From d8c83fb8249a132cec74192264a60ca5e37dd680 Mon Sep 17 00:00:00 2001 From: Jaanus Sellin Date: Fri, 30 May 2025 08:20:11 +0300 Subject: [PATCH] fix: do not allow creating cr for same environment (#10010) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If there are two concurrent requests to create or edit change requests, two separate ones may be created in parallel. The UI does not currently handle this scenario, and if additional changes are made, they might be added to a random existing change request—resulting in a messy and unpredictable state. This PR adds a unique index to the `change_requests` table ``` on (created_by, project, environment) WHERE state NOT IN ('Applied', 'Cancelled', 'Rejected', 'Scheduled'). ``` In the extremely rare case where such conflicting data already exists in a database, the migration will automatically cancel one of the conflicting change requests. --- ...e-request-segment-usage-read-model.test.ts | 10 +++- .../feature-search/feature.search.e2e.test.ts | 46 +++++++++++++++++-- .../20250529153326-cr-uniqueness.js | 32 +++++++++++++ 3 files changed, 81 insertions(+), 7 deletions(-) create mode 100644 src/migrations/20250529153326-cr-uniqueness.js diff --git a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts index ed6567ef97..22c78f9a09 100644 --- a/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts +++ b/src/lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model.test.ts @@ -9,6 +9,7 @@ import { randomId } from '../../util/index.js'; let db: ITestDb; let user: IUser; +let user2: IUser; const CR_ID = 123456; const CR_ID_2 = 234567; @@ -26,6 +27,10 @@ beforeAll(async () => { username: 'cr-creator', }); + user2 = await db.stores.userStore.insert({ + username: 'cr-creator-2', + }); + readModel = createChangeRequestSegmentUsageReadModel(db.rawDatabase); await db.stores.featureToggleStore.create('default', { @@ -56,13 +61,14 @@ const createCR = async ( state, changeRequestId = CR_ID, changeRequestTitle: string | null = CR_TITLE, + userId = user.id, ) => { await db.rawDatabase.table('change_requests').insert({ id: changeRequestId, environment: 'default', state, project: 'default', - created_by: user.id, + created_by: userId, created_at: '2023-01-01 00:00:00', min_approvals: 1, title: changeRequestTitle, @@ -212,7 +218,7 @@ test.each([ test(`If the same strategy appears in multiple CRs with the same segment, each segment should be listed as its own entry`, async () => { await createCR('In review', CR_ID, CR_TITLE); - await createCR('In review', CR_ID_2, null); + await createCR('In review', CR_ID_2, null, user2.id); const segmentId = 3; const strategyId = randomId(); diff --git a/src/lib/features/feature-search/feature.search.e2e.test.ts b/src/lib/features/feature-search/feature.search.e2e.test.ts index af8e74c109..6d5610e898 100644 --- a/src/lib/features/feature-search/feature.search.e2e.test.ts +++ b/src/lib/features/feature-search/feature.search.e2e.test.ts @@ -12,6 +12,7 @@ import { CREATE_FEATURE_STRATEGY, DEFAULT_PROJECT, type IUnleashStores, + TEST_AUDIT_USER, UPDATE_FEATURE_ENVIRONMENT, } from '../../types/index.js'; import { DEFAULT_ENV } from '../../util/index.js'; @@ -76,6 +77,22 @@ beforeAll(async () => { ], 'production', ); + + await app.services.userService.createUser( + { + username: 'admin@test.com', + rootRole: 1, + }, + TEST_AUDIT_USER, + ); + + await app.services.userService.createUser( + { + username: 'admin2@test.com', + rootRole: 1, + }, + TEST_AUDIT_USER, + ); }); afterAll(async () => { @@ -1339,15 +1356,26 @@ const createChangeRequest = async ({ feature, environment, state, -}: { id: number; feature: string; environment: string; state: string }) => { - await db - .rawDatabase('change_requests') - .insert({ id, environment, state, project: 'default', created_by: 1 }); + createdBy, +}: { + id: number; + feature: string; + environment: string; + state: string; + createdBy: number; +}) => { + await db.rawDatabase('change_requests').insert({ + id, + environment, + state, + project: 'default', + created_by: createdBy, + }); await db.rawDatabase('change_request_events').insert({ id, feature, action: 'updateEnabled', - created_by: 1, + created_by: createdBy, change_request_id: id, }); }; @@ -1361,48 +1389,56 @@ test('should return change request ids per environment', async () => { feature: 'my_feature_a', environment: 'production', state: 'In review', + createdBy: 1, }); await createChangeRequest({ id: 2, feature: 'my_feature_a', environment: 'production', state: 'Applied', + createdBy: 1, }); await createChangeRequest({ id: 3, feature: 'my_feature_a', environment: 'production', state: 'Cancelled', + createdBy: 1, }); await createChangeRequest({ id: 4, feature: 'my_feature_a', environment: 'production', state: 'Rejected', + createdBy: 1, }); await createChangeRequest({ id: 5, feature: 'my_feature_a', environment: 'development', state: 'Draft', + createdBy: 1, }); await createChangeRequest({ id: 6, feature: 'my_feature_a', environment: 'development', state: 'Scheduled', + createdBy: 1, }); await createChangeRequest({ id: 7, feature: 'my_feature_a', environment: 'development', state: 'Approved', + createdBy: 2, }); await createChangeRequest({ id: 8, feature: 'my_feature_b', environment: 'development', state: 'Approved', + createdBy: 3, }); const { body } = await searchFeatures({}); diff --git a/src/migrations/20250529153326-cr-uniqueness.js b/src/migrations/20250529153326-cr-uniqueness.js new file mode 100644 index 0000000000..1ec639abf6 --- /dev/null +++ b/src/migrations/20250529153326-cr-uniqueness.js @@ -0,0 +1,32 @@ +exports.up = (db, callback) => { + db.runSql( + ` + WITH ranked AS ( + SELECT id, + ROW_NUMBER() OVER ( + PARTITION BY created_by, project, environment + ORDER BY created_at DESC + ) AS rn + FROM change_requests + WHERE state NOT IN ('Applied', 'Cancelled', 'Rejected', 'Scheduled') + ) + UPDATE change_requests + SET state = 'Cancelled' + WHERE id IN ( + SELECT id FROM ranked WHERE rn > 1 + ); + + CREATE UNIQUE INDEX IF NOT EXISTS unique_pending_request_per_user_project_env + ON change_requests (created_by, project, environment) + WHERE state NOT IN ('Applied', 'Cancelled', 'Rejected', 'Scheduled'); + `, + callback, + ); +}; + +exports.down = (db, callback) => { + db.runSql( + ` DROP INDEX IF EXISTS unique_pending_request_per_user_project_env;`, + callback, + ); +};