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

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

Merged
merged 21 commits into from
May 8, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
dcac1b0
apollo-server-core: move logging into separate file
evans May 2, 2018
879a289
apollo-server-core: internalFormatError to formatApolloError
evans May 2, 2018
97cc12a
apollo-server-core: apolloError places properties in root, so they ar…
evans May 2, 2018
2f3eba2
apollo-server-core: wrap error thrown by context construction in apol…
evans May 2, 2018
f348355
apollo-server-core: added formatApolloError arguments to subscription…
evans May 3, 2018
dc07e92
apollo-server-core: stronger typing on context in GraphQLServerOption…
evans May 3, 2018
dfccfa6
apollo-server-core: allow creation of context to be asynchronous
evans May 3, 2018
2a36f0f
apollo-server-core: capture context creation failure in GraphQL error
evans May 3, 2018
f1c1e79
apollo-server-core: add check for null property in ApolloError
evans May 3, 2018
2f5d243
apollo-server-express: fix tests to include error code
evans May 3, 2018
35420e8
apollo-server-integration-testsuite: fix tests for apollo-server 2
evans May 3, 2018
cf48115
apollo-server-koa: add node types to koa
evans May 3, 2018
b101384
apollo-server-core: use getOwnPropertyNames to stay es5
evans May 4, 2018
fdd341c
build: remove node 4 tests
evans May 4, 2018
5fa8d88
changelogs: added to root and apollo-server-core
evans May 4, 2018
9522c5a
apollo-server-core: remove unsued async from BaseServer's request fun…
evans May 8, 2018
6be12f1
apollo-server-core: enrichError not exported and moved formatter chec…
evans May 8, 2018
7a284d5
apollo-server-core: add warning to options creation failure if debugD…
evans May 8, 2018
4225c08
apollo-server-core: remove Promise wrapping in async function return …
evans May 8, 2018
a08594f
apollo-server-core: run http options allows query to be an array
evans May 8, 2018
fd79cf7
apollo-server-core: clarify context creation can be async in changelog
evans May 8, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ jobs:
# Platform tests, each with the same tests but different platform or version.
# The docker tag represents the Node.js version and the full list is available
# at https://hub.docker.com/r/circleci/node/.
Node.js 4:
docker: [ { image: 'circleci/node:4' } ]
<<: *common_test_steps

Node.js 6:
docker: [ { image: 'circleci/node:6' } ]
<<: *common_test_steps
Expand Down Expand Up @@ -73,7 +69,6 @@ workflows:
version: 2
Build and Test:
jobs:
- Node.js 4
- Node.js 6
- Node.js 8
- Node.js 9
Expand Down
23 changes: 15 additions & 8 deletions packages/apollo-server-core/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ import {
ExecutionParams,
} from 'subscriptions-transport-ws';

import { internalFormatError } from './errors';
import { formatApolloErrors } from './errors';
import { GraphQLServerOptions as GraphQLOptions } from './graphqlOptions';
import { LogFunction } from './runQuery';
import { LogFunction, LogAction, LogStep } from './logging';

import {
Config,
Expand Down Expand Up @@ -213,7 +213,12 @@ export class ApolloServerBase<Request = RequestInit> {
connection.formatResponse = (value: ExecutionResult) => ({
...value,
errors:
value.errors && value.errors.map(err => internalFormatError(err)),
value.errors &&
formatApolloErrors(value.errors, {
formatter: this.requestOptions.formatError,
debug: this.requestOptions.debug,
logFunction: this.requestOptions.logFunction,
}),
});
let context: Context = this.context ? this.context : { connection };

Expand All @@ -223,8 +228,11 @@ export class ApolloServerBase<Request = RequestInit> {
? await this.context({ connection })
: context;
} catch (e) {
console.error(e);
throw e;
throw formatApolloErrors([e], {
formatter: this.requestOptions.formatError,
debug: this.requestOptions.debug,
logFunction: this.requestOptions.logFunction,
})[0];
}

return { ...connection, context };
Expand Down Expand Up @@ -270,17 +278,16 @@ export class ApolloServerBase<Request = RequestInit> {
async request(request: Request) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let context: Context = this.context ? this.context : { request };

//Differ context resolution to inside of runQuery
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: Defer?

context =
typeof this.context === 'function'
? await this.context({ req: request })
? async () => this.context({ req: request })
: context;

return {
schema: this.schema,
tracing: Boolean(this.engineEnabled),
cacheControl: Boolean(this.engineEnabled),
formatError: (e: GraphQLError) =>
internalFormatError(e, this.requestOptions.debug),
context,
// allow overrides from options
...this.requestOptions,
Expand Down
56 changes: 50 additions & 6 deletions packages/apollo-server-core/src/errors.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { GraphQLError } from 'graphql';
import { LogStep, LogAction, LogMessage, LogFunction } from './logging';

export class ApolloError extends Error {
public extensions;
Expand All @@ -8,14 +9,20 @@ export class ApolloError extends Error {
properties?: Record<string, any>,
) {
super(message);
this.extensions = { ...properties, code };

if (properties) {
Object.keys(properties).forEach(key => {
this[key] = properties[key];
});
}

//extensions are flattened to be included in the root of GraphQLError's, so
//don't add properties to extensions
this.extensions = { code };
}
}

export function internalFormatError(
error: GraphQLError,
debug: boolean = false,
) {
export function enrichError(error: GraphQLError, debug: boolean = false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const expanded: GraphQLError = {
message: error.message,
path: error.path,
Expand Down Expand Up @@ -95,7 +102,7 @@ export function fromGraphQLError(error: GraphQLError, options?: ErrorOptions) {
//copy the original error, while keeping all values non-enumerable, so they
//are not printed unless directly referenced
Object.defineProperty(copy, 'originalError', { value: {} });
Reflect.ownKeys(error).forEach(key => {
Object.getOwnPropertyNames(error).forEach(key => {
Object.defineProperty(copy.originalError, key, { value: error[key] });
});

Expand Down Expand Up @@ -133,3 +140,40 @@ export class ForbiddenError extends ApolloError {
super(message, 'FORBIDDEN');
}
}

export function formatApolloErrors(
errors: Array<Error>,
options?: {
formatter?: Function;
logFunction?: LogFunction;
debug?: boolean;
},
): Array<Error> {
const { formatter, debug, logFunction } = options;
return errors.map(error => enrichError(error, debug)).map(error => {
if (formatter !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

try {
return formatter(error);
} catch (err) {
logFunction({
action: LogAction.cleanup,
step: LogStep.status,
data: err,
key: 'error',
});

if (debug) {
return enrichError(err, debug);
} else {
//obscure error
const newError: GraphQLError = fromGraphQLError(
new GraphQLError('Internal server error'),
);
return enrichError(newError, debug);
}
}
} else {
return error;
}
}) as Array<Error>;
}
22 changes: 13 additions & 9 deletions packages/apollo-server-core/src/graphqlOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {
ValidationContext,
GraphQLFieldResolver,
} from 'graphql';
import { LogFunction } from './runQuery';
import { LogFunction } from './logging';
import { GraphQLExtension } from 'graphql-extensions';

/*
Expand All @@ -21,7 +21,11 @@ import { GraphQLExtension } from 'graphql-extensions';
* - (optional) debug: a boolean that will print additional debug logging if execution errors occur
*
*/
export interface GraphQLServerOptions<TContext = any> {
export interface GraphQLServerOptions<
TContext =
| (() => Promise<Record<string, any>> | Record<string, any>)
| Record<string, any>
> {
schema: GraphQLSchema;
formatError?: Function;
rootValue?: any;
Expand All @@ -40,15 +44,15 @@ export interface GraphQLServerOptions<TContext = any> {
export default GraphQLServerOptions;

export async function resolveGraphqlOptions(
options: GraphQLServerOptions | Function,
...args
options:
| GraphQLServerOptions
| ((
...args: Array<any>
) => Promise<GraphQLServerOptions> | GraphQLServerOptions),
...args: Array<any>
): Promise<GraphQLServerOptions> {
if (typeof options === 'function') {
try {
return await options(...args);
} catch (e) {
throw new Error(`Invalid options provided to ApolloServer: ${e.message}`);
}
return await options(...args);
} else {
return options;
}
Expand Down
11 changes: 3 additions & 8 deletions packages/apollo-server-core/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
export {
runQuery,
LogFunction,
LogMessage,
LogStep,
LogAction,
} from './runQuery';
export { runQuery } from './runQuery';
export { LogFunction, LogMessage, LogStep, LogAction } from './logging';
export { runHttpQuery, HttpQueryRequest, HttpQueryError } from './runHttpQuery';
export {
default as GraphQLOptions,
Expand All @@ -17,7 +12,7 @@ export {
ValidationError,
AuthenticationError,
ForbiddenError,
internalFormatError,
formatApolloErrors,
} from './errors';

// ApolloServer Base class
Expand Down
25 changes: 25 additions & 0 deletions packages/apollo-server-core/src/logging.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
export enum LogAction {
request,
parse,
validation,
execute,
setup,
cleanup,
}

export enum LogStep {
start,
end,
status,
}

export interface LogMessage {
action: LogAction;
step: LogStep;
key?: string;
data?: any;
}

export interface LogFunction {
(message: LogMessage);
}
69 changes: 54 additions & 15 deletions packages/apollo-server-core/src/runHttpQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ import {
default as GraphQLOptions,
resolveGraphqlOptions,
} from './graphqlOptions';
import { internalFormatError } from './errors';
import { formatApolloErrors } from './errors';

export interface HttpQueryRequest {
method: string;
query: Record<string, any>;
options: GraphQLOptions | Function;
options: GraphQLOptions | (() => Promise<GraphQLOptions> | GraphQLOptions);
}

export class HttpQueryError extends Error {
Expand Down Expand Up @@ -42,20 +42,31 @@ export async function runHttpQuery(
): Promise<string> {
let isGetRequest: boolean = false;
let optionsObject: GraphQLOptions;
const debugDefault =
process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test';

try {
optionsObject = await resolveGraphqlOptions(
request.options,
...handlerArguments,
);
} catch (e) {
throw new HttpQueryError(500, e.message);
// The options can be generated asynchronously, so we don't have access to
// the normal options provided by the user, such as: formatError,
// logFunction, debug. Therefore, we need to do some unnatural things, such
// as use NODE_ENV to determine the debug settings
e.message = `Invalid options provided to ApolloServer: ${e.message}`;
throw new HttpQueryError(
500,
JSON.stringify({
errors: formatApolloErrors([e], { debug: debugDefault }),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}),
true,
{
'Content-Type': 'application/json',
},
);
}
const formatErrorFn = optionsObject.formatError
? error => optionsObject.formatError(internalFormatError(error))
: internalFormatError;
const debugDefault =
process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test';
const debug =
optionsObject.debug !== undefined ? optionsObject.debug : debugDefault;
let requestPayload;
Expand Down Expand Up @@ -99,7 +110,7 @@ export async function runHttpQuery(
requestPayload = [requestPayload];
}

const requests: Array<ExecutionResult> = requestPayload.map(requestParams => {
const requests = requestPayload.map(async requestParams => {
try {
let query = requestParams.query;
let extensions = requestParams.extensions;
Expand Down Expand Up @@ -179,9 +190,30 @@ export async function runHttpQuery(
}
}

let context = optionsObject.context || {};
if (typeof context === 'function') {
context = context();
let context = optionsObject.context;
if (!context) {
//appease typescript compiler, otherwise could use || {}
context = {};
} else if (typeof context === 'function') {
try {
context = await context();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

@evans evans May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (e) {
e.message = `Context creation failed: ${e.message}`;
throw new HttpQueryError(
500,
JSON.stringify({
errors: formatApolloErrors([e], {
formatter: optionsObject.formatError,
debug,
logFunction: optionsObject.logFunction,
}),
}),
true,
{
'Content-Type': 'application/json',
},
);
}
} else if (isBatch) {
context = Object.assign(
Object.create(Object.getPrototypeOf(context)),
Expand All @@ -198,7 +230,7 @@ export async function runHttpQuery(
operationName: operationName,
logFunction: optionsObject.logFunction,
validationRules: optionsObject.validationRules,
formatError: formatErrorFn,
formatError: optionsObject.formatError,
formatResponse: optionsObject.formatResponse,
fieldResolver: optionsObject.fieldResolver,
debug: optionsObject.debug,
Expand All @@ -218,9 +250,16 @@ export async function runHttpQuery(
return Promise.reject(e);
}

return Promise.resolve({ errors: [formatErrorFn(e)] });
return Promise.resolve({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

errors: formatApolloErrors([e], {
formatter: optionsObject.formatError,
debug,
logFunction: optionsObject.logFunction,
}),
});
}
});
}) as Array<Promise<ExecutionResult>>;

const responses = await Promise.all(requests);

if (!isBatch) {
Expand Down
3 changes: 2 additions & 1 deletion packages/apollo-server-core/src/runQuery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ import {
parse,
} from 'graphql';

import { runQuery, LogAction, LogStep } from './runQuery';
import { runQuery } from './runQuery';
import { LogAction, LogStep } from './logging';

// Make the global Promise constructor Fiber-aware to simulate a Meteor
// environment.
Expand Down
Loading