From 036523e91329c5ba5dc2e1a78cfa8b32afd87789 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 23 May 2025 14:36:21 -0700 Subject: [PATCH 01/10] Name collisions from structurally named schema now error --- .changeset/social-bears-deny.md | 23 +++++++++++ .../tree/src/simple-tree/api/schemaFactory.ts | 28 +++++++++++-- .../simple-tree/api/schemaFactory.spec.ts | 41 +++++++++++++++++++ 3 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 .changeset/social-bears-deny.md diff --git a/.changeset/social-bears-deny.md b/.changeset/social-bears-deny.md new file mode 100644 index 000000000000..e9a6879312f0 --- /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 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. +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..402b12f48d95 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,25 @@ export class SchemaFactory< return out; } + 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[]), + ); + 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..188e9a417932 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.array([Child2, SchemaFactory.null]); + }, + validateUsageError(/collision/), + ); + }); }); // kind based narrowing example From c690e9490eb7448af0aa45093301c58cd1bd9fe3 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 23 May 2025 14:48:39 -0700 Subject: [PATCH 02/10] Update .changeset/social-bears-deny.md --- .changeset/social-bears-deny.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/social-bears-deny.md b/.changeset/social-bears-deny.md index e9a6879312f0..a3b99a30b699 100644 --- a/.changeset/social-bears-deny.md +++ b/.changeset/social-bears-deny.md @@ -5,7 +5,7 @@ --- 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 as they are not used together in the same tree. +It is legal to have multiple [TreeNodeSchema](https://fluidframework.com/docs/api/fluid-framework/treenodeschema-typealias) with the same name 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. Previously doing this would not error, and instead return the first structurally named schema with that name. Now this case throws an informative error: From 718bc3d301f1dfb1a12663d5ead965c4c358bacb Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 23 May 2025 15:58:01 -0700 Subject: [PATCH 03/10] Update packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 188e9a417932..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 @@ -1115,7 +1115,7 @@ describe("schemaFactory", () => { assert.equal(factory.map([SchemaFactory.null, Child1]), a); assert.throws( () => { - factory.array([Child2, SchemaFactory.null]); + factory.map([Child2, SchemaFactory.null]); }, validateUsageError(/collision/), ); From fea4ab0cff0988018dfab27649205adc6d80ecd6 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 23 May 2025 15:59:52 -0700 Subject: [PATCH 04/10] Apply suggestions from code review Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com> --- packages/dds/tree/src/simple-tree/api/schemaFactory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts index 402b12f48d95..ec2f7932abd8 100644 --- a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts +++ b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts @@ -994,7 +994,7 @@ export class SchemaFactory< 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.`, + `Structurally named schema collision: two schema named "${fullName}" were defined with different input schema.`, ); } return structural; From a3fd11b6742abce756ae13f04137a5ecfff18052 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 23 May 2025 16:00:36 -0700 Subject: [PATCH 05/10] Update .changeset/social-bears-deny.md Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com> --- .changeset/social-bears-deny.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/social-bears-deny.md b/.changeset/social-bears-deny.md index a3b99a30b699..97af0009bc07 100644 --- a/.changeset/social-bears-deny.md +++ b/.changeset/social-bears-deny.md @@ -5,7 +5,7 @@ --- 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 as they are not used together in the same tree. +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. Previously doing this would not error, and instead return the first structurally named schema with that name. Now this case throws an informative error: From e839c60d5ddb729b6a447531df439ce1a76039af Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 23 May 2025 16:01:37 -0700 Subject: [PATCH 06/10] Update packages/dds/tree/src/simple-tree/api/schemaFactory.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- packages/dds/tree/src/simple-tree/api/schemaFactory.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts index ec2f7932abd8..fa7fc61de4a5 100644 --- a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts +++ b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts @@ -981,6 +981,15 @@ export class SchemaFactory< return out; } + /** + * Retrieves or creates a structural {@link TreeNodeSchema} with the specified name and types. + * + * @param fullName - The unique 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 {@link UsageError} If a schema with the same name exists but has different input types. + */ private getStructuralType( fullName: string, types: TreeNodeSchema | readonly TreeNodeSchema[], From ac42833f985931c85a699ffe0a73888c742e7903 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 23 May 2025 16:02:44 -0700 Subject: [PATCH 07/10] Update message --- .changeset/social-bears-deny.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/social-bears-deny.md b/.changeset/social-bears-deny.md index 97af0009bc07..e601c9d33888 100644 --- a/.changeset/social-bears-deny.md +++ b/.changeset/social-bears-deny.md @@ -18,6 +18,6 @@ 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." +// "Structurally named schema collision: two schema named "Array<["Child"]>" were defined with different input schema." const b = factory.array(Child2); ``` From 9019773b4e1a73ecafb21e6c9a77babfba592373 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 23 May 2025 16:03:35 -0700 Subject: [PATCH 08/10] Update .changeset/social-bears-deny.md Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com> --- .changeset/social-bears-deny.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/social-bears-deny.md b/.changeset/social-bears-deny.md index e601c9d33888..14406799eccc 100644 --- a/.changeset/social-bears-deny.md +++ b/.changeset/social-bears-deny.md @@ -6,7 +6,7 @@ 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. +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: From 38e857d0f316451ada52f788e5ed97612064fa29 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 23 May 2025 16:05:33 -0700 Subject: [PATCH 09/10] Add a comment --- packages/dds/tree/src/simple-tree/api/schemaFactory.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts index fa7fc61de4a5..1946ce2b4359 100644 --- a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts +++ b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts @@ -1000,6 +1000,7 @@ export class SchemaFactory< 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( From 498144d06e915656a9f5f9eab4c7683406d8fabf Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 27 May 2025 09:29:59 -0700 Subject: [PATCH 10/10] Remove invalid link suggested by copilot, and correct details of doc comment. --- packages/dds/tree/src/simple-tree/api/schemaFactory.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts index 1946ce2b4359..674fbfcc1f6f 100644 --- a/packages/dds/tree/src/simple-tree/api/schemaFactory.ts +++ b/packages/dds/tree/src/simple-tree/api/schemaFactory.ts @@ -984,11 +984,11 @@ export class SchemaFactory< /** * Retrieves or creates a structural {@link TreeNodeSchema} with the specified name and types. * - * @param fullName - The unique name for the structural schema. + * @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 {@link UsageError} If a schema with the same name exists but has different input 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,