From 20556ecfb1fd873dfa35eaff76466e7e5d45f13e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Lytek?= Date: Wed, 22 May 2024 15:38:59 +0200 Subject: [PATCH 1/6] Add test to reproduce the issue --- tests/functional/generic-types.ts | 2 +- tests/functional/resolvers.ts | 51 +++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/tests/functional/generic-types.ts b/tests/functional/generic-types.ts index aeae47b87..2831bf078 100644 --- a/tests/functional/generic-types.ts +++ b/tests/functional/generic-types.ts @@ -408,7 +408,7 @@ describe("Generic types", () => { let schema: GraphQLSchema; let schemaIntrospection: IntrospectionSchema; - beforeAll(async () => { + beforeEach(async () => { function Base(TTypeClass: ClassType) { @ObjectType() class BaseClass { diff --git a/tests/functional/resolvers.ts b/tests/functional/resolvers.ts index c4ac43d5f..3b22dacaf 100644 --- a/tests/functional/resolvers.ts +++ b/tests/functional/resolvers.ts @@ -2582,4 +2582,55 @@ describe("Resolvers", () => { expect(dynamicField2).toBeDefined(); }); }); + + describe("Shared generic resolver", () => { + beforeEach(async () => { + getMetadataStorage().clear(); + }); + + it("should handle arguments correctly on multiple buildSchema runs", async () => { + @ObjectType() + class TestResponse { + @Field() + data!: string; + } + + @ArgsType() + class TestArgs { + @Field(() => Int, { defaultValue: 0 }) + testField!: number; + } + + function makeResolverClass() { + @Resolver(() => TestResponse) + abstract class TestResolver { + @Query(() => TestResponse) + async exampleQuery(@Args() args: TestArgs): Promise { + return { + data: `resolver ${args.testField}`, + }; + } + } + + return TestResolver; + } + + @Resolver() + class TestResolver extends makeResolverClass() {} + + const fistSchemaInfo = await getSchemaInfo({ + resolvers: [TestResolver], + }); + + expect(fistSchemaInfo.queryType.fields).toHaveLength(1); + expect(fistSchemaInfo.queryType.fields[0].args).toHaveLength(1); + + const secondSchemaInfo = await getSchemaInfo({ + resolvers: [TestResolver], + }); + + expect(secondSchemaInfo.queryType.fields).toHaveLength(1); + expect(secondSchemaInfo.queryType.fields[0].args).toHaveLength(1); + }); + }); }); From cc94f9798fc9e3e14053d04765eff9d0735100cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Lytek?= Date: Wed, 22 May 2024 15:39:59 +0200 Subject: [PATCH 2/6] Prevent subsequent build of metadata storage --- src/metadata/metadata-storage.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/metadata/metadata-storage.ts b/src/metadata/metadata-storage.ts index 9083f7ec1..b0faa99dc 100644 --- a/src/metadata/metadata-storage.ts +++ b/src/metadata/metadata-storage.ts @@ -80,6 +80,8 @@ export class MetadataStorage { params: ParamMetadata[] = []; + private hasAlreadyBeenBuilt = false; + collectQueryHandlerMetadata(definition: ResolverMetadata) { this.queries.push(definition); } @@ -174,6 +176,12 @@ export class MetadataStorage { } build(options: SchemaGeneratorOptions) { + if (this.hasAlreadyBeenBuilt) { + return; + } + + this.hasAlreadyBeenBuilt = true; + this.classDirectives.reverse(); this.fieldDirectives.reverse(); this.argumentDirectives.reverse(); @@ -214,10 +222,11 @@ export class MetadataStorage { this.argumentDirectives = []; this.classExtensions = []; this.fieldExtensions = []; - this.resolverClasses = []; this.fields = []; this.params = []; + + this.hasAlreadyBeenBuilt = false; } private buildClassMetadata(definitions: ClassMetadata[]) { From ab62434d4af3c625dc30fc9583bc4dfbed3f7404 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Lytek?= Date: Fri, 7 Jun 2024 12:39:11 +0200 Subject: [PATCH 3/6] Add test case for generic resolver and field resolvers --- tests/functional/resolvers.ts | 94 +++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/tests/functional/resolvers.ts b/tests/functional/resolvers.ts index 3b22dacaf..35f90ec1d 100644 --- a/tests/functional/resolvers.ts +++ b/tests/functional/resolvers.ts @@ -2632,5 +2632,99 @@ describe("Resolvers", () => { expect(secondSchemaInfo.queryType.fields).toHaveLength(1); expect(secondSchemaInfo.queryType.fields[0].args).toHaveLength(1); }); + + it("should handle field resolvers correctly on multiple buildSchema runs", async () => { + @ObjectType() + class TestResponse { + @Field() + data!: string; + } + + @ArgsType() + class TestArgs { + @Field(() => Int, { defaultValue: 0 }) + testField!: number; + } + + function makeResolverClass() { + @Resolver(() => TestResponse) + abstract class TestResolver { + @Query(() => TestResponse) + async exampleQuery(@Args() args: TestArgs): Promise { + return { + data: `resolver ${args.testField}`, + }; + } + } + + return TestResolver; + } + + @Resolver(() => TestResponse) + class TestResolver extends makeResolverClass() { + @FieldResolver(() => Boolean, { nullable: false }) + public async exampleFieldResolver(): Promise { + return true; + } + } + + @ObjectType() + class OtherTestResponse { + @Field() + data!: string; + } + + @ArgsType() + class OtherTestArgs { + @Field(() => Int, { defaultValue: 0 }) + testField!: number; + } + + function makeOtherResolverClass() { + @Resolver(() => OtherTestResponse) + abstract class OtherTestResolver { + @Query(() => OtherTestResponse) + async exampleQuery(@Args() args: OtherTestArgs): Promise { + return { + data: `resolver ${args.testField}`, + }; + } + } + + return OtherTestResolver; + } + + @Resolver(() => OtherTestResponse) + class OtherTestResolver extends makeOtherResolverClass() { + @FieldResolver(() => Boolean, { nullable: false }) + public async exampleFieldResolver(): Promise { + return true; + } + } + + const fistSchemaInfo = await getSchemaInfo({ + resolvers: [TestResolver], + }); + + const hasFoundFieldResolverInSchema = fistSchemaInfo.schemaIntrospection.types.some( + type => + type.kind === "OBJECT" && + type.name === "TestResponse" && + type.fields?.some(field => field.name === "exampleFieldResolver"), + ); + expect(hasFoundFieldResolverInSchema).toBeTruthy(); + + const secondSchemaInfo = await getSchemaInfo({ + resolvers: [OtherTestResolver], + }); + + const hasFoundFieldResolverInOtherSchema = secondSchemaInfo.schemaIntrospection.types.some( + type => + type.kind === "OBJECT" && + type.name === "OtherTestResponse" && + type.fields?.some(field => field.name === "exampleFieldResolver"), + ); + expect(hasFoundFieldResolverInOtherSchema).toBeTruthy(); + }); }); }); From 7d372cf2a1fd8e38e457605983c0c1161de3fce5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Lytek?= Date: Fri, 7 Jun 2024 12:40:22 +0200 Subject: [PATCH 4/6] Make a local copy of metadata storage to build it --- src/schema/schema-generator.ts | 46 +++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/src/schema/schema-generator.ts b/src/schema/schema-generator.ts index 56ed7a9c9..ffa11adec 100644 --- a/src/schema/schema-generator.ts +++ b/src/schema/schema-generator.ts @@ -40,6 +40,7 @@ import { import { type InterfaceClassMetadata } from "@/metadata/definitions/interface-class-metadata"; import { type ObjectClassMetadata } from "@/metadata/definitions/object-class-metadata"; import { getMetadataStorage } from "@/metadata/getMetadataStorage"; +import { MetadataStorage } from "@/metadata/metadata-storage"; import { createAdvancedFieldResolver, createBasicFieldResolver, @@ -118,10 +119,15 @@ export abstract class SchemaGenerator { private static usedInterfaceTypes = new Set(); + private static metadataStorage: MetadataStorage; + static generateFromMetadata(options: SchemaGeneratorOptions): GraphQLSchema { + this.metadataStorage = Object.assign(new MetadataStorage(), getMetadataStorage()); + this.metadataStorage.build(options); + this.checkForErrors(options); BuildContext.create(options); - getMetadataStorage().build(options); + this.buildTypesInfo(options.resolvers); const orphanedTypes = options.orphanedTypes ?? []; @@ -152,7 +158,7 @@ export abstract class SchemaGenerator { private static checkForErrors(options: SchemaGeneratorOptions) { ensureInstalledCorrectGraphQLPackage(); - if (getMetadataStorage().authorizedFields.length !== 0 && options.authChecker === undefined) { + if (this.metadataStorage.authorizedFields.length !== 0 && options.authChecker === undefined) { throw new Error( "You need to provide `authChecker` function for `@Authorized` decorator usage!", ); @@ -189,7 +195,7 @@ export abstract class SchemaGenerator { } private static buildTypesInfo(resolvers: Function[]) { - this.unionTypesInfo = getMetadataStorage().unions.map(unionMetadata => { + this.unionTypesInfo = this.metadataStorage.unions.map(unionMetadata => { // use closure to capture values from this selected schema build const unionObjectTypesInfo: ObjectTypeInfo[] = []; // called once after building all `objectTypesInfo` @@ -232,7 +238,7 @@ export abstract class SchemaGenerator { }; }); - this.enumTypesInfo = getMetadataStorage().enums.map(enumMetadata => { + this.enumTypesInfo = this.metadataStorage.enums.map(enumMetadata => { const enumMap = getEnumValuesMap(enumMetadata.enumObj); return { enumObj: enumMetadata.enumObj, @@ -253,7 +259,7 @@ export abstract class SchemaGenerator { }; }); - this.objectTypesInfo = getMetadataStorage().objectTypes.map(objectType => { + this.objectTypesInfo = this.metadataStorage.objectTypes.map(objectType => { const objectSuperClass = Object.getPrototypeOf(objectType.target); const hasExtended = objectSuperClass.prototype !== undefined; const getSuperClassType = () => { @@ -300,7 +306,7 @@ export abstract class SchemaGenerator { // support for implicitly implementing interfaces // get fields from interfaces definitions if (objectType.interfaceClasses) { - const implementedInterfaces = getMetadataStorage().interfaceTypes.filter(it => + const implementedInterfaces = this.metadataStorage.interfaceTypes.filter(it => objectType.interfaceClasses!.includes(it.target), ); implementedInterfaces.forEach(it => { @@ -312,7 +318,7 @@ export abstract class SchemaGenerator { let fields = fieldsMetadata.reduce>( (fieldsMap, field) => { - const { fieldResolvers } = getMetadataStorage(); + const { fieldResolvers } = this.metadataStorage; const filteredFieldResolversMetadata = fieldResolvers.filter( it => it.kind === "internal" || resolvers.includes(it.target), ); @@ -369,7 +375,7 @@ export abstract class SchemaGenerator { }; }); - this.interfaceTypesInfo = getMetadataStorage().interfaceTypes.map( + this.interfaceTypesInfo = this.metadataStorage.interfaceTypes.map( interfaceType => { const interfaceSuperClass = Object.getPrototypeOf(interfaceType.target); const hasExtended = interfaceSuperClass.prototype !== undefined; @@ -381,8 +387,8 @@ export abstract class SchemaGenerator { }; // fetch ahead the subset of object types that implements this interface - const implementingObjectTypesTargets = getMetadataStorage() - .objectTypes.filter( + const implementingObjectTypesTargets = this.metadataStorage.objectTypes + .filter( objectType => objectType.interfaceClasses && objectType.interfaceClasses.includes(interfaceType.target), @@ -419,7 +425,7 @@ export abstract class SchemaGenerator { // support for implicitly implementing interfaces // get fields from interfaces definitions if (interfaceType.interfaceClasses) { - const implementedInterfacesMetadata = getMetadataStorage().interfaceTypes.filter( + const implementedInterfacesMetadata = this.metadataStorage.interfaceTypes.filter( it => interfaceType.interfaceClasses!.includes(it.target), ); implementedInterfacesMetadata.forEach(it => { @@ -431,7 +437,7 @@ export abstract class SchemaGenerator { let fields = fieldsMetadata!.reduce>( (fieldsMap, field) => { - const fieldResolverMetadata = getMetadataStorage().fieldResolvers.find( + const fieldResolverMetadata = this.metadataStorage.fieldResolvers.find( resolver => resolver.getObjectType!() === field.target && resolver.methodName === field.name, @@ -490,7 +496,7 @@ export abstract class SchemaGenerator { }, ); - this.inputTypesInfo = getMetadataStorage().inputTypes.map(inputType => { + this.inputTypesInfo = this.metadataStorage.inputTypes.map(inputType => { const objectSuperClass = Object.getPrototypeOf(inputType.target); const getSuperClassType = () => { const superClassTypeInfo = this.inputTypesInfo.find( @@ -549,7 +555,7 @@ export abstract class SchemaGenerator { } private static buildRootQueryType(resolvers: Function[]): GraphQLObjectType { - const queriesHandlers = this.filterHandlersByResolvers(getMetadataStorage().queries, resolvers); + const queriesHandlers = this.filterHandlersByResolvers(this.metadataStorage.queries, resolvers); return new GraphQLObjectType({ name: "Query", @@ -559,7 +565,7 @@ export abstract class SchemaGenerator { private static buildRootMutationType(resolvers: Function[]): GraphQLObjectType | undefined { const mutationsHandlers = this.filterHandlersByResolvers( - getMetadataStorage().mutations, + this.metadataStorage.mutations, resolvers, ); if (mutationsHandlers.length === 0) { @@ -574,7 +580,7 @@ export abstract class SchemaGenerator { private static buildRootSubscriptionType(resolvers: Function[]): GraphQLObjectType | undefined { const subscriptionsHandlers = this.filterHandlersByResolvers( - getMetadataStorage().subscriptions, + this.metadataStorage.subscriptions, resolvers, ); if (subscriptionsHandlers.length === 0) { @@ -735,8 +741,8 @@ export abstract class SchemaGenerator { input.index, input.name, ); - const argDirectives = getMetadataStorage() - .argumentDirectives.filter( + const argDirectives = this.metadataStorage.argumentDirectives + .filter( it => it.target === target && it.fieldName === propertyName && @@ -752,7 +758,7 @@ export abstract class SchemaGenerator { astNode: getInputValueDefinitionNode(input.name, type, argDirectives), }; } else if (param.kind === "args") { - const argumentType = getMetadataStorage().argumentTypes.find( + const argumentType = this.metadataStorage.argumentTypes.find( it => it.target === param.getType(), ); if (!argumentType) { @@ -771,7 +777,7 @@ export abstract class SchemaGenerator { inheritanceChainClasses.push(superClass); } for (const argsTypeClass of inheritanceChainClasses.reverse()) { - const inheritedArgumentType = getMetadataStorage().argumentTypes.find( + const inheritedArgumentType = this.metadataStorage.argumentTypes.find( it => it.target === argsTypeClass, ); if (inheritedArgumentType) { From c8a316b430b0c7003ba2edcc4caf6a9e0a738db6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Lytek?= Date: Fri, 7 Jun 2024 12:40:40 +0200 Subject: [PATCH 5/6] Revert "Prevent subsequent build of metadata storage" This reverts commit cc94f9798fc9e3e14053d04765eff9d0735100cb. --- src/metadata/metadata-storage.ts | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/src/metadata/metadata-storage.ts b/src/metadata/metadata-storage.ts index b0faa99dc..9083f7ec1 100644 --- a/src/metadata/metadata-storage.ts +++ b/src/metadata/metadata-storage.ts @@ -80,8 +80,6 @@ export class MetadataStorage { params: ParamMetadata[] = []; - private hasAlreadyBeenBuilt = false; - collectQueryHandlerMetadata(definition: ResolverMetadata) { this.queries.push(definition); } @@ -176,12 +174,6 @@ export class MetadataStorage { } build(options: SchemaGeneratorOptions) { - if (this.hasAlreadyBeenBuilt) { - return; - } - - this.hasAlreadyBeenBuilt = true; - this.classDirectives.reverse(); this.fieldDirectives.reverse(); this.argumentDirectives.reverse(); @@ -222,11 +214,10 @@ export class MetadataStorage { this.argumentDirectives = []; this.classExtensions = []; this.fieldExtensions = []; + this.resolverClasses = []; this.fields = []; this.params = []; - - this.hasAlreadyBeenBuilt = false; } private buildClassMetadata(definitions: ClassMetadata[]) { From a8c6a8b05fde636ae7d789007a8a8dbda1edeec2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Lytek?= Date: Fri, 7 Jun 2024 12:47:04 +0200 Subject: [PATCH 6/6] Add changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc414b4eb..1a97b06bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ - make possible creating custom decorators on resolver class level - `createResolverClassMiddlewareDecorator` - support registering custom arg decorator via `createParameterDecorator` and its second argument `CustomParameterOptions` - `arg` (#1325) +## Fixes + +- properly build multiple schemas with generic resolvers, args and field resolvers (#1321) + ### Others - **Breaking Change**: update `graphql-scalars` peer dependency to `^1.23.0`