fixing enableExpressErrorHandler logic

[hybeats]

fixing this issue
#6422

[codecov]

Codecov Report

Merging #6423 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6423      +/-   ##
==========================================
+ Coverage   93.96%   93.99%   +0.02%     
==========================================
  Files         169      169              
  Lines       11787    11826      +39     
==========================================
+ Hits        11076    11116      +40     
+ Misses        711      710       -1     

Impacted Files	Coverage Δ
src/middlewares.js	97.20% <100.00%> (+0.03%)	⬆️
src/GraphQL/loaders/schemaDirectives.js	88.88% <0.00%> (-5.56%)	⬇️
src/GraphQL/loaders/parseClassMutations.js	98.96% <0.00%> (-1.04%)	⬇️
src/ParseServer.js	91.07% <0.00%> (-0.50%)	⬇️
src/RestWrite.js	93.48% <0.00%> (-0.17%)	⬇️
src/Controllers/DatabaseController.js	95.19% <0.00%> (-0.13%)	⬇️
src/Config.js	89.60% <0.00%> (ø)
src/Controllers/index.js	96.66% <0.00%> (ø)
src/Options/Definitions.js	100.00% <0.00%> (ø)
src/GraphQL/loaders/parseClassQueries.js	97.95% <0.00%> (ø)
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 292bdb7...ed4a250. Read the comment docs.

[dplewis]

@hybeats Looking over it again this looks good. There are existing tests for this

https://github.com/parse-community/parse-server/pull/4697/files#diff-5fed17ed228f1b7c0c8dd44628cd8191

You add test cases here.

Checkout the contributing guide on how to run tests

https://github.com/parse-community/parse-server/blob/master/CONTRIBUTING.md

[dplewis]

@hybeats I fixed the test, @davimacedo Let me know if more tests are needed.

[davimacedo]

LGTM!

[johanarnor]

Hi @davimacedo @dplewis! I stumbled upon this PR since we're currently adopting Sentry error reporting for our backend. To use their errorHandler middleware we're required to set enableExpressErrorHandler to true. However, after this PR was merged this also mean that the default Parse error handler is basically disabled.

I checked the history and found the PR that introduced this feature from the beginning (#4697). The original intention seems to have been quite different. Instead of replacing the default Parse error handler it just used next to let the error flow through subsequent middlewares, which would be optimal our use-case with Sentry.

So by merging this fix it also seems to have broken the intended behaviour. A better way might have been to introduce a overrideParseErrorHandler config or similar, instead of changing the existing behaviour. I'm not here to complain (I really appreciate the work you do!), first I'm just curious to see if agree with my analysis or if I've missed some vital part.

A workaround is of course to copy the logic from the Parse error handler and use that as a middleware in our own application, but that's not optimal.

[davimacedo]

Hi @johanarnor . Thanks for bringing this back up. The idea of having a config option to switch/customize the behavior sounds reasonable to me. Would you mind to create a new issue for discussion?

[johanarnor]

Hi @johanarnor . Thanks for bringing this back up. The idea of having a config option to switch/customize the behavior sounds reasonable to me. Would you mind to create a new issue for discussion?

Yep, it's done. See 👆