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

Stitching with formatError is very limited... #480

Closed
j opened this issue Nov 10, 2017 · 37 comments
Closed

Stitching with formatError is very limited... #480

j opened this issue Nov 10, 2017 · 37 comments

Comments

@j
Copy link

j commented Nov 10, 2017

If you want to format your errors when using stitching, you can't tell the error type from the error.

The issue seems to be located here:

https://github.com/apollographql/graphql-tools/blob/master/src/stitching/errors.ts#L80

So packages like apollo-errors don't work because you can't get the type from it.

One solution I think I see that's somewhat possible is to check if it's a schema created for a remote server and if so, run checkResultAndHandleErrors, otherwise, just throw the error as is since checkResultAndHandleErrors looks to be made for remote responses, and if it's local, the errors should just be thrown normally

https://github.com/apollographql/graphql-tools/blob/9ac0aeb25cd6c99d2e1ce6c14809d64f85b87eaf/src/stitching/mergeSchemas.ts#L322-L330

Thoughts?

@alexFaunt
Copy link

@j ran into this issue yesterday I think we can get around it easily by maintaining the original error in a merged error. Created a quick PR at #485 as we needed a workaround. But happy to help if there's a more in depth solution required.

@vjpr
Copy link

vjpr commented Jan 29, 2018

I ran into this today. Needs a fix. Very hard to debug.

I had a subscription field returning a list which was returning this in graphql: [null, null, null].

The issue was one of the fields in the list items missing. There is no way to see the error without turning off stitching.

This should be labelled as bug, not enhancement.

@stringbeans
Copy link
Contributor

i agree with @vjpr on this one. this issue should be marked as a bug and not an enhancement (@freiksenet )

im surprised there aren't more people complaining about this issue. is there a workaround that we're not aware of? how are other people debugging their local development with schema stitching when the stack traces are useless?

@jwb
Copy link

jwb commented Feb 26, 2018

This is a bug. It's impacting us.

@legopin
Copy link

legopin commented Feb 27, 2018

This is also impacting us.

We were unable to pass custom error codes because the merged schema swallows the thrown errors.

@suciuvlad
Copy link

+1

@totorokk
Copy link

totorokk commented Apr 3, 2018

We've also run into this, and had to turn off stitching to debug.

@oliviertassinari
Copy link

The issue can be closed given #637 was merged an released. I had to add some ugly error drill-down logic, at least, it's working.

@DavidStummer
Copy link

DavidStummer commented May 1, 2018

I am still having issues viewing any extra properties associated with an error inside formatError:

    const e = new Error('An Error...');
    e.data = 'Error data...';
    throw e;

Inside formatError I would expect to find the data property but I don't:

export const formatError = error => {
    const {originalError} = error;
    console.log(originalError.errors[0]);
    return error;
};

output:

{ Error: An Error...
    at _callee$ (/Users/davidstummer/code/DeliveriesOnDemand/ServiceLayer/app/admin/creditor/resolvers.js:12:15)
    ...
 message: 'An Error...',
  locations: [],
  path: [ 'pageCreditors' ] }

This only occurs when using mergeSchemas

Update: I managed to finally find my error object here:

error.originalError.errors[0].originalError

@pcorey
Copy link

pcorey commented Aug 20, 2018

I was also just bit by this. It took ~2 days to track down the issue. Locally generated errors should maintain their types and custom fields when passed into formatError. In my mind, this is a bug, not a missing feature.

@OlivierCuyp
Copy link

In my case, I have micro-services connected to an api gateway that makes the stitching.
The orignal error was deep buried: error.orignalError.errors[0].originalError.errors[0]
Here is how I solved the issue:

In User micro-service directrives/auth.js

const {
  SchemaDirectiveVisitor,
  AuthenticationError,
  ForbiddenError
} = require('apollo-server-express');

// ...
  if (!context.token) {
    throw new AuthenticationError('User not authenticated');
  }
// ...

In Api Gateway helpers/searchOriginalError.js

function searchOriginalError (error) {
  if (error.originalError) {
    return searchOriginalError(error.originalError);
  }
  if (error.errors) {
    return error.errors.map(searchOriginalError)[0];
  }
  return error;
}

module.exports = { searchOriginalError };

In Api Gateway index.js

const server = new ApolloServer({
  schema,
  context: ({ req }) => {
    // ...
  },
  formatError: (error) => {
    const originalError = searchOriginalError(error);
    logger.error(originalError);
    return originalError;
  }
});

I hope, this helps.

@krisread
Copy link

krisread commented Sep 6, 2018

For us, using formatError and simply returning the error with NO changes impacts behavior. That's silly right? E.g.

formatError: (error) => error

This ^ behaves differently than no formatError

We are using schema stitching... it seems like data is stripped or removed from the errors only if we have a formatError function defined.

@hwillson hwillson added bug and removed enhancement labels Sep 7, 2018
@todkap
Copy link

todkap commented Oct 18, 2018

My team has been playing with stitching and attempted many of the suggestions above (and related issues). We appear to be hitting the issue of exceptions being swallowed versus having to unwrap. Has anyone actually figured out how to override the internals of graphql-tools to monkeypatch the code to work around this?

If we add debug console output for the unwrap,

function searchOriginalError(error) {
    console.log('error', error);
    if (error.originalError) {
        return searchOriginalError(error.originalError);
    }
    if (error.errors) {
        return error.errors.map(searchOriginalError)[0];
    }
    return error;
}

we see the following..

error { [GraphQLError: [object Object]]
  message: '[object Object]',
  locations: [ { line: 2, column: 3 } ],
  path: [ 'createUser' ],
  extensions:
   { code: 'INTERNAL_SERVER_ERROR',
     exception: { errors: [Array], stacktrace: [Array] } } }
error { Error: [object Object]
    at new CombinedError (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql-tools/dist/stitching/errors.js:82:28)
    at Object.checkResultAndHandleErrors (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql-tools/dist/stitching/errors.js:98:15)
    at CheckResultAndHandleErrors.transformResult (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql-tools/dist/transforms/CheckResultAndHandleErrors.js:9:25)
    at /Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql-tools/dist/transforms/transforms.js:18:54
    at Array.reduce (<anonymous>)
    at applyResultTransforms (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql-tools/dist/transforms/transforms.js:17:23)
    at /Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql-tools/dist/stitching/delegateToSchema.js:93:50
    at step (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql-tools/dist/stitching/delegateToSchema.js:31:23)
    at Object.next (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql-tools/dist/stitching/delegateToSchema.js:12:53)
    at fulfilled (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql-tools/dist/stitching/delegateToSchema.js:3:58)
  errors:
   [ { Error: [object Object]
         at asErrorInstance (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql/execution/execute.js:541:43)
         at process._tickCallback (internal/process/next_tick.js:68:7) message: '[object Object]', locations: [], path: [Array] } ] }
error { Error: [object Object]
    at asErrorInstance (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql/execution/execute.js:541:43)
    at process._tickCallback (internal/process/next_tick.js:68:7)
  message: '[object Object]',
  locations: [],
  path: [ 'createUser' ] }
error Error: [object Object]
    at asErrorInstance (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql/execution/execute.js:541:43)
    at process._tickCallback (internal/process/next_tick.js:68:7)
[2018-10-18T08:32:03.600] [ERROR] server.js - Error: [object Object]
    at asErrorInstance (/Users/todd/Documents/workspace/token-factory-graphql/node_modules/graphql/execution/execute.js:541:43)
    at process._tickCallback (internal/process/next_tick.js:68:7)

The result to the client request becomes

{
  "data": {
    "createUser": null
  },
  "errors": [
    {}
  ]
}

@smber1
Copy link

smber1 commented Dec 5, 2018

Unfortunately @OlivierCuyp's solution didn't work in our case either. The upstream service errors weren't being exposed to formatError.

This flow, (flow 1) makeRemoteExecutableSchema => createResolver => checkResultsAndHandleErrors raised the upstream error.

However the subsequent (flow 2) MergeSchemas => delegateToSchema => applies CheckResultsAndHandleErrors as transform raised an error that had lost the upstream error context.

Narrowed it down to this line in checkResultAndHandleErrors. When this error is thrown, it's received by the mergeSchemas flow but has lost the real error context.

Workaround was to add an error handler to the httpLink that appended an additional error so that a CombinedError got thrown from checkResultAndHandleErrors like so

const errorLink = onError(({ graphQLErrors, response }) => {
  if (graphQLErrors && graphQLErrors.length === 1) {
    response.errors = graphQLErrors.concat(new ApolloError("hax0r"));
  }
});

Then flow 2 received an error with the right context that was then conveyed to formatError.

@januszhou
Copy link

@smber1 thanks for sharing it, it works for me

@owenallenaz
Copy link
Contributor

Hmmm... I've tried almost all of the approaches outlined in this thread and I just can't get any of them to work. In my system we have a main API gateway which links to other schemas. The flow we're following is pretty close to the docs

let remoteLink = new HttpLink({ uri : link.path, fetch });
let remoteSchema  = await introspectSchema(remoteLink);
validateSchema({ schema : remoteSchema, prefix : link.name });
let remoteExecutableSchema = makeRemoteExecutableSchema({
	schema : remoteSchema,
	link : remoteLink
});
// later
var schema = mergeSchemas({ schemas });

I am able to access the errors with the proper stack trace if I utilize the apollo-link-error. The problem is that I'm only able to console.log them, and can never return them to the screen. I tried appending to the reponse.errors as outlined by @smber1 but they would not appear in my formatError method. I was able to access an error.originalError but the stacktrace would be overwritten at that point and there was never an error.originalError.errors as some other comments have alluded to.

Right now apollo has settings so that it will display stacktraces based on debug or NODE_ENV. It feels like if stacktraces are enabled, then by default they should display in such a way that we can determine exactly which link they originated from and preserving their stack trace. That seems like what most people above are looking for, and it cases where people want to override that default behavior there is the formatError method. If the apollo-link-error package is meant to solve this problem then I think the docs need to be extrapolated a lot more. Right now they don't even show on how to integrate it with an existing schema stitched system. Now do they show things like utilizing response.errors so it's not clear if that's even an official pathway.

@micksi
Copy link

micksi commented Mar 27, 2019

Not saying this is the best solution but we have used an approach where we create a custom formatError for each apollo server instance unpacks the error and throws it again as an ApolloError

const stringify = require('json-stringify-safe');

const defaultFormatErrorHandler = (error) => {
    const err = JSON.parse(stringify(error));
    console.log(err);
    delete err.extensions.exception.config;
    delete err.extensions.exception.request;
    delete err.extensions.exception.response;
    return new ApolloError('ERROR', err.extensions.code, { exception: { ...err } });
  };

new Apollo({
    formatError: defaultFormatErrorHandler,
    ...
})

It is very hacky and the actual error gets buried one level deeper than necessary but it does the job of preserving the error code and the message in the first error layer which can then be inspected by the client.

Example error from our implementation

{
  "errors": [
    {
      "locations": [],
      "path": [
        "updateSektion"
      ],
      "extensions": {
        "code": "SECTOR9_NOT_LOGGED_IN_ERROR",
        "exception": {
          "exception": {
            "message": "Principal is not logged in",
            "locations": [
              {
                "line": 2,
                "column": 3
              }
            ],
            "path": [
              "updateSektion"
            ],
            "extensions": {
              "code": "SECTOR9_NOT_LOGGED_IN_ERROR",
              "exception": {
                "invalidJWT": true,
                "stacktrace": [
                  "Error: Principal is not logged in",
                  "    at Principal.requireLogIn (/home/node/app/node_modules/sector9/build/Principal.js:73:19)",
                  "    at exports.updateSektionMutation (/home/node/app/build/graphql/schemas/sektion/mutation.js:5:10)",
                  "    at field.resolve (/home/node/app/node_modules/apollo-server-core/node_modules/graphql-extensions/dist/index.js:128:26)",
                  "    at resolveFieldValueOrError (/home/node/app/node_modules/graphql/execution/execute.js:479:18)",
                  "    at resolveField (/home/node/app/node_modules/graphql/execution/execute.js:446:16)",
                  "    at /home/node/app/node_modules/graphql/execution/execute.js:262:18",
                  "    at /home/node/app/node_modules/graphql/jsutils/promiseReduce.js:32:10",
                  "    at Array.reduce (<anonymous>)",
                  "    at promiseReduce (/home/node/app/node_modules/graphql/jsutils/promiseReduce.js:29:17)",
                  "    at executeFieldsSerially (/home/node/app/node_modules/graphql/execution/execute.js:259:37)"
                ]
              }
            }
          },
          "stacktrace": [
            "GraphQLError",
            "    at Object.locatedError (/home/node/app/node_modules/graphql/error/locatedError.js:31:10)",
            "    at Object.checkResultAndHandleErrors (/home/node/app/node_modules/graphql-tools/dist/stitching/errors.js:99:23)",
            "    at /home/node/app/node_modules/graphql-tools/dist/stitching/makeRemoteExecutableSchema.js:159:52",
            "    at step (/home/node/app/node_modules/graphql-tools/dist/stitching/makeRemoteExecutableSchema.js:31:23)",
            "    at Object.next (/home/node/app/node_modules/graphql-tools/dist/stitching/makeRemoteExecutableSchema.js:12:53)",
            "    at fulfilled (/home/node/app/node_modules/graphql-tools/dist/stitching/makeRemoteExecutableSchema.js:3:58)",
            "    at process._tickCallback (internal/process/next_tick.js:68:7)"
          ]
        }
      }
    }
  ],
  "data": {
    "updateSektion": null
  }
}

@shreshthmohan
Copy link

shreshthmohan commented May 28, 2019

Those of you who just want to be able to use two endpoints can use Directional Composition. That is if you don't want to deal with the issues with stitching. But I think directional composition only works with two endpoints, not more.

apollographql/react-apollo#1863 (comment)
https://www.apollographql.com/docs/link/composition#directional

@kfrajtak
Copy link

Was this issue fixed? Thanks
I am using these packages

"apollo-server": "^2.4.8",
"apollo-server-core": "^2.5.0",
"apollo-server-express": "^2.4.8",

@yaacovCR
Copy link
Collaborator

This should be fixed in graphql-tools-fork. Please let me know if not working for you.

@kfrajtak
Copy link

kfrajtak commented Jun 28, 2019

Using graphql-tools-fork helped, but the error (Argument Validation Error in my case) is still being reported as INTERNAL_SERVER_ERROR, I would expect the code to be more specific as described here.

@yaacovCR
Copy link
Collaborator

If you could share a reproduction, I could take a look?

@kfrajtak
Copy link

Thanks for help, the problem is in the type-graphql library I am using that does not provide error codes.

@buuni
Copy link

buuni commented Jul 19, 2019

I use this:

  1. Create Error link, because Apollo Error handler loses flow. (thank @smber1 ). I am not creating a new error, you can increase the size of the array in another way. So, in formatError, we get an error completely.
import { onError } from 'apollo-link-error';

...

const ErrorLink = onError(({ graphQLErrors, response }) => {
	if (graphQLErrors && graphQLErrors.length === 1) {
		graphQLErrors.length = 2;
	}
});
  1. In formatError handler we generate new ApolloError`s from standard function in apollo-server-errors repo.
import { formatApolloErrors } from 'apollo-server-errors';

...

const formatError = (error) => {
	let errors = [];

	if(!error.originalError || !error.originalError.errors || error.originalError.errors.length <= 0) {
		return error;
	}

	error.originalError.errors.forEach((currentError) => {
		if(!currentError) return;
		let currentOriginal = currentError.originalError;

		const compiledErrors = formatApolloErrors(currentOriginal.errors || []);

		if(compiledErrors === null) {
			return;
		}

		errors.push(...compiledErrors.filter(err => !!err));
	});
	return (errors.length > 1) ? errors : errors[0];
};

For result: I throw Forbidden exception in the microservice and get response in the gateway service:

{
    "errors": [
        {
            "message": "Not authenticated as user.",
            "locations": [
                {
                    "line": 2,
                    "column": 3
                }
            ],
            "path": [
                "asset"
            ],
            "extensions": {
                "code": "FORBIDDEN",
                "exception": {
                    "stacktrace": [
                        "ForbiddenError: Not authenticated as user.",
                        "    at isAuthenticated (/usr/app/src/resolvers/authorization.js:5:32)",
                        "    at /usr/app/node_modules/graphql-resolvers/lib/combineResolvers.js:28:48"
                    ]
                }
            }
        }
    ],
    "data": {
        "asset": null
    }
}

And if exception thrown in the getaway service:

{
    "errors": [
        {
            "message": "Cannot query field \"asdasd\" on type \"Asset\". Did you mean \"alias\"?",
            "locations": [
                {
                    "line": 6,
                    "column": 9
                }
            ],
            "extensions": {
                "code": "GRAPHQL_VALIDATION_FAILED",
                "exception": {
                    "stacktrace": [
                        "GraphQLError: Cannot query field \"asdasd\" on type \"Asset\". Did you mean \"alias\"?",
                        "    at Object.Field (/usr/app/node_modules/graphql/validation/rules/FieldsOnCorrectType.js:53:31)",
                        "    at Object.enter (/usr/app/node_modules/graphql/language/visitor.js:324:29)",
                        "    at Object.enter (/usr/app/node_modules/graphql/language/visitor.js:375:25)",
                        "    at visit (/usr/app/node_modules/graphql/language/visitor.js:242:26)",
                        "    at Object.validate (/usr/app/node_modules/graphql/validation/validate.js:54:22)",
                        "    at validate (/usr/app/node_modules/apollo-server-core/src/requestPipeline.ts:426:22)",
                        "    at Object.<anonymous> (/usr/app/node_modules/apollo-server-core/src/requestPipeline.ts:250:32)",
                        "    at Generator.next (<anonymous>)",
                        "    at fulfilled (/usr/app/node_modules/apollo-server-core/dist/requestPipeline.js:4:58)",
                        "    at processTicksAndRejections (internal/process/task_queues.js:85:5)"
                    ]
                }
            }
        }
    ]
}

Unfortunately @OlivierCuyp's solution didn't work in our case either. The upstream service errors weren't being exposed to formatError.

This flow, (flow 1) makeRemoteExecutableSchema => createResolver => checkResultsAndHandleErrors raised the upstream error.

However the subsequent (flow 2) MergeSchemas => delegateToSchema => applies CheckResultsAndHandleErrors as transform raised an error that had lost the upstream error context.

Narrowed it down to this line in checkResultAndHandleErrors. When this error is thrown, it's received by the mergeSchemas flow but has lost the real error context.

Workaround was to add an error handler to the httpLink that appended an additional error so that a CombinedError got thrown from checkResultAndHandleErrors like so

const errorLink = onError(({ graphQLErrors, response }) => {
  if (graphQLErrors && graphQLErrors.length === 1) {
    response.errors = graphQLErrors.concat(new ApolloError("hax0r"));
  }
});

Then flow 2 received an error with the right context that was then conveyed to formatError.

@rmalca
Copy link

rmalca commented Jul 22, 2019

Has anyone tried this with apollo federation?

@yaacovCR
Copy link
Collaborator

Should be fixed by #1307.

@yaacovCR yaacovCR mentioned this issue Mar 29, 2020
22 tasks
@sgarner
Copy link

sgarner commented Jun 12, 2020

Not sure if it's related, but I encountered a similar issue where an error on a field in an underlying schema was being ignored by the stitched schema gateway. The gateway would try to return null for the entire object instead of passing through the error. This would then produce spurious errors that the object's id field can't be null, instead of the actual error from the underlying schema.

I am using [email protected], so this is not fixed by #1307.

It looks like this is caused by the code in handleNull at https://github.com/ardatan/graphql-tools/blob/v6.0.9/packages/delegate/src/results/handleNull.ts#L22 but I don't really understand well enough the thinking behind what that's trying to do, to say if it's wrong or not.

My solution for now has been to use a transformer that throws on all errors, like this:

import { CombinedError, stitchSchemas } from 'graphql-tools';

const throwOnAllResultErrors = {
  transformResult(result: any) {
    // always throw if an error is returned by the underlying schema
    if (result.errors != null && result.errors.length > 0) {
      throw new CombinedError(result.errors);
    }

    return result;
  }
};

stitchSchemas({
  subschemas: [
    {
      schema,
      transforms: [throwOnAllResultErrors],
    },
    {
      remoteSchema,
      transforms: [throwOnAllResultErrors],
    },
  ]
});

@yaacovCR
Copy link
Collaborator

Thanks for sharing your workaround.

It would be extremely helpful if you could also open a new issue with a minimal reproduction, including:

  1. minimally reproducing schema
  2. minimally reproducing query
  3. what is being returned by the subschema
  4. what is being returned by the gateway
  5. what you expect the gateway to return

Thanks!

@yaacovCR
Copy link
Collaborator

@sgarner

Key question is whether subschema provided path on error, see #1641 for example

@yaacovCR
Copy link
Collaborator

yaacovCR commented Jun 12, 2020

Your workaround would seem to prevent partial GraphQL response, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests