fix(tree2): pass and leverage revision metadata in invert (#18299)

Author: yann-achard-MS

experimental/dds/tree2/src/feature-libraries/modular-schema/fieldChangeHandler.ts

@@ -86,6 +86,7 @@ export interface FieldChangeRebaser<TChangeset> {
 		invertChild: NodeChangeInverter,
 		genId: IdAllocator,
 		crossFieldManager: CrossFieldManager,
+		revisionMetadata: RevisionMetadataSource,
 	): TChangeset;
 
 	/**
@@ -96,6 +97,7 @@ export interface FieldChangeRebaser<TChangeset> {
 		originalRevision: RevisionTag | undefined,
 		genId: IdAllocator,
 		crossFieldManager: CrossFieldManager,
+		revisionMetadata: RevisionMetadataSource,
 	): TChangeset;
 
 	/**

experimental/dds/tree2/src/feature-libraries/modular-schema/modularChangeFamily.ts

@@ -299,10 +299,14 @@ export class ModularChangeFamily
 		const genId: IdAllocator = idAllocatorFromState(idState);
 		const crossFieldTable = newCrossFieldTable<InvertData>();
 
+		const { revInfos } = getRevInfoFromTaggedChanges([change]);
+		const revisionMetadata = revisionMetadataSourceFromInfo(revInfos);
+
 		const invertedFields = this.invertFieldMap(
 			tagChange(change.change.fieldChanges, change.revision),
 			genId,
 			crossFieldTable,
+			revisionMetadata,
 		);
 
 		if (crossFieldTable.invalidatedFields.size > 0) {
@@ -317,6 +321,7 @@ export class ModularChangeFamily
 					originalRevision,
 					genId,
 					newCrossFieldManager(crossFieldTable),
+					revisionMetadata,
 				);
 				fieldChange.change = brand(amendedChange);
 			}
@@ -345,6 +350,7 @@ export class ModularChangeFamily
 		changes: TaggedChange<FieldChangeMap>,
 		genId: IdAllocator,
 		crossFieldTable: CrossFieldTable<InvertData>,
+		revisionMetadata: RevisionMetadataSource,
 	): FieldChangeMap {
 		const invertedFields: FieldChangeMap = new Map();
 
@@ -362,9 +368,11 @@ export class ModularChangeFamily
 						{ revision, change: childChanges },
 						genId,
 						crossFieldTable,
+						revisionMetadata,
 					),
 				genId,
 				manager,
+				revisionMetadata,
 			);
 
 			const invertedFieldChange: FieldChange = {
@@ -389,6 +397,7 @@ export class ModularChangeFamily
 		change: TaggedChange<NodeChangeset>,
 		genId: IdAllocator,
 		crossFieldTable: CrossFieldTable<InvertData>,
+		revisionMetadata: RevisionMetadataSource,
 	): NodeChangeset {
 		const inverse: NodeChangeset = {};
 
@@ -397,6 +406,7 @@ export class ModularChangeFamily
 				{ ...change, change: change.change.fieldChanges },
 				genId,
 				crossFieldTable,
+				revisionMetadata,
 			);
 		}
 

experimental/dds/tree2/src/feature-libraries/sequence-field/invert.ts

@@ -6,7 +6,7 @@
 import { assert, unreachableCase } from "@fluidframework/core-utils";
 import { ChangeAtomId, RevisionTag, TaggedChange } from "../../core";
 import { IdAllocator, fail } from "../../util";
-import { CrossFieldManager, CrossFieldTarget } from "../modular-schema";
+import { CrossFieldManager, CrossFieldTarget, RevisionMetadataSource } from "../modular-schema";
 import {
 	Changeset,
 	Mark,
@@ -22,7 +22,7 @@ import { MarkListFactory } from "./markListFactory";
 import {
 	areInputCellsEmpty,
 	extractMarkEffect,
-	getDetachCellId,
+	getDetachOutputId,
 	getEndpoint,
 	getOutputCellId,
 	isAttach,
@@ -48,12 +48,14 @@ export function invert<TNodeChange>(
 	invertChild: NodeChangeInverter<TNodeChange>,
 	genId: IdAllocator,
 	crossFieldManager: CrossFieldManager,
+	revisionMetadata: RevisionMetadataSource,
 ): Changeset<TNodeChange> {
 	return invertMarkList(
 		change.change,
 		change.revision,
 		invertChild,
 		crossFieldManager as CrossFieldManager<TNodeChange>,
+		revisionMetadata,
 	);
 }
 
@@ -75,11 +77,18 @@ function invertMarkList<TNodeChange>(
 	revision: RevisionTag | undefined,
 	invertChild: NodeChangeInverter<TNodeChange>,
 	crossFieldManager: CrossFieldManager<TNodeChange>,
+	revisionMetadata: RevisionMetadataSource,
 ): MarkList<TNodeChange> {
 	const inverseMarkList = new MarkListFactory<TNodeChange>();
 
 	for (const mark of markList) {
-		const inverseMarks = invertMark(mark, revision, invertChild, crossFieldManager);
+		const inverseMarks = invertMark(
+			mark,
+			revision,
+			invertChild,
+			crossFieldManager,
+			revisionMetadata,
+		);
 		inverseMarkList.push(...inverseMarks);
 	}
 
@@ -91,6 +100,7 @@ function invertMark<TNodeChange>(
 	revision: RevisionTag | undefined,
 	invertChild: NodeChangeInverter<TNodeChange>,
 	crossFieldManager: CrossFieldManager<TNodeChange>,
+	revisionMetadata: RevisionMetadataSource,
 ): Mark<TNodeChange>[] {
 	const type = mark.type;
 	switch (type) {
@@ -105,13 +115,11 @@ function invertMark<TNodeChange>(
 			assert(revision !== undefined, 0x5a1 /* Unable to revert to undefined revision */);
 			const markRevision = mark.revision ?? revision;
 			if (mark.cellId === undefined) {
+				const outputId = getDetachOutputId(mark, markRevision, revisionMetadata);
 				const inverse = withNodeChange(
 					{
 						type: "Insert",
-						cellId: mark.detachIdOverride ?? {
-							revision: markRevision,
-							localId: mark.id,
-						},
+						cellId: outputId,
 						count: mark.count,
 					},
 					invertNodeChange(mark.changes, invertChild),
@@ -164,10 +172,10 @@ function invertMark<TNodeChange>(
 				);
 			}
 
-			const cellId = getDetachCellId(
+			const cellId = getDetachOutputId(
 				mark,
 				mark.revision ?? revision ?? fail("Revision must be defined"),
-				undefined,
+				revisionMetadata,
 			) ?? {
 				revision: mark.revision ?? revision ?? fail("Revision must be defined"),
 				localId: mark.id,
@@ -223,8 +231,20 @@ function invertMark<TNodeChange>(
 				changes: mark.changes,
 				...mark.detach,
 			};
-			const attachInverses = invertMark(attach, revision, invertChild, crossFieldManager);
-			const detachInverses = invertMark(detach, revision, invertChild, crossFieldManager);
+			const attachInverses = invertMark(
+				attach,
+				revision,
+				invertChild,
+				crossFieldManager,
+				revisionMetadata,
+			);
+			const detachInverses = invertMark(
+				detach,
+				revision,
+				invertChild,
+				crossFieldManager,
+				revisionMetadata,
+			);
 
 			if (detachInverses.length === 0) {
 				return attachInverses;

experimental/dds/tree2/src/feature-libraries/sequence-field/rebase.ts

@@ -27,7 +27,7 @@ import {
 	cloneCellId,
 	areOutputCellsEmpty,
 	isNewAttach,
-	getDetachCellId,
+	getDetachOutputId,
 	getInputCellId,
 	isTransientEffect,
 	getOutputCellId,
@@ -380,7 +380,7 @@ function rebaseMarkIgnoreChild<TNodeChange>(
 	let rebasedMark = currMark;
 	if (markEmptiesCells(baseMark)) {
 		assert(isDetach(baseMark), 0x70b /* Only detach marks should empty cells */);
-		const baseCellId = getDetachCellId(baseMark, baseRevision, metadata);
+		const baseCellId = getDetachOutputId(baseMark, baseRevision, metadata);
 
 		// TODO: Should also check if this is a transient move source
 		if (isMoveSource(baseMark)) {

experimental/dds/tree2/src/feature-libraries/sequence-field/utils.ts

@@ -142,23 +142,23 @@ export function getOutputCellId(
 ): CellId | undefined {
 	if (markEmptiesCells(mark)) {
 		assert(isDetach(mark), 0x750 /* Only detaches can empty cells */);
-		return getDetachCellId(mark, revision, metadata);
+		return getDetachOutputId(mark, revision, metadata);
 	} else if (markFillsCells(mark)) {
 		return undefined;
 	} else if (isTransientEffect(mark)) {
-		return getDetachCellId(mark.detach, revision, metadata);
+		return getDetachOutputId(mark.detach, revision, metadata);
 	}
 
 	return getInputCellId(mark, revision, metadata);
 }
 
-export function getDetachCellId(
+export function getDetachOutputId(
 	mark: Detach,
 	revision: RevisionTag | undefined,
 	metadata: RevisionMetadataSource | undefined,
-): CellId {
+): ChangeAtomId {
 	return (
-		getOverrideCellId(mark) ?? {
+		getOverrideDetachId(mark) ?? {
 			revision: getIntentionIfMetadataProvided(mark.revision ?? revision, metadata),
 			localId: mark.id,
 		}
@@ -172,7 +172,7 @@ function getIntentionIfMetadataProvided(
 	return metadata === undefined ? revision : getIntention(revision, metadata);
 }
 
-function getOverrideCellId(mark: Detach): CellId | undefined {
+function getOverrideDetachId(mark: Detach): ChangeAtomId | undefined {
 	return mark.type !== "MoveOut" && mark.detachIdOverride !== undefined
 		? mark.detachIdOverride
 		: undefined;
@@ -470,7 +470,7 @@ function tryMergeEffects(
 	if (
 		isDetach(lhs) &&
 		isDetach(rhs) &&
-		!areMergeableCellIds(getOverrideCellId(lhs), lhsCount, getOverrideCellId(rhs))
+		!areMergeableCellIds(getOverrideDetachId(lhs), lhsCount, getOverrideDetachId(rhs))
 	) {
 		return undefined;
 	}

experimental/dds/tree2/src/test/feature-libraries/default-field-kinds/defaultFieldKinds.spec.ts

@@ -175,11 +175,13 @@ describe("defaultFieldKinds", () => {
 				return nodeChange2;
 			};
 
+			const taggedChange = { revision: mintRevisionTag(), change: change1WithChildChange };
 			const inverted = fieldHandler.rebaser.invert(
-				{ revision: mintRevisionTag(), change: change1WithChildChange },
+				taggedChange,
 				childInverter,
 				fakeIdAllocator,
 				failCrossFieldManager,
+				defaultRevisionMetadataFromChanges([taggedChange]),
 			);
 
 			assert.deepEqual(inverted.childChanges, [["self", nodeChange2]]);

experimental/dds/tree2/src/test/feature-libraries/default-field-kinds/optionalChangeRebaser.spec.ts

@@ -114,6 +114,7 @@ function invert(change: TaggedChange<OptionalChangeset>): OptionalChangeset {
 		// Optional fields should not generate IDs during invert
 		fakeIdAllocator,
 		failCrossFieldManager,
+		defaultRevisionMetadataFromChanges([change]),
 	);
 }
 

experimental/dds/tree2/src/test/feature-libraries/default-field-kinds/optionalField.spec.ts

@@ -231,6 +231,7 @@ describe("optionalField", () => {
 					childInverter,
 					fakeIdAllocator,
 					failCrossFieldManager,
+					defaultRevisionMetadataFromChanges([change1]),
 				),
 				expected,
 			);
@@ -296,6 +297,7 @@ describe("optionalField", () => {
 						() => assert.fail("Should not need to invert children"),
 						fakeIdAllocator,
 						failCrossFieldManager,
+						defaultRevisionMetadataFromChanges([deletion]),
 					),
 					tag2,
 					tag1,
@@ -561,6 +563,7 @@ describe("optionalField", () => {
 					() => assert.fail("Should not need to invert children"),
 					fakeIdAllocator,
 					failCrossFieldManager,
+					defaultRevisionMetadataFromChanges([clear]),
 				);
 				const actual = Array.from(
 					optionalChangeHandler.relevantRemovedTrees(restore, failingDelegate),
@@ -621,6 +624,7 @@ describe("optionalField", () => {
 						() => assert.fail("Should not need to invert children"),
 						fakeIdAllocator,
 						failCrossFieldManager,
+						defaultRevisionMetadataFromChanges([clear]),
 					),
 					mintRevisionTag(),
 				);

experimental/dds/tree2/src/test/feature-libraries/modular-schema/genericFieldKind.spec.ts

@@ -21,7 +21,11 @@ import {
 	deltaForSet,
 } from "../../../core";
 import { fakeIdAllocator, brand } from "../../../util";
-import { EncodingTestData, makeEncodingTestSuite } from "../../utils";
+import {
+	EncodingTestData,
+	defaultRevisionMetadataFromChanges,
+	makeEncodingTestSuite,
+} from "../../utils";
 import { IJsonCodec } from "../../../codec";
 import { singleJsonCursor } from "../../../domains";
 import { ValueChangeset, valueField, valueHandler } from "./basicRebasers";
@@ -85,11 +89,13 @@ const childComposer = (nodeChanges: TaggedChange<NodeChangeset>[]): NodeChangese
 
 const childInverter = (nodeChange: NodeChangeset): NodeChangeset => {
 	const valueChange = valueChangeFromNodeChange(nodeChange);
+	const taggedChange = makeAnonChange(valueChange);
 	const inverse = valueHandler.rebaser.invert(
-		makeAnonChange(valueChange),
+		taggedChange,
 		unexpectedDelegate,
 		fakeIdAllocator,
 		crossFieldManager,
+		defaultRevisionMetadataFromChanges([taggedChange]),
 	);
 	return nodeChangeFromValueChange(inverse);
 };
@@ -348,11 +354,13 @@ describe("Generic FieldKind", () => {
 				nodeChange: nodeChange2To1,
 			},
 		];
+		const taggedChange = makeAnonChange(forward);
 		const actual = genericFieldKind.changeHandler.rebaser.invert(
-			makeAnonChange(forward),
+			taggedChange,
 			childInverter,
 			fakeIdAllocator,
 			crossFieldManager,
+			defaultRevisionMetadataFromChanges([taggedChange]),
 		);
 		assert.deepEqual(actual, expected);
 	});