Refactor runHttpQuery and runQuery

[evans]

This PR breaks up runHttpQuery and runQuery into separate functions for each of the distinct steps. These steps include: creation of the query string, parse, validate and execute.

Additionally, this PR changes the error on a non-query for a GET request to a validation error.

A learning out of this refactor is that there's a fair amount of state that is passed deeply, namely debug and formatter. We could make runQuery into a class that is constructed on each request(similar to the extensions stack) and keeps this state.

[martijnwalraven]

Thanks, this is a great start! I think there is more we can do however.

Features like persisted queries and cache control are still tied to runHttpQuery, making it hard to reuse them in other transports. It also isn't clear where we could add whole query caching in a way that takes advantage of the persisted query hash, avoiding having to fetch the query string.

One thorny issue is batching, which really complicates the logic of runHttpQuery and seems to be one of the main reason features are hard to extract. Ideally, batching would be confined to the transport and the rest of the code wouldn't need to know about it. That may require some rethinking of error handling.

More generally, I think breaking up the pipeline into runHttpQuery and runQuery is doing more harm than good, because it keeps us from moving to a cleaner structure and performing optimizations.

We may want to move to a model where transport-specific entry points call smaller functions that are reusable across transports, instead of handing over control to runQuery altogether. With the current structure, there is no clean way to skip validation when using persisted queries for example (although we could probably use persistedQueryHit, passing arbitrary flags around like that makes the code hard to understand).

I think your suggestion of using a class to encapsulate parts of the request pipeline is worth investigating. There is a proliferation of config and options types (Config, GraphQLServerOptions, QueryOptions), making it difficult to understand what gets used where. I suspect we can clean this up by thinking in terms of coherent units instead of individual properties. And some of these units may be natural places for methods as well.

I tried to clean some of the code up, but resolving options and initializing the context is still pretty confusing. I think this is mostly because when we designed the API we were still thinking in terms of middleware handlers. And even when ApolloServer came along, initially the idea was that we would keep exporting the handlers.

So the main flow is through the handlers, which pass an array of handler arguments to runHttpQuery, which is used to call createGraphqlServerOptions, which translates the array into an object, which calls graphqlServerOptions, which calls the context function with the object. And then there is the weird error case where we return a throwing function. All this may be difficult to disentangle, because a lot of the tests work by calling the handlers directly. But I hope we will get to this eventually.

Cleaning up the main request flow should probably have priority though. My hope is that we can break features up even more, so they are encapsulated and can be reused across transports.

We may be able to move most of the cache control code from runHttpQuery to CacheControlExtension for example. That will require some rethinking of the GraphQLExtension API however, and we need to initialize the extension stack earlier (not wait until runQuery).

Let me know how you would like us to work on this. It may make sense to start with a design doc for some of this work. I'm happy to contribute as well. And we could always set up a meeting next week to talk or pair if that is helpful.

[evans]

closed in favor of request pipeline https://github.com/apollographql/apollo-server/tree/new-request-pipeline