feat(dds): do not opaquely encode handles in ops (microsoft#19242)

Update DDSes to not opaquely encode handles as strings, but rather pass
around actual objects.


[AB#6382](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/6382)

experimental/dds/attributable-map/api-report/attributable-map.api.md

@@ -96,9 +96,9 @@ export interface IValueChanged {
 
 // @internal
 export class LocalValueMaker {
-    constructor(serializer: IFluidSerializer);
+    constructor();
     fromInMemory(value: unknown): ILocalValue;
-    fromSerializable(serializable: ISerializableValue): ILocalValue;
+    fromSerializable(serializable: ISerializableValue, serializer: IFluidSerializer, bind: IFluidHandle): ILocalValue;
 }
 
 // @internal @sealed

experimental/dds/attributable-map/src/localValues.ts

@@ -114,29 +114,34 @@ export class PlainLocalValue implements ILocalValue {
 export class LocalValueMaker {
 	/**
 	 * Create a new LocalValueMaker.
-	 * @param serializer - The serializer to serialize / parse handles.
 	 */
-	public constructor(private readonly serializer: IFluidSerializer) {}
+	public constructor() {}
 
 	/**
 	 * Create a new local value from an incoming serialized value.
 	 * @param serializable - The serializable value to make local
 	 */
-	// eslint-disable-next-line import/no-deprecated
-	public fromSerializable(serializable: ISerializableValue): ILocalValue {
+	public fromSerializable(
+		// eslint-disable-next-line import/no-deprecated
+		serializable: ISerializableValue,
+		serializer: IFluidSerializer,
+		bind: IFluidHandle,
+	): ILocalValue {
 		// Migrate from old shared value to handles
 		if (serializable.type === ValueType[ValueType.Shared]) {
 			serializable.type = ValueType[ValueType.Plain];
 			const handle: ISerializedHandle = {
 				type: "__fluid_handle__",
 				url: serializable.value as string,
 			};
-			serializable.value = handle;
-		}
 
-		const translatedValue: unknown = parseHandles(serializable.value, this.serializer);
+			// NOTE: here we require the use of `parseHandles` because the roundtrip
+			// through a string is necessary to resolve the absolute path of
+			// legacy handles (`ValueType.Shared`)
+			serializable.value = serializer.encode(parseHandles(handle, serializer), bind);
+		}
 
-		return new PlainLocalValue(translatedValue);
+		return new PlainLocalValue(serializable.value);
 	}
 
 	/**

experimental/dds/attributable-map/src/mapKernel.ts

@@ -4,7 +4,7 @@
  */
 
 import { IFluidHandle } from "@fluidframework/core-interfaces";
-import { IFluidSerializer, ValueType } from "@fluidframework/shared-object-base";
+import { IFluidSerializer, ValueType, bindHandles } from "@fluidframework/shared-object-base";
 import { TypedEventEmitter } from "@fluid-internal/client-utils";
 import { assert, unreachableCase } from "@fluidframework/core-utils";
 import { ISequencedDocumentMessage } from "@fluidframework/protocol-definitions";
@@ -193,7 +193,7 @@ export class AttributableMapKernel {
 		private readonly isAttached: () => boolean,
 		private readonly eventEmitter: TypedEventEmitter<ISharedMapEvents>,
 	) {
-		this.localValueMaker = new LocalValueMaker(serializer);
+		this.localValueMaker = new LocalValueMaker();
 		this.messageHandlers = this.getMessageHandlers();
 		this.attribution = new Map();
 	}
@@ -305,21 +305,24 @@ export class AttributableMapKernel {
 
 		// Create a local value and serialize it.
 		const localValue = this.localValueMaker.fromInMemory(value);
-		const serializableValue = makeSerializable(localValue, this.serializer, this.handle);
 
 		// Set the value and attribution locally.
 		const previousValue = this.setCore(key, localValue, true);
 		this.setAttribution(key);
 
 		// If we are not attached, don't submit the op.
 		if (!this.isAttached()) {
+			// this is necessary to bind the potential handles in the value
+			// to this DDS, as we do not walk the object normally unless we
+			// are attached
+			bindHandles(localValue.value, this.serializer, this.handle);
 			return;
 		}
 
 		const op: IMapSetOperation = {
 			key,
 			type: "set",
-			value: serializableValue,
+			value: { type: localValue.type, value: localValue.value as unknown },
 		};
 		this.submitMapKeyMessage(op, previousValue);
 	}
@@ -452,7 +455,9 @@ export class AttributableMapKernel {
 	 * @param json - A JSON string containing serialized map data
 	 */
 	public populateFromSerializable(json: IMapDataObjectSerializable): void {
-		for (const [key, serializable] of Object.entries(json)) {
+		for (const [key, serializable] of Object.entries(
+			this.serializer.decode(json) as IMapDataObjectSerializable,
+		)) {
 			const localValue = {
 				key,
 				value: this.makeLocal(key, serializable),
@@ -474,10 +479,6 @@ export class AttributableMapKernel {
 		}
 	}
 
-	public populate(json: string): void {
-		this.populateFromSerializable(JSON.parse(json) as IMapDataObjectSerializable);
-	}
-
 	/**
 	 * Submit the given op if a handler is registered.
 	 * @param op - The operation to attempt to submit
@@ -506,7 +507,11 @@ export class AttributableMapKernel {
 				break;
 			}
 			case "set": {
-				this.set(op.key, this.localValueMaker.fromSerializable(op.value).value);
+				this.set(
+					op.key,
+					this.localValueMaker.fromSerializable(op.value, this.serializer, this.handle)
+						.value,
+				);
 				break;
 			}
 			default:
@@ -669,7 +674,11 @@ export class AttributableMapKernel {
 			serializable.type === ValueType[ValueType.Plain] ||
 			serializable.type === ValueType[ValueType.Shared]
 		) {
-			return this.localValueMaker.fromSerializable(serializable);
+			return this.localValueMaker.fromSerializable(
+				serializable,
+				this.serializer,
+				this.handle,
+			);
 		} else {
 			throw new Error("Unknown local value type");
 		}

experimental/dds/tree/api-report/experimental-tree.api.md

@@ -983,7 +983,7 @@ export class SharedTreeFactory implements IChannelFactory {
     get attributes(): IChannelAttributes;
     create(runtime: IFluidDataStoreRuntime, id: string): SharedTree;
     // (undocumented)
-    load(runtime: IFluidDataStoreRuntime, id: string, services: IChannelServices, _channelAttributes: Readonly<IChannelAttributes>): Promise<IChannel>;
+    load(runtime: IFluidDataStoreRuntime, id: string, services: IChannelServices, _channelAttributes: Readonly<IChannelAttributes>): Promise<SharedTree>;
     // (undocumented)
     static Type: string;
     // (undocumented)

experimental/dds/tree/src/SharedTree.ts

@@ -12,7 +12,6 @@ import {
 	IChannelFactory,
 	IChannelAttributes,
 	IChannelServices,
-	IChannel,
 } from '@fluidframework/datastore-definitions';
 import { AttachState } from '@fluidframework/container-definitions';
 import {
@@ -246,7 +245,7 @@ export class SharedTreeFactory implements IChannelFactory {
 		id: string,
 		services: IChannelServices,
 		_channelAttributes: Readonly<IChannelAttributes>
-	): Promise<IChannel> {
+	): Promise<SharedTree> {
 		const sharedTree = this.createSharedTree(runtime, id);
 		await sharedTree.load(services);
 		return sharedTree;
@@ -1067,28 +1066,14 @@ export class SharedTree extends SharedObject<ISharedTreeEvents> implements NodeI
 		// TODO:Type Safety: Improve type safety around op sending/parsing (e.g. discriminated union over version field somehow)
 		switch (op.version) {
 			case WriteFormat.v0_0_2:
-				return this.encoder_0_0_2.decodeEditOp(op, this.encodeSemiSerializedEdit.bind(this), this);
+				return this.encoder_0_0_2.decodeEditOp(op, (x) => x, this);
 			case WriteFormat.v0_1_1:
-				return this.encoder_0_1_1.decodeEditOp(
-					op,
-					this.encodeSemiSerializedEdit.bind(this),
-					this.idNormalizer,
-					this.interner
-				);
+				return this.encoder_0_1_1.decodeEditOp(op, (x) => x, this.idNormalizer, this.interner);
 			default:
 				fail('Unknown op version');
 		}
 	}
 
-	private encodeSemiSerializedEdit<T>(semiSerializedEdit: Edit<T>): Edit<T> {
-		// semiSerializedEdit may have handles which have been replaced by `serializer.encode`.
-		// Since there is no API to un-replace them except via parse, re-stringify the edit, then parse it.
-		// Stringify using JSON, not IFluidSerializer since OPs use JSON directly.
-		// TODO:Performance:#48025: Avoid this serialization round trip.
-		const encodedEdit: Edit<T> = this.serializer.parse(JSON.stringify(semiSerializedEdit));
-		return encodedEdit;
-	}
-
 	private processSequencedEdit(edit: Edit<ChangeInternal>, message: ISequencedDocumentMessage): void {
 		const { id: editId } = edit;
 		const wasLocalEdit = this.editLog.isLocalEdit(editId);
@@ -1381,13 +1366,13 @@ export class SharedTree extends SharedObject<ISharedTreeEvents> implements NodeI
 		if (this.isAttached()) {
 			switch (this.writeFormat) {
 				case WriteFormat.v0_0_2:
-					this.submitOp(this.encoder_0_0_2.encodeEditOp(edit, this.serializeEdit.bind(this), this));
+					this.submitOp(this.encoder_0_0_2.encodeEditOp(edit, (x) => x, this));
 					break;
 				case WriteFormat.v0_1_1:
 					this.submitOp(
 						this.encoder_0_1_1.encodeEditOp(
 							edit,
-							this.serializeEdit.bind(this),
+							(x) => x,
 							this.idCompressor.takeNextCreationRange(),
 							this.idNormalizer,
 							this.interner
@@ -1400,10 +1385,6 @@ export class SharedTree extends SharedObject<ISharedTreeEvents> implements NodeI
 		}
 	}
 
-	private serializeEdit<TChange>(preparedEdit: Edit<TChange>): Edit<TChange> {
-		return this.serializer.encode(preparedEdit, this.handle) as Edit<TChange>;
-	}
-
 	/** A type-safe `submitLocalMessage` wrapper to enforce op format */
 	private submitOp(content: SharedTreeOp | SharedTreeOp_0_0_2, localOpMetadata: unknown = undefined): void {
 		assert(
@@ -1491,7 +1472,7 @@ export class SharedTree extends SharedObject<ISharedTreeEvents> implements NodeI
 
 								stashedEdit = this.encoder_0_1_1.decodeEditOp(
 									sharedTreeOp,
-									this.encodeSemiSerializedEdit.bind(this),
+									(x) => x,
 									normalizer,
 									this.interner
 								);

experimental/dds/tree/src/migration-shim/migrationShim.ts

@@ -199,12 +199,12 @@ export class MigrationShim extends EventEmitterWithErrorHandling<IMigrationEvent
 			this.runtime.attachState === AttachState.Detached
 				? new NoDeltasChannelServices(services)
 				: this.generateShimServicesOnce(services);
-		this._legacyTree = (await this.legacyTreeFactory.load(
+		this._legacyTree = await this.legacyTreeFactory.load(
 			this.runtime,
 			this.id,
 			shimServices,
 			this.legacyTreeFactory.attributes
-		)) as LegacySharedTree;
+		);
 	}
 	public create(): void {
 		this._legacyTree = this.legacyTreeFactory.create(this.runtime, this.id);

packages/dds/cell/api-report/cell.api.md

@@ -7,6 +7,7 @@
 import { AttributionKey } from '@fluidframework/runtime-definitions';
 import { IChannelAttributes } from '@fluidframework/datastore-definitions';
 import { IChannelFactory } from '@fluidframework/datastore-definitions';
+import { IChannelServices } from '@fluidframework/datastore-definitions';
 import { IChannelStorageService } from '@fluidframework/datastore-definitions';
 import { IFluidDataStoreRuntime } from '@fluidframework/datastore-definitions';
 import { IFluidSerializer } from '@fluidframework/shared-object-base';
@@ -17,6 +18,22 @@ import { ISummaryTreeWithStats } from '@fluidframework/runtime-definitions';
 import { Serializable } from '@fluidframework/datastore-definitions';
 import { SharedObject } from '@fluidframework/shared-object-base';
 
+// @internal @sealed
+export class CellFactory implements IChannelFactory {
+    // (undocumented)
+    static readonly Attributes: IChannelAttributes;
+    // (undocumented)
+    get attributes(): IChannelAttributes;
+    // (undocumented)
+    create(document: IFluidDataStoreRuntime, id: string): ISharedCell;
+    // (undocumented)
+    load(runtime: IFluidDataStoreRuntime, id: string, services: IChannelServices, attributes: IChannelAttributes): Promise<ISharedCell>;
+    // (undocumented)
+    static readonly Type = "https://graph.microsoft.com/types/cell";
+    // (undocumented)
+    get type(): string;
+}
+
 // @internal
 export interface ICellAttributionOptions {
     // (undocumented)

packages/dds/cell/src/cell.ts

@@ -137,11 +137,6 @@ export class SharedCell<T = any>
 	 * {@inheritDoc ISharedCell.set}
 	 */
 	public set(value: Serializable<T>): void {
-		// Serialize the value if required.
-		const operationValue: ICellValue = {
-			value: this.serializer.encode(value, this.handle),
-		};
-
 		// Set the value locally.
 		const previousValue = this.setCore(value);
 		this.setAttribution();
@@ -151,6 +146,10 @@ export class SharedCell<T = any>
 			return;
 		}
 
+		const operationValue: ICellValue = {
+			value,
+		};
+
 		const op: ISetCellOperation = {
 			type: "setCell",
 			value: operationValue,
@@ -227,7 +226,8 @@ export class SharedCell<T = any>
 	protected async loadCore(storage: IChannelStorageService): Promise<void> {
 		const content = await readAndParse<ICellValue>(storage, snapshotFileName);
 
-		this.data = this.decode(content);
+		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+		this.data = this.serializer.decode(content.value);
 		this.attribution = content.attribution;
 	}
 
@@ -251,7 +251,7 @@ export class SharedCell<T = any>
 	private applyInnerOp(content: ICellOperation): Serializable<T> | undefined {
 		switch (content.type) {
 			case "setCell": {
-				return this.setCore(this.decode(content.value));
+				return this.setCore(content.value.value as Serializable<T>);
 			}
 
 			case "deleteCell": {
@@ -323,11 +323,6 @@ export class SharedCell<T = any>
 		return previousLocalValue;
 	}
 
-	private decode(cellValue: ICellValue): Serializable<T> {
-		const value = cellValue.value;
-		return this.serializer.decode(value) as Serializable<T>;
-	}
-
 	private createLocalOpMetadata(
 		op: ICellOperation,
 		previousValue?: Serializable<T>,
@@ -352,7 +347,7 @@ export class SharedCell<T = any>
 				break;
 			}
 			case "setCell": {
-				this.set(this.decode(cellContent.value));
+				this.set(cellContent.value.value as Serializable<T>);
 				break;
 			}
 			default: {

packages/dds/cell/src/cellFactory.ts

@@ -17,6 +17,8 @@ import { pkgVersion } from "./packageVersion";
  * {@link @fluidframework/datastore-definitions#IChannelFactory} for {@link ISharedCell}.
  *
  * @sealed
+ *
+ * @internal
  */
 export class CellFactory implements IChannelFactory {
 	/**

packages/dds/cell/src/index.ts

@@ -10,6 +10,7 @@
  */
 
 export { SharedCell } from "./cell";
+export { CellFactory } from "./cellFactory";
 export type {
 	ISharedCell,
 	ISharedCellEvents,

packages/dds/map/api-report/map.api.md

@@ -191,9 +191,9 @@ export interface IValueChanged {
 
 // @alpha
 export class LocalValueMaker {
-    constructor(serializer: IFluidSerializer);
+    constructor();
     fromInMemory(value: unknown): ILocalValue;
-    fromSerializable(serializable: ISerializableValue): ILocalValue;
+    fromSerializable(serializable: ISerializableValue, serializer: IFluidSerializer, bind: IFluidHandle): ILocalValue;
 }
 
 // @alpha @sealed