chore(tree): Simplify getAllowedContentIncompatibilities output (#23041)

Previous output allowed undefined in FieldKindIncompatibility instances where either the stored or view schema did not have some field implicitly. However, it is also possible to declare a field as not present explicitly using forbidden, which should be treated identically when considering compatibility. This updates the output representation as well as logic to normalize implicit cases to use forbidden. --------- Co-authored-by: Abram Sanderson <[email protected]>
microsoft · Nov 12, 2024 · cc3fb6d · cc3fb6d
1 parent 3b51758
commit cc3fb6d
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 48 deletions.
diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/discrepancies.ts b/packages/dds/tree/src/feature-libraries/modular-schema/discrepancies.ts
@@ -11,6 +11,7 @@ import {
 	LeafNodeStoredSchema,
 	MapNodeStoredSchema,
 	ObjectNodeStoredSchema,
+	storedEmptyFieldSchema,
 	type TreeFieldStoredSchema,
 	type TreeNodeSchemaIdentifier,
 	type TreeStoredSchema,
@@ -80,8 +81,8 @@ export interface AllowedTypeIncompatibility {
 export interface FieldKindIncompatibility {
 	identifier: string | undefined; // undefined indicates root field schema
 	mismatch: "fieldKind";
-	view: FieldKindIdentifier | undefined;
-	stored: FieldKindIdentifier | undefined;
+	view: FieldKindIdentifier;
+	stored: FieldKindIdentifier;
 }
 
 export interface ValueSchemaIncompatibility {
@@ -349,12 +350,15 @@ function trackObjectNodeDiscrepancies(
 
 	for (const [fieldKey, fieldStoredSchema] of view.objectNodeFields) {
 		viewFieldKeys.add(fieldKey);
-		if (!stored.objectNodeFields.has(fieldKey)) {
+		if (
+			!stored.objectNodeFields.has(fieldKey) &&
+			fieldStoredSchema.kind !== storedEmptyFieldSchema.kind
+		) {
 			differences.push({
 				identifier: fieldKey,
 				mismatch: "fieldKind",
 				view: fieldStoredSchema.kind,
-				stored: undefined,
+				stored: storedEmptyFieldSchema.kind,
 			} satisfies FieldKindIncompatibility);
 		} else {
 			differences.push(
@@ -371,12 +375,15 @@ function trackObjectNodeDiscrepancies(
 		if (viewFieldKeys.has(fieldKey)) {
 			continue;
 		}
-		differences.push({
-			identifier: fieldKey,
-			mismatch: "fieldKind",
-			view: undefined,
-			stored: fieldStoredSchema.kind,
-		} satisfies FieldKindIncompatibility);
+
+		if (fieldStoredSchema.kind !== storedEmptyFieldSchema.kind) {
+			differences.push({
+				identifier: fieldKey,
+				mismatch: "fieldKind",
+				view: storedEmptyFieldSchema.kind,
+				stored: fieldStoredSchema.kind,
+			} satisfies FieldKindIncompatibility);
+		}
 	}
 
 	return differences;
@@ -444,17 +451,7 @@ function validateFieldIncompatibility(incompatibility: FieldIncompatibility): bo
 			return incompatibility.stored.length === 0;
 		}
 		case "fieldKind": {
-			if (incompatibility.stored === undefined) {
-				// Add an optional field
-				if (incompatibility.view === "Optional") {
-					return true;
-				}
-			} else {
-				// Relax the field to make it more general
-				return compareFieldKind(incompatibility.stored, incompatibility.view);
-			}
-
-			break;
+			return posetLte(incompatibility.stored, incompatibility.view, fieldRealizer);
 		}
 		case "valueSchema": {
 			return false;
@@ -566,17 +563,6 @@ function posetLte<T>(a: T, b: T, realizer: Realizer<T>): boolean {
 	);
 }
 
-function compareFieldKind(
-	aKind: FieldKindIdentifier | undefined,
-	bKind: FieldKindIdentifier | undefined,
-): boolean {
-	if (aKind === undefined || bKind === undefined) {
-		return aKind === bKind;
-	}
-
-	return posetLte(aKind, bKind, fieldRealizer);
-}
-
 function throwUnsupportedNodeType(type: string): never {
 	throw new TypeError(`Unsupported node stored schema type: ${type}`);
 }
diff --git a/packages/dds/tree/src/test/feature-libraries/modular-schema/discrepancies.spec.ts b/packages/dds/tree/src/test/feature-libraries/modular-schema/discrepancies.spec.ts
@@ -342,7 +342,7 @@ describe("Schema Discrepancies", () => {
 						{
 							identifier: "y",
 							mismatch: "fieldKind",
-							view: undefined,
+							view: "Forbidden",
 							stored: "Optional",
 						},
 					],
@@ -446,20 +446,16 @@ describe("Schema Discrepancies", () => {
 				root,
 			);
 
-			assert.deepEqual(getAllowedContentIncompatibilities(emptyTree, emptyLocalFieldTree), [
-				{
-					identifier: testTreeNodeIdentifier,
-					mismatch: "fields",
-					differences: [
-						{
-							identifier: "x",
-							mismatch: "fieldKind",
-							view: undefined,
-							stored: "Forbidden",
-						},
-					],
-				},
-			]);
+			assert.equal(
+				allowsRepoSuperset(defaultSchemaPolicy, emptyTree, emptyLocalFieldTree),
+				true,
+			);
+			assert.equal(
+				allowsRepoSuperset(defaultSchemaPolicy, emptyLocalFieldTree, emptyTree),
+				true,
+			);
+
+			assert.deepEqual(getAllowedContentIncompatibilities(emptyTree, emptyLocalFieldTree), []);
 
 			assert.deepEqual(getAllowedContentIncompatibilities(emptyTree, objectNodeSchema), [
 				{
@@ -469,7 +465,7 @@ describe("Schema Discrepancies", () => {
 						{
 							identifier: "x",
 							mismatch: "fieldKind",
-							view: undefined,
+							view: "Forbidden",
 							stored: "Value",
 						},
 					],