Cleanup related to allowUnknownOptionalFields (microsoft#25074)

## Description

Change and clarify limitations related to alpha features
allowUnknownOptionalFields and importVerbose

See changeset for user facing details.

Internally this cleans up some debt where schema policy was being used
in ways that it was not intended.

## Breaking Changes

See changeset.

Author: CraigMacomber

.changeset/cruel-geese-shine.md

@@ -0,0 +1,18 @@
+---
+"@fluidframework/tree": minor
+"fluid-framework": minor
+"__section": tree
+---
+Change and clarify limitations related to alpha features allowUnknownOptionalFields and importVerbose
+
+[allowUnknownOptionalFields](https://fluidframework.com/docs/api/fluid-framework/schemafactoryobjectoptions-interface#allowunknownoptionalfields-propertysignature) currently has some limitations.
+To mitigate some bugs, [importVerbose](https://fluidframework.com/docs/api/fluid-framework/treealpha-interface#importverbose-methodsignature) has dropped support for unknown optional fields.
+Previously `importVerbose` would tolerate some unknown optional fields, but could not validate they comply with the document stored schema.
+This could cause some crashes, and likely document corruption.
+This support has been removed: now trying to create nodes containing unknown optional fields via `importVerbose` will throw a `UsageError`.
+There is no longer a way to create and insert nodes which contain subtrees for which there is no schema.
+
+Ideally `exportVerbose` and `importVerbose` could be used to round trip data while optionally preserving unknown optional fields, but this is currently not working and thus not supported.
+
+If exporting using [useStoredKeys](https://fluidframework.com/docs/api/fluid-framework/treeencodingoptions-interface#usestoredkeys-propertysignature), the unknown optional fields will be preserved but may not be able to be imported.
+If exporting not using `useStoredKeys`, a known bug currently causes an assertion to fail.

packages/dds/tree/src/core/schema-stored/schema.ts

@@ -123,27 +123,6 @@ export interface SchemaPolicy {
 	 * and will be unable to process any changes that use those FieldKinds.
 	 */
 	readonly fieldKinds: ReadonlyMap<FieldKindIdentifier, FieldKindData>;
-
-	/**
-	 * If true, new content inserted into the tree should be validated against the stored schema.
-	 * @remarks
-	 * TODO: AB#43546: This is not information used to interpret the stored schema: this configuration should be moved elsewhere.
-	 */
-	readonly validateSchema: boolean;
-
-	/**
-	 * Whether to allow a document to be opened when a particular stored schema (identified by `identifier`)
-	 * contains optional fields that are not known to the view schema.
-	 *
-	 * @privateRemarks
-	 * Plumbing this in via `SchemaPolicy` avoids needing to walk the view schema representation repeatedly in places
-	 * that need it (schema validation, view vs stored compatibility checks).
-	 *
-	 * TODO: AB#43546
-	 * This is not information used to interpret the stored schema: it is instead about view schema, and how compatible they are with a stored schema.
-	 * SchemaCompatibilityTester should be updated to not store this table in here, and then this field should be removed.
-	 */
-	allowUnknownOptionalFields(identifier: TreeNodeSchemaIdentifier): boolean;
 }
 
 /**

packages/dds/tree/src/feature-libraries/default-schema/defaultSchema.ts

@@ -12,6 +12,4 @@ import { fieldKinds } from "./defaultFieldKinds.js";
  */
 export const defaultSchemaPolicy: FullSchemaPolicy = {
 	fieldKinds,
-	validateSchema: false,
-	allowUnknownOptionalFields: () => false,
 };

packages/dds/tree/src/feature-libraries/default-schema/schemaChecker.ts

@@ -79,14 +79,11 @@ export function isNodeInSchema(
 				}
 				uncheckedFieldsFromNode.delete(fieldKey);
 			}
-			// The node has fields that we did not check as part of looking at every field defined in the node's schema
-			if (
-				uncheckedFieldsFromNode.size !== 0 &&
-				// TODO: AB#43547: This check is wrong. If a given view schema allows an unknown optional field, that does NOT mean the stored schema should allow unknown:
-				// In-fact, any data the view schema does not know about must still comply with the stored schema:
-				// if this were not the case schema evolution could not add any fields since they might already have out of schema data.
-				!schemaAndPolicy.policy.allowUnknownOptionalFields(node.type)
-			) {
+			// The node has fields that we did not check as part of looking at every field defined in the node's schema.
+			// Since this is testing compatibility with a stored schema (not view schema), "allowUnknownOptionalFields" does not exist at this layer.
+			// Code using this with a stored schema derived from a view schema rather than the document can be problematic because it may be missing unknown fields that the actual document has.
+			// Other schema evolution features like "staged" allowed types will likely cause similar issues elsewhere in this checker.
+			if (uncheckedFieldsFromNode.size !== 0) {
 				return SchemaValidationError.ObjectNode_FieldNotInSchema;
 			}
 		} else if (schema instanceof MapNodeStoredSchema) {

packages/dds/tree/src/shared-tree/schematizeTree.ts

@@ -219,7 +219,7 @@ export function ensureSchema(
 			return false;
 		}
 		case UpdateType.SchemaCompatible: {
-			checkout.updateSchema(toStoredSchema(viewSchema.viewSchemaRoot));
+			checkout.updateSchema(toStoredSchema(viewSchema.viewSchema.root));
 			return true;
 		}
 		default: {

packages/dds/tree/src/shared-tree/schematizingTreeView.ts

@@ -12,7 +12,7 @@ import type {
 import { assert, unreachableCase } from "@fluidframework/core-utils/internal";
 import { UsageError } from "@fluidframework/telemetry-utils/internal";
 
-import { anchorSlot, type SchemaPolicy } from "../core/index.js";
+import { anchorSlot } from "../core/index.js";
 import {
 	type NodeIdentifierManager,
 	defaultSchemaPolicy,
@@ -49,11 +49,11 @@ import {
 	HydratedContext,
 	SimpleContextSlot,
 	areImplicitFieldSchemaEqual,
-	createUnknownOptionalFieldPolicy,
 	prepareForInsertionContextless,
 	type FieldSchema,
 	toStoredSchema,
 	tryDisposeTreeNode,
+	TreeViewConfigurationAlpha,
 } from "../simple-tree/index.js";
 import {
 	type Breakable,
@@ -90,7 +90,6 @@ export class SchematizingSimpleTreeView<
 	 * Undefined iff uninitialized or disposed.
 	 */
 	private currentCompatibility: SchemaCompatibilityStatus | undefined;
-	private readonly schemaPolicy: SchemaPolicy;
 	public readonly events: Listenable<TreeViewEvents & TreeBranchEvents> &
 		IEmitter<TreeViewEvents & TreeBranchEvents> &
 		HasListeners<TreeViewEvents & TreeBranchEvents> = createEmitter();
@@ -132,13 +131,10 @@ export class SchematizingSimpleTreeView<
 		checkout.forest.anchors.slots.set(ViewSlot, this);
 
 		this.rootFieldSchema = normalizeFieldSchema(config.schema);
-		this.schemaPolicy = {
-			...defaultSchemaPolicy,
-			validateSchema: config.enableSchemaValidation,
-			allowUnknownOptionalFields: createUnknownOptionalFieldPolicy(this.rootFieldSchema),
-		};
 
-		this.viewSchema = new SchemaCompatibilityTester(this.schemaPolicy, this.rootFieldSchema);
+		const configAlpha = new TreeViewConfigurationAlpha({ schema: config.schema });
+
+		this.viewSchema = new SchemaCompatibilityTester(configAlpha);
 		// This must be initialized before `update` can be called.
 		this.currentCompatibility = {
 			canView: false,
@@ -175,13 +171,13 @@ export class SchematizingSimpleTreeView<
 		}
 
 		this.runSchemaEdit(() => {
-			const schema = toStoredSchema(this.viewSchema.viewSchemaRoot);
+			const schema = toStoredSchema(this.config.schema);
 			const mapTree = prepareForInsertionContextless(
 				content as InsertableContent | undefined,
 				this.rootFieldSchema,
 				{
 					schema,
-					policy: this.schemaPolicy,
+					policy: defaultSchemaPolicy,
 				},
 				this,
 			);
@@ -316,7 +312,7 @@ export class SchematizingSimpleTreeView<
 
 		if (compatibility.canView) {
 			this.flexTreeContext = new Context(
-				this.schemaPolicy,
+				defaultSchemaPolicy,
 				this.checkout,
 				this.nodeKeyManager,
 			);

packages/dds/tree/src/shared-tree/treeAlpha.ts

@@ -263,6 +263,14 @@ export interface TreeAlpha {
 	 * Construct tree content compatible with a field defined by the provided `schema`.
 	 * @param schema - The schema for what to construct. As this is an {@link ImplicitFieldSchema}, a {@link FieldSchema}, {@link TreeNodeSchema} or {@link AllowedTypes} array can be provided.
 	 * @param data - The data used to construct the field content. See {@link (TreeAlpha:interface).(exportVerbose:1)}.
+	 * @remarks
+	 * This currently does not support input containing {@link SchemaFactoryObjectOptions.allowUnknownOptionalFields| unknown optional fields}.
+	 * @privateRemarks
+	 * See TODOs in {@link TreeEncodingOptions}.
+	 *
+	 * TODO: clarify how this handles out of schema data.
+	 * Does it robustly validate? How do you use it with schema evolution features like staged allowed types and allowUnknownOptionalFields? Which errors are deferred until insertion/hydration?
+	 * Ensure whatever policy is chosen is documented, enforces, tested and applied consistently to all import code paths.
 	 */
 	importVerbose<const TSchema extends ImplicitFieldSchema>(
 		schema: TSchema,

packages/dds/tree/src/simple-tree/api/configuration.ts

@@ -47,14 +47,20 @@ import type { SimpleNodeSchema, SimpleTreeSchema } from "../simpleSchema.js";
  */
 export interface ITreeConfigurationOptions {
 	/**
-	 * If `true`, the tree will validate new content against its stored schema at insertion time
+	 * If `true`, the tree will perform additional validation of content against its stored schema
 	 * and throw an error if the new content doesn't match the expected schema.
 	 *
 	 * @defaultValue `false`.
 	 *
-	 * @remarks Enabling schema validation has a performance penalty when inserting new content into the tree because
+	 * @remarks
+	 * Currently most cases already have some schema validation, so this is mainly for additional validation which may be useful when debugging issues,
+	 * working with untyped APIs, or when the small performance overhead is a non-issue.
+	 *
+	 * Enabling schema validation has a performance penalty when inserting new content into the tree because
 	 * additional checks are done. Enable this option only in scenarios where you are ok with that operation being a
 	 * bit slower.
+	 *
+	 * For additional validation in more cases, see {@link ForestTypeExpensiveDebug}.
 	 */
 	enableSchemaValidation?: boolean;
 

packages/dds/tree/src/simple-tree/api/create.ts

@@ -4,9 +4,11 @@
  */
 
 import { assert } from "@fluidframework/core-utils/internal";
+import { UsageError } from "@fluidframework/telemetry-utils/internal";
 
 import {
 	CursorLocationType,
+	forbiddenFieldKindIdentifier,
 	mapCursorField,
 	mapCursorFields,
 	type ITreeCursorSynchronous,
@@ -27,7 +29,6 @@ import {
 	isFieldInSchema,
 } from "../../feature-libraries/index.js";
 import { getUnhydratedContext } from "../createContext.js";
-import { createUnknownOptionalFieldPolicy } from "../node-kinds/index.js";
 import type { SimpleNodeSchema, SimpleNodeSchemaBase } from "../simpleSchema.js";
 import { getStoredSchema } from "../toStoredSchema.js";
 import { unknownTypeError } from "./customTree.js";
@@ -52,11 +53,11 @@ export function createFromCursor<const TSchema extends ImplicitFieldSchema>(
 	const schemaValidationPolicy: SchemaAndPolicy = {
 		policy: {
 			...defaultSchemaPolicy,
-			allowUnknownOptionalFields: createUnknownOptionalFieldPolicy(schema),
 		},
 		schema: context.flexContext.schema,
 	};
 
+	// TODO: AB#43548: Using a stored schema from the possibly unhydrated flex tree context does not handle schema evolution features like "allowUnknownOptionalFields".
 	const maybeError = isFieldInSchema(
 		mapTrees,
 		flexSchema.rootFieldSchema,
@@ -80,27 +81,45 @@ export function createFromCursor<const TSchema extends ImplicitFieldSchema>(
 /**
  * Construct an {@link UnhydratedFlexTreeNode} from a cursor in Nodes mode.
  * @remarks
- * This does not validate the node is in schema.
+ * This does not fully validate the node is in schema, but does throw UsageErrors for some cases of out-of-schema content.
+ *
+ * Does not support unknown optional fields: will throw a UsageError if the field is not in schema.
+ * This cannot easily be fixed as this code requires a schema for each subtree to process, and none is available for unknown optional fields.
  */
 export function unhydratedFlexTreeFromCursor(
 	context: Context,
 	cursor: ITreeCursorSynchronous,
 ): UnhydratedFlexTreeNode {
 	assert(cursor.mode === CursorLocationType.Nodes, 0xbb4 /* Expected nodes cursor */);
 	const schema = context.schema.get(cursor.type) ?? unknownTypeError(cursor.type);
+	// TODO: AB#43548: Using a stored schema from for unhydrated flex trees does not handle schema evolution features like "allowUnknownOptionalFields".
 	const storedSchema = getStoredSchema(
 		schema as SimpleNodeSchemaBase<NodeKind> as SimpleNodeSchema,
 	);
 	const fields = new Map(
-		mapCursorFields(cursor, () => [
-			cursor.getFieldKey(),
-			createField(
-				context.flexContext,
-				storedSchema.getFieldSchema(cursor.getFieldKey()).kind,
+		mapCursorFields(cursor, () => {
+			const fieldSchema = storedSchema.getFieldSchema(cursor.getFieldKey());
+			if (fieldSchema.kind === forbiddenFieldKindIdentifier) {
+				// Check for unexpected fields before recursing into children:
+				// Code which hits this case is very likely to also use an unknown type in the unexpected field, which would give a more confusing error message.
+				// This case is detected here to improve error quality.
+				// Also note that if using the view schema from above to suppress this error for unknownOptionalFields, that would not provide a way to handle unknown types in those fields:
+				// they would still error, but with that more confusing message about unknown types.
+				throw new UsageError(
+					// Using JSON.stringify to handle quoting and escaping since both key and identifier can technically contain quotes themselves.
+					`Field ${JSON.stringify(cursor.getFieldKey())} is not defined in the schema ${JSON.stringify(schema.identifier)}.`,
+				);
+			}
+			return [
 				cursor.getFieldKey(),
-				mapCursorField(cursor, () => unhydratedFlexTreeFromCursor(context, cursor)),
-			),
-		]),
+				createField(
+					context.flexContext,
+					fieldSchema.kind,
+					cursor.getFieldKey(),
+					mapCursorField(cursor, () => unhydratedFlexTreeFromCursor(context, cursor)),
+				),
+			];
+		}),
 	);
 	return new UnhydratedFlexTreeNode(
 		{ type: cursor.type, value: cursor.value },

packages/dds/tree/src/simple-tree/api/customTree.ts

@@ -43,6 +43,12 @@ export interface TreeEncodingOptions {
 	 * @remarks
 	 * Has no effect on {@link NodeKind}s other than {@link NodeKind.Object}.
 	 * @defaultValue false.
+	 * @privateRemarks
+	 * TODO AB#43548:
+	 * Replace this with an enum that provides three options:
+	 * - `usePropertyKeys`: use property keys. Supported for import and export.
+	 * - `allStoredKeys`: use stored keys, and include unknown optional fields. Supported for export only, at least for the short term.
+	 * - `knownStoredKeys`: use stored keys but do not include unknown optional fields. Supported for import and export.
 	 */
 	readonly useStoredKeys?: boolean;
 }