Skip to content

Commit b148a52

Browse files
authored
fix(api): stop workspace import dropping every workflow variable (#6002)
The workspace importer guarded its variable write on `Array.isArray`, but `parseWorkflowVariables` — what the matching workspace export emits — returns the record form. Every variable was silently dropped, so exporting a workspace and importing it back lost all of them. It now uses the shared `normalizeImportedVariables`, which accepts both shapes. Also runs the workspace importer through `prepareWorkflowStateForPersistence`, the pipeline the editor, the v1 import API and the single-workflow admin import already share. Without it this route wrote raw parsed state, so a dangling edge tripped the `workflow_edges` foreign key and failed the restore, and blocks missing their backfilled columns could land unopenable. Repoints `persistImportedWorkflow` at the same helper, removing the last inline copy — all four import paths now normalize variables identically. Adds the first tests for these helpers, including the export -> import round trip that pins the record form the bug dropped.
1 parent ca13cc9 commit b148a52

3 files changed

Lines changed: 154 additions & 46 deletions

File tree

apps/sim/app/api/v1/admin/workspaces/[id]/import/route.ts

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,10 @@ import {
4141
extractWorkflowsFromZip,
4242
parseWorkflowJson,
4343
} from '@/lib/workflows/operations/import-export'
44+
import { prepareWorkflowStateForPersistence } from '@/lib/workflows/persistence/prepare-state'
4445
import { saveWorkflowToNormalizedTables } from '@/lib/workflows/persistence/utils'
4546
import { deduplicateWorkflowName } from '@/lib/workflows/utils'
47+
import { normalizeImportedVariables } from '@/lib/workflows/variables/parse'
4648
import { getWorkspaceWithOwner } from '@/lib/workspaces/permissions/utils'
4749
import { withAdminAuthParams } from '@/app/api/v1/admin/middleware'
4850
import {
@@ -52,7 +54,6 @@ import {
5254
} from '@/app/api/v1/admin/responses'
5355
import type {
5456
ImportResult,
55-
WorkflowVariable,
5657
WorkspaceImportRequest,
5758
WorkspaceImportResponse,
5859
} from '@/app/api/v1/admin/types'
@@ -270,7 +271,22 @@ async function importSingleWorkflow(
270271
variables: {},
271272
})
272273

273-
const saveResult = await saveWorkflowToNormalizedTables(workflowId, workflowData)
274+
/**
275+
* Same normalization the editor, the v1 import API and the single-workflow
276+
* admin import run, via the one shared implementation. Without it this route
277+
* wrote raw parsed state, so a dangling edge tripped the `workflow_edges`
278+
* foreign key and failed the restore, and blocks missing their backfilled
279+
* columns could land unopenable.
280+
*/
281+
const { state: preparedState, warnings } = prepareWorkflowStateForPersistence(workflowData)
282+
if (warnings.length > 0) {
283+
logger.warn(`Admin API: normalized "${dedupedName}" with warnings`, { warnings })
284+
}
285+
286+
const saveResult = await saveWorkflowToNormalizedTables(workflowId, {
287+
...workflowData,
288+
...preparedState,
289+
})
274290

275291
if (!saveResult.success) {
276292
await db.delete(workflow).where(eq(workflow.id, workflowId))
@@ -282,18 +298,13 @@ async function importSingleWorkflow(
282298
}
283299
}
284300

285-
if (workflowData.variables && Array.isArray(workflowData.variables)) {
286-
const variablesRecord: Record<string, WorkflowVariable> = {}
287-
workflowData.variables.forEach((v) => {
288-
const varId = v.id || generateId()
289-
variablesRecord[varId] = {
290-
id: varId,
291-
name: v.name,
292-
type: v.type || 'string',
293-
value: v.value,
294-
}
295-
})
296-
301+
/**
302+
* Previously guarded on `Array.isArray`, which silently dropped every
303+
* variable in the current record form — the exact shape this workspace's
304+
* own export emits — so an export/import round trip lost all of them.
305+
*/
306+
const variablesRecord = normalizeImportedVariables(workflowData.variables)
307+
if (Object.keys(variablesRecord).length > 0) {
297308
await db
298309
.update(workflow)
299310
.set({ variables: variablesRecord, updatedAt: new Date() })

apps/sim/lib/workflows/operations/import-export.ts

Lines changed: 10 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { createLogger } from '@sim/logger'
22
import { getErrorMessage } from '@sim/utils/errors'
3-
import { generateId } from '@sim/utils/id'
43
import { isRecordLike } from '@sim/utils/object'
54
import { ApiClientError } from '@/lib/api/client/errors'
65
import { requestJson } from '@/lib/api/client/request'
@@ -18,6 +17,7 @@ import {
1817
sanitizeForExport,
1918
} from '@/lib/workflows/sanitization/json-sanitizer'
2019
import { sanitizeMalformedSubBlocks } from '@/lib/workflows/sanitization/subblocks'
20+
import { normalizeImportedVariables } from '@/lib/workflows/variables/parse'
2121
import { regenerateWorkflowIds } from '@/stores/workflows/utils'
2222
import type { Variable, WorkflowState } from '@/stores/workflows/workflow/types'
2323

@@ -724,37 +724,15 @@ export async function persistImportedWorkflow({
724724
throw new Error(`Failed to save workflow state for ${newWorkflowId}`)
725725
}
726726

727-
if (workflowData.variables) {
728-
const variablesArray = Array.isArray(workflowData.variables)
729-
? workflowData.variables
730-
: Object.values(workflowData.variables)
731-
732-
if (variablesArray.length > 0) {
733-
type WorkflowVariablesBodyInput = NonNullable<
734-
Parameters<typeof requestJson<typeof workflowVariablesContract>>[1]['body']
735-
>
736-
const variablesRecord: WorkflowVariablesBodyInput['variables'] = {}
737-
738-
for (const variable of variablesArray) {
739-
const id =
740-
typeof variable.id === 'string' && variable.id.trim() ? variable.id : generateId()
741-
742-
variablesRecord[id] = {
743-
id,
744-
name: variable.name,
745-
type: variable.type,
746-
value: variable.value,
747-
}
748-
}
749-
750-
try {
751-
await requestJson(workflowVariablesContract, {
752-
params: { id: newWorkflowId },
753-
body: { variables: variablesRecord },
754-
})
755-
} catch {
756-
throw new Error(`Failed to save variables for ${newWorkflowId}`)
757-
}
727+
const variablesRecord = normalizeImportedVariables(workflowData.variables)
728+
if (Object.keys(variablesRecord).length > 0) {
729+
try {
730+
await requestJson(workflowVariablesContract, {
731+
params: { id: newWorkflowId },
732+
body: { variables: variablesRecord },
733+
})
734+
} catch {
735+
throw new Error(`Failed to save variables for ${newWorkflowId}`)
758736
}
759737
}
760738

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* Pins the contract between the two halves of every workflow export/import
5+
* round trip: `parseWorkflowVariables` produces what exports emit, and
6+
* `normalizeImportedVariables` consumes what imports receive. A shape the first
7+
* emits that the second drops is silent data loss on a restore, which is
8+
* exactly the bug the workspace importer shipped with.
9+
*/
10+
11+
import { describe, expect, it } from 'vitest'
12+
import { normalizeImportedVariables, parseWorkflowVariables } from '@/lib/workflows/variables/parse'
13+
14+
const VARIABLE = {
15+
id: 'var-1',
16+
name: 'apiHost',
17+
type: 'string' as const,
18+
value: 'https://example.com',
19+
}
20+
21+
describe('parseWorkflowVariables', () => {
22+
it('returns undefined for null so callers can omit the field', () => {
23+
expect(parseWorkflowVariables(null)).toBeUndefined()
24+
})
25+
26+
it('reads the current record form', () => {
27+
expect(parseWorkflowVariables({ 'var-1': VARIABLE })).toEqual({ 'var-1': VARIABLE })
28+
})
29+
30+
it('re-keys the legacy array form by variable id', () => {
31+
expect(parseWorkflowVariables([VARIABLE] as never)).toEqual({ 'var-1': VARIABLE })
32+
})
33+
34+
it('parses a JSON string column value', () => {
35+
expect(parseWorkflowVariables(JSON.stringify({ 'var-1': VARIABLE }) as never)).toEqual({
36+
'var-1': VARIABLE,
37+
})
38+
})
39+
40+
it('skips legacy array rows without a usable id instead of emitting an "undefined" key', () => {
41+
const result = parseWorkflowVariables([VARIABLE, { name: 'orphan' }, null] as never)
42+
43+
expect(Object.keys(result ?? {})).toEqual(['var-1'])
44+
})
45+
})
46+
47+
describe('normalizeImportedVariables', () => {
48+
it('accepts the record form', () => {
49+
expect(normalizeImportedVariables({ 'var-1': VARIABLE })).toEqual({ 'var-1': VARIABLE })
50+
})
51+
52+
it('accepts the legacy array form', () => {
53+
expect(normalizeImportedVariables([VARIABLE])).toEqual({ 'var-1': VARIABLE })
54+
})
55+
56+
it('returns an empty record for null, undefined and non-objects', () => {
57+
expect(normalizeImportedVariables(null)).toEqual({})
58+
expect(normalizeImportedVariables(undefined)).toEqual({})
59+
expect(normalizeImportedVariables('nope')).toEqual({})
60+
})
61+
62+
it('falls back to the map key when an entry carries no id', () => {
63+
expect(normalizeImportedVariables({ fromKey: { name: 'x', value: 1 } })).toMatchObject({
64+
fromKey: { id: 'fromKey', name: 'x' },
65+
})
66+
})
67+
68+
it('coerces an unrecognized type to string rather than persisting it', () => {
69+
const result = normalizeImportedVariables({ v: { ...VARIABLE, type: 'secret' } })
70+
71+
expect(result['var-1'].type).toBe('string')
72+
})
73+
74+
it('keeps a __proto__-keyed variable as an ordinary own property', () => {
75+
/**
76+
* Built via `JSON.parse` rather than a literal: an object literal's
77+
* `__proto__` sets the prototype, while `JSON.parse` creates a real own
78+
* key — and `JSON.parse` is how an import payload actually arrives.
79+
*/
80+
const payload = JSON.parse('{"__proto__": {"name": "sneaky", "value": 1}}')
81+
82+
const result = normalizeImportedVariables(payload)
83+
84+
expect(Object.keys(result)).toContain('__proto__')
85+
expect(Object.getPrototypeOf(result)).toBe(Object.prototype)
86+
})
87+
88+
it('trims a padded id so the key is referenceable', () => {
89+
expect(Object.keys(normalizeImportedVariables([{ ...VARIABLE, id: ' var-1 ' }]))).toEqual([
90+
'var-1',
91+
])
92+
})
93+
94+
it('skips non-object entries instead of writing junk variables', () => {
95+
expect(normalizeImportedVariables(['nope', null, VARIABLE])).toEqual({ 'var-1': VARIABLE })
96+
})
97+
})
98+
99+
describe('export -> import variable round trip', () => {
100+
/**
101+
* The regression guard. Workspace export writes whatever
102+
* `parseWorkflowVariables` returns — the record form — and the importer used
103+
* to guard on `Array.isArray`, so every variable was silently dropped on
104+
* restore.
105+
*/
106+
it('survives the record form that exports actually emit', () => {
107+
const exported = parseWorkflowVariables({ 'var-1': VARIABLE })
108+
109+
expect(exported).toBeDefined()
110+
expect(Array.isArray(exported)).toBe(false)
111+
expect(normalizeImportedVariables(exported)).toEqual({ 'var-1': VARIABLE })
112+
})
113+
114+
it('survives a legacy array-form export', () => {
115+
const exported = parseWorkflowVariables([VARIABLE] as never)
116+
117+
expect(normalizeImportedVariables(exported)).toEqual({ 'var-1': VARIABLE })
118+
})
119+
})

0 commit comments

Comments
 (0)