- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 140
Description
GraphQL spec section 7.1 describes a 3rd response field called extensions
that is intended to be used for custom data in addition to payload data and error responses. This is often used for metadata related to the query response such as performance tracing. Apollo GraphQL implements this on their server via an extensions middleware. We should probably follow a similar pattern here but we will need to support passing the extensions data to the client in the core in order to support middleware like that.
https://graphql.github.io/graphql-spec/June2018/#sec-Response-Format
The response map may also contain an entry with key extensions. This entry, if set, must have a map as its value. This entry is reserved for implementors to extend the protocol however they see fit, and hence there are no additional restrictions on its contents.
Activity
Cito commentedon Apr 29, 2019
Sounds interesting, but it is still unclear to me how this would be used by middleware.
Could provide a minimal example of such a middleware that shows how it is supposed to work and that can be used as a test/demo?
nikordaris commentedon Apr 29, 2019
Here is probably the most popular extension middleware i've seen. https://github.com/apollographql/apollo-tracing
The idea is you have middleware that can inject additional metadata about each resolver into the
extensions
field. Apollo decided to make this middleware different from normal resolver middleware because it was needing to inject data intoextensions
instead of the field resolver.Here is an example extension middleware for performance tracing in JS: https://github.com/apollographql/apollo-server/blob/master/packages/apollo-tracing/src/index.ts
And here is the extensions package for the framework: https://github.com/apollographql/apollo-server/blob/master/packages/graphql-extensions/src/index.ts
I'm not an expert on how Apollo implemented their middleware for extensions but it seems to be a popular approach given the other language support.
nikordaris commentedon Apr 29, 2019
I don't think we need to necessarily implement the middleware for it in this lib, just support to inject the extensions data. It would be on the graphql server to provide a way to inject extensions into the response
Cito commentedon Apr 29, 2019
Right. The resolver-based middleware that graphql-core currently supports is something different.
Cito commentedon May 2, 2019
@nikordaris - what do you think should be concretely done in graphql-core-next?
Should we add a field
extensions
to theExecutionResult
? Note that this wouldn't be unproblematic sinceExecutionResult
has been designed as a named tuple which would then have 3 instead of 2 elements. This could break code likedata, errors = result
.Or should we leave it to server implementations to define their own
ExtendedExecusionResult
?Maybe simply like this:
nikordaris commentedon May 3, 2019
Given
extensions
is part of the graphql spec I think it should be in core. It's hard to say what should be done further in core. I haven't looked through the code yet. But I would wager it will need to have some hooks in the resolver execution which is likely in core.nikordaris commentedon May 3, 2019
I have a PR in graphql-core for this but in hindsight I don't think I like the approach I took. I think we should do some kind of middleware pattern instead of allowing the return of
ExecutionResult
in resolvers. But I think the handing of the ExecutionResult object itself might be applicable in core-next. #205Cito commentedon May 3, 2019
Actually I think it should be in GraphQL.js which is the reference implementation for the spec by Facebook. So far the ExecutionResult there has only data and error fields. My preferred way is to try getting things implemented or solved in GraphQL.js first, and initiate a discussion there, getting some feedback and insight from their perspective, instead of trying to solve things here for Python only in our own ways. Maybe you would like to do that?
As explained, I see the divergence from GraphQL.js with concern. We already have added the resolver middleware in Python, and I have also, as I remember only now, added a custom ExecutionContext class like in graphql-core - you can pass it as a parameter when executing queries. Maybe you could also make use of that?
Changing ExecutionResult from a simple named 2-tuple to a non-tuple object or adding another field to the named tuple is easy, but would be a breaking change and reduce the simplicty of that object. I'd like to avoid that if possible.
nikordaris commentedon May 3, 2019
Ya that is a fair point. Looking through the repo now I see Apollo has docs describing how they extend the graphqljs
ExecutionResult
https://github.com/apollographql/apollo-server/blob/master/docs/source/features/schema-transforms.md#transformWhile I disagree with graphqljs excluding core support for it given it is described in the spec, it is the standard source of truth for implementing the spec so following their lead is definitely a good idea. We will just need to make sure
graphql-server-core
extendsExecutionResult
and adds support for injecting extensions.Cito commentedon May 3, 2019
I'm closing this for now. We can still reopen if we find it makes sense to change something in core itself as well, not only in server-core.