diff --git a/packages/dds/tree/src/core/schema-stored/schema.ts b/packages/dds/tree/src/core/schema-stored/schema.ts index 00ac01df93d2..c6176fd10d69 100644 --- a/packages/dds/tree/src/core/schema-stored/schema.ts +++ b/packages/dds/tree/src/core/schema-stored/schema.ts @@ -91,13 +91,24 @@ export enum ValueSchema { export type TreeTypeSet = ReadonlySet; /** - * Declarative portion of a Field Kind. + * Declarative portion of a {@link FlexFieldKind}. * * @remarks * Enough info about a field kind to know if a given tree is is schema. + * + * Note that compatibility between trees and schema is not sufficient to evaluate if a schema upgrade should be allowed. + * Currently schema upgrades are restricted to field kind changes which can not be cyclic (like version upgrades but not down grades). + * See {@link FlexFieldKind.allowsFieldSuperset} for more details. */ export interface FieldKindData { + /** + * Globally scoped identifier. + */ readonly identifier: FieldKindIdentifier; + /** + * Bound on the number of children that fields of this kind may have. + * TODO: consider replacing this with numeric upper and lower bounds. + */ readonly multiplicity: Multiplicity; } 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 4a51af3699e1..972c6990687f 100644 --- a/packages/dds/tree/src/feature-libraries/default-schema/defaultFieldKinds.ts +++ b/packages/dds/tree/src/feature-libraries/default-schema/defaultFieldKinds.ts @@ -59,7 +59,15 @@ export const noChangeHandler: FieldChangeHandler<0> = { getCrossFieldKeys: () => [], }; -export interface ValueFieldEditor extends FieldEditor { +/** + * {@link FieldEditor} for required fields (always contain exactly 1 child). + * @remarks + * This shares code with optional fields, since they are the same edit wise except setting to empty is not allowed, + * and the content is always assumed to not be empty. + * This means the actual edits implemented for optional fields are sufficient to support required fields + * which is why this is defined and implemented in terms of optional fields. + */ +export interface RequiredFieldEditor extends FieldEditor { /** * Creates a change which replaces the current value of the field with `newValue`. * @param ids - The ids for the fill and detach fields. @@ -68,20 +76,18 @@ export interface ValueFieldEditor extends FieldEditor { } const optionalIdentifier = brandConst("Optional")(); + /** * 0 or 1 items. */ -export const optional = new FlexFieldKind( - optionalIdentifier, - Multiplicity.Optional, - optionalChangeHandler, - (types, other) => +export const optional = new FlexFieldKind(optionalIdentifier, Multiplicity.Optional, { + changeHandler: optionalChangeHandler, + allowsTreeSupersetOf: (types, other) => (other.kind === sequence.identifier || other.kind === optionalIdentifier) && allowsTreeSchemaIdentifierSuperset(types, other.types), - new Set([]), -); +}); -export const valueFieldEditor: ValueFieldEditor = { +export const requiredFieldEditor: RequiredFieldEditor = { ...optionalFieldEditor, set: (ids: { fill: ChangeAtomId; @@ -89,9 +95,12 @@ export const valueFieldEditor: ValueFieldEditor = { }): OptionalChangeset => optionalFieldEditor.set(false, ids), }; -export const valueChangeHandler: FieldChangeHandler = { - ...optional.changeHandler, - editor: valueFieldEditor, +export const requiredFieldChangeHandler: FieldChangeHandler< + OptionalChangeset, + RequiredFieldEditor +> = { + ...optionalChangeHandler, + editor: requiredFieldEditor, }; const requiredIdentifier = brandConst("Value")(); @@ -99,11 +108,9 @@ const requiredIdentifier = brandConst("Value")(); /** * Exactly one item. */ -export const required = new FlexFieldKind( - requiredIdentifier, - Multiplicity.Single, - valueChangeHandler, - (types, other) => +export const required = new FlexFieldKind(requiredIdentifier, Multiplicity.Single, { + changeHandler: requiredFieldChangeHandler, + allowsTreeSupersetOf: (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, @@ -113,43 +120,35 @@ export const required = new FlexFieldKind( other.kind === requiredIdentifier || other.kind === optional.identifier) && allowsTreeSchemaIdentifierSuperset(types, other.types), - new Set(), -); +}); const sequenceIdentifier = brandConst("Sequence")(); /** * 0 or more items. */ -export const sequence = new FlexFieldKind( - sequenceIdentifier, - Multiplicity.Sequence, - sequenceFieldChangeHandler, - (types, other) => +export const sequence = new FlexFieldKind(sequenceIdentifier, Multiplicity.Sequence, { + changeHandler: sequenceFieldChangeHandler, + allowsTreeSupersetOf: (types, other) => other.kind === sequenceIdentifier && allowsTreeSchemaIdentifierSuperset(types, other.types), - // TODO: add normalizer/importers for handling ops from other kinds. - new Set([]), -); +}); const identifierFieldIdentifier = brandConst("Identifier")(); /** * Exactly one identifier. */ -export const identifier = new FlexFieldKind( - identifierFieldIdentifier, - Multiplicity.Single, - noChangeHandler, - (types, other) => +export const identifier = new FlexFieldKind(identifierFieldIdentifier, Multiplicity.Single, { + changeHandler: noChangeHandler, + allowsTreeSupersetOf: (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 || other.kind === optional.identifier || other.kind === identifierFieldIdentifier) && allowsTreeSchemaIdentifierSuperset(types, other.types), - new Set(), -); +}); /** * Exactly 0 items. @@ -182,10 +181,12 @@ export const identifier = new FlexFieldKind( export const forbidden = new FlexFieldKind( forbiddenFieldKindIdentifier, Multiplicity.Forbidden, - noChangeHandler, - // All multiplicities other than Value support empty. - (types, other) => fieldKinds.get(other.kind)?.multiplicity !== Multiplicity.Single, - new Set(), + { + changeHandler: noChangeHandler, + // All multiplicities other than Value support empty. + allowsTreeSupersetOf: (types, other) => + fieldKinds.get(other.kind)?.multiplicity !== Multiplicity.Single, + }, ); export const fieldKindConfigurations: ReadonlyMap< @@ -270,15 +271,31 @@ export const fieldKinds: ReadonlyMap = new M // TODO: ensure thy work in generated docs. // TODO: add these comments to the rest of the cases below. export interface Required - extends FlexFieldKind {} + extends FlexFieldKind {} export interface Optional - extends FlexFieldKind {} + extends FlexFieldKind< + OptionalFieldEditor, + typeof optionalIdentifier, + Multiplicity.Optional + > {} export interface Sequence - extends FlexFieldKind {} + extends FlexFieldKind< + SequenceFieldEditor, + typeof sequenceIdentifier, + Multiplicity.Sequence + > {} export interface Identifier - extends FlexFieldKind, "Identifier", Multiplicity.Single> {} + extends FlexFieldKind< + FieldEditor<0>, + typeof identifierFieldIdentifier, + Multiplicity.Single + > {} export interface Forbidden - extends FlexFieldKind, "Forbidden", Multiplicity.Forbidden> {} + extends FlexFieldKind< + FieldEditor<0>, + typeof forbiddenFieldKindIdentifier, + Multiplicity.Forbidden + > {} /** * Default FieldKinds with their editor types erased. 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 3245141016e9..f3ea1d7061c6 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/fieldKind.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/fieldKind.ts @@ -36,43 +36,34 @@ export class FlexFieldKind< // TODO: stronger typing // eslint-disable-next-line @typescript-eslint/no-explicit-any TEditor extends FieldEditor = FieldEditor, - TName extends string = string, + TName extends FieldKindIdentifier = FieldKindIdentifier, TMultiplicity extends Multiplicity = Multiplicity, > implements FieldKindData { protected _typeCheck!: MakeNominal; /** - * @param identifier - Globally scoped identifier. - * @param multiplicity - bound on the number of children that fields of this kind may have. - * TODO: replace with numeric upper and lower bounds. - * @param changeHandler - Change handling policy. - * @param allowsTreeSupersetOf - returns true iff `superset` supports all that this does - * and `superset` is an allowed upgrade. Does not have to handle the `never` cases. - * See {@link isNeverField}. - * TODO: when used as a method (instead of a free function like the other superset related functions), - * this name is/signature is confusing and seems backwards. - * @param handlesEditsFrom - Kinds (in addition to this) whose edits can be processed by changeHandler. - * If the kind of a field changes, and edits are rebased across that kind change, - * listing the other old kind here can prevent those edits from being conflicted and - * provide a chance to handle them. + * Change handling policy. */ + // TODO: stronger typing + // eslint-disable-next-line @typescript-eslint/no-explicit-any + public readonly changeHandler: FieldChangeHandler; + public constructor( - public readonly identifier: TName & FieldKindIdentifier, + public readonly identifier: TName, public readonly multiplicity: TMultiplicity, // TODO: stronger typing // eslint-disable-next-line @typescript-eslint/no-explicit-any - public readonly changeHandler: FieldChangeHandler, - private readonly allowsTreeSupersetOf: ( - originalTypes: TreeTypeSet, - superset: TreeFieldStoredSchema, - ) => boolean, - public readonly handlesEditsFrom: ReadonlySet, - ) {} + private readonly options: FieldKindOptions>, + ) { + this.changeHandler = options.changeHandler; + } /** * Returns true if and only if `superset` permits a (non-strict) superset of the subtrees * allowed by field made from `this` and `originalTypes`. + * + * TODO: clarify the relationship between this and FieldKindData, and issues with cyclic schema upgrades. */ public allowsFieldSuperset( policy: SchemaPolicy, @@ -93,10 +84,43 @@ export class FlexFieldKind< if (isNeverField(policy, originalData, superset)) { return false; } - return this.allowsTreeSupersetOf(originalTypes, superset); + return this.options.allowsTreeSupersetOf(originalTypes, superset); } } +/** + * Additional options for {@link FlexFieldKind}. + * + * @remarks + * Puts the more confusing parameters into this object so they get explicit names to help with clarity. + */ +export interface FieldKindOptions { + /** + * Change handling policy. + */ + 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`. + * @remarks + * Used by {@link FlexFieldKind.allowsFieldSuperset}, which handles the `never` cases before calling this. + */ + readonly allowsTreeSupersetOf: ( + originalTypes: TreeTypeSet, + superset: TreeFieldStoredSchema, + ) => boolean; + + /** + * Kinds (in addition to this) whose edits can be processed by changeHandler. + * If the kind of a field changes, and edits are rebased across that kind change, + * listing the other old kind here can prevent those edits from being conflicted and + * provide a chance to handle them. + */ + // TODO: provide this and use it for improved support for rebasing changes across schema upgrades. + // readonly handlesEditsFrom: ReadonlySet; +} + /** * Policy from the app for interpreting the stored schema. * The app must ensure consistency for all users of the document. 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 909becbab606..721466b197d3 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts @@ -169,9 +169,7 @@ function replaceRevisions( export const genericFieldKind: FlexFieldKind = new FlexFieldKind( brandConst("ModularEditBuilder.Generic")(), Multiplicity.Sequence, - genericChangeHandler, - (types, other) => false, - new Set(), + { changeHandler: genericChangeHandler, allowsTreeSupersetOf: (types, other) => false }, ); /** diff --git a/packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultFieldKinds.spec.ts b/packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultFieldKinds.spec.ts index 4e00abb5d192..2bd4572128a4 100644 --- a/packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultFieldKinds.spec.ts +++ b/packages/dds/tree/src/test/feature-libraries/default-field-kinds/defaultFieldKinds.spec.ts @@ -7,9 +7,9 @@ import { strict as assert, fail } from "node:assert"; import { makeAnonChange } from "../../../core/index.js"; import { - type ValueFieldEditor, - valueChangeHandler, - valueFieldEditor, + type RequiredFieldEditor, + requiredFieldChangeHandler, + requiredFieldEditor, // Allow import from file being tested. // eslint-disable-next-line import-x/no-internal-modules } from "../../../feature-libraries/default-schema/defaultFieldKinds.js"; @@ -62,15 +62,15 @@ const childComposer1_2 = ( }; describe("defaultFieldKinds", () => { - describe("valueFieldEditor.set", () => { - it("valueFieldEditor.set", () => { + describe("requiredFieldEditor.set", () => { + it("requiredFieldEditor.set", () => { const expected = Change.atOnce( Change.clear("self", brand(1)), Change.move(brand(41), "self"), ); const revision = mintRevisionTag(); assertEqual( - valueFieldEditor.set({ + requiredFieldEditor.set({ detach: { localId: brand(1), revision }, fill: { localId: brand(41), revision }, }), @@ -80,11 +80,11 @@ describe("defaultFieldKinds", () => { }); // TODO: - // These tests are covering value field usage patterns of optional field's rebaser (which value field uses). + // These tests are covering required field usage patterns of optional field's rebaser (which required field uses). // These patterns should be covered in the optional field tests and not be needed here (except perhaps for a minimal integration test). - describe("value field rebaser", () => { - const fieldHandler: FieldChangeHandler = - valueChangeHandler; + describe("required field rebaser", () => { + const fieldHandler: FieldChangeHandler = + requiredFieldChangeHandler; const childChange1 = Change.child(nodeChange1); const childChange2 = Change.child(nodeChange2); 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 89d522ed8091..7139e07d454c 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,7 +113,5 @@ export const valueHandler = { export const valueField = new FlexFieldKind( brandConst("Value")(), Multiplicity.Single, - valueHandler, - (a, b) => false, - new Set(), + { changeHandler: valueHandler, allowsTreeSupersetOf: (a, b) => false }, ); 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 bcde74d02c90..485864478503 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,9 +165,7 @@ const singleNodeHandler: FieldChangeHandler = { const singleNodeField = new FlexFieldKind( brandConst("SingleNode")(), Multiplicity.Single, - singleNodeHandler, - (a, b) => false, - new Set(), + { changeHandler: singleNodeHandler, allowsTreeSupersetOf: (a, b) => false }, ); export const fieldKindConfiguration: FieldKindConfiguration = new Map< @@ -1172,13 +1170,10 @@ describe("ModularChangeFamily", () => { ]; }, } as unknown as FieldChangeHandler>; - const hasRemovedRootsRefsField = new FlexFieldKind( - fieldKind, - Multiplicity.Single, - handler, - () => false, - new Set(), - ); + const hasRemovedRootsRefsField = new FlexFieldKind(fieldKind, Multiplicity.Single, { + changeHandler: handler, + allowsTreeSupersetOf: (a, b) => false, + }); const mockFieldKinds = new Map([[fieldKind, hasRemovedRootsRefsField]]); function relevantRemovedRoots(input: ModularChangeset): DeltaDetachedNodeId[] {