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

New Request Pipeline #1795

Merged
merged 62 commits into from
Oct 10, 2018
Merged

New Request Pipeline #1795

merged 62 commits into from
Oct 10, 2018

Conversation

abernix
Copy link
Member

@abernix abernix commented Oct 10, 2018

The Apollo Server codebase has grown and its complexity has increased in a number of ways over the past years. Over time, a few diverging paths have emerged in the codebase which have become increasingly difficult to traverse and oft required implementing logic in multiple places rather than in a single place.

This is not only a bad welcoming experience for newcomers, who might inaccurately zero in a particular part of the code only to later find out that it's one of three places they need to make a change, but it's also difficult to scale the codebase and test the multiple approaches as throughly as would be ideal.

Through a lot of hard work by @martijnwalraven (as exhibited in the commit history for this PR), the new request pipeline implemented by this PR brings improved clarity to the Apollo Server codebase and — at least for now, no breaking changes. We hope to further boil down some of the logic in Apollo Server into more manageable pieces which we feel will be easier to support for the Apollo team and the community as well.

Additionally, for some time now, Apollo Server has incorporated an experimental API known as graphql-extensions. That API originally surfaced to implement the instrumentation for the tracing and performance metrics which are consumed via Apollo Engine (in the past via Apollo Engine Proxy and as of Apollo Server 2.0, via apollo-engine-reporting.

The graphql-extensions API was never intended to be used for general consumption and we've tried to discouraged its more general use, though we know that enough time has gone by without alternatives that there are likely some uses in the wild that aren't under our wing.

We've now had the opportunity to think about and identify other integration points which make sense for Apollo Server, based on feedback and struggles that we've seen the community and our partners struggle with. Therefore, this new request pipeline introduces the notion of plugins which offer a number of life-cycle hooks. We've limited the scope of these hooks for now, so we can carefully consider each need, but we hope that it will be a foundation for further extensibility and flexibility — not to mention decoupling of logic which doesn't necessarily belong in the core.

We hope to iterate on these, and they are still subject to change, but we're planning on getting these into the next alpha of Apollo Server. We're working on getting the documentation in line for this new functionality, but for now, we'll leave it as an exercise to the determined.

abernix and others added 30 commits September 27, 2018 16:33
* Allow an optional function to resolve the rootValue

Passes the parsed DocumentNode AST to determine the root value,
useful when providing a different rootValue for query vs mutation

* Add API docs for rootValue
* Provide ability to specify client info in traces

Adds the createClientInfo to apollo-engine-reporting, which enables the
differentiation of clients based on the request, operation, and
variables. This could be extended to include the response. However for
the first release. It doesn't quite make sense.

* Use extensions and context in createClientInfo

* Remove support for clientAddress

The frontend will not support it in the near future

* create -> generate and make default generator

createClientInfo -> generateClientInfo

* Clarify default values
The `generateClientInfo` API, used to set client identification attributes
within traces, is an experimental API and is subject to removal or change in
a future (major) Apollo Server release.

Ref: #1631
…tests.

The linting will happen in CircleCI.  Additionally, we already have
post-commit hooks so doing linting on every test run seems to just be
generally slowing down development since any linting problems are
automatically fixed when I do a commit.
The ability to pass in functions or promises shouldn't affect the type variable.
…pto`.

This silences the deprecation messages which have been showing up in recent
local development on the new request pipeline work.  e.g.:

> ```
> [DEP0010] DeprecationWarning: crypto.createCredentials is deprecated.
>   Use tls.createSecureContext instead.
> [DEP0011] DeprecationWarning:
>   crypto.Credentials is deprecated. Use tls.SecureContext instead.
> ```
The actual resolving of the context happens in ApolloServer#graphQLServerOptions, but errors thrown while doing that are currently converted to a throwing function.
…pto`.

This silences the deprecation messages which have been showing up in recent
local development on the new request pipeline work.  e.g.:

> ```
> [DEP0010] DeprecationWarning: crypto.createCredentials is deprecated.
>   Use tls.createSecureContext instead.
> [DEP0011] DeprecationWarning:
>   crypto.Credentials is deprecated. Use tls.SecureContext instead.
> ```
While cacheControl can take a boolean when passing it in  to ApolloServer as part of a Config, these will be converted to default options before passing them on as options.
abernix and others added 3 commits October 10, 2018 19:14
* Pass the context along to all the extension methods

Addresses #1343

With this change you should now be able to implement an extension like so:

```javascript
class MyErrorTrackingExtension extends GraphQLExtension {
    willSendResponse(o) {
        const { context, graphqlResponse } = o

        context.trackErrors(graphqlResponse.errors)

        return o
    }
}
```

Edit by @evans :
fixes #1343
fixes #614 as the request object can be taken from context or from requestDidStart
fixes #631 ""

* Remove context from extra extension functions

The context shouldn't be necessary for format, parse, validation, and
execute. Format generally outputs the state of the extension. Parse and
validate don't depend on the context. Execution includes the context
argument as contextValue

* Move change entry under vNext
…eline.

Prior to embarking on the request pipeline work, and to facilitate
development on the new request pipeline, @martijnwalraven and myself
intentionally reverted some commits that had been recently introduced but
conflicted with the request pipeline branch that was already in-flight.

Rather than dealing with an incredibly difficult merge conflict, it was
easier to revert them an re-apply them later.

Original commits, reversions, and reintroductions:

* 6d6c9ff, 03a894d, b90ccc2
* 96af44e, 68c82e6, 81c4642
* 408198e, 2b470e1, 84bc834
* 4175f1b, 38e7b6a, 261994c

As is demonstrated by this short follow-up commit, this was all that was
necessary to make it work in the new model - once that model was finished.

While we're certain that the new request pipeline and its plugin model will
actually better support some of the behavior that required these initial
commits in the name of not introducing breaking changes, we feel it's
necessary to maintain their support.

I will point out, if you are at all affected by the above commits, we may
consider deprecating portions of those APIs in the not too distant future,
but we hope that the new model will help support those needs even better.
@codecov-io
Copy link

codecov-io commented Oct 10, 2018

Codecov Report

Merging #1795 into master will increase coverage by 3.98%.
The diff coverage is 62.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1795      +/-   ##
=========================================
+ Coverage   73.82%   77.8%   +3.98%     
=========================================
  Files          29      30       +1     
  Lines        1150    1149       -1     
  Branches      296     260      -36     
=========================================
+ Hits          849     894      +45     
+ Misses        290     246      -44     
+ Partials       11       9       -2
Impacted Files Coverage Δ
packages/apollo-server-core/src/graphqlOptions.ts 75% <ø> (ø) ⬆️
...ackages/apollo-server-express/src/expressApollo.ts 86.36% <100%> (ø) ⬆️
...kages/apollo-server-core/src/requestPipelineAPI.ts 100% <100%> (ø)
packages/apollo-server/src/index.ts 96.87% <100%> (+0.1%) ⬆️
packages/apollo-server-core/src/runHttpQuery.ts 47.22% <45.78%> (+17.02%) ⬆️
...ackages/apollo-server-core/src/utils/dispatcher.ts 47.05% <47.05%> (ø)
packages/apollo-server-core/src/requestPipeline.ts 73.59% <73.59%> (ø)
packages/apollo-datasource-rest/src/HTTPCache.ts 97.87% <0%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b05b5e...9089cc5. Read the comment docs.

@abernix abernix merged commit db8fdc8 into master Oct 10, 2018
@abernix abernix deleted the abernix/re-new-request-pipeline branch October 10, 2018 18:47
@maticzav maticzav mentioned this pull request Oct 13, 2018
4 tasks
abernix added a commit that referenced this pull request Oct 23, 2018
This commit follows-up on #1795, which introduced the new request pipeline, and
implements the triggers for its new life-cycle hooks within the various
integration packages.  Previously, the only implementation was within the
`apollo-server` package and didn't get triggered for those invoking the
`applyMiddleware` method on integrations (e.g. `apollo-server-express`,
`...-hapi` and `...-koa`).

While in an ideal world, ALL existing `applyMiddleware` functions would be
marked as `async` functions and allow us to `await ApolloServer#willStart`,
in practice the only `applyMiddleware` which is currently `async` is the one
within the Hapi implementation.  Therefore, we'll instead kick off the
`willStart` lifecycle hook as soon as `applyMiddleware` is started, return
as quickly as we have before and then (essentially) yield the completion of
Apollo Server's `willStart` prior to serving the first request — thus
ensuring the completion of server-startup activities.

Similarly, we'll do the same for `createHandler` methods on integrations
which don't utilize Node.js server frameworks but don't have `async`
handlers (e.g. AWS, Lambda, etc.).
abernix added a commit that referenced this pull request Oct 26, 2018
This commit follows-up on #1795, which introduced the new request pipeline, and
implements the triggers for its new life-cycle hooks within the various
integration packages.  Previously, the only implementation was within the
`apollo-server` package and didn't get triggered for those invoking the
`applyMiddleware` method on integrations (e.g. `apollo-server-express`,
`...-hapi` and `...-koa`).

While in an ideal world, ALL existing `applyMiddleware` functions would be
marked as `async` functions and allow us to `await ApolloServer#willStart`,
in practice the only `applyMiddleware` which is currently `async` is the one
within the Hapi implementation.  Therefore, we'll instead kick off the
`willStart` lifecycle hook as soon as `applyMiddleware` is started, return
as quickly as we have before and then (essentially) yield the completion of
Apollo Server's `willStart` prior to serving the first request — thus
ensuring the completion of server-startup activities.

Similarly, we'll do the same for `createHandler` methods on integrations
which don't utilize Node.js server frameworks but don't have `async`
handlers (e.g. AWS, Lambda, etc.).
abernix added a commit that referenced this pull request Nov 21, 2018
This is still WIP, but aims to demonstrate some of the new functionality and
flexibility introduced in Apollo Server 2.2's new request pipeline (See
#1795 for implementation
details).
abernix added a commit that referenced this pull request Apr 9, 2019
The full-query caching feature implemented in
#2437 originally took the
additional steps of also changing the `apollo-engine-reporting` module to
utilize the new request pipeline available in Apollo Server as of
#1795.

While that would have been nice, there was still some uncertainty about some
of the life-cycle hooks that would be necesssary to make that happen and it
wasn't worth blocking the implementation of full-query caching on those
stalled decisions.

Therefore, the changes to utilize new functionality in the request pipeline,
including what would have been a simplification of the way that
`apollo-engine-reporting` would have obtained the `operationName` (something
that is available via the API of the request-pipeline hooks), were backed
out and will land separately in a future PR.

The portion regarding `operationName` was inadvertently not backed out and
instead was left leveraging a life-cycle hook which was not available to the
`graphql-extensions` API: `didResolveOperation`.

This means that the code was not setting `operationName` in the way it
needed to and therefore `operationName` was always being left undefined (as
is sometimes permitted!) with this new `apollo-engine-reporting`.

This commit puts the functionality back to that which is required for the
`graphql-extensions` implementation, and the commit before this
(1ae7def) acts as a regression test (it
should pass, as of this commit, and fail before it).
abernix added a commit that referenced this pull request Apr 10, 2019
The full-query caching feature implemented in
#2437 originally took the
additional steps of also changing the `apollo-engine-reporting` module to
utilize the new request pipeline available in Apollo Server as of
#1795.

While that would have been nice, there was still some uncertainty about some
of the life-cycle hooks that would be necesssary to make that happen and it
wasn't worth blocking the implementation of full-query caching on those
stalled decisions.

Therefore, the changes to utilize new functionality in the request pipeline,
including what would have been a simplification of the way that
`apollo-engine-reporting` would have obtained the `operationName` (something
that is available via the API of the request-pipeline hooks), were backed
out and will land separately in a future PR.

The portion regarding `operationName` was inadvertently not backed out and
instead was left leveraging a life-cycle hook which was not available to the
`graphql-extensions` API: `didResolveOperation`.

This means that the code was not setting `operationName` in the way it
needed to and therefore `operationName` was always being left undefined (as
is sometimes permitted!) with this new `apollo-engine-reporting`.

This commit puts the functionality back to that which is required for the
`graphql-extensions` implementation, and the commit before this (8a43341)
acts as a regression test (it should pass, as of this commit, and fail
before it).
abernix added a commit that referenced this pull request Apr 11, 2019
* fix: Set `operationName` via `executionDidStart` and `willResolveField`.

The full-query caching feature implemented in
#2437 originally took the
additional steps of also changing the `apollo-engine-reporting` module to
utilize the new request pipeline available in Apollo Server as of
#1795.

While that would have been nice, there was still some uncertainty about some
of the life-cycle hooks that would be necesssary to make that happen and it
wasn't worth blocking the implementation of full-query caching on those
stalled decisions.

Therefore, the changes to utilize new functionality in the request pipeline,
including what would have been a simplification of the way that
`apollo-engine-reporting` would have obtained the `operationName` (something
that is available via the API of the request-pipeline hooks), were backed
out and will land separately in a future PR.

The portion regarding `operationName` was inadvertently not backed out and
instead was left leveraging a life-cycle hook which was not available to the
`graphql-extensions` API: `didResolveOperation`.

This means that the code was not setting `operationName` in the way it
needed to and therefore `operationName` was always being left undefined (as
is sometimes permitted!) with this new `apollo-engine-reporting`.

This commit puts the functionality back to that which is required for the
`graphql-extensions` implementation, and the commit before this (8a43341)
acts as a regression test (it should pass, as of this commit, and fail
before it).

* Add a regression test for `operationName` not being defined.

This should fail and then be fixed with an upcoming commit.
StephenBarlow pushed a commit that referenced this pull request Aug 23, 2019
This is still WIP, but aims to demonstrate some of the new functionality and
flexibility introduced in Apollo Server 2.2's new request pipeline (See
#1795 for implementation
details).
StephenBarlow pushed a commit that referenced this pull request Aug 28, 2019
This is still WIP, but aims to demonstrate some of the new functionality and
flexibility introduced in Apollo Server 2.2's new request pipeline (See
#1795 for implementation
details).
StephenBarlow pushed a commit that referenced this pull request Sep 13, 2019
This is still WIP, but aims to demonstrate some of the new functionality and
flexibility introduced in Apollo Server 2.2's new request pipeline (See
#1795 for implementation
details).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants