MergeTree: Deprecate the PropertyManager Class and Its Exposure (#22183)

Deprecate the PropertyManager Class and Its Exposure The `PropertyManager` class, along with the `propertyManager` properties and `addProperties` functions on segments and intervals, are not intended for external use. These elements will be removed in a future release for the following reasons: * There are no scenarios where their direct use is needed. * Using them directly will result in eventual consistency problems. * Upcoming features will require modifications to these mechanisms. I've removed most internal usages where the deprecated things are not necessary, to make later modifications easier. Additionally, we no longer instantiate PropertyManager class unless is it is needed. [AB#12580](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/12580) --------- Co-authored-by: Tyler Butler <[email protected]>
microsoft · Aug 13, 2024 · cbba695 · cbba695
1 parent 876b1b4
commit cbba695
Show file tree

Hide file tree

Showing 25 changed files with 202 additions and 175 deletions.
diff --git a/.changeset/fancy-books-smell.md b/.changeset/fancy-books-smell.md
@@ -0,0 +1,17 @@
+---
+"fluid-framework": minor
+"@fluidframework/merge-tree": minor
+"@fluidframework/sequence": minor
+"@fluid-experimental/sequence-deprecated": minor
+---
+---
+"section": "deprecation"
+---
+The PropertyManager class and related functions and properties are deprecated
+
+The `PropertyManager` class, along with the `propertyManager` properties and `addProperties` functions on segments and intervals, are not intended for external use.
+These elements will be removed in a future release for the following reasons:
+
+ * There are no scenarios where they need to be used directly.
+ * Using them directly will cause eventual consistency problems.
+ * Upcoming features will require modifications to these mechanisms.
diff --git a/examples/data-objects/prosemirror/src/fluidBridge.ts b/examples/data-objects/prosemirror/src/fluidBridge.ts
@@ -450,10 +450,7 @@ function sliceToGroupOpsInternal(
 	const node = schema.nodes[value.type];
 	if (node.isInline) {
 		if (value.type === "text") {
-			const segment = new TextSegment(value.text);
-			if (props) {
-				segment.addProperties(props);
-			}
+			const segment = TextSegment.make(value.text, props);
 			ops.push(createInsertSegmentOp(from + offset, segment));
 
 			offset = adjustOffset(from, offset, value.text.length, insert, gapDistance);
@@ -466,8 +463,7 @@ function sliceToGroupOpsInternal(
 				},
 			};
 
-			const marker = new Marker(ReferenceType.Simple);
-			marker.addProperties(nodeProps);
+			const marker = Marker.make(ReferenceType.Simple, nodeProps);
 			ops.push(createInsertSegmentOp(from + offset, marker));
 
 			offset = adjustOffset(from, offset, 1, insert, gapDistance);
@@ -483,8 +479,7 @@ function sliceToGroupOpsInternal(
 				},
 			};
 
-			const marker = new Marker(ReferenceType.Simple);
-			marker.addProperties(beginProps);
+			const marker = Marker.make(ReferenceType.Simple, beginProps);
 			ops.push(createInsertSegmentOp(from + offset, marker));
 
 			offset = adjustOffset(from, offset, 1, insert, gapDistance);
@@ -514,8 +509,7 @@ function sliceToGroupOpsInternal(
 				},
 			};
 
-			const marker = new Marker(ReferenceType.Simple);
-			marker.addProperties(endProps);
+			const marker = Marker.make(ReferenceType.Simple, endProps);
 			ops.push(createInsertSegmentOp(from + offset, marker));
 
 			offset = adjustOffset(from, offset, 1, insert, gapDistance);

diff --git a/experimental/dds/sequence-deprecated/src/sequenceFactory.ts b/experimental/dds/sequence-deprecated/src/sequenceFactory.ts
@@ -49,11 +49,7 @@ export class SharedObjectSequenceFactory implements IChannelFactory {
 	public static segmentFromSpec(segSpec: IJSONSegment): SubSequence<object> {
 		const runSegment = segSpec as IJSONRunSegment<object>;
 		if (runSegment.items) {
-			const seg = new SubSequence<object>(runSegment.items);
-			if (runSegment.props) {
-				seg.addProperties(runSegment.props);
-			}
-			return seg;
+			return new SubSequence<object>(runSegment.items, runSegment.props);
 		}
 
 		throw new Error(`Unrecognized IJSONObject`);
@@ -132,11 +128,7 @@ export class SharedNumberSequenceFactory implements IChannelFactory {
 	public static segmentFromSpec(segSpec: IJSONSegment): SubSequence<number> {
 		const runSegment = segSpec as IJSONRunSegment<number>;
 		if (runSegment.items) {
-			const seg = new SubSequence<number>(runSegment.items);
-			if (runSegment.props) {
-				seg.addProperties(runSegment.props);
-			}
-			return seg;
+			return new SubSequence<number>(runSegment.items, runSegment.props);
 		}
 
 		throw new Error(`Unrecognized IJSONObject`);

diff --git a/experimental/dds/sequence-deprecated/src/sparsematrix.ts b/experimental/dds/sequence-deprecated/src/sparsematrix.ts
@@ -39,18 +39,14 @@ export class PaddingSegment extends BaseSegment {
 	}
 	public static fromJSONObject(spec: any) {
 		if (spec && typeof spec === "object" && "pad" in spec) {
-			const segment = new PaddingSegment(spec.pad);
-			if (spec.props) {
-				segment.addProperties(spec.props);
-			}
-			return segment;
+			return new PaddingSegment(spec.pad, spec.props);
 		}
 		return undefined;
 	}
 	public readonly type = PaddingSegment.typeString;
 
-	constructor(size: number) {
-		super();
+	constructor(size: number, props?: PropertySet) {
+		super(props);
 		this.cachedLength = size;
 	}
 
@@ -111,20 +107,19 @@ export class RunSegment extends SubSequence<SparseMatrixItem> {
 	}
 	public static fromJSONObject(spec: any) {
 		if (spec && typeof spec === "object" && "items" in spec) {
-			const segment = new RunSegment(spec.items);
-			if (spec.props) {
-				segment.addProperties(spec.props);
-			}
-			return segment;
+			return new RunSegment(spec.items, spec.props);
 		}
 		return undefined;
 	}
 	public readonly type = RunSegment.typeString;
 
 	private tags: any[];
 
-	constructor(public items: SparseMatrixItem[]) {
-		super(items);
+	constructor(
+		public items: SparseMatrixItem[],
+		props?: PropertySet,
+	) {
+		super(items, props);
 		this.tags = new Array(items.length).fill(undefined);
 	}
 
@@ -261,10 +256,7 @@ export class SparseMatrixClass extends SharedSegmentSequence<MatrixSegment> {
 	public setItems(row: number, col: number, values: SparseMatrixItem[], props?: PropertySet) {
 		const start = rowColToPosition(row, col);
 		const end = start + values.length;
-		const segment = new RunSegment(values);
-		if (props) {
-			segment.addProperties(props);
-		}
+		const segment = new RunSegment(values, props);
 
 		this.replaceRange(start, end, segment);
 	}

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
@@ -18,9 +18,10 @@ export interface AttributionPolicy {
 
 // @alpha (undocumented)
 export abstract class BaseSegment implements ISegment {
+    constructor(properties?: PropertySet);
     // (undocumented)
     ack(segmentGroup: SegmentGroup, opArgs: IMergeTreeDeltaOpArgs): boolean;
-    // (undocumented)
+    // @deprecated
     addProperties(newProps: PropertySet, seq?: number, collaborating?: boolean, rollback?: PropertiesRollback): PropertySet;
     // (undocumented)
     protected addSerializedProps(jseg: IJSONSegment): void;
@@ -64,7 +65,7 @@ export abstract class BaseSegment implements ISegment {
     ordinal: string;
     // (undocumented)
     properties?: PropertySet;
-    // (undocumented)
+    // @deprecated
     propertyManager?: PropertiesManager;
     // (undocumented)
     removedClientIds?: number[];
@@ -445,6 +446,7 @@ export interface IRemovalInfo {
 // @alpha
 export interface ISegment extends IMergeNodeCommon, Partial<IRemovalInfo>, Partial<IMoveInfo> {
     ack(segmentGroup: SegmentGroup, opArgs: IMergeTreeDeltaOpArgs): boolean;
+    // @deprecated
     addProperties(newProps: PropertySet, seq?: number, collaborating?: boolean, rollback?: PropertiesRollback): PropertySet;
     // (undocumented)
     append(segment: ISegment): void;
@@ -461,6 +463,7 @@ export interface ISegment extends IMergeNodeCommon, Partial<IRemovalInfo>, Parti
     localRemovedSeq?: number;
     localSeq?: number;
     properties?: PropertySet;
+    // @deprecated
     propertyManager?: PropertiesManager;
     // (undocumented)
     readonly segmentGroups: SegmentGroupCollection;
@@ -521,6 +524,8 @@ export class LocalReferenceCollection {
 
 // @alpha @sealed (undocumented)
 export interface LocalReferencePosition extends ReferencePosition {
+    // (undocumented)
+    addProperties(newProps: PropertySet): void;
     // (undocumented)
     callbacks?: Partial<Record<"beforeSlide" | "afterSlide", (ref: LocalReferencePosition) => void>>;
     readonly canSlideToEndpoint?: boolean;
@@ -536,7 +541,7 @@ export interface MapLike<T> {
 
 // @alpha
 export class Marker extends BaseSegment implements ReferencePosition, ISegment {
-    constructor(refType: ReferenceType);
+    constructor(refType: ReferenceType, props?: PropertySet);
     // (undocumented)
     append(): void;
     // (undocumented)
@@ -635,7 +640,7 @@ export interface MergeTreeRevertibleDriver {
     removeRange(start: number, end: number): void;
 }
 
-// @alpha (undocumented)
+// @alpha @deprecated (undocumented)
 export class PropertiesManager {
     // (undocumented)
     ackPendingProperties(annotateOp: IMergeTreeAnnotateMsg): void;
@@ -648,7 +653,7 @@ export class PropertiesManager {
     hasPendingProperty(key: string): boolean;
 }
 
-// @alpha (undocumented)
+// @alpha @deprecated (undocumented)
 export enum PropertiesRollback {
     None = 0,
     Rollback = 1
@@ -659,7 +664,7 @@ export type PropertySet = MapLike<any>;
 
 // @alpha
 export interface ReferencePosition {
-    // (undocumented)
+    // @deprecated (undocumented)
     addProperties(newProps: PropertySet): void;
     getOffset(): number;
     getSegment(): ISegment | undefined;
@@ -765,7 +770,7 @@ export type SlidingPreference = (typeof SlidingPreference)[keyof typeof SlidingP
 
 // @alpha (undocumented)
 export class TextSegment extends BaseSegment {
-    constructor(text: string);
+    constructor(text: string, props?: PropertySet);
     // (undocumented)
     append(segment: ISegment): void;
     // (undocumented)

diff --git a/packages/dds/merge-tree/src/attributionPolicy.ts b/packages/dds/merge-tree/src/attributionPolicy.ts
@@ -136,25 +136,22 @@ function createPropertyTrackingMergeTreeCallbacks(
 ): AttributionCallbacks {
 	const toTrack = propNames.map((entry) => ({ propName: entry, channelName: entry }));
 	const attributeAnnotateOnSegments = (
+		isLocal: boolean,
 		deltaSegments: IMergeTreeSegmentDelta[],
-		{ op, sequencedMessage }: IMergeTreeDeltaOpArgs,
+		{ op }: IMergeTreeDeltaOpArgs,
 		key: AttributionKey,
 	): void => {
-		for (const { segment } of deltaSegments) {
+		for (const { segment, propertyDeltas } of deltaSegments) {
 			for (const { propName, channelName } of toTrack) {
 				const shouldAttributeInsert =
 					op.type === MergeTreeDeltaType.INSERT &&
 					segment.properties?.[propName] !== undefined;
 
-				const isLocal = sequencedMessage === undefined;
 				const shouldAttributeAnnotate =
 					op.type === MergeTreeDeltaType.ANNOTATE &&
 					// Only attribute annotations which change the tracked property
 					op.props[propName] !== undefined &&
-					// Local changes to the tracked property always take effect
-					(isLocal ||
-						// Acked changes only take effect if there isn't a pending local change
-						(!isLocal && !segment.propertyManager?.hasPendingProperty(propName)));
+					(isLocal || (propertyDeltas !== undefined && propName in propertyDeltas));
 
 				if (shouldAttributeInsert || shouldAttributeAnnotate) {
 					segment.attribution?.update(
@@ -170,6 +167,7 @@ function createPropertyTrackingMergeTreeCallbacks(
 			const { op, sequencedMessage } = opArgs;
 			if (op.type === MergeTreeDeltaType.ANNOTATE || op.type === MergeTreeDeltaType.INSERT) {
 				attributeAnnotateOnSegments(
+					sequencedMessage === undefined,
 					deltaSegments,
 					opArgs,
 					getAttributionKey(client, sequencedMessage),
@@ -179,6 +177,7 @@ function createPropertyTrackingMergeTreeCallbacks(
 		maintenance: ({ deltaSegments, operation }, opArgs, client): void => {
 			if (operation === MergeTreeMaintenanceType.ACKNOWLEDGED && opArgs !== undefined) {
 				attributeAnnotateOnSegments(
+					true,
 					deltaSegments,
 					opArgs,
 					getAttributionKey(client, opArgs.sequencedMessage),

diff --git a/packages/dds/merge-tree/src/localReference.ts b/packages/dds/merge-tree/src/localReference.ts
@@ -70,6 +70,13 @@ export interface LocalReferencePosition extends ReferencePosition {
 	 * special segments representing the position before or after the tree
 	 */
 	readonly canSlideToEndpoint?: boolean;
+
+	/**
+	 * @param newProps - Properties to add to this reference.
+	 * @remarks Note that merge-tree does not broadcast changes to other clients. It is up to the consumer
+	 * to ensure broadcast happens if that is desired.
+	 */
+	addProperties(newProps: PropertySet): void;
 }
 
 /**
@@ -125,7 +132,7 @@ class LocalReference implements LocalReferencePosition {
 		this.offset = offset;
 	}
 
-	public isLeaf(): boolean {
+	public isLeaf(): this is ISegment {
 		return false;
 	}
 

diff --git a/packages/dds/merge-tree/src/mergeTree.ts b/packages/dds/merge-tree/src/mergeTree.ts
@@ -85,6 +85,7 @@ import {
 	refHasTileLabel,
 	refTypeIncludesFlag,
 } from "./referencePositions.js";
+// eslint-disable-next-line import/no-deprecated
 import { PropertiesRollback } from "./segmentPropertiesManager.js";
 import { endpointPosAndSide, type SequencePlace } from "./sequencePlace.js";
 import { zamboniSegments } from "./zamboni.js";
@@ -1879,6 +1880,7 @@ export class MergeTree {
 		clientId: number,
 		seq: number,
 		opArgs: IMergeTreeDeltaOpArgs,
+		// eslint-disable-next-line import/no-deprecated
 		rollback: PropertiesRollback = PropertiesRollback.None,
 	): void {
 		this.ensureIntervalBoundary(start, refSeq, clientId);
@@ -2301,6 +2303,7 @@ export class MergeTree {
 						this.collabWindow.clientId,
 						UniversalSequenceNumber,
 						{ op: annotateOp },
+						// eslint-disable-next-line import/no-deprecated
 						PropertiesRollback.Rollback,
 					);
 					i++;