Revert change to remove encoding of handles out of the DDS (#20974)

Reverts #19242 due to a bug where duplicate attach ops are sent because
of the change to encoding handles.

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

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

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

@@ -114,34 +114,29 @@ export class PlainLocalValue implements ILocalValue {
 export class LocalValueMaker {
 	/**
 	 * Create a new LocalValueMaker.
+	 * @param serializer - The serializer to serialize / parse handles.
 	 */
-	public constructor() {}
+	public constructor(private readonly serializer: IFluidSerializer) {}
 
 	/**
 	 * Create a new local value from an incoming serialized value.
 	 * @param serializable - The serializable value to make local
 	 */
-	public fromSerializable(
-		// eslint-disable-next-line import/no-deprecated
-		serializable: ISerializableValue,
-		serializer: IFluidSerializer,
-		bind: IFluidHandle,
-	): ILocalValue {
+	// eslint-disable-next-line import/no-deprecated
+	public fromSerializable(serializable: ISerializableValue): 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,
 			};
-
-			// 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);
+			serializable.value = handle;
 		}
 
-		return new PlainLocalValue(serializable.value);
+		const translatedValue: unknown = parseHandles(serializable.value, this.serializer);
+
+		return new PlainLocalValue(translatedValue);
 	}
 
 	/**

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

@@ -4,7 +4,7 @@
  */
 
 import { IFluidHandle } from "@fluidframework/core-interfaces";
-import { IFluidSerializer, ValueType, bindHandles } from "@fluidframework/shared-object-base";
+import { IFluidSerializer, ValueType } 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();
+		this.localValueMaker = new LocalValueMaker(serializer);
 		this.messageHandlers = this.getMessageHandlers();
 		this.attribution = new Map();
 	}
@@ -305,24 +305,21 @@ 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: { type: localValue.type, value: localValue.value as unknown },
+			value: serializableValue,
 		};
 		this.submitMapKeyMessage(op, previousValue);
 	}
@@ -455,9 +452,7 @@ export class AttributableMapKernel {
 	 * @param json - A JSON string containing serialized map data
 	 */
 	public populateFromSerializable(json: IMapDataObjectSerializable): void {
-		for (const [key, serializable] of Object.entries(
-			this.serializer.decode(json) as IMapDataObjectSerializable,
-		)) {
+		for (const [key, serializable] of Object.entries(json)) {
 			const localValue = {
 				key,
 				value: this.makeLocal(key, serializable),
@@ -479,6 +474,10 @@ 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
@@ -507,11 +506,7 @@ export class AttributableMapKernel {
 				break;
 			}
 			case "set": {
-				this.set(
-					op.key,
-					this.localValueMaker.fromSerializable(op.value, this.serializer, this.handle)
-						.value,
-				);
+				this.set(op.key, this.localValueMaker.fromSerializable(op.value).value);
 				break;
 			}
 			default:
@@ -674,11 +669,7 @@ export class AttributableMapKernel {
 			serializable.type === ValueType[ValueType.Plain] ||
 			serializable.type === ValueType[ValueType.Shared]
 		) {
-			return this.localValueMaker.fromSerializable(
-				serializable,
-				this.serializer,
-				this.handle,
-			);
+			return this.localValueMaker.fromSerializable(serializable);
 		} else {
 			throw new Error("Unknown local value type");
 		}

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

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

experimental/dds/tree/src/SharedTree.ts

@@ -12,6 +12,7 @@ import {
 	IChannelFactory,
 	IChannelAttributes,
 	IChannelServices,
+	IChannel,
 } from '@fluidframework/datastore-definitions';
 import { AttachState } from '@fluidframework/container-definitions';
 import {
@@ -245,7 +246,7 @@ export class SharedTreeFactory implements IChannelFactory {
 		id: string,
 		services: IChannelServices,
 		_channelAttributes: Readonly<IChannelAttributes>
-	): Promise<SharedTree> {
+	): Promise<IChannel> {
 		const sharedTree = this.createSharedTree(runtime, id);
 		await sharedTree.load(services);
 		return sharedTree;
@@ -1066,14 +1067,28 @@ 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, (x) => x, this);
+				return this.encoder_0_0_2.decodeEditOp(op, this.encodeSemiSerializedEdit.bind(this), this);
 			case WriteFormat.v0_1_1:
-				return this.encoder_0_1_1.decodeEditOp(op, (x) => x, this.idNormalizer, this.interner);
+				return this.encoder_0_1_1.decodeEditOp(
+					op,
+					this.encodeSemiSerializedEdit.bind(this),
+					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);
@@ -1366,13 +1381,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, (x) => x, this));
+					this.submitOp(this.encoder_0_0_2.encodeEditOp(edit, this.serializeEdit.bind(this), this));
 					break;
 				case WriteFormat.v0_1_1:
 					this.submitOp(
 						this.encoder_0_1_1.encodeEditOp(
 							edit,
-							(x) => x,
+							this.serializeEdit.bind(this),
 							this.idCompressor.takeNextCreationRange(),
 							this.idNormalizer,
 							this.interner
@@ -1385,6 +1400,10 @@ 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(
@@ -1472,7 +1491,7 @@ export class SharedTree extends SharedObject<ISharedTreeEvents> implements NodeI
 
 								stashedEdit = this.encoder_0_1_1.decodeEditOp(
 									sharedTreeOp,
-									(x) => x,
+									this.encodeSemiSerializedEdit.bind(this),
 									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/src/cell.ts

@@ -136,6 +136,11 @@ 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();
@@ -145,10 +150,6 @@ export class SharedCell<T = any>
 			return;
 		}
 
-		const operationValue: ICellValue = {
-			value,
-		};
-
 		const op: ISetCellOperation = {
 			type: "setCell",
 			value: operationValue,
@@ -225,8 +226,7 @@ export class SharedCell<T = any>
 	protected async loadCore(storage: IChannelStorageService): Promise<void> {
 		const content = await readAndParse<ICellValue>(storage, snapshotFileName);
 
-		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-		this.data = this.serializer.decode(content.value);
+		this.data = this.decode(content);
 		this.attribution = content.attribution;
 	}
 
@@ -250,7 +250,7 @@ export class SharedCell<T = any>
 	private applyInnerOp(content: ICellOperation): Serializable<T> | undefined {
 		switch (content.type) {
 			case "setCell": {
-				return this.setCore(content.value.value as Serializable<T>);
+				return this.setCore(this.decode(content.value));
 			}
 
 			case "deleteCell": {
@@ -322,6 +322,11 @@ 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>,
@@ -346,7 +351,7 @@ export class SharedCell<T = any>
 				break;
 			}
 			case "setCell": {
-				this.set(cellContent.value.value as Serializable<T>);
+				this.set(this.decode(cellContent.value));
 				break;
 			}
 			default: {

packages/dds/cell/src/cellFactory.ts

@@ -15,10 +15,8 @@ import { pkgVersion } from "./packageVersion.js";
 
 /**
  * {@link @fluidframework/datastore-definitions#IChannelFactory} for {@link ISharedCell}.
- *
- * @sealed
- *
  * @internal
+ * @sealed
  */
 export class CellFactory implements IChannelFactory {
 	/**
@@ -51,6 +49,7 @@ export class CellFactory implements IChannelFactory {
 
 	/**
 	 * {@inheritDoc @fluidframework/datastore-definitions#IChannelFactory.load}
+	 * @internal
 	 */
 	public async load(
 		runtime: IFluidDataStoreRuntime,
@@ -65,6 +64,7 @@ export class CellFactory implements IChannelFactory {
 
 	/**
 	 * {@inheritDoc @fluidframework/datastore-definitions#IChannelFactory.create}
+	 * @internal
 	 */
 	public create(document: IFluidDataStoreRuntime, id: string): ISharedCell {
 		const cell = new SharedCell(id, document, this.attributes);

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

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