-
Notifications
You must be signed in to change notification settings - Fork 354
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
EntityResolver interface #1336
Conversation
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
Hello 👋 |
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... |
👋 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 |
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 |
@dariuszkuc - friendly poke to see if there is any status change on this... |
👋 I'm updating the way that we handle context to rely on the new High level logic
This is similar to what |
📝 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
📝 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
Superseded by #1349 |
…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
…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
📝 Description
The
EntityResolver
class usesGlobalScope.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 convenienceconstructor
is added to maintain the current initialization with an array ofFederatedTypeResolver
and is using thatGlobalScopeEntityResolver
to maintain backwards compatibility