From e6b2b5a05ca337ceb5c6a9806683797a8ee4fe72 Mon Sep 17 00:00:00 2001 From: Bernhard Seefeld Date: Thu, 16 Oct 2025 12:44:03 -0700 Subject: [PATCH 1/2] fix(runner): inline data URIs before handling write redirects Move data URI inlining logic to occur before write redirect (alias) handling in normalizeAndDiff. This ensures the correct evaluation order when a data URI contains a redirect link. Previously, when newValue was a data URI containing a redirect, the code would: 1. Check currentValue for redirects (line 290) 2. Follow currentValue redirect if present 3. Process data URI inlining later (line 318) This caused the redirect within the data URI to be written to the wrong location - it would follow the currentValue redirect first, then inline the data URI, resulting in the redirect being written to the destination of the currentValue alias instead of to the intended target. The fix reorders the logic so data URIs are inlined immediately after unwrapping proxies and cells (line 212), before any redirect handling. This preserves the invariant that redirect handling at line 290 only applies when newValue is not itself an alias. The order is now: 1. Unwrap proxies/cells 2. Inline data URIs (exposing any contained redirects) 3. Handle redirects in newValue 4. Handle redirects in currentValue This ensures redirects contained in data URIs are properly recognized and handled as part of newValue, not incorrectly written through a currentValue redirect. --- packages/runner/src/data-updating.ts | 146 ++++++++++++++------------- 1 file changed, 76 insertions(+), 70 deletions(-) diff --git a/packages/runner/src/data-updating.ts b/packages/runner/src/data-updating.ts index 53a915268..d5f576bf8 100644 --- a/packages/runner/src/data-updating.ts +++ b/packages/runner/src/data-updating.ts @@ -24,6 +24,11 @@ import { import { type IRuntime } from "./runtime.ts"; import { toURI } from "./uri-utils.ts"; +const diffLogger = getLogger("normalizeAndDiff", { + enabled: false, + level: "debug", +}); + /** * Traverses newValue and updates `current` and any relevant linked documents. * @@ -86,34 +91,6 @@ type ChangeSet = { * @param context - The context of the change. * @returns An array of changes that should be written. */ -const diffLogger = getLogger("normalizeAndDiff", { - enabled: false, - level: "debug", -}); - -/** - * Returns true if `target` is the immediate parent of `base` in the same document. - * - * Example: - * - base.path = ["internal", "__#1", "next"] - * - target.path = ["internal", "__#1"] - * - * This is used to decide when to collapse a self/parent link that would create - * a tight self-loop (e.g., obj.next -> obj) while allowing references to - * higher ancestors (like an item's `items` pointing to its containing array). - */ -function isImmediateParent( - target: NormalizedFullLink, - base: NormalizedFullLink, -): boolean { - return ( - target.id === base.id && - target.space === base.space && - target.path.length === base.path.length - 1 && - target.path.every((seg, i) => seg === base.path[i]) - ); -} - export function normalizeAndDiff( runtime: IRuntime, tx: IExtendedStorageTransaction, @@ -232,6 +209,54 @@ export function normalizeAndDiff( newValue = newValue.getAsLink(); } + // Check for links that are data: URIs and inline them, by calling + // normalizeAndDiff on the contents of the link. + if (isLink(newValue)) { + const parsedLink = parseLink(newValue, link); + if (parsedLink.id.startsWith("data:")) { + diffLogger.debug(() => + `[BRANCH_CELL_LINK] Data link detected, treating as contents at path=${pathStr}` + ); + // Use the tx code to make sure we read it the same way + let dataValue: any = tx.readValueOrThrow({ + ...parsedLink, + path: [], + }, options); + const path = [...parsedLink.path]; + // If there is a link on the way to `path`, follow it, appending remaining + // path to the target link. + for (;;) { + if (isAnyCellLink(dataValue)) { + const dataLink = parseLink(dataValue, parsedLink); + dataValue = createSigilLinkFromParsedLink({ + ...dataLink, + path: [...dataLink.path, ...path], + }); + break; + } + if (path.length > 0) { + if (isRecord(dataValue)) { + dataValue = dataValue[path.shift()!]; + } else { + dataValue = undefined; + break; + } + } else { + break; + } + } + return normalizeAndDiff( + runtime, + tx, + link, + dataValue, + context, + options, + seen, + ); + } + } + // If we're about to create a reference to ourselves, no-op if (areMaybeLinkAndNormalizedLinkSame(newValue, link)) { diffLogger.debug(() => @@ -318,48 +343,6 @@ export function normalizeAndDiff( seen, ); } - if (parsedLink.id.startsWith("data:")) { - diffLogger.debug(() => - `[BRANCH_CELL_LINK] Data link detected, treating as contents at path=${pathStr}` - ); - // If there is a data link treat it as writing it's contents instead. - - // Use the tx code to make sure we read it the same way - let dataValue: any = tx.readValueOrThrow({ - ...parsedLink, - path: [], - }, options); - const path = [...parsedLink.path]; - for (;;) { - if (isAnyCellLink(dataValue)) { - const dataLink = parseLink(dataValue, parsedLink); - dataValue = createSigilLinkFromParsedLink({ - ...dataLink, - path: [...dataLink.path, ...path], - }); - break; - } - if (path.length > 0) { - if (isRecord(dataValue)) { - dataValue = dataValue[path.shift()!]; - } else { - dataValue = undefined; - break; - } - } else { - break; - } - } - return normalizeAndDiff( - runtime, - tx, - link, - dataValue, - context, - options, - seen, - ); - } if ( isAnyCellLink(currentValue) && areLinksSame(newValue, currentValue, link) @@ -634,3 +617,26 @@ export function addCommonIDfromObjectID( traverse(obj); } + +/** + * Returns true if `target` is the immediate parent of `base` in the same document. + * + * Example: + * - base.path = ["internal", "__#1", "next"] + * - target.path = ["internal", "__#1"] + * + * This is used to decide when to collapse a self/parent link that would create + * a tight self-loop (e.g., obj.next -> obj) while allowing references to + * higher ancestors (like an item's `items` pointing to its containing array). + */ +function isImmediateParent( + target: NormalizedFullLink, + base: NormalizedFullLink, +): boolean { + return ( + target.id === base.id && + target.space === base.space && + target.path.length === base.path.length - 1 && + target.path.every((seg, i) => seg === base.path[i]) + ); +} From 1f261c7141a9193417b3dfff92490ea0da28ff92 Mon Sep 17 00:00:00 2001 From: Bernhard Seefeld Date: Thu, 16 Oct 2025 12:48:29 -0700 Subject: [PATCH 2/2] added test --- packages/runner/test/data-updating.test.ts | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/packages/runner/test/data-updating.test.ts b/packages/runner/test/data-updating.test.ts index ab5220429..3f2792036 100644 --- a/packages/runner/test/data-updating.test.ts +++ b/packages/runner/test/data-updating.test.ts @@ -13,6 +13,7 @@ import { areNormalizedLinksSame, createSigilLinkFromParsedLink, isAnyCellLink, + isSigilLink, parseLink, } from "../src/link-utils.ts"; import { type IExtendedStorageTransaction } from "../src/storage/interface.ts"; @@ -1095,6 +1096,80 @@ describe("data-updating", () => { expect(value.result).toBe(100); }); + it("should inline data URI containing redirect without writing redirect to wrong location", () => { + // Setup: Create two separate cells - source and destination + const sourceCell = runtime.getCell<{ value: number }>( + space, + "data URI redirect source value", + undefined, + tx, + ); + sourceCell.set({ value: 99 }); + + const destinationCell = runtime.getCell<{ value: number }>( + space, + "data URI redirect destination value", + undefined, + tx, + ); + destinationCell.set({ value: 42 }); + + // Create a data URI that contains a redirect pointing to sourceCell + const redirectAlias = sourceCell.key("value").getAsWriteRedirectLink(); + const dataCell = runtime.getImmutableCell( + space, + redirectAlias, + undefined, + tx, + ); + + // Create a target cell that currently has an alias to destinationCell + const targetCell = runtime.getCell<{ result: any }>( + space, + "data URI redirect target cell", + undefined, + tx, + ); + targetCell.setRaw({ + result: destinationCell.key("value").getAsWriteRedirectLink(), + }); + + const current = targetCell.key("result").getAsNormalizedFullLink(); + + // Write the data cell (which contains a redirect to sourceCell) to the target + // Before the fix: data URI was not inlined early enough, and the redirect + // would be written to destinationCell.value instead of target.result + // After the fix: data URI is inlined first, exposing the redirect, which is + // then properly written to target.result + const changes = normalizeAndDiff( + runtime, + tx, + current, + dataCell.getAsLink(), + ); + + // Should write the new redirect to target Cell.result + // Note: The change writes a redirect (alias object with $alias key) + expect(changes.length).toBe(1); + expect(changes[0].location.id).toBe( + targetCell.getAsNormalizedFullLink().id, + ); + expect(changes[0].location.path).toEqual(["result"]); + // The value should be the redirect link + expect(isSigilLink(changes[0].value)).toBe(true); + const parsedLink = parseLink(changes[0].value); + expect(parsedLink?.overwrite).toBe("redirect"); + + applyChangeSet(tx, changes); + + // Verify that targetCell now points to sourceCell's value (99), not destinationCell's (42) + expect(targetCell.get().result).toBe(99); + + // Verify neither source nor destination cells were modified + expect(sourceCell.get()).toEqual({ value: 99 }); + expect(destinationCell.get()).toEqual({ value: 42 }); + }); + describe("addCommonIDfromObjectID", () => { it("should handle arrays", () => { const obj = { items: [{ id: "item1", name: "First Item" }] };