Skip to content

Commit 862a65e

Browse files
CraigMacomberCraigMacomber
authored
Better errors when constructing trees (#26388)
## Description Better errors when constructing trees ## Breaking Changes Anything depending on the error messages contents could be impacted.
1 parent 74a04e8 commit 862a65e

5 files changed

Lines changed: 103 additions & 51 deletions

File tree

‎.changeset/wild-ghosts-report.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
"@fluidframework/tree": minor
3+
"fluid-framework": minor
4+
"__section": tree
5+
---
6+
Improve error messages when failing to construct nodes
7+
8+
The error messages when constructing tree nodes have been improved.
9+
Several cases now list not only the schema identifiers, but also schema names which can help when there are identifier collisions and make it easier to find the implementations.
10+
Additionally some cases which did not include what schema were encountered and which were allowed now include both.

‎packages/dds/tree/src/simple-tree/unhydratedFlexTreeFromInsertable.ts‎

Lines changed: 51 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*/
55

66
import type { IFluidHandle } from "@fluidframework/core-interfaces";
7-
import { assert, fail } from "@fluidframework/core-utils/internal";
7+
import { assert, debugAssert, fail } from "@fluidframework/core-utils/internal";
88
import { UsageError } from "@fluidframework/telemetry-utils/internal";
99

1010
import { filterIterable, hasSingle, oneFromIterable } from "../util/index.js";
@@ -58,7 +58,9 @@ export function unhydratedFlexTreeFromInsertable<TIn extends InsertableContent |
5858
if (data === undefined) {
5959
// TODO: this code-path should support defaults
6060
if (normalizedFieldSchema.kind !== FieldKind.Optional) {
61-
throw new UsageError("Got undefined for non-optional field.");
61+
throw new UsageError(
62+
`Got undefined for non-optional field expecting one of ${quotedAllowedTypesWithNames(normalizedFieldSchema.allowedTypeSet)}`,
63+
);
6264
}
6365
return undefined as TIn extends undefined ? undefined : UnhydratedFlexTreeNode;
6466
}
@@ -71,6 +73,47 @@ export function unhydratedFlexTreeFromInsertable<TIn extends InsertableContent |
7173
return flexTree as TIn extends undefined ? undefined : UnhydratedFlexTreeNode;
7274
}
7375

76+
/**
77+
* Throw a usage error with a helpful message about `schema` not being in `allowedTypes` for insertable content.
78+
*/
79+
function allowedTypesInsertableSchemaError(
80+
allowedTypes: ReadonlySet<TreeNodeSchema>,
81+
schema: TreeNodeSchema,
82+
): never {
83+
debugAssert(
84+
() =>
85+
!allowedTypes.has(schema) ||
86+
"This function should only be called if the schema is not in the allowed types.",
87+
);
88+
const map = new Map([...allowedTypes].map((s) => [s.identifier, s]));
89+
const expected = map.get(schema.identifier);
90+
if (expected !== undefined) {
91+
throw new UsageError(
92+
`A node with schema ${quotedSchemaIdentifierWithName(schema)} was provided where a node with that identifier is allowed, but the actual schema required (${quotedSchemaIdentifierWithName(expected)}) is not the same schema object.
93+
TreeNodeSchema have significant object identity and thus the exact same object must be used as the schema when defining what nodes are allowed and when constructing the node to use.`,
94+
);
95+
}
96+
throw new UsageError(
97+
`Expected insertable for one of ${quotedAllowedTypesWithNames(allowedTypes)}. Got node with schema ${quotedSchemaIdentifierWithName(schema)}.
98+
Nodes are valid insertable objects, but only if their schema are in the allowed list.`,
99+
);
100+
}
101+
102+
/**
103+
* Gets a description of a schema for use in error messages.
104+
*/
105+
function quotedSchemaIdentifierWithName(schema: TreeNodeSchema): string {
106+
// eslint-disable-next-line @typescript-eslint/no-unsafe-function-type
107+
return `${JSON.stringify(schema.identifier)} (name: ${JSON.stringify((schema as Function).name)})`;
108+
}
109+
110+
/**
111+
* Gets a description of an allowedTypes for use in error messages.
112+
*/
113+
function quotedAllowedTypesWithNames(allowedTypes: Iterable<TreeNodeSchema>): string {
114+
return `[${[...allowedTypes].map(quotedSchemaIdentifierWithName).join(", ")}]`;
115+
}
116+
74117
/**
75118
* Copy content from `data` into a UnhydratedFlexTreeNode.
76119
*/
@@ -83,10 +126,12 @@ export function unhydratedFlexTreeFromInsertableNode(
83126
const inner = kernel.getInnerNodeIfUnhydrated();
84127
if (inner === undefined) {
85128
// The node is already hydrated, meaning that it already got inserted into the tree previously
86-
throw new UsageError("A node may not be inserted into the tree more than once");
129+
throw new UsageError(
130+
`A node with schema ${quotedSchemaIdentifierWithName(kernel.schema)} was inserted into the tree more than once. This is not supported.`,
131+
);
87132
} else {
88133
if (!allowedTypes.has(kernel.schema)) {
89-
throw new UsageError("Invalid schema for this context.");
134+
allowedTypesInsertableSchemaError(allowedTypes, kernel.schema);
90135
}
91136
return inner;
92137
}
@@ -112,17 +157,13 @@ function getType(
112157
const possibleTypes = getPossibleTypes(allowedTypes, data);
113158
if (possibleTypes.length === 0) {
114159
throw new UsageError(
115-
`The provided data is incompatible with all of the types allowed by the schema. The set of allowed types is: ${JSON.stringify(
116-
[...allowedTypes].map((schema) => schema.identifier),
117-
)}.`,
160+
`The provided data is incompatible with all of the types allowed by the schema. The set of allowed types is: ${quotedAllowedTypesWithNames(allowedTypes)}.`,
118161
);
119162
}
120163
if (!hasSingle(possibleTypes)) {
121164
throw new UsageError(
122165
`The provided data is compatible with more than one type allowed by the schema.
123-
The set of possible types is ${JSON.stringify([
124-
...possibleTypes.map((schema) => schema.identifier),
125-
])}.
166+
The set of possible types is ${quotedAllowedTypesWithNames(possibleTypes)}.
126167
Explicitly construct an unhydrated node of the desired type to disambiguate.
127168
For class-based schema, this can be done by replacing an expression like "{foo: 1}" with "new MySchema({foo: 1})".`,
128169
);

‎packages/dds/tree/src/test/simple-tree/api/treeNodeApi.spec.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3251,10 +3251,7 @@ describe("treeNodeApi", () => {
32513251

32523252
const content2 = { x: new C({}) as TreeNode as B };
32533253
TreeAlpha.tagContentSchema(A, content2);
3254-
assert.throws(
3255-
() => new A(content2),
3256-
validateUsageError(/Invalid schema for this context/),
3257-
);
3254+
assert.throws(() => new A(content2), validateUsageError(/Expected insertable for/));
32583255
});
32593256
});
32603257

‎packages/dds/tree/src/test/simple-tree/node-kinds/recordNode.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -449,7 +449,7 @@ describe("RecordNode", () => {
449449
// @ts-expect-error: Intentionally setting a value of an incompatible type.
450450
record.foo = new OtherRecursiveRecord({ x: 100 });
451451
},
452-
validateUsageError(/Invalid schema for this context/),
452+
validateUsageError(/Expected insertable for/),
453453
);
454454
});
455455

‎packages/dds/tree/src/test/simple-tree/unhydratedFlexTreeFromInsertable.spec.ts‎

Lines changed: 40 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -573,24 +573,6 @@ describe("unhydratedFlexTreeFromInsertable", () => {
573573
/The provided data is incompatible with all of the types allowed by the schema/,
574574
);
575575
});
576-
577-
it("Throws for structurally valid data, but created with a different schema.", () => {
578-
const schemaFactory = new SchemaFactory("test");
579-
class TestSchema extends schemaFactory.object("testObject", {
580-
field: schemaFactory.string,
581-
}) {}
582-
583-
class TestSchema2 extends schemaFactory.object("testObject", {
584-
field: schemaFactory.string,
585-
}) {}
586-
587-
const testData = new TestSchema2({ field: "test" });
588-
589-
assert.throws(
590-
() => unhydratedFlexTreeFromInsertable(testData, TestSchema),
591-
validateUsageError("Invalid schema for this context."),
592-
);
593-
});
594576
});
595577

596578
describe("record", () => {
@@ -777,19 +759,6 @@ describe("unhydratedFlexTreeFromInsertable", () => {
777759
/The provided data is incompatible with all of the types allowed by the schema/,
778760
);
779761
});
780-
781-
it("Throws for structurally valid data, but created with a different schema.", () => {
782-
const schemaFactory = new SchemaFactoryAlpha("test");
783-
class TestSchema extends schemaFactory.record("test-a", schemaFactory.string) {}
784-
class TestSchema2 extends schemaFactory.record("test-b", schemaFactory.string) {}
785-
786-
const testData = new TestSchema2({ field: "test" });
787-
788-
assert.throws(
789-
() => unhydratedFlexTreeFromInsertable(testData, TestSchema),
790-
validateUsageError("Invalid schema for this context."),
791-
);
792-
});
793762
});
794763

795764
describe("object", () => {
@@ -1286,20 +1255,55 @@ describe("unhydratedFlexTreeFromInsertable", () => {
12861255
assert.deepEqual(deepCopyMapTree(actual), expected);
12871256
});
12881257

1258+
it("Structurally valid data, but created with a different schema object", () => {
1259+
const schemaFactory = new SchemaFactoryAlpha("test");
1260+
class TestSchema extends schemaFactory.map("a", schemaFactory.string) {}
1261+
class TestSchema2 extends schemaFactory.map("a", schemaFactory.string) {}
1262+
1263+
const testData = new TestSchema2({});
1264+
1265+
assert.throws(
1266+
() => unhydratedFlexTreeFromInsertable(testData, TestSchema),
1267+
validateUsageError(`A node with schema "test.a" (name: "TestSchema2") was provided where a node with that identifier is allowed, but the actual schema required ("test.a" (name: "TestSchema")) is not the same schema object.
1268+
TreeNodeSchema have significant object identity and thus the exact same object must be used as the schema when defining what nodes are allowed and when constructing the node to use.`),
1269+
);
1270+
});
1271+
1272+
it("Structurally valid data, but created with a different schema identifier", () => {
1273+
const schemaFactory = new SchemaFactoryAlpha("test");
1274+
class TestSchema extends schemaFactory.map("a", schemaFactory.string) {}
1275+
class TestSchema2 extends schemaFactory.map("b", schemaFactory.string) {}
1276+
1277+
const testData = new TestSchema2({});
1278+
1279+
assert.throws(
1280+
() => unhydratedFlexTreeFromInsertable(testData, TestSchema),
1281+
validateUsageError(`Expected insertable for one of ["test.a" (name: "TestSchema")]. Got node with schema "test.b" (name: "TestSchema2").
1282+
Nodes are valid insertable objects, but only if their schema are in the allowed list.`),
1283+
);
1284+
});
1285+
12891286
it("ambiguous unions", () => {
12901287
const schemaFactory = new SchemaFactory("test");
1291-
const a = schemaFactory.object("a", { x: schemaFactory.string });
1292-
const b = schemaFactory.object("b", { x: schemaFactory.string });
1293-
const allowedTypes = [a, b];
1288+
class A extends schemaFactory.object("a", { x: schemaFactory.string }) {}
1289+
class B extends schemaFactory.object("b", { x: schemaFactory.string }) {}
1290+
const allowedTypes = [A, B];
12941291

12951292
assert.throws(
12961293
() => unhydratedFlexTreeFromInsertable({}, allowedTypes),
1297-
/\["test.a","test.b"]/,
1294+
validateUsageError(
1295+
`The provided data is incompatible with all of the types allowed by the schema. The set of allowed types is: ["test.a" (name: "A"), "test.b" (name: "B")].`,
1296+
),
12981297
);
12991298
assert.throws(
13001299
() => unhydratedFlexTreeFromInsertable({ x: "hello" }, allowedTypes),
1301-
/\["test.a","test.b"]/,
1300+
validateUsageError(`The provided data is compatible with more than one type allowed by the schema.
1301+
The set of possible types is ["test.a" (name: "A"), "test.b" (name: "B")].
1302+
Explicitly construct an unhydrated node of the desired type to disambiguate.
1303+
For class-based schema, this can be done by replacing an expression like "{foo: 1}" with "new MySchema({foo: 1})".`),
13021304
);
1305+
1306+
unhydratedFlexTreeFromInsertable(new A({ x: "hello" }), allowedTypes);
13031307
});
13041308

13051309
it("unambiguous unions", () => {

0 commit comments

Comments
 (0)