From a1ab815fc4a4645d39443ee11f76973ef1e1517d Mon Sep 17 00:00:00 2001 From: Ivan Starkov Date: Fri, 13 Sep 2024 12:32:07 +0300 Subject: [PATCH] fix: Inserting Slots within Slots from the Marketplace causes infinite loop and crash (#4113) ## Description closes #4111 - [x] - Fixes client side - [x] - Fixes server side https://github.com/user-attachments/assets/8bb6ffdc-2e78-45eb-b98b-e697daee822a ## Steps for reproduction Now seems like impossible to save broken project. Tested locally on broken project. ## Code Review - [ ] hi @kof, I need you to do - conceptual review (architecture, feature-correctness) - detailed review (read every line) - test it on preview ## Before requesting a review - [ ] made a self-review - [ ] added inline comments where things may be not obvious (the "why", not "what") ## Before merging - [ ] tested locally and on preview environment (preview dev login: 5de6) - [ ] updated [test cases](https://github.com/webstudio-is/webstudio/blob/main/apps/builder/docs/test-cases.md) document - [ ] added tests - [ ] if any new env variables are added, added them to `.env` file --- apps/builder/app/routes/rest.patch.ts | 25 ++- apps/builder/app/shared/instance-utils.ts | 13 ++ apps/builder/app/shared/sync/sync-stores.ts | 3 +- packages/project-build/src/db/build.ts | 21 ++- packages/project-build/src/index.ts | 1 + .../src/shared/graph-utils.test.ts | 147 ++++++++++++++++++ .../project-build/src/shared/graph-utils.ts | 93 +++++++++++ 7 files changed, 299 insertions(+), 4 deletions(-) create mode 100644 packages/project-build/src/shared/graph-utils.test.ts create mode 100644 packages/project-build/src/shared/graph-utils.ts diff --git a/apps/builder/app/routes/rest.patch.ts b/apps/builder/app/routes/rest.patch.ts index 06f8d4feedc7..aff5be010f30 100644 --- a/apps/builder/app/routes/rest.patch.ts +++ b/apps/builder/app/routes/rest.patch.ts @@ -18,7 +18,11 @@ import { Resources, Resource, } from "@webstudio-is/sdk"; -import { type Build, MarketplaceProduct } from "@webstudio-is/project-build"; +import { + type Build, + findCycles, + MarketplaceProduct, +} from "@webstudio-is/project-build"; import { parsePages, parseStyleSourceSelections, @@ -31,6 +35,7 @@ import { parseConfig, serializeConfig, loadRawBuildById, + parseInstanceData, } from "@webstudio-is/project-build/index.server"; import { patchAssets } from "@webstudio-is/asset-uploader/index.server"; import type { Project } from "@webstudio-is/project"; @@ -183,8 +188,24 @@ export const action = async ({ if (namespace === "instances") { const instances = - buildData.instances ?? parseData(build.instances); + buildData.instances ?? parseInstanceData(build.instances); + buildData.instances = applyPatches(instances, patches); + + // Detect cycles + const cycles = findCycles(buildData.instances.values()); + if (cycles.length > 0) { + console.error( + "Cycles detected in the instance tree after patching", + cycles + ); + + return { + status: "error", + errors: "Cycles detected in the instance tree", + }; + } + continue; } diff --git a/apps/builder/app/shared/instance-utils.ts b/apps/builder/app/shared/instance-utils.ts index ce4f57709eb4..9c006f0e3bd8 100644 --- a/apps/builder/app/shared/instance-utils.ts +++ b/apps/builder/app/shared/instance-utils.ts @@ -62,6 +62,7 @@ import { isBaseBreakpoint } from "./breakpoints"; import { humanizeString } from "./string-utils"; import { serverSyncStore } from "./sync"; import { setDifference, setUnion } from "./shim"; +import { breakCyclesMutable, findCycles } from "@webstudio-is/project-build"; export const updateWebstudioData = (mutate: (data: WebstudioData) => void) => { serverSyncStore.createTransaction( @@ -105,6 +106,18 @@ export const updateWebstudioData = (mutate: (data: WebstudioData) => void) => { styles, assets, }); + + const cycles = findCycles(instances.values()); + + // Detect and fix cycles in the instance tree, then report + if (cycles.length > 0) { + toast.info("Detected and fixed cycles in the instance tree."); + + breakCyclesMutable( + instances.values(), + (node) => node.component === "Slot" + ); + } } ); }; diff --git a/apps/builder/app/shared/sync/sync-stores.ts b/apps/builder/app/shared/sync/sync-stores.ts index 4f336aee105a..3806af1c6086 100644 --- a/apps/builder/app/shared/sync/sync-stores.ts +++ b/apps/builder/app/shared/sync/sync-stores.ts @@ -336,7 +336,6 @@ export const useBuilderStore = (publish: Publish) => { // @todo subscribe prematurely and compute initial changes // from current state whenever new app is connected unsubscribeStoresState = syncStoresState("builder", publish); - unsubscribeStoresChanges = syncStoresChanges("builder", publish); // immerhin data is sent only initially so not part of syncStoresState // expect data to be populated by the time effect is called const data = []; @@ -361,6 +360,8 @@ export const useBuilderStore = (publish: Publish) => { data, }, }); + + unsubscribeStoresChanges = syncStoresChanges("builder", publish); }); const unsubscribeDisconnect = subscribe("disconnect", () => { diff --git a/packages/project-build/src/db/build.ts b/packages/project-build/src/db/build.ts index aab43a7b3275..e4a81d7fe92d 100644 --- a/packages/project-build/src/db/build.ts +++ b/packages/project-build/src/db/build.ts @@ -25,10 +25,22 @@ import { parseDeployment } from "./deployment"; import { serializePages } from "./pages"; import { createDefaultPages } from "../shared/pages-utils"; import type { MarketplaceProduct } from "../shared//marketplace"; +import { breakCyclesMutable } from "../shared/graph-utils"; const parseCompactData = (serialized: string) => JSON.parse(serialized) as Item[]; +const parseCompactInstanceData = (serialized: string) => { + const instances = JSON.parse(serialized) as Instance[]; + + // @todo: Remove after measurements on real data + console.time("breakCyclesMutable"); + breakCyclesMutable(instances, (node) => node.component === "Slot"); + console.timeEnd("breakCyclesMutable"); + + return instances; +}; + export const parseData = ( string: string ): Map => { @@ -36,6 +48,13 @@ export const parseData = ( return new Map(list.map((item) => [item.id, item])); }; +export const parseInstanceData = ( + string: string +): Map => { + const list = parseCompactInstanceData(string); + return new Map(list.map((item) => [item.id, item])); +}; + export const serializeData = ( data: Map ) => { @@ -71,7 +90,7 @@ const parseCompactBuild = async ( props: parseCompactData(build.props), dataSources: parseCompactData(build.dataSources), resources: parseCompactData(build.resources), - instances: parseCompactData(build.instances), + instances: parseCompactInstanceData(build.instances), deployment: parseDeployment(build.deployment), marketplaceProduct: parseConfig( build.marketplaceProduct diff --git a/packages/project-build/src/index.ts b/packages/project-build/src/index.ts index 3a7df2eeef28..06c0552c418d 100644 --- a/packages/project-build/src/index.ts +++ b/packages/project-build/src/index.ts @@ -1,3 +1,4 @@ export * from "./types"; export * from "./shared/pages-utils"; export * from "./shared/marketplace"; +export * from "./shared/graph-utils"; diff --git a/packages/project-build/src/shared/graph-utils.test.ts b/packages/project-build/src/shared/graph-utils.test.ts new file mode 100644 index 000000000000..f744dfe8af36 --- /dev/null +++ b/packages/project-build/src/shared/graph-utils.test.ts @@ -0,0 +1,147 @@ +import { test, expect, describe } from "@jest/globals"; +import { findCycles, breakCyclesMutable } from "./graph-utils"; +import type { Instance } from "@webstudio-is/sdk"; + +const typeId = "id" as const; + +describe("findCycles", () => { + test("should return an empty array for an empty graph", () => { + const graph: Instance[] = []; + const result = findCycles(graph); + expect(result).toEqual([]); + }); + + test("should return an empty array for a graph with no cycles", () => { + const graph = [ + { id: "1", children: [{ type: typeId, value: "2" }] }, + { id: "2", children: [{ type: typeId, value: "3" }] }, + { id: "3", children: [] }, + ]; + const result = findCycles(graph); + expect(result).toEqual([]); + }); + + test("should return a single cycle for a graph with one cycle", () => { + const graph = [ + { id: "1", children: [{ type: typeId, value: "2" }] }, + { id: "2", children: [{ type: typeId, value: "3" }] }, + { id: "3", children: [{ type: typeId, value: "1" }] }, + ]; + const result = findCycles(graph); + expect(result).toEqual([["1", "2", "3", "1"]]); + }); + + test("should return multiple cycles for a graph with multiple cycles", () => { + const graph = [ + { id: "1", children: [{ type: typeId, value: "2" }] }, + { + id: "2", + children: [ + { type: typeId, value: "3" }, + { type: typeId, value: "4" }, + ], + }, + { id: "3", children: [{ type: typeId, value: "1" }] }, + { id: "4", children: [{ type: typeId, value: "2" }] }, + ]; + const result = findCycles(graph); + expect(result).toEqual([ + ["1", "2", "3", "1"], + ["2", "4", "2"], + ]); + }); + + test("should return multiple cycles for a graph with multiple inline cycles", () => { + const graph = [ + { id: "1", children: [{ type: typeId, value: "2" }] }, + { + id: "2", + children: [{ type: typeId, value: "3" }], + }, + { + id: "3", + children: [ + { type: typeId, value: "4" }, + { type: typeId, value: "2" }, + ], + }, + { id: "4", children: [{ type: typeId, value: "1" }] }, + ]; + + const result = findCycles(graph); + + expect(result).toEqual([ + ["1", "2", "3", "4", "1"], + ["2", "3", "2"], + ]); + }); +}); + +describe("breakCyclesMutable", () => { + test("should return the same instances for an empty graph", () => { + const result = breakCyclesMutable([], () => false); + + expect(result).toEqual([]); + }); + + test("should return the same instances for a graph with no cycles", () => { + const instances = [ + { id: "1", component: "Slot", children: [{ type: typeId, value: "2" }] }, + { id: "2", children: [{ type: typeId, value: "3" }] }, + { id: "3", children: [] }, + ]; + const result = breakCyclesMutable( + instances, + (node) => node?.component === "Slot" + ); + expect(result).toEqual(instances); + }); + + test("should break a single cycle in the graph", () => { + const instances = [ + { id: "1", children: [{ type: typeId, value: "2" }] }, + { id: "2", children: [{ type: typeId, value: "3" }] }, + { id: "3", component: "Slot", children: [{ type: typeId, value: "1" }] }, + ]; + + const result = breakCyclesMutable( + instances, + (node) => node?.component === "Slot" + ); + + expect(result).toEqual([ + { id: "1", children: [{ type: typeId, value: "2" }] }, + { id: "2", children: [] }, + { id: "3", component: "Slot", children: [{ type: typeId, value: "1" }] }, + ]); + }); + + test("should break multiple cycles in the graph", () => { + const instances = [ + { id: "1", children: [{ type: typeId, value: "2" }] }, + { + id: "2", + children: [ + { type: typeId, value: "3" }, + { type: typeId, value: "4" }, + ], + }, + { id: "3", component: "Slot", children: [{ type: typeId, value: "1" }] }, + { id: "4", component: "Slot", children: [{ type: typeId, value: "2" }] }, + ]; + + const result = breakCyclesMutable( + instances, + (node) => node?.component === "Slot" + ); + expect(result).toEqual([ + { id: "1", children: [{ type: typeId, value: "2" }] }, + { + id: "2", + children: [], + }, + { id: "3", component: "Slot", children: [{ type: typeId, value: "1" }] }, + { id: "4", component: "Slot", children: [{ type: typeId, value: "2" }] }, + ]); + }); +}); diff --git a/packages/project-build/src/shared/graph-utils.ts b/packages/project-build/src/shared/graph-utils.ts new file mode 100644 index 000000000000..92723ef3659e --- /dev/null +++ b/packages/project-build/src/shared/graph-utils.ts @@ -0,0 +1,93 @@ +import type { Instance } from "@webstudio-is/sdk"; + +type InstanceId = Instance["id"]; + +// Depth-First Search (DFS) algorithm to find cycles in a directed graph +export const findCycles = ( + graph: Iterable> +): InstanceId[][] => { + const adjacencyList: Record = {}; + + // Build adjacency list + for (const node of graph) { + adjacencyList[node.id] = node.children + .filter((child) => child.type === "id") + .map((child) => child.value); + } + + const visited = new Set(); + const path: InstanceId[] = []; + const cycles: InstanceId[][] = []; + + const dfs = (nodeId: string): void => { + if (path.includes(nodeId)) { + const cycleStart = path.indexOf(nodeId); + cycles.push(path.slice(cycleStart).concat(nodeId)); + return; + } + + if (visited.has(nodeId)) { + return; + } + + visited.add(nodeId); + path.push(nodeId); + + for (const childId of adjacencyList[nodeId] || []) { + dfs(childId); + } + + path.pop(); + }; + + // Start DFS from each node + for (const node of graph) { + if (!visited.has(node.id)) { + dfs(node.id); + } + } + + return cycles; +}; + +export const breakCyclesMutable = >( + instances: Iterable, + breakOn: (node: T) => boolean +) => { + const cycles = findCycles(instances); + if (cycles.length === 0) { + return instances; + } + + const cycleInstances = new Map(); + const cycleInstanceIdSet = new Set(cycles.flat()); + + // Pick all instances that are part of the cycle + for (const instance of instances) { + if (cycleInstanceIdSet.has(instance.id)) { + cycleInstances.set(instance.id, instance); + } + } + + for (const cycle of cycles) { + // Find slot or take last instance + const slotId = + cycle.find((id) => breakOn(cycleInstances.get(id)!)) ?? + cycle[cycle.length - 1]; + + // Remove slot from children of all instances in the cycle + for (const id of cycle) { + const instance = cycleInstances.get(id); + if (instance === undefined) { + continue; + } + + if (instance.children.find((child) => child.value === slotId)) { + instance.children = instance.children.filter( + (child) => child.value !== slotId + ); + } + } + } + return instances; +};