tree: Implicitly constructed object nodes now only consider own prope…

…rties during validation (#22453)

## Description

See changeset for details.

.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.

packages/dds/tree/src/simple-tree/objectNode.ts

@@ -94,6 +94,8 @@ export type FieldHasDefault<T extends ImplicitFieldSchema> = 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.
  *

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<string, InsertableContent>)[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<FieldKey, readonly MapTree[]>,
 	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;
+			}
 		}
 	}
 

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", () => {

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<typeof x, Function>;
+					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<typeof x, number>;
+					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<typeof x, number | undefined>;
+						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<typeof Schema> = 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<typeof Schema> = {};
+						}
+					});
+				});
+			});
+
+			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<typeof x, number>;
-				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<typeof a.constructor, number | Function>;
+				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", () => {