Skip to content

Commit

Permalink
tree: Cleanup object property tests, and fix bug (microsoft#22748)
Browse files Browse the repository at this point in the history
## Description

This cleans up a section of the object node tests which used schema
types in a complicated generic way that's fragile and hard to work with.

When rewriting these tests, I focused the new ones on testing aspects
that actually have special logic and are likely to break instead of just
different value types. Thus the tests now cover the odd normalization
cases of numbers.

These tests found a couple of issues:
- unhydrated node handling of null was incorrect (see changeset)
- Some type errors were thrown for invalid user input. TO help keep it
easy to tell which errors are our bugs and which are app bugs I've made
these fluid usage errors.
- A needless check for NaN was included where the check for isFinaite
would handle it correctly (Number.isFinite considers NaNs to not be
finite: comment already calls out NaN and it has test coverage so this
seems like a safe change.)
  • Loading branch information
CraigMacomber authored Oct 8, 2024
1 parent a8d7871 commit 6a75bd0
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 37 deletions.
12 changes: 12 additions & 0 deletions .changeset/curvy-tables-kiss.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"@fluidframework/tree": minor
"fluid-framework": minor
---
---
"section": tree
---

Fix reading of `null` from unhydrated trees

Unhydrated trees containing object nodes with required fields set to `null` used to throw an error.
This was a bug: `null` is a valid value in tree's whose schema allow it, and this specific case now correctly returns `null` values when appropriate without erroring.
7 changes: 6 additions & 1 deletion packages/dds/tree/src/simple-tree/core/unhydratedFlexTree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,12 @@ class EagerMapTreeRequiredField
implements FlexTreeRequiredField
{
public override get content(): FlexTreeUnknownUnboxed {
return super.content ?? fail("Expected EagerMapTree required field to have a value");
// This cannot use ?? since null is a legal value here.
assert(
super.content !== undefined,
"Expected EagerMapTree required field to have a value",
);
return super.content;
}
}

Expand Down
6 changes: 3 additions & 3 deletions packages/dds/tree/src/simple-tree/toMapTree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,14 +262,14 @@ function mapValueWithFallbacks(
// Our serialized data format does not support -0.
// Map such input to +0.
return 0;
} else if (Number.isNaN(value) || !Number.isFinite(value)) {
} else if (!Number.isFinite(value)) {
// Our serialized data format does not support NaN nor +/-∞.
// If the schema supports `null`, fall back to that. Otherwise, throw.
// This is intended to match JSON's behavior for such values.
if (allowedTypes.has(nullSchema)) {
return null;
} else {
throw new TypeError(`Received unsupported numeric value: ${value}.`);
throw new UsageError(`Received unsupported numeric value: ${value}.`);
}
} else {
return value;
Expand All @@ -287,7 +287,7 @@ function mapValueWithFallbacks(
}
}
default:
throw new TypeError(`Received unsupported leaf value: ${value}.`);
throw new UsageError(`Received unsupported leaf value: ${value}.`);
}
}

Expand Down
88 changes: 55 additions & 33 deletions packages/dds/tree/src/test/simple-tree/objectNode.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ import {
typeNameSymbol,
typeSchemaSymbol,
type NodeBuilderData,
type NodeKind,
type TreeNodeSchema,
} from "../../simple-tree/index.js";

import { describeHydration, hydrate, pretty } from "./utils.js";
Expand Down Expand Up @@ -207,41 +205,65 @@ describeHydration(
});
});

// Regression test for accidental use of ?? preventing null values from being read correctly.
it("can read null field", () => {
class Root extends schemaFactory.object("", {
x: schemaFactory.null,
}) {}
const node = init(Root, { x: null });
assert.equal(node.x, null);
});

describe("supports setting", () => {
describe("primitives", () => {
function check<const TNode>(
schema: TreeNodeSchema<string, NodeKind, TNode>,
before: TNode,
after: TNode,
) {
describe(`required ${typeof before} `, () => {
it(`(${pretty(before)} -> ${pretty(after)})`, () => {
const Root = schemaFactory.object("", { value: schema });
const root = init(Root, { value: before });
assert.equal(root.value, before);
root.value = after;
assert.equal(root.value, after);
});
});
it("required", () => {
class Root extends schemaFactory.object("", {
x: schemaFactory.number,
}) {}
const node = init(Root, { x: 5 });
assert.equal(node.x, 5);
node.x = 6;
assert.equal(node.x, 6);
});

describe(`optional ${typeof before}`, () => {
it(`(undefined -> ${pretty(before)} -> ${pretty(after)})`, () => {
const root = init(
schemaFactory.object("", { value: schemaFactory.optional(schema) }),
{ value: undefined },
);
assert.equal(root.value, undefined);
root.value = before;
assert.equal(root.value, before);
root.value = after;
assert.equal(root.value, after);
});
});
}
it("optional", () => {
class Root extends schemaFactory.object("", {
y: schemaFactory.optional(schemaFactory.number),
}) {}
const node = init(Root, {});
assert.equal(node.y, undefined);
node.y = 6;
assert.equal(node.y, 6);
node.y = undefined;
assert.equal(node.y, undefined);
});

it("invalid normalize numbers", () => {
class Root extends schemaFactory.object("", {
x: [schemaFactory.number, schemaFactory.null],
}) {}
const node = init(Root, { x: NaN });
assert.equal(node.x, null);
node.x = 6;
assert.equal(node.x, 6);
node.x = Infinity;
assert.equal(node.x, null);
node.x = -0;
assert(Object.is(node.x, 0));
});

check(schemaFactory.boolean, false, true);
check(schemaFactory.number, 0, 1);
check(schemaFactory.string, "", "!");
it("invalid numbers error", () => {
class Root extends schemaFactory.object("", {
x: schemaFactory.number,
}) {}
const node = init(Root, { x: 1 });
assert.throws(() => {
node.x = NaN;
}, validateUsageError(/NaN/));
assert.equal(node.x, 1);
node.x = -0;
assert(Object.is(node.x, 0));
});
});

describe("required object", () => {
Expand Down

0 comments on commit 6a75bd0

Please sign in to comment.