fix: ParseError message handling

[stewones]

New Pull Request Checklist

• I am not disclosing a vulnerability.
• I am creating this PR in reference to an issue.

Issue Description

The problem is that log is unclear when some random error happens in the server, eg: MongoDB query has exceeded memory limit

error ParseError: [object Object]
    at handleError (/my-project/node_modules/parse-server/node_modules/parse/lib/node/RESTController.js:418:17)
    at processTicksAndRejections (internal/process/task_queues.js:95:5) {
  code: 1
}

Approach

My proposal is just to make sure the error message is a string otherwise, stringify it.

So this would be the returned log for the example above

error ParseError: "{\"ok\":0,\"code\":292,\"codeName\":\"QueryExceededMemoryLimitNoDiskUseAllowed\"}"
    at handleError (/my-project/node_modules/parse-server/node_modules/parse/lib/node/RESTController.js:418:17)
    at processTicksAndRejections (internal/process/task_queues.js:95:5) {
  code: 1
}

Please note that it's strictly necessary making sure those logs are readable, especially when relying on monitoring services like GCP Error Reporting, New Relic, Data Dog, etc...

• Add tests
• Add changes to documentation (guides, repository pages, in-code descriptions)

[parse-github-assistant]

Thanks for opening this pull request!

• ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

[mtrezza]

error ParseError: "{"ok":0,"code":292,"codeName":"QueryExceededMemoryLimitNoDiskUseAllowed"}"

I would say that this is actually an unwanted error message style. [object object] is really an indication that an error is not properly logged, but just passed through from a dependency. So maybe it's not even a good idea to just stringily and output whatever is passed to ParseError.

The problem is that it's not clear whether this error message is from Parse Server, the Parse JS SDK or MongoDB. People may then search for "Parse Server error 292" and not find an answer for the issue, or worse references to a completely unrelated Parse Server error 292. Whereas "MongoDB error 292" would likely yield a better search result. It's also unclear for the developer what the "\"ok\":0,\ part means and what relevance it has, because we are just passing everything through without context.

Maybe the better solution is to properly write an error message and package the dependency error into it, like:

error ParseError: MongoDB error 292: QueryExceededMemoryLimitNoDiskUseAllowed

If the MongoDB errors vary in structure, then we may not be able to that; but we should at least add the error source, like:

error ParseError: MongoDB error: "{"ok":0,"code":292,"codeName":"QueryExceededMemoryLimitNoDiskUseAllowed"}"

Maybe we can also get rid of the quotes and format it more nicely, with util.inspect().

[stewones]

I think I get your point @mtrezza
let me see if I can improve it

[stewones]

this is how it's looking like now

[codecov]

Codecov Report

Base: 99.89% // Head: 99.89% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (09c54c6) compared to base (6125419).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##            alpha    #1619   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files          61       61           
  Lines        5973     5984   +11     
  Branches     1367     1373    +6     
=======================================
+ Hits         5967     5978   +11     
  Misses          6        6           

Impacted Files	Coverage Δ
src/ParseError.js	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mtrezza]

Going back, I think we should never throw an object directly without context. These edge cases should not even make it through a PR review, because looking at the spec, I would say that new Error(message) expects a string, not an object. From that I would say that ParseError(message) also expects message to be a string.

If we want to include the original error object for reference, we can use the options of the Error object, see this:

throw new Error("New error message", { cause: err });

It seems the issue is not that ParseError doesn't handle non-string values. The issue is that the code throws non-string values, which seems to be incorrect from a type perspective. I would say this PR doesn't need any of these object to string conversions. It should just fix how the error in thrown originally.

[stewones]

These edge cases should not even make it through a PR review

Yeah, I agree but we don't control third-party code running on our servers.

while I'm ok removing the edge case it doesn't resolve the issue. if it happens to Parse catch some error wrongly thrown, the server will still log ParseError: [object Object] giving no clue. Even worse, it's co-relating one external dumb mistake with Parse.

It should just fix how the error in thrown originally.

what do you mean?

[stewones]

If we want to include the original error object for reference, we can use the options of the Error object, see this:

not at all, we just want a nice and readable error message. I personally think my approach by stringifying objects could be useful for those scenarios where we don't control what is coming externally. developers are humans and make mistakes, and unfortunately for JS engines new Error({...}) is a valid syntax, even though docs say it's not.

[matheusdavidson]

Hey @stewones @mtrezza, I also have this problem, I agree the PR seems to be solving the problem directly but what is the best way to solve this @mtrezza? cause we do have an [object, object] going on.

Should we add a cause like you suggested? even tho that's no implemented yet. Or is it in a higher class?

[mtrezza]

Where is the code that throws the error and creates a new ParseError object?

[stewones]

Where is the code that throws the error and creates a new ParseError object?

@mtrezza did you see my screenshot above? it starts from the RESTController, I'm not an expert in Parse codebase (yet :D) but I don't think it's something worth it to go deep into every place where ParseError is consumed just to verify if an object or string is being passed.

why not treat this for general purpose like I'm doing here? at least accept the fact that we can't always expect a string in ParseError (i.e. MongoDB is throwing a new Error('string')) and we must decode this message to avoid the [object Object] bubble.

[stewones]

MongoDB is throwing a new Error('string')

how do I know this?

because if I simply put a message.toString() inside ParseError it shows the log correctly ^

I just expanded it to a broader situation where the edge cases mentioned in the beginning may happen. I'm a Parse user for years and started to contribute only recently, but this ParseError: [object Object] thing is smth that always disgusted me so much. we need to fix this asap! also, it will be "required" at some point for the allowDiskUse option I'm going to PR, cause users will get the [object Object] if they don't set a sort specification (as shown in the ^ screenshot)

happy to remove those edge cases if you don't like it and let only message.toString(). but keep in mind that people will continue co-relating those errors with Parse giving a negative perception of the library.

just lmk

[mtrezza]

I get your point, we should get rid of these [obj obj] entries. If the error is thrown in RESTController, then maybe the solution is to construct a proper error there, and not just create an error as new ParseError(anything). See my argument above for why I think that would be better. I think outputting a dependency error object without context is not good error logging.

We could take some inspiration from the Error object (from which ParseError inherits) and its argument types. The Error spec has a dedicated cause argument for re-throwing another error. Like we are re-throwing the MongoDB error. But that is separate from the message argument, which seems to be a string type. In other words, if we pass an object to a string type argument, then of course we will log [obj obj] and the solution is to pass a string.

In fact, the type of message is already a string:

Parse-SDK-JS/src/ParseError.js

Line 18 in 5c14029

* @param {string} message A detailed description of the error.

If we had typescript already rolled out, then it would show a type mismatch error in Parse Server where an object is passed, not in here the Parse JS SDK. I get that just stringifying message is a quick solution. But we'd be violating our own type definitions.

I think it would help to understand this better if we would first find the code in Parse Server that throws the MongoDB error.