tree: Allow array from map and map from array

.changeset/moody-toys-dance.md

@@ -0,0 +1,31 @@
+---
+"@fluidframework/tree": minor
+"fluid-framework": minor
+---
+
+Allow constructing ArrayNodes from Maps and MapNodes from arrays when unambiguous.
+
+Since the types for ArrayNodes and MapNodes indicate they can be constructed from iterables,
+it should work, even if those iterables are themselves arrays or maps.
+To avoid this being a breaking change, a priority system was introduced.
+ArrayNodes will only be implicitly constructable from JavaScript Map objects in contexts where no MapNodes are allowed.
+Similarly MapNodes will only be implicitly constructable from JavaScript Array objects in contexts where no ArrayNodes are allowed.
+
+In practice, the main case in which this is likely to matter is when implicitly constructing a map node. If you provide an array of key value pairs, this now works instead of erroring, as long as no ArrayNode is valid at that location in the tree.
+
+```typescript
+class MyMapNode extends schemaFactory.map("x", schemaFactory.number) {}
+class Root extends schemaFactory.object("root", { data: MyMapNode }) {}
+// This now works (before it compiled, but error at runtime):
+const fromArray = new Root({ data: [["x", 5]] });
+```
+
+Prior versions used to have to do:
+```typescript
+new Root({ data: new MyMapNode([["x", 5]]) });
+```
+or:
+```typescript
+new Root({ data: new Map([["x", 5]]) });
+```
+Both of these options still work: strictly more cases are allowed with this change.

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

@@ -932,13 +932,7 @@ export function arraySchema<
 		): MapTreeNode {
 			return getOrCreateMapTreeNode(
 				flexSchema,
-				mapTreeFromNodeData(
-					// Ensure input iterable is not an map. See TODO in shallowCompatibilityTest.
-					Array.from(
-						input as Iterable<InsertableTreeNodeFromImplicitAllowedTypes<T>>,
-					) as object,
-					this as unknown as ImplicitAllowedTypes,
-				),
+				mapTreeFromNodeData(input as object, this as unknown as ImplicitAllowedTypes),
 			);
 		}
 

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

@@ -238,8 +238,7 @@ export function mapSchema<
 			return getOrCreateMapTreeNode(
 				flexSchema,
 				mapTreeFromNodeData(
-					// Ensure input iterable is not an array. See TODO in shallowCompatibilityTest.
-					new Map(input as Iterable<readonly [string, InsertableContent]>),
+					input as Iterable<readonly [string, InsertableContent]>,
 					this as unknown as ImplicitAllowedTypes,
 				),
 			);

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

@@ -309,7 +309,7 @@ function mapValueWithFallbacks(
  * be thrown if the tree does not conform to the schema. If undefined, no validation against the stored schema is done.
  */
 function arrayToMapTreeFields(
-	data: readonly InsertableContent[],
+	data: Iterable<InsertableContent>,
 	allowedTypes: ReadonlySet<TreeNodeSchema>,
 	schemaValidationPolicy: SchemaAndPolicy | undefined,
 ): ExclusiveMapTree[] {
@@ -338,15 +338,15 @@ function arrayToMapTreeFields(
 
 /**
  * Transforms data under an Array schema.
- * @param data - The tree data to be transformed. Must be an array.
+ * @param data - The tree data to be transformed. Must be an iterable.
  * @param schema - The schema associated with the value.
  * @param schemaValidationPolicy - The stored schema and policy to be used for validation, if the policy says schema
  * validation should happen. If it does, the input tree will be validated against this schema + policy, and an error will
  * be thrown if the tree does not conform to the schema. If undefined, no validation against the stored schema is done.
  */
 function arrayToMapTree(data: InsertableContent, schema: TreeNodeSchema): ExclusiveMapTree {
 	assert(schema.kind === NodeKind.Array, 0x922 /* Expected an array schema. */);
-	if (!isReadonlyArray(data)) {
+	if (!(typeof data === "object" && data !== null && Symbol.iterator in data)) {
 		throw new UsageError(`Input data is incompatible with Array schema: ${data}`);
 	}
 
@@ -367,7 +367,7 @@ function arrayToMapTree(data: InsertableContent, schema: TreeNodeSchema): Exclus
 
 /**
  * Transforms data under a Map schema.
- * @param data - The tree data to be transformed. Must be a TypeScript Map.
+ * @param data - The tree data to be transformed. Must be an iterable.
  * @param schema - The schema associated with the value.
  * @param schemaValidationPolicy - The stored schema and policy to be used for validation, if the policy says schema
  * validation should happen. If it does, the input tree will be validated against this schema + policy, and an error will
@@ -497,13 +497,41 @@ export function getPossibleTypes(
 	allowedTypes: ReadonlySet<TreeNodeSchema>,
 	data: ContextuallyTypedNodeData,
 ): TreeNodeSchema[] {
+	let best = CompatibilityLevel.None;
 	const possibleTypes: TreeNodeSchema[] = [];
 	for (const schema of allowedTypes) {
-		if (shallowCompatibilityTest(schema, data)) {
+		const level = shallowCompatibilityTest(schema, data);
+		if (level > best) {
+			possibleTypes.length = 0;
+			best = level;
+		}
+		if (best === level) {
 			possibleTypes.push(schema);
 		}
 	}
-	return possibleTypes;
+	return best === CompatibilityLevel.None ? [] : possibleTypes;
+}
+
+/**
+ * Indicates a compatibility level for inferring a schema to apply to insertable data.
+ * @remarks
+ * Only the highest compatibility options are used.
+ * This approach allows adding new possible matching at a new lower compatibility level as a non breaking change,
+ * since that way they can't make a case that was compatible before ambiguous now.
+ */
+enum CompatibilityLevel {
+	/**
+	 * Not compatible. Constructor typing indicates incompatibility.
+	 */
+	None = 0,
+	/**
+	 * Additional compatibility cases added in Fluid Framework 2.2.
+	 */
+	Low = 1,
+	/**
+	 * Compatible in Fluid Framework 2.0.
+	 */
+	Normal = 2,
 }
 
 /**
@@ -516,46 +544,59 @@ export function getPossibleTypes(
 function shallowCompatibilityTest(
 	schema: TreeNodeSchema,
 	data: ContextuallyTypedNodeData,
-): boolean {
+): CompatibilityLevel {
 	assert(
 		data !== undefined,
 		0x889 /* undefined cannot be used as contextually typed data. Use ContextuallyTypedFieldData. */,
 	);
 
 	if (isTreeValue(data)) {
-		return allowsValue(schema, data);
+		return allowsValue(schema, data) ? CompatibilityLevel.Normal : CompatibilityLevel.None;
 	}
 	if (schema.kind === NodeKind.Leaf) {
-		return false;
+		return CompatibilityLevel.None;
 	}
 
-	// TODO:
-	// current typing (of schema based constructors and thus implicit node construction)
+	// Typing (of schema based constructors and thus implicit node construction)
 	// allows iterables for constructing maps and arrays.
-	// Some current users of this API may have unions of maps and arrays,
+	// Some users of this API may have unions of maps and arrays,
 	// and rely on Arrays ending up as array nodes and maps as Map nodes,
 	// despite both being iterable and thus compatible with both.
-	// In the future, a better solution could be a priority based system where an array would be parsed as an array when unioned with a map,
+	// This uses a priority based system where an array would be parsed as an array when unioned with a map,
 	// but if in a map only context, could still be used as a map.
-	// Then this method would return a quality of fit, not just a boolean.
-	// For now, special case map and array before checking iterable to avoid regressing the union of map and array case.
 
 	if (data instanceof Map) {
-		return schema.kind === NodeKind.Map;
+		switch (schema.kind) {
+			case NodeKind.Map:
+				return CompatibilityLevel.Normal;
+			case NodeKind.Array:
+				// Maps are iterable, so type checking does allow constructing an ArrayNode from a map if the array's type is an array that includes the key and value types of the map.
+				return CompatibilityLevel.Low;
+			default:
+				return CompatibilityLevel.None;
+		}
 	}
 
 	if (isReadonlyArray(data)) {
-		return schema.kind === NodeKind.Array;
+		switch (schema.kind) {
+			case NodeKind.Array:
+				return CompatibilityLevel.Normal;
+			case NodeKind.Map:
+				// Arrays are iterable, so type checking does allow constructing an array from a MapNode from an if the array's type is key values pairs for the map.
+				return CompatibilityLevel.Low;
+			default:
+				return CompatibilityLevel.None;
+		}
 	}
 
 	const mapOrArray = schema.kind === NodeKind.Array || schema.kind === NodeKind.Map;
 
 	if (Symbol.iterator in data) {
-		return mapOrArray;
+		return mapOrArray ? CompatibilityLevel.Normal : CompatibilityLevel.None;
 	}
 
 	if (mapOrArray) {
-		return false;
+		return CompatibilityLevel.None;
 	}
 
 	// Assume record-like object
@@ -571,11 +612,11 @@ 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 false;
+			return CompatibilityLevel.None;
 		}
 	}
 
-	return true;
+	return CompatibilityLevel.Normal;
 }
 
 function allowsValue(schema: TreeNodeSchema, value: TreeValue): boolean {

packages/dds/tree/src/test/simple-tree/arrayNode.spec.ts

@@ -835,4 +835,48 @@ describe("ArrayNode", () => {
 			assert.deepEqual(result2, [1, 2, 3]);
 		});
 	});
+
+	it("explicit construction", () => {
+		class Schema extends schemaFactory.array(
+			"x",
+			schemaFactory.array([schemaFactory.number, schemaFactory.string]),
+		) {}
+		const data = [["x", 5]] as const;
+		const json = JSON.stringify(data);
+		const fromArray = new Schema(data);
+		assert.equal(JSON.stringify(fromArray), json);
+		const fromMap = new Schema(new Map(data));
+		assert.equal(JSON.stringify(fromMap), json);
+		const fromIterable = new Schema(new Map(data).entries());
+		assert.equal(JSON.stringify(fromIterable), json);
+	});
+
+	describe("implicit construction", () => {
+		it("fromArray", () => {
+			class Schema extends schemaFactory.array("x", schemaFactory.number) {}
+			class Root extends schemaFactory.object("root", { data: Schema }) {}
+			const fromArray = new Root({ data: [5] });
+			assert.deepEqual([...fromArray.data], [5]);
+		});
+		it("fromMap", () => {
+			class Schema extends schemaFactory.array(
+				"x",
+				schemaFactory.array([schemaFactory.number, schemaFactory.string]),
+			) {}
+			class Root extends schemaFactory.object("root", { data: Schema }) {}
+
+			const data = [["x", 5]] as const;
+			const json = JSON.stringify(data);
+
+			const fromMap = new Root({ data: new Map(data) });
+			assert.equal(JSON.stringify(fromMap.data), json);
+		});
+		it("fromIterable", () => {
+			class Schema extends schemaFactory.array("x", schemaFactory.number) {}
+			class Root extends schemaFactory.object("root", { data: Schema }) {}
+			const fromArray = new Root({ data: [5] });
+			const fromIterable = new Root({ data: new Set([5]) });
+			assert.deepEqual([...fromIterable.data], [5]);
+		});
+	});
 });

packages/dds/tree/src/test/simple-tree/mapNode.spec.ts

@@ -79,8 +79,7 @@ describe("MapNode", () => {
 		class Schema extends schemaFactory.map("x", schemaFactory.number) {}
 		class Root extends schemaFactory.object("root", { data: Schema }) {}
 		const data = [["x", 5]] as const;
-		// See TODO in shallowCompatibilityTest for how to enable this case.
-		it.skip("fromArray", () => {
+		it("fromArray", () => {
 			const fromArray = new Root({ data });
 			assert.deepEqual([...fromArray.data], data);
 		});

packages/dds/tree/src/test/simple-tree/toMapTree.spec.ts

@@ -1447,6 +1447,16 @@ describe("toMapTree", () => {
 					[mapSchema, arraySchema],
 				);
 			});
+
+			it("array vs map low priority matching", () => {
+				const f = new SchemaFactory("test");
+				const arraySchema = f.array([f.null]);
+				const mapSchema = f.map([f.null]);
+				// Array makes map
+				assert.deepEqual(getPossibleTypes(new Set([mapSchema]), []), [mapSchema]);
+				// Map makes array
+				assert.deepEqual(getPossibleTypes(new Set([arraySchema]), new Map()), [arraySchema]);
+			});
 		});
 
 		describe("addDefaultsToMapTree", () => {