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 19 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


### v1.3.6

* Recognize requests with Apollo Persisted Queries and return `PersistedQueryNotSupported` to the client instead of a confusing error. [PR #982](https://github.com/apollographql/apollo-server/pull/982)
Expand Down
2 changes: 2 additions & 0 deletions packages/apollo-server-core/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

* `apollo-server-core`: add `mocks` parameter to the base constructor(applies to all variants) [PR#1017](https://github.com/apollographql/apollo-server/pull/1017)
* `apollo-server-core`: Remove printing of stack traces with `debug` option and include response in logging function[PR#1018](https://github.com/apollographql/apollo-server/pull/1018)
25 changes: 16 additions & 9 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 @@ -267,20 +275,19 @@ export class ApolloServerBase<Request = RequestInit> {
if (this.engine || engineInRequestPath) this.engineEnabled = true;
}

async request(request: Request) {
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.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely agree! Will be another PR most likely

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

//Defer context resolution to inside of runQuery
context =
typeof this.context === 'function'
? await this.context({ req: request })
? () => 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
59 changes: 53 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,
) {
function enrichError(error: GraphQLError, debug: boolean = false) {
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,43 @@ 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;

const enrichedErrors = errors.map(error => enrichError(error, debug));

if (!formatter) {
return enrichedErrors;
}

return enrichedErrors.map(error => {
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);
}
}
}) 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);
}
Loading