apollographql / apollo-server

🌍  Spec-compliant and production ready JavaScript GraphQL server that lets you develop in a schema-first way. Built for Express, Connect, Hapi, Koa, and more.
https://www.apollographql.com/docs/apollo-server/
MIT License
13.81k stars 2.03k forks source link

Apollo Server 2: Error when setting keys on Context and using DataSources #1247

Closed Saeris closed 6 years ago

Saeris commented 6 years ago

Ran into an issue in configuring my server where I would receive the following error if I set any key at all on the context object while using DataSources:

Please use the dataSources config option instead of putting dataSources on the context yourself.

Here's a snippet of my server config:

// ...

const server = new ApolloServer({
  schema: makeExecutableSchema({
    typeDefs: [...Object.values(types), ...Object.values(enums), ...Object.values(inputs)],
    resolvers,
    logger: {
      log: err => error(`An error has occurred while resolving a GraphQL request:`, err)
    }
  }),
  formatError,
  context: {
    images: `https://image.tmdb.org/t/p/`
  },
  dataSources: () => ({
    MovieDB: new MovieDB()
  }),
  debug: true
})

// ...

I can throw together a repl to reproduce this error if needed, but it should be pretty straight forward. From what I understand, Apollo doesn't want you to set a dataSources key on the context object, but instead throws an error if any key is set at all. I was able to work around this by refactoring my code to use an Images datasource instead, but this is clearly broken behavior. There's numerous reasons why you'd want to make data available via the context object, so this would be a breaking change for a lot of users if not addressed.

As a side note, even with debug: true none of these errors get logged to the node.js console. I spent a long time trying to figure out what the problem was, because all I was getting in my Playground was "error": "Response not successful: Received status code 400" every time I tried to run a query. Later I discovered that if I went to the network tab and inspected the response object I was getting back from the server, the actual error message would turn up there. It should also be noted that I had my server configured to use Apollo Engine as well, and I didn't see any error messages show up there either, all it was receiving was introspection queries from loading the playground. Not sure what needs to be fixed there, but I can tell you it was a total pain to try and debug.

martijnwalraven commented 6 years ago

It should definitely be possible to use context as intended, and I have a demo app running that works just fine. We do just check for dataSources. So there must be something more specific going on.

Is there any chance you are using query batching? If have a suspicion it might be related to that. (We 'clone' the context for every request in the batch and that might somehow include the dataSources, although I don't directly see how that happens.)

Saeris commented 6 years ago

Let me put you together a minimal example, because I don't believe I'm doing anything out of the ordinary like query batching.

martijnwalraven commented 6 years ago

Ah, I think I figured it out! You're setting the context directly, instead of defining a context function that creates it afresh for every request. And that means when we put dataSources on the context, the next request will fail (because it is already there). Definitely a bug on our end, I'll get a fix in!

martijnwalraven commented 6 years ago

As a workaround, you could change:

 context: {
    images: `https://image.tmdb.org/t/p/`
  },

to:

 context: () => ({
    images: `https://image.tmdb.org/t/p/`
  }),
Saeris commented 6 years ago

Yeah okay, that's probably the issue. None of that was documented so it's hard to tell what's the proper way to use DataSources in your server config. Deleting the context key in my server options is what fixed my issue.

Anyways here's the example even though we don't really need it anymore.

https://repl.it/@Saeris/Apollo-Server-2-Context-Error-Example

martijnwalraven commented 6 years ago

This should be fixed in the latest release.