Added field fetching method for Expedia #3571
Conversation
|
This needs more tests, especially around the Chained code to ensure we capture that its working ok |
| @@ -496,6 +497,7 @@ Async.CombinedBuilder<FieldValueInfo> getAsyncFieldValueInfo( | |||
| Object fetchedObject = invokeDataFetcher(executionContext, parameters, fieldDef, dataFetchingEnvironment, dataFetcher); | |||
| executionContext.getDataLoaderDispatcherStrategy().fieldFetched(executionContext, parameters, dataFetcher, fetchedObject); | |||
| fetchCtx.onDispatched(); | |||
| fetchCtx.onFetchedValue(fetchedObject); | |||
There was a problem hiding this comment.
context callback now gets the fetched value
|
PR looks good to me, the Chained instrumentation looks ok, as is similar to the other hooks using custom InstrumentationContext. as you mentioned the purpose of this PR is to expose the resolved value from the DataFetcher, if we know that returned is a CompletableFuture or a materialized object we would know when is the right time to dispatch data loaders. More information about our custom data loader dispatch instrumentation: is this instrumentation something that |
…tions to show new methods is called and it delegates ok as well via Legacy one
bbakerman
changed the title
|
was able to verify this PR and our custom data loader dispatching instrumentation is working again. @bbakerman @dondonz i was wondering, if possible, an official RC release, this will help build tools to resolve dependencies properly. |
### 📝 Description waiting for an official release of this PR graphql-java/graphql-java#3571 UPDATE: next graphql-java update will be on July, https://github.com/graphql-java/graphql-java/milestone/69 they helped us releasing from this branch graphql-java/graphql-java#3571 which will unblock us. https://github.com/graphql-java/graphql-java/releases/tag/v22.0
|
@samuelAndalon Thanks for the feedback. We'll include this PR in a special release. |
|
Hi @samuelAndalon we've included this PR in today's v22.1 release. It should be landing in Maven very soon. Here are the release notes: https://github.com/graphql-java/graphql-java/releases/tag/v22.1 |
When we changed InstrumentationContext to no longer take a CF as an argument, the callback to
beginFieldFetchcan no long know about the value that was fetched.Expedia for example used to this know if the value was a SYNC or ASYNC value for this dispatch code.
This adds a new method with a new context class that can be called back with the object that was fetched (it might not be complete if its a CF say)
The default implementation calls back to the old method so that we dont break any one. But should it? v22 is only a few days old - if v22.1 was also breaking for this - the impact would be low - no zero but very low