From e73d0f4137bf753167e2a8eb8c69f678fd517543 Mon Sep 17 00:00:00 2001 From: Andreas Fitzek Date: Tue, 14 Oct 2025 14:09:07 +0200 Subject: [PATCH] chore(core): Ensure unique display name for roles (#20601) --- .../backend-test-utils/MIGRATION_TESTING.md | 5 + .../src/migration-test-helpers.ts | 11 + .../common/1760020838000-UniqueRoleNames.ts | 64 +++ .../@n8n/db/src/migrations/mysqldb/index.ts | 2 + .../db/src/migrations/postgresdb/index.ts | 2 + .../@n8n/db/src/migrations/sqlite/index.ts | 2 + packages/cli/src/services/role.service.ts | 22 +- .../cross-project-access.test.ts | 2 + .../custom-roles-functionality.test.ts | 1 + .../resource-access-matrix.test.ts | 1 + .../controllers/role.controller-db.test.ts | 6 + .../integration/services/role.service.test.ts | 40 +- .../cli/test/integration/shared/db/roles.ts | 14 +- .../cli/test/integration/users.api.test.ts | 2 +- .../1760020838000-unique-role-names.test.ts | 438 ++++++++++++++++++ 15 files changed, 595 insertions(+), 17 deletions(-) create mode 100644 packages/@n8n/db/src/migrations/common/1760020838000-UniqueRoleNames.ts create mode 100644 packages/cli/test/migration/1760020838000-unique-role-names.test.ts diff --git a/packages/@n8n/backend-test-utils/MIGRATION_TESTING.md b/packages/@n8n/backend-test-utils/MIGRATION_TESTING.md index 33083c31255..375954e3e9f 100644 --- a/packages/@n8n/backend-test-utils/MIGRATION_TESTING.md +++ b/packages/@n8n/backend-test-utils/MIGRATION_TESTING.md @@ -24,6 +24,11 @@ Runs a single migration by name. **Throws:** - `UnexpectedError` if the migration is not found or database is not initialized +## `undoLastSingleMigration(): Promise` + +Undoes the last single migration. + + ## Usage Example ```typescript diff --git a/packages/@n8n/backend-test-utils/src/migration-test-helpers.ts b/packages/@n8n/backend-test-utils/src/migration-test-helpers.ts index 9afbf82d0aa..a065e43927e 100644 --- a/packages/@n8n/backend-test-utils/src/migration-test-helpers.ts +++ b/packages/@n8n/backend-test-utils/src/migration-test-helpers.ts @@ -90,6 +90,17 @@ export async function initDbUpToMigration(beforeMigrationName: string): Promise< } } +/** + * Undo the last single migration down. + * Useful for testing the down path of a specific migration after inserting test data. + */ +export async function undoLastSingleMigration(): Promise { + const dataSource = Container.get(DataSource); + await dataSource.undoLastMigration({ + transaction: 'each', + }); +} + /** * Run a single migration by name. * Useful for testing a specific migration after inserting test data. diff --git a/packages/@n8n/db/src/migrations/common/1760020838000-UniqueRoleNames.ts b/packages/@n8n/db/src/migrations/common/1760020838000-UniqueRoleNames.ts new file mode 100644 index 00000000000..d067f939353 --- /dev/null +++ b/packages/@n8n/db/src/migrations/common/1760020838000-UniqueRoleNames.ts @@ -0,0 +1,64 @@ +import type { Role } from '../../entities'; +import type { MigrationContext, ReversibleMigration } from '../migration-types'; + +export class UniqueRoleNames1760020838000 implements ReversibleMigration { + async up({ isMysql, escape, runQuery }: MigrationContext) { + const tableName = escape.tableName('role'); + const displayNameColumn = escape.columnName('displayName'); + const slugColumn = escape.columnName('slug'); + const createdAtColumn = escape.columnName('createdAt'); + const allRoles: Array> = await runQuery( + `SELECT ${slugColumn}, ${displayNameColumn} FROM ${tableName} ORDER BY ${displayNameColumn}, ${createdAtColumn} ASC`, + ); + + // Group roles by displayName in memory + const groupedByName = new Map>>(); + + for (const role of allRoles) { + const existing = groupedByName.get(role.displayName) || []; + existing.push(role); + groupedByName.set(role.displayName, existing); + } + + for (const [_, roles] of groupedByName.entries()) { + if (roles.length > 1) { + const duplicates = roles.slice(1); + let index = 2; + for (const role of duplicates.values()) { + let newDisplayName = `${role.displayName} ${index}`; + while (allRoles.some((r) => r.displayName === newDisplayName)) { + index++; + newDisplayName = `${role.displayName} ${index}`; + } + await runQuery( + `UPDATE ${tableName} SET ${displayNameColumn} = :displayName WHERE ${slugColumn} = :slug`, + { + displayName: newDisplayName, + slug: role.slug, + }, + ); + index++; + } + } + } + + const indexName = escape.indexName('UniqueRoleDisplayName'); + // MySQL cannot create an index on a column with a type of TEXT or BLOB without a length limit + // The (100) specifies the maximum length of the index key + // meaning that only the first 100 characters of the displayName column will be used for indexing + // But since in our DTOs we limit the displayName to 100 characters, we can safely use this prefix length + await runQuery( + isMysql + ? `CREATE UNIQUE INDEX ${indexName} ON ${tableName} (${displayNameColumn}(100))` + : `CREATE UNIQUE INDEX ${indexName} ON ${tableName} (${displayNameColumn})`, + ); + } + + async down({ isMysql, escape, runQuery }: MigrationContext) { + const tableName = escape.tableName('role'); + const indexName = escape.indexName('UniqueRoleDisplayName'); + await runQuery( + isMysql ? `ALTER TABLE ${tableName} DROP INDEX ${indexName}` : `DROP INDEX ${indexName}`, + ); + } +} diff --git a/packages/@n8n/db/src/migrations/mysqldb/index.ts b/packages/@n8n/db/src/migrations/mysqldb/index.ts index 4dbbe3905da..feb130f87ec 100644 --- a/packages/@n8n/db/src/migrations/mysqldb/index.ts +++ b/packages/@n8n/db/src/migrations/mysqldb/index.ts @@ -101,6 +101,7 @@ import { ReplaceDataStoreTablesWithDataTables1754475614602 } from '../common/175 import { AddTimestampsToRoleAndRoleIndexes1756906557570 } from '../common/1756906557570-AddTimestampsToRoleAndRoleIndexes'; import { ChangeValueTypesForInsights1759399811000 } from '../common/1759399811000-ChangeValueTypesForInsights'; import { CreateChatHubTables1760019379982 } from '../common/1760019379982-CreateChatHubTables'; +import { UniqueRoleNames1760020838000 } from '../common/1760020838000-UniqueRoleNames'; import type { Migration } from '../migration-types'; import { UpdateParentFolderIdColumn1740445074052 } from '../mysqldb/1740445074052-UpdateParentFolderIdColumn'; @@ -209,4 +210,5 @@ export const mysqlMigrations: Migration[] = [ AddAudienceColumnToApiKeys1758731786132, ChangeValueTypesForInsights1759399811000, CreateChatHubTables1760019379982, + UniqueRoleNames1760020838000, ]; diff --git a/packages/@n8n/db/src/migrations/postgresdb/index.ts b/packages/@n8n/db/src/migrations/postgresdb/index.ts index bca14c1dd03..f399b61c34a 100644 --- a/packages/@n8n/db/src/migrations/postgresdb/index.ts +++ b/packages/@n8n/db/src/migrations/postgresdb/index.ts @@ -101,6 +101,7 @@ import { AddTimestampsToRoleAndRoleIndexes1756906557570 } from '../common/175690 import { AddAudienceColumnToApiKeys1758731786132 } from '../common/1758731786132-AddAudienceColumnToApiKey'; import { ChangeValueTypesForInsights1759399811000 } from '../common/1759399811000-ChangeValueTypesForInsights'; import { CreateChatHubTables1760019379982 } from '../common/1760019379982-CreateChatHubTables'; +import { UniqueRoleNames1760020838000 } from '../common/1760020838000-UniqueRoleNames'; import type { Migration } from '../migration-types'; export const postgresMigrations: Migration[] = [ @@ -207,4 +208,5 @@ export const postgresMigrations: Migration[] = [ AddAudienceColumnToApiKeys1758731786132, ChangeValueTypesForInsights1759399811000, CreateChatHubTables1760019379982, + UniqueRoleNames1760020838000, ]; diff --git a/packages/@n8n/db/src/migrations/sqlite/index.ts b/packages/@n8n/db/src/migrations/sqlite/index.ts index dfeceaab698..55c12939e6a 100644 --- a/packages/@n8n/db/src/migrations/sqlite/index.ts +++ b/packages/@n8n/db/src/migrations/sqlite/index.ts @@ -95,6 +95,7 @@ import { CreateDataStoreTables1754475614601 } from '../common/1754475614601-Crea import { ReplaceDataStoreTablesWithDataTables1754475614602 } from '../common/1754475614602-ReplaceDataStoreTablesWithDataTables'; import { AddTimestampsToRoleAndRoleIndexes1756906557570 } from '../common/1756906557570-AddTimestampsToRoleAndRoleIndexes'; import { ChangeValueTypesForInsights1759399811000 } from '../common/1759399811000-ChangeValueTypesForInsights'; +import { UniqueRoleNames1760020838000 } from '../common/1760020838000-UniqueRoleNames'; import type { Migration } from '../migration-types'; import { LinkRoleToProjectRelationTable1753953244168 } from './../common/1753953244168-LinkRoleToProjectRelationTable'; import { AddProjectIdToVariableTable1758794506893 } from './1758794506893-AddProjectIdToVariableTable'; @@ -201,6 +202,7 @@ const sqliteMigrations: Migration[] = [ AddAudienceColumnToApiKeys1758731786132, ChangeValueTypesForInsights1759399811000, CreateChatHubTables1760019379982, + UniqueRoleNames1760020838000, ]; export { sqliteMigrations }; diff --git a/packages/cli/src/services/role.service.ts b/packages/cli/src/services/role.service.ts index 0d4f49856cd..f634259a43c 100644 --- a/packages/cli/src/services/role.service.ts +++ b/packages/cli/src/services/role.service.ts @@ -35,6 +35,7 @@ import { UnexpectedError, UserError } from 'n8n-workflow'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { RoleCacheService } from './role-cache.service'; +import { isUniqueConstraintError } from '@/response-helper'; @Service() export class RoleService { @@ -143,6 +144,11 @@ export class RoleService { if (error instanceof UserError && error.message === 'Cannot update system roles') { throw new BadRequestError('Cannot update system roles'); } + + if (error instanceof Error && isUniqueConstraintError(error)) { + throw new BadRequestError(`A role with the name "${displayName}" already exists`); + } + throw error; } } @@ -159,12 +165,20 @@ export class RoleService { role.systemRole = false; role.roleType = newRole.roleType; role.slug = `${newRole.roleType}:${newRole.displayName.toLowerCase().replace(/[^a-z0-9]+/g, '-')}-${Math.random().toString(36).substring(2, 8)}`; - const createdRole = await this.roleRepository.save(role); - // Invalidate cache after role creation - await this.roleCacheService.invalidateCache(); + try { + const createdRole = await this.roleRepository.save(role); - return this.dbRoleToRoleDTO(createdRole); + // Invalidate cache after role creation + await this.roleCacheService.invalidateCache(); + + return this.dbRoleToRoleDTO(createdRole); + } catch (error) { + if (error instanceof Error && isUniqueConstraintError(error)) { + throw new BadRequestError(`A role with the name "${newRole.displayName}" already exists`); + } + throw error; + } } async checkRolesExist( diff --git a/packages/cli/test/integration/access-control/cross-project-access.test.ts b/packages/cli/test/integration/access-control/cross-project-access.test.ts index adf47b1603b..dabf6227a4a 100644 --- a/packages/cli/test/integration/access-control/cross-project-access.test.ts +++ b/packages/cli/test/integration/access-control/cross-project-access.test.ts @@ -4,6 +4,7 @@ import { randomCredentialPayload, createWorkflow, mockInstance, + testDb, } from '@n8n/backend-test-utils'; import type { Project, User, Role } from '@n8n/db'; @@ -126,6 +127,7 @@ describe('Cross-Project Access Control Tests', () => { }); afterAll(async () => { + await testDb.truncate(['User']); await cleanupRolesAndScopes(); }); diff --git a/packages/cli/test/integration/access-control/custom-roles-functionality.test.ts b/packages/cli/test/integration/access-control/custom-roles-functionality.test.ts index a4f15d5ffc7..a68a1764072 100644 --- a/packages/cli/test/integration/access-control/custom-roles-functionality.test.ts +++ b/packages/cli/test/integration/access-control/custom-roles-functionality.test.ts @@ -166,6 +166,7 @@ describe('Custom Role Functionality Tests', () => { }); afterAll(async () => { + await testDb.truncate(['User']); await cleanupRolesAndScopes(); }); diff --git a/packages/cli/test/integration/access-control/resource-access-matrix.test.ts b/packages/cli/test/integration/access-control/resource-access-matrix.test.ts index 4b4273511ff..d1980b06d03 100644 --- a/packages/cli/test/integration/access-control/resource-access-matrix.test.ts +++ b/packages/cli/test/integration/access-control/resource-access-matrix.test.ts @@ -135,6 +135,7 @@ describe('Resource Access Control Matrix Tests', () => { }); afterAll(async () => { + await testDb.truncate(['User']); await cleanupRolesAndScopes(); }); diff --git a/packages/cli/test/integration/controllers/role.controller-db.test.ts b/packages/cli/test/integration/controllers/role.controller-db.test.ts index b7670ebd849..c78480da0b8 100644 --- a/packages/cli/test/integration/controllers/role.controller-db.test.ts +++ b/packages/cli/test/integration/controllers/role.controller-db.test.ts @@ -10,7 +10,9 @@ import { PROJECT_EDITOR_ROLE, PROJECT_OWNER_ROLE, PROJECT_VIEWER_ROLE, + RoleRepository, } from '@n8n/db'; +import { Container } from '@n8n/di'; describe('RoleController - Integration Tests', () => { const testServer = setupTestServer({ endpointGroups: ['role'] }); @@ -32,6 +34,10 @@ describe('RoleController - Integration Tests', () => { afterEach(async () => { await cleanupRolesAndScopes(); + // Clear custom roles + await Container.get(RoleRepository).delete({ + systemRole: false, + }); }); afterAll(async () => { diff --git a/packages/cli/test/integration/services/role.service.test.ts b/packages/cli/test/integration/services/role.service.test.ts index 70627217ab1..71f3d85629f 100644 --- a/packages/cli/test/integration/services/role.service.test.ts +++ b/packages/cli/test/integration/services/role.service.test.ts @@ -53,8 +53,8 @@ afterAll(async () => { }); afterEach(async () => { - await cleanupRolesAndScopes(); await testDb.truncate(['User']); + await cleanupRolesAndScopes(); }); describe('RoleService', () => { @@ -909,6 +909,25 @@ describe('RoleService', () => { expect(result.slug).toContain('role'); expect(result.slug).toContain('name'); }); + + it('should throw BadRequestError when a role with the same display name already exists', async () => { + const testScopes = await createTestScopes(); + const createRoleDto: CreateRoleDto = { + displayName: 'Existing Role', + roleType: 'project', + scopes: [testScopes.readScope.slug], + }; + + await roleService.createCustomRole(createRoleDto); + + const duplicateRoleDto: CreateRoleDto = { + displayName: 'Existing Role', + roleType: 'project', + scopes: [testScopes.writeScope.slug], + }; + + await expect(roleService.createCustomRole(duplicateRoleDto)).rejects.toThrow(BadRequestError); + }); }); describe('updateCustomRole', () => { @@ -1032,6 +1051,25 @@ describe('RoleService', () => { 'The following scopes are invalid: invalid:scope', ); }); + + it('should throw error when a role with the same display name already exists', async () => { + // + // ARRANGE + // + const existingRole = await createRole(); + const otherExistingRole = await createRole(); + + const updateRoleDto: UpdateRoleDto = { + displayName: existingRole.displayName, + }; + + // + // ACT & ASSERT + // + await expect( + roleService.updateCustomRole(otherExistingRole.slug, updateRoleDto), + ).rejects.toThrow(`A role with the name "${existingRole.displayName}" already exists`); + }); }); describe('removeCustomRole', () => { diff --git a/packages/cli/test/integration/shared/db/roles.ts b/packages/cli/test/integration/shared/db/roles.ts index 8a221fa569c..6c0f9566d44 100644 --- a/packages/cli/test/integration/shared/db/roles.ts +++ b/packages/cli/test/integration/shared/db/roles.ts @@ -10,7 +10,7 @@ export async function createRole(overrides: Partial = {}): Promise { const defaultRole: Partial = { slug: `test-role-${Math.random().toString(36).substring(7)}`, - displayName: 'Test Role', + displayName: `Test Role ${Math.random().toString(36).substring(7)}`, description: 'A test role for integration testing', systemRole: false, roleType: 'project', @@ -164,11 +164,7 @@ export async function cleanupRolesAndScopes(): Promise { .getMany(); for (const role of testRoles) { - try { - await roleRepository.delete({ slug: role.slug }); - } catch (error) { - // Ignore errors for system roles or roles with dependencies - } + await roleRepository.delete({ slug: role.slug }); } // Delete test scopes @@ -178,10 +174,6 @@ export async function cleanupRolesAndScopes(): Promise { .getMany(); for (const scope of testScopes) { - try { - await scopeRepository.delete({ slug: scope.slug }); - } catch (error) { - // Ignore errors for scopes with dependencies - } + await scopeRepository.delete({ slug: scope.slug }); } } diff --git a/packages/cli/test/integration/users.api.test.ts b/packages/cli/test/integration/users.api.test.ts index 731a93d228a..3b40a6b7449 100644 --- a/packages/cli/test/integration/users.api.test.ts +++ b/packages/cli/test/integration/users.api.test.ts @@ -1644,7 +1644,7 @@ describe('PATCH /users/:id/role', () => { test('should change to existing custom role', async () => { const customRole = 'custom:role'; - await createRole({ slug: customRole, displayName: 'Custom Role', roleType: 'global' }); + await createRole({ slug: customRole, displayName: 'Custom Role 1', roleType: 'global' }); const response = await ownerAgent.patch(`/users/${member.id}/role`).send({ newRoleName: customRole, }); diff --git a/packages/cli/test/migration/1760020838000-unique-role-names.test.ts b/packages/cli/test/migration/1760020838000-unique-role-names.test.ts new file mode 100644 index 00000000000..78bb874d70c --- /dev/null +++ b/packages/cli/test/migration/1760020838000-unique-role-names.test.ts @@ -0,0 +1,438 @@ +import { + createTestMigrationContext, + initDbUpToMigration, + runSingleMigration, + undoLastSingleMigration, + type TestMigrationContext, +} from '@n8n/backend-test-utils'; +import { DbConnection } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { DataSource } from '@n8n/typeorm'; + +const MIGRATION_NAME = 'UniqueRoleNames1760020838000'; + +interface RoleData { + slug: string; + displayName: string; + createdAt: Date; + systemRole?: boolean; + roleType?: string; + description?: string | null; +} + +interface RoleRow { + slug: string; + displayName: string; + createdAt: Date; +} + +describe('UniqueRoleNames Migration', () => { + let dataSource: DataSource; + + beforeAll(async () => { + // Initialize DB connection without running migrations + const dbConnection = Container.get(DbConnection); + await dbConnection.init(); + + dataSource = Container.get(DataSource); + + // Run migrations up to (but not including) target migration + await initDbUpToMigration(MIGRATION_NAME); + }); + + afterAll(async () => { + const dbConnection = Container.get(DbConnection); + await dbConnection.close(); + }); + + /** + * Helper function to insert a test role with controlled timestamp + */ + async function insertTestRole(context: TestMigrationContext, roleData: RoleData): Promise { + const tableName = context.escape.tableName('role'); + const slugColumn = context.escape.columnName('slug'); + const displayNameColumn = context.escape.columnName('displayName'); + const createdAtColumn = context.escape.columnName('createdAt'); + const updatedAtColumn = context.escape.columnName('updatedAt'); + const systemRoleColumn = context.escape.columnName('systemRole'); + const roleTypeColumn = context.escape.columnName('roleType'); + const descriptionColumn = context.escape.columnName('description'); + + const systemRole = roleData.systemRole ?? false; + const roleType = roleData.roleType ?? 'project'; + const description = roleData.description ?? null; + + await context.queryRunner.query( + `INSERT INTO ${tableName} (${slugColumn}, ${displayNameColumn}, ${createdAtColumn}, ${updatedAtColumn}, ${systemRoleColumn}, ${roleTypeColumn}, ${descriptionColumn}) VALUES (?, ?, ?, ?, ?, ?, ?)`, + [ + roleData.slug, + roleData.displayName, + roleData.createdAt, + roleData.createdAt, + systemRole, + roleType, + description, + ], + ); + } + + /** + * Helper function to retrieve all roles ordered by creation date + */ + async function getAllRoles(context: TestMigrationContext): Promise { + const tableName = context.escape.tableName('role'); + const slugColumn = context.escape.columnName('slug'); + const displayNameColumn = context.escape.columnName('displayName'); + const createdAtColumn = context.escape.columnName('createdAt'); + + const roles = await context.queryRunner.query( + `SELECT ${slugColumn} as slug, ${displayNameColumn} as displayName, ${createdAtColumn} as createdAt FROM ${tableName} ORDER BY ${createdAtColumn} ASC`, + ); + + return roles; + } + + describe('Schema Migration', () => { + it('should create unique index and correctly rename all duplicate roles', async () => { + // Create migration context for schema queries + const context = createTestMigrationContext(dataSource); + + // Test Scenario 1: 3 roles with same displayName "Duplicate Name" + await insertTestRole(context, { + slug: 'test-role-oldest', + displayName: 'Duplicate Name', + createdAt: new Date('2024-01-01T00:00:00.000Z'), + }); + await insertTestRole(context, { + slug: 'test-role-middle', + displayName: 'Duplicate Name', + createdAt: new Date('2024-01-02T00:00:00.000Z'), + }); + await insertTestRole(context, { + slug: 'test-role-newest', + displayName: 'Duplicate Name', + createdAt: new Date('2024-01-03T00:00:00.000Z'), + }); + + // Test Scenario 2: 2 duplicate "Editor" roles + await insertTestRole(context, { + slug: 'editor-first', + displayName: 'Editor', + createdAt: new Date('2025-01-01T00:00:00.000Z'), + }); + await insertTestRole(context, { + slug: 'editor-second', + displayName: 'Editor', + createdAt: new Date('2025-01-02T00:00:00.000Z'), + }); + + // Test Scenario 3: 5 duplicate "Manager" roles + for (let i = 1; i <= 5; i++) { + await insertTestRole(context, { + slug: `manager-${i}`, + displayName: 'Manager', + createdAt: new Date(`2025-02-0${i}T00:00:00.000Z`), + }); + } + + // Test Scenario 4: Multiple independent duplicate groups + // Group 1: 3 "Admin" roles + await insertTestRole(context, { + slug: 'admin-1', + displayName: 'Admin', + createdAt: new Date('2025-03-01T00:00:00.000Z'), + }); + await insertTestRole(context, { + slug: 'admin-2', + displayName: 'Admin', + createdAt: new Date('2025-03-02T00:00:00.000Z'), + }); + await insertTestRole(context, { + slug: 'admin-3', + displayName: 'Admin', + createdAt: new Date('2025-03-03T00:00:00.000Z'), + }); + + // Group 2: 2 "Reviewer" roles + await insertTestRole(context, { + slug: 'reviewer-1', + displayName: 'Reviewer', + createdAt: new Date('2025-03-04T00:00:00.000Z'), + }); + await insertTestRole(context, { + slug: 'reviewer-2', + displayName: 'Reviewer', + createdAt: new Date('2025-03-05T00:00:00.000Z'), + }); + + // Check conflict with generated display name conflict + await insertTestRole(context, { + slug: 'reviewer-3', + displayName: 'Reviewer 2', + createdAt: new Date('2025-03-05T00:00:00.000Z'), + }); + + // Group 3: 1 "Viewer" role (no duplicates) + await insertTestRole(context, { + slug: 'viewer-1', + displayName: 'Viewer', + createdAt: new Date('2025-03-06T00:00:00.000Z'), + }); + + // Verify pre-migration state - all roles exist with original displayNames + const beforeRoles = await getAllRoles(context); + expect(beforeRoles.filter((r) => r.displayName === 'Duplicate Name')).toHaveLength(3); + expect(beforeRoles.filter((r) => r.displayName === 'Editor')).toHaveLength(2); + expect(beforeRoles.filter((r) => r.displayName === 'Manager')).toHaveLength(5); + expect(beforeRoles.filter((r) => r.displayName === 'Admin')).toHaveLength(3); + expect(beforeRoles.filter((r) => r.displayName === 'Reviewer')).toHaveLength(2); + expect(beforeRoles.filter((r) => r.displayName === 'Viewer')).toHaveLength(1); + + // Run the migration + await runSingleMigration(MIGRATION_NAME); + + // Release old query runner before creating new one + await context.queryRunner.release(); + + // Create fresh context after migration + const postMigrationContext = createTestMigrationContext(dataSource); + + const tableName = postMigrationContext.escape.tableName('role'); + const displayNameColumn = postMigrationContext.escape.columnName('displayName'); + const slugColumn = postMigrationContext.escape.columnName('slug'); + const indexName = postMigrationContext.escape.indexName('UniqueRoleDisplayName'); + + // Verify all duplicate roles were renamed correctly + const afterRoles = await getAllRoles(postMigrationContext); + + // Test Scenario 1: 3 "Duplicate Name" roles + const oldestRole = afterRoles.find((r) => r.slug === 'test-role-oldest'); + const middleRole = afterRoles.find((r) => r.slug === 'test-role-middle'); + const newestRole = afterRoles.find((r) => r.slug === 'test-role-newest'); + expect(oldestRole?.displayName).toBe('Duplicate Name'); // Oldest keeps original + expect(middleRole?.displayName).toBe('Duplicate Name 2'); // Second gets " 2" + expect(newestRole?.displayName).toBe('Duplicate Name 3'); // Third gets " 3" + + // Test Scenario 2: 2 "Editor" roles + const editorFirst = afterRoles.find((r) => r.slug === 'editor-first'); + const editorSecond = afterRoles.find((r) => r.slug === 'editor-second'); + expect(editorFirst?.displayName).toBe('Editor'); // Oldest keeps original + expect(editorSecond?.displayName).toBe('Editor 2'); // Second gets " 2" + + // Test Scenario 3: 5 "Manager" roles + const manager1 = afterRoles.find((r) => r.slug === 'manager-1'); + const manager2 = afterRoles.find((r) => r.slug === 'manager-2'); + const manager3 = afterRoles.find((r) => r.slug === 'manager-3'); + const manager4 = afterRoles.find((r) => r.slug === 'manager-4'); + const manager5 = afterRoles.find((r) => r.slug === 'manager-5'); + expect(manager1?.displayName).toBe('Manager'); // Oldest keeps original + expect(manager2?.displayName).toBe('Manager 2'); + expect(manager3?.displayName).toBe('Manager 3'); + expect(manager4?.displayName).toBe('Manager 4'); + expect(manager5?.displayName).toBe('Manager 5'); + + // Test Scenario 4: Multiple independent groups + const admin1 = afterRoles.find((r) => r.slug === 'admin-1'); + const admin2 = afterRoles.find((r) => r.slug === 'admin-2'); + const admin3 = afterRoles.find((r) => r.slug === 'admin-3'); + expect(admin1?.displayName).toBe('Admin'); + expect(admin2?.displayName).toBe('Admin 2'); + expect(admin3?.displayName).toBe('Admin 3'); + + const reviewer1 = afterRoles.find((r) => r.slug === 'reviewer-1'); + const reviewer2 = afterRoles.find((r) => r.slug === 'reviewer-2'); + expect(reviewer1?.displayName).toBe('Reviewer'); + expect(reviewer2?.displayName).toBe('Reviewer 3'); + + const reviewer3 = afterRoles.find((r) => r.slug === 'reviewer-3'); + expect(reviewer3?.displayName).toBe('Reviewer 2'); + + const viewer1 = afterRoles.find((r) => r.slug === 'viewer-1'); + expect(viewer1?.displayName).toBe('Viewer'); // Unchanged (no duplicates) + + // Verify unique index exists based on database type + if (postMigrationContext.isSqlite) { + const indexes = await postMigrationContext.queryRunner.query( + `PRAGMA index_list(${tableName})`, + ); + const uniqueIndex = indexes.find( + (idx: { name: string; unique: number }) => + idx.name.includes('UniqueRoleDisplayName') && idx.unique === 1, + ); + expect(uniqueIndex).toBeDefined(); + } else if (postMigrationContext.isPostgres) { + const result = await postMigrationContext.queryRunner.query( + `SELECT indexname FROM pg_indexes WHERE tablename = ${tableName} AND indexname = ${indexName}`, + ); + expect(result).toHaveLength(1); + + // Verify index is unique + const uniqueCheck = await postMigrationContext.queryRunner.query( + `SELECT i.relname as index_name, ix.indisunique + FROM pg_class t + JOIN pg_index ix ON t.oid = ix.indrelid + JOIN pg_class i ON i.oid = ix.indexrelid + WHERE t.relname = ${tableName} AND i.relname = ${indexName}`, + ); + expect(uniqueCheck[0].indisunique).toBe(true); + } else if (postMigrationContext.isMysql) { + const result = await postMigrationContext.queryRunner.query( + `SHOW INDEXES FROM ${tableName} WHERE Key_name = ${indexName}`, + ); + expect(result).toHaveLength(1); + expect(result[0].Non_unique).toBe(0); // 0 means unique + } + + // Verify index enforces uniqueness by attempting duplicate insert + await postMigrationContext.queryRunner.query( + `INSERT INTO ${tableName} (${slugColumn}, ${displayNameColumn}, ${postMigrationContext.escape.columnName('createdAt')}, ${postMigrationContext.escape.columnName('updatedAt')}, ${postMigrationContext.escape.columnName('systemRole')}, ${postMigrationContext.escape.columnName('roleType')}) VALUES (?, ?, ?, ?, ?, ?)`, + ['test-duplicate-attempt', 'Unique Test Name', new Date(), new Date(), false, 'project'], + ); + + const attemptDuplicateInsert = async () => { + return await postMigrationContext.queryRunner.query( + `INSERT INTO ${tableName} (${slugColumn}, ${displayNameColumn}, ${postMigrationContext.escape.columnName('createdAt')}, ${postMigrationContext.escape.columnName('updatedAt')}, ${postMigrationContext.escape.columnName('systemRole')}, ${postMigrationContext.escape.columnName('roleType')}) VALUES (?, ?, ?, ?, ?, ?)`, + [ + 'test-duplicate-attempt-2', + 'Unique Test Name', + new Date(), + new Date(), + false, + 'project', + ], + ); + }; + + await expect(attemptDuplicateInsert()).rejects.toThrow(); + + // Cleanup + await postMigrationContext.queryRunner.release(); + }); + + it('should remove unique index on rollback', async () => { + // NOTE: This test skips duplicate scenarios since migration already ran in previous test + // We're testing rollback functionality independently + + // Run up() migration first (already done in test set up) + // runSingleMigration checks if already executed and skips if needed + await runSingleMigration(MIGRATION_NAME); + + // Create fresh context + const upContext = createTestMigrationContext(dataSource); + + const tableName = upContext.escape.tableName('role'); + const indexName = upContext.escape.indexName('UniqueRoleDisplayName'); + + // Verify unique index exists + if (upContext.isSqlite) { + const indexes = await upContext.queryRunner.query(`PRAGMA index_list(${tableName})`); + const uniqueIndex = indexes.find( + (idx: { name: string; unique: number }) => + idx.name.includes('UniqueRoleDisplayName') && idx.unique === 1, + ); + expect(uniqueIndex).toBeDefined(); + } else if (upContext.isPostgres) { + const result = await upContext.queryRunner.query( + `SELECT indexname FROM pg_indexes WHERE tablename = ${tableName} AND indexname = ${indexName}`, + ); + expect(result).toHaveLength(1); + } else if (upContext.isMysql) { + const result = await upContext.queryRunner.query( + `SHOW INDEXES FROM ${tableName} WHERE Key_name = ${indexName}`, + ); + expect(result).toHaveLength(1); + } + + await upContext.queryRunner.release(); + + await undoLastSingleMigration(); + + // Create fresh context after rollback + const postRollbackContext = createTestMigrationContext(dataSource); + + // Verify index is removed (DB-specific queries) + if (postRollbackContext.isSqlite) { + const indexes = await postRollbackContext.queryRunner.query( + `PRAGMA index_list(${tableName})`, + ); + const uniqueIndex = indexes.find( + (idx: { name: string; unique: number }) => + idx.name.includes('UniqueRoleDisplayName') && idx.unique === 1, + ); + expect(uniqueIndex).toBeUndefined(); + } else if (postRollbackContext.isPostgres) { + const result = await postRollbackContext.queryRunner.query( + `SELECT indexname FROM pg_indexes WHERE tablename = ${tableName} AND indexname = ${indexName}`, + ); + expect(result).toHaveLength(0); + } else if (postRollbackContext.isMysql) { + const result = await postRollbackContext.queryRunner.query( + `SHOW INDEXES FROM ${tableName} WHERE Key_name = ${indexName}`, + ); + expect(result).toHaveLength(0); + } + + // Verify duplicate displayNames can be inserted again + // Insert 2 roles with same displayName to confirm duplicates allowed + const slugColumn = postRollbackContext.escape.columnName('slug'); + const displayNameColumn = postRollbackContext.escape.columnName('displayName'); + const createdAtColumn = postRollbackContext.escape.columnName('createdAt'); + const updatedAtColumn = postRollbackContext.escape.columnName('updatedAt'); + const systemRoleColumn = postRollbackContext.escape.columnName('systemRole'); + const roleTypeColumn = postRollbackContext.escape.columnName('roleType'); + + await postRollbackContext.queryRunner.query( + `INSERT INTO ${tableName} (${slugColumn}, ${displayNameColumn}, ${createdAtColumn}, ${updatedAtColumn}, ${systemRoleColumn}, ${roleTypeColumn}) VALUES (?, ?, ?, ?, ?, ?)`, + ['rollback-test-1', 'Duplicate After Rollback', new Date(), new Date(), false, 'project'], + ); + + await postRollbackContext.queryRunner.query( + `INSERT INTO ${tableName} (${slugColumn}, ${displayNameColumn}, ${createdAtColumn}, ${updatedAtColumn}, ${systemRoleColumn}, ${roleTypeColumn}) VALUES (?, ?, ?, ?, ?, ?)`, + ['rollback-test-2', 'Duplicate After Rollback', new Date(), new Date(), false, 'project'], + ); + + // Verify both roles were inserted successfully + const duplicateRoles = await postRollbackContext.queryRunner.query( + `SELECT ${slugColumn} as slug, ${displayNameColumn} as displayName FROM ${tableName} WHERE ${displayNameColumn} = ?`, + ['Duplicate After Rollback'], + ); + + expect(duplicateRoles).toHaveLength(2); + + // Cleanup + await postRollbackContext.queryRunner.release(); + }); + }); + + describe('Post-Migration Capacity', () => { + it('should accept unique displayNames after migration', async () => { + const context = createTestMigrationContext(dataSource); + + const tableName = context.escape.tableName('role'); + const slugColumn = context.escape.columnName('slug'); + const displayNameColumn = context.escape.columnName('displayName'); + const createdAtColumn = context.escape.columnName('createdAt'); + const updatedAtColumn = context.escape.columnName('updatedAt'); + const systemRoleColumn = context.escape.columnName('systemRole'); + const roleTypeColumn = context.escape.columnName('roleType'); + + // Insert role with unique displayName + await context.queryRunner.query( + `INSERT INTO ${tableName} (${slugColumn}, ${displayNameColumn}, ${createdAtColumn}, ${updatedAtColumn}, ${systemRoleColumn}, ${roleTypeColumn}) VALUES (?, ?, ?, ?, ?, ?)`, + ['unique-role-test', 'Unique Role Name', new Date(), new Date(), false, 'project'], + ); + + // Verify retrieval using SQL + const [result] = await context.queryRunner.query( + `SELECT ${slugColumn} as slug, ${displayNameColumn} as displayName FROM ${tableName} WHERE ${slugColumn} = ?`, + ['unique-role-test'], + ); + + expect(result).toBeDefined(); + expect(result.displayName).toBe('Unique Role Name'); + + // Cleanup + await context.queryRunner.release(); + }); + }); +});