Log ParseErrors and Prevent Express from Logging error object #3431

acinader · 2017-01-25T01:05:13Z

Log Parse Errors so they are intelligible.

The problem this pr is trying to solve:

When an error occurs on the server, a message should
be returned to the client, and a message should be logged.

Currently, on the server, the log is just [object, object]

This pr will stop calling the default express error handler
which causes two problems: 1. it writes to console instead of log file
2. the output is completely useless! :)

Instead, we'll log the error ourselves using the ParseServer's logger.

fixes: #661

acinader · 2017-01-25T01:06:41Z

src/ServerError.js

+ *                  about the error for logging on the server
+ * @param {String} stack the stack trace
+ */
+export default class ServerError extends Parse.Error {


I tried simply extending Error, but express wanted to handle it differently. Its possible that it was another mistake, so i could go back and recheck, but i like this better anyway now.....

What was the issue with Express?

i'll re-try it and see what the problem was again.

well, first off: myObj.constructor.name as a hack to get the classname of an object is my new favorite javascriptism.

so, I can't explain exactly why it is happening, but I can tell you what I observe. When I extend ServerError from Error, the err param of middlewares.handleParseError is an Error, not a ServerError as one would expect. If I extend ServerError from Parse.Error then the err param is a ServerError as one would expect.

So an interesting thing to note is that the Error object does have all of the members of the ServerError, but it fails the test err instanceof ParseServer.Error which causes bad things to ensue.

So I could conceivably add a reliable marker to the ParseServer class that I could test, say add a member like isServerError = true or some such, but that doesn't appeal to me.

In any event, open to another approach and also glad to go with the marker approach if you prefer it.

PS could def use some help with the failing unit test if you have a clue....

yeah instance of is notably broken in JS :) can you try with err.constructor == ServerError Does it help?

nope:

console.log(err.constructor == ServerError);
false
console.log(err.constructor == Error);
true

something is futzing with my object. I'd guess express. I made a half assed attempt to figure out what was causing it in the express source code...no joy.

facebook-github-bot · 2017-01-25T01:29:12Z

@acinader updated the pull request - view changes

acinader · 2017-01-25T02:54:44Z

hm. my test is failing on travis with { [Error: unauthorized: master key is required] status: 403, message: 'unauthorized: master key is required' }

but it works locally. I'm trying to figure it out....

So that error message is coming from one of two places:

middlewares.enforceMasterKeyAccess

or

middlewares.promiseEnforceMasterKeyAccess

Neither of those two functions get called when I run my new unit test on my machine, but one is obviously getting called on travis. i'm stumped.

I have ruled out it being some env var of mine with this neat little trick:

env -i bash --norc --noprofile which gives you a clean shell. cool.

facebook-github-bot · 2017-01-25T03:52:04Z

@acinader updated the pull request - view changes

facebook-github-bot · 2017-01-25T17:17:30Z

@acinader updated the pull request - view changes

facebook-github-bot · 2017-01-25T17:23:34Z

@acinader updated the pull request - view changes

facebook-github-bot · 2017-01-25T18:05:46Z

@acinader updated the pull request - view changes

steven-supersolid · 2017-01-25T18:10:12Z

src/middlewares.js

  }
-  next(err);
+


Moving this will lead to next() not being called in certain situations

that is intentional. in the case that we have a ParseServer.Error, we handle logging the error. So we don't call next(err) which then go to the Express' error logging backstop: https://github.com/expressjs/express/blob/3c54220a3495a7a2cdf580c3289ee37e835c0190/lib/application.js#L161 which is what leads to the [object, object] getting emitted to console.error

I should add that my experience with express is pretty limited, so if there would be a bad side affect of not calling next(err), don't assume that I have contemplated that. but my thought is that we've taken care of the final step in this failed request which is to log it, so next isn't required.

steven-supersolid · 2017-01-25T18:17:03Z

src/ServerError.js

+ * @constructor
+ * @param {ErrorCode} code the Parse.Error code
+ * @param {String} message will be sent to clients
+ * @param {String} serverErrorLog additional information


nit: the comment here refers to serverErrorLog but the parameter and property name are both serverLogError.

The thing being passed in is actually an error message, so perhaps serverErrorMessage or similar would be be clearer?

ooh. nice catch. thanks.

steven-supersolid · 2017-01-25T18:20:17Z

src/middlewares.js

-  if (err instanceof Parse.Error) {
-    var httpStatus;
-
+  if (err instanceof Parse.Error || err instanceof ParseServer.Error) {


nit: If ParseServer.Error extends Parse.Error then won't instanceof Parse.Error be enough to check for?

you'd think, wouldn't you? Too bad javascript sucks ;)

err ServerError {code: 130, message: "Could not store file.", serverLogError: "it failed", stack: "Error: it failed at Object.createFile (/Users/…"} err instanceof Parse.Error "false"

just to be extra sure, tested it again :)

facebook-github-bot · 2017-01-25T18:23:38Z

@acinader updated the pull request - view changes

facebook-github-bot · 2017-01-25T18:48:53Z

@acinader updated the pull request - view changes

facebook-github-bot · 2017-01-25T22:59:47Z

@acinader updated the pull request - view changes

facebook-github-bot · 2017-01-25T23:06:44Z

@acinader updated the pull request - view changes

facebook-github-bot · 2017-01-25T23:30:44Z

@acinader updated the pull request - view changes

acinader · 2017-01-26T00:11:58Z

@flovilmart @steven-supersolid ok, so now my unit test passes on travis. by, um, making the test wait a second before doing anything.

So I took a look at the next step for this pull requests: looking at other new Parse.Error()'s for other candidates. But I didn't find any, while its likely/possible that I missed one or two, what I was looking for is a case where a promise resolves and we don't log the reason for the failure.

So unless I am missing something, the real value of this pr is not adding the new class, but simply logging the Parse.Error in middlewares.js and not letting the error go to the default handler in express.

So two options that I'd like your input on:

merge Add logging on failure to create file #3424 and then change this pr to remove the new class and just keep the middleware changes to log Parse.Errors and not call express default logger.
close Add logging on failure to create file #3424 and merge this as is and then if we find another candidate where Parse.Error is incomplete, we can change it to a ParseServer.Error

I think 1 is the way to go....

steven-supersolid · 2017-01-26T10:22:03Z

I prefer option 1 too, it keeps each PR small. Have you thought how you will return the stack if you remove the new class? Perhaps the stack is not that useful though if the error is something like 'session token invalid' so would be happy to not have that returned in those cases.

acinader · 2017-01-26T18:39:32Z

in the number 1 case, I wont return the stack, I'll log the stack where the error occurs in the FileRouter in pr #3424 which will be just fine.

I'll fix up both prs as soon as I get a chance, but may not be till tomorrow.

facebook-github-bot · 2017-01-28T18:06:43Z

@acinader updated the pull request - view changes

The problem this pr is trying to solve: When an error occurs on the server, a message should be returned to the client, and a message should be logged. Currently, on the server, the log is just [object, object] This pr will stop calling the default express error handler which causes two problems: 1. it writes to console instead of log file 2. the output is completely useless! :) Instead, we'll log the error ourselves using the ParseServer's logger. fixes: parse-community#661

facebook-github-bot · 2017-01-28T18:10:03Z

@acinader updated the pull request - view changes

acinader · 2017-01-28T18:10:48Z

@steven-supersolid I think that both this and #3424 are now ready to merge

steven-supersolid · 2017-01-30T15:36:11Z

LGTM. I don't have merge access so someone can do

facebook-github-bot added the GH Review: review-needed label Jan 25, 2017

acinader commented Jan 25, 2017

View reviewed changes

acinader force-pushed the create-ParseServerError-class branch from 4eeec1e to b8c59b9 Compare January 25, 2017 01:29

acinader force-pushed the create-ParseServerError-class branch from b8c59b9 to bd00a94 Compare January 25, 2017 03:51

steven-supersolid reviewed Jan 25, 2017

View reviewed changes

acinader force-pushed the create-ParseServerError-class branch from f230669 to 8fe4e6f Compare January 25, 2017 23:05

acinader changed the title ~~Create a ServerError class.~~ Log ParseErrors and Prevent Express from Logging error object Jan 28, 2017

acinader force-pushed the create-ParseServerError-class branch from 7730cbb to e59c814 Compare January 28, 2017 18:06

acinader force-pushed the create-ParseServerError-class branch from e59c814 to 145a1fe Compare January 28, 2017 18:09

flovilmart merged commit f864141 into parse-community:master Jan 30, 2017

dplewis mentioned this pull request Mar 23, 2018

Cloud Job unexpected error kills server #3270

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log ParseErrors and Prevent Express from Logging error object #3431

Log ParseErrors and Prevent Express from Logging error object #3431

acinader commented Jan 25, 2017 •

edited

Loading

acinader Jan 25, 2017

flovilmart Jan 25, 2017

acinader Jan 25, 2017

acinader Jan 25, 2017

flovilmart Jan 25, 2017

acinader Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

acinader commented Jan 25, 2017 •

edited

Loading

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

steven-supersolid Jan 25, 2017

acinader Jan 25, 2017

acinader Jan 25, 2017

steven-supersolid Jan 25, 2017 •

edited

Loading

acinader Jan 25, 2017

steven-supersolid Jan 25, 2017

acinader Jan 25, 2017 •

edited

Loading

acinader Jan 25, 2017

acinader Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

acinader commented Jan 26, 2017

steven-supersolid commented Jan 26, 2017 •

edited

Loading

acinader commented Jan 26, 2017 •

edited

Loading

facebook-github-bot commented Jan 28, 2017

facebook-github-bot commented Jan 28, 2017

acinader commented Jan 28, 2017

steven-supersolid commented Jan 30, 2017

Log ParseErrors and Prevent Express from Logging error object #3431

Log ParseErrors and Prevent Express from Logging error object #3431

Conversation

acinader commented Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 25, 2017

acinader commented Jan 25, 2017 • edited Loading

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-supersolid Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acinader Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

facebook-github-bot commented Jan 25, 2017

acinader commented Jan 26, 2017

steven-supersolid commented Jan 26, 2017 • edited Loading

acinader commented Jan 26, 2017 • edited Loading

facebook-github-bot commented Jan 28, 2017

facebook-github-bot commented Jan 28, 2017

acinader commented Jan 28, 2017

steven-supersolid commented Jan 30, 2017

acinader commented Jan 25, 2017 •

edited

Loading

acinader commented Jan 25, 2017 •

edited

Loading

steven-supersolid Jan 25, 2017 •

edited

Loading

acinader Jan 25, 2017 •

edited

Loading

steven-supersolid commented Jan 26, 2017 •

edited

Loading

acinader commented Jan 26, 2017 •

edited

Loading