mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-11 08:07:15 +00:00
fix(frontend): prevent duplicate asset node ids crashing flow graph (#9367)
This commit is contained in:
@@ -63,7 +63,7 @@
|
||||
type: 'asset' as const,
|
||||
parentId: node.id,
|
||||
data: { asset, displayedAccessType: 'r' },
|
||||
id: `${node.id}-asset-in-${asset.kind}-${asset.path}`,
|
||||
id: `${node.id}-asset-in-${asset.kind}-${asset.path}-${i}`,
|
||||
width: inputAssetWidth,
|
||||
position: {
|
||||
x:
|
||||
@@ -100,7 +100,7 @@
|
||||
type: 'asset' as const,
|
||||
parentId: node.id,
|
||||
data: { asset, displayedAccessType: 'w' },
|
||||
id: `${node.id}-asset-out-${asset.kind}-${asset.path}`,
|
||||
id: `${node.id}-asset-out-${asset.kind}-${asset.path}-${i}`,
|
||||
width: outputAssetWidth,
|
||||
position: {
|
||||
x:
|
||||
@@ -136,7 +136,7 @@
|
||||
allAssetNodes.push(...(inputAssetNodes ?? []), ...(outputAssetNodes ?? []))
|
||||
|
||||
// If there are more than 3 assets, we create an overflow node
|
||||
if (overflowedInputAssets.length)
|
||||
if (overflowedInputAssets.length) {
|
||||
allAssetNodes.push({
|
||||
type: 'assetsOverflowed',
|
||||
data: { overflowedAssets: overflowedInputAssets, displayedAccessType: 'r' },
|
||||
@@ -148,14 +148,15 @@
|
||||
y: READ_ASSET_Y_OFFSET
|
||||
}
|
||||
} satisfies Node & AssetsOverflowedN)
|
||||
allAssetEdges.push({
|
||||
id: `${node.id}-assets-overflowed-in-edge`,
|
||||
source: `${node.id}-assets-overflowed-in`,
|
||||
target: node.id,
|
||||
type: 'empty',
|
||||
data: { class: '!opacity-35 dark:!opacity-20' }
|
||||
})
|
||||
if (overflowedOutputAssets.length)
|
||||
allAssetEdges.push({
|
||||
id: `${node.id}-assets-overflowed-in-edge`,
|
||||
source: `${node.id}-assets-overflowed-in`,
|
||||
target: node.id,
|
||||
type: 'empty',
|
||||
data: { class: '!opacity-35 dark:!opacity-20' }
|
||||
})
|
||||
}
|
||||
if (overflowedOutputAssets.length) {
|
||||
allAssetNodes.push({
|
||||
type: 'assetsOverflowed',
|
||||
data: { overflowedAssets: overflowedOutputAssets, displayedAccessType: 'w' },
|
||||
@@ -167,13 +168,14 @@
|
||||
y: WRITE_ASSET_Y_OFFSET
|
||||
}
|
||||
} satisfies Node & AssetsOverflowedN)
|
||||
allAssetEdges.push({
|
||||
id: `${node.id}-assets-overflowed-out-edge`,
|
||||
source: node.id,
|
||||
target: `${node.id}-assets-overflowed-out`,
|
||||
type: 'empty',
|
||||
data: { class: '!opacity-35 dark:!opacity-25' }
|
||||
})
|
||||
allAssetEdges.push({
|
||||
id: `${node.id}-assets-overflowed-out-edge`,
|
||||
source: node.id,
|
||||
target: `${node.id}-assets-overflowed-out`,
|
||||
type: 'empty',
|
||||
data: { class: '!opacity-35 dark:!opacity-25' }
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
let ret: ReturnType<typeof computeAssetNodes> = {
|
||||
@@ -274,8 +276,8 @@
|
||||
<Tooltip class={'pr-1 flex items-center justify-center'}>
|
||||
<AlertTriangle size={16} class="text-orange-500" />
|
||||
{#snippet text()}
|
||||
Could not find resource
|
||||
{/snippet}
|
||||
Could not find resource
|
||||
{/snippet}
|
||||
</Tooltip>
|
||||
{:else if isSelected && assetCanBeExplored(data.asset, cachedResourceMetadata) && !$userStore?.operator}
|
||||
<div transition:slide={{ axis: 'x', duration: 100 }}>
|
||||
@@ -291,29 +293,27 @@
|
||||
{/if}
|
||||
</div>
|
||||
{#snippet text()}
|
||||
|
||||
{#if usageCount !== undefined}
|
||||
Used in {pluralize(usageCount, 'step')}<br />
|
||||
{/if}
|
||||
<a
|
||||
href={undefined}
|
||||
class={twMerge(
|
||||
'text-xs',
|
||||
data.asset.kind === 'resource' ? 'text-accent cursor-pointer' : 'text-hint'
|
||||
)}
|
||||
onclick={() => {
|
||||
if (data.asset.kind === 'resource')
|
||||
flowGraphAssetsCtx?.val.resourceEditorDrawer?.initEdit(data.asset.path)
|
||||
}}
|
||||
>
|
||||
{data.asset.path}
|
||||
</a><br />
|
||||
<span class="text-hint text-xs">
|
||||
{formatAssetKind({ ...data.asset, metadata: cachedResourceMetadata })}</span
|
||||
>
|
||||
<AssetColumnBadges columns={assetColumns} disableTooltip />
|
||||
|
||||
{/snippet}
|
||||
{#if usageCount !== undefined}
|
||||
Used in {pluralize(usageCount, 'step')}<br />
|
||||
{/if}
|
||||
<a
|
||||
href={undefined}
|
||||
class={twMerge(
|
||||
'text-xs',
|
||||
data.asset.kind === 'resource' ? 'text-accent cursor-pointer' : 'text-hint'
|
||||
)}
|
||||
onclick={() => {
|
||||
if (data.asset.kind === 'resource')
|
||||
flowGraphAssetsCtx?.val.resourceEditorDrawer?.initEdit(data.asset.path)
|
||||
}}
|
||||
>
|
||||
{data.asset.path}
|
||||
</a><br />
|
||||
<span class="text-hint text-xs">
|
||||
{formatAssetKind({ ...data.asset, metadata: cachedResourceMetadata })}</span
|
||||
>
|
||||
<AssetColumnBadges columns={assetColumns} disableTooltip />
|
||||
{/snippet}
|
||||
</Tooltip>
|
||||
{/snippet}
|
||||
</NodeWrapper>
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
import { describe, it, expect, vi } from 'vitest'
|
||||
|
||||
// Mock heavy transitive imports pulled in by AssetNode.svelte's instance script
|
||||
vi.mock('monaco-editor', () => ({}))
|
||||
vi.mock('$lib/components/meltComponents', () => ({ Tooltip: {} }))
|
||||
vi.mock('../../../ExploreAssetButton.svelte', () => ({
|
||||
default: {},
|
||||
assetCanBeExplored: () => false
|
||||
}))
|
||||
vi.mock('$lib/components/icons/AssetGenericIcon.svelte', () => ({ default: {} }))
|
||||
vi.mock('$lib/components/assets/AssetColumnBadges.svelte', () => ({ default: {} }))
|
||||
vi.mock('./NodeWrapper.svelte', () => ({ default: {} }))
|
||||
|
||||
import { computeAssetNodes } from './AssetNode.svelte'
|
||||
|
||||
function nodeWithAssets(id: string, assets: any[]) {
|
||||
return { id, position: { x: 0, y: 0 }, data: { assets } }
|
||||
}
|
||||
|
||||
describe('computeAssetNodes (WIN-1998)', () => {
|
||||
it('produces unique node and edge ids when a module lists the same asset twice', () => {
|
||||
// Two assets with identical kind+path (e.g. read twice, or r + rw) — both
|
||||
// display as inputs. Before the fix these collided on the same node id and
|
||||
// crashed SvelteFlow with `each_key_duplicate`.
|
||||
const dup = { kind: 'resource', path: 'f/foo/bar', access_type: 'r' }
|
||||
const { newAssetNodes, newAssetEdges } = computeAssetNodes([
|
||||
nodeWithAssets('moduleA', [{ ...dup }, { ...dup }])
|
||||
])
|
||||
|
||||
const nodeIds = newAssetNodes.map((n) => n.id)
|
||||
expect(new Set(nodeIds).size).toBe(nodeIds.length)
|
||||
|
||||
const edgeIds = newAssetEdges.map((e) => e.id)
|
||||
expect(new Set(edgeIds).size).toBe(edgeIds.length)
|
||||
})
|
||||
|
||||
it('does not emit overflow edges when there is no overflow node (<=3 assets)', () => {
|
||||
const { newAssetNodes, newAssetEdges } = computeAssetNodes([
|
||||
nodeWithAssets('moduleB', [{ kind: 'resource', path: 'f/a/x', access_type: 'r' }])
|
||||
])
|
||||
|
||||
// No overflow node should be created for a single asset...
|
||||
expect(newAssetNodes.some((n) => n.type === 'assetsOverflowed')).toBe(false)
|
||||
// ...and therefore no dangling edge should reference a missing overflow node.
|
||||
const nodeIdSet = new Set(newAssetNodes.map((n) => n.id).concat('moduleB'))
|
||||
for (const e of newAssetEdges) {
|
||||
expect(nodeIdSet.has(e.source as string)).toBe(true)
|
||||
expect(nodeIdSet.has(e.target as string)).toBe(true)
|
||||
}
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user