-
-
Notifications
You must be signed in to change notification settings - Fork 677
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
Conversation
@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. |
@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. |
@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 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 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. |
This reverts commit cc94f97.
9053a66
to
c8a316b
Compare
@syonthomas 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. |
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 🚀