apollographql / apollographql/apollo-server

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

Open
#4,667 1 comment 0 reactions 0 assignees View on GitHub
🔌 plugins
Dominant language
TypeScript
Stars
14k
Forks
2k
Avg merge
2d 14h
Merged PRs (30d)
2

Description

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`.

Contributor guide

Open the contributing guide

Research direction

Start by tracing whenResultIsFinished() and wrapField(), then compare their behavior with fieldResolver(), completeValueCatchingError(), completeObjectValue(), and collectAndExecuteSubfields(). The work needs an agreed result and error contract for didResolveField, including awaiting handlers and handling rejected, nullable, nested, and promise-containing lists, followed by focused tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
graphql, typescript
Domain
backend-api-design
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.