diff --git a/packages/dds/merge-tree/src/client.ts b/packages/dds/merge-tree/src/client.ts index dfffdc7cba1c..34d839549059 100644 --- a/packages/dds/merge-tree/src/client.ts +++ b/packages/dds/merge-tree/src/client.ts @@ -960,6 +960,89 @@ export class Client extends TypedEventEmitter { }; } + private resetAnnotateOp( + resetOp: IMergeTreeAnnotateMsg | IMergeTreeAnnotateAdjustMsg, + segment: ISegmentLeaf, + segmentPosition: number, + ): IMergeTreeDeltaOp | undefined { + assert( + segment.propertyManager?.hasPendingProperties(resetOp.props ?? resetOp.adjust) === true, + 0x036 /* "Segment has no pending properties" */, + ); + // if the segment has been removed or obliterated, there's no need to send the annotate op + // unless the remove was local, in which case the annotate must have come + // before the remove + if (!isRemovedAndAcked(segment)) { + return resetOp.props === undefined + ? createAdjustRangeOp( + segmentPosition, + segmentPosition + segment.cachedLength, + resetOp.adjust, + ) + : createAnnotateRangeOp( + segmentPosition, + segmentPosition + segment.cachedLength, + resetOp.props, + ); + } + return undefined; + } + + private resetInsertOp( + resetOp: IMergeTreeInsertMsg, + segment: ISegmentLeaf, + segmentPosition: number, + squash: boolean, + ): IMergeTreeDeltaOp | undefined { + if (isInserted(segment) && opstampUtils.isSquashedOp(segment.insert)) { + return undefined; + } + assert( + isInserted(segment) && opstampUtils.isLocal(segment.insert), + 0x037 /* "Segment already has assigned sequence number" */, + ); + const removeInfo = toRemovalInfo(segment); + + const unusedStamp: OperationStamp = { seq: 0, clientId: 0 }; + if (removeInfo !== undefined && squash) { + assert( + removeInfo.removes.length === 1 || + opstampUtils.isAcked(removeInfo.removes[removeInfo.removes.length - 2]), + 0xbaf /* Expected only one local remove */, + ); + this.squashInsertion(segment); + return undefined; + } else if (removeInfo !== undefined && opstampUtils.isAcked(removeInfo.removes[0])) { + assert( + removeInfo.removes[0].type === "sliceRemove", + 0xb5c /* Remove on insertion must be caused by obliterate. */, + ); + errorIfOptionNotTrue(this._mergeTree.options, "mergeTreeEnableObliterateReconnect"); + // the segment was remotely obliterated, so is considered removed + // we set the seq to the universal seq and remove the local seq, + // so its length is not considered for subsequent local changes + // this allows us to not send the op as even the local client will ignore the segment + overwriteInfo(segment, { + insert: { + type: "insert", + seq: UniversalSequenceNumber, + localSeq: undefined, + clientId: NonCollabClient, + }, + }); + this._mergeTree.blockUpdatePathLengths(segment.parent, unusedStamp, true); + return undefined; + } + + const segInsertOp: ISegment = segment.clone(); + const opProps = + isObject(resetOp.seg) && "props" in resetOp.seg && isObject(resetOp.seg.props) + ? { ...resetOp.seg.props } + : undefined; + segInsertOp.properties = opProps; + return createInsertSegmentOp(segmentPosition, segInsertOp); + } + private resetPendingDeltaToOps( resetOp: IMergeTreeDeltaOp, @@ -1174,82 +1257,12 @@ export class Client extends TypedEventEmitter { let newOp: IMergeTreeDeltaOp | undefined; switch (resetOp.type) { case MergeTreeDeltaType.ANNOTATE: { - assert( - segment.propertyManager?.hasPendingProperties(resetOp.props ?? resetOp.adjust) === - true, - 0x036 /* "Segment has no pending properties" */, - ); - // if the segment has been removed or obliterated, there's no need to send the annotate op - // unless the remove was local, in which case the annotate must have come - // before the remove - if (!isRemovedAndAcked(segment)) { - newOp = - resetOp.props === undefined - ? createAdjustRangeOp( - segmentPosition, - segmentPosition + segment.cachedLength, - resetOp.adjust, - ) - : createAnnotateRangeOp( - segmentPosition, - segmentPosition + segment.cachedLength, - resetOp.props, - ); - } + newOp = this.resetAnnotateOp(resetOp, segment, segmentPosition); break; } case MergeTreeDeltaType.INSERT: { - if (isInserted(segment) && opstampUtils.isSquashedOp(segment.insert)) { - break; - } - assert( - isInserted(segment) && opstampUtils.isLocal(segment.insert), - 0x037 /* "Segment already has assigned sequence number" */, - ); - const removeInfo = toRemovalInfo(segment); - - const unusedStamp: OperationStamp = { seq: 0, clientId: 0 }; - if (removeInfo !== undefined && squash) { - assert( - removeInfo.removes.length === 1 || - opstampUtils.isAcked(removeInfo.removes[removeInfo.removes.length - 2]), - 0xbaf /* Expected only one local remove */, - ); - this.squashInsertion(segment); - break; - } else if (removeInfo !== undefined && opstampUtils.isAcked(removeInfo.removes[0])) { - assert( - removeInfo.removes[0].type === "sliceRemove", - 0xb5c /* Remove on insertion must be caused by obliterate. */, - ); - errorIfOptionNotTrue( - this._mergeTree.options, - "mergeTreeEnableObliterateReconnect", - ); - // the segment was remotely obliterated, so is considered removed - // we set the seq to the universal seq and remove the local seq, - // so its length is not considered for subsequent local changes - // this allows us to not send the op as even the local client will ignore the segment - overwriteInfo(segment, { - insert: { - type: "insert", - seq: UniversalSequenceNumber, - localSeq: undefined, - clientId: NonCollabClient, - }, - }); - this._mergeTree.blockUpdatePathLengths(segment.parent, unusedStamp, true); - break; - } - - const segInsertOp: ISegment = segment.clone(); - const opProps = - isObject(resetOp.seg) && "props" in resetOp.seg && isObject(resetOp.seg.props) - ? { ...resetOp.seg.props } - : undefined; - segInsertOp.properties = opProps; - newOp = createInsertSegmentOp(segmentPosition, segInsertOp); + newOp = this.resetInsertOp(resetOp, segment, segmentPosition, squash); break; } @@ -1270,11 +1283,19 @@ export class Client extends TypedEventEmitter { } if (newOp) { + let newPreviousProps: WeakMap | undefined; + if (segmentGroup.previousProps) { + const sourceProps = segmentGroup.previousProps.get(segment); + newPreviousProps = new WeakMap(); + if (sourceProps !== undefined) { + newPreviousProps.set(segment, sourceProps); + } + } const newSegmentGroup: SegmentGroup = { segments: [], localSeq: segmentGroup.localSeq, refSeq: this.getCollabWindow().currentSeq, - previousProps: segmentGroup.previousProps?.slice(0), + previousProps: newPreviousProps, }; segment.segmentGroups.enqueue(newSegmentGroup); diff --git a/packages/dds/merge-tree/src/mergeTree.ts b/packages/dds/merge-tree/src/mergeTree.ts index 23f0925b4ee6..90afa30b2e79 100644 --- a/packages/dds/merge-tree/src/mergeTree.ts +++ b/packages/dds/merge-tree/src/mergeTree.ts @@ -1426,7 +1426,7 @@ export class MergeTree { refSeq: this.collabWindow.currentSeq, }; if (previousProps) { - _segmentGroup.previousProps = []; + _segmentGroup.previousProps = new WeakMap(); } this.pendingSegments.push(_segmentGroup); } @@ -1438,7 +1438,7 @@ export class MergeTree { throw new Error("All segments in group should have previousProps or none"); } if (previousProps) { - _segmentGroup.previousProps!.push(previousProps); + _segmentGroup.previousProps!.set(segment, previousProps); } const segmentGroups = (segment.segmentGroups ??= new SegmentGroupCollection(segment)); @@ -2449,7 +2449,6 @@ export class MergeTree { ) { throw new Error("Rollback op doesn't match last edit"); } - let i = 0; for (const segment of pendingSegmentGroup.segments) { const segmentSegmentGroup = segment?.segmentGroups?.pop(); assert( @@ -2476,7 +2475,11 @@ export class MergeTree { { op: removeOp, rollback: true }, ); } /* op.type === MergeTreeDeltaType.ANNOTATE */ else { - const props = pendingSegmentGroup.previousProps![i]; + const props = pendingSegmentGroup.previousProps!.get(segment); + assert( + props !== undefined, + "Segment missing previousProps entry on annotate rollback", + ); // If the segment has been removed by a concurrent operation, we can't use // position-based annotateRange because findRollbackPosition returns a position @@ -2505,7 +2508,6 @@ export class MergeTree { { op: annotateOp, rollback: true }, ); } - i++; } } } else { diff --git a/packages/dds/merge-tree/src/mergeTreeNodes.ts b/packages/dds/merge-tree/src/mergeTreeNodes.ts index ab755a4060d4..021b524a8928 100644 --- a/packages/dds/merge-tree/src/mergeTreeNodes.ts +++ b/packages/dds/merge-tree/src/mergeTreeNodes.ts @@ -233,7 +233,7 @@ export interface ObliterateInfo { export interface SegmentGroup { segments: ISegmentLeaf[]; - previousProps?: PropertySet[]; + previousProps?: WeakMap; localSeq?: number; refSeq: number; obliterateInfo?: ObliterateInfo; diff --git a/packages/dds/merge-tree/src/segmentGroupCollection.ts b/packages/dds/merge-tree/src/segmentGroupCollection.ts index 5456892c8080..4350c9ef37a5 100644 --- a/packages/dds/merge-tree/src/segmentGroupCollection.ts +++ b/packages/dds/merge-tree/src/segmentGroupCollection.ts @@ -6,6 +6,7 @@ import { DoublyLinkedList, walkList } from "@fluidframework/core-utils/internal"; import type { SegmentGroup, ISegmentLeaf } from "./mergeTreeNodes.js"; +import type { PropertySet } from "./properties.js"; export class SegmentGroupCollection { private readonly segmentGroups: DoublyLinkedList; @@ -48,13 +49,21 @@ export class SegmentGroupCollection { walkList(this.segmentGroups, (sg) => segmentGroups.enqueueOnCopy(sg.data, this.segment)); } + /** + * Returns the previousProps entry paired with this collection's segment within the given + * segmentGroup, or undefined if the group has no previousProps or no entry exists for the segment. + */ + public previousPropsForSegment(segmentGroup: SegmentGroup): PropertySet | undefined { + return segmentGroup.previousProps?.get(this.segment); + } + private enqueueOnCopy(segmentGroup: SegmentGroup, sourceSegment: ISegmentLeaf): void { this.enqueue(segmentGroup); if (segmentGroup.previousProps) { - // duplicate the previousProps for this segment - const index = segmentGroup.segments.indexOf(sourceSegment); - if (index !== -1) { - segmentGroup.previousProps.push(segmentGroup.previousProps[index]); + // duplicate the previousProps entry for the destination segment, keyed off the source's entry + const sourceProps = segmentGroup.previousProps.get(sourceSegment); + if (sourceProps !== undefined) { + segmentGroup.previousProps.set(this.segment, sourceProps); } } } diff --git a/packages/dds/merge-tree/src/test/segmentGroupCollection.spec.ts b/packages/dds/merge-tree/src/test/segmentGroupCollection.spec.ts index 4845aaea0cf4..d7d9ecacb491 100644 --- a/packages/dds/merge-tree/src/test/segmentGroupCollection.spec.ts +++ b/packages/dds/merge-tree/src/test/segmentGroupCollection.spec.ts @@ -5,7 +5,13 @@ import { strict as assert } from "node:assert"; -import { assignChild, MergeBlock, type ISegmentPrivate } from "../mergeTreeNodes.js"; +import { + assignChild, + MergeBlock, + type ISegmentLeaf, + type ISegmentPrivate, + type SegmentGroup, +} from "../mergeTreeNodes.js"; import { SegmentGroupCollection } from "../segmentGroupCollection.js"; import { type IHasInsertionInfo, overwriteInfo } from "../segmentInfos.js"; import { TextSegment } from "../textSegment.js"; @@ -61,6 +67,53 @@ describe("segmentGroupCollection", () => { assert.equal(dequeuedSegmentGroup, segmentGroup); }); + describe(".previousPropsForSegment", () => { + it("returns undefined when the group has no previousProps", () => { + const segmentGroup: SegmentGroup = { segments: [], localSeq: 1, refSeq: 0 }; + segmentGroups.enqueue(segmentGroup); + + assert.equal(segmentGroups.previousPropsForSegment(segmentGroup), undefined); + }); + + it("returns undefined when the segment has no entry in previousProps", () => { + const otherSegment = overwriteInfo(TextSegment.make("xyz"), { + insert: { + type: "insert", + clientId: 0, + seq: 1, + }, + }); + assignChild(parent, otherSegment, parent.childCount++); + const previousProps = new WeakMap(); + const segmentGroup: SegmentGroup = { + segments: [], + localSeq: 1, + refSeq: 0, + previousProps, + }; + // Only the unrelated segment has an entry; `segment` does not. + previousProps.set(otherSegment as ISegmentLeaf, { foo: "bar" }); + segmentGroups.enqueue(segmentGroup); + + assert.equal(segmentGroups.previousPropsForSegment(segmentGroup), undefined); + }); + + it("returns the previousProps entry for the collection's segment", () => { + const expectedProps = { color: "blue" }; + const previousProps = new WeakMap(); + const segmentGroup: SegmentGroup = { + segments: [], + localSeq: 1, + refSeq: 0, + previousProps, + }; + previousProps.set(segment as ISegmentLeaf, expectedProps); + segmentGroups.enqueue(segmentGroup); + + assert.equal(segmentGroups.previousPropsForSegment(segmentGroup), expectedProps); + }); + }); + it(".copyTo", () => { const segmentGroupCount = 6; while (segmentGroups.size < segmentGroupCount) {