Resolve SequencePlace positions to number positions in client (micros…

…oft#22521) Instead of passing SequencePlace arguments through client into mergeTree, resolve them into the correct number positions in client. This way, all of the mergeTree methods involved in obliterate can operate only on numbers, instead of `number|string` positions. Also removes unused `Side` fields on ISegment - these were leftover from an initial design. [AB#15190](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/15190)
pragya91 · Sep 17, 2024 · 2a26c2b · 2a26c2b
1 parent fc0a441
commit 2a26c2b
Show file tree

Hide file tree

Showing 8 changed files with 68 additions and 111 deletions.
diff --git a/packages/dds/merge-tree/api-report/merge-tree.legacy.alpha.api.md b/packages/dds/merge-tree/api-report/merge-tree.legacy.alpha.api.md
@@ -459,7 +459,6 @@ export interface ISegment extends IMergeNodeCommon, Partial<IRemovalInfo>, Parti
     // (undocumented)
     clone(): ISegment;
     readonly endpointType?: "start" | "end";
-    endSide?: Side.Before | Side.After;
     localRefs?: LocalReferenceCollection;
     localRemovedSeq?: number;
     localSeq?: number;
@@ -471,7 +470,6 @@ export interface ISegment extends IMergeNodeCommon, Partial<IRemovalInfo>, Parti
     seq?: number;
     // (undocumented)
     splitAt(pos: number): ISegment | undefined;
-    startSide?: Side.Before | Side.After;
     // (undocumented)
     toJSONObject(): any;
     // (undocumented)

diff --git a/packages/dds/merge-tree/src/client.ts b/packages/dds/merge-tree/src/client.ts
@@ -255,8 +255,11 @@ export class Client extends TypedEventEmitter<IClientEvents> {
 	 * @param start - The inclusive start of the range to obliterate
 	 * @param end - The exclusive end of the range to obliterate
 	 */
-	// eslint-disable-next-line import/no-deprecated
-	public obliterateRangeLocal(start: number, end: number): IMergeTreeObliterateMsg {
+	public obliterateRangeLocal(
+		start: number,
+		end: number,
+		// eslint-disable-next-line import/no-deprecated
+	): IMergeTreeObliterateMsg {
 		const obliterateOp = createObliterateRangeOp(start, end);
 		this.applyObliterateRangeOp({ op: obliterateOp });
 		return obliterateOp;

diff --git a/packages/dds/merge-tree/src/mergeTree.ts b/packages/dds/merge-tree/src/mergeTree.ts
@@ -90,7 +90,6 @@ import {
 } from "./referencePositions.js";
 // eslint-disable-next-line import/no-deprecated
 import { PropertiesRollback } from "./segmentPropertiesManager.js";
-import { endpointPosAndSide, type SequencePlace } from "./sequencePlace.js";
 import { SortedSegmentSet } from "./sortedSegmentSet.js";
 import { zamboniSegments } from "./zamboni.js";
 
@@ -1601,11 +1600,7 @@ export class MergeTree {
 		return { next };
 	};
 
-	private ensureIntervalBoundary(
-		pos: number | "start" | "end",
-		refSeq: number,
-		clientId: number,
-	): void {
+	private ensureIntervalBoundary(pos: number, refSeq: number, clientId: number): void {
 		const splitNode = this.insertingWalk(
 			this.root,
 			pos,
@@ -1649,21 +1644,14 @@ export class MergeTree {
 
 	private insertingWalk(
 		block: MergeBlock,
-		pos: number | "start" | "end",
+		pos: number,
 		refSeq: number,
 		clientId: number,
 		seq: number,
 		context: InsertContext,
 		isLastChildBlock: boolean = true,
 	): MergeBlock | undefined {
-		let _pos: number;
-		if (pos === "start") {
-			_pos = 0;
-		} else if (pos === "end") {
-			_pos = this.root.mergeTree?.getLength(refSeq, clientId) ?? 0;
-		} else {
-			_pos = pos;
-		}
+		let _pos: number = pos;
 
 		const children = block.children;
 		let childIndex: number;
@@ -1879,8 +1867,8 @@ export class MergeTree {
 	}
 
 	public obliterateRange(
-		start: SequencePlace,
-		end: SequencePlace,
+		start: number,
+		end: number,
 		refSeq: number,
 		clientId: number,
 		seq: number,
@@ -1889,20 +1877,8 @@ export class MergeTree {
 	): void {
 		errorIfOptionNotTrue(this.options, "mergeTreeEnableObliterate");
 
-		const { startPos, startSide, endPos, endSide } = endpointPosAndSide(start, end);
-
-		assert(
-			startPos !== undefined &&
-				endPos !== undefined &&
-				startSide !== undefined &&
-				endSide !== undefined &&
-				startPos !== "end" &&
-				endPos !== "start",
-			0x9e2 /* start and end cannot be undefined because they were not passed in as undefined */,
-		);
-
-		this.ensureIntervalBoundary(startPos, refSeq, clientId);
-		this.ensureIntervalBoundary(endPos, refSeq, clientId);
+		this.ensureIntervalBoundary(start, refSeq, clientId);
+		this.ensureIntervalBoundary(end, refSeq, clientId);
 
 		let _overwrite = overwrite;
 		const localOverlapWithRefs: ISegment[] = [];
@@ -1919,20 +1895,9 @@ export class MergeTree {
 			localSeq,
 			segmentGroup: undefined,
 		};
-		const normalizedStartPos = startPos === "start" || startPos === undefined ? 0 : startPos;
-		const normalizedEndPos =
-			endPos === "end" || endPos === undefined ? this.getLength(refSeq, clientId) : endPos;
 
-		const { segment: startSeg } = this.getContainingSegment(
-			normalizedStartPos,
-			refSeq,
-			clientId,
-		);
-		const { segment: endSeg } = this.getContainingSegment(
-			normalizedEndPos - 1,
-			refSeq,
-			clientId,
-		);
+		const { segment: startSeg } = this.getContainingSegment(start, refSeq, clientId);
+		const { segment: endSeg } = this.getContainingSegment(end - 1, refSeq, clientId);
 		assert(
 			startSeg !== undefined && endSeg !== undefined,
 			0xa3f /* segments cannot be undefined */,
@@ -2684,28 +2649,18 @@ export class MergeTree {
 		leaf: ISegmentAction<TClientData>,
 		accum: TClientData,
 		post?: BlockAction<TClientData>,
-		start: SequencePlace = 0,
-		end?: SequencePlace,
+		start: number = 0,
+		end?: number,
 		localSeq?: number,
 		visibilitySeq: number = refSeq,
 	): void {
-		const maybeEndPos = end ?? this.nodeLength(this.root, refSeq, clientId, localSeq) ?? 0;
-		if (maybeEndPos === start) {
+		const endPos = end ?? this.nodeLength(this.root, refSeq, clientId, localSeq) ?? 0;
+		if (endPos === start) {
 			return;
 		}
 
 		let pos = 0;
-		let { startPos, endPos } = endpointPosAndSide(start, end);
 
-		startPos = startPos === "start" || startPos === undefined ? 0 : startPos;
-		endPos =
-			endPos === "end" || endPos === undefined
-				? this.root.mergeTree?.getLength(refSeq, clientId) ?? 0
-				: endPos;
-		assert(
-			startPos !== "end" && endPos !== "start",
-			0x9e3 /* start cannot be 'end' and end cannot be 'start' */,
-		);
 		depthFirstNodeWalk(
 			this.root,
 			this.root.children[0],
@@ -2733,15 +2688,13 @@ export class MergeTree {
 
 				const nextPos = pos + lenAtRefSeq;
 				// start is beyond the current node, so we can skip it
-				if (typeof startPos === "number" && startPos >= nextPos) {
+				if (start >= nextPos) {
 					pos = nextPos;
 					return NodeAction.Skip;
 				}
 
 				if (node.isLeaf()) {
-					if (
-						leaf(node, pos, refSeq, clientId, startPos - pos, endPos - pos, accum) === false
-					) {
+					if (leaf(node, pos, refSeq, clientId, start - pos, endPos - pos, accum) === false) {
 						return NodeAction.Exit;
 					}
 					pos = nextPos;
@@ -2751,7 +2704,7 @@ export class MergeTree {
 			post === undefined
 				? undefined
 				: (block): boolean =>
-						post(block, pos, refSeq, clientId, startPos - pos, endPos - pos, accum),
+						post(block, pos, refSeq, clientId, start - pos, endPos - pos, accum),
 		);
 	}
 }
diff --git a/packages/dds/merge-tree/src/mergeTreeNodes.ts b/packages/dds/merge-tree/src/mergeTreeNodes.ts
@@ -30,7 +30,6 @@ import {
 import { SegmentGroupCollection } from "./segmentGroupCollection.js";
 // eslint-disable-next-line import/no-deprecated
 import { PropertiesManager, PropertiesRollback } from "./segmentPropertiesManager.js";
-import { Side } from "./sequencePlace.js";
 
 /**
  * Common properties for a node in a merge tree.
@@ -263,14 +262,6 @@ export interface ISegment extends IMergeNodeCommon, Partial<IRemovalInfo>, Parti
 	 * Properties that have been added to this segment via annotation.
 	 */
 	properties?: PropertySet;
-	/**
-	 * Stores side information passed to obliterate for the start of a range.
-	 */
-	startSide?: Side.Before | Side.After;
-	/**
-	 * Stores side information passed to obliterate for the end of a range.
-	 */
-	endSide?: Side.Before | Side.After;
 
 	/**
 	 * Add properties to this segment via annotation.

diff --git a/packages/dds/merge-tree/src/test/obliterate.partialLength.spec.ts b/packages/dds/merge-tree/src/test/obliterate.partialLength.spec.ts
@@ -10,6 +10,7 @@ import { MergeTreeDeltaType } from "../ops.js";
 import { TestClient } from "./testClient.js";
 import {
 	insertText,
+	obliterateRange,
 	useStrictPartialLengthChecks,
 	validatePartialLengths,
 } from "./testUtils.js";
@@ -113,7 +114,8 @@ describe("obliterate partial lengths", () => {
 	describe("overlapping remove+obliterate", () => {
 		it("passes for local remove and remote obliterate", () => {
 			const localRemoveOp = client.removeRangeLocal(0, "hello ".length);
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: 0,
 				end: "hello ".length,
 				refSeq,
@@ -174,7 +176,8 @@ describe("obliterate partial lengths", () => {
 				refSeq,
 				client.getLongClientId(remoteClientId),
 			);
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: 0,
 				end: "hello ".length,
 				refSeq,
@@ -215,7 +218,8 @@ describe("obliterate partial lengths", () => {
 	describe("overlapping obliterate+obliterate", () => {
 		it("passes for local obliterate and remote obliterate", () => {
 			const localObliterateOp = client.obliterateRangeLocal(0, "hello ".length);
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: 0,
 				end: "hello ".length,
 				refSeq,
@@ -246,7 +250,8 @@ describe("obliterate partial lengths", () => {
 		});
 
 		it("passes for remote obliterate and local obliterate", () => {
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: 0,
 				end: "hello ".length,
 				refSeq,

diff --git a/packages/dds/merge-tree/src/test/obliterate.spec.ts b/packages/dds/merge-tree/src/test/obliterate.spec.ts
@@ -9,7 +9,7 @@ import type { ObliterateInfo } from "../mergeTreeNodes.js";
 import { MergeTreeDeltaType } from "../ops.js";
 
 import { TestClient } from "./testClient.js";
-import { insertText } from "./testUtils.js";
+import { insertText, obliterateRange } from "./testUtils.js";
 
 describe("obliterate", () => {
 	let client: TestClient;
@@ -41,7 +41,8 @@ describe("obliterate", () => {
 
 	describe("concurrent obliterate and insert", () => {
 		it("removes text for obliterate then insert", () => {
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: 0,
 				end: client.getLength(),
 				refSeq,
@@ -73,7 +74,8 @@ describe("obliterate", () => {
 				props: undefined,
 				opArgs: { op: { type: MergeTreeDeltaType.INSERT } },
 			});
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: 0,
 				end: "hello world".length,
 				refSeq,
@@ -95,7 +97,8 @@ describe("obliterate", () => {
 				props: undefined,
 				opArgs: { op: { type: MergeTreeDeltaType.INSERT } },
 			});
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: 1,
 				end: "hello world".length,
 				refSeq,
@@ -110,7 +113,8 @@ describe("obliterate", () => {
 
 	describe("endpoint behavior", () => {
 		it("does not expand to include text inserted at start", () => {
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: 5,
 				end: "hello world".length,
 				refSeq,
@@ -132,7 +136,8 @@ describe("obliterate", () => {
 			assert.equal(client.getText(), "helloXXX");
 		});
 		it("does not expand to include text inserted at end", () => {
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: 0,
 				end: "hello".length,
 				refSeq,
@@ -181,7 +186,8 @@ describe("obliterate", () => {
 			const obliterateEnd = client.getLength();
 			const startSeg = client.getContainingSegment(obliterateStart);
 			const endSeg = client.getContainingSegment(obliterateEnd);
-			client.obliterateRange({
+			obliterateRange({
+				mergeTree: client.mergeTree,
 				start: obliterateStart,
 				end: obliterateEnd,
 				refSeq,
@@ -203,11 +209,11 @@ describe("obliterate", () => {
 			assert.equal(client.getText(), "");
 
 			startSeg.segment?.localRefs?.walkReferences((ref) => {
-				const oblProps = ref.properties?.obliterate as ObliterateInfo | undefined;
+				const oblProps = ref.properties?.obliterate as ObliterateInfo;
 				assert(oblProps?.start !== undefined, "start ref should NOT be removed");
 			});
 			endSeg.segment?.localRefs?.walkReferences((ref) => {
-				const oblProps = ref.properties?.obliterate as ObliterateInfo | undefined;
+				const oblProps = ref.properties?.obliterate as ObliterateInfo;
 				assert(oblProps?.end !== undefined, "end ref should NOT be removed");
 			});
 

diff --git a/packages/dds/merge-tree/src/test/testClient.ts b/packages/dds/merge-tree/src/test/testClient.ts
@@ -22,9 +22,8 @@ import { MergeTreeTextHelper } from "../MergeTreeTextHelper.js";
 import { Client } from "../client.js";
 import { DoublyLinkedList } from "../collections/index.js";
 import { UnassignedSequenceNumber } from "../constants.js";
-import { IMergeTreeOptions, ReferencePosition, type SequencePlace } from "../index.js";
+import { IMergeTreeOptions, ReferencePosition } from "../index.js";
 import { MergeTree, getSlideToSegoff } from "../mergeTree.js";
-import { IMergeTreeDeltaOpArgs } from "../mergeTreeDeltaCallback.js";
 import {
 	backwardExcursion,
 	forwardExcursion,
@@ -189,26 +188,6 @@ export class TestClient extends Client {
 		});
 	}
 
-	public obliterateRange({
-		start,
-		end,
-		refSeq,
-		clientId,
-		seq,
-		overwrite = false,
-		opArgs,
-	}: {
-		start: SequencePlace;
-		end: SequencePlace;
-		refSeq: number;
-		clientId: number;
-		seq: number;
-		overwrite?: boolean;
-		opArgs: IMergeTreeDeltaOpArgs;
-	}): void {
-		this.mergeTree.obliterateRange(start, end, refSeq, clientId, seq, overwrite, opArgs);
-	}
-
 	public getText(start?: number, end?: number): string {
 		return this.textHelper.getText(this.getCurrentSeq(), this.getClientId(), "", start, end);
 	}