[WIP][Bug 890538] Update API handling & response codes for the Login server #140
Conversation
| res.json( 404, { error: err || "User not found for ID: " + id } ); | ||
| return; | ||
| metrics.increment( "user.get.error" ); | ||
| throw new Error( "Database helper failure" ); |
There was a problem hiding this comment.
It seems odd to me to mix json error code returns on the response with throwing exceptions--you can't really have it both ways.
There was a problem hiding this comment.
I see what you mean. I built the user.js controller architecture to assume the errors have all been processed by the individual SQL and MONGO handles. It was the only way I could think of to de-couple that logic from this controller.
I supposed I could just return 500 in the case that a more specific error object isn't available?
There was a problem hiding this comment.
Yes, I'd send back a 500 with some info. If you have an interface that deals with json responses, you have to follow-through on that for every path, not just the success paths.
There was a problem hiding this comment.
Ah, I remember why I did this now. If this error triggers, there is /no/ information available other than the fact that something went wrong.
All errors that CAN be caught have already been processed, so its a signal that something... very very odd has happened. Returning a 500 still makes sense, but I get the feeling there should be something else too.
What do you think?
There was a problem hiding this comment.
I'd say just 500 on the response here rather than throw.
How does this affect login API calls with emails that aren't webmakers? This happens often because of the legacy popcorn and thimble makes that we added to the Make API.
There was a problem hiding this comment.
Its backwards compatible for email as _id
There was a problem hiding this comment.
Looking at this again, I'm not sure I know what you mean @cadecairos
ref: https://bugzilla.mozilla.org/show_bug.cgi?id=890538