ExecutionResult missing extensions field

[nikordaris]

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.

[Cito]

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]

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 into extensions 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]

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]

Right. The resolver-based middleware that graphql-core currently supports is something different.

[Cito]

@nikordaris - what do you think should be concretely done in graphql-core-next?

Should we add a field extensions to the ExecutionResult? Note that this wouldn't be unproblematic since ExecutionResult has been designed as a named tuple which would then have 3 instead of 2 elements. This could break code like data, errors = result.

Or should we leave it to server implementations to define their own ExtendedExecusionResult?

Maybe simply like this:

class ExtendedExecutionResult(ExecutionResult):
    extensions: Optional[Dict[str, Any]]

    def __new__(cls, data, errors, extensions):
        self = super().__new__(cls, data, errors)
        self.extensions = extensions
        return self

[nikordaris]

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]

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. #205

[Cito]

Given extensions is part of the graphql spec I think it should be in core.

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]

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#transform

While 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 extends ExecutionResult and adds support for injecting extensions.

[Cito]

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.