fix(tree): Resolve several inconsistencies in allowsRepoSuperset and …

…isRepoSuperset (#22999)

## Description

Fixes several bugs/inconsistencies in `allowsRepoSuperset` and
`isRepoSuperset` and adds unit tests for this behavior.

## Bugs

1. Forbidden fields should not be convertible to required fields, but
`isRepoSuperset` allowed this conversion.
2. The production codepath did not correctly handle queries between
value schema nodes and other types of nodes (without the added code in
`comparison.ts`, one could hit `0x893` directly below)
3. `isRepoSuperset` did not previously permit addition of new node kinds

## Context

`allowsRepoSuperset` is the current implementation used by the
production codepath to check compatibility between a document's stored
and view schema. There is ongoing work on the schema evolution front to
transition this codepath to one which is more flexible / easily
debugged, which is the logic in `discrepancies.ts`. The general
philosophy behind `discrepancies.ts` is to split the compatibility check
into two parts: one bit of code which detects all differences between
view and stored schema, and another bit of code which decides if any of
those differences should prohibit document operations (read, write,
upgrade schema, etc). This PR helps bring the newer implementation to
parity with what we have today.

---------

Co-authored-by: Abram Sanderson <[email protected]>

packages/dds/tree/src/feature-libraries/modular-schema/comparison.ts

@@ -54,6 +54,10 @@ export function allowsTreeSuperset(
 		return false;
 	}
 
+	if (superset instanceof LeafNodeStoredSchema) {
+		return false;
+	}
+
 	assert(
 		original instanceof MapNodeStoredSchema || original instanceof ObjectNodeStoredSchema,
 		0x893 /* unsupported node kind */,

packages/dds/tree/src/feature-libraries/modular-schema/discrepancies.ts

@@ -17,6 +17,7 @@ import {
 	type TreeTypeSet,
 	type ValueSchema,
 } from "../../core/index.js";
+import { brand } from "../../util/index.js";
 
 // TODO:
 // The comparisons in this file seem redundant with those in comparison.ts.
@@ -404,7 +405,11 @@ export function isRepoSuperset(view: TreeStoredSchema, stored: TreeStoredSchema)
 	for (const incompatibility of incompatibilities) {
 		switch (incompatibility.mismatch) {
 			case "nodeKind": {
-				return false;
+				if (incompatibility.stored !== undefined) {
+					// It's fine for the view schema to know of a node type that the stored schema doesn't know about.
+					return false;
+				}
+				break;
 			}
 			case "valueSchema":
 			case "allowedTypes":
@@ -460,41 +465,116 @@ function validateFieldIncompatibility(incompatibility: FieldIncompatibility): bo
 }
 
 /**
- * A mapping that defines the order of field kinds for comparison purposes.
- * The numeric values indicate the hierarchy or "strength" of each field kind, where lower numbers are more restrictive.
- * This is used to determine if one field kind can be considered a superset of another.
+ * A linear extension of a partially-ordered set of `T`s. See:
+ * https://en.wikipedia.org/wiki/Linear_extension
  *
- * - "Forbidden": The most restrictive, represented by 1. Indicates a forbidden field.
- * - "Value": Represented by 2. Indicates a required field with a specific value.
- * - "Optional": Represented by 3. Indicates an optional field.
+ * The linear extension is represented as a lookup from each poset element to its index in the linear extension.
+ */
+type LinearExtension<T> = Map<T, number>;
+
+/**
+ * A realizer for a partially-ordered set. See:
+ * https://en.wikipedia.org/wiki/Order_dimension
+ */
+type Realizer<T> = LinearExtension<T>[];
+
+/**
+ * @privateRemarks
+ * TODO: Knowledge of specific field kinds is not appropriate for modular schema.
+ * This bit of field comparison should be dependency injected by default-schema if this comparison logic remains in modular-schema
+ * (this is analogous to what is done in comparison.ts).
+ */
+const FieldKindIdentifiers = {
+	forbidden: brand<FieldKindIdentifier>("Forbidden"),
+	required: brand<FieldKindIdentifier>("Value"),
+	identifier: brand<FieldKindIdentifier>("Identifier"),
+	optional: brand<FieldKindIdentifier>("Optional"),
+	sequence: brand<FieldKindIdentifier>("Sequence"),
+};
+
+/**
+ * A realizer for the partial order of field kind relaxability.
  *
- * Note:
- * - "Sequence": (Currently commented out) was intended to represent a sequence field kind with a value of 4.
- * Relaxing non-sequence fields to sequences is not currently supported but may be considered in the future.
+ * It seems extremely likely that this partial order will remain dimension 2 over time (i.e. the set of allowed relaxations can be visualized
+ * with a [dominance drawing](https://en.wikipedia.org/wiki/Dominance_drawing)), so this strategy allows efficient comarison between field kinds
+ * without excessive casework.
  *
- * TODO: We may need more coverage in realm to prove the correctness of the Forbidden -\> Value transaction
+ * Hasse diagram for the partial order is shown below (lower fields can be relaxed to higher fields):
+ * ```
+ * sequence
+ *    |
+ * optional
+ *    |    \
+ * required forbidden
+ *    |
+ * identifier
+ * ```
  */
-const fieldKindOrder: { [key: string]: number } = {
-	"Forbidden": 1,
-	"Value": 2,
-	"Optional": 3,
-	// "Sequence": 4,  // Relaxing non-sequence fields to sequences is not currently supported, though we could consider doing so in the future.
-};
+const fieldRealizer: Realizer<FieldKindIdentifier> = [
+	[
+		FieldKindIdentifiers.forbidden,
+		FieldKindIdentifiers.identifier,
+		FieldKindIdentifiers.required,
+		FieldKindIdentifiers.optional,
+		FieldKindIdentifiers.sequence,
+	],
+	[
+		FieldKindIdentifiers.identifier,
+		FieldKindIdentifiers.required,
+		FieldKindIdentifiers.forbidden,
+		FieldKindIdentifiers.optional,
+		FieldKindIdentifiers.sequence,
+	],
+].map((extension) => new Map(extension.map((identifier, index) => [identifier, index])));
+
+const PosetComparisonResult = {
+	Less: "<",
+	Greater: ">",
+	Equal: "=",
+	Incomparable: "||",
+} as const;
+type PosetComparisonResult =
+	(typeof PosetComparisonResult)[keyof typeof PosetComparisonResult];
+
+function comparePosetElements<T>(a: T, b: T, realizer: Realizer<T>): PosetComparisonResult {
+	let hasLessThanResult = false;
+	let hasGreaterThanResult = false;
+	for (const extension of realizer) {
+		const aIndex = extension.get(a);
+		const bIndex = extension.get(b);
+		assert(aIndex !== undefined && bIndex !== undefined, "Invalid realizer");
+		if (aIndex < bIndex) {
+			hasLessThanResult = true;
+		} else if (aIndex > bIndex) {
+			hasGreaterThanResult = true;
+		}
+	}
+
+	return hasLessThanResult
+		? hasGreaterThanResult
+			? PosetComparisonResult.Incomparable
+			: PosetComparisonResult.Less
+		: hasGreaterThanResult
+			? PosetComparisonResult.Greater
+			: PosetComparisonResult.Equal;
+}
+
+function posetLte<T>(a: T, b: T, realizer: Realizer<T>): boolean {
+	const comparison = comparePosetElements(a, b, realizer);
+	return (
+		comparison === PosetComparisonResult.Less || comparison === PosetComparisonResult.Equal
+	);
+}
 
 function compareFieldKind(
 	aKind: FieldKindIdentifier | undefined,
 	bKind: FieldKindIdentifier | undefined,
 ): boolean {
 	if (aKind === undefined || bKind === undefined) {
-		return false;
-	}
-
-	if (!(aKind in fieldKindOrder) || !(bKind in fieldKindOrder)) {
-		return false;
+		return aKind === bKind;
 	}
 
-	// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-	return fieldKindOrder[aKind]! <= fieldKindOrder[bKind]!;
+	return posetLte(aKind, bKind, fieldRealizer);
 }
 
 function throwUnsupportedNodeType(type: string): never {

packages/dds/tree/src/test/feature-libraries/modular-schema/discrepancies.spec.ts

@@ -19,11 +19,25 @@ import {
 	FieldKinds,
 	getAllowedContentIncompatibilities,
 	allowsRepoSuperset,
-	isRepoSuperset,
+	isRepoSuperset as isRepoSupersetOriginal,
+	type FlexFieldKind,
 } from "../../../feature-libraries/index.js";
 import { brand } from "../../../util/index.js";
 import { fieldSchema } from "./comparison.spec.js";
 
+// Runs both superset-checking codepaths and verifies they produce consistent results.
+// This function can go away once the older codepath is removed, see comment on the top of `discrepancies.ts` for more information.
+function isRepoSuperset(superset: TreeStoredSchema, original: TreeStoredSchema): boolean {
+	const allowsSupersetResult = allowsRepoSuperset(defaultSchemaPolicy, original, superset);
+	const isRepoSupersetResult = isRepoSupersetOriginal(superset, original);
+	assert.equal(
+		allowsSupersetResult,
+		isRepoSupersetResult,
+		`Inconsistent results for allowsRepoSuperset (${allowsSupersetResult}) and isRepoSuperset (${isRepoSupersetResult})`,
+	);
+	return isRepoSupersetResult;
+}
+
 /**
  * Validates the consistency between `isRepoSuperset` and `allowsRepoSuperset` functions.
  *
@@ -32,14 +46,12 @@ import { fieldSchema } from "./comparison.spec.js";
  */
 function validateSuperset(view: TreeStoredSchema, stored: TreeStoredSchema) {
 	assert.equal(isRepoSuperset(view, stored), true);
-	assert.equal(allowsRepoSuperset(defaultSchemaPolicy, stored, view), true);
 }
 
 function validateStrictSuperset(view: TreeStoredSchema, stored: TreeStoredSchema) {
 	validateSuperset(view, stored);
 	// assert the superset relationship does not keep in reversed direction
 	assert.equal(isRepoSuperset(stored, view), false);
-	assert.equal(allowsRepoSuperset(defaultSchemaPolicy, view, stored), false);
 }
 
 // Arbitrary schema names used in tests
@@ -144,7 +156,7 @@ describe("Schema Discrepancies", () => {
 		 * TODO: If we decide to support this behavior, we will need better e2e tests for this scenario. Additionally,
 		 * we may need to adjust the encoding of map nodes and object nodes to ensure consistent encoding.
 		 */
-		assert.equal(isRepoSuperset(objectNodeSchema, mapNodeSchema), false);
+		assert.equal(isRepoSupersetOriginal(objectNodeSchema, mapNodeSchema), false);
 		assert.equal(
 			allowsRepoSuperset(defaultSchemaPolicy, objectNodeSchema, mapNodeSchema),
 			true,
@@ -525,6 +537,41 @@ describe("Schema Discrepancies", () => {
 			validateStrictSuperset(mapNodeSchema3, mapNodeSchema2);
 		});
 
+		it("Detects new node kinds as a superset", () => {
+			const emptySchema: TreeStoredSchema = {
+				rootFieldSchema: fieldSchema(FieldKinds.forbidden, []),
+				nodeSchema: new Map(),
+			};
+
+			const numberSchema = new LeafNodeStoredSchema(ValueSchema.Number);
+			const optionalNumberSchema: TreeStoredSchema = {
+				rootFieldSchema: fieldSchema(FieldKinds.optional, [numberName]),
+				nodeSchema: new Map([[numberName, numberSchema]]),
+			};
+
+			validateStrictSuperset(optionalNumberSchema, emptySchema);
+		});
+
+		it("Detects changed node kinds as not a superset", () => {
+			// Name used for the node which has a changed type
+			const schemaName = brand<TreeNodeSchemaIdentifier>("test");
+
+			const numberSchema = new LeafNodeStoredSchema(ValueSchema.Number);
+			const schemaA: TreeStoredSchema = {
+				rootFieldSchema: fieldSchema(FieldKinds.optional, [schemaName]),
+				nodeSchema: new Map([[schemaName, numberSchema]]),
+			};
+
+			const objectSchema = new ObjectNodeStoredSchema(new Map());
+			const schemaB: TreeStoredSchema = {
+				rootFieldSchema: fieldSchema(FieldKinds.optional, [schemaName]),
+				nodeSchema: new Map([[schemaName, objectSchema]]),
+			};
+
+			assert.equal(isRepoSuperset(schemaA, schemaB), false);
+			assert.equal(isRepoSuperset(schemaB, schemaA), false);
+		});
+
 		it("Adding to the set of allowed types for a field", () => {
 			const mapNodeSchema2 = createMapNodeSchema(
 				fieldSchema(FieldKinds.optional, []),
@@ -597,5 +644,68 @@ describe("Schema Discrepancies", () => {
 
 			assert.equal(isRepoSuperset(leafNodeSchema1, leafNodeSchema2), false);
 		});
+
+		describe("on field kinds for root fields of identical content", () => {
+			const allFieldKinds = Object.values(FieldKinds);
+			const testCases: {
+				superset: FlexFieldKind;
+				original: FlexFieldKind;
+				expected: boolean;
+			}[] = [
+				{ superset: FieldKinds.forbidden, original: FieldKinds.identifier, expected: false },
+				{ superset: FieldKinds.forbidden, original: FieldKinds.optional, expected: false },
+				{ superset: FieldKinds.forbidden, original: FieldKinds.required, expected: false },
+				{ superset: FieldKinds.forbidden, original: FieldKinds.sequence, expected: false },
+				{ superset: FieldKinds.identifier, original: FieldKinds.forbidden, expected: false },
+				{ superset: FieldKinds.identifier, original: FieldKinds.optional, expected: false },
+				{ superset: FieldKinds.identifier, original: FieldKinds.required, expected: false },
+				{ superset: FieldKinds.identifier, original: FieldKinds.sequence, expected: false },
+				{ superset: FieldKinds.optional, original: FieldKinds.forbidden, expected: true },
+				{ superset: FieldKinds.optional, original: FieldKinds.identifier, expected: true },
+				{ superset: FieldKinds.optional, original: FieldKinds.required, expected: true },
+				{ superset: FieldKinds.optional, original: FieldKinds.sequence, expected: false },
+				{ superset: FieldKinds.required, original: FieldKinds.forbidden, expected: false },
+				{ superset: FieldKinds.required, original: FieldKinds.identifier, expected: true },
+				{ superset: FieldKinds.required, original: FieldKinds.optional, expected: false },
+				{ superset: FieldKinds.required, original: FieldKinds.sequence, expected: false },
+				// Note: despite the fact that all field types can be relaxed to a sequence field, note that
+				// this is not possible using the public API for creating schemas, since the degrees of freedom in creating
+				// sequence fields are restricted: `SchemaFactory`'s `array` builder adds a node which is transparent via
+				// the simple-tree API, but nonetheless results in incompatibility.
+				{ superset: FieldKinds.sequence, original: FieldKinds.forbidden, expected: true },
+				{ superset: FieldKinds.sequence, original: FieldKinds.identifier, expected: true },
+				{ superset: FieldKinds.sequence, original: FieldKinds.optional, expected: true },
+				{ superset: FieldKinds.sequence, original: FieldKinds.required, expected: true },
+				// All field kinds are a (non-proper) superset of themselves
+				...Object.values(FieldKinds).map((kind) => ({
+					superset: kind,
+					original: kind,
+					expected: true,
+				})),
+			];
+
+			it("verify this test is exhaustive", () => {
+				// Test case expectations below are generated manually. When new supported field kinds are added, this suite must be updated.
+				// This likely also necessitates changes to the production code this describe block validates.
+				assert.equal(allFieldKinds.length, 5);
+				assert.equal(allFieldKinds.length ** 2, testCases.length);
+			});
+
+			for (const { superset, original, expected } of testCases) {
+				it(`${superset.identifier} ${expected ? "⊇" : "⊉"} ${original.identifier}`, () => {
+					const schemaA: TreeStoredSchema = {
+						rootFieldSchema: fieldSchema(superset, [numberName]),
+						nodeSchema: new Map([[numberName, new LeafNodeStoredSchema(ValueSchema.Number)]]),
+					};
+
+					const schemaB: TreeStoredSchema = {
+						rootFieldSchema: fieldSchema(original, [numberName]),
+						nodeSchema: new Map([[numberName, new LeafNodeStoredSchema(ValueSchema.Number)]]),
+					};
+
+					assert.equal(isRepoSuperset(schemaA, schemaB), expected);
+				});
+			}
+		});
 	});
 });