From cc3fb6da963abc7871e86901076969764351b2bc Mon Sep 17 00:00:00 2001
From: Abram Sanderson <Abram.Sanderson@microsoft.com>
Date: Tue, 12 Nov 2024 14:49:59 -0800
Subject: [PATCH] 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 <absander@microsoft.com>
---
 .../modular-schema/discrepancies.ts           | 50 +++++++------------
 .../modular-schema/discrepancies.spec.ts      | 28 +++++------
 2 files changed, 30 insertions(+), 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
index e24df9c299c..613a94cf730 100644
--- 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
index 7e6707b929f..7d2e53bd411 100644
--- 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",
 						},
 					],