From 27faa56f5ae334e0b65fdd84c75764645e64f063 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 9 Sep 2024 20:02:51 -0700 Subject: [PATCH] tree: Implicitly constructed object nodes now only consider own properties during validation (#22453) ## Description See changeset for details. --- .changeset/twelve-shrimps-jog.md | 16 ++++ .../dds/tree/src/simple-tree/objectNode.ts | 2 + .../dds/tree/src/simple-tree/toMapTree.ts | 30 +++++- .../tree/src/test/simple-tree/mapTree.spec.ts | 47 +++++++++ .../src/test/simple-tree/objectNode.spec.ts | 95 +++++++++++++++++-- 5 files changed, 179 insertions(+), 11 deletions(-) create mode 100644 .changeset/twelve-shrimps-jog.md diff --git a/.changeset/twelve-shrimps-jog.md b/.changeset/twelve-shrimps-jog.md new file mode 100644 index 000000000000..f571ccbec53b --- /dev/null +++ b/.changeset/twelve-shrimps-jog.md @@ -0,0 +1,16 @@ +--- +"@fluidframework/tree": minor +"fluid-framework": minor +--- +--- +"section": "tree" +--- +Implicitly constructed object nodes now only consider own properties during validation. + +When determining if some given data is compatible with a particular ObjectNode schema, both inherited and own properties used to be considered. +However when actually constructing the node from the data, only own properties were used. +This could lead to input containing inherited properties getting validated, but producing out of schema nodes missing fields. +This has been fixed and now both code paths use the same check, for own properties, when evaluating if an input has a property that should be considered for providing the content of the node's field. + +This may cause some cases which previously exhibited data corruption to now throw a usage error reporting the data is incompatible. +Such cases may need to copy data from the objects with inherited properties into new objects with own properties before constructing nodes from them. diff --git a/packages/dds/tree/src/simple-tree/objectNode.ts b/packages/dds/tree/src/simple-tree/objectNode.ts index 945bfb68ed5f..30f0bf0627b2 100644 --- a/packages/dds/tree/src/simple-tree/objectNode.ts +++ b/packages/dds/tree/src/simple-tree/objectNode.ts @@ -94,6 +94,8 @@ export type FieldHasDefault = T extends FieldSche * Helper used to produce types for: * * 1. Insertable content which can be used to construct an object node. + * In this case, only own properties are considered. + * This reduces the risk of incorrectly interpreting data at the cost of occasionally requiring users to convert data into a compatible format. * * 2. Insertable content which is an unhydrated object node. * diff --git a/packages/dds/tree/src/simple-tree/toMapTree.ts b/packages/dds/tree/src/simple-tree/toMapTree.ts index 3193eb0f0eb8..694c4858b503 100644 --- a/packages/dds/tree/src/simple-tree/toMapTree.ts +++ b/packages/dds/tree/src/simple-tree/toMapTree.ts @@ -408,7 +408,7 @@ function objectToMapTree(data: FactoryContent, schema: TreeNodeSchema): Exclusiv // Loop through field keys without data. // This does NOT apply defaults. for (const [key, fieldInfo] of schema.flexKeyMap) { - if (Object.hasOwnProperty.call(data, key)) { + if (checkFieldProperty(data, key)) { const value = (data as Record)[key as string]; setFieldValue(fields, value, fieldInfo.schema, fieldInfo.storedKey); } @@ -420,6 +420,24 @@ function objectToMapTree(data: FactoryContent, schema: TreeNodeSchema): Exclusiv }; } +/** + * Check {@link FactoryContentObject} for a property which could be store a field. + * @remarks + * The currently policy is to only consider own properties. + * See {@link InsertableObjectFromSchemaRecord} for where this policy is documented in the public API. + * + * Explicit undefined members are considered to exist, as long as they are own properties. + */ +function checkFieldProperty( + data: FactoryContentObject, + key: string | symbol, +): data is { + readonly [P in string]: InsertableContent | undefined; +} { + // This policy only allows own properties. + return Object.hasOwnProperty.call(data, key); +} + function setFieldValue( fields: Map, fieldValue: InsertableContent | undefined, @@ -587,8 +605,14 @@ function shallowCompatibilityTest( // If the schema has a required key which is not present in the input object, reject it. for (const [fieldKey, fieldSchema] of schema.fields) { - if (data[fieldKey] === undefined && fieldSchema.requiresValue) { - return CompatibilityLevel.None; + if (fieldSchema.requiresValue) { + if (checkFieldProperty(data, fieldKey)) { + if (data[fieldKey] === undefined) { + return CompatibilityLevel.None; + } + } else { + return CompatibilityLevel.None; + } } } diff --git a/packages/dds/tree/src/test/simple-tree/mapTree.spec.ts b/packages/dds/tree/src/test/simple-tree/mapTree.spec.ts index 3e17d95446a7..b70701e90ad4 100644 --- a/packages/dds/tree/src/test/simple-tree/mapTree.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/mapTree.spec.ts @@ -1556,6 +1556,53 @@ describe("toMapTree", () => { // Map makes array assert.deepEqual(getPossibleTypes(new Set([arraySchema]), new Map()), [arraySchema]); }); + + it("inherited properties types", () => { + const f = new SchemaFactory("test"); + class Optional extends f.object("x", { + constructor: f.optional(f.number), + }) {} + class Required extends f.object("x", { + constructor: f.number, + }) {} + class Other extends f.object("y", { + other: f.number, + }) {} + // Ignore inherited constructor field + assert.deepEqual(getPossibleTypes(new Set([Optional, Required, Other]), {}), [ + Optional, + ]); + // Allow overridden field + assert.deepEqual( + getPossibleTypes(new Set([Optional, Required, Other]), { constructor: 5 }), + [Optional, Required], + ); + // Allow overridden undefined + assert.deepEqual( + getPossibleTypes(new Set([Optional, Required, Other]), { constructor: undefined }), + [Optional], + ); + // Multiple Fields + assert.deepEqual( + getPossibleTypes(new Set([Optional, Required, Other]), { + constructor: undefined, + other: 6, + }), + [Optional, Other], + ); + assert.deepEqual( + getPossibleTypes(new Set([Optional, Required, Other]), { + constructor: 5, + other: 6, + }), + [Optional, Required, Other], + ); + // No properties + assert.deepEqual( + getPossibleTypes(new Set([Optional, Required, Other]), Object.create(null)), + [Optional], + ); + }); }); describe("addDefaultsToMapTree", () => { diff --git a/packages/dds/tree/src/test/simple-tree/objectNode.spec.ts b/packages/dds/tree/src/test/simple-tree/objectNode.spec.ts index ca89ab19ac1d..7533c0bff4ad 100644 --- a/packages/dds/tree/src/test/simple-tree/objectNode.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/objectNode.spec.ts @@ -7,7 +7,12 @@ import { strict as assert } from "assert"; import { validateAssertionError } from "@fluidframework/test-runtime-utils/internal"; -import { SchemaFactory, type NodeKind, type TreeNodeSchema } from "../../simple-tree/index.js"; +import { + SchemaFactory, + type NodeBuilderData, + type NodeKind, + type TreeNodeSchema, +} from "../../simple-tree/index.js"; import { describeHydration, hydrate, pretty } from "./utils.js"; import type { requireAssignableTo } from "../../util/index.js"; @@ -20,17 +25,83 @@ describeHydration( "ObjectNode", (init) => { describe("shadowing", () => { - it("constructor", () => { - // constructor is a special case, since one is built in on the derived type. + describe("constructor", () => { + it("empty", () => { + class Schema extends schemaFactory.object("x", {}) {} + const n = init(Schema, {}); + // constructor is a special case, since one is built in on the derived type. + // Check that it is exposed as expected based on type: + const x = n.constructor; + // eslint-disable-next-line @typescript-eslint/ban-types + type check_ = requireAssignableTo; + assert.equal(x, Schema); + }); + + it("required", () => { + class Schema extends schemaFactory.object("x", { + constructor: schemaFactory.number, + }) {} + + const n = init(Schema, { constructor: 5 }); + + const x = n.constructor; + type check_ = requireAssignableTo; + assert.equal(x, 5); + }); + + describe("optional", () => { + class Schema extends schemaFactory.object("x", { + constructor: schemaFactory.optional(schemaFactory.number), + }) {} + + it("explicit undefined", () => { + const n = init(Schema, { constructor: undefined }); + const x = n.constructor; + type check_ = requireAssignableTo; + assert.equal(x, undefined); + }); + + it("default", () => { + // Example of how a type conversion that allows using literals with defaults can still be allowed to compile in the presence of overloaded inherited values. + const data: { [P in "constructor"]?: undefined } = {}; + const insertable: NodeBuilderData = data; + + const n = init(Schema, insertable); + const x = n.constructor; + assert.equal(x, undefined); + + { + // In this particular case of overloads, TypeScript knows this is unsafe, but in other similar cases (like the one above), it can compile without error. + // @ts-expect-error Unsafely construct insertable with correct type. + const _insertable: NodeBuilderData = {}; + } + }); + }); + }); + + it("union", () => { class Schema extends schemaFactory.object("x", { constructor: schemaFactory.number, }) {} + class Other extends schemaFactory.object("y", { + other: schemaFactory.number, + }) {} - const n = init(Schema, { constructor: 5 }); - - const x = n.constructor; - type check_ = requireAssignableTo; - assert.equal(x, 5); + // TODO: + // "init" can't handle field schema, so this uses hydrate, making the two versions of this test the same. + // Either: + // 1. Generalize init + // 2. Reorganize these tests to avoid hitting this requirement + // 3. Some other refactor to resolve this + const a = hydrate([Schema, Other], { constructor: 5 }); + const b = hydrate([Schema, Other], { other: 6 }); + + // eslint-disable-next-line @typescript-eslint/ban-types + type check_ = requireAssignableTo; + assert.equal(a.constructor, 5); + assert.equal(b.constructor, Other); + assert(Tree.is(b, Other)); + assert.equal(b.other, 6); }); }); @@ -255,6 +326,14 @@ describeHydration( // TODO }); }); + + it("default optional field", () => { + class Schema extends schemaFactory.object("x", { + x: schemaFactory.optional(schemaFactory.number), + }) {} + const n = init(Schema, {}); + assert.equal(n.x, undefined); + }); }, () => { describe("shadowing", () => {