properly handle remote (POJO) errors

[alfaproject]

This is a test to reproduce issue #1571 as asked by @yaacovCR

I've allowed edits by maintainers, so feel free to use this PR to fix the issue or just close it if it's no longer necessary.

[yaacovCR]

I could be missing something, there is no path on generated error, how can it be matched to the null access token?

[alfaproject]

@yaacovCR I've added the path now, sorry for that.

[yaacovCR]

I think this is just a test setup issue, your tests should not just be objects with similar properties to GraphQL Errors, they should be actual error objects, because we return the actual objects to the graphql execution algorithm, which will automatically throw objects of type Error.

[alfaproject]

@yaacovCR maybe I'm missing something but this works fine in graphql-tools v4. The response from the network is not serialised into a GraphQLError. An example from an official Apollo link: https://github.com/apollographql/apollo-link/blob/master/packages/apollo-link-http/src/httpLink.ts

I can change my own custom link response by checking if there's an errors attribute in the body and if there is, I can convert each element in it to a GraphQLError but that just proves my point that this is a breaking change that wasn't mentioned.

[alfaproject]

It's a JSON.parse result from the network request which is a plain object. You guys actually have the same in your examples: https://www.graphql-tools.com/docs/remote-schemas/#using-cross-fetch

[yaacovCR]

If I am correct in terms of what populates the errors array from an execution results, this is a breaking change, but would only affect a testing environment, not a production environment with real GraphQL errors in that array. I think I would consider it a bug in the original implementation

[alfaproject]

After your comment, I tested what you said and it is now working as before, but I wasn't expecting having to do these changes. I honestly spent hours on this because I thought I did something wrong.

I've just changed my own custom link to this and it now works:

diff --git a/apps/lambda-api/src/apollo/apollo-link-lambda.ts b/apps/lambda-api/src/apollo/apollo-link-lambda.ts
index ebb16819e..bd76fb579 100644
--- a/apps/lambda-api/src/apollo/apollo-link-lambda.ts
+++ b/apps/lambda-api/src/apollo/apollo-link-lambda.ts
@@ -1,6 +1,6 @@
 import { Lambda } from '@tma/lambda-common/lambda';
 import { ApolloLink, Observable } from 'apollo-link';
-import { print } from 'graphql/language/printer';
+import { GraphQLError, print } from 'graphql';
 import { Container } from 'typedi';

 import { Context } from '../context';
@@ -14,6 +14,11 @@ export function createLambdaLink(serviceName: string, functionName: string) {
       lambda
         .invokeGraphQL(serviceName, functionName, print(operation.query), operation.variables, graphqlContext.headers)
         .then(result => {
+          if (result.errors) {
+            for (const error of result.errors) {
+              Object.setPrototypeOf(error, GraphQLError.prototype);
+            }
+          }
           observer.next(result);
           observer.complete();
         })

[alfaproject]

I'm not sure what you mean by a 'real' GraphQL error because the response that you get from the network is a plain object due to the fact that you apply JSON.parse on a string or a buffer or whatever else.

But if you want to get that plain object back to a class/function prototype then you have to do it after deserialising it like I just showed in the diff in my previous comment.

Basically:

const networkResponse = '{"errors":[{"extensions":{"code":"UNAUTHENTICATED","exception":{"stacktrace":[]}},"locations":[{"line":2,"column":3}],"message":"INVALID_CREDENTIALS","path":["login"]}],"data":null}';
const response = JSON.parse(networkResponse);

console.log(response.errors[0].constructor.name);
// Object

Object.setPrototypeOf(response.errors[0], GraphQLError.prototype);
console.log(response.errors[0].constructor.name);
// GraphQLError

[alfaproject]

I'm ok if you want to close it but I think you guys should put a note in the documentation at least, or maybe an example with a popular link like the Apollo HTTP link. In the meanwhile, I got the workaround working after your feedback, so all good. Thanks for helping. (:

[yaacovCR]

I thought about it on drive home and agree = bug. I think the link I was using basically converted to GraphQLError and so I didn't hit this, and test code pretending to be remote is not testing well enough with serialization and deserialization.

I think we should fix in this library rather than in fetchers.

Thanks for walking me through it.

Basically, you cannot rely on prototype of graphql response, agreed all?

[alfaproject]

Yes, I agree. (:

[yaacovCR]

Thanks @alfaproject -- this will hopefully improve our error handling tremendously.

[theguild-bot]

The latest changes of this PR are available as alpha in npm: 6.0.6-alpha-b493948.0

Quickly update your package.json by running:

npx match-version @graphql-tools 6.0.6-alpha-b493948.0