Skip to content

Fix unhydrated node parent tracking #24708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 27, 2025
Merged

Fix unhydrated node parent tracking #24708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f67f0b
Fix unhydrated node parent tracking
CraigMacomber May 23, 2025
f9d7a52
Update .changeset/eight-ears-feel.md
CraigMacomber May 23, 2025
dd919c4
Merge branch 'main' into UnhydratedParent
CraigMacomber May 23, 2025
e326108
Update comment
CraigMacomber May 27, 2025
631d94d
Deduplicate key2 test
CraigMacomber May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/eight-ears-feel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"@fluidframework/tree": minor
"fluid-framework": minor
"__section": tree
---
Fix Tree.key and Tree.parent for Unhydrated nodes after edits

Check failure on line 6 in .changeset/eight-ears-feel.md

View workflow job for this annotation

GitHub Actions / vale 

[vale] reported by reviewdog 🐶
[Vale.Spelling] Did you really mean 'Unhydrated'?

Raw Output:
{"message": "[Vale.Spelling] Did you really mean 'Unhydrated'?", "location": {"path": ".changeset/eight-ears-feel.md", "range": {"start": {"line": 6, "column": 34}}}, "severity": "ERROR"}

In some cases, editing [Unhydrated](https://fluidframework.com/docs/api/fluid-framework/unhydrated-typealias) nodes could result in incorrect results being returned from [Tree.key](https://fluidframework.com/docs/data-structures/tree/nodes#treekey) and [Tree.parent](https://fluidframework.com/docs/data-structures/tree/nodes#treeparent).

Check failure on line 8 in .changeset/eight-ears-feel.md

View workflow job for this annotation

GitHub Actions / vale 

[vale] reported by reviewdog 🐶
[Vale.Spelling] Did you really mean 'Unhydrated'?

Raw Output:
{"message": "[Vale.Spelling] Did you really mean 'Unhydrated'?", "location": {"path": ".changeset/eight-ears-feel.md", "range": {"start": {"line": 8, "column": 25}}}, "severity": "ERROR"}

Check warning on line 8 in .changeset/eight-ears-feel.md

View workflow job for this annotation

GitHub Actions / vale 

[vale] reported by reviewdog 🐶
[Microsoft.Wordiness] Consider using 'sometimes' instead of 'In some cases'.

Raw Output:
{"message": "[Microsoft.Wordiness] Consider using 'sometimes' instead of 'In some cases'.", "location": {"path": ".changeset/eight-ears-feel.md", "range": {"start": {"line": 8, "column": 1}}}, "severity": "INFO"}
This has been fixed.
26 changes: 12 additions & 14 deletions packages/dds/tree/src/simple-tree/core/unhydratedFlexTree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,13 +402,24 @@ class UnhydratedFlexTreeField implements FlexTreeField {
*/
protected edit(edit: (mapTrees: ExclusiveMapTree[]) => void | ExclusiveMapTree[]): void {
const oldMapTrees = this.parent.mapTree.fields.get(this.key) ?? [];

// Clear parents for all old map trees.
for (const tree of oldMapTrees) {
tryUnhydratedFlexTreeNode(tree)?.adoptBy(undefined);
}

const newMapTrees = edit(oldMapTrees) ?? oldMapTrees;
if (newMapTrees.length > 0) {
this.parent.mapTree.fields.set(this.key, newMapTrees);
} else {
this.parent.mapTree.fields.delete(this.key);
}

// Set parents for all new map trees.
for (const [index, tree] of newMapTrees.entries()) {
tryUnhydratedFlexTreeNode(tree)?.adoptBy(this, index);
}

this.onEdit?.();
}

Expand All @@ -434,16 +445,6 @@ class EagerMapTreeOptionalField
{
public readonly editor = {
set: (newContent: ExclusiveMapTree | undefined): void => {
// If the new content is a UnhydratedFlexTreeNode, it needs to have its parent pointer updated
if (newContent !== undefined) {
nodeCache.get(newContent)?.adoptBy(this, 0);
}
// If the old content is a UnhydratedFlexTreeNode, it needs to have its parent pointer unset
const oldContent = this.mapTrees[0];
if (oldContent !== undefined) {
nodeCache.get(oldContent)?.adoptBy(undefined);
}

this.edit((mapTrees) => {
if (newContent !== undefined) {
mapTrees[0] = newContent;
Expand Down Expand Up @@ -484,10 +485,8 @@ export class UnhydratedTreeSequenceField
{
public readonly editor: UnhydratedTreeSequenceFieldEditBuilder = {
insert: (index, newContent): void => {
for (let i = 0; i < newContent.length; i++) {
const c = newContent[i];
for (const c of newContent) {
assert(c !== undefined, 0xa0a /* Unexpected sparse array content */);
nodeCache.get(c)?.adoptBy(this, index + i);
}
this.edit((mapTrees) => {
if (newContent.length < 1000) {
Expand All @@ -503,7 +502,6 @@ export class UnhydratedTreeSequenceField
for (let i = index; i < index + count; i++) {
const c = this.mapTrees[i];
assert(c !== undefined, 0xa0b /* Unexpected sparse array */);
nodeCache.get(c)?.adoptBy(undefined);
}
let removed: ExclusiveMapTree[] | undefined;
this.edit((mapTrees) => {
Expand Down
125 changes: 70 additions & 55 deletions packages/dds/tree/src/test/simple-tree/api/treeNodeApi.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import {
TestTreeProviderLite,
validateUsageError,
} from "../../utils.js";
import { getViewForForkedBranch, hydrate } from "../utils.js";
import { describeHydration, getViewForForkedBranch, hydrate } from "../utils.js";
import { brand, type areSafelyAssignable, type requireTrue } from "../../../util/index.js";

import {
Expand Down Expand Up @@ -165,39 +165,70 @@ describe("treeNodeApi", () => {
});
});

it("key", () => {
class Child extends schema.object("Child", {
x: Point,
y: schema.optional(Point, { key: "stable-y" }),
}) {}
const Root = schema.array(Child);
const config = new TreeViewConfiguration({ schema: Root });
const view = getView(config);
view.initialize([
{ x: {}, y: undefined },
{ x: {}, y: {} },
]);
const { root } = view;
assert.equal(Tree.key(root), rootFieldKey);
assert.equal(Tree.key(root[0]), 0);
assert.equal(Tree.key(root[0].x), "x");
assert.equal(Tree.key(root[1]), 1);
assert.equal(Tree.key(root[1].x), "x");
assert(root[1].y !== undefined);
assert.equal(Tree.key(root[1].y), "y");
});
describeHydration("upward path", (init) => {
for (const [name, keyApi] of [
["key", (n: TreeNode): string | undefined | number => Tree.key(n)],
["key2", (n: TreeNode): string | undefined | number => TreeAlpha.key2(n)],
] as const) {
it(name, () => {
class Child extends schema.object("Child", {
x: Point,
y: schema.optional(Point, { key: "stable-y" }),
}) {}
class Root extends schema.array("Root", Child) {}
const root = init(Root, [
{ x: {}, y: undefined },
{ x: {}, y: {} },
]);

it("parent", () => {
class Child extends schema.object("Child", { x: Point }) {}
const Root = schema.array(Child);
const config = new TreeViewConfiguration({ schema: Root });
const view = getView(config);
view.initialize([{ x: {} }, { x: {} }]);
const { root } = view;
assert.equal(Tree.parent(root), undefined);
assert.equal(Tree.parent(root[0]), root);
assert.equal(Tree.parent(root[1]), root);
assert.equal(Tree.parent(root[1].x), root[1]);
// This is this how we handle root keys.
// Seems odd for detached fields other than root to have `rootFieldKey` key though.
// Exactly which key is given in this case is undocumented, it could change in the future.
// TreeAlpha.key2 just gives undefined, which is documented.
const rootKey = name === "key" ? rootFieldKey : undefined;

assert.equal(keyApi(root), rootKey);
assert.equal(keyApi(root[0]), 0);
assert.equal(keyApi(root[0].x), "x");
assert.equal(keyApi(root[1]), 1);
assert.equal(keyApi(root[1].x), "x");
assert(root[1].y !== undefined);
assert.equal(keyApi(root[1].y), "y");

const added = new Child({ x: {}, y: {} });

assert.equal(keyApi(added), rootKey);

// Check index is updated after insert.
root.insertAtStart(added);
assert.equal(keyApi(root[2]), 2);
assert.equal(keyApi(added), 0);

// Check index is updated after removal.
root.removeRange(0, 1);
assert.equal(keyApi(root[1]), 1);
assert.equal(keyApi(added), rootKey);
});
}

it("parent", () => {
class Child extends schema.object("Child", { x: Point }) {}
class Root extends schema.array("Root", Child) {}
const root = init(Root, [{ x: {} }, { x: {} }]);

assert.equal(Tree.parent(root), undefined);
assert.equal(Tree.parent(root[0]), root);
assert.equal(Tree.parent(root[1]), root);
assert.equal(Tree.parent(root[1].x), root[1]);

const added = new Child({ x: {} });

assert.equal(Tree.parent(added), undefined);
root.insertAtStart(added);
assert.equal(Tree.parent(added), root);
root.removeRange(0, 1);
assert.equal(Tree.parent(added), undefined);
});
});

it("treeStatus", () => {
Expand All @@ -215,29 +246,13 @@ describe("treeNodeApi", () => {
assert.equal(Tree.status(root), TreeStatus.InDocument);
assert.equal(Tree.status(child), TreeStatus.Removed);
assert.equal(Tree.status(newChild), TreeStatus.InDocument);
// TODO: test Deleted status.
});

it("key2", () => {
class Child extends schema.object("Child", {
x: Point,
y: schema.optional(Point, { key: "stable-y" }),
}) {}
const Root = schema.array(Child);
const config = new TreeViewConfiguration({ schema: Root });
const view = getView(config);
view.initialize([
{ x: {}, y: undefined },
{ x: {}, y: {} },
]);
const { root } = view;
assert.equal(TreeAlpha.key2(root), undefined);
assert.equal(TreeAlpha.key2(root[0]), 0);
assert.equal(TreeAlpha.key2(root[0].x), "x");
assert.equal(TreeAlpha.key2(root[1]), 1);
assert.equal(TreeAlpha.key2(root[1].x), "x");
assert(root[1].y !== undefined);
assert.equal(TreeAlpha.key2(root[1].y), "y");
view.dispose();
assert.equal(Tree.status(root), TreeStatus.Deleted);
assert.equal(Tree.status(child), TreeStatus.Deleted);
assert.equal(Tree.status(newChild), TreeStatus.Deleted);

// TODO: test Deleted status when caused by removal from the tree + expiring from removed status.
});

describe("shortID", () => {
Expand Down
Loading