diff --git a/packages/dds/tree/src/feature-libraries/default-schema/defaultFieldKinds.ts b/packages/dds/tree/src/feature-libraries/default-schema/defaultFieldKinds.ts index dbb1f770b358..75ffdcedb288 100644 --- a/packages/dds/tree/src/feature-libraries/default-schema/defaultFieldKinds.ts +++ b/packages/dds/tree/src/feature-libraries/default-schema/defaultFieldKinds.ts @@ -82,7 +82,7 @@ const optionalIdentifier = brandConst("Optional")(); */ export const optional = new FlexFieldKind(optionalIdentifier, Multiplicity.Optional, { changeHandler: optionalChangeHandler, - allowsTreeSupersetOf: (types, other) => + allowedMonotonicUpgrade: (types, other) => (other.kind === sequence.identifier || other.kind === optionalIdentifier) && allowsTreeSchemaIdentifierSuperset(types, other.types), }); @@ -110,7 +110,7 @@ const requiredIdentifier = brandConst("Value")(); */ export const required = new FlexFieldKind(requiredIdentifier, Multiplicity.Single, { changeHandler: requiredFieldChangeHandler, - allowsTreeSupersetOf: (types, other) => + allowedMonotonicUpgrade: (types, other) => // By omitting Identifier here, // this is making a policy choice that a schema upgrade cannot be done from required to identifier. // Since an identifier can be upgraded into a required field, @@ -129,7 +129,7 @@ const sequenceIdentifier = brandConst("Sequence")(); */ export const sequence = new FlexFieldKind(sequenceIdentifier, Multiplicity.Sequence, { changeHandler: sequenceFieldChangeHandler, - allowsTreeSupersetOf: (types, other) => + allowedMonotonicUpgrade: (types, other) => other.kind === sequenceIdentifier && allowsTreeSchemaIdentifierSuperset(types, other.types), }); @@ -141,7 +141,7 @@ const identifierFieldIdentifier = brandConst("Identifier")( */ export const identifier = new FlexFieldKind(identifierFieldIdentifier, Multiplicity.Single, { changeHandler: noChangeHandler, - allowsTreeSupersetOf: (types, other) => + allowedMonotonicUpgrade: (types, other) => // Allows upgrading from identifier to required: which way this upgrade is allowed to go is a subjective policy choice. (other.kind === sequence.identifier || other.kind === requiredIdentifier || @@ -184,7 +184,7 @@ export const forbidden = new FlexFieldKind( { changeHandler: noChangeHandler, // All multiplicities other than Value support empty. - allowsTreeSupersetOf: (types, other) => + allowedMonotonicUpgrade: (types, other) => fieldKinds.get(other.kind)?.multiplicity !== Multiplicity.Single, }, ); 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 69f1738cd11e..92f7ff1777a8 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/comparison.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/comparison.ts @@ -134,7 +134,7 @@ export function allowsFieldSuperset( ): boolean { return ( policy.fieldKinds.get(original.kind) ?? fail(0xb1b /* missing kind */) - ).allowsFieldSuperset(policy, originalData, original.types, superset); + ).allowedMonotonicUpgrade(policy, originalData, original.types, superset); } /** diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/fieldKind.ts b/packages/dds/tree/src/feature-libraries/modular-schema/fieldKind.ts index f3ea1d7061c6..a1b58f697673 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/fieldKind.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/fieldKind.ts @@ -60,12 +60,13 @@ export class FlexFieldKind< } /** - * Returns true if and only if `superset` permits a (non-strict) superset of the subtrees - * allowed by field made from `this` and `originalTypes`. + * True if a client should be able to {@link TreeView.upgradeSchema} from a field schema using this field kind and `originalTypes` to `superset`. + * @remarks * - * TODO: clarify the relationship between this and FieldKindData, and issues with cyclic schema upgrades. + * @privateRemarks + * See also {@link FieldKindOptions.allowedMonotonicUpgrade} for constraints on the implementation. */ - public allowsFieldSuperset( + public allowedMonotonicUpgrade( policy: SchemaPolicy, originalData: TreeStoredSchema, originalTypes: TreeTypeSet, @@ -84,7 +85,7 @@ export class FlexFieldKind< if (isNeverField(policy, originalData, superset)) { return false; } - return this.options.allowsTreeSupersetOf(originalTypes, superset); + return this.options.allowedMonotonicUpgrade(originalTypes, superset); } } @@ -101,12 +102,29 @@ export interface FieldKindOptions { readonly changeHandler: TFieldChangeHandler; /** - * Returns true if and only if `superset` permits a (non-strict) superset of the subtrees - * allowed by field made from `this` and `originalTypes`. + * True if a client should be able to {@link TreeView.upgradeSchema} from a field schema using this field kind and `originalTypes` to `superset`. * @remarks - * Used by {@link FlexFieldKind.allowsFieldSuperset}, which handles the `never` cases before calling this. + * Must return false if such an upgrade could violate any invariants of `superset` for any document which is compatible with `this` + `originalTypes`. + * + * Unlike the rest of the FieldKind API, this function may change over time without changing the FieldKind identifier without causing decoherence between clients. + * It has a different set of constraints: + * + * - This must never allow an upgrade that could violate any invariants of any field kind required by any version of client for any document content (that was not already invalid). + * This prevents a schema upgrade from causing a document to become out of schema. + * - The set of implementations of this function across all fields kinds and all client versions must never permit a cycle. + * This prevents applications which simply do an upgrade when possible from being abler to have two clients both upgrade where one is actually a down grade and they cause an infinite loop of schema upgrade edits. + * + * To help maintain these invariants, any cases where the allowed document content does not increase (but is the same so the upgrade is still in schema) must be considered carefully. + * Such cases, if allowed, could lead to cycles if their inverse is also allowed. + * These cases, if supported, can be removed, but if doing so must be documented and still considered when avoiding cycles. + * + * Used by {@link FlexFieldKind.allowedMonotonicUpgrade}, which handles the `never` cases (empty type sets) before calling this: the implementer does not need to handle those. + * + * TODO: this design is rather limiting, and there is some planned work in this area: + * - Provide schema upgrade schema not reliant on this to ensure monotonicity of schema upgrades like AB#7482. + * - This monotonic upgrade scheme can remain useful for features like AB#53604 so it should be kept and refined. */ - readonly allowsTreeSupersetOf: ( + readonly allowedMonotonicUpgrade: ( originalTypes: TreeTypeSet, superset: TreeFieldStoredSchema, ) => boolean; diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts b/packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts index 721466b197d3..a382848ec5b1 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts @@ -169,7 +169,7 @@ function replaceRevisions( export const genericFieldKind: FlexFieldKind = new FlexFieldKind( brandConst("ModularEditBuilder.Generic")(), Multiplicity.Sequence, - { changeHandler: genericChangeHandler, allowsTreeSupersetOf: (types, other) => false }, + { changeHandler: genericChangeHandler, allowedMonotonicUpgrade: (types, other) => false }, ); /** diff --git a/packages/dds/tree/src/test/feature-libraries/modular-schema/basicRebasers.ts b/packages/dds/tree/src/test/feature-libraries/modular-schema/basicRebasers.ts index 7139e07d454c..5500932d01f0 100644 --- a/packages/dds/tree/src/test/feature-libraries/modular-schema/basicRebasers.ts +++ b/packages/dds/tree/src/test/feature-libraries/modular-schema/basicRebasers.ts @@ -113,5 +113,5 @@ export const valueHandler = { export const valueField = new FlexFieldKind( brandConst("Value")(), Multiplicity.Single, - { changeHandler: valueHandler, allowsTreeSupersetOf: (a, b) => false }, + { changeHandler: valueHandler, allowedMonotonicUpgrade: (a, b) => false }, ); diff --git a/packages/dds/tree/src/test/feature-libraries/modular-schema/comparison.spec.ts b/packages/dds/tree/src/test/feature-libraries/modular-schema/comparison.spec.ts index 081097d4d774..09b31dc01622 100644 --- a/packages/dds/tree/src/test/feature-libraries/modular-schema/comparison.spec.ts +++ b/packages/dds/tree/src/test/feature-libraries/modular-schema/comparison.spec.ts @@ -185,7 +185,7 @@ describe("Schema Comparison", () => { updateTreeSchema(repo, emptyTree.name, emptyTree.schema); const identifierField = fieldSchema(FieldKinds.identifier, [emptyTree.name]); { - const result = FieldKinds.required.allowsFieldSuperset( + const result = FieldKinds.required.allowedMonotonicUpgrade( defaultSchemaPolicy, repo, new Set([emptyTree.name]), @@ -194,7 +194,7 @@ describe("Schema Comparison", () => { assert.equal(result, false); } { - const result = FieldKinds.identifier.allowsFieldSuperset( + const result = FieldKinds.identifier.allowedMonotonicUpgrade( defaultSchemaPolicy, repo, new Set([emptyTree.name]), diff --git a/packages/dds/tree/src/test/feature-libraries/modular-schema/modularChangeFamily.spec.ts b/packages/dds/tree/src/test/feature-libraries/modular-schema/modularChangeFamily.spec.ts index 485864478503..c7a7d8d449ea 100644 --- a/packages/dds/tree/src/test/feature-libraries/modular-schema/modularChangeFamily.spec.ts +++ b/packages/dds/tree/src/test/feature-libraries/modular-schema/modularChangeFamily.spec.ts @@ -165,7 +165,7 @@ const singleNodeHandler: FieldChangeHandler = { const singleNodeField = new FlexFieldKind( brandConst("SingleNode")(), Multiplicity.Single, - { changeHandler: singleNodeHandler, allowsTreeSupersetOf: (a, b) => false }, + { changeHandler: singleNodeHandler, allowedMonotonicUpgrade: (a, b) => false }, ); export const fieldKindConfiguration: FieldKindConfiguration = new Map< @@ -1172,7 +1172,7 @@ describe("ModularChangeFamily", () => { } as unknown as FieldChangeHandler>; const hasRemovedRootsRefsField = new FlexFieldKind(fieldKind, Multiplicity.Single, { changeHandler: handler, - allowsTreeSupersetOf: (a, b) => false, + allowedMonotonicUpgrade: (a, b) => false, }); const mockFieldKinds = new Map([[fieldKind, hasRemovedRootsRefsField]]);