Enables context creation to be async and capture errors with opt-in logging #1024

evans · 2018-05-03T06:51:48Z

This PR allows the context option in apollo-server-core's runQuery/runHttpQuery to be an async function, which would be common in authentication fetching scenarios.

Additionally, this PR increases the capture and formatting of errors during the creation of the options and context.

…e found in originalError

…lo error to add code

… callback

…s as a function

evans · 2018-05-04T05:16:08Z

packages/apollo-server-core/src/runHttpQuery.ts

+    throw new HttpQueryError(
+      500,
+      JSON.stringify({
+        errors: formatApolloErrors([e], { debug: debugDefault }),


This error will return when the options to runHttpQuery are wrapped in a function, which throws an error. Currently this will only get called when the old middleware creation(expressGraphQL) is used, since the new server construction expects an object.

This line makes me nervous, since a production server that does not use a NODE_ENV of 'production' or 'test' will return a stacktrace to the client. A stacktrace is extremely informative for server creators to debug their systems. The other option would be to log the error in the console, which is non-ideal, since logging should be opt-in. The unfortunate part is that the user provided logging function is not resolved yet when the error occurs.

I appreciate you raising this concern here explicitly. Is there any way we can await the resolution of the logging function prior to throwing?

abernix

Looking pretty good! Some comments within...

abernix · 2018-05-07T12:02:35Z

packages/apollo-server-core/src/ApolloServer.ts

@@ -270,17 +278,16 @@ export class ApolloServerBase<Request = RequestInit> {
  async request(request: Request) {


The await itself has been removed, does this need to be an async function still?

abernix · 2018-05-07T12:04:24Z

packages/apollo-server-core/src/errors.ts

-  error: GraphQLError,
-  debug: boolean = false,
-) {
+export function enrichError(error: GraphQLError, debug: boolean = false) {


Does this need to be exported? Preference to not expose it if we don't need to!

abernix · 2018-05-08T06:07:31Z

packages/apollo-server-core/src/ApolloServer.ts

@@ -270,17 +278,16 @@ export class ApolloServerBase<Request = RequestInit> {
  async request(request: Request) {
    let context: Context = this.context ? this.context : { request };

+    //Differ context resolution to inside of runQuery


typo: Defer?

abernix · 2018-05-08T07:39:48Z

packages/apollo-server-core/src/errors.ts

+): Array<Error> {
+  const { formatter, debug, logFunction } = options;
+  return errors.map(error => enrichError(error, debug)).map(error => {
+    if (formatter !== undefined) {


Rather than having this larger if block surrounding the totality of this map, could we do something like:

if (!formatter) { return error; } // Remainder of existing logic, indented by two less spaces. try { // ...

?

By hoisting that logic, it allows the reader of this code to focus on the remaining functionality, rather than needing to keep in mind that we're still inside a conditional block (the if).

abernix · 2018-05-08T07:46:08Z

packages/apollo-server-core/src/runHttpQuery.ts

@@ -218,9 +250,16 @@ export async function runHttpQuery(
        return Promise.reject(e);
      }

-      return Promise.resolve({ errors: [formatErrorFn(e)] });
+      return Promise.resolve({


The Promise.resolve here can be removed since an async function implicitly wraps return values in Promise.resolve.

abernix · 2018-05-08T07:57:09Z

packages/apollo-server-core/src/runHttpQuery.ts

+    throw new HttpQueryError(
+      500,
+      JSON.stringify({
+        errors: formatApolloErrors([e], { debug: debugDefault }),


I appreciate you raising this concern here explicitly. Is there any way we can await the resolution of the logging function prior to throwing?

…ction

…k to out of map

…efault enabled

…values

glasser

This seems basically fine although I don't actually know the motivation behind it. A few questions below.

glasser · 2018-05-08T18:19:44Z

CHANGELOG.md

@@ -4,6 +4,8 @@ All of the packages in the `apollo-server` repo are released with the same versi

 ### vNEXT

+* Remove tests and guaranteed support for Node 4 [PR #1024](https://github.com/apollographql/apollo-server/pull/1024)


What's the motivation here? (I can certainly believe it's a pain to maintain Node 4 compat but I'm curious what the straw that broke the camel's back was.)

https://circleci.com/gh/apollographql/apollo-server/4564?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

The same code and setup lead to a different error code. Possibly worth investigating, though I think it's more distracting than valuable

glasser · 2018-05-08T18:26:26Z

packages/apollo-server-core/CHANGELOG.md

@@ -2,5 +2,7 @@

 ### vNEXT

+* `apollo-server-core`: Replace console.error with logFunction for opt-in logging [PR #1024](https://github.com/apollographql/apollo-server/pull/1024)
+* `apollo-server-core`: errors in context creation can be async and is formatted to include error code [PR #1024](https://github.com/apollographql/apollo-server/pull/1024)


not just error can be async — context creation in general can be async?

glasser · 2018-05-08T18:27:19Z

packages/apollo-server-core/src/ApolloServer.ts

@@ -267,20 +275,19 @@ export class ApolloServerBase<Request = RequestInit> {
    if (this.engine || engineInRequestPath) this.engineEnabled = true;
  }

-  async request(request: Request) {
+  request(request: Request) {


just want to reiterate now that 2.0 is me and you that I find the name of this method to be incomprehensible and I hope we change it before release. Doesn't have to be in this PR.

Absolutely agree! Will be another PR most likely

glasser · 2018-05-08T18:28:43Z

packages/apollo-server-core/src/runHttpQuery.ts

+        context = {};
+      } else if (typeof context === 'function') {
+        try {
+          context = await context();


So isn't this a backwards-incompatible change? Doesn't this break non-async context functions?

I mean that's OK because this is 2.0 I guess, but let's not bury the lede here...

await unwraps a Promise if it's there. If the value on the rhs of await is an object or value, then it is returned immediately. I think this is bug fix or at minimum a strict addition to api to allow the context to return a Promise in addition to a value. What this change does break is context functions that returns a promise and the resolvers expect the context argument to be a Promise. I think this case is pretty unlikely and not a good best practice.

https://github.com/apollographql/apollo-server/pull/1024/files/4225c08b10c90dad5c0acc0b27fc72f10482ad12#diff-5102db46bcc7f6cc0c59149e9cc375d3R26 is the type addition to the context

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await
shows an example that has await 20.

ghost added the ⛲️ feature New addition or enhancement to existing solutions label May 3, 2018

evans force-pushed the refactor-2.0 branch from e43b1d4 to 4dad99c Compare May 3, 2018 18:01

evans force-pushed the evans/context-error branch from ba5ed13 to e06aa30 Compare May 3, 2018 19:51

evans added 9 commits May 3, 2018 12:54

apollo-server-core: move logging into separate file

dcac1b0

apollo-server-core: internalFormatError to formatApolloError

879a289

apollo-server-core: apolloError places properties in root, so they ar…

97cc12a

…e found in originalError

apollo-server-core: wrap error thrown by context construction in apol…

2f3eba2

…lo error to add code

apollo-server-core: added formatApolloError arguments to subscription…

f348355

… callback

apollo-server-core: stronger typing on context in GraphQLServerOption…

dc07e92

…s as a function

apollo-server-core: allow creation of context to be asynchronous

dfccfa6

apollo-server-core: capture context creation failure in GraphQL error

2a36f0f

apollo-server-core: add check for null property in ApolloError

f1c1e79

evans force-pushed the evans/context-error branch from e06aa30 to f1c1e79 Compare May 3, 2018 19:56

evans added 5 commits May 3, 2018 13:01

apollo-server-express: fix tests to include error code

2f5d243

apollo-server-integration-testsuite: fix tests for apollo-server 2

35420e8

apollo-server-koa: add node types to koa

cf48115

apollo-server-core: use getOwnPropertyNames to stay es5

b101384

build: remove node 4 tests

fdd341c

evans force-pushed the evans/context-error branch from 412cc1c to fdd341c Compare May 4, 2018 00:50

evans commented May 4, 2018

View reviewed changes

evans requested review from abernix and jbaxleyiii May 4, 2018 17:46

changelogs: added to root and apollo-server-core

5fa8d88

evans added this to the Release 2.0 milestone May 4, 2018

abernix reviewed May 8, 2018

View reviewed changes

evans added 4 commits May 8, 2018 11:01

apollo-server-core: remove unsued async from BaseServer's request fun…

9522c5a

…ction

apollo-server-core: enrichError not exported and moved formatter chec…

6be12f1

…k to out of map

apollo-server-core: add warning to options creation failure if debugD…

7a284d5

…efault enabled

apollo-server-core: remove Promise wrapping in async function return …

4225c08

…values

glasser reviewed May 8, 2018

View reviewed changes

evans added 2 commits May 8, 2018 11:39

apollo-server-core: run http options allows query to be an array

a08594f

apollo-server-core: clarify context creation can be async in changelog

fd79cf7

evans merged commit 33afa32 into refactor-2.0 May 8, 2018

evans deleted the evans/context-error branch May 8, 2018 21:14

dependencies bot mentioned this pull request Aug 15, 2018

Update apollo-server-express in / from 1.2.0 to 2.0.2 LibreTubeApp/LibreTube#158

Open

github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enables context creation to be async and capture errors with opt-in logging #1024

Enables context creation to be async and capture errors with opt-in logging #1024

evans commented May 3, 2018

evans May 4, 2018

abernix May 8, 2018

abernix left a comment

abernix May 7, 2018

abernix May 7, 2018

abernix May 8, 2018

abernix May 8, 2018

abernix May 8, 2018

abernix May 8, 2018

glasser left a comment

glasser May 8, 2018

evans May 8, 2018

glasser May 8, 2018

glasser May 8, 2018

evans May 8, 2018

glasser May 8, 2018

evans May 8, 2018 •

edited

Loading

		@@ -270,17 +278,16 @@ export class ApolloServerBase<Request = RequestInit> {
		async request(request: Request) {

		@@ -4,6 +4,8 @@ All of the packages in the `apollo-server` repo are released with the same versi

		### vNEXT

		* Remove tests and guaranteed support for Node 4 [PR #1024](https://github.com/apollographql/apollo-server/pull/1024)

Enables context creation to be async and capture errors with opt-in logging #1024

Enables context creation to be async and capture errors with opt-in logging #1024

Conversation

evans commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evans May 8, 2018 • edited Loading

Choose a reason for hiding this comment

evans May 8, 2018 •

edited

Loading