Better error reporting of cloud code bugs

[sarsonj]

Our app is heavily using cloud code. It is sometimes complex and there can be bugs.

However, currently it is difficult to get errors easily.

In logs, the stack trace is useless - it doesn't show, where the bug appears, like:

error: Failed running cloud function getXmppServer for user MNi5llzPW1 with:
   Input: {}
   Error: {"message":"dsadsa is not defined","code":141} {"functionName":"getXmppServer","error":{"message":"dsadsa is not defined","code":141},"params":{},"user":"MNi5llzPW1"}
 error: Parse error: dsadsa is not defined {"code":141,"stack":"Error: dsadsa is not defined\n    at error (/opt/app/node_modules/parse-server/lib/Routers/FunctionsRouter.js:121:16)\n    at process._tickCallback (internal/process/next_tick.js:68:7)"}

I don't care about FunctionsRouter.js, I want to get right place of error, that is currently getXmppServer.js line 4. It would be nice to have correct stack trace in log.

I also want to integrace Parse server with external tool, that collects app exeptions, like Sentry. Sentry have support for express.js, however, it doesn't currently catch Cloud Code exceptions. Is there any way to do this?

Is your feature request related to a problem? Please describe.
It is very difficult to debug Cloud Code bugs when we don't know, where the bug appear.

Describe the solution you'd like
Show correct file / line number where bug appears and allow to handle this bug by any express.js middleware.

[sarsonj]

I realized, that error is passed to express.js middleware, when configuration option on Parse server enableExpressErrorHandler is used. Cool. Second problem solved, I have my errors in Sentry.

However, the stack trace looks useless:

[omairvaiyani]

My team are currently tackling this issue, though we do capture information before it bubbles to Parse. For us, the issue is that we lose some information when it's dispatched back to the requesting client. I wasnt aware of the enableExpressErrorHandler option. I'll comment here our findings tomorrow and consider a PR if needed. But please be aware that sending stack trace information is not a good idea inside the request object, but it would be helpful to have within Sentry logs.

[omairvaiyani]

It appears that parse server is poised to only dispatching Parse.Error instances, a stance which I understand the reasoning behind given the history of the project.

The enableExpressErrorHandler allows developers to manage any exceptions before they reach the requesting client, which is a great start, however the exceptions are intercepted by the parse server codebase before reaching our exception handling express middleware.

As an example, if our cloud function throws an error of the instance BadRequestException with our own code 1234, this error will be caught by Parse in FunctionsRouter, and re-thrown as a Parse.Error, losing any rich data, including the stacktrace, instance type, instance variables, etc, before reaching our middleware.

A similar pattern occurs for errors thrown in beforeSave et al. hooks within
Triggers.

I imagine there are other scenarios where requests created by the user, which result in exceptions thrown by our code, are intercepted and re-thrown by parse server. We would need to identify these scenarios as they will form scope of any resultant fix.

Errors thrown within parse server, for example, if a request to a non-existent cloud function is received, would not have to change as part of this fix, as these are predetermined scenarios which developers should handle within their middleware.

Use case:

• Better error logging: the LoggerAdapter currently receives these intercepted errors where all rich information thrown within an application is lost
• Customised error codes: the error codes available in Parse.Error are mostly related to issues within Parse, whilst any application specific errors are converted to SCRIPT_FAILED/141. This leaves little room for custom handling of business specific problems, leaving developers to only use the message property.
• Custom error handling: by maintaining the original error instance inside the application's exception middleware, we can restrict what errors are displayed to the client, and what is not. Currently, the only way to do this is by using the message property to distinguish the types of error. This is counterproductive as the message string should only serve the purpose of human reading, not programmatic decisioning.

Solution 1 (non breaking):
Introduce an option into the parse config which disables error masking, i.e., we check this option within Trigger, FunctionsRouter and other identified locations, and simply forward the error as is downstream.

Possible problems:

• May break downstream within parse server if handlers expect Parse.Error instances
• May be difficult to get the options information for the application within these identified locations

Solution 2 (breaking for applications with enableExpressErrorHandler:true):
Completely remove this behaviour of re-throwing errors within these locations - and only re-throw if enableExpressErrorHandler is disabled. Therefore, the re-throw would only occur in one location within parse server: at parse server's own exception middleware.

Possible problems:

• Users with enableExpressErrorHandler:true will have to update their middleware to be aware of this change

Solution 3 (in next comment)

--
Both solutions may have further downstream effects in client-side SDKs, which expect a Parse.Error style response. This would likely limit the usage to REST/GraphQL users only.

In either case - we'll test out these solutions in a fork, but will definitely need input from the community before a possible PR.

[omairvaiyani]

We have a third-solution which is more of a workaround, but appears to be a quick change to our codebase that requires no PR for parse-server!

Solution 3
Wrap our application's request handlers with a try/catch that re-throws our exceptions as Parse.Error instances - but then attach our original error as an instance property on the Parse.Error object. This new error goes through the parse server internals, and emerges unchanged in our express middleware, at which point we can extract the original error.

This behaviour assumes that the internal code within Parse does not:
1 - re-throw Parse.Error instances
2 - breaks if unexpected properties within Parse.Error objects are found
Neither of these problems have surfaced yet, but both will need test-cases in your application to run every time your upgrade parse server.

Currently unknown - how this effects GraphQL. We'll tackle this later.

[sarsonj]

Thanks @omairvaiyani for Solution 3.

I already tried similar approach - to try / catch our cloudCode functions and put exception to Sentry directly, however I had problem that exception was not catch. Probably I did something wrong. Your re-throw approach is much better, because then error goes to logs as well.

Could you please send example of simple cloudCode function that do this (Solution 3)? I would like to test this on our codebase, thanks!

[omairvaiyani]

@sarsonj No problem glad our problem is also helping out yours. The proposed third solution does currently have a limitation in that parse-server is still sending the response back to the requesting client before sending the error to our middleware. This means that the enableExpressErrorHandler:true option only helps with logging, but not with customising the error response payload. Therefore, this solution only meets 1 of our 2 criteria. We'll be forking the library and battle testing our changes before submitting a PR.

Example usage for you:

const requestWrapper = (requestHandler) => {
  return async (request) => {
  try {
    const response = await requestHandler(request);
    return response;
  } catch (e) {
    const parseError = new Parse.Error(e.code, e.message);
    parseError._original = e;
    throw parseError;
  }
  }
}

Parse.Cloud.define('foo',  requestWrapper(fooHandler) );
Parse.Cloud.beforeSave('_User',  requestWrapper(userBeforeSaveHandler) );

[sarsonj]

It's more like a hack, but I added console.log(e) and now in sentry I have content of orginal exception, because it shows recent events in timeline. It also appears in app log itself.

So that big improvement for us, hope this exeption propagation will be implemented in the core.

[omairvaiyani]

I believe your own workaround along with the suggestions I described in Solution 3 above are sufficient for now. Closing issue but for other contributors, there's probably some useful insights here to consider.