diff --git a/packages/@n8n/api-types/src/dto/packages/__tests__/import-package-request.dto.test.ts b/packages/@n8n/api-types/src/dto/packages/__tests__/import-package-request.dto.test.ts index d3c7ab45ca5..c01e8b3c28c 100644 --- a/packages/@n8n/api-types/src/dto/packages/__tests__/import-package-request.dto.test.ts +++ b/packages/@n8n/api-types/src/dto/packages/__tests__/import-package-request.dto.test.ts @@ -183,7 +183,7 @@ describe('ImportPackageRequestDto', () => { ).toBe(false); }); - it.each(['keep-existing', 'fail'] as const)( + it.each(['keep-existing', 'fail', 'overwrite'] as const)( 'accepts %s as a dataTableSchemaConflictPolicy value', (dataTableSchemaConflictPolicy) => { const result = ImportPackageRequestDto.safeParse({ diff --git a/packages/@n8n/api-types/src/dto/packages/import-package-request.dto.ts b/packages/@n8n/api-types/src/dto/packages/import-package-request.dto.ts index 5f8e27c3f1b..569261c22e8 100644 --- a/packages/@n8n/api-types/src/dto/packages/import-package-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/packages/import-package-request.dto.ts @@ -108,7 +108,10 @@ export class ImportPackageRequestDto extends Z.class({ overwriteDeletionPolicy: optionalEnum(['archive', 'hard-delete'], 'archive'), dataTableMatchingMode: optionalEnum(['by-id'], 'by-id'), dataTableMissingMode: optionalEnum(['create', 'must-preexist', 'do-nothing'], 'create'), - dataTableSchemaConflictPolicy: optionalEnum(['keep-existing', 'fail'], 'keep-existing'), + dataTableSchemaConflictPolicy: optionalEnum( + ['keep-existing', 'fail', 'overwrite'], + 'keep-existing', + ), variableMissingMode: optionalEnum( ['do-nothing', 'must-preexist', 'create-stub', 'create-with-value'], 'create-with-value', diff --git a/packages/@n8n/cli/docs/commands/package.md b/packages/@n8n/cli/docs/commands/package.md index 9ddac1800e1..c5a5b479eb8 100644 --- a/packages/@n8n/cli/docs/commands/package.md +++ b/packages/@n8n/cli/docs/commands/package.md @@ -79,8 +79,8 @@ n8n-cli package import --file=export.n8np --workflow-conflict-policy=fail --bind | `--credential-matching-mode` | How credential references are matched on the target instance: `id-only` (default, match by id), `name-and-type` (match by exact name and type), or `type-only` (match by type). For `name-and-type` and `type-only`, candidates are ranked by scope — owned by the target project, then shared into it, then global — and ties within a scope use the most recently updated credential. | | `--credential-missing-mode` | What to do when a referenced credential cannot be resolved. `create-stub` (instance default) creates empty placeholder credentials in the target project; `must-preexist` requires every referenced credential to already exist. | | `--data-table-matching-mode` | How data tables referenced by the package's workflows are matched on the target instance: `by-id` (default and only mode) matches the target-project table with the same id — imported tables keep their source id — and never falls back to name matching. | -| `--data-table-missing-mode` | What to do when a referenced data table is absent in the target project. `create` (instance default) creates it from the package schema — keeping the source id, with no rows; `must-preexist` requires it to already exist; `do-nothing` skips creation. Matched tables are always used as-is and schema-validated (all package columns present with the same name and type), even under `do-nothing`. | -| `--data-table-schema-conflict-policy` | How strictly a matched data table's schema is compared. Every package column must exist on the matched target table with the same name and type — a missing column or a type mismatch always rejects. `keep-existing` (instance default) ignores additional columns the target table has of its own; `fail` is the strict drift-detection choice and rejects those too. Neither policy alters the matched target table — package columns are never added to it. | +| `--data-table-missing-mode` | What to do when a referenced data table is absent in the target project. `create` (instance default) creates it from the package schema — keeping the source id, with no rows; `must-preexist` requires it to already exist; `do-nothing` skips creation. Under the `keep-existing` and `fail` schema conflict policies, matched tables are schema-validated (all package columns present with the same name and type), even under `do-nothing`. The import never imports table rows. Matched tables change only under `--data-table-schema-conflict-policy=overwrite`, which keeps their rows. | +| `--data-table-schema-conflict-policy` | How strictly a matched data table's schema is compared. Under `keep-existing` and `fail`, every package column must exist on the matched target table with the same name and type. A missing column or a type mismatch rejects the import. `keep-existing` (instance default) ignores additional columns the target table has of its own; `fail` is the strict drift-detection choice and rejects those too. `keep-existing` and `fail` never alter the matched target table. `overwrite` changes the matched target table to match the package and can delete data in specific columns while preserving rows. Removing, retyping, or renaming a column deletes the data in that column. The changes apply to all workflows that use the table. | | `--variable-missing-mode` | What to do when a referenced variable is absent from both the target project and global scope: `create-with-value` (instance default) creates it with the package value and reports it under `variables.created`, falling back to an empty stub under `variables.stubbed` when the package carries no value for it; `create-stub` always creates an empty value; `do-nothing` reports unresolved names without creating anything; `must-preexist` rejects the import. What happens to a variable that *does* resolve is `--variable-conflict-policy`'s job. Requires a variables-enabled license only when the import creates a variable. | | `--variable-conflict-policy` | What to do when a referenced variable resolves in the target project or global scope but the package bundles a different value for it. `keep-existing` (instance default) leaves the target value alone and reports the name under `variables.matched`; `overwrite` silently replaces the value of the existing variable at whichever scope it was found — including a global variable other projects also read — and reports the name under `variables.updated`; `fail` rejects the import. No policy touches a resolved variable when there is nothing to change: either the package bundles no value for it (values excluded at export, or an exported value that was itself empty), or the value it bundles already matches the target's. Under `overwrite`, a project package whose projects hold *different* values for a name they all resolve to one row — a global none of them shadows — is rejected: one row cannot carry both values. Requires a variables-enabled license only when the import overwrites. | | `--variable-parent-policy` | Where `create-with-value` and `create-stub` place missing variables for workflow/folder packages (`project`, the behaviour when omitted, uses the target project; `global` uses global scope). Must be omitted for project packages, which reject it with a 400 — their placement follows the package layout, so a variable bundled under a project is created in that project and one bundled at the top level is created globally. | @@ -93,6 +93,7 @@ Requires the API key to hold: - `workflow:import` — always - `workflow:delete` and `folder:delete` — when the effective folder conflict policy is `overwrite` (set directly, or inherited from `--project-conflict-policy=overwrite`) - `dataTable:create` — when the package references data tables and `--data-table-missing-mode` is `create` +- `dataTable:update` — when `--data-table-schema-conflict-policy=overwrite` changes at least one matched table - `variable:create` — when the import actually creates a variable, i.e. `--variable-missing-mode` is `create-with-value` (the default) or `create-stub` and at least one referenced variable does not already resolve. A package whose variables all resolve creates nothing and needs neither this scope nor a variables-enabled license. - `variable:update` — when the import would overwrite a variable, i.e. `--variable-conflict-policy=overwrite` and at least one resolved variable's value differs from the package's. `keep-existing` (the default) never overwrites and needs neither this scope nor a variables-enabled license. - `tag:create` — when the import would create a tag (under `--tag-missing-mode create`, the instance default; tags that match, are dropped, or belong only to skipped workflows need no scope) diff --git a/packages/@n8n/cli/src/__tests__/package-error.test.ts b/packages/@n8n/cli/src/__tests__/package-error.test.ts index 5e31826657b..01a5de26baf 100644 --- a/packages/@n8n/cli/src/__tests__/package-error.test.ts +++ b/packages/@n8n/cli/src/__tests__/package-error.test.ts @@ -43,6 +43,58 @@ describe('toPackagesError', () => { ); }); + it('explains a data table schema mismatch and what overwrite would change', () => { + const result = toPackagesError( + new ApiError(422, 'Import blocked', undefined, { + issues: [ + { + type: 'data-table-unresolved', + kind: 'schema-incompatible', + sourceId: 'dt1', + name: 'First', + missingColumns: ['BaaId'], + typeMismatches: [], + overwriteChanges: [ + { kind: 'remove-column', column: 'note', type: 'string', destructive: true }, + { kind: 'add-column', column: 'BaaId', type: 'string', destructive: false }, + ], + usedByWorkflows: ['wf1'], + }, + ], + }), + ); + + const hint = (result as ApiError).hint ?? ''; + expect(hint).toContain( + 'data table "First" (dt1) does not match the package schema (missing columns: BaaId), used by workflow(s) wf1', + ); + expect(hint).toContain( + '--data-table-schema-conflict-policy=overwrite would: remove column note (data lost), add column BaaId', + ); + }); + + it('explains a data table rename to a name another table has', () => { + const result = toPackagesError( + new ApiError(409, 'Import blocked', undefined, { + issues: [ + { + type: 'data-table-unresolved', + kind: 'name-conflict', + sourceId: 'orders1', + name: 'Sales', + currentName: 'Orders', + conflictingTableId: 'sales1', + usedByWorkflows: ['wf1'], + }, + ], + }), + ); + + expect((result as ApiError).hint ?? '').toContain( + 'data table "Orders" (orders1) cannot be renamed to "Sales": the name is also used by table sales1, used by workflow(s) wf1', + ); + }); + it('returns non-ApiError values unchanged', () => { const error = new Error('boom'); expect(toPackagesError(error)).toBe(error); diff --git a/packages/@n8n/cli/src/client.ts b/packages/@n8n/cli/src/client.ts index 6c200da100f..e2fa434a97a 100644 --- a/packages/@n8n/cli/src/client.ts +++ b/packages/@n8n/cli/src/client.ts @@ -91,7 +91,7 @@ export interface ImportPackageCounts { }; }; credentials: { matched: number; stubbed: number }; - dataTables: { matched: number; created: number }; + dataTables: { matched: number; created: number; updated: number }; variables: { matched: number; created: number; diff --git a/packages/@n8n/cli/src/commands/package/import.ts b/packages/@n8n/cli/src/commands/package/import.ts index 77cc57ec9db..3f005c386a7 100644 --- a/packages/@n8n/cli/src/commands/package/import.ts +++ b/packages/@n8n/cli/src/commands/package/import.ts @@ -86,14 +86,14 @@ export default class PackageImport extends BaseCommand { }), dataTableMissingMode: Flags.string({ description: - 'What to do when a referenced data table is absent in the target project (default on the instance: create). Matched tables are always schema-validated, even with do-nothing', + 'What to do when a referenced data table is absent in the target project (default on the instance: create). Under the keep-existing and fail schema conflict policies, matched tables are schema-validated, even with do-nothing. Matched tables change only under --data-table-schema-conflict-policy=overwrite', options: ['create', 'must-preexist', 'do-nothing'], aliases: ['data-table-missing-mode'], }), dataTableSchemaConflictPolicy: Flags.string({ description: - 'How strictly a matched target data table schema is compared: keep-existing (instance default) requires every package column but ignores additional columns the target table has of its own; fail rejects any difference. Neither policy alters the matched target table', - options: ['keep-existing', 'fail'], + 'How strictly a matched target data table schema is compared: keep-existing (instance default) requires every package column but ignores additional columns the target table has of its own; fail rejects any difference. keep-existing and fail never alter the matched target table. overwrite changes the matched target table to match the package and can delete data in specific columns while preserving rows. Removing, retyping, or renaming a column deletes the data in that column. The changes apply to all workflows that use the table. Needs dataTable:update when a table changes.', + options: ['keep-existing', 'fail', 'overwrite'], aliases: ['data-table-schema-conflict-policy'], }), variableMissingMode: Flags.string({ diff --git a/packages/@n8n/cli/src/commands/package/package-error.ts b/packages/@n8n/cli/src/commands/package/package-error.ts index e38c60baed6..617b9546994 100644 --- a/packages/@n8n/cli/src/commands/package/package-error.ts +++ b/packages/@n8n/cli/src/commands/package/package-error.ts @@ -54,8 +54,45 @@ type BlockingIssue = name?: string; missingScope?: string; usedByWorkflows: string[]; + } + | { + type: 'data-table-unresolved'; + kind: string; + sourceId?: string; + name?: string; + missingScope?: string; + missingColumns?: string[]; + typeMismatches?: Array<{ column: string }>; + extraColumns?: string[]; + overwriteChanges?: DataTableSchemaChange[]; + currentName?: string; + conflictingTableId?: string; + usedByWorkflows: string[]; }; +type DataTableSchemaChange = { destructive?: boolean } & ( + | { kind: 'add-column'; column: string } + | { kind: 'remove-column'; column: string } + | { kind: 'change-column-type'; column: string; from: string; to: string } + | { kind: 'reorder-columns' } + | { kind: 'rename-table'; from: string; to: string } +); + +function describeSchemaChange(change: DataTableSchemaChange): string { + switch (change.kind) { + case 'add-column': + return `add column ${change.column}`; + case 'remove-column': + return `remove column ${change.column}`; + case 'change-column-type': + return `change column ${change.column} from ${change.from} to ${change.to}`; + case 'reorder-columns': + return 'reorder columns'; + case 'rename-table': + return `rename table "${change.from}" to "${change.to}"`; + } +} + function formatIssue(issue: unknown): string { if (typeof issue !== 'object' || issue === null) return JSON.stringify(issue); const it = issue as Partial & Record; @@ -114,6 +151,32 @@ function formatIssue(issue: unknown): string { } return `tag "${it.name}" (${it.sourceId}) unresolved (${it.kind}), used by workflow(s) ${usedBy}`; } + if (it.type === 'data-table-unresolved') { + const usedBy = Array.isArray(it.usedByWorkflows) ? it.usedByWorkflows.join(', ') : ''; + if (it.kind === 'permission-denied') { + return `data table import requires the ${it.missingScope} scope, needed by workflow(s) ${usedBy}`; + } + if (it.kind === 'schema-incompatible') { + const reasons = [ + it.missingColumns?.length ? `missing columns: ${it.missingColumns.join(', ')}` : '', + it.typeMismatches?.length + ? `different types: ${it.typeMismatches.map(({ column }) => column).join(', ')}` + : '', + it.extraColumns?.length ? `extra columns: ${it.extraColumns.join(', ')}` : '', + ].filter(Boolean); + const changes = it.overwriteChanges?.length + ? `\n --data-table-schema-conflict-policy=overwrite would: ${it.overwriteChanges.map((change) => (change.destructive ? `${describeSchemaChange(change)} (data lost)` : describeSchemaChange(change))).join(', ')}` + : ''; + return `data table "${it.name}" (${it.sourceId}) does not match the package schema (${reasons.join('; ')}), used by workflow(s) ${usedBy}${changes}`; + } + if (it.kind === 'name-conflict') { + const action = it.currentName + ? `"${it.currentName}" (${it.sourceId}) cannot be renamed to "${it.name}"` + : `"${it.name}" (${it.sourceId}) cannot be created`; + return `data table ${action}: the name is also used by table ${it.conflictingTableId}, used by workflow(s) ${usedBy}`; + } + return `data table "${it.name}" (${it.sourceId}) unresolved (${it.kind}), used by workflow(s) ${usedBy}`; + } return JSON.stringify(issue); } diff --git a/packages/cli/src/events/__tests__/log-streaming-event-relay.test.ts b/packages/cli/src/events/__tests__/log-streaming-event-relay.test.ts index 8db6db60e5f..39e38bd08b3 100644 --- a/packages/cli/src/events/__tests__/log-streaming-event-relay.test.ts +++ b/packages/cli/src/events/__tests__/log-streaming-event-relay.test.ts @@ -113,6 +113,7 @@ describe('LogStreamingEventRelay', () => { dataTables: { matched: 0, created: 1, + updated: 0, requirements: 1, }, variables: { diff --git a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts index 542b10c0763..af7668faeee 100644 --- a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts +++ b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts @@ -3158,6 +3158,7 @@ describe('TelemetryEventRelay', () => { dataTables: { matched: 1, created: 1, + updated: 1, requirements: 2, }, variables: { @@ -3211,6 +3212,7 @@ describe('TelemetryEventRelay', () => { credentials_required: 3, data_tables_matched: 1, data_tables_created: 1, + data_tables_updated: 1, data_tables_required: 2, variables_matched: 1, variables_missing: 1, diff --git a/packages/cli/src/events/relays/telemetry.event-relay.ts b/packages/cli/src/events/relays/telemetry.event-relay.ts index 125cdcb7e12..e8f67155976 100644 --- a/packages/cli/src/events/relays/telemetry.event-relay.ts +++ b/packages/cli/src/events/relays/telemetry.event-relay.ts @@ -1375,6 +1375,7 @@ export class TelemetryEventRelay extends EventRelay { credentials_required: counts.credentials.requirements, data_tables_matched: counts.dataTables.matched, data_tables_created: counts.dataTables.created, + data_tables_updated: counts.dataTables.updated, data_tables_required: counts.dataTables.requirements, variables_matched: counts.variables.matched, variables_missing: counts.variables.missing, diff --git a/packages/cli/src/modules/data-table/__tests__/data-table-column.repository.test.ts b/packages/cli/src/modules/data-table/__tests__/data-table-column.repository.test.ts index 182c9a3e8ef..c45eb1bd499 100644 --- a/packages/cli/src/modules/data-table/__tests__/data-table-column.repository.test.ts +++ b/packages/cli/src/modules/data-table/__tests__/data-table-column.repository.test.ts @@ -1,4 +1,5 @@ import { testModules } from '@n8n/backend-test-utils'; +import type { TransactionRunner } from '@n8n/db'; import type { DataSource, EntityManager } from '@n8n/typeorm'; import type { Mock, Mocked } from 'vitest'; import { mock } from 'vitest-mock-extended'; @@ -39,7 +40,11 @@ describe('DataTableColumnRepository', () => { manager: mockEntityManager, }); - repository = new DataTableColumnRepository(mockDataSource, mockDDLService); + repository = new DataTableColumnRepository( + mockDataSource, + mockDDLService, + mock(), + ); }); describe('renameColumn', () => { diff --git a/packages/cli/src/modules/data-table/data-table-column.repository.ts b/packages/cli/src/modules/data-table/data-table-column.repository.ts index c8ac03de332..e770543ab90 100644 --- a/packages/cli/src/modules/data-table/data-table-column.repository.ts +++ b/packages/cli/src/modules/data-table/data-table-column.repository.ts @@ -1,7 +1,7 @@ import { DataTableCreateColumnSchema } from '@n8n/api-types'; -import { withTransaction } from '@n8n/db'; +import { BaseRepository, type OperationContext, TransactionRunner, withTransaction } from '@n8n/db'; import { Service } from '@n8n/di'; -import { DataSource, EntityManager, Repository } from '@n8n/typeorm'; +import { DataSource, EntityManager } from '@n8n/typeorm'; import { DATA_TABLE_SYSTEM_COLUMNS, DATA_TABLE_SYSTEM_TESTING_COLUMN, @@ -13,16 +13,18 @@ import { DataTableDDLService } from './data-table-ddl.service'; import { DataTable } from './data-table.entity'; import { DataTableColumnNameConflictError } from './errors/data-table-column-name-conflict.error'; import { DataTableColumnNotFoundError } from './errors/data-table-column-not-found.error'; +import { DataTableNotFoundError } from './errors/data-table-not-found.error'; import { DataTableSystemColumnNameConflictError } from './errors/data-table-system-column-name-conflict.error'; import { DataTableValidationError } from './errors/data-table-validation.error'; @Service() -export class DataTableColumnRepository extends Repository { +export class DataTableColumnRepository extends BaseRepository { constructor( dataSource: DataSource, private ddlService: DataTableDDLService, + transactionRunner: TransactionRunner, ) { - super(DataTableColumn, dataSource.manager); + super(DataTableColumn, dataSource.manager, transactionRunner); } /** @@ -138,6 +140,41 @@ export class DataTableColumnRepository extends Repository { }); } + /** Drops changed columns before it adds new ones. SQLite column names are case-insensitive, so `foo` must go before `Foo` is added. */ + async replaceSchema( + dataTableId: string, + projectId: string, + schema: { name: string; columns: Array> }, + ctx: OperationContext = {}, + ) { + await this.runInTransaction(ctx, async (em) => { + if (!(await em.existsBy(DataTable, { id: dataTableId, projectId }))) { + throw new DataTableNotFoundError(dataTableId); + } + + const wantedTypes = new Map(schema.columns.map(({ name, type }) => [name, type])); + const keptColumnNames = new Set(); + for (const column of await this.getColumns(dataTableId, em)) { + if (wantedTypes.get(column.name) === column.type) keptColumnNames.add(column.name); + else await this.deleteColumn(dataTableId, column, em); + } + + for (const { name, type } of schema.columns) { + if (!keptColumnNames.has(name)) await this.addColumn(dataTableId, { name, type }, em); + } + + for (const [index, { name }] of schema.columns.entries()) { + await em.update(DataTableColumn, { dataTableId, name }, { index }); + } + + await em.update( + DataTable, + { id: dataTableId, projectId }, + { name: schema.name, updatedAt: new Date() }, + ); + }); + } + async moveColumn( dataTableId: string, column: DataTableColumn, diff --git a/packages/cli/src/modules/data-table/data-table.service.ts b/packages/cli/src/modules/data-table/data-table.service.ts index 4efe1b86fdb..7dad7e7c5c5 100644 --- a/packages/cli/src/modules/data-table/data-table.service.ts +++ b/packages/cli/src/modules/data-table/data-table.service.ts @@ -284,6 +284,17 @@ export class DataTableService { return result; } + async replaceSchema( + dataTableId: string, + projectId: string, + schema: { name: string; columns: Array> }, + ) { + const table = await this.validateDataTableExists(dataTableId, projectId); + if (table.name !== schema.name) await this.validateUniqueName(schema.name, projectId); + + await this.dataTableColumnRepository.replaceSchema(dataTableId, projectId, schema); + } + async moveColumn( dataTableId: string, projectId: string, diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-data-tables.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-data-tables.integration.test.ts index ba98cc170b4..ed600977577 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/import-data-tables.integration.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-data-tables.integration.test.ts @@ -1,8 +1,8 @@ import { LicenseState, ModuleRegistry } from '@n8n/backend-common'; import { EventService } from '@n8n/backend-services'; -import { createTeamProject, testDb, testModules } from '@n8n/backend-test-utils'; +import { createTeamProject, linkUserToProject, testDb, testModules } from '@n8n/backend-test-utils'; import type { Project, User } from '@n8n/db'; -import { FolderRepository, WorkflowRepository } from '@n8n/db'; +import { FolderRepository, ProjectRepository, WorkflowRepository } from '@n8n/db'; import { Container } from '@n8n/di'; import type { INode, INodeParameterResourceLocator, Workflow } from 'n8n-workflow'; import { NodeOperationError } from 'n8n-workflow'; @@ -11,11 +11,13 @@ import { mock } from 'vitest-mock-extended'; import { ForbiddenError } from '@n8n/errors'; import type { RelayEventMap } from '@/events/maps/relay.event-map'; import { mockDataTableSizeValidator } from '@/modules/data-table/__tests__/test-helpers'; +import { DataTableDDLService } from '@/modules/data-table/data-table-ddl.service'; import { DataTableProxyService } from '@/modules/data-table/data-table-proxy.service'; import { DataTableRepository } from '@/modules/data-table/data-table.repository'; import { DataTableService } from '@/modules/data-table/data-table.service'; import { createFolder } from '@test-integration/db/folders'; -import { createOwner } from '@test-integration/db/users'; +import { createCustomRoleWithScopeSlugs } from '@test-integration/db/roles'; +import { createMember, createOwner } from '@test-integration/db/users'; import { LicenseMocker } from '@test-integration/license'; import { initNodeTypes } from '@test-integration/utils'; @@ -28,11 +30,12 @@ import { buildEntityPackageBuffer, dataTableRequirement, serializedDataTable, + serializedProject, serializedWorkflowWithDataTable, type PackageWorkflow, } from './fixtures/package-fixtures'; import { buildWorkflowReferencingDataTables } from './utils/test-builders'; -import { streamToBuffer } from './utils/tar-support'; +import { readExport, streamToBuffer } from './utils/tar-support'; let service: N8nPackagesService; let dataTableService: DataTableService; @@ -110,6 +113,13 @@ async function buildDataTablePackage( return { packageBuffer, workflows }; } +async function expectBlocked(promise: Promise, ...issues: Array>) { + await expect(promise).rejects.toMatchObject({ + message: expect.stringContaining('Import blocked'), + meta: { issues: issues.map((issue) => expect.objectContaining(issue)) }, + }); +} + async function tablesInProject(projectId: string) { return await dataTableRepository.find({ where: { projectId }, @@ -194,7 +204,12 @@ describe('workflow package import — with data tables', () => { expect(options.dataTableMatchingMode).toBe('by-id'); expect(options.dataTableMissingMode).toBe('create'); expect(options.dataTableSchemaConflictPolicy).toBe('keep-existing'); - expect(counts.dataTables).toEqual({ matched: 1, created: 1, requirements: 2 }); + expect(counts.dataTables).toEqual({ + matched: 1, + created: 1, + updated: 0, + requirements: 2, + }); } finally { emitSpy.mockRestore(); } @@ -345,13 +360,6 @@ describe('workflow package import — with data tables', () => { }); describe('blocked imports write nothing', () => { - async function expectBlocked(promise: Promise, issue: Record) { - await expect(promise).rejects.toMatchObject({ - message: expect.stringContaining('Import blocked'), - meta: { issues: [expect.objectContaining(issue)] }, - }); - } - it('blocks when a matched table is missing a package column', async () => { const existing = await dataTableService.createDataTable(project.id, { name: 'Customers', @@ -367,6 +375,9 @@ describe('workflow package import — with data tables', () => { sourceId: existing.id, missingColumns: ['signed_up_at'], typeMismatches: [], + overwriteChanges: [ + { kind: 'add-column', column: 'signed_up_at', type: 'date', destructive: false }, + ], usedByWorkflows: ['wf-0'], }); @@ -400,6 +411,9 @@ describe('workflow package import — with data tables', () => { missingColumns: [], typeMismatches: [], extraColumns: ['extra'], + overwriteChanges: [ + { kind: 'remove-column', column: 'extra', type: 'boolean', destructive: true }, + ], }, ); @@ -657,6 +671,8 @@ describe('workflow package import — with data tables', () => { }); describe('public API key scopes', () => { + afterEach(() => licenseMocker.reset()); + it('rejects a table-creating import when the API key lacks the dataTable:create scope', async () => { const { packageBuffer } = await buildDataTablePackage([serializedDataTable()]); @@ -682,6 +698,396 @@ describe('workflow package import — with data tables', () => { expect(await tablesInProject(project.id)).toHaveLength(1); }); + + it('rejects a project package that creates a table when the API key lacks the dataTable:create scope', async () => { + licenseMocker.enable('feat:projectRole:admin'); + licenseMocker.setQuota('quota:maxTeamProjects', 100); + const table = serializedDataTable(); + const packageBuffer = await buildEntityPackageBuffer({ + projects: [{ target: 'projects/p1', project: serializedProject({ id: 'P1', name: 'p1' }) }], + workflows: [ + { + target: 'projects/p1/workflows/wf-0', + workflow: serializedWorkflowWithDataTable({ + id: 'wf-0', + name: 'Workflow 0', + dataTableId: table.id, + }), + }, + ], + dataTables: [{ target: 'data-tables/dt-0', dataTable: table }], + manifestExtras: { requirements: { dataTables: [dataTableRequirement(table, ['wf-0'])] } }, + }); + + const importWithKeyScopes = async (apiKeyScopes: string[]) => + await service.importPackage( + importPackageRequest({ user: owner, packageBuffer, apiKeyScopes }), + ); + + await expect( + importWithKeyScopes(['project:create', 'project:update', 'workflow:import']), + ).rejects.toBeInstanceOf(ForbiddenError); + + expect(await Container.get(ProjectRepository).findOneBy({ id: 'P1' })).toBeNull(); + expect(await dataTableRepository.count()).toBe(0); + expect(await workflowRepository.count()).toBe(0); + + const result = await importWithKeyScopes([ + 'project:create', + 'project:update', + 'workflow:import', + 'dataTable:create', + ]); + expect(result.dataTables).toMatchObject({ created: 1 }); + }); + }); + + describe('overwrite schema conflict policy', () => { + async function importWithOverwrite(params: Omit) { + return await importPackage({ + projectId: project.id, + dataTableSchemaConflictPolicy: 'overwrite', + ...params, + }); + } + + async function columnsOf(tableId: string) { + const columns = await dataTableService.getColumns(tableId, project.id); + return columns.map(({ name, type, index }) => ({ name, type, index })); + } + + async function exportedDataTableFile(workflowId: string) { + const { stream } = await service.exportPackage({ user: owner, workflowIds: [workflowId] }); + const { entries } = await readExport(stream); + return entries.find(({ name }) => name.endsWith('/data-table.json'))!.content; + } + + async function memberWithoutDataTableUpdate() { + const member = await createMember(); + const role = await createCustomRoleWithScopeSlugs( + [ + 'project:read', + 'project:list', + 'project:update', + 'workflow:create', + 'workflow:read', + 'workflow:update', + 'workflow:import', + 'workflow:list', + 'workflow:publish', + 'folder:create', + 'folder:read', + 'folder:update', + 'folder:list', + 'credential:read', + 'credential:list', + 'dataTable:create', + 'dataTable:read', + ], + { roleType: 'project' }, + ); + await linkUserToProject(member, project, role.slug); + return member; + } + + it('exports a byte-identical data table file after an overwrite import', async () => { + const table = await dataTableService.createDataTable(project.id, { + name: 'Customers', + columns: [ + { name: 'email', type: 'string' }, + { name: 'age', type: 'number' }, + { name: 'active', type: 'boolean' }, + { name: 'signed_up_at', type: 'date' }, + ], + }); + const workflow = await buildWorkflowReferencingDataTables({ + name: 'Customers workflow', + project, + references: [{ dataTableId: table.id }], + }); + const { stream } = await service.exportPackage({ user: owner, workflowIds: [workflow.id] }); + const packageBuffer = await streamToBuffer(stream); + const packageFile = await exportedDataTableFile(workflow.id); + await workflowRepository.delete({ id: workflow.id }); + + const columnId = async (name: string) => + (await dataTableService.getColumns(table.id, project.id)).find( + (column) => column.name === name, + )!.id; + await dataTableService.updateDataTable(table.id, project.id, { name: 'Clients' }); + await dataTableService.addColumn(table.id, project.id, { name: 'extra', type: 'string' }); + await dataTableService.deleteColumn(table.id, project.id, await columnId('active')); + await dataTableService.deleteColumn(table.id, project.id, await columnId('age')); + await dataTableService.addColumn(table.id, project.id, { name: 'age', type: 'string' }); + await dataTableService.moveColumn(table.id, project.id, await columnId('signed_up_at'), { + targetIndex: 0, + }); + + const result = await importWithOverwrite({ user: owner, packageBuffer }); + + expect(result.dataTables).toEqual({ matched: 0, created: 0, updated: 1 }); + expect(await exportedDataTableFile(result.workflows[0].localId)).toEqual(packageFile); + }); + + it('adds, removes, retypes, and renames columns, and keeps data in all other columns', async () => { + const table = await dataTableService.createDataTable(project.id, { + name: 'Customers', + columns: [ + { name: 'foo', type: 'string' }, + { name: 'score', type: 'string' }, + { name: 'note', type: 'string' }, + { name: 'email', type: 'string' }, + ], + }); + await dataTableService.insertRows(table.id, project.id, [ + { foo: 'x', score: '10', note: 'first', email: 'a@example.com' }, + ]); + const packageColumns: SerializedDataTable['columns'] = [ + { name: 'email', type: 'string', index: 0 }, + { name: 'note', type: 'string', index: 1 }, + { name: 'Foo', type: 'string', index: 2 }, + { name: 'score', type: 'number', index: 3 }, + { name: 'added', type: 'boolean', index: 4 }, + ]; + const { packageBuffer } = await buildDataTablePackage([ + serializedDataTable({ id: table.id, columns: packageColumns }), + ]); + + await importWithOverwrite({ user: owner, packageBuffer }); + + expect(await columnsOf(table.id)).toEqual(packageColumns); + const { data } = await dataTableService.getManyRowsAndCount(table.id, project.id, {}); + expect(data).toHaveLength(1); + expect(data[0]).toMatchObject({ + email: 'a@example.com', + note: 'first', + Foo: null, + score: null, + added: null, + }); + expect(data[0]).not.toHaveProperty('foo'); + }); + + it('blocks a table-changing overwrite when the user cannot update data tables in the project', async () => { + const member = await memberWithoutDataTableUpdate(); + const table = await dataTableService.createDataTable(project.id, { + name: 'Customers', + columns: [{ name: 'email', type: 'string' }], + }); + const { packageBuffer } = await buildDataTablePackage([ + serializedDataTable({ id: table.id }), + ]); + + await expectBlocked(importWithOverwrite({ user: member, packageBuffer }), { + type: 'data-table-unresolved', + kind: 'permission-denied', + missingScope: 'dataTable:update', + }); + + expect(await columnsOf(table.id)).toEqual([{ name: 'email', type: 'string', index: 0 }]); + expect(await workflowRepository.count()).toBe(0); + }); + + it('does not require dataTable:update when the table already matches the package', async () => { + const member = await memberWithoutDataTableUpdate(); + const table = await dataTableService.createDataTable(project.id, { + name: 'Customers', + columns: [ + { name: 'email', type: 'string' }, + { name: 'signed_up_at', type: 'date' }, + ], + }); + const { packageBuffer } = await buildDataTablePackage([ + serializedDataTable({ id: table.id }), + ]); + + const result = await importWithOverwrite({ + user: member, + packageBuffer, + apiKeyScopes: ['workflow:import', 'dataTable:create'], + }); + + expect(result.dataTables).toEqual({ matched: 1, created: 0, updated: 0 }); + }); + + it('rejects a table-changing overwrite when the API key lacks dataTable:update', async () => { + const table = await dataTableService.createDataTable(project.id, { + name: 'Orders', + columns: [{ name: 'email', type: 'string' }], + }); + const { packageBuffer } = await buildDataTablePackage([ + serializedDataTable({ id: table.id }), + ]); + + await expect( + importWithOverwrite({ + user: owner, + packageBuffer, + apiKeyScopes: ['workflow:import', 'dataTable:create'], + }), + ).rejects.toBeInstanceOf(ForbiddenError); + + expect((await dataTableService.getOne(table.id, project.id)).name).toBe('Orders'); + expect(await columnsOf(table.id)).toEqual([{ name: 'email', type: 'string', index: 0 }]); + expect(await workflowRepository.count()).toBe(0); + + const result = await importWithOverwrite({ + user: owner, + packageBuffer, + apiKeyScopes: ['workflow:import', 'dataTable:create', 'dataTable:update'], + }); + expect(result.dataTables).toMatchObject({ updated: 1 }); + }); + + it('blocks a rename when another table in the project already has the package name', async () => { + const columns = [ + { name: 'email', type: 'string' as const }, + { name: 'signed_up_at', type: 'date' as const }, + ]; + const orders = await dataTableService.createDataTable(project.id, { + name: 'Orders', + columns, + }); + const sales = await dataTableService.createDataTable(project.id, { name: 'Sales', columns }); + + const { packageBuffer } = await buildDataTablePackage([ + serializedDataTable({ id: orders.id, name: 'Sales' }), + ]); + await expectBlocked(importWithOverwrite({ user: owner, packageBuffer }), { + type: 'data-table-unresolved', + kind: 'name-conflict', + sourceId: orders.id, + currentName: 'Orders', + name: 'Sales', + conflictingTableId: sales.id, + }); + expect((await dataTableService.getOne(orders.id, project.id)).name).toBe('Orders'); + expect(await workflowRepository.count()).toBe(0); + + const { packageBuffer: swapBuffer } = await buildDataTablePackage([ + serializedDataTable({ id: orders.id, name: 'Sales' }), + serializedDataTable({ id: sales.id, name: 'Orders' }), + ]); + await expectBlocked( + importWithOverwrite({ user: owner, packageBuffer: swapBuffer }), + { kind: 'name-conflict', sourceId: orders.id, conflictingTableId: sales.id }, + { kind: 'name-conflict', sourceId: sales.id, conflictingTableId: orders.id }, + ); + expect((await dataTableService.getOne(orders.id, project.id)).name).toBe('Orders'); + expect((await dataTableService.getOne(sales.id, project.id)).name).toBe('Sales'); + }); + + it('blocks a rename to a name that a new table in the same import uses', async () => { + const orders = await dataTableService.createDataTable(project.id, { + name: 'Orders', + columns: [ + { name: 'email', type: 'string' }, + { name: 'signed_up_at', type: 'date' }, + ], + }); + const { packageBuffer } = await buildDataTablePackage([ + serializedDataTable({ id: orders.id, name: 'Sales' }), + serializedDataTable({ id: 'newsales1', name: 'Sales' }), + ]); + + await expectBlocked( + importWithOverwrite({ user: owner, packageBuffer }), + { kind: 'name-conflict', sourceId: 'newsales1', conflictingTableId: orders.id }, + { + kind: 'name-conflict', + sourceId: orders.id, + currentName: 'Orders', + name: 'Sales', + conflictingTableId: 'newsales1', + }, + ); + + const tables = await tablesInProject(project.id); + expect(tables.map(({ id, name }) => ({ id, name }))).toEqual([ + { id: orders.id, name: 'Orders' }, + ]); + }); + + it('rolls back every change to a table when one of its changes fails', async () => { + const table = await dataTableService.createDataTable(project.id, { + name: 'Customers', + columns: [ + { name: 'email', type: 'string' }, + { name: 'extra', type: 'string' }, + ], + }); + await dataTableService.insertRows(table.id, project.id, [ + { email: 'a@example.com', extra: 'keep me' }, + ]); + const { packageBuffer } = await buildDataTablePackage([ + serializedDataTable({ + id: table.id, + columns: [ + { name: 'email', type: 'string', index: 0 }, + { name: 'first_new', type: 'string', index: 1 }, + { name: 'second_new', type: 'string', index: 2 }, + ], + }), + ]); + const ddlService = Container.get(DataTableDDLService); + const realAddColumn = ddlService.addColumn.bind(ddlService); + const addColumn = vi + .spyOn(ddlService, 'addColumn') + .mockImplementationOnce(realAddColumn) + .mockRejectedValueOnce(new Error('Column could not be added')); + + try { + await expect(importWithOverwrite({ user: owner, packageBuffer })).rejects.toThrow( + 'Column could not be added', + ); + expect(addColumn.mock.calls.map(([, column]) => column.name)).toEqual([ + 'first_new', + 'second_new', + ]); + } finally { + addColumn.mockRestore(); + } + + expect(await columnsOf(table.id)).toEqual([ + { name: 'email', type: 'string', index: 0 }, + { name: 'extra', type: 'string', index: 1 }, + ]); + const { data } = await dataTableService.getManyRowsAndCount(table.id, project.id, {}); + expect(data).toEqual([expect.objectContaining({ email: 'a@example.com', extra: 'keep me' })]); + expect(data[0]).not.toHaveProperty('first_new'); + expect(data[0]).not.toHaveProperty('second_new'); + expect(await workflowRepository.count()).toBe(0); + }); + + it('normalizes duplicate and non-contiguous column indexes by index, then name', async () => { + const table = await dataTableService.createDataTable(project.id, { + name: 'Customers', + columns: [ + { name: 'a', type: 'string' }, + { name: 'b', type: 'string' }, + { name: 'c', type: 'string' }, + ], + }); + const { packageBuffer } = await buildDataTablePackage([ + serializedDataTable({ + id: table.id, + columns: [ + { name: 'c', type: 'string', index: 1 }, + { name: 'b', type: 'string', index: 5 }, + { name: 'a', type: 'string', index: 5 }, + ], + }), + ]); + + const result = await importWithOverwrite({ user: owner, packageBuffer }); + + expect(result.dataTables).toEqual({ matched: 0, created: 0, updated: 1 }); + expect(await columnsOf(table.id)).toEqual([ + { name: 'c', type: 'string', index: 0 }, + { name: 'a', type: 'string', index: 1 }, + { name: 'b', type: 'string', index: 2 }, + ]); + }); }); describe('folder packages', () => { diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-package.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-package.integration.test.ts index 74f7a94bcff..933e6f7d6b5 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/import-package.integration.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-package.integration.test.ts @@ -1289,6 +1289,7 @@ describe('Package import event emission', () => { dataTables: { matched: 0, created: 0, + updated: 0, requirements: 0, }, variables: { @@ -1388,6 +1389,7 @@ describe('Package import event emission', () => { dataTables: { matched: 0, created: 0, + updated: 0, requirements: 0, }, variables: { @@ -1457,6 +1459,7 @@ describe('Package import event emission', () => { dataTables: { matched: 0, created: 0, + updated: 0, requirements: 0, }, variables: { @@ -1528,6 +1531,7 @@ describe('Package import event emission', () => { dataTables: { matched: 0, created: 0, + updated: 0, requirements: 0, }, variables: { diff --git a/packages/cli/src/modules/n8n-packages/engine/__tests__/import-telemetry.test.ts b/packages/cli/src/modules/n8n-packages/engine/__tests__/import-telemetry.test.ts index 5a87d05777a..833f67ce15d 100644 --- a/packages/cli/src/modules/n8n-packages/engine/__tests__/import-telemetry.test.ts +++ b/packages/cli/src/modules/n8n-packages/engine/__tests__/import-telemetry.test.ts @@ -44,7 +44,7 @@ const scope = (input: { removedFolders?: RemovedFolderSummary[]; credentialResult: CredentialApplyResult; requirements?: PackageCredentialRequirement[]; - dataTable?: { matched: number; created: number; requirements: number }; + dataTable?: { matched: number; created: number; updated?: number; requirements: number }; variables?: { matched: number; missing: number; @@ -95,7 +95,12 @@ const scope = (input: { folderSummaries: [], bindings: { workflows: new Map(), credentials: new Map() }, credentialResult: input.credentialResult, - dataTablePlan: { creations: new Array(dt.created), failures: [], matchedCount: dt.matched }, + dataTablePlan: { + creations: new Array(dt.created), + updates: new Array(dt.updated ?? 0), + failures: [], + matchedCount: dt.matched, + }, variablePlan: { matched: [ ...Array.from({ length: vars.matched }, (_, i) => `matched-var-${i}`), @@ -206,7 +211,7 @@ describe('emitPackageImportedEvent', () => { stubbed: [], }, requirements: [requirement('credA')], - dataTable: { matched: 1, created: 0, requirements: 1 }, + dataTable: { matched: 1, created: 0, updated: 1, requirements: 2 }, variables: { matched: 1, missing: 0, requirements: 1 }, tags: { matched: ['T1'], created: ['T2'], requirementIds: ['T1', 'T2'] }, }), @@ -256,7 +261,7 @@ describe('emitPackageImportedEvent', () => { workflows: { created: 1, updated: 1, skipped: 1, archived: 0, deleted: 0 }, folders: { removed: 0 }, credentials: { matched: 1, created: 1, requirements: 2 }, - dataTables: { matched: 1, created: 2, requirements: 3 }, + dataTables: { matched: 1, created: 2, updated: 1, requirements: 4 }, // scope 2's two missing requirements were created, so post-apply missing is 0; its // overwritten name matched first but is counted as updated, not matched. variables: { matched: 1, missing: 0, created: 1, stubbed: 1, updated: 1, requirements: 4 }, diff --git a/packages/cli/src/modules/n8n-packages/engine/import-gates.ts b/packages/cli/src/modules/n8n-packages/engine/import-gates.ts index 43636810a2f..ab9252a9104 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-gates.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-gates.ts @@ -2,6 +2,7 @@ import type { LicenseState } from '@n8n/backend-common'; import { ForbiddenError } from '@n8n/errors'; +import type { DataTableImportPlan } from '../entities/data-table/data-table.types'; import type { TagImportPlan } from '../entities/tag/tag.types'; import type { WorkflowImportPlan } from '../entities/workflow/workflow-import.types'; @@ -40,10 +41,25 @@ export function assertVariableWritesAllowed(options: { } /** - * Plan-derived, unlike the pre-plan data-table gate: a tag must - * never block an import that would not write it (skipped consumers, disabled - * tags, dropped conflicts), so the assert looks at what the plans actually - * create, rename, or reconcile. + * Plan-derived: requires a scope only when a plan creates or changes a table. Project packages + * have no pre-plan create gate, so this gate also covers them. + */ +export function assertDataTableWritesAllowed( + apiKeyScopes: string[] | undefined, + dataTablePlans: DataTableImportPlan[], +): void { + if (dataTablePlans.some((plan) => plan.creations.length > 0)) { + assertPackageImportApiKeyScopes(apiKeyScopes, ['dataTable:create']); + } + if (dataTablePlans.some((plan) => plan.updates.length > 0)) { + assertPackageImportApiKeyScopes(apiKeyScopes, ['dataTable:update']); + } +} + +/** + * Plan-derived: a tag must never block an import that would not write it + * (skipped consumers, disabled tags, dropped conflicts), so the assert looks + * at what the plans actually create, rename, or reconcile. */ export function assertTagWritesAllowed( apiKeyScopes: string[] | undefined, diff --git a/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts b/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts index 0a056a7c34d..c76a53f837d 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts @@ -68,7 +68,7 @@ import type { import type { PackageWorkflowRequirement } from '../spec/requirements.schema'; import { ContentImportPolicyGate, contentImportTransport } from './content-import-policy'; import { toImportBlockedError } from './import-blocked.error'; -import { assertVariableWritesAllowed } from './import-gates'; +import { assertDataTableWritesAllowed, assertVariableWritesAllowed } from './import-gates'; export interface ImportOrchestrationInput { context: ImportContext; @@ -158,6 +158,10 @@ export class ImportOrchestrator { hasCreations: creations.length > 0, hasOverwrites: overwrites.length > 0, }); + assertDataTableWritesAllowed( + options.apiKeyScopes, + plans.map((plan) => plan.dataTablePlan), + ); for (const { input, variablePlan } of plans) { if (variablePlan.creations.length > 0) { diff --git a/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts b/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts index 076dfce5828..9cafd2ea4c8 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts @@ -68,6 +68,7 @@ export function emitPackageImportedEvent( (total, plan) => total + plan.creations.length, 0, ); + const dataTablesUpdated = dataTablePlans.reduce((total, plan) => total + plan.updates.length, 0); const variableRequirements = scopes.reduce( (total, { variableRequest }) => total + (variableRequest.requirements?.length ?? 0), @@ -154,6 +155,7 @@ export function emitPackageImportedEvent( dataTables: { matched: dataTablesMatched, created: dataTablesCreated, + updated: dataTablesUpdated, requirements: dataTableRequirements, }, variables: { diff --git a/packages/cli/src/modules/n8n-packages/engine/project-package-importer.ts b/packages/cli/src/modules/n8n-packages/engine/project-package-importer.ts index 77bc7fd623f..dcd6de2fb3f 100644 --- a/packages/cli/src/modules/n8n-packages/engine/project-package-importer.ts +++ b/packages/cli/src/modules/n8n-packages/engine/project-package-importer.ts @@ -182,6 +182,7 @@ export class ProjectPackageImporter { const stubbed: string[] = []; let dataTablesMatched = 0; let dataTablesCreated = 0; + let dataTablesUpdated = 0; const variablesMatched: string[] = []; const variablesMissing: string[] = []; const variablesCreated: string[] = []; @@ -203,6 +204,7 @@ export class ProjectPackageImporter { stubbed.push(...content.credentialResult.stubbed); dataTablesMatched += content.dataTablePlan.matchedCount; dataTablesCreated += content.dataTablePlan.creations.length; + dataTablesUpdated += content.dataTablePlan.updates.length; variablesMatched.push(...content.variablePlan.matched); variablesMissing.push(...content.variablePlan.missing.map(({ name }) => name)); variablesCreated.push(...content.variableResult.created); @@ -229,7 +231,11 @@ export class ProjectPackageImporter { projects: projectSummaries, bindings: mergeBindings(...scopedBindings), credentials: { matched, stubbed }, - dataTables: { matched: dataTablesMatched, created: dataTablesCreated }, + dataTables: { + matched: dataTablesMatched, + created: dataTablesCreated, + updated: dataTablesUpdated, + }, variables: reconcileVariableSummary({ matched: variablesMatched, missing: variablesMissing, diff --git a/packages/cli/src/modules/n8n-packages/engine/workflow-package-importer.ts b/packages/cli/src/modules/n8n-packages/engine/workflow-package-importer.ts index 2023dafe160..cab36b5a59e 100644 --- a/packages/cli/src/modules/n8n-packages/engine/workflow-package-importer.ts +++ b/packages/cli/src/modules/n8n-packages/engine/workflow-package-importer.ts @@ -173,6 +173,7 @@ export class WorkflowPackageImporter { dataTables: { matched: content.dataTablePlan.matchedCount, created: content.dataTablePlan.creations.length, + updated: content.dataTablePlan.updates.length, }, variables: toVariableSummary(content.variablePlan, content.variableResult), tags: toTagSummary(content.tagPlan), diff --git a/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-compat.test.ts b/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-compat.test.ts index f2229de3af2..f74f55f38a7 100644 --- a/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-compat.test.ts +++ b/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-compat.test.ts @@ -1,4 +1,4 @@ -import { findSchemaIncompatibility } from '../data-table-compat'; +import { diffDataTableSchema, findSchemaIncompatibility } from '../data-table-compat'; const packageColumns = [ { name: 'email', type: 'string' as const, index: 0 }, @@ -72,3 +72,87 @@ describe('findSchemaIncompatibility', () => { }); }); }); + +describe('diffDataTableSchema', () => { + const packageTable = { id: 'dt1', name: 'Customers', columns: packageColumns }; + + it('reports no changes for an identical table', () => { + expect( + diffDataTableSchema(packageTable, { + name: 'Customers', + columns: [ + { name: 'email', type: 'string', index: 0 }, + { name: 'age', type: 'number', index: 1 }, + ], + }), + ).toEqual([]); + }); + + it('detects every kind of change and lists them in a fixed order', () => { + expect( + diffDataTableSchema(packageTable, { + name: 'Orders', + columns: [ + { name: 'age', type: 'string', index: 0 }, + { name: 'extra', type: 'boolean', index: 1 }, + ], + }), + ).toEqual([ + { kind: 'remove-column', column: 'extra', type: 'boolean', destructive: true }, + { + kind: 'change-column-type', + column: 'age', + from: 'string', + to: 'number', + destructive: true, + }, + { kind: 'add-column', column: 'email', type: 'string', destructive: false }, + { kind: 'reorder-columns', destructive: false }, + { kind: 'rename-table', from: 'Orders', to: 'Customers', destructive: false }, + ]); + }); + + it('treats a column that differs only in case as removed and added', () => { + expect( + diffDataTableSchema(packageTable, { + name: 'Customers', + columns: [ + { name: 'Email', type: 'string', index: 0 }, + { name: 'age', type: 'number', index: 1 }, + ], + }), + ).toEqual([ + { kind: 'remove-column', column: 'Email', type: 'string', destructive: true }, + { kind: 'add-column', column: 'email', type: 'string', destructive: false }, + ]); + }); + + it('lists changes in column order when the target columns arrive unsorted', () => { + expect( + diffDataTableSchema(packageTable, { + name: 'Customers', + columns: [ + { name: 'second', type: 'string', index: 3 }, + { name: 'email', type: 'string', index: 0 }, + { name: 'first', type: 'string', index: 2 }, + { name: 'age', type: 'number', index: 1 }, + ], + }), + ).toEqual([ + { kind: 'remove-column', column: 'first', type: 'string', destructive: true }, + { kind: 'remove-column', column: 'second', type: 'string', destructive: true }, + ]); + }); + + it('reports a reorder when the target has a gap in its column positions', () => { + expect( + diffDataTableSchema(packageTable, { + name: 'Customers', + columns: [ + { name: 'email', type: 'string', index: 0 }, + { name: 'age', type: 'number', index: 2 }, + ], + }), + ).toEqual([{ kind: 'reorder-columns', destructive: false }]); + }); +}); diff --git a/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-importer.test.ts b/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-importer.test.ts index 75816f2a710..bb4605a6e47 100644 --- a/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-importer.test.ts +++ b/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-importer.test.ts @@ -55,7 +55,7 @@ describe('DataTableImporter.plan', () => { const plan = await importer.plan(context, makeRequest({ requirements: undefined })); - expect(plan).toEqual({ creations: [], failures: [], matchedCount: 0 }); + expect(plan).toEqual({ creations: [], updates: [], failures: [], matchedCount: 0 }); expect(moduleRegistry.isActive).not.toHaveBeenCalled(); }); @@ -75,7 +75,9 @@ describe('DataTableImporter.plan', () => { const plan = await importer.plan(context, makeRequest()); - expect(plan.failures).toEqual([{ kind: 'permission-denied', usedByWorkflows: ['wf-1'] }]); + expect(plan.failures).toEqual([ + { kind: 'permission-denied', missingScope: 'dataTable:create', usedByWorkflows: ['wf-1'] }, + ]); expect(userHasScopesMock).toHaveBeenCalledWith(context.user, ['dataTable:create'], false, { projectId: context.projectId, }); @@ -86,7 +88,7 @@ describe('DataTableImporter.plan', () => { const plan = await importer.plan(context, makeRequest({ missingMode: 'do-nothing' })); - expect(plan).toEqual({ creations: [], failures: [], matchedCount: 0 }); + expect(plan).toEqual({ creations: [], updates: [], failures: [], matchedCount: 0 }); expect(userHasScopesMock).not.toHaveBeenCalled(); }); diff --git a/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-schema-conflict-policy.test.ts b/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-schema-conflict-policy.test.ts index f9a7ff0ea4e..26d25bc24c6 100644 --- a/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-schema-conflict-policy.test.ts +++ b/packages/cli/src/modules/n8n-packages/entities/data-table/__tests__/data-table-schema-conflict-policy.test.ts @@ -6,11 +6,11 @@ const packageColumns = [ ]; const identicalTarget = [ - { name: 'email', type: 'string' }, - { name: 'age', type: 'number' }, + { name: 'email', type: 'string', index: 0 }, + { name: 'age', type: 'number', index: 1 }, ]; -const supersetTarget = [...identicalTarget, { name: 'extra', type: 'boolean' }]; +const supersetTarget = [...identicalTarget, { name: 'extra', type: 'boolean', index: 2 }]; describe('findSchemaConflict', () => { describe('keep-existing', () => { @@ -39,8 +39,8 @@ describe('findSchemaConflict', () => { it('combines extra columns with missing and mismatched ones', () => { expect( findSchemaConflict('fail', packageColumns, [ - { name: 'age', type: 'date' }, - { name: 'extra', type: 'boolean' }, + { name: 'age', type: 'date', index: 0 }, + { name: 'extra', type: 'boolean', index: 1 }, ]), ).toEqual({ missingColumns: ['email'], @@ -49,9 +49,20 @@ describe('findSchemaConflict', () => { }); }); + it('lists extra columns in column order when the target columns arrive unsorted', () => { + expect( + findSchemaConflict('fail', packageColumns, [ + { name: 'second', type: 'string', index: 3 }, + { name: 'email', type: 'string', index: 0 }, + { name: 'first', type: 'string', index: 2 }, + { name: 'age', type: 'number', index: 1 }, + ]), + ).toEqual({ missingColumns: [], typeMismatches: [], extraColumns: ['first', 'second'] }); + }); + it('still rejects a missing column even without extras', () => { expect( - findSchemaConflict('fail', packageColumns, [{ name: 'email', type: 'string' }]), + findSchemaConflict('fail', packageColumns, [{ name: 'email', type: 'string', index: 0 }]), ).toEqual({ missingColumns: ['age'], typeMismatches: [], diff --git a/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-compat.ts b/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-compat.ts index 08b56e8b609..90f670c4ba0 100644 --- a/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-compat.ts +++ b/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-compat.ts @@ -1,5 +1,10 @@ -import type { DataTableColumnTypeMismatch } from './data-table.types'; -import type { SerializedDataTableColumn } from '../../spec/serialized/data-table.schema'; +import type { DataTableColumnType } from 'n8n-workflow'; + +import type { DataTableColumnTypeMismatch, DataTableSchemaOperation } from './data-table.types'; +import type { + SerializedDataTable, + SerializedDataTableColumn, +} from '../../spec/serialized/data-table.schema'; export interface SchemaIncompatibility { missingColumns: string[]; @@ -38,3 +43,54 @@ export function findSchemaIncompatibility( if (missingColumns.length === 0 && typeMismatches.length === 0) return null; return { missingColumns, typeMismatches }; } + +export function diffDataTableSchema( + packageTable: SerializedDataTable, + target: { + name: string; + columns: Array<{ name: string; type: DataTableColumnType; index: number }>; + }, +): DataTableSchemaOperation[] { + const packageColumnsByName = new Map(packageTable.columns.map((column) => [column.name, column])); + const targetColumns = [...target.columns].sort((a, b) => a.index - b.index); + const targetColumnsByName = new Map(targetColumns.map((column) => [column.name, column])); + const operations: DataTableSchemaOperation[] = []; + + for (const { name, type } of targetColumns) { + if (!packageColumnsByName.has(name)) { + operations.push({ kind: 'remove-column', column: name, type, destructive: true }); + } + } + for (const { name, type } of targetColumns) { + const packageType = packageColumnsByName.get(name)?.type; + if (packageType !== undefined && packageType !== type) { + operations.push({ + kind: 'change-column-type', + column: name, + from: type, + to: packageType, + destructive: true, + }); + } + } + for (const { name, type } of packageTable.columns) { + if (!targetColumnsByName.has(name)) { + operations.push({ kind: 'add-column', column: name, type, destructive: false }); + } + } + const isReordered = packageTable.columns.some((column) => { + const targetIndex = targetColumnsByName.get(column.name)?.index; + return targetIndex !== undefined && targetIndex !== column.index; + }); + if (isReordered) operations.push({ kind: 'reorder-columns', destructive: false }); + if (target.name !== packageTable.name) { + operations.push({ + kind: 'rename-table', + from: target.name, + to: packageTable.name, + destructive: false, + }); + } + + return operations; +} diff --git a/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-importer.ts b/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-importer.ts index c951485bc91..a5bd8fc07b6 100644 --- a/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-importer.ts +++ b/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-importer.ts @@ -7,6 +7,7 @@ import type { DataTable } from '@/modules/data-table/data-table.entity'; import { DataTableService } from '@/modules/data-table/data-table.service'; import { userHasScopes } from '@/permissions.ee/check-access'; +import { diffDataTableSchema } from './data-table-compat'; import { matchTargetTable } from './data-table-matching-mode'; import { decideAbsentTable } from './data-table-missing-mode'; import type { TableEffect } from './data-table-missing-mode'; @@ -16,12 +17,11 @@ import type { DataTableImportPlan, DataTableImportRequest, DataTableResolutionFailure, + DataTableSchemaOperation, + DataTableUpdate, } from './data-table.types'; -import type { - DataTableMissingMode, - DataTableSchemaConflictPolicy, - ImportContext, -} from '../../n8n-packages.types'; +import { DataTableSchemaConflictPolicy } from '../../n8n-packages.types'; +import type { DataTableMissingMode, ImportContext } from '../../n8n-packages.types'; import type { PackageDataTableRequirement } from '../../spec/requirements.schema'; import type { SerializedDataTable } from '../../spec/serialized/data-table.schema'; @@ -30,6 +30,12 @@ interface PlannedCreation { requirement: PackageDataTableRequirement; } +interface PlannedUpdate extends DataTableUpdate { + requirement: PackageDataTableRequirement; +} + +type RequirementEffect = TableEffect | { action: 'update'; operations: DataTableSchemaOperation[] }; + @Service() export class DataTableImporter { constructor( @@ -39,8 +45,8 @@ export class DataTableImporter { /** * Resolves the package's data table references against the target project. - * Read-only: matched tables are compat-checked and otherwise used as-is - * (never renamed or altered); absent tables become planned creations, + * Read-only: matched tables are compat-checked, then planned as updates under + * `overwrite` or used as-is otherwise; absent tables become planned creations, * failures, or skips per missing mode. Mirrors the credential side's `plan`. */ async plan( @@ -48,17 +54,22 @@ export class DataTableImporter { request: DataTableImportRequest, ): Promise { const requirements = request.requirements ?? []; - if (requirements.length === 0) return { creations: [], failures: [], matchedCount: 0 }; + if (requirements.length === 0) { + return { creations: [], updates: [], failures: [], matchedCount: 0 }; + } if (!this.moduleRegistry.isActive('data-table')) { return { creations: [], + updates: [], failures: [{ kind: 'module-disabled', usedByWorkflows: workflowsUsing(requirements) }], matchedCount: 0, }; } - const packageTablesById = new Map(request.packageDataTables.map((table) => [table.id, table])); + const packageTablesById = new Map( + request.packageDataTables.map((table) => [table.id, normalizeColumnIndexes(table)]), + ); const targets = await this.dataTableService.findDataTablesByIds( requirements.map(({ id }) => id), ); @@ -66,6 +77,7 @@ export class DataTableImporter { const candidates = { projectId: context.projectId, targetsById }; const creations: PlannedCreation[] = []; + const updates: PlannedUpdate[] = []; const failures: DataTableResolutionFailure[] = []; let matchedCount = 0; @@ -88,31 +100,36 @@ export class DataTableImporter { request.schemaConflictPolicy, ); if (effect.action === 'create') creations.push({ table: packageTable, requirement }); - else if (effect.action === 'fail') failures.push(effect.failure); + else if (effect.action === 'update') { + updates.push({ table: packageTable, operations: effect.operations, requirement }); + } else if (effect.action === 'fail') failures.push(effect.failure); else if (matchedTargetTable) matchedCount++; } - failures.push(...(await this.creationFailures(context, creations))); + failures.push(...(await this.writeFailures(context, creations, updates))); - return { creations: creations.map(({ table }) => table), failures, matchedCount }; + return { + creations: creations.map(({ table }) => table), + updates: updates.map(({ table, operations }) => ({ table, operations })), + failures, + matchedCount, + }; } /** - * Creates the planned tables with their package (source) ids. Matched tables - * need no action: ids are preserved, so node references already resolve. + * Creates the planned tables with their package (source) ids, and changes + * the planned updates to the package schema. Ids are preserved, so node + * references already resolve. */ async apply(context: ImportContext, plan: DataTableImportPlan): Promise { - if (plan.creations.length === 0) return; - // Defense in depth: the plan phase already reports a missing scope as a // blocking issue, but apply re-checks before writing anything. - if ( - !(await userHasScopes(context.user, ['dataTable:create'], false, { - projectId: context.projectId, - })) - ) { + if (plan.creations.length > 0 && !(await hasProjectScope(context, 'dataTable:create'))) { throw new ForbiddenError('User is missing a scope required to create a data table'); } + if (plan.updates.length > 0 && !(await hasProjectScope(context, 'dataTable:update'))) { + throw new ForbiddenError('User is missing a scope required to update a data table'); + } for (const table of plan.creations) { await this.dataTableService.createDataTable( @@ -121,45 +138,72 @@ export class DataTableImporter { table.id, ); } + + for (const { table } of plan.updates) { + await this.dataTableService.replaceSchema(table.id, context.projectId, { + name: table.name, + columns: table.columns, + }); + } } - /** Guards that only apply to tables about to be created: permission, and name uniqueness inside the target project. */ - private async creationFailures( + /** Guards that only apply to tables about to be written: permission, and name uniqueness inside the target project. */ + private async writeFailures( context: ImportContext, creations: PlannedCreation[], + updates: PlannedUpdate[], ): Promise { - if (creations.length === 0) return []; - const failures: DataTableResolutionFailure[] = []; - const canCreate = await userHasScopes(context.user, ['dataTable:create'], false, { - projectId: context.projectId, - }); - if (!canCreate) { + if (creations.length > 0 && !(await hasProjectScope(context, 'dataTable:create'))) { failures.push({ kind: 'permission-denied', + missingScope: 'dataTable:create', usedByWorkflows: workflowsUsing(creations.map(({ requirement }) => requirement)), }); } + if (updates.length > 0 && !(await hasProjectScope(context, 'dataTable:update'))) { + failures.push({ + kind: 'permission-denied', + missingScope: 'dataTable:update', + usedByWorkflows: workflowsUsing(updates.map(({ requirement }) => requirement)), + }); + } + + const nameClaims: Array = [ + ...creations, + ...updates.flatMap(({ table, requirement, operations }) => { + const rename = operations.find((operation) => operation.kind === 'rename-table'); + return rename ? [{ table, requirement, currentName: rename.from }] : []; + }), + ]; + if (nameClaims.length === 0) return failures; + const existingByName = new Map( ( await this.dataTableService.findDataTablesByNamesInProject( context.projectId, - creations.map(({ table }) => table.name), + nameClaims.map(({ table }) => table.name), ) ).map((table) => [table.name, table]), ); - const packageTablesPerName = new Map(); - for (const { table } of creations) { - packageTablesPerName.set(table.name, (packageTablesPerName.get(table.name) ?? 0) + 1); - } - for (const { table, requirement } of creations) { + for (const claim of nameClaims) { // A same-named target table here is never the match candidate (matching is // by id), and two package tables of one name cannot both land in one project. - if (existingByName.has(table.name) || packageTablesPerName.get(table.name)! > 1) { - failures.push(createFailure(requirement, 'name-conflict')); + const conflictingTableId = + existingByName.get(claim.table.name)?.id ?? + nameClaims.find((other) => other !== claim && other.table.name === claim.table.name)?.table + .id; + if (conflictingTableId) { + failures.push( + createFailure(claim.requirement, 'name-conflict', { + name: claim.table.name, + conflictingTableId, + currentName: claim.currentName, + }), + ); } } @@ -169,10 +213,11 @@ export class DataTableImporter { /** * Decides the fate of one package table reference, independent of how the - * target was matched: matched tables are compat-checked and otherwise used - * as-is; absent tables follow the missing mode, with globally unique ids - * guarding planned creations. `existingWithSameId` is the instance-wide (any - * project) occupant of the requirement's id, only consulted for creations. + * target was matched: matched tables are compat-checked, then diffed against + * the package under `overwrite` or used as-is otherwise; absent tables follow + * the missing mode, with globally unique ids guarding planned creations. + * `existingWithSameId` is the instance-wide (any project) occupant of the + * requirement's id, only consulted for creations. */ function resolveRequirement( requirement: PackageDataTableRequirement, @@ -181,7 +226,7 @@ function resolveRequirement( existingTableWithSameId: DataTable | undefined, missingMode: DataTableMissingMode, schemaConflictPolicy: DataTableSchemaConflictPolicy, -): TableEffect { +): RequirementEffect { if (matchedTargetTable) { const incompatibility = findSchemaConflict( schemaConflictPolicy, @@ -191,9 +236,16 @@ function resolveRequirement( if (incompatibility) { return { action: 'fail', - failure: createFailure(requirement, 'schema-incompatible', incompatibility), + failure: createFailure(requirement, 'schema-incompatible', { + ...incompatibility, + overwriteChanges: diffDataTableSchema(packageTable, matchedTargetTable), + }), }; } + if (schemaConflictPolicy === DataTableSchemaConflictPolicy.Overwrite) { + const operations = diffDataTableSchema(packageTable, matchedTargetTable); + if (operations.length > 0) return { action: 'update', operations }; + } // Matched: used as-is. Ids are preserved on import, so the workflow // node references already point at the matched table. return { action: 'skip' }; @@ -214,6 +266,20 @@ function resolveRequirement( return effect; } +function normalizeColumnIndexes(table: SerializedDataTable): SerializedDataTable { + const columns = [...table.columns] + .sort((a, b) => a.index - b.index || (a.name < b.name ? -1 : 1)) + .map((column, index) => ({ ...column, index })); + return { ...table, columns }; +} + +async function hasProjectScope( + context: ImportContext, + scope: 'dataTable:create' | 'dataTable:update', +): Promise { + return await userHasScopes(context.user, [scope], false, { projectId: context.projectId }); +} + /** Sorted unique workflow ids referencing the given requirements. */ function workflowsUsing(requirements: PackageDataTableRequirement[]): string[] { return [...new Set(requirements.flatMap(({ usedByWorkflows }) => usedByWorkflows))].sort(); diff --git a/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-schema-conflict-policy.ts b/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-schema-conflict-policy.ts index 44c664fe712..ea01c5bac5e 100644 --- a/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-schema-conflict-policy.ts +++ b/packages/cli/src/modules/n8n-packages/entities/data-table/data-table-schema-conflict-policy.ts @@ -3,14 +3,14 @@ import type { SchemaIncompatibility } from './data-table-compat'; import type { DataTableSchemaConflictPolicy } from '../../n8n-packages.types'; import type { SerializedDataTableColumn } from '../../spec/serialized/data-table.schema'; -type TargetColumns = Array<{ name: string; type: string }>; +type TargetColumns = Array<{ name: string; type: string; index: number }>; /** * Decides whether a matched table's schema blocks the import. `keep-existing` * accepts a target that satisfies the package schema, even when the target has * additional columns of its own; `fail` is the strict drift-detection choice - * and rejects any difference, including a harmless superset. Both are - * non-destructive — the matched target table is never altered either way. + * and rejects any difference, including a harmless superset. Neither alters + * the matched target table. `overwrite` never blocks on schema. */ /* eslint-disable @typescript-eslint/naming-convention -- API data table schema conflict policy keys */ const SCHEMA_CONFLICTS: Record< @@ -25,13 +25,15 @@ const SCHEMA_CONFLICTS: Record< const incompatibility = findSchemaIncompatibility(packageColumns, targetColumns); const packageColumnNames = new Set(packageColumns.map(({ name }) => name)); - const extraColumns = targetColumns + const extraColumns = [...targetColumns] + .sort((a, b) => a.index - b.index) .map(({ name }) => name) .filter((name) => !packageColumnNames.has(name)); if (extraColumns.length === 0) return incompatibility; return { missingColumns: [], typeMismatches: [], ...incompatibility, extraColumns }; }, + overwrite: () => null, }; /* eslint-enable @typescript-eslint/naming-convention */ diff --git a/packages/cli/src/modules/n8n-packages/entities/data-table/data-table.types.ts b/packages/cli/src/modules/n8n-packages/entities/data-table/data-table.types.ts index 6a6ad7e574c..bf632ca391d 100644 --- a/packages/cli/src/modules/n8n-packages/entities/data-table/data-table.types.ts +++ b/packages/cli/src/modules/n8n-packages/entities/data-table/data-table.types.ts @@ -1,3 +1,5 @@ +import type { DataTableColumnType } from 'n8n-workflow'; + import type { DataTableMatchingMode, DataTableMissingMode, @@ -38,6 +40,14 @@ export type DataTableResolutionFailure = { typeMismatches?: DataTableColumnTypeMismatch[]; /** For `schema-incompatible` under the `fail` policy: target columns not in the package schema. */ extraColumns?: string[]; + /** For `schema-incompatible`: the changes `overwrite` would make to the target table. */ + overwriteChanges?: DataTableSchemaOperation[]; + /** For `permission-denied`: the project scope the user lacks. */ + missingScope?: 'dataTable:create' | 'dataTable:update'; + /** For `name-conflict`: the other table that holds or claims the name. */ + conflictingTableId?: string; + /** For a rename `name-conflict`: the matched table's current name. */ + currentName?: string; usedByWorkflows: string[]; }; @@ -47,7 +57,14 @@ export function createFailure( details: Partial< Pick< DataTableResolutionFailure, - 'existingProjectId' | 'missingColumns' | 'typeMismatches' | 'extraColumns' + | 'name' + | 'existingProjectId' + | 'missingColumns' + | 'typeMismatches' + | 'extraColumns' + | 'overwriteChanges' + | 'conflictingTableId' + | 'currentName' > > = {}, ): DataTableResolutionFailure { @@ -60,6 +77,25 @@ export function createFailure( }; } +/** `destructive` operations delete the data in a column. */ +export type DataTableSchemaOperation = + | { kind: 'add-column'; column: string; type: DataTableColumnType; destructive: false } + | { kind: 'remove-column'; column: string; type: DataTableColumnType; destructive: true } + | { + kind: 'change-column-type'; + column: string; + from: DataTableColumnType; + to: DataTableColumnType; + destructive: true; + } + | { kind: 'reorder-columns'; destructive: false } + | { kind: 'rename-table'; from: string; to: string; destructive: false }; + +export interface DataTableUpdate { + table: SerializedDataTable; + operations: DataTableSchemaOperation[]; +} + export interface DataTableImportRequest { requirements: PackageDataTableRequirement[] | undefined; /** The package's `data-table.json` contents, keyed off the manifest entries. */ @@ -72,6 +108,8 @@ export interface DataTableImportRequest { export interface DataTableImportPlan { /** Tables to create in the target project, keeping their package (source) id. */ creations: SerializedDataTable[]; + /** Matched tables to change to the package schema under the `overwrite` policy. */ + updates: DataTableUpdate[]; failures: DataTableResolutionFailure[]; /** Requirements resolved to an existing compatible table, used as-is. Carried for telemetry. */ matchedCount: number; diff --git a/packages/cli/src/modules/n8n-packages/n8n-packages.service.ts b/packages/cli/src/modules/n8n-packages/n8n-packages.service.ts index fb8206ab3c5..fbde4e24b8e 100644 --- a/packages/cli/src/modules/n8n-packages/n8n-packages.service.ts +++ b/packages/cli/src/modules/n8n-packages/n8n-packages.service.ts @@ -644,7 +644,7 @@ function emptyImportResult(manifest: PackageManifest): ImportResult { projects: [], bindings: createBindings(), credentials: { matched: [], stubbed: [] }, - dataTables: { matched: 0, created: 0 }, + dataTables: { matched: 0, created: 0, updated: 0 }, variables: { matched: [], created: [], stubbed: [], updated: [], missing: [] }, tags: { matched: [], created: [], renamed: [], reconciled: [], skipped: [] }, }); diff --git a/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts b/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts index 39ffe6a1c4e..081c22686b4 100644 --- a/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts +++ b/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts @@ -135,10 +135,12 @@ export const DataTableMissingMode = { } as const; export const DataTableSchemaConflictPolicy = { - /** Accepts a matched target able that has every package column, ignoring additional columns the target table has of its own. Never alters the target table. */ + /** Accepts a matched target table that has every package column, ignoring additional columns the target table has of its own. Never alters the target table. */ KeepExisting: 'keep-existing', /** Strict drift detection: fails the import on any schema difference, including target-only columns. */ Fail: 'fail', + /** Changes a matched target table to match the package schema: renames the table, adds, removes, and retypes columns, and sets the column order. Data in removed or retyped columns is lost. */ + Overwrite: 'overwrite', } as const; export const VariableMissingMode = { @@ -410,6 +412,7 @@ export type ImportPackageEventCounts = { dataTables: { matched: number; created: number; + updated: number; requirements: number; }; variables: { @@ -681,6 +684,7 @@ export interface ImportVariableSummary { export interface ImportDataTableSummary { matched: number; created: number; + updated: number; } /** Tag names (not ids), grouped by how the import resolved them. */ diff --git a/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/paths/n8n-packages.import.yml b/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/paths/n8n-packages.import.yml index b0adee1ad93..450429e47f7 100644 --- a/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/paths/n8n-packages.import.yml +++ b/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/paths/n8n-packages.import.yml @@ -236,26 +236,36 @@ post: the target project. `create` (default) creates it from the package schema, keeping the source id, with no rows; requires the `dataTable:create` scope. `must-preexist` rejects the import. - `do-nothing` skips creation of missing tables — matched tables are - still validated for schema compatibility and can still block the - import. Matched tables are always used as-is: never renamed, - altered, or filled. + `do-nothing` skips creation of missing tables. Under the + `keep-existing` and `fail` schema conflict policies, matched tables + are still validated for schema compatibility and can still block + the import. The import never imports table rows. Matched tables + change only under `dataTableSchemaConflictPolicy=overwrite`, which + keeps their rows. dataTableSchemaConflictPolicy: type: string enum: - keep-existing - fail + - overwrite default: keep-existing - description: > + description: | How strictly a matched data table's schema is compared. `keep-existing` (default) accepts a target table that has every - package column with the same name and type, even when the target table - also has additional columns of its own. `fail` is the strict - drift-detection choice: any difference, including such - target-only columns, rejects the import. A missing package - column or a type mismatch always rejects. Both policies are - non-destructive — the matched target table is never altered, and - package columns are never added to it. + package column with the same name and type. The target table can + have extra columns. `fail` rejects any difference, including + target-only columns. Under `keep-existing` and `fail`, a missing + package column or a type mismatch rejects the import, and the + target table does not change. + + `overwrite` changes the matched target table to match the package + and can delete data in specific columns while preserving rows. + Removing, retyping, or renaming a column deletes the data in that + column. The changes apply to all workflows that use the table. A + blocked `keep-existing` or `fail` import lists them in + `overwriteChanges`. + Requires the `dataTable:update` scope only when a matched table + changes. variableMissingMode: type: string enum: @@ -378,7 +388,9 @@ post: Import blocked by at least one conflict among the issues — a workflow source-id conflict, a folder conflict (id under a different parent, id owned by another project, or a `fail`-policy match), a tag conflict - (rename drift or name collision), a variable whose bundled value + (rename drift or name collision), a data table conflict (id owned by + another project, or a name that another table in the project has), a + variable whose bundled value differs from the resolved target's under `variableConflictPolicy=fail`, or one row two projects of a package would overwrite with different values. diff --git a/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/schemas/importResult.yml b/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/schemas/importResult.yml index 46487f28a74..4ff6050c19e 100644 --- a/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/schemas/importResult.yml +++ b/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/schemas/importResult.yml @@ -275,6 +275,7 @@ properties: required: - matched - created + - updated properties: matched: type: integer @@ -282,6 +283,9 @@ properties: created: type: integer minimum: 0 + updated: + type: integer + minimum: 0 variables: type: object description: > diff --git a/packages/cli/test/integration/public-api/n8n-packages-import.test.ts b/packages/cli/test/integration/public-api/n8n-packages-import.test.ts index ab2b78d1f8b..4333f691e4b 100644 --- a/packages/cli/test/integration/public-api/n8n-packages-import.test.ts +++ b/packages/cli/test/integration/public-api/n8n-packages-import.test.ts @@ -265,6 +265,7 @@ describe('POST /n8n-packages/import', () => { dataTables: { matched: 0, created: 0, + updated: 0, }, variables: { matched: [], @@ -323,7 +324,7 @@ describe('POST /n8n-packages/import', () => { .field('missingNodeTypeMode', 'fail') .field('dataTableMatchingMode', 'by-id') .field('dataTableMissingMode', 'must-preexist') - .field('dataTableSchemaConflictPolicy', 'fail') + .field('dataTableSchemaConflictPolicy', 'overwrite') .field('variableMissingMode', 'create-with-value') .field('variableConflictPolicy', 'overwrite') .field('variableParentPolicy', 'project')