Move validation of schema change codec up to tree change codec (#26817)

## Description

Move validation of schema change codec up to tree change codec.

This is incremental progress toward a state where we consistently use
codec versions only in places where we include the version in the actual
encoded data and dispatch based on that.

For places where we do not do explicit versioning of data, we want to
use alternatively clearly distinct schemes modularizing the code for
handling different data and different versions of the encodings for that
data. This case can be strongly typed and use normal direct code
dependencies to pull in the relevant logic.
As part of this, moving all the schema validation logic further up the
stack toward where the version dispatching happens (which is what this
PR is doing) helps is get toward a point where we can snapshot the
schema for each version of our codecs, and use that to help validate the
format not changed. This approach also reduces the need to do schema
validation to fewer places, eventually to only within
ClientVersionDispatchingCodecBuilder once fully applied.

Author: CraigMacomber

packages/dds/tree/src/codec/codec.ts

@@ -202,6 +202,27 @@ export interface IJsonCodec<
 	encodedSchema?: TAnySchema;
 }
 
+/**
+ * Part of a codec.
+ * @remarks
+ * Encode and decode logic and schema for some chunk of data.
+ * Can be composed into larger codecs, and eventually versioned at the top level using
+ * {@link VersionDispatchingCodecBuilder}.
+ *
+ * This portion of a codec is not responsible for managing versioning or validation of the data against the schema.
+ */
+export interface JsonCodecPart<TDecoded, TEncodedSchema extends TAnySchema, TContext = void>
+	extends IEncoder<TDecoded, Static<TEncodedSchema>, TContext>,
+		IDecoder<TDecoded, Static<TEncodedSchema>, TContext> {
+	/**
+	 * TypeBox schema which describes the encoded format for this chunk of data.
+	 * @remarks
+	 * The user of this codec can use this to build its own larger schema,
+	 * until eventually it is provided to the {@link VersionDispatchingCodecBuilder}.
+	 */
+	encodedSchema: TEncodedSchema;
+}
+
 /**
  * Type erase the more detailed encoded type from a codec.
  */
@@ -398,6 +419,7 @@ export function withSchemaValidation<
 			}
 			return codec.decode(encoded, context);
 		},
+		encodedSchema: schema,
 	};
 }
 
@@ -529,6 +551,14 @@ export const FluidClientVersion = {
  */
 export const currentVersion: MinimumVersionForCollab = runtimeUtilsCleanedPackageVersion;
 
+/**
+ * TODO:
+ * This needs to be documented.
+ * Its documentation should cover at least the following:
+ * - Is this used for anything other than testing.
+ * - What should be included as children. For example should it include versioned codecs which dispatch base on the min version for collaboration? If so, what version of them should be used?
+ * - What risks does having this mitigate?
+ */
 export interface CodecTree {
 	readonly name: string;
 	readonly version: FormatVersion;

packages/dds/tree/src/codec/index.ts

@@ -28,6 +28,7 @@ export {
 	extractJsonValidator,
 	type CodecName,
 	eraseEncodedType,
+	type JsonCodecPart,
 } from "./codec.js";
 export {
 	DiscriminatedUnionDispatcher,

packages/dds/tree/src/feature-libraries/index.ts

@@ -194,10 +194,8 @@ export { DetachedFieldIndexSummarizer } from "./detachedFieldIndexSummarizer.js"
 
 export {
 	type SchemaChange,
-	makeSchemaChangeCodecs,
+	makeSchemaChangeCodec,
 	EncodedSchemaChange,
-	getCodecTreeForSchemaChangeFormat,
-	SchemaChangeFormatVersion,
 } from "./schema-edits/index.js";
 
 export { makeMitigatedChangeFamily } from "./mitigatedChangeFamily.js";

packages/dds/tree/src/feature-libraries/schema-edits/index.ts

@@ -3,10 +3,6 @@
  * Licensed under the MIT License.
  */
 
-export {
-	makeSchemaChangeCodecs,
-	getCodecTreeForSchemaChangeFormat,
-	SchemaChangeFormatVersion,
-} from "./schemaChangeCodecs.js";
+export { makeSchemaChangeCodec } from "./schemaChangeCodecs.js";
 export type { SchemaChange } from "./schemaChangeTypes.js";
 export { EncodedSchemaChange } from "./schemaChangeFormat.js";

packages/dds/tree/src/feature-libraries/schema-edits/schemaChangeCodecs.ts

@@ -4,73 +4,24 @@
  */
 
 import { assert } from "@fluidframework/core-utils/internal";
-import type { MinimumVersionForCollab } from "@fluidframework/runtime-definitions/internal";
 
-import {
-	type CodecTree,
-	type CodecWriteOptions,
-	type ICodecFamily,
-	type IJsonCodec,
-	makeCodecFamily,
-	withSchemaValidation,
-} from "../../codec/index.js";
-import { strictEnum, type Values } from "../../util/index.js";
+import type { CodecWriteOptions, JsonCodecPart } from "../../codec/index.js";
 import { schemaCodecBuilder } from "../schema-index/index.js";
 
 import { EncodedSchemaChange } from "./schemaChangeFormat.js";
 import type { SchemaChange } from "./schemaChangeTypes.js";
 
 /**
- * Create a family of schema change codecs.
- * @param options - Specifies common codec options, including which `validator` to use.
- * @returns The composed codec family.
- * @remarks
- * Data encoded with this codec is not versioned.
- * Users of this codec must therefore ensure that the decoder always knows which version was used.
- */
-export function makeSchemaChangeCodecs(
-	options: CodecWriteOptions,
-): ICodecFamily<SchemaChange> {
-	// TODO:
-	// Inlining the schema change codec V1 into its parent codec,
-	// removing the use of codec family here.
-	return makeCodecFamily([[SchemaChangeFormatVersion.v1, makeSchemaChangeCodecV1(options)]]);
-}
-
-/**
- * The format version for the schema change.
- * @remarks
- * The SchemaChangeFormat is not explicitly versioned in the data.
- *
- * TODO: Inline this codec's formats into the parent codec that references it, rather than treating this like a versioned codec. See related notes in makeSchemaChangeCodecs.
- */
-export const SchemaChangeFormatVersion = strictEnum("SchemaChangeFormatVersion", {
-	v1: 1,
-});
-export type SchemaChangeFormatVersion = Values<typeof SchemaChangeFormatVersion>;
-
-export function getCodecTreeForSchemaChangeFormat(
-	version: SchemaChangeFormatVersion,
-	clientVersion: MinimumVersionForCollab,
-): CodecTree {
-	return {
-		name: "SchemaChange",
-		version,
-		children: [schemaCodecBuilder.getCodecTree(clientVersion)],
-	};
-}
-
-/**
- * This is independently versioned from the schemaCodec version.
+ * Creates a codec for schema changes.
  * @param options - The codec options.
- * @returns The composed schema change codec.
+ * @returns The composed schema change codec part.
  */
-function makeSchemaChangeCodecV1(
+export function makeSchemaChangeCodec(
 	options: CodecWriteOptions,
-): IJsonCodec<SchemaChange, EncodedSchemaChange> {
+): JsonCodecPart<SchemaChange, typeof EncodedSchemaChange> {
 	const schemaCodec = schemaCodecBuilder.build(options);
-	const schemaChangeCodec: IJsonCodec<SchemaChange, EncodedSchemaChange> = {
-		encode: (schemaChange) => {
+	return {
+		encode: (schemaChange: SchemaChange): EncodedSchemaChange => {
 			assert(
 				!schemaChange.isInverse,
 				0x933 /* Inverse schema changes should never be transmitted */,
@@ -80,7 +31,7 @@ function makeSchemaChangeCodecV1(
 				old: schemaCodec.encode(schemaChange.schema.old),
 			};
 		},
-		decode: (encoded) => {
+		decode: (encoded: EncodedSchemaChange): SchemaChange => {
 			return {
 				schema: {
 					new: schemaCodec.decode(encoded.new),
@@ -91,6 +42,4 @@ function makeSchemaChangeCodecV1(
 		},
 		encodedSchema: EncodedSchemaChange,
 	};
-
-	return withSchemaValidation(EncodedSchemaChange, schemaChangeCodec, options.jsonValidator);
 }

packages/dds/tree/src/shared-tree/sharedTreeChangeCodecs.ts

@@ -12,7 +12,6 @@ import {
 	DiscriminatedUnionDispatcher,
 	type FormatVersion,
 	type ICodecFamily,
-	type ICodecOptions,
 	type IJsonCodec,
 	makeCodecFamily,
 	withSchemaValidation,
@@ -21,12 +20,10 @@ import type { ChangeEncodingContext, TreeStoredSchema } from "../core/index.js";
 import {
 	ModularChangeFormatVersion,
 	type ModularChangeset,
-	type SchemaChange,
-	SchemaChangeFormatVersion,
 	defaultSchemaPolicy,
 	getCodecTreeForModularChangeFormat,
-	getCodecTreeForSchemaChangeFormat,
-	makeSchemaChangeCodecs,
+	makeSchemaChangeCodec,
+	schemaCodecBuilder,
 } from "../feature-libraries/index.js";
 import {
 	strictEnum,
@@ -45,9 +42,6 @@ export function makeSharedTreeChangeCodecFamily(
 	modularChangeCodecFamily: ICodecFamily<ModularChangeset, ChangeEncodingContext>,
 	options: CodecWriteOptions,
 ): ICodecFamily<SharedTreeChange, ChangeEncodingContext> {
-	// TODO: since this is using the SchemaChangeCodec without explicit versioning,
-	// it would probably be better to depend on its format directly without going through the codec family.
-	const schemaChangeCodecs = makeSchemaChangeCodecs(options);
 	const versions: [
 		FormatVersion,
 		IJsonCodec<
@@ -56,22 +50,15 @@ export function makeSharedTreeChangeCodecFamily(
 			EncodedSharedTreeChange,
 			ChangeEncodingContext
 		>,
-	][] = [...dependenciesForChangeFormat.entries()].map(
-		([format, { modularChange, schemaChange }]) => [
-			format,
-			makeSharedTreeChangeCodec(
-				modularChangeCodecFamily.resolve(modularChange),
-				schemaChangeCodecs.resolve(schemaChange),
-				options,
-			),
-		],
-	);
+	][] = [...dependenciesForChangeFormat.entries()].map(([format, { modularChange }]) => [
+		format,
+		makeSharedTreeChangeCodec(modularChangeCodecFamily.resolve(modularChange), options),
+	]);
 	return makeCodecFamily(versions);
 }
 
 interface ChangeFormatDependencies {
 	readonly modularChange: ModularChangeFormatVersion;
-	readonly schemaChange: SchemaChangeFormatVersion;
 }
 
 /**
@@ -117,21 +104,18 @@ export const dependenciesForChangeFormat = new Map<
 		SharedTreeChangeFormatVersion.v3,
 		{
 			modularChange: ModularChangeFormatVersion.v3,
-			schemaChange: SchemaChangeFormatVersion.v1,
 		},
 	],
 	[
 		SharedTreeChangeFormatVersion.v4,
 		{
 			modularChange: ModularChangeFormatVersion.v4,
-			schemaChange: SchemaChangeFormatVersion.v1,
 		},
 	],
 	[
 		SharedTreeChangeFormatVersion.v5,
 		{
 			modularChange: ModularChangeFormatVersion.v5,
-			schemaChange: SchemaChangeFormatVersion.v1,
 		},
 	],
 ]);
@@ -140,14 +124,14 @@ export function getCodecTreeForChangeFormat(
 	version: SharedTreeChangeFormatVersion,
 	clientVersion: MinimumVersionForCollab,
 ): CodecTree {
-	const { modularChange, schemaChange } =
+	const { modularChange } =
 		dependenciesForChangeFormat.get(version) ?? fail(0xc78 /* Unknown change format */);
 	return {
 		name: "SharedTreeChange",
 		version,
 		children: [
 			getCodecTreeForModularChangeFormat(modularChange),
-			getCodecTreeForSchemaChangeFormat(schemaChange, clientVersion),
+			schemaCodecBuilder.getCodecTree(clientVersion),
 		],
 	};
 }
@@ -159,14 +143,14 @@ function makeSharedTreeChangeCodec(
 		JsonCompatibleReadOnly,
 		ChangeEncodingContext
 	>,
-	schemaChangeCodec: IJsonCodec<SchemaChange>,
-	codecOptions: ICodecOptions,
+	codecOptions: CodecWriteOptions,
 ): IJsonCodec<
 	SharedTreeChange,
 	EncodedSharedTreeChange,
 	EncodedSharedTreeChange,
 	ChangeEncodingContext
 > {
+	const schemaChangeCodec = makeSchemaChangeCodec(codecOptions);
 	const decoderLibrary = new DiscriminatedUnionDispatcher<
 		EncodedSharedTreeInnerChange,
 		[context: ChangeEncodingContext],
@@ -187,7 +171,7 @@ function makeSharedTreeChangeCodec(
 	});
 
 	return withSchemaValidation(
-		EncodedSharedTreeChange,
+		EncodedSharedTreeChange(schemaChangeCodec.encodedSchema),
 		{
 			encode: (change, context) => {
 				const changes: EncodedSharedTreeInnerChange[] = [];

packages/dds/tree/src/shared-tree/sharedTreeChangeFormat.ts

@@ -3,17 +3,32 @@
  * Licensed under the MIT License.
  */
 
-import { type Static, Type } from "@sinclair/typebox";
+import { type Static, type TSchema, Type } from "@sinclair/typebox";
 
+import type { EncodedSchemaChange } from "../feature-libraries/index.js";
 import { JsonCompatibleReadOnlySchema } from "../util/index.js";
 
-export const EncodedSharedTreeInnerChange = Type.Object({
-	schema: Type.Optional(JsonCompatibleReadOnlySchema),
-	data: Type.Optional(JsonCompatibleReadOnlySchema),
-});
+// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
+export function EncodedSharedTreeInnerChange<TEncodedSchema extends TSchema>(
+	encodedSchemaSchema: TEncodedSchema,
+) {
+	return Type.Object({
+		schema: Type.Optional(encodedSchemaSchema),
+		data: Type.Optional(JsonCompatibleReadOnlySchema),
+	});
+}
 
-export type EncodedSharedTreeInnerChange = Static<typeof EncodedSharedTreeInnerChange>;
+export type EncodedSharedTreeInnerChange<
+	TEncodedSchema extends TSchema = typeof EncodedSchemaChange,
+> = Static<ReturnType<typeof EncodedSharedTreeInnerChange<TEncodedSchema>>>;
 
-export const EncodedSharedTreeChange = Type.Array(EncodedSharedTreeInnerChange);
+// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
+export function EncodedSharedTreeChange<TEncodedSchema extends TSchema>(
+	encodedSchemaSchema: TEncodedSchema,
+) {
+	return Type.Array(EncodedSharedTreeInnerChange(encodedSchemaSchema));
+}
 
-export type EncodedSharedTreeChange = Static<typeof EncodedSharedTreeChange>;
+export type EncodedSharedTreeChange<
+	TEncodedSchema extends TSchema = typeof EncodedSchemaChange,
+> = Static<ReturnType<typeof EncodedSharedTreeChange<TEncodedSchema>>>;