diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/comparison.ts b/packages/dds/tree/src/feature-libraries/modular-schema/comparison.ts index 7b9cf2de884..c603ad6b6a2 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/comparison.ts +++ b/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 */, 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 19c1871e47d..e24df9c299c 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/discrepancies.ts +++ b/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. - * - * 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. - * - * TODO: We may need more coverage in realm to prove the correctness of the Forbidden -\> Value transaction + * The linear extension is represented as a lookup from each poset element to its index in the linear extension. */ -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. +type LinearExtension = Map; + +/** + * A realizer for a partially-ordered set. See: + * https://en.wikipedia.org/wiki/Order_dimension + */ +type Realizer = LinearExtension[]; + +/** + * @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("Forbidden"), + required: brand("Value"), + identifier: brand("Identifier"), + optional: brand("Optional"), + sequence: brand("Sequence"), }; +/** + * A realizer for the partial order of field kind relaxability. + * + * 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. + * + * Hasse diagram for the partial order is shown below (lower fields can be relaxed to higher fields): + * ``` + * sequence + * | + * optional + * | \ + * required forbidden + * | + * identifier + * ``` + */ +const fieldRealizer: Realizer = [ + [ + 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(a: T, b: T, realizer: Realizer): 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(a: T, b: T, realizer: Realizer): 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; + return aKind === bKind; } - if (!(aKind in fieldKindOrder) || !(bKind in fieldKindOrder)) { - return false; - } - - // 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 { 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 7a8a6542746..5f1e5e7185e 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 @@ -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("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); + }); + } + }); }); });