Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

executeOperation does not invoke context with expected arguments #2886

Closed
atkinchris opened this issue Jun 19, 2019 · 9 comments
Closed

executeOperation does not invoke context with expected arguments #2886

atkinchris opened this issue Jun 19, 2019 · 9 comments

Comments

@atkinchris
Copy link

atkinchris commented Jun 19, 2019

When calling executeOperation, context is generated for the request if context is a function. However, the context function is not invoked with any arguments, unlike when Apollo Server is used in a http environment.

This prevents the context function from performing actions based on the incoming request, such as authentication, when using executeOperation. The incoming request (and others) are in scope at the time context is generated, as these are passed into executeOperation as GraphQLRequest.

options.context = options.context()

// rather than

options.context = options.context({ req: request })

options.context = (options.context as () => never)();

@atkinchris atkinchris changed the title executeOperation does not invoke context with expected signature executeOperation does not invoke context with expected arguments Jun 19, 2019
@abernix
Copy link
Member

abernix commented Aug 26, 2019

Loosely related to #2727. (Just drawing connections here for planning purposes.)

@fazzabi
Copy link

fazzabi commented Jan 30, 2020

I have exactly the same problem as you.
I am creating REST routes that call GraphQL, I need the request to create my graphql context. Can someone explain to me why request is undefined?

@fazzabi
Copy link

fazzabi commented Jan 31, 2020

This solution works well for me

const {graphqlResponse} = await runHttpQuery([request, h], {
    method: request.method.toUpperCase(),
    options: {
        ...graphQLServer.config,
        context: graphQLServer.config.context({request})
    },
    query: {
        operationName,
        query,
        variables: {
            ...request.payload,
            ...request.params,
            ...request.query
        }
    },
    request
});

@bitglue
Copy link

bitglue commented Mar 12, 2021

I think I'm grappling with the same issue: I'm using the express middleware for most things, but I also have an endpoint implemented in express where I want to do a GraphQL query, manipulate the result, then send the result (with express). executeOperation seems like the right way to approach this, but when it creates the context it doesn't pass any arguments, unlike other middlewares. In my case I'm doing it to maintain backwards compatibility with REST clients that existed before the adoption of GraphQL.

However I'm not sure about this:

The incoming request (and others) are in scope at the time context is generated

It seems like the GraphQLRequest is in scope, but the HTTP request (for example, the req argument to an express handler) is not.

Since different middlewares pass different arguments, can executeOperation know what the arguments should be? Perhaps executeOperation itself should have a parameter which it passes on to the context function? In my case I know exactly what the arguments should be, I just don't have any way to communicate that to executeOperation.

@bitglue
Copy link

bitglue commented Mar 12, 2021

An additional difficulty seems to be all the ways I might work around this end up hitting some protected or private member. For example, executeOperation begins:

    public async executeOperation(request: GraphQLRequest) {
      const options = await this.graphQLServerOptions();

But, graphQLServerOptions is protected.

.config, mentioned in @fazzabi's workaround, is private.

bitglue added a commit to bitglue/apollo-server that referenced this issue Mar 12, 2021
Fixes apollographql#2886

Integrations pass implementation-specific arguments to the context
function, but executeOperation passes no arguments. This allows the
caller of executeOperation to specify those arguments, either to emulate
the behavior of an integration or to achieve some other user-specific
behavior. This seems appropriate, as the context function is the
designated entry point for such user-specific behavior.
@glasser
Copy link
Member

glasser commented Mar 12, 2021

The challenge here is that the arguments passed to the context function are specific to the integration and describe, well, the HTTP context. But executeOperation is a method (designed originally for testing IIUC) that specifically bypasses the HTTP layer and jumps directly to running the operation.

It looks like the solution in #5026 basically is saying "if you as a user want to figure out exactly what the argument that the integration you chose passes to graphQLServerOptions is and do your best to replicate it, then it can work". I worry that this makes it hard for the integrations to evolve over time without breaking users of executeOperation, but perhaps it can be documented as something that might change from version to version? I mean you already have to learn what the arguments are in order to implement your context function so maybe that's fine?

@bitglue
Copy link

bitglue commented Mar 12, 2021

I'd say the approach isn't necessarily to be compatibility with an implementation, it just so happens for me that's convenient because:

  1. I'm calling executeOperation from an express handler, so I already have req and res, and
  2. our context function integrates with some other internal libraries that do tracing, logging, metrics and stuff which is designed to work in any express application, so req and res are already expected (a lot of this could be done with an Apollo plugin instead, but we already had the library, and it works for our non-apollo express applications also, so that's convenient)

Generally, resolvers, ApolloServerPlugins, maybe other things will sometimes need to know about things specific to the integration. The problem is the existing implementations provide that to the context function, but the one-off, anonymous integrations that are effectively created by executeOperation have no mechanism to communicate that integration-specific information at all.

In the case where compatibility with an existing implementation isn't possible or desired, then the context function can do some reflection on the argument provided and do the right thing. This is something we're already doing in fact, because we get requests through two different integrations. It's a little inconvenient but it seems to be part of the essential complexity of the problem, and it's manageable because all that complexity of dealing with multiple integrations converges on the context function which can then present a uniform interface to the resolvers and whatnot in whatever way makes sense for the application.

@glasser
Copy link
Member

glasser commented Apr 9, 2021

I've released a prerelease with this fix, version 2.23.0-alpha.0. Try out the alpha and see if it works for you! Please provide any feedback in #5094.

@glasser
Copy link
Member

glasser commented Apr 14, 2021

This is released in Apollo Server 2.23.0.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants