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

Refactor GraphQlSource builder to separate programmatic vs SDL way of create GraphQLSchema #312

Closed
ruben-garciapariente opened this issue Mar 1, 2022 · 6 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@ruben-garciapariente
Copy link

We would like to be able to define the schema programmatically instead of through an SDL file.
In our first attempt we have used a GraphQlSourceBuilderCustomizer and used the schemaFactory method, but we noticed that the DefaultTypeResolver is not registered correctly and it is necessary to have an SDL file even if it is empty.

Could you consider adding a method to the DefaultGraphQlSourceBuilder that would allow to directly insert a GraphQLSchema?

Many thanks & regards,
Rubén

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 1, 2022
@rstoyanchev rstoyanchev self-assigned this Mar 3, 2022
@rstoyanchev
Copy link
Contributor

Generally, SDL is what we aim to support support and that's what GraphQlSource.Builder is built for. I'm not sure we should aim to support the programmatic approach in the same builder. I would be concerned about the full implications of what that entails, what Builder options apply to which, but also what else might not work in combination.

If we wanted to support programmatic, it would maybe be a different builder, or perhaps the two builders could share a common base builder, but again I have not looked in detail and it is not planned currently.

For now, consider bypassing the builder and creating a GraphQlSource through your code. If there are issues with that approach we could consider small changes to make it possible at least.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Mar 8, 2022
@ruben-garciapariente
Copy link
Author

ruben-garciapariente commented Mar 10, 2022

Thank you very much for your suggestion. I tried to create my own GraphQlSource. I did a quick test and got it working, but I had to make a number of ugly changes in spring-graphql:

  1. To inject the DataFecher in the SDL you are using RuntimeWiring.Builder, but to be able to do this without SDL you need GraphQLCodeRegistry.Builder (at least I didn't know how to do it otherwise).
    To solve this I have modified the AnnotatedControllerConfigurer class and created a new configure(GraphQLCodeRegistry.Builder builder) method with the code practically identical to configure(RuntimeWiring.Builder builder). In addition to creating an interface equivalent to RuntimeWiringConfigurer but for GraphQLCodeRegistry.Builder
    If we want to use QueryDSL we would have the same problem. To solve it I created a class equivalent to AutoRegistrationRuntimeWiringConfigurer but for GraphQLCodeRegistry and I created a new static method in QuerydslDataFetcher equivalent to autoRegistrationConfigurer.

  2. To avoid repeating a lot of code I made public:

  • ContextDataFetcherDecorator and its TYPE_VISITOR attribute.
  • ExceptionResolversExceptionHandler

In short, I haven't seen a simple way to make it work with small changes :_______(

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Mar 10, 2022
@rstoyanchev
Copy link
Contributor

Thanks for sharing the outcome. For the RuntimeWiringConfigurer, I'm wondering if it would work to create your own RuntimeWiring.newRuntimeWiring() then use AnnotatedControllerConfigurer to accumulate the registrations, and in the end build and get the CodeRegistry from RuntimeWiring?

Overall I'm wondering if we could open things up a bit with the GraphQlSource.Builder implementation and perhaps have a base class that's agnostic to SDL vs programmatic schema creation.

@ruben-garciapariente
Copy link
Author

Thank you very much for your advice. I have tried using the CodeRegistry obtained from RuntimeWiring, but it doesn't seem like a viable alternative.

When the dataFetcher are registered in RuntimeWiring.Builder, they are not registered in GraphQLCodeRegistry. I check this because when get the CodeRegistry from runtimeWiring.getCodeRegistry(), the dataFetcherMap attribute is empty.
On the other hand, if I register the dataFecher directly in GraphQLCodeRegistry.Builder, the dataFetcherMap object does contain the registered dataFecher.

Instead, maybe it is possible to do it the other way around (correctly generate a RuntimeWiring from a GraphQLCodeRegistry).

If this is really as I have seen, could it make sense to change the public void configure(RuntimeWiring.Builder builder builder) method to configure(GraphQLCodeRegistry.Builder builder builder)?

@rstoyanchev
Copy link
Contributor

You're right that RuntimeWiring builder doesn't populate the CodeRegistry, but it gives you access to the data fetcher registrations, and it would be trivial to add those to the CodeRegistry. That's what SchemaGeneratorHelper does later on from the RuntimeWiring.

I don't think there is any benefit to change AnnotatedControllerConfigurer to populate the CodeRegistry directly.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Apr 19, 2022
@rstoyanchev rstoyanchev added this to the 1.0.0-RC1 milestone Apr 19, 2022
@rstoyanchev rstoyanchev changed the title Define a schema using java instead of SDL Refactor GraphQlSource builder to separate programmatic vs SDL way of create GraphQLSchema Apr 19, 2022
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Apr 19, 2022
rstoyanchev added a commit that referenced this issue Apr 19, 2022
Separate more clearly the SDL builder options from other common options
independent of how GraphQLSchema is created.

See gh-312
bclozel added a commit to spring-projects/spring-boot that referenced this issue Apr 19, 2022
@rstoyanchev
Copy link
Contributor

@ruben-garciapariente I've made a few changes as follows.

Firstly, refactored the GraphQlSource builder a little bit, to create a base Builder with common options, irrespective of how GraphQLSchema is created, along with a sub-builder that assists with the SDL way, and an additional static factory builder method that accepts an externally built GraphQLSchema instance.

Along with that I added an extra method to AnnotatedControllerConfigurer that accepts a GraphQLCodeRegistry.Bulider to which to apply the registrations. Hopefully this changes will be helpful. Do let us know how it works out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants