ExecutionResult missing extensions field about graphql-core HOT 10 CLOSED

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?

from graphql-core.

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.

from graphql-core.

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

from graphql-core.

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

from graphql-core.

@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

from graphql-core.

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.

from graphql-core.

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

from graphql-core.

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.

from graphql-core.

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.

from graphql-core.

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.

from graphql-core.