GraphQLError: fix empty locations if error got nodes without locati…

…ons (#3325)
graphql · Oct 20, 2021 · b936411 · b936411
1 parent a3d215b
commit b936411
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 44 deletions.
diff --git a/src/error/GraphQLError.ts b/src/error/GraphQLError.ts
@@ -1,7 +1,7 @@
 import { isObjectLike } from '../jsutils/isObjectLike';
 import type { Maybe } from '../jsutils/Maybe';
 
-import type { ASTNode } from '../language/ast';
+import type { ASTNode, Location } from '../language/ast';
 import type { Source } from '../language/source';
 import type { SourceLocation } from '../language/location';
 import { getLocation } from '../language/location';
@@ -92,65 +92,40 @@ export class GraphQLError extends Error {
     this.originalError = originalError ?? undefined;
 
     // Compute list of blame nodes.
-    this.nodes = Array.isArray(nodes)
-      ? nodes.length !== 0
-        ? nodes
-        : undefined
-      : nodes
-      ? [nodes]
-      : undefined;
+    this.nodes = undefinedIfEmpty(
+      Array.isArray(nodes) ? nodes : nodes ? [nodes] : undefined,
+    );
+
+    const nodeLocations = undefinedIfEmpty(
+      this.nodes
+        ?.map((node) => node.loc)
+        .filter((loc): loc is Location => loc != null),
+    );
 
     // Compute locations in the source for the given nodes/positions.
-    this.source = source ?? undefined;
-    if (!this.source && this.nodes) {
-      this.source = this.nodes[0].loc?.source;
-    }
+    this.source = source ?? nodeLocations?.[0]?.source;
 
-    if (positions) {
-      this.positions = positions;
-    } else if (this.nodes) {
-      const positionsFromNodes = [];
-      for (const node of this.nodes) {
-        if (node.loc) {
-          positionsFromNodes.push(node.loc.start);
-        }
-      }
-      this.positions = positionsFromNodes;
-    }
-    if (this.positions && this.positions.length === 0) {
-      this.positions = undefined;
-    }
+    this.positions = positions ?? nodeLocations?.map((loc) => loc.start);
 
-    if (positions && source) {
-      this.locations = positions.map((pos) => getLocation(source, pos));
-    } else if (this.nodes) {
-      const locationsFromNodes = [];
-      for (const node of this.nodes) {
-        if (node.loc) {
-          locationsFromNodes.push(getLocation(node.loc.source, node.loc.start));
-        }
-      }
-      this.locations = locationsFromNodes;
-    }
+    this.locations =
+      positions && source
+        ? positions.map((pos) => getLocation(source, pos))
+        : nodeLocations?.map((loc) => getLocation(loc.source, loc.start));
 
     const originalExtensions = isObjectLike(originalError?.extensions)
       ? originalError?.extensions
       : undefined;
-    // TODO: merge `extensions` and `originalExtensions`
     this.extensions = extensions ?? originalExtensions ?? Object.create(null);
 
     // Include (non-enumerable) stack trace.
+    // istanbul ignore next (See: 'https://github.com/graphql/graphql-js/issues/2317')
     if (originalError?.stack) {
       Object.defineProperty(this, 'stack', {
         value: originalError.stack,
         writable: true,
         configurable: true,
       });
-      return;
-    }
-
-    // istanbul ignore next (See: 'https://github.com/graphql/graphql-js/issues/2317')
-    if (Error.captureStackTrace) {
+    } else if (Error.captureStackTrace) {
       Error.captureStackTrace(this, GraphQLError);
     } else {
       Object.defineProperty(this, 'stack', {
@@ -208,6 +183,12 @@ export class GraphQLError extends Error {
   }
 }
 
+function undefinedIfEmpty<T>(
+  array: Array<T> | undefined,
+): Array<T> | undefined {
+  return array === undefined || array.length === 0 ? undefined : array;
+}
+
 /**
  * See: https://spec.graphql.org/draft/#sec-Errors
  */

diff --git a/src/error/__tests__/GraphQLError-test.ts b/src/error/__tests__/GraphQLError-test.ts
@@ -100,6 +100,21 @@ describe('GraphQLError', () => {
     });
   });
 
+  it('converts node without location to undefined source, positions and locations', () => {
+    const fieldNodeNoLocation = {
+      ...fieldNode,
+      loc: undefined,
+    };
+
+    const e = new GraphQLError('msg', fieldNodeNoLocation);
+    expect(e).to.deep.include({
+      nodes: [fieldNodeNoLocation],
+      source: undefined,
+      positions: undefined,
+      locations: undefined,
+    });
+  });
+
   it('converts source and positions to locations', () => {
     const e = new GraphQLError('msg', null, source, [6]);
     expect(e).to.deep.include({

diff --git a/src/validation/__tests__/validation-test.ts b/src/validation/__tests__/validation-test.ts
@@ -142,7 +142,6 @@ describe('Validate: Limit maximum number of validation errors', () => {
   function invalidFieldError(fieldName: string) {
     return {
       message: `Cannot query field "${fieldName}" on type "QueryRoot".`,
-      locations: [],
     };
   }