didResolveField hooks are executed in a dangling promise and may receive the wrong result

[sachindshinde]

While trying to add some code to the didResolveField handler of ApolloServerPluginUsageReporting, I noticed some issues:

• It looks like didResolveField is executed in a dangling promise created in whenResultIsFinished(), so there's no way for the plugin to wait on didResolveField handlers to finish executing.
• In the case where result is an array of elements where at least one element is a promise, the code in whenResultIsFinished() will pass an undefined result to the handler if at least one promise rejects. In reality, if the type inside the list is nullable (e.g. [String]), then the response will contain a result list, but with nulls for the rejected promises (e.g. ["foo", null]). In my particular use case, I need the actual result.
• It looks like whenResultIsFinished() is trying to handle list cases, but it doesn't do it fully, e.g. a promise that resolves to a list of promises, or nested list types (e.g. [[String]]).

In terms of what the solution would be, two thoughts come to mind:

• The simplest option would be to just pass the promise-or-value returned by the execution of fieldResolver() inside wrapField() directly to the didResolveField handler. The user has to manually unwrap the promise-or-value according to GraphQL spec (really just adapting the code in completeValueCatchingError() in graphql-js and other functions further down the call stack). But it gives the user the most flexibility, and it's more performant in the event that the user doesn't need to resolve the entirety of the result. The downside is that this would be a backwards-incompatible change, and that doing the unwrapping can be hairy.
• The more complex option would be to have Apollo Server adapt the code in completeValueCatchingError() and the other functions it ends up calling, and use that to compute the result up until subfields begin executing. Basically in the copy-pasted version of completeObjectValue(), you'd leave out the call to collectAndExecuteSubfields() and just return the source object. (You'd need to make some changes around error handling as well, since those functions normally just add them to the execution context.) Similar to the first case, the result would be conveyed via a promise-or-value passed to the didResolveField handler. This is probably closest to what a user imagines when they think of "result" in this context, requires the least work from their end, and can be backwards compatible (I say "can" because right now didResolveField accepts a single error instead of an array, but that could be worked around e.g. with a new argument to didResolveField). This has the downside in that it may perform a lot of unneeded work (e.g. serializing scalars), and duplicate a lot of data in memory (e.g. for lengthy arrays). In my case, I'm just looking to get a set of the enum values that are in the result, so I don't need to duplicate the entire result in-memory, and I don't care about non-enum results. Another downside is that this option assumes the user cares specifically about the result as it appears in the GraphQL response and not about the result as returned by the resolver, though this can be easily remedied through a new argument to didResolveField e.g. rawResult/resultBeforeCompletion.

[glasser]

The best fix here would be to extend graphql-js to allow a non-hacky way of observing resolver calls.