diff --git a/.changeset/social-bears-deny.md b/.changeset/social-bears-deny.md new file mode 100644 index 000000000000..14406799eccc --- /dev/null +++ b/.changeset/social-bears-deny.md @@ -0,0 +1,23 @@ +--- +"@fluidframework/tree": minor +"fluid-framework": minor +"__section": tree +--- +Name collisions from structurally named schema now error + +It is legal to have multiple [TreeNodeSchema](https://fluidframework.com/docs/api/fluid-framework/treenodeschema-typealias) with the same name so long as they are not used together in the same tree. +Using different schema with the same name when building otherwise identical [structurally named](https://fluidframework.com/docs/api/fluid-framework/schemafactory-class#schemafactory-remarks) in the same [SchemaFactory](https://fluidframework.com/docs/api/fluid-framework/schemafactory-class) is not valid, however. +Previously doing this would not error, and instead return the first structurally named schema with that name. +Now this case throws an informative error: + +```typescript +const factory = new SchemaFactory(undefined); +class Child1 extends factory.object("Child", {}) {} +class Child2 extends factory.object("Child", {}) {} + +const a = factory.map(Child1); + +// Throws a UsageError with the message: +// "Structurally named schema collision: two schema named "Array<["Child"]>" were defined with different input schema." +const b = factory.array(Child2); +``` diff --git a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts index b6642607234f..674fbfcc1f6f 100644 --- a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts +++ b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts @@ -6,6 +6,7 @@ import type { IFluidHandle } from "@fluidframework/core-interfaces"; import { assert, unreachableCase } from "@fluidframework/core-utils/internal"; import { isFluidHandle } from "@fluidframework/runtime-utils/internal"; +import { UsageError } from "@fluidframework/telemetry-utils/internal"; import type { TreeValue } from "../../core/index.js"; import type { NodeIdentifierManager } from "../../feature-libraries/index.js"; @@ -14,6 +15,7 @@ import type { NodeIdentifierManager } from "../../feature-libraries/index.js"; import type { TreeAlpha } from "../../shared-tree/index.js"; import { type RestrictiveStringRecord, + compareSets, getOrCreate, isReadonlyArray, } from "../../util/index.js"; @@ -60,6 +62,7 @@ import { type ImplicitAnnotatedAllowedTypes, type UnannotateImplicitAllowedTypes, type UnannotateSchemaRecord, + normalizeAllowedTypes, } from "../schemaTypes.js"; import { createFieldSchemaUnsafe } from "./schemaFactoryRecursive.js"; @@ -766,9 +769,9 @@ export class SchemaFactory< if (allowedTypes === undefined) { const types = nameOrAllowedTypes as (T & TreeNodeSchema) | readonly TreeNodeSchema[]; const fullName = structuralName("Map", types); - return getOrCreate( - this.structuralTypes, + return this.getStructuralType( fullName, + types, () => this.namedMap( fullName as TName, @@ -954,7 +957,7 @@ export class SchemaFactory< if (allowedTypes === undefined) { const types = nameOrAllowedTypes as (T & TreeNodeSchema) | readonly TreeNodeSchema[]; const fullName = structuralName("Array", types); - return getOrCreate(this.structuralTypes, fullName, () => + return this.getStructuralType(fullName, types, () => this.namedArray(fullName, nameOrAllowedTypes as T, false, true), ) as TreeNodeSchemaClass< ScopedSchemaName, @@ -978,6 +981,35 @@ export class SchemaFactory< return out; } + /** + * Retrieves or creates a structural {@link TreeNodeSchema} with the specified name and types. + * + * @param fullName - The name for the structural schema. + * @param types - The input schema(s) used to define the structural schema. + * @param builder - A function that builds the schema if it does not already exist. + * @returns The structural {@link TreeNodeSchema} associated with the given name and types. + * @throws `UsageError` if a schema structurally named schema with the same name is cached in `structuralTypes` but had different input types. + */ + private getStructuralType( + fullName: string, + types: TreeNodeSchema | readonly TreeNodeSchema[], + builder: () => TreeNodeSchema, + ): TreeNodeSchema { + const structural = getOrCreate(this.structuralTypes, fullName, builder); + const inputTypes = new Set(normalizeAllowedTypes(types)); + const outputTypes = new Set( + normalizeAllowedTypes(structural.info as TreeNodeSchema | readonly TreeNodeSchema[]), + ); + // If our cached value had a different set of types then were requested, the user must have caused a collision. + const same = compareSets({ a: inputTypes, b: outputTypes }); + if (!same) { + throw new UsageError( + `Structurally named schema collision: two schema named "${fullName}" were defined with different input schema.`, + ); + } + return structural; + } + /** * Define a {@link TreeNodeSchema} for a {@link (TreeArrayNode:interface)}. * diff --git a/packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts b/packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts index 78d76de9a9bc..37efedf55559 100644 --- a/packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts @@ -1079,6 +1079,47 @@ describe("schemaFactory", () => { assert.deepEqual(getKeys(arr), [0]); assert.deepEqual(getKeys(mapNode), ["x"]); }); + + it("structural type collision: single type", () => { + const factory = new SchemaFactory(""); + class Child1 extends factory.object("Child", {}) {} + class Child2 extends factory.object("Child", {}) {} + + const a = factory.array(Child1); + // No error: this type is the same as the one above. + assert.equal(factory.array(Child1), a); + + // Error: this type is different from the one above. + assert.throws( + () => { + factory.array(Child2); + }, + validateUsageError(/collision/), + ); + + assert.equal(factory.array([Child1]), a); + assert.throws( + () => { + factory.array([Child2]); + }, + validateUsageError(/collision/), + ); + }); + + it("structural type collision: multi type", () => { + const factory = new SchemaFactory(""); + class Child1 extends factory.object("Child", {}) {} + class Child2 extends factory.object("Child", {}) {} + + const a = factory.map([Child1, SchemaFactory.null]); + assert.equal(factory.map([SchemaFactory.null, Child1]), a); + assert.throws( + () => { + factory.map([Child2, SchemaFactory.null]); + }, + validateUsageError(/collision/), + ); + }); }); // kind based narrowing example