Merge pull request #65 from lifeomic/circularContext

Handle synchronous resolver errors

Author: David Tanner

.eslintrc.yml

@@ -1,28 +1,20 @@
 {
-  "overrides": [
-    {
-      "files": ["**/*.js"],
-      "extends": [
-          "plugin:@lifeomic/eslint-plugin-node/recommended"
-      ]
-    },
-    {
-      "files": ["**/*.ts"],
-      "extends": [
-          "plugin:@lifeomic/eslint-plugin-node/recommended",
-          "eslint:recommended",
-          "plugin:@typescript-eslint/eslint-recommended",
-          "plugin:@typescript-eslint/recommended"
-      ],
-      "parser": "@typescript-eslint/parser",
-      "plugins": ["@typescript-eslint"],
-      "rules": {
-        "@typescript-eslint/no-explicit-any": "off",
-        "@typescript-eslint/ban-ts-ignore": "off",
-        "@typescript-eslint/explicit-function-return-type": "off",
-        "@typescript-eslint/no-non-null-assertion": "off",
-        "@typescript-eslint/no-var-requires": "off"
-      }
-    }
-  ]
+  "ignorePatterns": [
+     "**/__generated__/**/*"
+  ],
+  "extends": [
+      "plugin:@lifeomic/eslint-plugin-node/recommended",
+      "eslint:recommended",
+      "plugin:@typescript-eslint/eslint-recommended",
+      "plugin:@typescript-eslint/recommended"
+  ],
+  "parser": "@typescript-eslint/parser",
+  "plugins": ["@typescript-eslint"],
+  "rules": {
+    "@typescript-eslint/no-explicit-any": "off",
+    "@typescript-eslint/ban-ts-ignore": "off",
+    "@typescript-eslint/explicit-function-return-type": "off",
+    "@typescript-eslint/no-non-null-assertion": "off",
+    "@typescript-eslint/no-var-requires": "off"
+  }
 }

.gitignore

@@ -3,3 +3,4 @@ node_modules/
 
 src/**/*.js
 test/**/*.js
+**/__generated__/

.npmignore

@@ -3,4 +3,7 @@ test/
 .travis*
 TODO.MD
 .nyc_output/
-images/
+images/
+.github/
+.eslintrc.yml
+.codegen.yml

codegen.yml

@@ -0,0 +1,9 @@
+overwrite: true
+schema: ./test/**/*.graphql
+generates:
+  test/__generated__/graphql.ts:
+    plugins:
+      - typescript
+    config:
+      useIndexSignature: true
+      namingConvention: keep

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@lifeomic/graphql-resolvers-xray-tracing",
-  "version": "4.0.0",
+  "version": "5.0.0",
   "description": "A GraphQL middleware to enable X-Ray tracing subsegments for GraphQL resolvers",
   "homepage": "https://github.com/lifeomic/graphql-resolvers-xray-tracing#readme",
   "bugs": {
@@ -12,10 +12,15 @@
   },
   "scripts": {
     "lint": "eslint . -f codeframe",
+    "pretypes": "yarn cleanTypes",
+    "types": "graphql-codegen",
+    "pretest": "yarn types",
     "test": "nyc ava",
     "posttest": "yarn lint && tsc --noEmit",
+    "postinstall": "if [ -d test ]; then yarn types; fi",
     "coverage": "nyc report --reporter=text-lcov > ./.nyc_output/lcov.info",
-    "prepublishOnly": "yarn tsc"
+    "prepublishOnly": "yarn tsc",
+    "cleanTypes": "rm -rf test/__generated__"
   },
   "main": "src/traceResolvers.js",
   "types": "src/traceResolvers.ts",
@@ -32,6 +37,9 @@
   "author": "LifeOmic <development@lifeomic.com>",
   "license": "MIT",
   "devDependencies": {
+    "@graphql-codegen/cli": "1.2.1",
+    "@graphql-codegen/typescript": "1.2.1",
+    "@graphql-toolkit/file-loading": "^0.10.7",
     "@lifeomic/eslint-plugin-node": "^2.0.1",
     "@types/graphql": "^14.0.7",
     "@types/promise-retry": "^1.1.3",
@@ -81,6 +89,6 @@
     "access": "public"
   },
   "peerDependencies": {
-    "aws-xray-sdk-core": "^2.0.0 || ^3.0.0"
+    "aws-xray-sdk-core": "^2.5.0 || ^3.0.0"
   }
 }

src/traceResolvers.ts

@@ -2,7 +2,6 @@ import { applyMiddlewareToDeclaredResolvers } from 'graphql-middleware';
 import AWSXRay from 'aws-xray-sdk-core';
 import { GraphQLResolveInfo, GraphQLSchema, ResponsePath } from 'graphql';
 import { GraphQLSchemaWithFragmentReplacements, IMiddlewareResolver } from 'graphql-middleware/dist/types';
-const isPromise = require('is-promise');
 
 function fieldPathFromInfo (info: GraphQLResolveInfo) {
   let path: ResponsePath | undefined = info.path;
@@ -19,20 +18,15 @@ export default <TSource = any, TContext = any, TArgs = any>(schema: GraphQLSchem
   const tracer: IMiddlewareResolver<TSource, TContext, TArgs> = async (resolver, parent, args, ctx, info) => {
     const fieldPath = fieldPathFromInfo(info);
     return AWSXRay.captureAsyncFunc(`GraphQL ${fieldPath}`, async (subsegment) => {
-      const result = resolver();
-
-      // When AWS_XRAY_CONTEXT_MISSING is set to LOG_MISSING and no context was
-      // found, then the subsegment will be null and nothing should be done
-      if (subsegment) {
-        if (isPromise(result)) {
-          result.then(() => subsegment.close(), (error: Error | any) => {
-            subsegment.close(error);
-          });
-        } else {
-          subsegment.close();
-        }
+      // When AWS_XRAY_CONTEXT_MISSING is set to LOG_MISSING and no context is found then subsegment is undefined.
+      try {
+        const result = await resolver();
+        subsegment?.close();
+        return result;
+      } catch (error: any) {
+        subsegment?.close(error);
+        throw error;
       }
-      return result;
     });
   };
 

test/helpers/schema.graphql

@@ -0,0 +1,17 @@
+type Parent {
+    name: String!
+}
+
+type Query {
+    hello: String!
+    throwsSynchronously: String!
+    throwsAsynchronously: String!
+    waitFor(id: String!): Boolean
+    parent: Parent!
+}
+
+type Mutation {
+    createBlocking: String!
+    resolve(id: String!): Boolean
+    reject(id: String!): Boolean
+}

test/helpers/schema.ts

@@ -1,5 +1,8 @@
 import { makeExecutableSchema } from 'graphql-tools';
 import { v4 as uuid } from 'uuid';
+import { loadFiles } from '@graphql-toolkit/file-loading';
+import path from 'path';
+import traceResolvers from '../../src/traceResolvers';
 
 const blocked = new Map();
 
@@ -11,6 +14,10 @@ function throwsSynchronously () {
   throw new Error('Some error');
 }
 
+function throwsAsynchronously () {
+  throw new Error('Some error');
+}
+
 function createBlocking () {
   const id = uuid();
 
@@ -62,6 +69,7 @@ const resolvers = {
   Query: {
     hello,
     throwsSynchronously,
+    throwsAsynchronously,
     waitFor,
     parent
   },
@@ -72,23 +80,10 @@ const resolvers = {
   }
 };
 
-const typeDefs = `
-type Parent {
-  name: String!
-}
-
-type Query {
-  hello: String!
-  throwsSynchronously: String!
-  waitFor(id: String!): Boolean
-  parent: Parent!
-}
-
-type Mutation {
-  createBlocking: String!
-  resolve(id: String!): Boolean
-  reject(id: String!): Boolean
+export function traceSchema () {
+  const schema = makeExecutableSchema({
+    typeDefs: loadFiles(path.join(__dirname, '**/*.graphql')),
+    resolvers
+  });
+  return traceResolvers(schema);
 }
-`;
-
-export default makeExecutableSchema({ typeDefs, resolvers });

test/traceResolvers-contextMissing.test.ts

@@ -1,9 +1,9 @@
-import traceResolvers from '../src/traceResolvers';
-import { graphql } from 'graphql';
-import schema from './helpers/schema';
+import { graphql, GraphQLError, Source } from 'graphql';
+import { traceSchema } from './helpers/schema';
 import nock from 'nock';
 import anyTest, { TestInterface } from 'ava';
 import AWSXRay from 'aws-xray-sdk-core';
+import { ExecutionResult, ExecutionResultDataDefault } from 'graphql/execution/execute';
 AWSXRay.setContextMissingStrategy('LOG_ERROR');
 AWSXRay.setLogger({
   debug: () => undefined,
@@ -13,17 +13,24 @@ AWSXRay.setLogger({
 });
 AWSXRay.capturePromise();
 
+type GraphQlQuery = Parameters<typeof graphql>[1];
+
+const source = new Source('', '', {
+  column: 3,
+  line: 1
+});
+
 interface TestContext {
-  graphql: (query: Parameters<typeof graphql>[1]) => ReturnType<typeof graphql>;
+  graphql: <TData = ExecutionResultDataDefault>(query: GraphQlQuery) => Promise<ExecutionResult<TData>>;
 }
 
 const test = anyTest as TestInterface<TestContext>;
 
 test.beforeEach((t) => {
   nock.disableNetConnect();
   nock.enableNetConnect('127.0.0.1');
+  const schema = traceSchema();
 
-  traceResolvers(schema);
   t.context.graphql = (query) => graphql(schema, query);
 });
 
@@ -45,3 +52,21 @@ test('Traced resolvers can return a value', async (t) => {
     }
   });
 });
+
+test('Traced resolvers will throw exceptions when throwsSynchronously', async (t) => {
+  const { graphql } = t.context;
+  const result = await graphql('{ throwsSynchronously }');
+
+  const expected = new GraphQLError('Some error', undefined, source, [2], ['throwsSynchronously']);
+  t.is(result.errors?.length, 1);
+  t.deepEqual(result.errors, [expected]);
+});
+
+test('Traced resolvers will throw exceptions when throwsAsynchronously', async (t) => {
+  const { graphql } = t.context;
+  const result = await graphql('{ throwsAsynchronously }');
+
+  const expected = new GraphQLError('Some error', undefined, source, [2], ['throwsAsynchronously']);
+  t.is(result.errors?.length, 1);
+  t.deepEqual(result.errors, [expected]);
+});

test/traceResolvers.test.ts

@@ -1,32 +1,43 @@
-import traceResolvers from '../src/traceResolvers';
 import { graphql } from 'graphql';
-import schema from './helpers/schema';
+import { traceSchema } from './helpers/schema';
 import nock from 'nock';
 import anyTest, { ExecutionContext, TestInterface } from 'ava';
 import AWSXRay, { Segment, Subsegment } from 'aws-xray-sdk-core';
 import retryPromise from 'promise-retry';
+import { ExecutionResult, ExecutionResultDataDefault } from 'graphql/execution/execute';
+import { Mutation } from './__generated__/graphql';
 
 AWSXRay.capturePromise();
 
+type GraphQlQuery = Parameters<typeof graphql>[1];
+type Namespace = ReturnType<typeof AWSXRay.getNamespace>;
+
 interface TestContext {
+  ns: Namespace;
   segment: Segment;
-  graphql: (query: string) => Promise<any>;
+  graphql: <TData = ExecutionResultDataDefault>(query: GraphQlQuery) => Promise<ExecutionResult<TData>>;
 }
 
 const test = anyTest as TestInterface<TestContext>;
 
 test.beforeEach(function (test) {
+  const schema = traceSchema();
   nock.disableNetConnect();
   nock.enableNetConnect('127.0.0.1');
 
-  const ns = AWSXRay.getNamespace();
-  const segment = new AWSXRay.Segment('parent');
+  const ns: Namespace = AWSXRay.getNamespace();
+  test.context.ns = ns;
 
+  const segment = new AWSXRay.Segment('parent');
   test.context.segment = segment;
-  traceResolvers(schema);
-  test.context.graphql = ns.bind(function (query: string) {
+
+  test.context.graphql = ns.bind(function (query: GraphQlQuery) {
     AWSXRay.setSegment(segment);
-    return graphql(schema, query);
+    try {
+      return graphql(schema, query);
+    } finally {
+      segment.close();
+    }
   });
 });
 
@@ -84,11 +95,22 @@ test('Trace segments are reported as errors when resolver throws an error synchr
   test.truthy(segment.subsegments![0].fault);
 });
 
+test('Trace segments are reported as errors when resolver throws an error asynchronously', async function (test) {
+  const { segment, graphql } = test.context;
+  await graphql('{ throwsAsynchronously }');
+
+  test.is(segment.subsegments?.length, 1);
+  test.is(segment.subsegments![0].name, 'GraphQL throwsAsynchronously');
+  test.falsy(segment.subsegments![0].in_progress);
+  // @ts-ignore
+  test.truthy(segment.subsegments![0].fault);
+});
+
 async function testAsyncResolver (test: ExecutionContext<TestContext>, unblockQueryBuilder: (id: string) => string): Promise<Subsegment> {
   const { segment, graphql } = test.context;
 
-  const createResult = await graphql('mutation { createBlocking }');
-  const blockedId = createResult.data.createBlocking;
+  const createResult = await graphql<Mutation>('mutation { createBlocking }');
+  const blockedId = createResult.data!.createBlocking;
 
   test.is(segment.subsegments?.length, 1);
   test.is(segment.subsegments![0].name, 'GraphQL createBlocking');