Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(schema): prevent issues with subsequent schema builds #1698

Merged
merged 6 commits into from
Jun 7, 2024

Conversation

MichalLytek
Copy link
Owner

This PR is another approach (after #1691 by community) to fix an issue related to subsequent schema builds, especially when we have resolvers with inheritance and those resolvers queries/mutations contains args.

The approach to fix this was to just block subsequent metadata storage .build() actions, which were mutating the initial metadata storage values.

Fixes #1321 🚀

@ghost
Copy link

ghost commented May 30, 2024

@MichalLytek , thank you for taking the time to address this issue and for your efforts with this PR. While it successfully resolves the associated test case, this solution does not work in my project.

I use buildSchema with different options. I suspect the issue arises because build now returns early even if other options are provided.

I will investigate further and try to reproduce the issue with a test case.

@MichalLytek
Copy link
Owner Author

@syonthomas Thanks for the feedback. Could you share us the different options you use? Would be nice to add it to this PR as a failing test case.

@ghost
Copy link

ghost commented May 31, 2024

@MichalLytek You are welcome!

I've identified an issue with the handling of field resolvers. Below is a draft of a test case that will fail with the current implementation but will pass with my newest solution. In the second run exampleFieldResolver is missing in OtherTestResolver.

tests/functional/resolver.ts

describe("Shared generic resolver", () => {
   beforeEach(async () => {
     getMetadataStorage().clear();
   });

   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<TestResponse> {
           return {
             data: `resolver ${args.testField}`,
           };
         }
       }

       return TestResolver;
     }

     @Resolver(() => TestResponse)
     class TestResolver extends makeResolverClass() {
       @FieldResolver(() => Boolean, { nullable: false })
       public async exampleFieldResolver(): Promise<boolean> {
         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<OtherTestResponse> {
           return {
             data: `resolver ${args.testField}`,
           };
         }
       }

       return OtherTestResolver;
     }

     @Resolver(() => OtherTestResponse)
     class OtherTestResolver extends makeOtherResolverClass() {
       @FieldResolver(() => Boolean, { nullable: false })
       public async exampleFieldResolver(): Promise<boolean> {
         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();
   });
 });

In my newest solution, I have changed schema-generator.ts to use a copy of MetadataStorage

  static generateFromMetadata(options: SchemaGeneratorOptions): GraphQLSchema {
    this.metadataStorage = Object.assign(new MetadataStorage(), getMetadataStorage());
    this.metadataStorage.build(options);

Feel free to reach out if there's anything else I can assist you with.

@MichalLytek MichalLytek force-pushed the fix-double-schema-build branch from 9053a66 to c8a316b Compare June 7, 2024 10:40
@MichalLytek
Copy link
Owner Author

@syonthomas
Thanks for the feedback 👍

I think this is the only solution for now. #183 could solve this but this is a major rewrite of the core, won't happen anytime soon.
For now let's just use your apporach. It's better than the first PR as we do not restore but make a local copy to work on.
Tests are green so I hope that we won't break anyones production setup 😃

@MichalLytek MichalLytek merged commit 5081422 into master Jun 7, 2024
5 checks passed
@MichalLytek MichalLytek deleted the fix-double-schema-build branch June 7, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix 🐛 🔨 PR that fixes a known bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two mounted apollo servers (resolvers with inhertance) – second one lacks ArgsType in query
1 participant