From e5e7c0bf4520fd07aa5eacacf5b03a4252e18d00 Mon Sep 17 00:00:00 2001 From: Charlie Kolb Date: Fri, 19 Sep 2025 16:36:31 +0200 Subject: [PATCH] fix(core): Delete data tables on project delete (no-changelog) (#19683) Co-authored-by: Jaakko Husso --- .../data-store.service.integration.test.ts | 162 ++++++++++++++++++ .../data-table/data-store.repository.ts | 52 +++++- .../modules/data-table/data-store.service.ts | 4 + .../__tests__/project.service.ee.test.ts | 3 + .../cli/src/services/project.service.ee.ts | 27 ++- .../frontend/@n8n/i18n/src/locales/en.json | 6 +- .../Projects/ProjectDeleteDialog.test.ts | 12 +- .../Projects/ProjectDeleteDialog.vue | 18 +- .../editor-ui/src/stores/projects.store.ts | 20 ++- .../src/views/ProjectSettings.test.ts | 13 +- .../editor-ui/src/views/ProjectSettings.vue | 14 +- 11 files changed, 301 insertions(+), 30 deletions(-) diff --git a/packages/cli/src/modules/data-table/__tests__/data-store.service.integration.test.ts b/packages/cli/src/modules/data-table/__tests__/data-store.service.integration.test.ts index c1c9a6efe9a..3589af6a58f 100644 --- a/packages/cli/src/modules/data-table/__tests__/data-store.service.integration.test.ts +++ b/packages/cli/src/modules/data-table/__tests__/data-store.service.integration.test.ts @@ -2683,4 +2683,166 @@ describe('dataStore', () => { await expect(result).rejects.toThrow(DataStoreValidationError); }); }); + + describe('transferDataStoresByProjectId', () => { + it('should transfer all data stores from one project to another', async () => { + // ARRANGE + const { id: dataStoreId1, name: dataStoreName1 } = await dataStoreService.createDataStore( + project1.id, + { + name: 'dataStore1', + columns: [{ name: 'col1', type: 'string' }], + }, + ); + const { id: dataStoreId2, name: dataStoreName2 } = await dataStoreService.createDataStore( + project1.id, + { + name: 'dataStore2', + columns: [{ name: 'col1', type: 'string' }], + }, + ); + + // ACT + await dataStoreService.transferDataStoresByProjectId(project1.id, project2.id); + + // ASSERT + const dataStore1 = await dataStoreRepository.findOneByOrFail({ + id: dataStoreId1, + }); + expect(dataStore1).toBeDefined(); + expect(dataStore1.projectId).toBe(project2.id); + expect(dataStore1.name).toBe(dataStoreName1); + + const dataStore2 = await dataStoreRepository.findOneByOrFail({ + id: dataStoreId2, + }); + expect(dataStore2).toBeDefined(); + expect(dataStore2.projectId).toBe(project2.id); + expect(dataStore2.name).toBe(dataStoreName2); + }); + + it('should not affect data stores in other projects', async () => { + // ARRANGE + const { id: dataStoreId1 } = await dataStoreService.createDataStore(project1.id, { + name: 'dataStore1', + columns: [{ name: 'col1', type: 'string' }], + }); + const { id: dataStoreId2 } = await dataStoreService.createDataStore(project2.id, { + name: 'dataStore2', + columns: [{ name: 'col1', type: 'string' }], + }); + + // ACT + await dataStoreService.transferDataStoresByProjectId(project1.id, project2.id); + + // ASSERT + const dataStore1 = await dataStoreRepository.findOneByOrFail({ + id: dataStoreId1, + }); + expect(dataStore1).toBeDefined(); + expect(dataStore1.projectId).toBe(project2.id); + + const dataStore2 = await dataStoreRepository.findOneByOrFail({ + id: dataStoreId2, + project: { + id: project2.id, + }, + }); + expect(dataStore2).toBeDefined(); + expect(dataStore2.projectId).toBe(project2.id); + }); + + it('should rename data stores if name conflict occurs in target project', async () => { + // ARRANGE + const { id: dataStoreId1 } = await dataStoreService.createDataStore(project1.id, { + name: 'dataStore', + columns: [{ name: 'col1', type: 'string' }], + }); + await dataStoreService.createDataStore(project2.id, { + name: 'dataStore', + columns: [{ name: 'col1', type: 'string' }], + }); + + // ACT + await dataStoreService.transferDataStoresByProjectId(project1.id, project2.id); + + // ASSERT + const dataStore1 = await dataStoreRepository.findOneByOrFail({ + id: dataStoreId1, + }); + expect(dataStore1).toBeDefined(); + expect(dataStore1.projectId).toBe(project2.id); + expect(dataStore1.name).toBe(`dataStore (${project1.name})`); + }); + }); + + describe('deleteDataStoreByProjectId', () => { + it('should delete all data stores for a given project ID', async () => { + // ARRANGE + const { id: dataStoreId1 } = await dataStoreService.createDataStore(project1.id, { + name: 'dataStore1', + columns: [{ name: 'col1', type: 'string' }], + }); + const { id: dataStoreId2 } = await dataStoreService.createDataStore(project1.id, { + name: 'dataStore2', + columns: [{ name: 'col1', type: 'string' }], + }); + + // ACT + await dataStoreService.deleteDataStoreByProjectId(project1.id); + + // ASSERT + await expect( + dataStoreRepository.findOneByOrFail({ + id: dataStoreId1, + project: { + id: project1.id, + }, + }), + ).rejects.toThrow(); + + await expect( + dataStoreRepository.findOneByOrFail({ + id: dataStoreId2, + project: { + id: project1.id, + }, + }), + ).rejects.toThrow(); + }); + + it('should not delete data stores for other projects', async () => { + // ARRANGE + const { id: dataStoreId1 } = await dataStoreService.createDataStore(project1.id, { + name: 'dataStoreA', + columns: [{ name: 'col1', type: 'string' }], + }); + const { id: dataStoreId2 } = await dataStoreService.createDataStore(project2.id, { + name: 'dataStoreB', + columns: [{ name: 'col1', type: 'string' }], + }); + + // ACT + await dataStoreService.deleteDataStoreByProjectId(project1.id); + + // ASSERT + await expect( + dataStoreRepository.findOneByOrFail({ + id: dataStoreId1, + project: { + id: project1.id, + }, + }), + ).rejects.toThrow(); + + const dataStore2 = await dataStoreRepository.findOneByOrFail({ + id: dataStoreId2, + project: { + id: project2.id, + }, + }); + expect(dataStore2).toBeDefined(); + expect(dataStore2.id).toBe(dataStoreId2); + }); + }); }); diff --git a/packages/cli/src/modules/data-table/data-store.repository.ts b/packages/cli/src/modules/data-table/data-store.repository.ts index 5965f5a04d7..c8b2f59809e 100644 --- a/packages/cli/src/modules/data-table/data-store.repository.ts +++ b/packages/cli/src/modules/data-table/data-store.repository.ts @@ -4,6 +4,7 @@ import { type ListDataStoreQueryDto, } from '@n8n/api-types'; import { GlobalConfig } from '@n8n/config'; +import { Project } from '@n8n/db'; import { Service } from '@n8n/di'; import { DataSource, EntityManager, Repository, SelectQueryBuilder } from '@n8n/typeorm'; import { UnexpectedError } from 'n8n-workflow'; @@ -12,6 +13,7 @@ import { DataStoreRowsRepository } from './data-store-rows.repository'; import { DataStoreUserTableName, DataTablesSizeData } from './data-store.types'; import { DataTableColumn } from './data-table-column.entity'; import { DataTable } from './data-table.entity'; +import { DataStoreNameConflictError } from './errors/data-store-name-conflict.error'; import { DataStoreValidationError } from './errors/data-store-validation.error'; import { isValidColumnName, toTableId, toTableName } from './utils/sql-utils'; @@ -77,9 +79,8 @@ export class DataStoreRepository extends Repository { return createdDataStore; } - async deleteDataStore(dataStoreId: string, entityManager?: EntityManager) { - const executor = entityManager ?? this.manager; - return await executor.transaction(async (em) => { + async deleteDataStore(dataStoreId: string, tx?: EntityManager) { + const run = async (em: EntityManager) => { const queryRunner = em.queryRunner; if (!queryRunner) { throw new UnexpectedError('QueryRunner is not available'); @@ -87,8 +88,51 @@ export class DataStoreRepository extends Repository { await em.delete(DataTable, { id: dataStoreId }); await this.dataStoreRowsRepository.dropTable(dataStoreId, queryRunner); - return true; + }; + + if (tx) { + return await run(tx); + } + + return await this.manager.transaction(run); + } + + async transferDataStoreByProjectId(fromProjectId: string, toProjectId: string) { + if (fromProjectId === toProjectId) return false; + + return await this.manager.transaction(async (em) => { + const existingTables = await em.findBy(DataTable, { projectId: fromProjectId }); + + let transferred = false; + for (const existing of existingTables) { + let name = existing.name; + const hasNameClash = await em.existsBy(DataTable, { + name, + projectId: toProjectId, + }); + + if (hasNameClash) { + const project = await em.findOneByOrFail(Project, { id: fromProjectId }); + name = `${existing.name} (${project.name})`; + + const stillHasNameClash = await em.existsBy(DataTable, { + name, + projectId: toProjectId, + }); + + if (stillHasNameClash) { + throw new DataStoreNameConflictError( + `Failed to transfer data store "${existing.name}" to the target project "${toProjectId}". A data table with the same name already exists in the target project.`, + ); + } + } + + await em.update(DataTable, { id: existing.id }, { name, projectId: toProjectId }); + transferred = true; + } + + return transferred; }); } diff --git a/packages/cli/src/modules/data-table/data-store.service.ts b/packages/cli/src/modules/data-table/data-store.service.ts index ebcaf5da34b..461e1ac1081 100644 --- a/packages/cli/src/modules/data-table/data-store.service.ts +++ b/packages/cli/src/modules/data-table/data-store.service.ts @@ -66,6 +66,10 @@ export class DataStoreService { return true; } + async transferDataStoresByProjectId(fromProjectId: string, toProjectId: string) { + return await this.dataStoreRepository.transferDataStoreByProjectId(fromProjectId, toProjectId); + } + async deleteDataStoreByProjectId(projectId: string) { return await this.dataStoreRepository.deleteDataStoreByProjectId(projectId); } diff --git a/packages/cli/src/services/__tests__/project.service.ee.test.ts b/packages/cli/src/services/__tests__/project.service.ee.test.ts index e898a9cac7e..79c9c6ed385 100644 --- a/packages/cli/src/services/__tests__/project.service.ee.test.ts +++ b/packages/cli/src/services/__tests__/project.service.ee.test.ts @@ -1,4 +1,5 @@ import type { ProjectRelation } from '@n8n/api-types'; +import type { ModuleRegistry } from '@n8n/backend-common'; import type { DatabaseConfig } from '@n8n/config'; import { type Project, @@ -23,6 +24,7 @@ describe('ProjectService', () => { const roleService = mock(); const sharedCredentialsRepository = mock(); const cacheService = mock(); + const moduleRegistry = mock({ entities: [] }); const projectService = new ProjectService( mock(), projectRepository, @@ -32,6 +34,7 @@ describe('ProjectService', () => { cacheService, mock(), mock({ type: 'postgresdb' }), + moduleRegistry, ); describe('addUsersToProject', () => { diff --git a/packages/cli/src/services/project.service.ee.ts b/packages/cli/src/services/project.service.ee.ts index 8a1a4567c8b..529eeeb3215 100644 --- a/packages/cli/src/services/project.service.ee.ts +++ b/packages/cli/src/services/project.service.ee.ts @@ -1,5 +1,5 @@ import type { CreateProjectDto, ProjectType, UpdateProjectDto } from '@n8n/api-types'; -import { LicenseState } from '@n8n/backend-common'; +import { LicenseState, ModuleRegistry } from '@n8n/backend-common'; import { DatabaseConfig } from '@n8n/config'; import { UNLIMITED_LICENSE_QUOTA } from '@n8n/constants'; import type { User } from '@n8n/db'; @@ -73,6 +73,7 @@ export class ProjectService { private readonly cacheService: CacheService, private readonly licenseState: LicenseState, private readonly databaseConfig: DatabaseConfig, + private readonly moduleRegistry: ModuleRegistry, ) {} private get workflowService() { @@ -93,6 +94,12 @@ export class ProjectService { ); } + private get dataTableService() { + return import('@/modules/data-table/data-store.service').then(({ DataStoreService }) => + Container.get(DataStoreService), + ); + } + async deleteProject( user: User, projectId: string, @@ -115,11 +122,12 @@ export class ProjectService { targetProject = await this.getProjectWithScope(user, migrateToProject, [ 'credential:create', 'workflow:create', + 'dataStore:create', ]); if (!targetProject) { throw new NotFoundError( - `Could not find project to migrate to. ID: ${targetProject}. You may lack permissions to create workflow and credentials in the target project.`, + `Could not find project to migrate to. ID: ${targetProject}. You may lack permissions to create workflow, credentials or data tables in the target project.`, ); } } @@ -176,10 +184,21 @@ export class ProjectService { // 5. delete shared workflows into this project // Cascading deletes take care of this. - // 6. delete project + // 6. delete or migrate associated data tables + if (this.moduleRegistry.isActive('data-table')) { + const dataTableService = await this.dataTableService; + + if (targetProject) { + await dataTableService.transferDataStoresByProjectId(project.id, targetProject.id); + } else { + await dataTableService.deleteDataStoreByProjectId(project.id); + } + } + + // 7. delete project await this.projectRepository.remove(project); - // 7. delete project relations + // 8. delete project relations // Cascading deletes take care of this. } diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 2fd502ee728..c082a307944 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -3063,10 +3063,10 @@ "projects.settings.role.viewer": "@:_reusableBaseText.roles.viewer", "projects.settings.delete.title": "Delete \"{projectName}\" Project?", "projects.settings.delete.message": "What should we do with the project data?", - "projects.settings.delete.message.empty": "There are no workflows or credentials in this project.", - "projects.settings.delete.question.transfer.label": "Transfer its workflows and credentials to another project or user", + "projects.settings.delete.message.empty": "There are no workflows, credentials or data tables in this project.", + "projects.settings.delete.question.transfer.label": "Transfer its workflows, credentials and data tables to another project or user", "projects.settings.delete.question.transfer.title": "Project or user to transfer to", - "projects.settings.delete.question.wipe.label": "Delete its workflows and credentials", + "projects.settings.delete.question.wipe.label": "Delete its workflows, credentials and data tables", "projects.settings.delete.question.wipe.title": "Type \"delete all data\" to confirm", "projects.settings.delete.question.wipe.placeholder": "delete all data", "projects.settings.delete.confirm": "Yes, I am sure", diff --git a/packages/frontend/editor-ui/src/components/Projects/ProjectDeleteDialog.test.ts b/packages/frontend/editor-ui/src/components/Projects/ProjectDeleteDialog.test.ts index 73190fc7f83..6e9cdac745e 100644 --- a/packages/frontend/editor-ui/src/components/Projects/ProjectDeleteDialog.test.ts +++ b/packages/frontend/editor-ui/src/components/Projects/ProjectDeleteDialog.test.ts @@ -25,8 +25,12 @@ describe('ProjectDeleteDialog', () => { props: { currentProject, projects: [], - isCurrentProjectEmpty: true, modelValue: true, + resourceCounts: { + credentials: 0, + workflows: 0, + dataTables: 0, + }, }, }); @@ -43,8 +47,12 @@ describe('ProjectDeleteDialog', () => { props: { currentProject, projects: [], - isCurrentProjectEmpty: false, modelValue: true, + resourceCounts: { + credentials: 0, + workflows: 1, + dataTables: 0, + }, }, }); diff --git a/packages/frontend/editor-ui/src/components/Projects/ProjectDeleteDialog.vue b/packages/frontend/editor-ui/src/components/Projects/ProjectDeleteDialog.vue index 7313b1b922f..954992318ff 100644 --- a/packages/frontend/editor-ui/src/components/Projects/ProjectDeleteDialog.vue +++ b/packages/frontend/editor-ui/src/components/Projects/ProjectDeleteDialog.vue @@ -3,11 +3,12 @@ import { ref, computed } from 'vue'; import type { Project, ProjectListItem, ProjectSharingData } from '@/types/projects.types'; import ProjectSharing from '@/components/Projects/ProjectSharing.vue'; import { useI18n } from '@n8n/i18n'; +import type { ResourceCounts } from '@/stores/projects.store'; type Props = { currentProject: Project | null; projects: ProjectListItem[]; - isCurrentProjectEmpty: boolean; + resourceCounts: ResourceCounts; }; const props = defineProps(); @@ -21,13 +22,20 @@ const locale = useI18n(); const selectedProject = ref(null); const operation = ref<'transfer' | 'wipe' | null>(null); const wipeConfirmText = ref(''); +const hasMovableResources = computed( + () => + props.resourceCounts.credentials + + props.resourceCounts.workflows + + props.resourceCounts.dataTables > + 0, +); const isValid = computed(() => { const expectedWipeConfirmation = locale.baseText( 'projects.settings.delete.question.wipe.placeholder', ); return ( - props.isCurrentProjectEmpty || + !hasMovableResources.value || (operation.value === 'transfer' && !!selectedProject.value) || (operation.value === 'wipe' && wipeConfirmText.value === expectedWipeConfirmation) ); @@ -53,12 +61,12 @@ const onDelete = () => { interpolate: { projectName: props.currentProject?.name ?? '' }, }) " - width="500" + width="650" > - {{ + {{ locale.baseText('projects.settings.delete.message.empty') }} -
+
{{ locale.baseText('projects.settings.delete.message') }} diff --git a/packages/frontend/editor-ui/src/stores/projects.store.ts b/packages/frontend/editor-ui/src/stores/projects.store.ts index 0933f7b9b1a..503782eb7f0 100644 --- a/packages/frontend/editor-ui/src/stores/projects.store.ts +++ b/packages/frontend/editor-ui/src/stores/projects.store.ts @@ -2,6 +2,7 @@ import { defineStore } from 'pinia'; import { ref, watch, computed } from 'vue'; import { useRoute } from 'vue-router'; import { useRootStore } from '@n8n/stores/useRootStore'; +import * as dataStoreApi from '@/features/dataStore/dataStore.api'; import * as projectsApi from '@/api/projects.api'; import * as workflowsApi from '@/api/workflows'; import * as workflowsEEApi from '@/api/workflows.ee'; @@ -19,6 +20,12 @@ import { getResourcePermissions } from '@n8n/permissions'; import type { CreateProjectDto, UpdateProjectDto } from '@n8n/api-types'; import { useSourceControlStore } from '@/stores/sourceControl.store'; +export type ResourceCounts = { + credentials: number; + workflows: number; + dataTables: number; +}; + export const useProjectsStore = defineStore(STORES.PROJECTS, () => { const route = useRoute(); const rootStore = useRootStore(); @@ -193,13 +200,18 @@ export const useProjectsStore = defineStore(STORES.PROJECTS, () => { } }; - const isProjectEmpty = async (projectId: string) => { - const [credentials, workflows] = await Promise.all([ + const getResourceCounts = async (projectId: string): Promise => { + const [credentials, workflows, dataTables] = await Promise.all([ credentialsApi.getAllCredentials(rootStore.restApiContext, { projectId }), workflowsApi.getWorkflows(rootStore.restApiContext, { projectId }), + dataStoreApi.fetchDataStoresApi(rootStore.restApiContext, projectId), ]); - return credentials.length === 0 && workflows.count === 0; + return { + credentials: credentials.length, + workflows: workflows.count, + dataTables: dataTables.count, + }; }; watch( @@ -263,6 +275,6 @@ export const useProjectsStore = defineStore(STORES.PROJECTS, () => { getProjectsCount, setProjectNavActiveIdByWorkflowHomeProject, moveResourceToProject, - isProjectEmpty, + getResourceCounts, }; }); diff --git a/packages/frontend/editor-ui/src/views/ProjectSettings.test.ts b/packages/frontend/editor-ui/src/views/ProjectSettings.test.ts index 10afa9e9372..6069544f0bc 100644 --- a/packages/frontend/editor-ui/src/views/ProjectSettings.test.ts +++ b/packages/frontend/editor-ui/src/views/ProjectSettings.test.ts @@ -264,7 +264,11 @@ describe('ProjectSettings', () => { it('deletes project with transfer or wipe based on modal selection', async () => { projectsStore.deleteProject.mockResolvedValue(); - projectsStore.isProjectEmpty.mockResolvedValue(false); + projectsStore.getResourceCounts.mockResolvedValue({ + credentials: 0, + workflows: 1, + dataTables: 0, + }); const r1 = renderComponent(); const deleteButton = r1.getByTestId('project-settings-delete-button'); @@ -290,7 +294,12 @@ describe('ProjectSettings', () => { // Case 2: Empty project, wiping directly (no transfer) projectsStore.deleteProject.mockClear(); - projectsStore.isProjectEmpty.mockResolvedValue(true); + projectsStore.getResourceCounts.mockResolvedValue({ + credentials: 0, + workflows: 0, + dataTables: 0, + }); + // unmount previous instance and re-render to reset dialog internal state r1.unmount(); const r2 = renderComponent(); diff --git a/packages/frontend/editor-ui/src/views/ProjectSettings.vue b/packages/frontend/editor-ui/src/views/ProjectSettings.vue index 001afb0c346..3020104d090 100644 --- a/packages/frontend/editor-ui/src/views/ProjectSettings.vue +++ b/packages/frontend/editor-ui/src/views/ProjectSettings.vue @@ -8,7 +8,7 @@ import { useDebounceFn } from '@vueuse/core'; import { useUsersStore } from '@/stores/users.store'; import type { IUser } from '@n8n/rest-api-client/api/users'; import { useI18n } from '@n8n/i18n'; -import { useProjectsStore } from '@/stores/projects.store'; +import { type ResourceCounts, useProjectsStore } from '@/stores/projects.store'; import type { Project, ProjectRelation } from '@/types/projects.types'; import { useToast } from '@/composables/useToast'; import { VIEWS } from '@/constants'; @@ -49,7 +49,11 @@ const upgradeDialogVisible = ref(false); const isDirty = ref(false); const isValid = ref(false); -const isCurrentProjectEmpty = ref(true); +const resourceCounts = ref({ + credentials: -1, + dataTables: -1, + workflows: -1, +}); const formData = ref>({ name: '', description: '', @@ -346,9 +350,7 @@ const onDelete = async () => { await projectsStore.getAvailableProjects(); if (projectsStore.currentProjectId) { - isCurrentProjectEmpty.value = await projectsStore.isProjectEmpty( - projectsStore.currentProjectId, - ); + resourceCounts.value = await projectsStore.getResourceCounts(projectsStore.currentProjectId); } dialogVisible.value = true; @@ -607,7 +609,7 @@ onMounted(() => {