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 9af72376c35..5e0b74140c1 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 @@ -21,6 +21,7 @@ describe('ImportPackageRequestDto', () => { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'create-with-value', + variableConflictPolicy: 'keep-existing', tagMissingMode: 'create', tagConflictPolicy: 'skip', }); @@ -48,6 +49,7 @@ describe('ImportPackageRequestDto', () => { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'create-with-value', + variableConflictPolicy: 'keep-existing', tagMissingMode: 'create', tagConflictPolicy: 'skip', }); @@ -77,6 +79,7 @@ describe('ImportPackageRequestDto', () => { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'create-with-value', + variableConflictPolicy: 'keep-existing', tagMissingMode: 'create', tagConflictPolicy: 'skip', }); @@ -105,6 +108,7 @@ describe('ImportPackageRequestDto', () => { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'create-with-value', + variableConflictPolicy: 'keep-existing', tagMissingMode: 'create', tagConflictPolicy: 'skip', }); @@ -401,6 +405,7 @@ describe('ImportPackageRequestDto', () => { { field: 'dataTableMissingMode', expected: 'create' }, { field: 'dataTableSchemaConflictPolicy', expected: 'keep-existing' }, { field: 'variableMissingMode', expected: 'create-with-value' }, + { field: 'variableConflictPolicy', expected: 'keep-existing' }, { field: 'tagMissingMode', expected: 'create' }, { field: 'tagConflictPolicy', expected: 'skip' }, ] as const)('defaults $field when the value is an empty string', ({ field, expected }) => { @@ -451,6 +456,39 @@ describe('ImportPackageRequestDto', () => { }); }); + describe('variableConflictPolicy', () => { + it('defaults variableConflictPolicy to keep-existing when omitted', () => { + const result = ImportPackageRequestDto.safeParse({ workflowConflictPolicy: 'fail' }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.variableConflictPolicy).toBe('keep-existing'); + } + }); + + it.each(['keep-existing', 'overwrite', 'fail'] as const)( + 'accepts %s as a variableConflictPolicy value', + (variableConflictPolicy) => { + const result = ImportPackageRequestDto.safeParse({ + variableConflictPolicy, + workflowConflictPolicy: 'fail', + }); + expect(result.success).toBe(true); + if (result.success) { + expect(result.data.variableConflictPolicy).toBe(variableConflictPolicy); + } + }, + ); + + it('rejects unsupported variableConflictPolicy values', () => { + expect( + ImportPackageRequestDto.safeParse({ + variableConflictPolicy: 'merge-values', + workflowConflictPolicy: 'fail', + }).success, + ).toBe(false); + }); + }); + describe('tagMissingMode', () => { it('defaults tagMissingMode to create when omitted', () => { const result = ImportPackageRequestDto.safeParse({ workflowConflictPolicy: 'fail' }); 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 7f05d579b60..b9f85e62507 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 @@ -18,6 +18,7 @@ export const IMPORT_PACKAGE_REQUEST_FORM_FIELDS = [ 'dataTableMissingMode', 'dataTableSchemaConflictPolicy', 'variableMissingMode', + 'variableConflictPolicy', 'variableParentPolicy', 'tagMissingMode', 'tagConflictPolicy', @@ -108,6 +109,7 @@ export class ImportPackageRequestDto extends Z.class({ ['do-nothing', 'must-preexist', 'create-stub', 'create-with-value'], 'create-with-value', ), + variableConflictPolicy: optionalEnum(['keep-existing', 'overwrite', 'fail'], 'keep-existing'), variableParentPolicy: optionalEnumNoDefault(['project', 'global']), tagMissingMode: optionalEnum(['create', 'do-nothing'], 'create'), tagConflictPolicy: optionalEnum(['skip', 'fail', 'rename'], 'skip'), diff --git a/packages/@n8n/cli/docs/commands/package.md b/packages/@n8n/cli/docs/commands/package.md index 18cac21d58a..8bfbade0487 100644 --- a/packages/@n8n/cli/docs/commands/package.md +++ b/packages/@n8n/cli/docs/commands/package.md @@ -69,7 +69,8 @@ n8n-cli package import --file=export.n8np --workflow-conflict-policy=fail --bind | `--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. | -| `--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. Matched variables are never overwritten. Requires a variables-enabled license only when the import creates a variable. | +| `--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. | | `--tag-missing-mode` | What to do when a tag referenced by the package's workflows is absent on the target instance — tags are matched by source id, never by name. `create` (instance default) creates the tag globally with its source id and name; `do-nothing` imports the workflows without the missing tags and lists them under `tags.skipped`. | | `--tag-conflict-policy` | What to do when a referenced tag conflicts on the target instance — the same-id target tag carries a different name (rename drift), or the tag's name is held by a different tag (name collision). `skip` (instance default) imports the workflows without the conflicted tags and lists them under `tags.skipped`; `fail` rejects the import; `rename` renames a drifted target tag to the package name and reconciles a name collision by re-keying the existing tag to the package (source) id, keeping its name and taggings; a drifted tag whose package name is held by another tag still rejects the import. | @@ -80,6 +81,7 @@ Requires the API key to hold: - `workflow:import` — always - `dataTable:create` — when the package references data tables and `--data-table-missing-mode` is `create` - `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) - `tag:update` — when the import would rename or reconcile a tag (under `--tag-conflict-policy rename`). @@ -94,6 +96,10 @@ issues. Examples: - an unresolved variable under `--variable-missing-mode=must-preexist`, or a creating variable mode whose new variables would exceed the instance variable limit +- a resolved variable whose value differs from the package's under + `--variable-conflict-policy=fail`, or, under + `--variable-conflict-policy=overwrite`, one row the projects of a package + would overwrite with different values Under the default `--credential-missing-mode=create-stub`, missing credentials are stubbed instead of blocking the import. diff --git a/packages/@n8n/cli/src/__tests__/client.test.ts b/packages/@n8n/cli/src/__tests__/client.test.ts index 944841dfe4a..8f48085f273 100644 --- a/packages/@n8n/cli/src/__tests__/client.test.ts +++ b/packages/@n8n/cli/src/__tests__/client.test.ts @@ -288,6 +288,30 @@ describe('N8nClient packages', () => { ); }); + describe('variableConflictPolicy', () => { + it.each(['keep-existing', 'overwrite', 'fail'])('sends %s when provided', async (policy) => { + fetchMock.mockResolvedValue( + jsonResponse(200, { + workflows: [], + bindings: {}, + credentials: { matched: [], stubbed: [] }, + variables: { matched: [], missing: [], created: [], stubbed: [], updated: [] }, + }), + ); + + await client.importPackage( + { buffer: Buffer.from('package-bytes'), filename: 'export.n8np' }, + { + workflowConflictPolicy: 'fail', + variableConflictPolicy: policy, + }, + ); + + const form = (fetchMock.mock.calls[0] as [string, RequestInit])[1].body as FormData; + expect(form.get('variableConflictPolicy')).toBe(policy); + }); + }); + describe('variableParentPolicy', () => { it.each(['project', 'global'])('sends %s when provided', async (policy) => { fetchMock.mockResolvedValue( diff --git a/packages/@n8n/cli/src/__tests__/package-error.test.ts b/packages/@n8n/cli/src/__tests__/package-error.test.ts index acf1000a8d0..b1f2b1fea08 100644 --- a/packages/@n8n/cli/src/__tests__/package-error.test.ts +++ b/packages/@n8n/cli/src/__tests__/package-error.test.ts @@ -66,6 +66,51 @@ describe('toPackagesError', () => { expect(hint).toContain('w1, w2'); }); + it('lists variable-conflict issues for a 409, naming the scope', () => { + const result = toPackagesError( + new ApiError(409, 'Import blocked', undefined, { + issues: [ + { + type: 'variable-conflict', + name: 'API_URL', + projectId: 'p1', + usedByWorkflows: ['w1'], + }, + { type: 'variable-conflict', name: 'DB_URL', usedByWorkflows: ['w2'] }, + ], + }), + ); + + const hint = (result as ApiError).hint ?? ''; + expect(hint).toContain('variable "API_URL" in project p1 holds a different value'); + expect(hint).toContain('w1'); + expect(hint).toContain('variable "DB_URL" in the global scope holds a different value'); + expect(hint).toContain('w2'); + }); + + it('lists variable-limit-exceeded issues for a 422', () => { + const result = toPackagesError( + new ApiError(422, 'Import blocked', undefined, { + issues: [ + { + type: 'variable-limit-exceeded', + limit: 5, + remaining: 1, + requested: 3, + names: ['API_URL', 'DB_URL', 'TOKEN'], + usedByWorkflows: ['w1', 'w2'], + }, + ], + }), + ); + + const hint = (result as ApiError).hint ?? ''; + expect(hint).toContain('variable limit reached: 3 new variable(s)'); + expect(hint).toContain('API_URL, DB_URL, TOKEN'); + expect(hint).toContain('1 of 5 remaining'); + expect(hint).toContain('w1, w2'); + }); + it('lists tag-unresolved issues for a 409', () => { const result = toPackagesError( new ApiError(409, 'Import blocked', undefined, { diff --git a/packages/@n8n/cli/src/__tests__/package-import.test.ts b/packages/@n8n/cli/src/__tests__/package-import.test.ts index 5bde2ca3bb8..8d2457ba94d 100644 --- a/packages/@n8n/cli/src/__tests__/package-import.test.ts +++ b/packages/@n8n/cli/src/__tests__/package-import.test.ts @@ -26,6 +26,7 @@ interface ImportFlags { dataTableMissingMode?: string; dataTableSchemaConflictPolicy?: string; variableMissingMode?: string; + variableConflictPolicy?: string; variableParentPolicy?: string; tagMissingMode?: string; tagConflictPolicy?: string; @@ -75,6 +76,7 @@ describe('package import command', () => { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'create-with-value', + variableConflictPolicy: 'overwrite', variableParentPolicy: 'global', tagMissingMode: 'do-nothing', tagConflictPolicy: 'rename', @@ -102,6 +104,7 @@ describe('package import command', () => { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'create-with-value', + variableConflictPolicy: 'overwrite', variableParentPolicy: 'global', tagMissingMode: 'do-nothing', tagConflictPolicy: 'rename', diff --git a/packages/@n8n/cli/src/client.ts b/packages/@n8n/cli/src/client.ts index 081d70db3c5..76f270b7b75 100644 --- a/packages/@n8n/cli/src/client.ts +++ b/packages/@n8n/cli/src/client.ts @@ -24,6 +24,7 @@ export interface ImportPackageFields { dataTableMissingMode?: string; dataTableSchemaConflictPolicy?: string; variableMissingMode?: string; + variableConflictPolicy?: string; variableParentPolicy?: string; tagMissingMode?: string; tagConflictPolicy?: string; diff --git a/packages/@n8n/cli/src/commands/package/import.ts b/packages/@n8n/cli/src/commands/package/import.ts index 2a291842774..47ecb9d062c 100644 --- a/packages/@n8n/cli/src/commands/package/import.ts +++ b/packages/@n8n/cli/src/commands/package/import.ts @@ -89,6 +89,12 @@ export default class PackageImport extends BaseCommand { options: ['do-nothing', 'must-preexist', 'create-stub', 'create-with-value'], aliases: ['variable-missing-mode'], }), + variableConflictPolicy: Flags.string({ + description: + 'What to do when a referenced variable already resolves in the target project or global scope but the package bundles a different value for it (default: keep-existing). keep-existing leaves the target value alone; overwrite silently replaces the value of the existing variable at whichever scope it was found — including a global one other projects read — and needs a variables-enabled license plus variable:update (projectVariable:update for a project-scoped variable); fail rejects the import. No policy touches a resolved variable when there is nothing to change: either the package bundles no value for it (excluded at export, or an exported value that was itself empty), or the value it bundles already matches the one on the target. Under overwrite, a project package whose projects hold different values for a name they all resolve to one row is rejected: one row cannot carry both values', + options: ['keep-existing', 'overwrite', 'fail'], + aliases: ['variable-conflict-policy'], + }), variableParentPolicy: Flags.string({ description: 'Where creating variable modes 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', @@ -141,6 +147,7 @@ export default class PackageImport extends BaseCommand { dataTableMissingMode: flags.dataTableMissingMode, dataTableSchemaConflictPolicy: flags.dataTableSchemaConflictPolicy, variableMissingMode: flags.variableMissingMode, + variableConflictPolicy: flags.variableConflictPolicy, variableParentPolicy: flags.variableParentPolicy, tagMissingMode: flags.tagMissingMode, tagConflictPolicy: flags.tagConflictPolicy, diff --git a/packages/@n8n/cli/src/commands/package/package-error.ts b/packages/@n8n/cli/src/commands/package/package-error.ts index c77e899cfb3..390128625be 100644 --- a/packages/@n8n/cli/src/commands/package/package-error.ts +++ b/packages/@n8n/cli/src/commands/package/package-error.ts @@ -9,6 +9,15 @@ type BlockingIssue = } | { type: 'credential-unresolved'; kind: string; sourceId: string; usedByWorkflows: string[] } | { type: 'variable-unresolved'; name: string; usedByWorkflows: string[] } + | { type: 'variable-conflict'; name: string; projectId?: string; usedByWorkflows: string[] } + | { + type: 'variable-limit-exceeded'; + limit: number; + remaining: number; + requested: number; + names: string[]; + usedByWorkflows: string[]; + } | { type: 'missing-node-type'; nodeType: string; @@ -38,6 +47,16 @@ function formatIssue(issue: unknown): string { const usedBy = Array.isArray(it.usedByWorkflows) ? it.usedByWorkflows.join(', ') : ''; return `variable "${it.name}" unresolved, used by workflow(s) ${usedBy}`; } + if (it.type === 'variable-conflict') { + const usedBy = Array.isArray(it.usedByWorkflows) ? it.usedByWorkflows.join(', ') : ''; + const scope = it.projectId ? `project ${it.projectId}` : 'the global scope'; + return `variable "${it.name}" in ${scope} holds a different value, used by workflow(s) ${usedBy}`; + } + if (it.type === 'variable-limit-exceeded') { + const usedBy = Array.isArray(it.usedByWorkflows) ? it.usedByWorkflows.join(', ') : ''; + const names = Array.isArray(it.names) ? it.names.join(', ') : ''; + return `variable limit reached: ${it.requested} new variable(s) (${names}) with ${it.remaining} of ${it.limit} remaining, used by workflow(s) ${usedBy}`; + } if (it.type === 'missing-node-type') { const usedBy = Array.isArray(it.usedByWorkflows) ? it.usedByWorkflows.join(', ') : ''; return `node type ${it.nodeType} @ v${it.typeVersion} missing on this instance, used by workflow(s) ${usedBy}`; 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 2c40e35fee9..ff360ab3ae7 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 @@ -77,6 +77,7 @@ describe('LogStreamingEventRelay', () => { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'create-stub', + variableConflictPolicy: 'keep-existing', variableParentPolicy: 'project', tagMissingMode: 'create', tagConflictPolicy: 'skip', @@ -110,6 +111,7 @@ describe('LogStreamingEventRelay', () => { missing: 1, created: 0, stubbed: 0, + updated: 0, requirements: 1, }, tags: { @@ -147,6 +149,7 @@ describe('LogStreamingEventRelay', () => { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'create-stub', + variableConflictPolicy: 'keep-existing', variableParentPolicy: 'project', tagMissingMode: 'create', tagConflictPolicy: 'skip', 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 b8ebedb72d0..c1ef13933e5 100644 --- a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts +++ b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts @@ -2259,6 +2259,7 @@ describe('TelemetryEventRelay', () => { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'create-stub', + variableConflictPolicy: 'overwrite', variableParentPolicy: 'global', tagMissingMode: 'create', tagConflictPolicy: 'rename', @@ -2291,6 +2292,7 @@ describe('TelemetryEventRelay', () => { missing: 1, created: 3, stubbed: 2, + updated: 4, requirements: 2, }, tags: { @@ -2318,6 +2320,7 @@ describe('TelemetryEventRelay', () => { data_table_missing_mode: 'create', data_table_schema_conflict_policy: 'keep-existing', variable_missing_mode: 'create-stub', + variable_conflict_policy: 'overwrite', variable_parent_policy: 'global', tag_missing_mode: 'create', tag_conflict_policy: 'rename', @@ -2334,6 +2337,7 @@ describe('TelemetryEventRelay', () => { variables_missing: 1, variables_with_value_created: 3, variables_stubs_created: 2, + variables_updated: 4, variables_required: 2, tags_matched: 6, tags_created: 7, diff --git a/packages/cli/src/events/relays/telemetry.event-relay.ts b/packages/cli/src/events/relays/telemetry.event-relay.ts index 1cf3835ebcd..535471ed39d 100644 --- a/packages/cli/src/events/relays/telemetry.event-relay.ts +++ b/packages/cli/src/events/relays/telemetry.event-relay.ts @@ -1066,6 +1066,7 @@ export class TelemetryEventRelay extends EventRelay { data_table_missing_mode: options.dataTableMissingMode, data_table_schema_conflict_policy: options.dataTableSchemaConflictPolicy, variable_missing_mode: options.variableMissingMode, + variable_conflict_policy: options.variableConflictPolicy, variable_parent_policy: options.variableParentPolicy, tag_missing_mode: options.tagMissingMode, tag_conflict_policy: options.tagConflictPolicy, @@ -1082,6 +1083,7 @@ export class TelemetryEventRelay extends EventRelay { variables_missing: counts.variables.missing, variables_with_value_created: counts.variables.created, variables_stubs_created: counts.variables.stubbed, + variables_updated: counts.variables.updated, variables_required: counts.variables.requirements, tags_matched: counts.tags.matched, tags_created: counts.tags.created, 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 d10b69b42aa..e91689b7277 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 @@ -75,6 +75,7 @@ async function importPackage(params: ImportParams) { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'do-nothing', + variableConflictPolicy: 'keep-existing', variableParentPolicy: 'project', tagMissingMode: 'create', tagConflictPolicy: 'skip', diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-folders.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-folders.integration.test.ts index 6f6ce66c4ff..16f8050677a 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/import-folders.integration.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-folders.integration.test.ts @@ -49,6 +49,7 @@ async function importFolders(params: FolderImportParams) { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'do-nothing', + variableConflictPolicy: 'keep-existing', variableParentPolicy: 'project', tagMissingMode: 'create', tagConflictPolicy: 'skip', 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 f7dc6cbb983..f8e49f29491 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 @@ -73,6 +73,7 @@ type ImportPackageParams = Omit< | 'dataTableMissingMode' | 'dataTableSchemaConflictPolicy' | 'variableMissingMode' + | 'variableConflictPolicy' | 'variableParentPolicy' | 'tagMissingMode' | 'tagConflictPolicy' @@ -92,6 +93,7 @@ type ImportPackageParams = Omit< | 'dataTableMissingMode' | 'dataTableSchemaConflictPolicy' | 'variableMissingMode' + | 'variableConflictPolicy' | 'variableParentPolicy' | 'tagMissingMode' | 'tagConflictPolicy' @@ -111,6 +113,7 @@ async function importPackage(params: ImportPackageParams) { dataTableMissingMode: DataTableMissingMode.Create, dataTableSchemaConflictPolicy: DataTableSchemaConflictPolicy.KeepExisting, variableMissingMode: 'do-nothing', + variableConflictPolicy: 'keep-existing', variableParentPolicy: 'project', tagMissingMode: 'create', tagConflictPolicy: 'skip', @@ -1220,6 +1223,7 @@ describe('Package import event emission', () => { missing: 0, created: 0, stubbed: 0, + updated: 0, requirements: 0, }, tags: { @@ -1314,6 +1318,7 @@ describe('Package import event emission', () => { missing: 0, created: 0, stubbed: 0, + updated: 0, requirements: 0, }, tags: { @@ -1377,6 +1382,7 @@ describe('Package import event emission', () => { missing: 0, created: 0, stubbed: 0, + updated: 0, requirements: 0, }, tags: { @@ -1442,6 +1448,7 @@ describe('Package import event emission', () => { missing: 0, created: 0, stubbed: 0, + updated: 0, requirements: 0, }, tags: { diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-projects.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-projects.integration.test.ts index b61fdef2b02..642cbd691b2 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/import-projects.integration.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-projects.integration.test.ts @@ -18,6 +18,7 @@ import { } from '@n8n/db'; import type { User } from '@n8n/db'; import { Container } from '@n8n/di'; +import { pickVariableForProject } from 'n8n-workflow'; import { ActiveWorkflowManager } from '@/active-workflow-manager'; import { VariablesService } from '@/environments.ee/variables/variables.service.ee'; @@ -69,6 +70,7 @@ async function importProjects( dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'do-nothing', + variableConflictPolicy: 'keep-existing', tagMissingMode: 'create', tagConflictPolicy: 'skip', ...overrides, @@ -786,6 +788,7 @@ describe('project shell import', () => { missing: ['ABSENT_VAR'], created: [], stubbed: [], + updated: [], }); // do-nothing mode does not create variables. expect(await Container.get(VariablesRepository).count()).toBe(1); @@ -807,6 +810,7 @@ describe('project shell import', () => { missing: ['API_URL'], created: [], stubbed: [], + updated: [], }); }); }); @@ -894,6 +898,7 @@ describe('project shell import', () => { missing: [], created: [], stubbed: expect.arrayContaining(['GLOBAL_VAR', 'PROJECT_VAR']), + updated: [], }); expect(result.variables.stubbed).toHaveLength(2); @@ -925,6 +930,7 @@ describe('project shell import', () => { missing: [], created: [], stubbed: ['GLOBAL_VAR'], + updated: [], }); const created = await Container.get(VariablesRepository).find({ @@ -963,6 +969,7 @@ describe('project shell import', () => { missing: [], created: [], stubbed: ['API_URL'], + updated: [], }); const created = await Container.get(VariablesRepository).find({ relations: { project: true }, @@ -1005,6 +1012,7 @@ describe('project shell import', () => { missing: [], created: [], stubbed: ['API_URL'], + updated: [], }); const created = await Container.get(VariablesRepository).find({ relations: { project: true }, @@ -1054,6 +1062,7 @@ describe('project shell import', () => { missing: [], created: [], stubbed: ['THEIRS'], + updated: [], }); const created = await Container.get(VariablesRepository).find({ relations: { project: true }, @@ -1088,6 +1097,7 @@ describe('project shell import', () => { missing: [], created: [], stubbed: expect.arrayContaining(['API_URL', 'SHARED_URL']), + updated: [], }); expect(result.variables.stubbed).toHaveLength(2); @@ -1125,6 +1135,7 @@ describe('project shell import', () => { missing: [], created: [], stubbed: ['SHARED_URL'], + updated: [], }); const created = await Container.get(VariablesRepository).find({ relations: { project: true }, @@ -1152,6 +1163,7 @@ describe('project shell import', () => { missing: [], created: [], stubbed: ['API_URL'], + updated: [], }); const rows = await Container.get(VariablesRepository).find({ relations: { project: true }, @@ -1270,6 +1282,7 @@ describe('project shell import', () => { missing: [], created: ['API_URL'], stubbed: [], + updated: [], }); const rows = await Container.get(VariablesRepository).find({ relations: { project: true }, @@ -1331,9 +1344,218 @@ describe('project shell import', () => { missing: [], created: ['API_URL'], stubbed: ['API_URL'], + updated: [], }); }); }); + + describe('overwrite conflict policy', () => { + beforeEach(() => { + licenseMocker.enable('feat:variables'); + }); + + const sharedGlobalPackage = async (value: string) => + await twoProjectPackage({ + variables: [ + { + id: 'v1', + target: 'variables/shared_url', + variable: { name: 'SHARED_URL', type: 'string', value }, + }, + ], + requirements: [{ name: 'SHARED_URL', usedByWorkflows: ['WFA', 'WFB'] }], + }); + + const perProjectPackage = async (brie: string, stilton: string) => + await twoProjectPackage({ + variables: [ + { + id: 'v1', + target: 'projects/brie/variables/api_url', + variable: { name: 'API_URL', type: 'string', value: brie }, + }, + { + id: 'v2', + target: 'projects/stilton/variables/api_url', + variable: { name: 'API_URL', type: 'string', value: stilton }, + }, + ], + requirements: [{ name: 'API_URL', usedByWorkflows: ['WFA', 'WFB'] }], + }); + + it('reports one update when both projects resolve the same global variable', async () => { + await createVariable('SHARED_URL', 'https://existing.example.com'); + const packageBuffer = await sharedGlobalPackage('https://bundled.example.com'); + + const result = await importProjects(owner, packageBuffer, undefined, { + variableConflictPolicy: 'overwrite', + }); + + expect(result.variables).toEqual({ + matched: [], + missing: [], + created: [], + stubbed: [], + updated: ['SHARED_URL'], + }); + const rows = await Container.get(VariablesRepository).find({ + relations: { project: true }, + }); + expect(rows.map(({ key, value, project }) => ({ key, value, project }))).toEqual([ + { key: 'SHARED_URL', value: 'https://bundled.example.com', project: null }, + ]); + }); + + it('leaves a differing global value alone under keep-existing', async () => { + await createVariable('SHARED_URL', 'https://existing.example.com'); + const packageBuffer = await sharedGlobalPackage('https://bundled.example.com'); + + const result = await importProjects(owner, packageBuffer); + + expect(result.variables).toMatchObject({ matched: ['SHARED_URL'], updated: [] }); + expect((await Container.get(VariablesRepository).find())[0].value).toBe( + 'https://existing.example.com', + ); + }); + + it('blocks the whole package under fail, leaving no project behind', async () => { + await createVariable('SHARED_URL', 'https://existing.example.com'); + const packageBuffer = await sharedGlobalPackage('https://bundled.example.com'); + + const error = await importProjects(owner, packageBuffer, undefined, { + variableConflictPolicy: 'fail', + }).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(ConflictError); + expect((error as ConflictError).meta?.issues).toEqual([ + { type: 'variable-conflict', name: 'SHARED_URL', usedByWorkflows: ['WFA'] }, + { type: 'variable-conflict', name: 'SHARED_URL', usedByWorkflows: ['WFB'] }, + ]); + expect(await findProject('P1')).toBeNull(); + expect(await findProject('P2')).toBeNull(); + expect((await Container.get(VariablesRepository).find())[0].value).toBe( + 'https://existing.example.com', + ); + }); + + it('blocks when the projects disagree about the value one row would hold', async () => { + // Neither project has its own row yet, so both resolve to the global and would write it. + await createVariable('API_URL', 'https://existing.example.com'); + const packageBuffer = await perProjectPackage( + 'https://brie.example.com', + 'https://stilton.example.com', + ); + + const error = await importProjects(owner, packageBuffer, undefined, { + variableConflictPolicy: 'overwrite', + }).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(ConflictError); + // One issue per scope, as the `fail` policy reports the same collision. + expect((error as ConflictError).meta?.issues).toEqual([ + { type: 'variable-conflict', name: 'API_URL', usedByWorkflows: ['WFA'] }, + { type: 'variable-conflict', name: 'API_URL', usedByWorkflows: ['WFB'] }, + ]); + expect(await findProject('P1')).toBeNull(); + expect(await findProject('P2')).toBeNull(); + expect((await Container.get(VariablesRepository).find())[0].value).toBe( + 'https://existing.example.com', + ); + }); + + it('updates each project row on its own when both projects already own one', async () => { + const packageBuffer = await perProjectPackage( + 'https://brie.example.com', + 'https://stilton.example.com', + ); + // A project has to exist before it can hold a variable, so the first import creates them. + await importProjects(owner, packageBuffer); + for (const [sourceId, value] of [ + ['P1', 'https://old-brie.example.com'], + ['P2', 'https://old-stilton.example.com'], + ]) { + await createProjectVariable('API_URL', value, (await findProject(sourceId))!); + } + + const result = await importProjects(owner, packageBuffer, undefined, { + variableConflictPolicy: 'overwrite', + }); + + expect(result.variables).toMatchObject({ updated: ['API_URL'] }); + const rows = await Container.get(VariablesRepository).find({ + relations: { project: true }, + }); + expect( + rows.map(({ key, value, project }) => ({ key, value, scope: project?.id ?? 'global' })), + ).toEqual( + expect.arrayContaining([ + { key: 'API_URL', value: 'https://brie.example.com', scope: 'P1' }, + { key: 'API_URL', value: 'https://stilton.example.com', scope: 'P2' }, + ]), + ); + expect(rows).toHaveLength(2); + }); + + it('writes the shared row once when the projects agree on the value', async () => { + await createVariable('API_URL', 'https://existing.example.com'); + const packageBuffer = await perProjectPackage( + 'https://agreed.example.com', + 'https://agreed.example.com', + ); + const updateSpy = vi.spyOn(Container.get(VariablesService), 'update'); + const emitSpy = vi.spyOn(Container.get(EventService), 'emit'); + + try { + const result = await importProjects(owner, packageBuffer, undefined, { + variableConflictPolicy: 'overwrite', + }); + + expect(result.variables).toMatchObject({ updated: ['API_URL'] }); + // Both projects wanted the same value, so the second scope has nothing left to write. + expect(updateSpy).toHaveBeenCalledTimes(1); + const rows = await Container.get(VariablesRepository).find({ + relations: { project: true }, + }); + expect(rows.map(({ key, value, project }) => ({ key, value, project }))).toEqual([ + { key: 'API_URL', value: 'https://agreed.example.com', project: null }, + ]); + const importedEvents = emitSpy.mock.calls.filter( + ([name]) => name === 'n8n-package-imported', + ); + const payload = importedEvents[0][1] as RelayEventMap['n8n-package-imported']; + expect(payload.counts.variables).toMatchObject({ updated: 1 }); + } finally { + updateSpy.mockRestore(); + emitSpy.mockRestore(); + } + }); + + it('reuses a global the target already configured rather than shadowing it', async () => { + // Where a variable sat in the source says nothing about where it must land in the target. + await createVariable('API_URL', 'https://prod.example.com'); + const packageBuffer = await perProjectPackage( + 'https://staging-brie.example.com', + 'https://staging-stilton.example.com', + ); + + const result = await importProjects(owner, packageBuffer, undefined, { + variableMissingMode: 'create-with-value', + }); + + expect(result.variables).toMatchObject({ matched: ['API_URL'], created: [], updated: [] }); + const rows = await Container.get(VariablesRepository).find({ + relations: { project: true }, + }); + expect(rows.map(({ key, value, project }) => ({ key, value, project }))).toEqual([ + { key: 'API_URL', value: 'https://prod.example.com', project: null }, + ]); + for (const projectId of ['P1', 'P2']) { + expect(pickVariableForProject(rows, 'API_URL', projectId)?.value).toBe( + 'https://prod.example.com', + ); + } + }); + }); }); it('emits a single n8n-package-imported event aggregating every project in the package', async () => { diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-subworkflows.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-subworkflows.integration.test.ts index 2405b5f7887..d637ef8a589 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/import-subworkflows.integration.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-subworkflows.integration.test.ts @@ -17,6 +17,7 @@ import { MissingNodeTypeMode, TagConflictPolicy, TagMissingMode, + VariableConflictPolicy, VariableMissingMode, WorkflowConflictPolicy, WorkflowIdPolicy, @@ -48,6 +49,7 @@ async function importPackage(params: ImportPackageParams) { dataTableMissingMode: DataTableMissingMode.Create, dataTableSchemaConflictPolicy: DataTableSchemaConflictPolicy.KeepExisting, variableMissingMode: VariableMissingMode.DoNothing, + variableConflictPolicy: VariableConflictPolicy.KeepExisting, tagMissingMode: TagMissingMode.Create, tagConflictPolicy: TagConflictPolicy.Skip, ...params, diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-tags.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-tags.integration.test.ts index 1b6d8e12d89..3fc89650847 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-tags.integration.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-tags.integration.test.ts @@ -72,6 +72,7 @@ async function importPackage(params: ImportParams) { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'do-nothing', + variableConflictPolicy: 'keep-existing', missingNodeTypeMode: 'fail', tagMissingMode: 'create', tagConflictPolicy: 'skip', diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-variables.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-variables.integration.test.ts index e5b8129faf3..1b6059c434f 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-variables.integration.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-workflow-with-variables.integration.test.ts @@ -11,8 +11,10 @@ import { VariablesRepository, WorkflowRepository } from '@n8n/db'; import { Container } from '@n8n/di'; import { VariablesService } from '@/environments.ee/variables/variables.service.ee'; +import type { ConflictError } from '@/errors/response-errors/conflict.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { VariableCountLimitReachedError } from '@/errors/variable-count-limit-reached.error'; +import { WorkflowCreationService } from '@/workflows/workflow-creation.service'; import { createMember, createOwner } from '@test-integration/db/users'; import { createProjectVariable, createVariable } from '@test-integration/db/variables'; import { LicenseMocker } from '@test-integration/license'; @@ -72,6 +74,7 @@ async function importPackage(params: ImportParams) { dataTableMissingMode: 'create', dataTableSchemaConflictPolicy: 'keep-existing', variableMissingMode: 'do-nothing', + variableConflictPolicy: 'keep-existing', variableParentPolicy: 'project', missingNodeTypeMode: 'fail', tagMissingMode: 'create', @@ -130,6 +133,7 @@ describe('workflow package import — with variables', () => { missing: ['API_URL'], created: [], stubbed: [], + updated: [], }); expect(await variablesRepository.count()).toBe(variablesBefore); expect(await variablesInProject(targetProject.id)).toEqual([]); @@ -162,6 +166,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: [], + updated: [], }); expect(await variablesRepository.count()).toBe(variablesBefore); const targetVars = await variablesInProject(targetProject.id); @@ -194,6 +199,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: [], + updated: [], }); expect(await variablesInProject(targetProject.id)).toEqual([]); expect(await variablesRepository.count()).toBe(variablesBefore); @@ -222,7 +228,13 @@ describe('workflow package import — with variables', () => { expect(result.workflows).toHaveLength(1); expect(result.workflows[0].status).toBe('created'); - expect(result.variables).toEqual({ matched: [], missing: [], created: [], stubbed: [] }); + expect(result.variables).toEqual({ + matched: [], + missing: [], + created: [], + stubbed: [], + updated: [], + }); }); it('blocks the import and writes nothing when a referenced variable is unresolved', async () => { @@ -286,6 +298,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: [], + updated: [], }); expect(await variablesRepository.count()).toBe(variablesBefore); }); @@ -318,6 +331,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: [], + updated: [], }); expect(await variablesInProject(targetProject.id)).toEqual([]); expect(await variablesRepository.count()).toBe(variablesBefore); @@ -363,6 +377,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: ['API_URL'], + updated: [], }); // The stub (empty value) lands in the target project; the source row is untouched. const layout = await variableLayout(); @@ -401,6 +416,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: ['API_URL'], + updated: [], }); // The stub is created at the global scope — nothing lands in the target project. const layout = await variableLayout(); @@ -436,6 +452,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: ['API_URL'], + updated: [], }); const personalProject = await getPersonalProject(owner); const layout = await variableLayout(); @@ -471,6 +488,7 @@ describe('workflow package import — with variables', () => { missing: [], created: ['API_URL'], stubbed: [], + updated: [], }); const personalProject = await getPersonalProject(owner); const layout = await variableLayout(); @@ -631,6 +649,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: ['API_URL'], + updated: [], }); expect(await variablesInProject(targetProject.id)).toHaveLength(1); }); @@ -662,6 +681,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: [], + updated: [], }); // No new rows, and the target's existing value is not overwritten by an empty stub. const layout = await variableLayout(); @@ -717,7 +737,7 @@ describe('workflow package import — with variables', () => { expect(await variablesInProject(targetProject.id)).toEqual([]); }); - it('writes no workflow when the stub fails a quota the preflight had cleared', async () => { + it('fails the import, leaving the workflow, when the stub fails a quota the preflight had cleared', async () => { const owner = await createOwner(); const sourceProject = await createTeamProject('Source', owner); const targetProject = await createTeamProject('Target', owner); @@ -730,8 +750,7 @@ describe('workflow package import — with variables', () => { const packageBuffer = await exportWorkflowPackage(owner, workflow.id); const workflowsBefore = await workflowRepository.count(); - // Stands in for a concurrent writer taking the last slot after the preflight passed: - // variables are applied first precisely so this leaves nothing half-imported. + // The workflow survives the failure: an accepted trade for applying variables after it. vi.spyOn(variablesService, 'create').mockRejectedValueOnce( new VariableCountLimitReachedError('Variables limit reached'), ); @@ -745,7 +764,7 @@ describe('workflow package import — with variables', () => { }), ).rejects.toThrow('Variables limit reached'); - expect(await workflowRepository.count()).toBe(workflowsBefore); + expect(await workflowRepository.count()).toBe(workflowsBefore + 1); expect(await variablesInProject(targetProject.id)).toEqual([]); }); }); @@ -821,6 +840,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: [], + updated: [], }); }, ); @@ -846,7 +866,13 @@ describe('workflow package import — with variables', () => { }); expect(result.workflows[0].status).toBe('created'); - expect(result.variables).toEqual({ matched: [], missing: [], created: [], stubbed: [] }); + expect(result.variables).toEqual({ + matched: [], + missing: [], + created: [], + stubbed: [], + updated: [], + }); }); it('names the licence, not the API key scope, when an unlicensed key also lacks variable:create', async () => { @@ -902,6 +928,7 @@ describe('workflow package import — with variables', () => { missing: [], created: ['API_URL'], stubbed: [], + updated: [], }); expect(await variableLayout()).toEqual( expect.arrayContaining([ @@ -961,6 +988,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: ['API_URL'], + updated: [], }); expect(await variableLayout()).toEqual( expect.arrayContaining([{ key: 'API_URL', scope: targetProject.id, value: '' }]), @@ -991,6 +1019,7 @@ describe('workflow package import — with variables', () => { missing: [], created: [], stubbed: [], + updated: [], }); expect((await variablesInProject(targetProject.id))[0].value).toBe( 'https://target.example.com', @@ -1064,4 +1093,526 @@ describe('workflow package import — with variables', () => { ).resolves.toMatchObject({ variables: { missing: ['API_URL'] } }); }); }); + + describe('conflict policy', () => { + beforeEach(() => { + licenseMocker.reset(); + licenseMocker.enable('feat:variables'); + }); + + async function variableLayout() { + const rows = await variablesRepository.find({ relations: { project: true } }); + return rows.map((v) => ({ key: v.key, scope: v.project?.id ?? 'global', value: v.value })); + } + + /** + * A workflow referencing API_URL, exported from a source project holding `sourceValue`. + * Omitting values produces a package with nothing to compare against the target. + */ + async function packageReferencing( + owner: User, + sourceValue: string, + { includeVariableValues = true } = {}, + ) { + const sourceProject = await createTeamProject('Source', owner); + await createProjectVariable('API_URL', sourceValue, sourceProject); + const workflow = await buildWorkflowReferencingVariables({ + name: 'Workflow with vars', + project: sourceProject, + variableNames: ['API_URL'], + }); + return { + sourceProject, + workflow, + packageBuffer: await exportWorkflowPackage(owner, workflow.id, includeVariableValues), + }; + } + + describe('keep-existing', () => { + it('leaves a differing project-scoped value untouched and reports it as matched', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { sourceProject, packageBuffer } = await packageReferencing(owner, 'from-source'); + await createProjectVariable('API_URL', 'from-target', targetProject); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'keep-existing', + }); + + expect(result.workflows[0].status).toBe('created'); + expect(result.variables).toEqual({ + matched: ['API_URL'], + missing: [], + created: [], + stubbed: [], + updated: [], + }); + expect(await variableLayout()).toEqual( + expect.arrayContaining([ + { key: 'API_URL', scope: sourceProject.id, value: 'from-source' }, + { key: 'API_URL', scope: targetProject.id, value: 'from-target' }, + ]), + ); + }); + + it('leaves a differing global value untouched', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + await createVariable('API_URL', 'from-global'); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'keep-existing', + }); + + expect(result.variables).toMatchObject({ matched: ['API_URL'], updated: [] }); + expect(await variableLayout()).toEqual( + expect.arrayContaining([{ key: 'API_URL', scope: 'global', value: 'from-global' }]), + ); + expect(await variablesInProject(targetProject.id)).toEqual([]); + }); + }); + + describe('overwrite', () => { + it('replaces a differing project-scoped value and reports it as updated', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { sourceProject, packageBuffer } = await packageReferencing(owner, 'from-source'); + await createProjectVariable('API_URL', 'from-target', targetProject); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'overwrite', + }); + + expect(result.workflows[0].status).toBe('created'); + expect(result.variables).toEqual({ + matched: [], + missing: [], + created: [], + stubbed: [], + updated: ['API_URL'], + }); + const layout = await variableLayout(); + expect(layout).toEqual( + expect.arrayContaining([ + { key: 'API_URL', scope: sourceProject.id, value: 'from-source' }, + { key: 'API_URL', scope: targetProject.id, value: 'from-source' }, + ]), + ); + expect(layout).toHaveLength(2); + }); + + it('replaces a differing global value at the global scope', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + await createVariable('API_URL', 'from-global'); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'overwrite', + }); + + expect(result.variables).toMatchObject({ matched: [], updated: ['API_URL'] }); + expect(await variableLayout()).toEqual( + expect.arrayContaining([{ key: 'API_URL', scope: 'global', value: 'from-source' }]), + ); + expect(await variablesInProject(targetProject.id)).toEqual([]); + }); + + it('leaves the value alone when the package carries no variable values', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'from-source', { + includeVariableValues: false, + }); + await createProjectVariable('API_URL', 'from-target', targetProject); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'overwrite', + }); + + expect(result.variables).toMatchObject({ matched: ['API_URL'], updated: [] }); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-target'); + }); + + it('leaves the value alone when the package bundles an empty value', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, ''); + await createProjectVariable('API_URL', 'from-target', targetProject); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'overwrite', + }); + + expect(result.variables).toMatchObject({ matched: ['API_URL'], updated: [] }); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-target'); + }); + + it('reports an identical value as matched rather than updated', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'same-everywhere'); + await createProjectVariable('API_URL', 'same-everywhere', targetProject); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'overwrite', + }); + + expect(result.variables).toMatchObject({ matched: ['API_URL'], updated: [] }); + }); + + it('leaves the overwrite unapplied when the workflow write fails', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + await createProjectVariable('API_URL', 'from-target', targetProject); + const workflowsBefore = await workflowRepository.count(); + vi.spyOn(Container.get(WorkflowCreationService), 'createWorkflow').mockRejectedValueOnce( + new Error('workflow write failed'), + ); + + await expect( + importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'overwrite', + }), + ).rejects.toThrow('workflow write failed'); + + expect(await workflowRepository.count()).toBe(workflowsBefore); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-target'); + }); + + it('skips an overwrite whose variable another writer deleted after the plan', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + const target = await createProjectVariable('API_URL', 'from-target', targetProject); + // Deletes the row once the plan has read it, which is the window the write reopens. + vi.spyOn(variablesService, 'getAllCached').mockImplementationOnce(async () => { + const planned = await variablesRepository.find({ relations: { project: true } }); + await variablesRepository.delete(target.id); + await variablesService.updateCache(); + return planned; + }); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'overwrite', + }); + + // Nothing left to overwrite, so the import finishes rather than failing on a missing row. + expect(result.workflows[0].status).toBe('created'); + expect(result.variables).toMatchObject({ matched: ['API_URL'], updated: [] }); + expect(await variablesInProject(targetProject.id)).toEqual([]); + }); + }); + + describe('fail', () => { + it('blocks the import and writes nothing when a value differs', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { workflow, packageBuffer } = await packageReferencing(owner, 'from-source'); + await createProjectVariable('API_URL', 'from-target', targetProject); + const workflowsBefore = await workflowRepository.count(); + + await expect( + importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'fail', + }), + ).rejects.toMatchObject({ + message: /Import blocked/, + meta: { + issues: [ + { + type: 'variable-conflict', + name: 'API_URL', + projectId: targetProject.id, + usedByWorkflows: [workflow.id], + }, + ], + }, + }); + + expect(await workflowRepository.count()).toBe(workflowsBefore); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-target'); + }); + + it('names the global scope of the conflicting variable by omitting the project id', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { workflow, packageBuffer } = await packageReferencing(owner, 'from-source'); + await createVariable('API_URL', 'from-global'); + + const error = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'fail', + }).catch((e: unknown) => e); + + expect((error as ConflictError).meta?.issues).toEqual([ + { type: 'variable-conflict', name: 'API_URL', usedByWorkflows: [workflow.id] }, + ]); + }); + + it('imports when the resolved value matches the package value', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'same-everywhere'); + await createProjectVariable('API_URL', 'same-everywhere', targetProject); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'fail', + }); + + expect(result.workflows[0].status).toBe('created'); + expect(result.variables).toMatchObject({ matched: ['API_URL'], updated: [] }); + }); + + it('imports when the package bundles an empty value', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, ''); + await createProjectVariable('API_URL', 'from-target', targetProject); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'fail', + }); + + expect(result.workflows[0].status).toBe('created'); + expect(result.variables).toMatchObject({ matched: ['API_URL'], updated: [] }); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-target'); + }); + }); + + /** + * A target row holding an empty value still holds a value, unlike an empty *package* value, + * which is nothing to write. So `overwrite` fills such a row and `fail` rejects it. + */ + describe('empty target value', () => { + async function importAgainst(variableConflictPolicy: 'keep-existing' | 'overwrite' | 'fail') { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { workflow, packageBuffer } = await packageReferencing(owner, 'from-source'); + await createProjectVariable('API_URL', '', targetProject); + + return { + targetProject, + workflow, + result: await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy, + }).catch((error: unknown) => error), + }; + } + + it('leaves the empty value in place under keep-existing', async () => { + const { targetProject, result } = await importAgainst('keep-existing'); + + expect(result).toMatchObject({ variables: { matched: ['API_URL'], updated: [] } }); + expect((await variablesInProject(targetProject.id))[0].value).toBe(''); + }); + + it('fills the empty value under overwrite', async () => { + const { targetProject, result } = await importAgainst('overwrite'); + + expect(result).toMatchObject({ variables: { matched: [], updated: ['API_URL'] } }); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-source'); + }); + + it('blocks the import under fail', async () => { + const { targetProject, workflow, result } = await importAgainst('fail'); + + expect((result as ConflictError).meta?.issues).toEqual([ + { + type: 'variable-conflict', + name: 'API_URL', + projectId: targetProject.id, + usedByWorkflows: [workflow.id], + }, + ]); + expect((await variablesInProject(targetProject.id))[0].value).toBe(''); + }); + }); + + it.each(['keep-existing', 'overwrite', 'fail'] as const)( + 'defers to the missing mode under %s when the variable does not resolve', + async (variableConflictPolicy) => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableMissingMode: 'create-with-value', + variableConflictPolicy, + }); + + expect(result.variables).toMatchObject({ created: ['API_URL'], updated: [] }); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-source'); + }, + ); + + describe('gating', () => { + it('rejects an overwrite of a global variable without the global variable:update scope', async () => { + const owner = await createOwner(); + const member = await createMember(); + const targetProject = await createTeamProject('Target', owner); + // An editor may import and update project variables, but rewriting a global row + // additionally requires the global variable:update scope, which members lack. + await linkUserToProject(member, targetProject, 'project:editor'); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + await createVariable('API_URL', 'from-global'); + const workflowsBefore = await workflowRepository.count(); + + await expect( + importPackage({ + user: member, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'overwrite', + }), + ).rejects.toThrow('You are not allowed to update global variables'); + + expect(await workflowRepository.count()).toBe(workflowsBefore); + expect(await variableLayout()).toEqual( + expect.arrayContaining([{ key: 'API_URL', scope: 'global', value: 'from-global' }]), + ); + }); + + it('rejects an overwrite without projectVariable:update in the target project', async () => { + const owner = await createOwner(); + const member = await createMember(); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + const personalProject = await getPersonalProject(member); + await createProjectVariable('API_URL', 'from-personal', personalProject); + const workflowsBefore = await workflowRepository.count(); + + // A member may import into their own personal project, but the personal-owner role + // does not carry projectVariable:update. + await expect( + importPackage({ user: member, packageBuffer, variableConflictPolicy: 'overwrite' }), + ).rejects.toThrow('You are not allowed to update variables in this project'); + + expect(await workflowRepository.count()).toBe(workflowsBefore); + expect((await variablesInProject(personalProject.id))[0].value).toBe('from-personal'); + }); + + it('rejects an overwrite when the API key lacks the variable:update scope', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + await createProjectVariable('API_URL', 'from-target', targetProject); + const workflowsBefore = await workflowRepository.count(); + + await expect( + importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + apiKeyScopes: ['workflow:import'], + variableConflictPolicy: 'overwrite', + }), + ).rejects.toBeInstanceOf(ForbiddenError); + + expect(await workflowRepository.count()).toBe(workflowsBefore); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-target'); + }); + + it('overwrites for an API key carrying variable:update', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + await createProjectVariable('API_URL', 'from-target', targetProject); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + apiKeyScopes: ['workflow:import', 'variable:update'], + variableConflictPolicy: 'overwrite', + }); + + expect(result.variables).toMatchObject({ updated: ['API_URL'] }); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-source'); + }); + + it('rejects an overwrite when variables are not licensed', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'from-source'); + await createProjectVariable('API_URL', 'from-target', targetProject); + licenseMocker.disable('feat:variables'); + const workflowsBefore = await workflowRepository.count(); + + await expect( + importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'overwrite', + }), + ).rejects.toThrow(/license does not allow variables/); + + expect(await workflowRepository.count()).toBe(workflowsBefore); + expect((await variablesInProject(targetProject.id))[0].value).toBe('from-target'); + }); + + it('imports under fail without a variables licence, since nothing is written', async () => { + const owner = await createOwner(); + const targetProject = await createTeamProject('Target', owner); + const { packageBuffer } = await packageReferencing(owner, 'same-everywhere'); + await createProjectVariable('API_URL', 'same-everywhere', targetProject); + licenseMocker.disable('feat:variables'); + + const result = await importPackage({ + user: owner, + projectId: targetProject.id, + packageBuffer, + variableConflictPolicy: 'fail', + }); + + expect(result.variables).toMatchObject({ matched: ['API_URL'] }); + }); + }); + }); }); diff --git a/packages/cli/src/modules/n8n-packages/engine/__tests__/import-result.test.ts b/packages/cli/src/modules/n8n-packages/engine/__tests__/import-result.test.ts index 32fb84be84b..1ec70576ab0 100644 --- a/packages/cli/src/modules/n8n-packages/engine/__tests__/import-result.test.ts +++ b/packages/cli/src/modules/n8n-packages/engine/__tests__/import-result.test.ts @@ -77,7 +77,8 @@ describe('reconcileVariableSummary', () => { created: [], stubbed: [], skipped: ['API_URL'], + updated: [], }), - ).toEqual({ matched: ['API_URL'], missing: [], created: [], stubbed: [] }); + ).toEqual({ matched: ['API_URL'], missing: [], created: [], stubbed: [], updated: [] }); }); }); 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 be39ceb992c..df4ff27037f 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 @@ -42,10 +42,10 @@ const scope = (input: { matched: number; missing: number; requirements: number; - /** How the scope's `missing` names were resolved: with a package value, as an empty stub, or already there. */ - valued?: number; + createdWithValue?: number; stubbed?: number; existing?: number; + overwritten?: number; }; /** Tag ids per resolution bucket; the same id may recur across scopes. */ tags?: { @@ -65,12 +65,19 @@ const scope = (input: { const dt = input.dataTable ?? { matched: 0, created: 0, requirements: 0 }; const vars = input.variables ?? { matched: 0, missing: 0, requirements: 0 }; const missingVariableNames = Array.from({ length: vars.missing }, (_, i) => `missing-var-${i}`); - const [valuedCount, stubbedCount] = [vars.valued ?? 0, vars.stubbed ?? 0]; - const createdVariableNames = missingVariableNames.slice(0, valuedCount); - const stubbedVariableNames = missingVariableNames.slice(valuedCount, valuedCount + stubbedCount); + const [createdCount, stubbedCount] = [vars.createdWithValue ?? 0, vars.stubbed ?? 0]; + const createdVariableNames = missingVariableNames.slice(0, createdCount); + const stubbedVariableNames = missingVariableNames.slice( + createdCount, + createdCount + stubbedCount, + ); const existingVariableNames = missingVariableNames.slice( - valuedCount + stubbedCount, - valuedCount + stubbedCount + (vars.existing ?? 0), + createdCount + stubbedCount, + createdCount + stubbedCount + (vars.existing ?? 0), + ); + const overwrittenVariableNames = Array.from( + { length: vars.overwritten ?? 0 }, + (_, i) => `overwritten-var-${i}`, ); const tags = input.tags ?? {}; const toTagRefs = (ids: string[] = []) => ids.map((id) => ({ id, name: `name-of-${id}` })); @@ -81,16 +88,27 @@ const scope = (input: { credentialResult: input.credentialResult, dataTablePlan: { creations: new Array(dt.created), failures: [], matchedCount: dt.matched }, variablePlan: { - matched: Array.from({ length: vars.matched }, (_, i) => `matched-var-${i}`), + matched: [ + ...Array.from({ length: vars.matched }, (_, i) => `matched-var-${i}`), + ...overwrittenVariableNames, + ], missing: missingVariableNames.map((name) => ({ name, usedByWorkflows: [] })), creations: [...createdVariableNames, ...stubbedVariableNames, ...existingVariableNames].map( (name) => ({ name, usedByWorkflows: [] }), ), + conflicts: overwrittenVariableNames.map((name) => ({ name, usedByWorkflows: [] })), + overwrites: overwrittenVariableNames.map((name) => ({ + variableId: `id-of-${name}`, + name, + value: 'from-package', + usedByWorkflows: [], + })), }, variableResult: { created: createdVariableNames, stubbed: stubbedVariableNames, skippedExisting: existingVariableNames, + updated: overwrittenVariableNames, }, tagPlan: { matched: toTagRefs(tags.matched), @@ -120,6 +138,7 @@ const scope = (input: { variableRequest: { requirements: vars.requirements === 0 ? undefined : new Array(vars.requirements), missingMode: 'do-nothing', + conflictPolicy: 'keep-existing', }, tagRequest: { requirements: (tags.requirementIds ?? []).map((id) => ({ @@ -141,6 +160,7 @@ const request = mock({ credentialMissingMode: 'create-stub', workflowPublishingPolicy: 'preserve-published-state', variableMissingMode: 'create-stub', + variableConflictPolicy: 'overwrite', variableParentPolicy: 'global', missingNodeTypeMode: 'fail', tagMissingMode: 'create', @@ -190,7 +210,14 @@ describe('emitPackageImportedEvent', () => { }, requirements: [requirement('credB')], dataTable: { matched: 0, created: 2, requirements: 2 }, - variables: { matched: 0, missing: 2, requirements: 2, valued: 1, stubbed: 1 }, + variables: { + matched: 0, + missing: 2, + requirements: 3, + createdWithValue: 1, + stubbed: 1, + overwritten: 1, + }, // T2 recurs from scope 1: tags are global, so it must count once. tags: { created: ['T2'], @@ -219,13 +246,15 @@ describe('emitPackageImportedEvent', () => { workflows: { created: 1, updated: 1, skipped: 1 }, credentials: { matched: 1, created: 1, requirements: 2 }, dataTables: { matched: 1, created: 2, requirements: 3 }, - // scope 2's two missing requirements were created, so post-apply missing is 0. - variables: { matched: 1, missing: 0, created: 1, stubbed: 1, requirements: 3 }, + // 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 }, // T2 and its requirement appear in both scopes but count once (unique tag ids). tags: { matched: 1, created: 1, renamed: 1, reconciled: 1, skipped: 1, requirements: 5 }, }); expect(payload.packageSourceId).toBe('src-1'); expect(payload.options.variableMissingMode).toBe('create-stub'); + expect(payload.options.variableConflictPolicy).toBe('overwrite'); expect(payload.options.variableParentPolicy).toBe('global'); expect(payload.options.tagMissingMode).toBe('create'); expect(payload.options.tagConflictPolicy).toBe('rename'); @@ -243,7 +272,7 @@ describe('emitPackageImportedEvent', () => { outcomes: [outcome('wf1', 'WF1', 'created')], credentialResult: { bindings: new Map(), matched: [], stubbed: [] }, // Of three missing requirements, one was created with a value and one already existed. - variables: { matched: 0, missing: 3, requirements: 3, valued: 1, existing: 1 }, + variables: { matched: 0, missing: 3, requirements: 3, createdWithValue: 1, existing: 1 }, }), ], }); @@ -255,6 +284,7 @@ describe('emitPackageImportedEvent', () => { missing: 1, created: 1, stubbed: 0, + updated: 0, requirements: 3, }); }); @@ -272,7 +302,7 @@ describe('emitPackageImportedEvent', () => { projectId: 'P1', outcomes: [outcome('wf1', 'WF1', 'created')], credentialResult: { bindings: new Map(), matched: [], stubbed: [] }, - variables: { matched: 0, missing: 1, requirements: 1, valued: 1 }, + variables: { matched: 0, missing: 1, requirements: 1, createdWithValue: 1 }, }), scope({ projectId: 'P2', @@ -288,6 +318,7 @@ describe('emitPackageImportedEvent', () => { missing: 0, created: 1, stubbed: 0, + updated: 0, requirements: 2, }); }); diff --git a/packages/cli/src/modules/n8n-packages/engine/__tests__/package-layout.test.ts b/packages/cli/src/modules/n8n-packages/engine/__tests__/package-layout.test.ts index e03f46829e0..ccb2a9ed3dc 100644 --- a/packages/cli/src/modules/n8n-packages/engine/__tests__/package-layout.test.ts +++ b/packages/cli/src/modules/n8n-packages/engine/__tests__/package-layout.test.ts @@ -73,7 +73,7 @@ describe('package-layout', () => { const scopeOf = (manifestVariables: ManifestEntry[], scopePrefix: string) => placedIn(manifestVariables, scopePrefix).globalPlacement ? 'global' : 'project'; - // A requirement names a variable, never a path, so both files answer to it — this test is for hand-made packages only. + // A requirement names a variable, never a path, so both files answer to it. Hand-made only. it('rejects two files claiming one name in the same directory', () => { const entries = [ named('API_URL', 'variables/api_url'), diff --git a/packages/cli/src/modules/n8n-packages/engine/import-blocked.error.ts b/packages/cli/src/modules/n8n-packages/engine/import-blocked.error.ts index 47d02f6f5fc..f9fd3da19e0 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-blocked.error.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-blocked.error.ts @@ -17,6 +17,7 @@ export function toImportBlockedError( issue.type === 'workflow-id-conflict' || issue.type === 'workflow-folder-conflict' || issue.type === 'folder-conflict' || + issue.type === 'variable-conflict' || (issue.type === 'data-table-unresolved' && (issue.kind === 'id-conflict' || issue.kind === 'name-conflict')) || (issue.type === 'tag-unresolved' && 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 20ae5c0a211..fb48ef018fa 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-gates.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-gates.ts @@ -17,23 +17,25 @@ export function assertPackageImportApiKeyScopes( } /** - * Gated on what the import will create, not on what the package requires: a package whose variables - * all resolve creates nothing, so it needs neither the licence nor the scope. Mirrors the variables UI. + * Gated on what the import will write, not on what the package requires: a package whose variables + * all resolve writes nothing, so it needs neither the licence nor a scope. Mirrors the variables UI. */ -export function assertVariableCreationAllowed(options: { +export function assertVariableWritesAllowed(options: { licenseState: LicenseState; apiKeyScopes: string[] | undefined; hasCreations: boolean; + hasOverwrites: boolean; }): void { - const { licenseState, apiKeyScopes, hasCreations } = options; - if (!hasCreations) return; + const { licenseState, apiKeyScopes, hasCreations, hasOverwrites } = options; + if (!hasCreations && !hasOverwrites) return; if (!licenseState.isVariablesLicensed()) { throw new ForbiddenError( - 'Your license does not allow variables. Importing a package that creates variables requires a license that supports variables.', + 'Your license does not allow variables. Importing a package that writes variables requires a license that supports variables.', ); } - assertPackageImportApiKeyScopes(apiKeyScopes, ['variable:create']); + if (hasCreations) assertPackageImportApiKeyScopes(apiKeyScopes, ['variable:create']); + if (hasOverwrites) assertPackageImportApiKeyScopes(apiKeyScopes, ['variable:update']); } /** 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 d3528e0921f..1aafda73647 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-orchestrator.ts @@ -26,6 +26,7 @@ import { TagImporter } from '../entities/tag/tag-importer'; import { contestedReconcileTargetFailures, droppedTagIds } from '../entities/tag/tag.types'; import type { TagImportPlan, TagImportRequest } from '../entities/tag/tag.types'; import { VariableImporter } from '../entities/variable/variable-importer'; +import { divergentOverwrites } from '../entities/variable/variable.types'; import type { VariableApplyResult, VariableImportPlan, @@ -58,7 +59,7 @@ import type { } from '../n8n-packages.types'; import type { PackageWorkflowRequirement } from '../spec/requirements.schema'; import { toImportBlockedError } from './import-blocked.error'; -import { assertVariableCreationAllowed } from './import-gates'; +import { assertVariableWritesAllowed } from './import-gates'; export interface ImportOrchestrationInput { context: ImportContext; @@ -122,29 +123,34 @@ export class ImportOrchestrator { ) {} /** - * Gates variable creation in instance-to-project order so the broadest cause wins: licence and API - * key scope, then each scope's create permission, then the quota. An unlicensed instance also reports - * a zero quota, which would otherwise surface as a limit issue instead of the real cause. + * Licence and scope before quota: an unlicensed instance also reports a zero quota, which would + * otherwise surface as a limit issue instead of the real cause. */ async assertNotBlocked( plans: ImportPlan[], options: { apiKeyScopes: string[] | undefined }, ): Promise { const creations = plans.flatMap((plan) => plan.variablePlan.creations); + const overwrites = plans.flatMap((plan) => plan.variablePlan.overwrites); - assertVariableCreationAllowed({ + assertVariableWritesAllowed({ licenseState: this.licenseState, apiKeyScopes: options.apiKeyScopes, hasCreations: creations.length > 0, + hasOverwrites: overwrites.length > 0, }); for (const { input, variablePlan } of plans) { - if (variablePlan.creations.length === 0) continue; - await this.variableImporter.assertCanCreate( - input.context, - variablePlan.creations, - input.projectPendingCreation ?? false, - ); + if (variablePlan.creations.length > 0) { + await this.variableImporter.assertCanCreate( + input.context, + variablePlan.creations, + input.projectPendingCreation ?? false, + ); + } + if (variablePlan.overwrites.length > 0) { + await this.variableImporter.assertCanUpdate(input.context, variablePlan.overwrites); + } } const issues = plans.flatMap((plan) => plan.blockingIssues); @@ -158,6 +164,10 @@ export class ImportOrchestrator { ).map((failure): BlockingIssue => ({ type: 'tag-unresolved', ...failure })), ); + for (const conflict of divergentOverwrites(overwrites)) { + issues.push({ type: 'variable-conflict', ...conflict }); + } + const quotaFailure = await this.variableImporter.quotaFailure(creations); if (quotaFailure) issues.push({ type: 'variable-limit-exceeded', ...quotaFailure }); @@ -250,11 +260,7 @@ export class ImportOrchestrator { } = plan; const { context, credentialRequest } = input; - // Variables and tags go first: their creations are the apply steps that can still fail - // after the blocking-issue gate (a near-quota or unique-index race), and they depend on - // nothing below — applying them before any other write keeps a raced import from - // persisting anything else. - const variableResult = await this.variableImporter.apply(context, variablePlan); + // Tags go first because the workflow write attaches them by id. await this.tagImporter.apply(context, tagPlan); const folderSummaries = await this.folderImporter.apply(folderContext, folderPlan); @@ -292,6 +298,11 @@ export class ImportOrchestrator { }), ); + // Last of the writes: an overwrite is the only step that rewrites pre-existing data, and no + // step above reads a variable, since `$vars` resolves by name at runtime. Still ahead of the + // publish sweep, which evaluates trigger parameters against variable values. + const variableResult = await this.variableImporter.apply(context, variablePlan); + return { workflowOutcomes: outcomes.map((outcome) => withBlockedFromPublish(outcome, blockedFromPublish.get(outcome.sourceWorkflowId)), @@ -352,6 +363,9 @@ export class ImportOrchestrator { ...this.variableImporter .blockingFailures(variableRequest, variablePlan) .map((failure): BlockingIssue => ({ type: 'variable-unresolved', ...failure })), + ...this.variableImporter + .blockingConflicts(variableRequest, variablePlan) + .map((conflict): BlockingIssue => ({ type: 'variable-conflict', ...conflict })), ...missingNodeTypeBlockingFailures(missingNodeTypeMode, missingNodeTypes).map( ({ type, typeVersion, usedByWorkflows }): BlockingIssue => ({ type: 'missing-node-type', diff --git a/packages/cli/src/modules/n8n-packages/engine/import-result.ts b/packages/cli/src/modules/n8n-packages/engine/import-result.ts index efb41874d93..d7cd84cd81b 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-result.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-result.ts @@ -85,21 +85,29 @@ export function reconcileVariableSummary(input: { created: Iterable; stubbed: Iterable; skipped: Iterable; + updated: Iterable; }): ImportVariableSummary { const matched = new Set(input.matched); const created = new Set(input.created); const stubbed = new Set(input.stubbed); const skipped = new Set(input.skipped); + const updated = new Set(input.updated); // A skipped name that no scope of this import created genuinely pre-existed, so it counts as matched. for (const name of skipped) { if (!created.has(name) && !stubbed.has(name)) matched.add(name); } + // An overwritten name matched first, but the import rewrote it, so `updated` wins. + for (const name of updated) { + matched.delete(name); + } + return { matched: [...matched], created: [...created], stubbed: [...stubbed], + updated: [...updated], missing: [...new Set(input.missing)].filter( (name) => !created.has(name) && !stubbed.has(name) && !skipped.has(name), ), @@ -116,6 +124,7 @@ export function toVariableSummary( created: result.created, stubbed: result.stubbed, skipped: result.skippedExisting, + updated: result.updated, }); } 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 0c63be9bf73..e65c28aa6ee 100644 --- a/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts +++ b/packages/cli/src/modules/n8n-packages/engine/import-telemetry.ts @@ -73,10 +73,10 @@ export function emitPackageImportedEvent( created: scopes.flatMap(({ imported }) => imported.variableResult.created), stubbed: scopes.flatMap(({ imported }) => imported.variableResult.stubbed), skipped: scopes.flatMap(({ imported }) => imported.variableResult.skippedExisting), + updated: scopes.flatMap(({ imported }) => imported.variableResult.updated), }); - // Rows, not reconciled names: a name created in two projects is two variables, and every sibling - // count reports entities the same way. - const countCreatedRows = (pick: (result: ImportContentResult) => string[]) => + // Rows, not reconciled names: a name written in two projects is two rows. + const countRows = (pick: (result: ImportContentResult) => string[]) => scopes.reduce((total, { imported }) => total + pick(imported).length, 0); // Tags are global, so several scopes may plan the same tag; count each id once. @@ -105,6 +105,7 @@ export function emitPackageImportedEvent( dataTableMissingMode: request.dataTableMissingMode, dataTableSchemaConflictPolicy: request.dataTableSchemaConflictPolicy, variableMissingMode: request.variableMissingMode, + variableConflictPolicy: request.variableConflictPolicy, // An omitted policy places variables in the project, so report what the import did. variableParentPolicy: request.variableParentPolicy ?? VariableParentPolicy.Project, tagMissingMode: request.tagMissingMode, @@ -136,8 +137,9 @@ export function emitPackageImportedEvent( variables: { matched: variableSummary.matched.length, missing: variableSummary.missing.length, - created: countCreatedRows(({ variableResult }) => variableResult.created), - stubbed: countCreatedRows(({ variableResult }) => variableResult.stubbed), + created: countRows(({ variableResult }) => variableResult.created), + stubbed: countRows(({ variableResult }) => variableResult.stubbed), + updated: countRows(({ variableResult }) => variableResult.updated), requirements: variableRequirements, }, tags: { diff --git a/packages/cli/src/modules/n8n-packages/engine/package-layout.ts b/packages/cli/src/modules/n8n-packages/engine/package-layout.ts index 4148f85b206..28ac4b2d6fa 100644 --- a/packages/cli/src/modules/n8n-packages/engine/package-layout.ts +++ b/packages/cli/src/modules/n8n-packages/engine/package-layout.ts @@ -1,7 +1,9 @@ import { UserError } from 'n8n-workflow'; +import { variableMissingModeUsesPackageValue } from '../entities/variable/variable-missing-mode'; import type { PlacedVariableRequirement } from '../entities/variable/variable.types'; -import { VariableParentPolicy } from '../n8n-packages.types'; +import { VariableConflictPolicy, VariableParentPolicy } from '../n8n-packages.types'; +import type { ImportVariableProperties } from '../n8n-packages.types'; import type { ManifestEntry } from '../spec/manifest.schema'; import type { PackageVariableRequirement } from '../spec/requirements.schema'; import type { SerializedVariable } from '../spec/serialized/variable.schema'; @@ -24,6 +26,17 @@ export function workflowsInScope( ); } +export function needsBundledVariableValues( + request: ImportVariableProperties, + hasRequirements: boolean, +): boolean { + return ( + hasRequirements && + (variableMissingModeUsesPackageValue(request.variableMissingMode) || + request.variableConflictPolicy !== VariableConflictPolicy.KeepExisting) + ); +} + /** The entry bundling `name` for one scope: its own variables/ entry beats a top-level one, a tie is ambiguous. */ function variableBundleEntry( entries: ManifestEntry[] | undefined, 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 36e014bddc4..09e018fc9d0 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 @@ -8,7 +8,6 @@ import type { CredentialBindingRequest } from '../entities/credential/credential import type { DataTableImportRequest } from '../entities/data-table/data-table.types'; import { ProjectImporter } from '../entities/project/project-importer'; import type { TagImportRequest } from '../entities/tag/tag.types'; -import { variableMissingModeUsesPackageValue } from '../entities/variable/variable-missing-mode'; import type { VariableImportRequest } from '../entities/variable/variable.types'; import { collectPlannedWorkflowBindings } from '../entities/workflow/workflow-importer'; import { WorkflowPublisher } from '../entities/workflow/workflow-publisher'; @@ -24,7 +23,7 @@ import type { } from '../n8n-packages.types'; import { mergeBindings } from '../n8n-packages.types'; import { assertPackageImportApiKeyScopes, assertTagWritesAllowed } from './import-gates'; -import { placeByLayout } from './package-layout'; +import { needsBundledVariableValues, placeByLayout } from './package-layout'; import { ImportOrchestrator, type ImportContentResult, @@ -66,11 +65,12 @@ export class ProjectPackageImporter { const projects = await this.packageParser.getProjects(reader); const projectPlan = await this.projectImporter.plan(request.user, projects); - const bundledVariables = - (manifest.requirements?.variables?.length ?? 0) > 0 && - variableMissingModeUsesPackageValue(request.variableMissingMode) - ? await this.packageParser.getVariables(reader) - : undefined; + const bundledVariables = needsBundledVariableValues( + request, + (manifest.requirements?.variables?.length ?? 0) > 0, + ) + ? await this.packageParser.getVariables(reader) + : undefined; // Projects the user is creating (vs matching an existing one). They will be admin of these, // so publish is always allowed and the project need not exist while its contents are planned. const pendingCreateIds = new Set( @@ -145,6 +145,7 @@ export class ProjectPackageImporter { const variablesCreated: string[] = []; const variablesStubbed: string[] = []; const variablesSkipped: string[] = []; + const variablesUpdated: string[] = []; const tagSummaries: ImportTagSummary[] = []; const scopes: PackageImportScope[] = []; @@ -161,6 +162,7 @@ export class ProjectPackageImporter { variablesCreated.push(...content.variableResult.created); variablesStubbed.push(...content.variableResult.stubbed); variablesSkipped.push(...content.variableResult.skippedExisting); + variablesUpdated.push(...content.variableResult.updated); tagSummaries.push(toTagSummary(content.tagPlan)); scopes.push({ context: plan.input.context, @@ -187,6 +189,7 @@ export class ProjectPackageImporter { created: variablesCreated, stubbed: variablesStubbed, skipped: variablesSkipped, + updated: variablesUpdated, }), tags: unionTagSummaries(tagSummaries), }); @@ -233,6 +236,7 @@ export class ProjectPackageImporter { bundledVariables, }), missingMode: request.variableMissingMode, + conflictPolicy: request.variableConflictPolicy, }; // Untrimmed on purpose: the tag importer scopes by this project's workflows' own 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 c7573836885..366137e6355 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 @@ -13,7 +13,6 @@ import { ProjectService } from '@/services/project.service.ee'; import type { CredentialBindingRequest } from '../entities/credential/credential.types'; import type { DataTableImportRequest } from '../entities/data-table/data-table.types'; import type { TagImportRequest } from '../entities/tag/tag.types'; -import { variableMissingModeUsesPackageValue } from '../entities/variable/variable-missing-mode'; import type { VariableImportRequest } from '../entities/variable/variable.types'; import { WorkflowPublisher } from '../entities/workflow/workflow-publisher'; import type { PackageReader } from '../io/package-reader'; @@ -31,7 +30,7 @@ import { } from './import-result'; import { emitPackageImportedEvent } from './import-telemetry'; import { N8nPackageParser } from './n8n-package-parser'; -import { placeByPolicy } from './package-layout'; +import { needsBundledVariableValues, placeByPolicy } from './package-layout'; import type { PackageManifest } from '../spec/manifest.schema'; /** @@ -92,11 +91,12 @@ export class WorkflowPackageImporter { }; const variableRequirements = identifyRequirements(manifest.requirements?.variables, workflows); - const bundledVariables = - (variableRequirements?.length ?? 0) > 0 && - variableMissingModeUsesPackageValue(request.variableMissingMode) - ? await this.packageParser.getVariables(reader) - : undefined; + const bundledVariables = needsBundledVariableValues( + request, + (variableRequirements?.length ?? 0) > 0, + ) + ? await this.packageParser.getVariables(reader) + : undefined; const variableRequest: VariableImportRequest = { requirements: placeByPolicy({ requirements: variableRequirements, @@ -105,6 +105,7 @@ export class WorkflowPackageImporter { bundledVariables, }), missingMode: request.variableMissingMode, + conflictPolicy: request.variableConflictPolicy, }; const tagRequest: TagImportRequest = { diff --git a/packages/cli/src/modules/n8n-packages/entities/variable/__tests__/variable-importer.test.ts b/packages/cli/src/modules/n8n-packages/entities/variable/__tests__/variable-importer.test.ts index a2b565e1e5c..61cc638e5ba 100644 --- a/packages/cli/src/modules/n8n-packages/entities/variable/__tests__/variable-importer.test.ts +++ b/packages/cli/src/modules/n8n-packages/entities/variable/__tests__/variable-importer.test.ts @@ -49,6 +49,8 @@ describe('VariableImporter.apply', () => { matched: [], missing: [{ name: 'API_KEY', usedByWorkflows: ['wf-1'] }], creations: [{ name: 'API_KEY', projectId: 'proj-target', usedByWorkflows: ['wf-1'] }], + conflicts: [], + overwrites: [], }; it('reports a creation with an empty package value as stubbed, not created', async () => { @@ -60,7 +62,7 @@ describe('VariableImporter.apply', () => { creations: [{ ...projectCreationPlan.creations[0], value: '' }], }); - expect(result).toEqual({ created: [], stubbed: ['API_KEY'], skippedExisting: [] }); + expect(result).toEqual({ created: [], stubbed: ['API_KEY'], skippedExisting: [], updated: [] }); }); it('treats a concurrent create as a skip when the variable now exists at the destination', async () => { @@ -78,6 +80,6 @@ describe('VariableImporter.apply', () => { const result = await importer.apply(context, projectCreationPlan); - expect(result).toEqual({ created: [], stubbed: [], skippedExisting: ['API_KEY'] }); + expect(result).toEqual({ created: [], stubbed: [], skippedExisting: ['API_KEY'], updated: [] }); }); }); diff --git a/packages/cli/src/modules/n8n-packages/entities/variable/variable-importer.ts b/packages/cli/src/modules/n8n-packages/entities/variable/variable-importer.ts index a61f6571dac..2b0479b6644 100644 --- a/packages/cli/src/modules/n8n-packages/entities/variable/variable-importer.ts +++ b/packages/cli/src/modules/n8n-packages/entities/variable/variable-importer.ts @@ -20,12 +20,15 @@ import { } from './variable.types'; import type { VariableApplyResult, + VariableConflict, VariableCreation, VariableImportPlan, VariableImportRequest, VariableLimitFailure, + VariableOverwrite, VariableResolutionFailure, } from './variable.types'; +import { VariableConflictPolicy } from '../../n8n-packages.types'; import type { ImportContext } from '../../n8n-packages.types'; @Service() @@ -35,7 +38,9 @@ export class VariableImporter { /** Resolves requirements against the target project then global, mirroring runtime `$vars` precedence. */ async plan(context: ImportContext, request: VariableImportRequest): Promise { const requirements = request.requirements ?? []; - if (requirements.length === 0) return { matched: [], missing: [], creations: [] }; + if (requirements.length === 0) { + return { matched: [], missing: [], creations: [], conflicts: [], overwrites: [] }; + } const allVariables = await this.variablesService.getAllCached(); const variablesByKey = new Map(); @@ -48,8 +53,12 @@ export class VariableImporter { const matched: string[] = []; const missing: VariableResolutionFailure[] = []; const creations: VariableCreation[] = []; + const conflicts: VariableConflict[] = []; + const overwrites: VariableOverwrite[] = []; const createsMissing = variableMissingModeCreates(request.missingMode); const usesPackageValue = variableMissingModeUsesPackageValue(request.missingMode); + const comparesValues = request.conflictPolicy !== VariableConflictPolicy.KeepExisting; + const overwritesConflicts = request.conflictPolicy === VariableConflictPolicy.Overwrite; for (const requirement of requirements) { const picked = pickVariableForProject( @@ -59,6 +68,25 @@ export class VariableImporter { ); if (picked) { matched.push(requirement.name); + + const packageValue = requirement.packageValue; + // An empty bundled value is nothing to write, the same way a creation treats it as a stub. + if (!comparesValues || !packageValue || packageValue === picked.value) { + continue; + } + + const scope = picked.project ? { projectId: picked.project.id } : {}; + const usedByWorkflows = [...new Set(requirement.usedByWorkflows)].sort(); + conflicts.push({ name: requirement.name, ...scope, usedByWorkflows }); + if (overwritesConflicts) { + overwrites.push({ + variableId: picked.id, + name: requirement.name, + ...scope, + value: packageValue, + usedByWorkflows, + }); + } continue; } missing.push(createFailure(requirement)); @@ -73,7 +101,7 @@ export class VariableImporter { } } - return { matched, missing, creations }; + return { matched, missing, creations, conflicts, overwrites }; } /** Deduplicates by destination first: one global variable planned by several scopes is one new row. */ @@ -93,6 +121,10 @@ export class VariableImporter { return variableBlockingFailures(request.missingMode, plan); } + blockingConflicts(request: VariableImportRequest, plan: VariableImportPlan): VariableConflict[] { + return request.conflictPolicy === VariableConflictPolicy.Fail ? plan.conflicts : []; + } + /** * Re-checks the destination against a fresh cache before each create, so a variable an earlier * scope of this import already created is skipped rather than duplicated. `VariablesService.create` @@ -136,10 +168,23 @@ export class VariableImporter { } } + const updated: string[] = []; + for (const overwrite of plan.overwrites) { + const current = await this.variablesService.getCached(overwrite.variableId); + // Nothing to write if another writer deleted the row, or an earlier scope already applied it. + if (!current || current.value === overwrite.value) continue; + + await this.variablesService.update(context.user, overwrite.variableId, { + value: overwrite.value, + }); + updated.push(overwrite.name); + } + return { created: [...new Set(created)], stubbed: [...new Set(stubbed)], skippedExisting: [...new Set(skippedExisting)], + updated: [...new Set(updated)], }; } @@ -152,27 +197,68 @@ export class VariableImporter { ); } + private targetScopes(targets: Array<{ projectId?: string }>, skipProjectId?: string) { + const projectIds = new Set(); + for (const { projectId } of targets) { + if (projectId && projectId !== skipProjectId) projectIds.add(projectId); + } + + return { + touchesGlobal: targets.some(({ projectId }) => !projectId), + projectIds, + }; + } + async assertCanCreate( context: ImportContext, creations: VariableCreation[], projectPendingCreation: boolean, ): Promise { - const needsGlobal = creations.some((creation) => !creation.projectId); - const needsProject = creations.some((creation) => creation.projectId); + // A project this import is about to create has no scopes to look up yet; the user becomes its + // admin on creation, and `VariablesService.create` re-checks at apply time. + const { touchesGlobal, projectIds } = this.targetScopes( + creations, + projectPendingCreation ? context.projectId : undefined, + ); - if (needsGlobal && !hasGlobalScope(context.user, 'variable:create')) { + if (touchesGlobal && !hasGlobalScope(context.user, 'variable:create')) { throw new ForbiddenError('You are not allowed to create global variables'); } - // A project this import is about to create has no scopes to look up yet; the user becomes its - // admin on creation, and `VariablesService.create` re-checks at apply time. - if (needsProject && !projectPendingCreation) { - const allowed = await userHasScopes(context.user, ['projectVariable:create'], false, { - projectId: context.projectId, - }); - if (!allowed) { + for (const projectId of projectIds) { + const projectVariableCreationAllowed = await userHasScopes( + context.user, + ['projectVariable:create'], + false, + { + projectId, + }, + ); + if (!projectVariableCreationAllowed) { throw new ForbiddenError('You are not allowed to create variables in this project'); } } } + + async assertCanUpdate(context: ImportContext, overwrites: VariableOverwrite[]): Promise { + const { touchesGlobal, projectIds } = this.targetScopes(overwrites); + + if (touchesGlobal && !hasGlobalScope(context.user, 'variable:update')) { + throw new ForbiddenError('You are not allowed to update global variables'); + } + + for (const projectId of projectIds) { + const projectVariableUpdateAllowed = await userHasScopes( + context.user, + ['projectVariable:update'], + false, + { + projectId, + }, + ); + if (!projectVariableUpdateAllowed) { + throw new ForbiddenError('You are not allowed to update variables in this project'); + } + } + } } diff --git a/packages/cli/src/modules/n8n-packages/entities/variable/variable.types.ts b/packages/cli/src/modules/n8n-packages/entities/variable/variable.types.ts index c605cd1e563..6731656378e 100644 --- a/packages/cli/src/modules/n8n-packages/entities/variable/variable.types.ts +++ b/packages/cli/src/modules/n8n-packages/entities/variable/variable.types.ts @@ -1,7 +1,7 @@ import type { User } from '@n8n/db'; import type { PackageWriter } from '../../io/package-writer'; -import type { VariableMissingMode } from '../../n8n-packages.types'; +import type { VariableConflictPolicy, VariableMissingMode } from '../../n8n-packages.types'; import type { ManifestEntry } from '../../spec/manifest.schema'; import type { PackageVariableRequirement } from '../../spec/requirements.schema'; @@ -32,6 +32,7 @@ export type PlacedVariableRequirement = PackageVariableRequirement & { export interface VariableImportRequest { requirements: PlacedVariableRequirement[] | undefined; missingMode: VariableMissingMode; + conflictPolicy: VariableConflictPolicy; } export interface VariableResolutionFailure { @@ -46,6 +47,20 @@ export interface VariableCreation { usedByWorkflows: string[]; } +export interface VariableConflict { + name: string; + projectId?: string; + usedByWorkflows: string[]; +} + +export interface VariableOverwrite { + variableId: string; + name: string; + projectId?: string; + value: string; + usedByWorkflows: string[]; +} + export interface VariableLimitFailure { limit: number; remaining: number; @@ -84,6 +99,24 @@ export function dedupeCreationsByDestination(creations: VariableCreation[]): Var return [...byDestination.values()]; } +/** + * Scopes resolve independently, so two can land on one row — a global neither shadows — and disagree + * about its value, where the last write would silently win. + */ +export function divergentOverwrites(overwrites: VariableOverwrite[]): VariableConflict[] { + const firstValueByRow = new Map(); + const divergent = new Set(); + for (const { variableId, value } of overwrites) { + const seen = firstValueByRow.get(variableId); + if (seen === undefined) firstValueByRow.set(variableId, value); + else if (seen !== value) divergent.add(variableId); + } + + return overwrites + .filter(({ variableId }) => divergent.has(variableId)) + .map(({ variableId, value, ...conflict }) => conflict); +} + /** Reports the planned creations that do not fit the remaining quota. `quota` of `null` means unlimited. */ export function computeVariableLimitFailure( creations: VariableCreation[], @@ -104,10 +137,13 @@ export interface VariableImportPlan { matched: string[]; missing: VariableResolutionFailure[]; creations: VariableCreation[]; + conflicts: VariableConflict[]; + overwrites: VariableOverwrite[]; } export interface VariableApplyResult { created: string[]; stubbed: string[]; skippedExisting: string[]; + updated: string[]; } 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 11bb493b075..7db44f9c1e5 100644 --- a/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts +++ b/packages/cli/src/modules/n8n-packages/n8n-packages.types.ts @@ -4,6 +4,7 @@ import type { Readable } from 'node:stream'; import type { DataTableResolutionFailure } from './entities/data-table/data-table.types'; import type { TagResolutionFailure } from './entities/tag/tag.types'; import type { + VariableConflict, VariableLimitFailure, VariableResolutionFailure, } from './entities/variable/variable.types'; @@ -98,6 +99,15 @@ export const VariableMissingMode = { CreateWithValue: 'create-with-value', } as const; +export const VariableConflictPolicy = { + /** Leaves the target value alone, even when the package bundles a different one. */ + KeepExisting: 'keep-existing', + /** Replaces the target value with the package's, at whichever scope the variable was found. */ + Overwrite: 'overwrite', + /** Rejects the import when the package bundles a value that differs from the target's. */ + Fail: 'fail', +} as const; + export const VariableParentPolicy = { Project: 'project', Global: 'global', @@ -142,6 +152,9 @@ export type DataTableSchemaConflictPolicy = export type VariableMissingMode = (typeof VariableMissingMode)[keyof typeof VariableMissingMode]; +export type VariableConflictPolicy = + (typeof VariableConflictPolicy)[keyof typeof VariableConflictPolicy]; + export type VariableParentPolicy = (typeof VariableParentPolicy)[keyof typeof VariableParentPolicy]; export type TagMissingMode = (typeof TagMissingMode)[keyof typeof TagMissingMode]; @@ -197,6 +210,7 @@ export type ImportDataTableProperties = { export type ImportVariableProperties = { variableMissingMode: VariableMissingMode; + variableConflictPolicy: VariableConflictPolicy; variableParentPolicy?: VariableParentPolicy; }; @@ -256,6 +270,7 @@ export type ImportPackageEventCounts = { missing: number; created: number; stubbed: number; + updated: number; requirements: number; }; tags: { @@ -344,6 +359,7 @@ export type BlockingIssue = | ({ type: 'data-table-unresolved' } & DataTableResolutionFailure) | ({ type: 'tag-unresolved' } & TagResolutionFailure) | ({ type: 'variable-unresolved' } & VariableResolutionFailure) + | ({ type: 'variable-conflict' } & VariableConflict) | ({ type: 'variable-limit-exceeded' } & VariableLimitFailure) | { type: 'missing-node-type'; @@ -417,6 +433,7 @@ export interface ImportVariableSummary { missing: string[]; created: string[]; stubbed: string[]; + updated: string[]; } /** Tag names (not ids), grouped by how the import resolved them. */ diff --git a/packages/cli/src/public-api/v1/handlers/n8n-packages/__tests__/n8n-packages.handler.test.ts b/packages/cli/src/public-api/v1/handlers/n8n-packages/__tests__/n8n-packages.handler.test.ts index 8066b04a6f8..055198c72c6 100644 --- a/packages/cli/src/public-api/v1/handlers/n8n-packages/__tests__/n8n-packages.handler.test.ts +++ b/packages/cli/src/public-api/v1/handlers/n8n-packages/__tests__/n8n-packages.handler.test.ts @@ -644,6 +644,7 @@ describe('n8n-packages handler', () => { projectId: 'proj-brie', workflowConflictPolicy: 'fail', variableMissingMode: 'create-with-value', + variableConflictPolicy: 'keep-existing', variableParentPolicy: undefined, }), ); @@ -668,6 +669,24 @@ describe('n8n-packages handler', () => { ); }); + it('forwards variableConflictPolicy when provided', async () => { + const result = { package: {}, workflows: [], bindings: {}, credentials: {} }; + mockService.importPackage.mockResolvedValue(result as never); + const res = { status: vi.fn().mockReturnThis(), json: vi.fn() } as unknown as Response; + + const caught = await runImport( + makeImportRequest({ projectId: 'proj-brie', variableConflictPolicy: 'overwrite' }, [ + 'workflow:import', + ]), + res, + ); + + expect(caught).toBeUndefined(); + expect(mockService.importPackage).toHaveBeenCalledWith( + expect.objectContaining({ variableConflictPolicy: 'overwrite' }), + ); + }); + it('forwards variableParentPolicy when provided', async () => { const result = { package: {}, workflows: [], bindings: {}, credentials: {} }; mockService.importPackage.mockResolvedValue(result as never); diff --git a/packages/cli/src/public-api/v1/handlers/n8n-packages/n8n-packages.handler.ts b/packages/cli/src/public-api/v1/handlers/n8n-packages/n8n-packages.handler.ts index 56cd07b55e9..804d77242c7 100644 --- a/packages/cli/src/public-api/v1/handlers/n8n-packages/n8n-packages.handler.ts +++ b/packages/cli/src/public-api/v1/handlers/n8n-packages/n8n-packages.handler.ts @@ -210,6 +210,7 @@ const n8nPackagesHandlers: N8nPackagesHandlers = { dataTableMissingMode: payload.data.dataTableMissingMode, dataTableSchemaConflictPolicy: payload.data.dataTableSchemaConflictPolicy, variableMissingMode: payload.data.variableMissingMode, + variableConflictPolicy: payload.data.variableConflictPolicy, variableParentPolicy: payload.data.variableParentPolicy, tagMissingMode: payload.data.tagMissingMode, tagConflictPolicy: payload.data.tagConflictPolicy, 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 13071edf305..0ba86ba0c5d 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 @@ -225,6 +225,39 @@ post: creates a variable requires a license that permits variables and, for API key callers, the `variable:create` scope; a package whose variables all already resolve creates nothing and needs neither. + variableConflictPolicy: + type: string + enum: + - keep-existing + - overwrite + - fail + default: keep-existing + description: > + 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` (default) leaves the target value + untouched and reports the name under `variables.matched`. + `overwrite` silently replaces the value of the existing variable + at whichever scope it was found — the target project or the + global scope, so a global variable other projects also read can + be rewritten by this import — and reports the name under + `variables.updated`; it needs a license that permits variables, + the `variable:update` / `projectVariable:update` permission on + that scope, and, for API key callers, the `variable:update` + scope. `fail` rejects the import with a 409 instead. Every + policy leaves a resolved variable alone when there is nothing + to change — either the package bundles no value for it (values + were excluded at export, or the exported value was itself + empty), or the value it bundles already matches the target's. + The reverse does not hold: a resolved variable holding an empty + value — a stub an earlier import created, say — still counts as + a value, so `overwrite` fills it and `fail` rejects it. 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, typically — is rejected with a 409: one row cannot + carry both values, and the import will not pick for you. Give + the projects their own variables in the target, or import them + separately. variableParentPolicy: type: string enum: @@ -493,6 +526,7 @@ post: - missing - created - stubbed + - updated properties: matched: type: array @@ -500,7 +534,9 @@ post: type: string description: > Variable names that resolved to an existing variable - (importing project's scope first, then global). + (importing project's scope first, then global) and were + left untouched. A resolved variable this import rewrote is + listed under `updated` instead. missing: type: array items: @@ -526,6 +562,14 @@ post: Variable names created with an empty value by this import under `variableMissingMode=create-stub`, or because `create-with-value` had no exported value. Empty for other outcomes. + updated: + type: array + items: + type: string + description: > + Variable names whose existing value this import replaced with + the package value under `variableConflictPolicy=overwrite`. + Empty for other policies. tags: type: object description: > @@ -600,8 +644,11 @@ post: description: > 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), or a tag conflict - (rename drift or name collision). + owned by another project, or a `fail`-policy match), a tag conflict + (rename drift or name collision), 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. content: application/json: schema: diff --git a/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/schemas/importBlockingIssue.yml b/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/schemas/importBlockingIssue.yml index 5e5ed8e9046..25eeb33c8aa 100644 --- a/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/schemas/importBlockingIssue.yml +++ b/packages/cli/src/public-api/v1/handlers/n8n-packages/spec/schemas/importBlockingIssue.yml @@ -266,6 +266,35 @@ oneOf: type: array items: type: string + - type: object + description: > + A variable that resolved in the target project or the global scope, but + whose value differs from the one the package bundles for it, under + `variableConflictPolicy=fail`. Also reported under `overwrite`, once per + scope, when the projects of a package resolve one row and disagree about + the value it should hold. Values are never reported — only the name and + the scope the variable was found in. + required: + - type + - name + - usedByWorkflows + properties: + type: + type: string + enum: + - variable-conflict + name: + type: string + description: Name of the variable whose value differs. + projectId: + type: string + description: > + Project owning the resolved variable. Absent when it resolved at the + global scope. + usedByWorkflows: + type: array + items: + type: string - type: object description: > Creating the package's variables under `create-stub` or `create-with-value` 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 769d63c474e..4ee2126e666 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 @@ -261,6 +261,7 @@ describe('POST /n8n-packages/import', () => { missing: [], created: [], stubbed: [], + updated: [], }, tags: { matched: [], @@ -291,6 +292,7 @@ describe('POST /n8n-packages/import', () => { missing: [], created: ['API_URL'], stubbed: [], + updated: [], }); const created = await getVariableByKey('API_URL'); expect(created).toMatchObject({ value: 'https://packaged.example.com' }); @@ -313,6 +315,7 @@ describe('POST /n8n-packages/import', () => { .field('dataTableMissingMode', 'must-preexist') .field('dataTableSchemaConflictPolicy', 'fail') .field('variableMissingMode', 'create-with-value') + .field('variableConflictPolicy', 'overwrite') .field('variableParentPolicy', 'project') .field('tagMissingMode', 'do-nothing') .field('tagConflictPolicy', 'fail')