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

EntityResolver interface #1336

Conversation

rubencagnie-toast
Copy link

📝 Description

The EntityResolver class uses GlobalScope.future to dispatch the load for the entity resolving. It is easy to accidentally create resource or memory leaks when GlobalScope is used.

This PR abstracts out the actual entity resolving logic as an interface. The logic has moved to a resolve method, that can be called from any class that implements this interface.

The EntityResolver can be injected into the FederatedSchemaGeneratorHooks class, in case this needs to have a different way of being called. There is a default implementation (GlobalScopeEntityResolver), which keeps the current behavior in tact. A convenience constructor is added to maintain the current initialization with an array of FederatedTypeResolver and is using that GlobalScopeEntityResolver to maintain backwards compatibility

The `EntityResolver` class uses `GlobalScope.future` to dispatch the load for the entity resolving. It is easy to accidentally create resource or memory leaks when GlobalScope is used.

This PR abstracts out the actual entity resolving logic as an interface. The logic has moved to a `resolve` method, that can be called from any class that implements this interface.

The EntityResolver can be injected into the `FederatedSchemaGeneratorHooks` class, in case this needs to have a different way of being called. There is a default implementation (`GlobalScopeEntityResolver`), which keeps the current behavior in tact. A convenience `constructor` is added to maintain the current initialization with an array of `FederatedTypeResolver` and is using that `GlobalScopeEntityResolver` to maintain backwards compatibility
@dariuszkuc
Copy link
Collaborator

Hello 👋
Thanks for opening the PR. It's interesting idea but depending on the integration with spring-graphql we might be able to simplify this logic and rely on Spring to propagate all contextual data. AFAIK spring-graphql is targeting Spring '22 for the GA.

@rubencagnie
Copy link

Thanks for the reply @dariuszkuc. We are not using spring, so that suggestion wouldn't apply to us :(

As this would behave the exact same way as before when you don't specify a EntityResolver, would it be possible to get this in? This would drastically simply our workflow as today we use the hooks to achieve the same, but we had to copy over a part of the code, which is obviously not ideal...

@dariuszkuc
Copy link
Collaborator

👋 I think might be a good idea abstract this logic away and while this part does not directly depend on Spring integration, it is still a major change that might be impacted with the upcoming changes due to potential graphql-spring integration. I'll try to block some time this week to look into how those integrations might look like to better understand the execution model there.

@rubencagnie-toast
Copy link
Author

That would be great, @dariuszkuc

Overall, I think it would be great if this framework has a base interface of defining the coroutine contexts that the resolvers are executed in. Then you can add the specific implementation depending on what framework you run in.

Thanks for looking into this and let me know if I can be of any help

@rubencagnie-toast
Copy link
Author

@dariuszkuc - friendly poke to see if there is any status change on this...

@dariuszkuc
Copy link
Collaborator

dariuszkuc commented Jan 21, 2022

👋 I'm updating the way that we handle context to rely on the new GraphQLContextMap to propagate the scope information. Unfortunately it is a breaking change so will start cutting new alpha version as well.

High level logic

  1. users can provide custom context map through a factory
  2. this map can contain an entry for custom coroutine context (e.g. setting up MDC, thread locals etc)
  3. graphql server (before executing the query) creates supervisor scope using the current context + custom one from the user and stores it in the context map
  4. data fetchers/resolvers will then attempt to retrieve the scope from the context (if not available fallback to global scope) and use it to execute suspending functions

This is similar to what spring-graphql project does and should provide easy interop in the future. I should have a PR ready soon.

dariuszkuc pushed a commit to dariuszkuc/graphql-kotlin that referenced this pull request Feb 1, 2022
📝 Description

Attempts to standardize how we can propagate the coroutine/reactor context created by the server when processing incoming requests to the data fetchers that would resolve the underlying fields.

`GraphQLContext` map was introduced in `graphql-java` 17 as means to standardize usage of GraphQL context (previously it could be any object). We can store the original GraphQL request context in the map and then use it from within the data fetchers.

This is a breaking change as we are changing signatures of `GraphQLContextFactory` and `FunctionDataFetcher`.

🔗 Related Issues

* ExpediaGroup#1336
* ExpediaGroup#1318
* ExpediaGroup#1300
* ExpediaGroup#1257
dariuszkuc added a commit that referenced this pull request Feb 2, 2022
📝 Description

Attempts to standardize how we can propagate the coroutine/reactor context created by the server when processing incoming requests to the data fetchers that would resolve the underlying fields.

`GraphQLContext` map was introduced in `graphql-java` 17 as means to standardize usage of GraphQL context (previously it could be any object). We can store the original GraphQL request context in the map and then use it from within the data fetchers.

This is a breaking change as we are changing signatures of `GraphQLContextFactory` and `FunctionDataFetcher`.

🔗 Related Issues

* #1336
* #1318
* #1300
* #1257
@dariuszkuc
Copy link
Collaborator

Superseded by #1349

@dariuszkuc dariuszkuc closed this Feb 2, 2022
dariuszkuc added a commit to dariuszkuc/graphql-kotlin that referenced this pull request Feb 8, 2022
…oup#1349)

📝 Description

Attempts to standardize how we can propagate the coroutine/reactor context created by the server when processing incoming requests to the data fetchers that would resolve the underlying fields.

`GraphQLContext` map was introduced in `graphql-java` 17 as means to standardize usage of GraphQL context (previously it could be any object). We can store the original GraphQL request context in the map and then use it from within the data fetchers.

This is a breaking change as we are changing signatures of `GraphQLContextFactory` and `FunctionDataFetcher`.

🔗 Related Issues

* ExpediaGroup#1336
* ExpediaGroup#1318
* ExpediaGroup#1300
* ExpediaGroup#1257
dariuszkuc added a commit to dariuszkuc/graphql-kotlin that referenced this pull request Aug 5, 2022
…oup#1349)

📝 Description

Attempts to standardize how we can propagate the coroutine/reactor context created by the server when processing incoming requests to the data fetchers that would resolve the underlying fields.

`GraphQLContext` map was introduced in `graphql-java` 17 as means to standardize usage of GraphQL context (previously it could be any object). We can store the original GraphQL request context in the map and then use it from within the data fetchers.

This is a breaking change as we are changing signatures of `GraphQLContextFactory` and `FunctionDataFetcher`.

🔗 Related Issues

* ExpediaGroup#1336
* ExpediaGroup#1318
* ExpediaGroup#1300
* ExpediaGroup#1257
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants