diff --git a/packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts b/packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts index 7b3fb03f15c2..f04c82be3d76 100644 --- a/packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts +++ b/packages/dds/tree/src/feature-libraries/default-schema/defaultEditBuilder.ts @@ -160,8 +160,17 @@ export interface IDefaultEditBuilder { destinationIndex: number, ): void; - // TODO: document + /** + * Add a constraint that the node at the given path must exist. + * @param path - The path to the node that must exist. + */ addNodeExistsConstraint(path: UpPath): void; + + /** + * Add a constraint that the node at the given path must exist when reverting a change. + * @param path - The path to the node that must exist when reverting a change. + */ + addNodeExistsConstraintOnRevert(path: UpPath): void; } /** @@ -191,6 +200,10 @@ export class DefaultEditBuilder implements ChangeFamilyEditor, IDefaultEditBuild this.modularBuilder.addNodeExistsConstraint(path, this.mintRevisionTag()); } + public addNodeExistsConstraintOnRevert(path: UpPath): void { + this.modularBuilder.addNodeExistsConstraintOnRevert(path, this.mintRevisionTag()); + } + public valueField(field: FieldUpPath): ValueFieldEditBuilder { return { set: (newContent: ITreeCursorSynchronous): void => { diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/fieldChangeHandler.ts b/packages/dds/tree/src/feature-libraries/modular-schema/fieldChangeHandler.ts index 7338c849567c..8237c9718672 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/fieldChangeHandler.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/fieldChangeHandler.ts @@ -20,6 +20,12 @@ import type { CrossFieldManager } from "./crossFieldQueries.js"; import type { CrossFieldKeyRange, NodeId } from "./modularChangeTypes.js"; import type { EncodedNodeChangeset } from "./modularChangeFormat.js"; +export type NestedChangesIndices = [ + NodeId, + number | undefined /* inputIndex */, + number | undefined /* outputIndex */, +][]; + /** * Functionality provided by a field kind which will be composed with other `FieldChangeHandler`s to * implement a unified ChangeFamily supporting documents with multiple field kinds. @@ -75,13 +81,16 @@ export interface FieldChangeHandler< * @param change - The field change to get the child changes from. * * @returns The set of `NodeId`s that correspond to nested changes in the given `change`. - * Each `NodeId` is associated with the index of the node in the field in the input context of the changeset - * (or `undefined` if the node is not attached in the input context). + * Each `NodeId` is associated with the following: + * - index of the node in the field in the input context of the changeset (or `undefined` if the node is not + * attached in the input context). + * - index of the node in the field in the output context of the changeset (or `undefined` if the node is not + * attached in the output context). * For all returned entries where the index is defined, * the indices are are ordered from smallest to largest (with no duplicates). * The returned array is owned by the caller. */ - getNestedChanges(change: TChangeset): [NodeId, number | undefined][]; + getNestedChanges(change: TChangeset): NestedChangesIndices; /** * @returns A list of all cross-field keys contained in the change. 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 473a97943789..fbf5b1ad823b 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/genericFieldKind.ts @@ -17,6 +17,7 @@ import { assert } from "@fluidframework/core-utils/internal"; import type { CrossFieldManager } from "./crossFieldQueries.js"; import type { FieldChangeHandler, + NestedChangesIndices, NodeChangeComposer, NodeChangePruner, NodeChangeRebaser, @@ -82,8 +83,9 @@ function compose( return composed; } -function getNestedChanges(change: GenericChangeset): [NodeId, number | undefined][] { - return change.toArray().map(([index, nodeChange]) => [nodeChange, index]); +function getNestedChanges(change: GenericChangeset): NestedChangesIndices { + // For generic changeset, the indices in the input and output contexts are the same. + return change.toArray().map(([index, nodeChange]) => [nodeChange, index, index]); } function rebaseGenericChange( diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/index.ts b/packages/dds/tree/src/feature-libraries/modular-schema/index.ts index a13179d02c28..d829136aed98 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/index.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/index.ts @@ -41,6 +41,7 @@ export { type ToDelta, NodeAttachState, type FieldChangeEncodingContext, + type NestedChangesIndices, } from "./fieldChangeHandler.js"; export type { CrossFieldKeyRange, diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts b/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts index da90aed60095..da1103122496 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeFamily.ts @@ -649,8 +649,21 @@ export class ModularChangeFamily crossFieldTable: ComposeTable, revisionMetadata: RevisionMetadataSource, ): NodeChangeset { + // WARNING: this composition logic assumes that we never make compositions of the following form: + // change1: a changeset that impact the existence of a node + // change2: a node-exists constraint on that node. + // This is currently enforced by the fact that constraints which apply to the input context are included first in the composition. + // If that weren't the case, we would need to rebase the status of the constraint backward over the changes from change1. const nodeExistsConstraint = change1.nodeExistsConstraint ?? change2.nodeExistsConstraint; + // WARNING: this composition logic assumes that we never make compositions of the following form: + // change1: a node-exists-on-revert constraint on a node + // change2: a changeset that impacts the existence of that node + // This is currently enforced by the fact that constraints which apply to the revert are included last in the composition. + // If that weren't the case, we would need to rebase the status of the constraint forward over the changes from change2. + const nodeExistsConstraintOnRevert = + change1.nodeExistsConstraintOnRevert ?? change2.nodeExistsConstraintOnRevert; + const composedFieldChanges = this.composeFieldMaps( change1.fieldChanges, change2.fieldChanges, @@ -670,6 +683,10 @@ export class ModularChangeFamily composedNodeChange.nodeExistsConstraint = nodeExistsConstraint; } + if (nodeExistsConstraintOnRevert !== undefined) { + composedNodeChange.nodeExistsConstraintOnRevert = nodeExistsConstraintOnRevert; + } + return composedNodeChange; } @@ -778,7 +795,8 @@ export class ModularChangeFamily crossFieldKeys, maxId: genId.getMaxId(), revisions: revInfos, - constraintViolationCount: change.change.constraintViolationCount, + constraintViolationCount: change.change.constraintViolationCountOnRevert, + constraintViolationCountOnRevert: change.change.constraintViolationCount, destroys, }); } @@ -835,6 +853,19 @@ export class ModularChangeFamily ): NodeChangeset { const inverse: NodeChangeset = {}; + // If the node has a constraint, it should be inverted to a node-exist-on-revert constraint. This ensure that if + // the inverse is inverted again, the original input constraint will be restored. + if (change.nodeExistsConstraint !== undefined) { + inverse.nodeExistsConstraintOnRevert = change.nodeExistsConstraint; + } + + // The node-exist-on-revert constraint of a node is the constraint that should apply when the a change is reverted. + // So, it should become the constraint in the inverse. If this constraint is violated when applying the inverse, + // it will be discarded. + if (change.nodeExistsConstraintOnRevert !== undefined) { + inverse.nodeExistsConstraint = change.nodeExistsConstraintOnRevert; + } + if (change.fieldChanges !== undefined) { inverse.fieldChanges = this.invertFieldMap( change.fieldChanges, @@ -878,8 +909,6 @@ export class ModularChangeFamily fieldsWithUnattachedChild: new Set(), }; - const constraintState = newConstraintState(change.constraintViolationCount ?? 0); - const getBaseRevisions = (): RevisionTag[] => revisionInfoFromTaggedChange(over).map((info) => info.revision); @@ -895,7 +924,6 @@ export class ModularChangeFamily crossFieldTable, rebasedNodes, genId, - constraintState, rebaseMetadata, ); @@ -907,10 +935,16 @@ export class ModularChangeFamily genId, ); + const constraintState = newConstraintState(change.constraintViolationCount ?? 0); + const revertConstraintState = newConstraintState( + change.constraintViolationCountOnRevert ?? 0, + ); this.updateConstraintsForFields( rebasedFields, NodeAttachState.Attached, + NodeAttachState.Attached, constraintState, + revertConstraintState, rebasedNodes, ); @@ -923,6 +957,7 @@ export class ModularChangeFamily maxId: idState.maxId, revisions: change.revisions, constraintViolationCount: constraintState.violationCount, + constraintViolationCountOnRevert: revertConstraintState.violationCount, builds: change.builds, destroys: change.destroys, refreshers: change.refreshers, @@ -937,7 +972,6 @@ export class ModularChangeFamily crossFieldTable: RebaseTable, rebasedNodes: ChangeAtomIdBTree, genId: IdAllocator, - constraintState: ConstraintState, metadata: RebaseRevisionMetadata, ): FieldChangeMap { const change = crossFieldTable.newChange; @@ -960,7 +994,6 @@ export class ModularChangeFamily genId, crossFieldTable, metadata, - constraintState, ); setInChangeAtomIdMap(rebasedNodes, newId, rebasedNode); @@ -1336,7 +1369,6 @@ export class ModularChangeFamily genId: IdAllocator, crossFieldTable: RebaseTable, revisionMetadata: RebaseRevisionMetadata, - constraintState: ConstraintState, ): NodeChangeset { const change = nodeChangeFromId(crossFieldTable.newChange.nodeChanges, newId); const over = nodeChangeFromId(crossFieldTable.baseChange.nodeChanges, baseId); @@ -1365,38 +1397,58 @@ export class ModularChangeFamily rebasedChange.nodeExistsConstraint = change.nodeExistsConstraint; } + if (change?.nodeExistsConstraintOnRevert !== undefined) { + rebasedChange.nodeExistsConstraintOnRevert = change.nodeExistsConstraintOnRevert; + } + setInChangeAtomIdMap(crossFieldTable.baseToRebasedNodeId, baseId, newId); return rebasedChange; } private updateConstraintsForFields( fields: FieldChangeMap, - parentAttachState: NodeAttachState, + parentInputAttachState: NodeAttachState, + parentOutputAttachState: NodeAttachState, constraintState: ConstraintState, + revertConstraintState: ConstraintState, nodes: ChangeAtomIdBTree, ): void { for (const field of fields.values()) { const handler = getChangeHandler(this.fieldKinds, field.fieldKind); - for (const [nodeId, index] of handler.getNestedChanges(field.change)) { - const isDetached = index === undefined; - const attachState = - parentAttachState === NodeAttachState.Detached || isDetached + for (const [nodeId, inputIndex, outputIndex] of handler.getNestedChanges(field.change)) { + const isInputDetached = inputIndex === undefined; + const inputAttachState = + parentInputAttachState === NodeAttachState.Detached || isInputDetached + ? NodeAttachState.Detached + : NodeAttachState.Attached; + const isOutputDetached = outputIndex === undefined; + const outputAttachState = + parentOutputAttachState === NodeAttachState.Detached || isOutputDetached ? NodeAttachState.Detached : NodeAttachState.Attached; - this.updateConstraintsForNode(nodeId, attachState, constraintState, nodes); + this.updateConstraintsForNode( + nodeId, + inputAttachState, + outputAttachState, + nodes, + constraintState, + revertConstraintState, + ); } } } private updateConstraintsForNode( nodeId: NodeId, - attachState: NodeAttachState, - constraintState: ConstraintState, + inputAttachState: NodeAttachState, + outputAttachState: NodeAttachState, nodes: ChangeAtomIdBTree, + constraintState: ConstraintState, + revertConstraintState: ConstraintState, ): void { const node = nodes.get([nodeId.revision, nodeId.localId]) ?? fail("Unknown node ID"); if (node.nodeExistsConstraint !== undefined) { - const isNowViolated = attachState === NodeAttachState.Detached; + const isNowViolated = inputAttachState === NodeAttachState.Detached; if (node.nodeExistsConstraint.violated !== isNowViolated) { node.nodeExistsConstraint = { ...node.nodeExistsConstraint, @@ -1405,9 +1457,26 @@ export class ModularChangeFamily constraintState.violationCount += isNowViolated ? 1 : -1; } } + if (node.nodeExistsConstraintOnRevert !== undefined) { + const isNowViolated = outputAttachState === NodeAttachState.Detached; + if (node.nodeExistsConstraintOnRevert.violated !== isNowViolated) { + node.nodeExistsConstraintOnRevert = { + ...node.nodeExistsConstraintOnRevert, + violated: isNowViolated, + }; + revertConstraintState.violationCount += isNowViolated ? 1 : -1; + } + } if (node.fieldChanges !== undefined) { - this.updateConstraintsForFields(node.fieldChanges, attachState, constraintState, nodes); + this.updateConstraintsForFields( + node.fieldChanges, + inputAttachState, + outputAttachState, + constraintState, + revertConstraintState, + nodes, + ); } } @@ -1907,6 +1976,7 @@ export function updateRefreshers( maxId, revisions, constraintViolationCount, + constraintViolationCountOnRevert, builds, destroys, } = change; @@ -1920,6 +1990,7 @@ export function updateRefreshers( maxId: maxId as number, revisions, constraintViolationCount, + constraintViolationCountOnRevert, builds, destroys, refreshers, @@ -2061,7 +2132,11 @@ export function rebaseRevisionMetadataFromInfo( } function isEmptyNodeChangeset(change: NodeChangeset): boolean { - return change.fieldChanges === undefined && change.nodeExistsConstraint === undefined; + return ( + change.fieldChanges === undefined && + change.nodeExistsConstraint === undefined && + change.nodeExistsConstraintOnRevert === undefined + ); } export function getFieldKind( @@ -2504,6 +2579,7 @@ function makeModularChangeset( maxId: number; revisions?: readonly RevisionInfo[]; constraintViolationCount?: number; + constraintViolationCountOnRevert?: number; builds?: ChangeAtomIdBTree; destroys?: ChangeAtomIdBTree; refreshers?: ChangeAtomIdBTree; @@ -2528,6 +2604,12 @@ function makeModularChangeset( if (props.constraintViolationCount !== undefined && props.constraintViolationCount > 0) { changeset.constraintViolationCount = props.constraintViolationCount; } + if ( + props.constraintViolationCountOnRevert !== undefined && + props.constraintViolationCountOnRevert > 0 + ) { + changeset.constraintViolationCountOnRevert = props.constraintViolationCountOnRevert; + } if (props.builds !== undefined && props.builds.size > 0) { changeset.builds = props.builds; } @@ -2702,6 +2784,27 @@ export class ModularEditBuilder extends EditBuilder { ), ); } + + public addNodeExistsConstraintOnRevert(path: UpPath, revision: RevisionTag): void { + const nodeChange: NodeChangeset = { + nodeExistsConstraintOnRevert: { violated: false }, + }; + + this.applyChange( + tagChange( + buildModularChangesetFromNode({ + path, + nodeChange, + nodeChanges: newTupleBTree(), + nodeToParent: newTupleBTree(), + crossFieldKeys: newCrossFieldKeyTable(), + idAllocator: this.idAllocator, + revision, + }), + revision, + ), + ); + } } function buildModularChangesetFromField(props: { diff --git a/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeTypes.ts b/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeTypes.ts index 474aa32a0999..10c06624d236 100644 --- a/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeTypes.ts +++ b/packages/dds/tree/src/feature-libraries/modular-schema/modularChangeTypes.ts @@ -56,7 +56,16 @@ export interface ModularChangeset extends HasFieldChanges { */ readonly nodeAliases: ChangeAtomIdBTree; readonly crossFieldKeys: CrossFieldKeyTable; + /** + * The number of constraint violations that apply to the input context of the changeset, i.e., before this change is applied. + * If this count is greater than 0, it will prevent the changeset from being applied. + */ readonly constraintViolationCount?: number; + /** + * The number of constraint violations that apply to the revert of the changeset. If this count is greater than 0, it will + * prevent the changeset from being reverted or undone. + */ + readonly constraintViolationCountOnRevert?: number; readonly builds?: ChangeAtomIdBTree; readonly destroys?: ChangeAtomIdBTree; readonly refreshers?: ChangeAtomIdBTree; @@ -97,7 +106,10 @@ export interface NodeExistsConstraint { * Changeset for a subtree rooted at a specific node. */ export interface NodeChangeset extends HasFieldChanges { + /** Keeps track of whether node exists constraint has been violated by this change */ nodeExistsConstraint?: NodeExistsConstraint; + /** Keeps track of whether node exists constraint will be violated when this change is reverted */ + nodeExistsConstraintOnRevert?: NodeExistsConstraint; } export type NodeId = ChangeAtomId; diff --git a/packages/dds/tree/src/feature-libraries/optional-field/optionalField.ts b/packages/dds/tree/src/feature-libraries/optional-field/optionalField.ts index 0f405a3bcb53..408cbc6320c9 100644 --- a/packages/dds/tree/src/feature-libraries/optional-field/optionalField.ts +++ b/packages/dds/tree/src/feature-libraries/optional-field/optionalField.ts @@ -39,6 +39,7 @@ import { type NodeId, type RelevantRemovedRootsFromChild, type ToDelta, + type NestedChangesIndices, } from "../modular-schema/index.js"; import type { @@ -536,6 +537,16 @@ function areEqualRegisterIds(id1: RegisterId, id2: RegisterId): boolean { return id1 === "self" || id2 === "self" ? id1 === id2 : areEqualChangeAtomIds(id1, id2); } +function areEqualRegisterIdsOpt( + id1: RegisterId | undefined, + id2: RegisterId | undefined, +): boolean { + if (id1 === undefined || id2 === undefined) { + return id1 === id2; + } + return areEqualRegisterIds(id1, id2); +} + function getBidirectionalMaps(moves: OptionalChangeset["moves"]): { srcToDst: ChangeAtomIdMap; dstToSrc: ChangeAtomIdMap; @@ -720,11 +731,29 @@ export const optionalChangeHandler: FieldChangeHandler< getCrossFieldKeys: (_change) => [], }; -function getNestedChanges(change: OptionalChangeset): [NodeId, number | undefined][] { - return change.childChanges.map(([register, nodeId]) => [ - nodeId, - register === "self" ? 0 : undefined, - ]); +function getNestedChanges(change: OptionalChangeset): NestedChangesIndices { + // True iff the content of the field changes in some way + const isFieldContentChanged = + change.valueReplace !== undefined && change.valueReplace.src !== "self"; + + // The node that is moved into the field (if any). + const nodeMovedIntoField = change.valueReplace?.src; + + return change.childChanges.map(([register, nodeId]) => { + // The node is removed in the input context iif register is not self. + const inputIndex = register === "self" ? 0 : undefined; + const outputIndex = + register === "self" + ? // If the node starts out as not-removed, it is removed in the output context iff the field content is changed + isFieldContentChanged + ? undefined + : 0 + : // If the node starts out as removed, then it remains removed in the output context iff it is not the node that is moved into the field + !areEqualRegisterIdsOpt(register, nodeMovedIntoField) + ? undefined + : 0; + return [nodeId, inputIndex, outputIndex]; + }); } function* relevantRemovedRoots( diff --git a/packages/dds/tree/src/feature-libraries/sequence-field/utils.ts b/packages/dds/tree/src/feature-libraries/sequence-field/utils.ts index cfcf7924373e..ea73bc837fd4 100644 --- a/packages/dds/tree/src/feature-libraries/sequence-field/utils.ts +++ b/packages/dds/tree/src/feature-libraries/sequence-field/utils.ts @@ -19,6 +19,7 @@ import { CrossFieldTarget, type NodeId, type CrossFieldKeyRange, + type NestedChangesIndices, } from "../modular-schema/index.js"; import type { @@ -61,15 +62,25 @@ export function createEmpty(): Changeset { return []; } -export function getNestedChanges(change: Changeset): [NodeId, number | undefined][] { - const output: [NodeId, number | undefined][] = []; - let index = 0; - for (const { changes, cellId, count } of change) { +export function getNestedChanges(change: Changeset): NestedChangesIndices { + const output: NestedChangesIndices = []; + let inputIndex = 0; + let outputIndex = 0; + for (const mark of change) { + const { changes, count } = mark; if (changes !== undefined) { - output.push([changes, cellId === undefined ? index : undefined]); + output.push([ + changes, + !areInputCellsEmpty(mark) ? inputIndex : undefined /* inputIndex */, + !areOutputCellsEmpty(mark) ? outputIndex : undefined /* outputIndex */, + ]); + } + if (!areInputCellsEmpty(mark)) { + inputIndex += count; } - if (cellId === undefined) { - index += count; + + if (!areOutputCellsEmpty(mark)) { + outputIndex += count; } } return output; diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index 1dd3c45958b1..d3272a773ee7 100644 --- a/packages/dds/tree/src/shared-tree/treeCheckout.ts +++ b/packages/dds/tree/src/shared-tree/treeCheckout.ts @@ -1022,6 +1022,9 @@ class EditLock { addNodeExistsConstraint(path) { editor.addNodeExistsConstraint(path); }, + addNodeExistsConstraintOnRevert(path) { + editor.addNodeExistsConstraintOnRevert(path); + }, }; } diff --git a/packages/dds/tree/src/simple-tree/api/simpleSchemaToJsonSchema.ts b/packages/dds/tree/src/simple-tree/api/simpleSchemaToJsonSchema.ts index 5e2ebb69d576..6e1334c13a1b 100644 --- a/packages/dds/tree/src/simple-tree/api/simpleSchemaToJsonSchema.ts +++ b/packages/dds/tree/src/simple-tree/api/simpleSchemaToJsonSchema.ts @@ -6,7 +6,7 @@ import { unreachableCase } from "@fluidframework/core-utils/internal"; import { UsageError } from "@fluidframework/telemetry-utils/internal"; import { ValueSchema } from "../../core/index.js"; -import { getOrCreate, hasSingle, type Mutable } from "../../util/index.js"; +import { copyProperty, getOrCreate, hasSingle, type Mutable } from "../../util/index.js"; import type { JsonArrayNodeSchema, JsonFieldSchema, @@ -149,11 +149,7 @@ function convertObjectNodeSchema(schema: SimpleObjectNodeSchema): JsonObjectNode anyOf: allowedTypes, }; - // Don't include "description" property at all if it's not present in the input. - if (value.metadata?.description !== undefined) { - output.description = value.metadata.description; - } - + copyProperty(value.metadata, "description", output); properties[key] = output; if (value.kind === FieldKind.Required) { diff --git a/packages/dds/tree/src/simple-tree/api/viewSchemaToSimpleSchema.ts b/packages/dds/tree/src/simple-tree/api/viewSchemaToSimpleSchema.ts index f356e3d530f7..b46bcc8a70b9 100644 --- a/packages/dds/tree/src/simple-tree/api/viewSchemaToSimpleSchema.ts +++ b/packages/dds/tree/src/simple-tree/api/viewSchemaToSimpleSchema.ts @@ -20,7 +20,7 @@ import type { SimpleTreeSchema, } from "./simpleSchema.js"; import type { ValueSchema } from "../../core/index.js"; -import { getOrCreate, type Mutable } from "../../util/index.js"; +import { copyProperty, getOrCreate, type Mutable } from "../../util/index.js"; import { isObjectNodeSchema, type ObjectNodeSchema } from "../objectNodeTypes.js"; import { NodeKind, type TreeNodeSchema } from "../core/index.js"; @@ -41,11 +41,7 @@ export function toSimpleTreeSchema(schema: ImplicitFieldSchema): SimpleTreeSchem definitions, }; - // Include the "description" property only if it's present on the input. - if (normalizedSchema.metadata !== undefined) { - output.metadata = normalizedSchema.metadata; - } - + copyProperty(normalizedSchema, "metadata", output); return output; } @@ -140,10 +136,7 @@ function fieldSchemaToSimpleSchema(schema: FieldSchema): SimpleFieldSchema { allowedTypes, }; - // Don't include "metadata" property at all if it's not present. - if (schema.metadata !== undefined) { - result.metadata = schema.metadata; - } + copyProperty(schema, "metadata", result); // eslint-disable-next-line @typescript-eslint/no-explicit-any (schema as any)[simpleFieldSchemaCacheSymbol] = result; 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 dafb71262fa7..a34ae17c3dff 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 @@ -148,7 +148,7 @@ const singleNodeHandler: FieldChangeHandler = { // We create changesets by composing an empty single node field with a change to the child. // We don't want the temporarily empty single node field to be pruned away leaving us with a generic field instead. isEmpty: (change) => false, - getNestedChanges: (change) => (change === undefined ? [] : [[change, 0]]), + getNestedChanges: (change) => (change === undefined ? [] : [[change, 0, 0]]), createEmpty: () => undefined, getCrossFieldKeys: (_change) => [], }; diff --git a/packages/dds/tree/src/test/feature-libraries/optional-field/optionalField.spec.ts b/packages/dds/tree/src/test/feature-libraries/optional-field/optionalField.spec.ts index acd374b0454c..7c573f81fdc3 100644 --- a/packages/dds/tree/src/test/feature-libraries/optional-field/optionalField.spec.ts +++ b/packages/dds/tree/src/test/feature-libraries/optional-field/optionalField.spec.ts @@ -43,6 +43,8 @@ import { testRebaserAxioms } from "./optionalChangeRebaser.test.js"; import { testCodecs } from "./optionalFieldChangeCodecs.test.js"; import { deepFreeze } from "@fluidframework/test-runtime-utils/internal"; import { testReplaceRevisions } from "./replaceRevisions.test.js"; +// eslint-disable-next-line import/no-internal-modules +import type { NestedChangesIndices } from "../../../feature-libraries/modular-schema/fieldChangeHandler.js"; /** * A change to a child encoding as a simple placeholder string. @@ -946,7 +948,27 @@ describe("optionalField", () => { it("includes changes to the node in the field", () => { const change: OptionalChangeset = Change.child(nodeId1); const actual = optionalChangeHandler.getNestedChanges(change); - assert.deepEqual(actual, [[nodeId1, 0]]); + const expected: NestedChangesIndices = [[nodeId1, 0, 0]]; + assert.deepEqual(actual, expected); + }); + it("includes changes to a node being removed from the field", () => { + const change: OptionalChangeset = Change.atOnce( + Change.child(nodeId1), + Change.clear("self", brand(41)), + ); + const actual = optionalChangeHandler.getNestedChanges(change); + const expected: NestedChangesIndices = [[nodeId1, 0, undefined]]; + assert.deepEqual(actual, expected); + }); + it("includes changes to a node being moved into from the field", () => { + const change: OptionalChangeset = Change.atOnce( + Change.reserve("self", brand(41)), + Change.childAt(brand(42), nodeId1), + Change.move(brand(42), "self"), + ); + const actual = optionalChangeHandler.getNestedChanges(change); + const expected: NestedChangesIndices = [[nodeId1, undefined, 0]]; + assert.deepEqual(actual, expected); }); it("includes changes to removed nodes", () => { const change: OptionalChangeset = Change.atOnce( @@ -954,10 +976,11 @@ describe("optionalField", () => { Change.childAt(brand(42), nodeId2), ); const actual = optionalChangeHandler.getNestedChanges(change); - assert.deepEqual(actual, [ - [nodeId1, undefined], - [nodeId2, undefined], - ]); + const expected: NestedChangesIndices = [ + [nodeId1, undefined, undefined], + [nodeId2, undefined, undefined], + ]; + assert.deepEqual(actual, expected); }); }); }); diff --git a/packages/dds/tree/src/test/feature-libraries/sequence-field/sequenceGetNestedChanges.test.ts b/packages/dds/tree/src/test/feature-libraries/sequence-field/sequenceGetNestedChanges.test.ts index d42704c9583a..ca7eec005c9d 100644 --- a/packages/dds/tree/src/test/feature-libraries/sequence-field/sequenceGetNestedChanges.test.ts +++ b/packages/dds/tree/src/test/feature-libraries/sequence-field/sequenceGetNestedChanges.test.ts @@ -12,6 +12,8 @@ import { brand } from "../../../util/index.js"; import { MarkMaker as Mark } from "./testEdits.js"; import type { RevisionTag } from "../../../core/index.js"; import { mintRevisionTag } from "../../utils.js"; +// eslint-disable-next-line import/no-internal-modules +import type { NestedChangesIndices } from "../../../feature-libraries/modular-schema/fieldChangeHandler.js"; const tag1: RevisionTag = mintRevisionTag(); const nodeId1: NodeId = { localId: brand(1) }; @@ -25,12 +27,17 @@ export function testGetNestedChanges() { assert.deepEqual(actual, []); }); it("includes changes to nodes in the field", () => { - const change = [Mark.modify(nodeId1), { count: 42 }, Mark.modify(nodeId2)]; + const change = [ + Mark.remove(1, brand(42), { changes: nodeId1 }), + { count: 42 }, + Mark.modify(nodeId2), + ]; const actual = sequenceFieldChangeHandler.getNestedChanges(change); - assert.deepEqual(actual, [ - [nodeId1, 0], - [nodeId2, 43], - ]); + const expected: NestedChangesIndices = [ + [nodeId1, 0, undefined], + [nodeId2, 43, 42], + ]; + assert.deepEqual(actual, expected); }); it("includes changes to removed nodes", () => { const change = [ @@ -38,10 +45,11 @@ export function testGetNestedChanges() { Mark.modify(nodeId2, { revision: tag1, localId: brand(43) }), ]; const actual = sequenceFieldChangeHandler.getNestedChanges(change); - assert.deepEqual(actual, [ - [nodeId1, undefined], - [nodeId2, undefined], - ]); + const expected: NestedChangesIndices = [ + [nodeId1, undefined, 0], + [nodeId2, undefined, undefined], + ]; + assert.deepEqual(actual, expected); }); }); } diff --git a/packages/dds/tree/src/test/shared-tree/editing.spec.ts b/packages/dds/tree/src/test/shared-tree/editing.spec.ts index 0d9395585cea..f7c80c218cfd 100644 --- a/packages/dds/tree/src/test/shared-tree/editing.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/editing.spec.ts @@ -24,7 +24,7 @@ import { } from "../../core/index.js"; import { cursorForJsonableTreeNode } from "../../feature-libraries/index.js"; import type { ITreeCheckout, TreeStoredContent } from "../../shared-tree/index.js"; -import { type JsonCompatible, brand, makeArray } from "../../util/index.js"; +import { type JsonCompatible, brand, fail, makeArray } from "../../util/index.js"; import { checkoutWithContent, createTestUndoRedoStacks, @@ -3146,6 +3146,119 @@ describe("Editing", () => { }); }); + describe("Inverse preconditions", () => { + it("inverse constraint not violated by interim change", () => { + const tree = makeTreeFromJson({ foo: "A" }); + const stack = createTestUndoRedoStacks(tree.events); + + // Make transaction on a branch that does the following: + // 1. Changes value of "foo" to "B". + // 2. Adds inverse constraint on existence of node "B" on field "foo". + tree.transaction.start(); + tree.editor + .valueField({ parent: rootNode, field: brand("foo") }) + .set(singleJsonCursor("B")); + tree.editor.addNodeExistsConstraintOnRevert({ + parent: rootNode, + parentField: brand("foo"), + parentIndex: 0, + }); + tree.transaction.commit(); + expectJsonTree(tree, [{ foo: "B" }]); + + const changedFooAtoB = stack.undoStack[0] ?? fail("Missing undo"); + + // This change should not violate the constraint in the inverse because it is changing + // a different node on filed "bar". + tree.editor + .optionalField({ parent: rootNode, field: brand("bar") }) + .set(singleJsonCursor("C"), true); + + // This revert should apply since its constraint has not been violated + changedFooAtoB.revert(); + expectJsonTree(tree, [{ foo: "A", bar: "C" }]); + + stack.unsubscribe(); + }); + + it("inverse constraint violated by a change between the original and the revert", () => { + const tree = makeTreeFromJson({ foo: "A" }); + const stack = createTestUndoRedoStacks(tree.events); + + // Make transaction on a branch that does the following: + // 1. Changes value of "foo" to "B". + // 2. Adds inverse constraint on existence of node "B" on field "foo". + tree.transaction.start(); + tree.editor + .valueField({ parent: rootNode, field: brand("foo") }) + .set(singleJsonCursor("B")); + tree.editor.addNodeExistsConstraintOnRevert({ + parent: rootNode, + parentField: brand("foo"), + parentIndex: 0, + }); + tree.transaction.commit(); + expectJsonTree(tree, [{ foo: "B" }]); + + const changedFooAtoB = stack.undoStack[0] ?? fail("Missing undo"); + + // This change should violate the inverse constraint because it changes the + // node "B" to "C" on field "foo". + tree.editor + .valueField({ parent: rootNode, field: brand("foo") }) + .set(singleJsonCursor("C")); + + // This revert should do nothing since its constraint has been violated. + changedFooAtoB.revert(); + expectJsonTree(tree, [{ foo: "C" }]); + + stack.unsubscribe(); + }); + + it("inverse constraint violated while rebasing the original change", () => { + const tree = makeTreeFromJson({ foo: "A", bar: "old" }); + const branch = tree.branch(); + + // Make transaction on a branch that does the following: + // 1. Changes value of "bar" to "new". + // 2. Adds inverse constraint on existence of node "A" on field "foo". + branch.transaction.start(); + branch.editor + .valueField({ parent: rootNode, field: brand("bar") }) + .set(singleJsonCursor("new")); + branch.editor.addNodeExistsConstraintOnRevert({ + parent: rootNode, + parentField: brand("foo"), + parentIndex: 0, + }); + branch.transaction.commit(); + expectJsonTree(branch, [{ foo: "A", bar: "new" }]); + + // This change replaces the node "A" on field "foo" to "C" which would violate + // the undo constraint on the branch transaction when the branch is rebased into tree. + tree.editor + .valueField({ parent: rootNode, field: brand("foo") }) + .set(singleJsonCursor("C")); + branch.rebaseOnto(tree); + + const stack = createTestUndoRedoStacks(tree.events); + // This is done after the rebase so that the rebased transaction is at the tip of the branch + // and doesn't go through any more rebases. This validates the scenario where an inverse is + // directly applied without any rebases. + tree.merge(branch); + const changedBarOldToNew = stack.undoStack[0] ?? fail("Missing undo"); + + expectJsonTree(tree, [{ foo: "C", bar: "new" }]); + + // The inverse constraint will be violated and so the revert will not be applied, leaving the value + // of "bar" at "new" + changedBarOldToNew.revert(); + expectJsonTree(tree, [{ foo: "C", bar: "new" }]); + + stack.unsubscribe(); + }); + }); + it("Rebase over conflicted change", () => { const tree1 = makeTreeFromJsonSequence(["A", "B"]); const tree2 = tree1.branch(); diff --git a/packages/dds/tree/src/test/util/utils.spec.ts b/packages/dds/tree/src/test/util/utils.spec.ts index ebfb79081817..f2f8f1311152 100644 --- a/packages/dds/tree/src/test/util/utils.spec.ts +++ b/packages/dds/tree/src/test/util/utils.spec.ts @@ -7,6 +7,7 @@ import { strict as assert } from "node:assert"; import { capitalize, + copyProperty, defineLazyCachedProperty, mapIterable, transformObjectMap, @@ -70,4 +71,29 @@ describe("Utils", () => { assert.equal(objWithProperty.prop, 3); assert.equal(count, 1); }); + + describe("copyProperty", () => { + it("copies a known property", () => { + const source = { a: 3 }; + const destination = {}; + copyProperty(source, "a", destination); + // `destination` should now be typed to have a property "a" + assert.equal(destination.a, 3); + }); + + it("does nothing if the property is not present", () => { + const source = {}; + const destination = {}; + copyProperty(undefined, "a", destination); + copyProperty(source, "a", destination); + assert.equal(Reflect.has(destination, "a"), false); + }); + + it("does nothing if the property is present but undefined", () => { + const source = { a: undefined }; + const destination = {}; + copyProperty(source, "a", destination); + assert.equal(Reflect.has(destination, "a"), false); + }); + }); }); diff --git a/packages/dds/tree/src/util/index.ts b/packages/dds/tree/src/util/index.ts index efa0b6960a90..64ac5046f538 100644 --- a/packages/dds/tree/src/util/index.ts +++ b/packages/dds/tree/src/util/index.ts @@ -94,6 +94,7 @@ export { hasSome, hasSingle, defineLazyCachedProperty, + copyPropertyIfDefined as copyProperty, } from "./utils.js"; export { ReferenceCountedBase, type ReferenceCounted } from "./referenceCounting.js"; diff --git a/packages/dds/tree/src/util/utils.ts b/packages/dds/tree/src/util/utils.ts index 3407441c83d3..ee2a857e585d 100644 --- a/packages/dds/tree/src/util/utils.ts +++ b/packages/dds/tree/src/util/utils.ts @@ -563,3 +563,49 @@ export function defineLazyCachedProperty< }); return obj as typeof obj & { [P in K]: V }; } + +/** + * Copies a given property from one object to another if and only if the property is defined. + * @param source - The object to copy the property from. + * If `source` is undefined or does not have the property defined, then this function will do nothing. + * @param property - The property to copy. + * @param destination - The object to copy the property to. + * @remarks This function is useful for copying properties from one object to another while minimizing the presence of `undefined` values. + * If `property` is not present on `source` - or if `property` is present, but has a value of `undefined` - then this function will not modify `destination`. + * This is different from doing `destination.foo = source.foo`, which would define a `"foo"` property on `destination` with the value `undefined`. + * + * If the type of `source` is known to have `property`, then this function asserts that the type of `destination` has `property` as well after the call. + * + * This function first reads the property value (if present) from `source` and then sets it on `destination`, as opposed to e.g. directly copying the property descriptor. + * @privateRemarks The first overload of this function allows auto-complete to suggest property names from `source`, but by having the second overload we still allow for arbitrary property names. + */ +export function copyPropertyIfDefined( + source: S | undefined, + property: K, + destination: D, +): asserts destination is K extends keyof S ? D & { [P in K]: S[K] } : D; +export function copyPropertyIfDefined< + S extends object, + K extends string | number | symbol, + D extends object, +>( + source: S | undefined, + property: K, + destination: D, +): asserts destination is K extends keyof S ? D & { [P in K]: S[K] } : D; +export function copyPropertyIfDefined< + S extends object, + K extends string | number | symbol, + D extends object, +>( + source: S | undefined, + property: K, + destination: D, +): asserts destination is K extends keyof S ? D & { [P in K]: S[K] } : D { + if (source !== undefined) { + const value = (source as { [P in K]?: unknown })[property]; + if (value !== undefined) { + (destination as { [P in K]: unknown })[property] = value; + } + } +} diff --git a/packages/test/test-end-to-end-tests/src/test/stashedOps.spec.ts b/packages/test/test-end-to-end-tests/src/test/stashedOps.spec.ts index bbbd0f522538..2e8ef17dc532 100644 --- a/packages/test/test-end-to-end-tests/src/test/stashedOps.spec.ts +++ b/packages/test/test-end-to-end-tests/src/test/stashedOps.spec.ts @@ -249,8 +249,9 @@ const waitForSummary = async ( container, testConfig, ); - await summarizeNow(summarizer); + const { summaryVersion } = await summarizeNow(summarizer); summarizingContainer.close(); + return summaryVersion; }; // Introduced in 0.37 // REVIEW: enable compat testing @@ -2025,14 +2026,21 @@ describeCompat("stashed ops", "NoCompat", (getTestObjectProvider, apis) => { }); it("applies stashed ops with no saved ops", async function () { - // TODO: This test is consistently failing when ran against AFR. See ADO:7968 - if (provider.driver.type === "routerlicious" && provider.driver.endpointName === "frs") { + // Waiting for summary takes many seconds and can timeout these tests on real services. + // That coverage isn't a marginal gain over local server testing, so only test against local server. + if (provider.driver.type !== "local") { this.skip(); } - await waitForSummary(provider, container1, testContainerConfig); - // avoid our join op being saved - const headers: IRequestHeader = { [LoaderHeader.loadMode]: { deltaConnection: "none" } }; + // We want to test the case where we stash ops based on the sequence number of the snapshot we load from + // So step 1 is to complete a summary so we can load from it. + const summaryVersion = await waitForSummary(provider, container1, testContainerConfig); + + // avoid our join op being saved (so saved ops is empty and the map op below has the right ref seq) + const headers: IRequestHeader = { + [LoaderHeader.loadMode]: { deltaConnection: "none" }, + [LoaderHeader.version]: summaryVersion, + }; const container: IContainerExperimental = await loader.resolve({ url, headers }); const dataStore = (await container.getEntryPoint()) as ITestFluidObject; const map = await dataStore.getSharedObject(mapId); @@ -2041,8 +2049,13 @@ describeCompat("stashed ops", "NoCompat", (getTestObjectProvider, apis) => { const stashBlob = await container.closeAndGetPendingLocalState?.(); assert(stashBlob); const pendingState = JSON.parse(stashBlob); + // make sure the container loaded from summary and we have no saved ops - assert.strictEqual(pendingState.savedOps.length, 0); + assert.strictEqual(pendingState.savedOps.length, 0, "Expected no saved ops"); + assert( + pendingState.pendingRuntimeState.pending.pendingStates[0].referenceSequenceNumber > 0, + "Expected the pending state to have some ops with non-zero ref seq (should match the snapshot sequence number)", + ); // load container with pending ops, which should resend the op not sent by previous container const container2 = await loader.resolve({ url }, stashBlob); diff --git a/tools/api-markdown-documenter/CHANGELOG.md b/tools/api-markdown-documenter/CHANGELOG.md index adcb1946b04c..1c926086d39d 100644 --- a/tools/api-markdown-documenter/CHANGELOG.md +++ b/tools/api-markdown-documenter/CHANGELOG.md @@ -59,6 +59,10 @@ await MarkdownRenderer.renderApiModel({ - `RenderHtmlConfig` -> `RenderHtmlConfiguration` - `ToHtmlConfig` -> `ToHtmlConfiguration` +#### Utility function renames + +- `ApiItemUtilities.getQualifiedApiItemName` -> `ApiItemUtilities.getFileSafeNameForApiItem` + #### Configuration properties made `readonly` - `ApiItemTransformations` diff --git a/tools/api-markdown-documenter/api-report/api-markdown-documenter.alpha.api.md b/tools/api-markdown-documenter/api-report/api-markdown-documenter.alpha.api.md index 239db9bced06..fcfd3d56e58d 100644 --- a/tools/api-markdown-documenter/api-report/api-markdown-documenter.alpha.api.md +++ b/tools/api-markdown-documenter/api-report/api-markdown-documenter.alpha.api.md @@ -116,9 +116,9 @@ declare namespace ApiItemUtilities { getDefaultValueBlock, getDeprecatedBlock, getExampleBlocks, + getFileSafeNameForApiItem, getModifiers, getModifierTags, - getQualifiedApiItemName, getReleaseTag, getReturnsBlock, getSeeBlocks, @@ -425,6 +425,9 @@ function getDeprecatedBlock(apiItem: ApiItem): DocSection | undefined; // @public function getExampleBlocks(apiItem: ApiItem): readonly DocSection[] | undefined; +// @public +function getFileSafeNameForApiItem(apiItem: ApiItem): string; + // @public function getHeadingForApiItem(apiItem: ApiItem, config: ApiItemTransformationConfiguration, headingLevel?: number): Heading; @@ -437,9 +440,6 @@ function getModifiers(apiItem: ApiItem, modifiersToOmit?: ApiModifier[]): ApiMod // @public function getModifierTags(apiItem: ApiItem): ReadonlySet; -// @public -function getQualifiedApiItemName(apiItem: ApiItem): string; - // @public function getReleaseTag(apiItem: ApiItem): ReleaseTag | undefined; diff --git a/tools/api-markdown-documenter/api-report/api-markdown-documenter.beta.api.md b/tools/api-markdown-documenter/api-report/api-markdown-documenter.beta.api.md index da53e8b61adb..f33b0167f424 100644 --- a/tools/api-markdown-documenter/api-report/api-markdown-documenter.beta.api.md +++ b/tools/api-markdown-documenter/api-report/api-markdown-documenter.beta.api.md @@ -116,9 +116,9 @@ declare namespace ApiItemUtilities { getDefaultValueBlock, getDeprecatedBlock, getExampleBlocks, + getFileSafeNameForApiItem, getModifiers, getModifierTags, - getQualifiedApiItemName, getReleaseTag, getReturnsBlock, getSeeBlocks, @@ -425,6 +425,9 @@ function getDeprecatedBlock(apiItem: ApiItem): DocSection | undefined; // @public function getExampleBlocks(apiItem: ApiItem): readonly DocSection[] | undefined; +// @public +function getFileSafeNameForApiItem(apiItem: ApiItem): string; + // @public function getHeadingForApiItem(apiItem: ApiItem, config: ApiItemTransformationConfiguration, headingLevel?: number): Heading; @@ -437,9 +440,6 @@ function getModifiers(apiItem: ApiItem, modifiersToOmit?: ApiModifier[]): ApiMod // @public function getModifierTags(apiItem: ApiItem): ReadonlySet; -// @public -function getQualifiedApiItemName(apiItem: ApiItem): string; - // @public function getReleaseTag(apiItem: ApiItem): ReleaseTag | undefined; diff --git a/tools/api-markdown-documenter/api-report/api-markdown-documenter.public.api.md b/tools/api-markdown-documenter/api-report/api-markdown-documenter.public.api.md index 0db41570440f..adbadf58a979 100644 --- a/tools/api-markdown-documenter/api-report/api-markdown-documenter.public.api.md +++ b/tools/api-markdown-documenter/api-report/api-markdown-documenter.public.api.md @@ -116,9 +116,9 @@ declare namespace ApiItemUtilities { getDefaultValueBlock, getDeprecatedBlock, getExampleBlocks, + getFileSafeNameForApiItem, getModifiers, getModifierTags, - getQualifiedApiItemName, getReleaseTag, getReturnsBlock, getSeeBlocks, @@ -425,6 +425,9 @@ function getDeprecatedBlock(apiItem: ApiItem): DocSection | undefined; // @public function getExampleBlocks(apiItem: ApiItem): readonly DocSection[] | undefined; +// @public +function getFileSafeNameForApiItem(apiItem: ApiItem): string; + // @public function getHeadingForApiItem(apiItem: ApiItem, config: ApiItemTransformationConfiguration, headingLevel?: number): Heading; @@ -437,9 +440,6 @@ function getModifiers(apiItem: ApiItem, modifiersToOmit?: ApiModifier[]): ApiMod // @public function getModifierTags(apiItem: ApiItem): ReadonlySet; -// @public -function getQualifiedApiItemName(apiItem: ApiItem): string; - // @public function getReleaseTag(apiItem: ApiItem): ReleaseTag | undefined; diff --git a/tools/api-markdown-documenter/src/ApiItemUtilitiesModule.ts b/tools/api-markdown-documenter/src/ApiItemUtilitiesModule.ts index 29311c1a81b0..60e397638707 100644 --- a/tools/api-markdown-documenter/src/ApiItemUtilitiesModule.ts +++ b/tools/api-markdown-documenter/src/ApiItemUtilitiesModule.ts @@ -20,9 +20,9 @@ export { getDefaultValueBlock, getDeprecatedBlock, getExampleBlocks, + getFileSafeNameForApiItem, getModifiers, getModifierTags, - getQualifiedApiItemName, getReleaseTag, getReturnsBlock, getSeeBlocks, diff --git a/tools/api-markdown-documenter/src/api-item-transforms/ApiItemTransformUtilities.ts b/tools/api-markdown-documenter/src/api-item-transforms/ApiItemTransformUtilities.ts index 17f0a1aef4da..d9a68844b5fa 100644 --- a/tools/api-markdown-documenter/src/api-item-transforms/ApiItemTransformUtilities.ts +++ b/tools/api-markdown-documenter/src/api-item-transforms/ApiItemTransformUtilities.ts @@ -12,7 +12,7 @@ import type { Link } from "../Link.js"; import { getApiItemKind, getFilteredParent, - getQualifiedApiItemName, + getFileSafeNameForApiItem, getReleaseTag, getValueOrDerived, type ValidApiItemKind, @@ -246,7 +246,7 @@ function createQualifiedDocumentNameForApiItem( hierarchyConfig: HierarchyConfiguration, ): string { const apiItemKind = getApiItemKind(apiItem); - let documentName = getQualifiedApiItemName(apiItem); + let documentName = getFileSafeNameForApiItem(apiItem); if (apiItemKind !== ApiItemKind.Package) { // If the item is not a package, append its "kind" to the document name to ensure uniqueness. // Packages strictly live at the root of the document hierarchy (beneath the model), and only @@ -265,7 +265,7 @@ function createQualifiedDocumentNameForApiItem( currentItem.kind !== "Model" && hierarchyConfig[getApiItemKind(currentItem)].kind !== HierarchyKind.Folder ) { - documentName = `${getQualifiedApiItemName(currentItem)}-${documentName}`; + documentName = `${getFileSafeNameForApiItem(currentItem)}-${documentName}`; currentItem = getFilteredParent(currentItem); } @@ -331,7 +331,7 @@ function getHeadingIdForApiItem( // Generate ID information for everything back to that point let hierarchyItem = apiItem; while (!doesItemRequireOwnDocument(hierarchyItem, config.hierarchy)) { - const qualifiedName = getQualifiedApiItemName(hierarchyItem); + const qualifiedName = getFileSafeNameForApiItem(hierarchyItem); // Since we're walking up the tree, we'll build the string from the end for simplicity baseName = baseName === undefined ? qualifiedName : `${qualifiedName}-${baseName}`; diff --git a/tools/api-markdown-documenter/src/api-item-transforms/configuration/DocumentationSuite.ts b/tools/api-markdown-documenter/src/api-item-transforms/configuration/DocumentationSuite.ts index a52ec48643e6..b12b3e8f5f54 100644 --- a/tools/api-markdown-documenter/src/api-item-transforms/configuration/DocumentationSuite.ts +++ b/tools/api-markdown-documenter/src/api-item-transforms/configuration/DocumentationSuite.ts @@ -12,11 +12,11 @@ import { } from "@microsoft/api-extractor-model"; import { + getApiItemKind, getConciseSignature, + getReleaseTag, getSingleLineExcerptText, isDeprecated, - getReleaseTag, - getApiItemKind, } from "../../utilities/index.js"; import { diff --git a/tools/api-markdown-documenter/src/api-item-transforms/helpers/Helpers.ts b/tools/api-markdown-documenter/src/api-item-transforms/helpers/Helpers.ts index d043863c919f..d33a1c23002e 100644 --- a/tools/api-markdown-documenter/src/api-item-transforms/helpers/Helpers.ts +++ b/tools/api-markdown-documenter/src/api-item-transforms/helpers/Helpers.ts @@ -50,7 +50,7 @@ import { import { type ApiFunctionLike, injectSeparator, - getQualifiedApiItemName, + getFileSafeNameForApiItem, getSeeBlocks, getThrowsBlocks, getDeprecatedBlock, @@ -99,7 +99,7 @@ export function createSignatureSection( return wrapInSection(contents, { title: "Signature", - id: `${getQualifiedApiItemName(apiItem)}-signature`, + id: `${getFileSafeNameForApiItem(apiItem)}-signature`, }); } } @@ -136,7 +136,7 @@ export function createSeeAlsoSection( return wrapInSection(contents, { title: "See Also", - id: `${getQualifiedApiItemName(apiItem)}-see-also`, + id: `${getFileSafeNameForApiItem(apiItem)}-see-also`, }); } @@ -496,7 +496,7 @@ export function createRemarksSection( tsdocNodeTransformOptions, ), ], - { title: "Remarks", id: `${getQualifiedApiItemName(apiItem)}-remarks` }, + { title: "Remarks", id: `${getFileSafeNameForApiItem(apiItem)}-remarks` }, ); } @@ -532,7 +532,7 @@ export function createThrowsSection( return wrapInSection(paragraphs, { title: headingText, - id: `${getQualifiedApiItemName(apiItem)}-throws`, + id: `${getFileSafeNameForApiItem(apiItem)}-throws`, }); } @@ -616,7 +616,7 @@ export function createExamplesSection( return wrapInSection(exampleSections, { title: headingText, - id: `${getQualifiedApiItemName(apiItem)}-examples`, + id: `${getFileSafeNameForApiItem(apiItem)}-examples`, }); } @@ -734,7 +734,7 @@ function createExampleSection( exampleParagraph = stripTitleFromParagraph(exampleParagraph, exampleTitle, logger); } - const headingId = `${getQualifiedApiItemName(example.apiItem)}-example${ + const headingId = `${getFileSafeNameForApiItem(example.apiItem)}-example${ example.exampleNumber ?? "" }`; @@ -892,7 +892,7 @@ export function createParametersSection( [createParametersSummaryTable(apiFunctionLike.parameters, apiFunctionLike, config)], { title: "Parameters", - id: `${getQualifiedApiItemName(apiFunctionLike)}-parameters`, + id: `${getFileSafeNameForApiItem(apiFunctionLike)}-parameters`, }, ); } @@ -951,7 +951,7 @@ export function createReturnsSection( ? undefined : wrapInSection(children, { title: "Returns", - id: `${getQualifiedApiItemName(apiItem)}-returns`, + id: `${getFileSafeNameForApiItem(apiItem)}-returns`, }); } diff --git a/tools/api-markdown-documenter/src/utilities/ApiItemUtilities.ts b/tools/api-markdown-documenter/src/utilities/ApiItemUtilities.ts index 465b2dd760ac..cbd6fac3d847 100644 --- a/tools/api-markdown-documenter/src/utilities/ApiItemUtilities.ts +++ b/tools/api-markdown-documenter/src/utilities/ApiItemUtilities.ts @@ -166,9 +166,26 @@ export function getFilteredParent(apiItem: ApiItem): ApiItem | undefined { } /** - * Converts bad filename characters to underscores. + * Gets a qualified representation of the API item's display name, accounting for function/method overloads + * by adding a suffix (such as "myMethod_2") as needed to guarantee uniqueness. */ -function getSafeFilenameForName(apiItemName: string): string { +function getQualifiedDisplayName(apiItem: ApiItem): string { + let qualifiedName: string = apiItem.displayName; + if (ApiParameterListMixin.isBaseClassOf(apiItem) && apiItem.overloadIndex > 1) { + // Subtract one for compatibility with earlier releases of API Documenter. + // (This will get revamped when we fix GitHub issue #1308) + qualifiedName += `_${apiItem.overloadIndex - 1}`; + } + return qualifiedName; +} + +/** + * Gets a filename-safe representation of the provided API item name. + * + * @remarks + * - Handles invalid filename characters. + */ +export function getFileSafeNameForApiItemName(apiItemName: string): string { // eslint-disable-next-line unicorn/better-regex, no-useless-escape const badFilenameCharsRegExp: RegExp = /[^a-z0-9_\-\.]/gi; @@ -180,30 +197,20 @@ function getSafeFilenameForName(apiItemName: string): string { /** * Gets a filename-safe representation of the API item's display name. - */ -function getFileSafeNameForApiItem(apiItem: ApiItem): string { - return apiItem.kind === ApiItemKind.Package - ? getSafeFilenameForName(getUnscopedPackageName(apiItem as ApiPackage)) - : getSafeFilenameForName(apiItem.displayName); -} - -// TODO: rename to be a bit more specific -/** - * Adjusts the name of the item as needed. - * Accounts for method overloads by adding a suffix such as "myMethod_2". + * + * @remarks + * - Handles invalid filename characters. + * + * - Qualifies the API item's name, accounting for function/method overloads by adding a suffix (such as "myMethod_2") + * as needed to guarantee uniqueness. * * @param apiItem - The API item for which the qualified name is being queried. * * @public */ -export function getQualifiedApiItemName(apiItem: ApiItem): string { - let qualifiedName: string = getFileSafeNameForApiItem(apiItem); - if (ApiParameterListMixin.isBaseClassOf(apiItem) && apiItem.overloadIndex > 1) { - // Subtract one for compatibility with earlier releases of API Documenter. - // (This will get revamped when we fix GitHub issue #1308) - qualifiedName += `_${apiItem.overloadIndex - 1}`; - } - return qualifiedName; +export function getFileSafeNameForApiItem(apiItem: ApiItem): string { + const qualifiedDisplayName = getQualifiedDisplayName(apiItem); + return getFileSafeNameForApiItemName(qualifiedDisplayName); } /**