API errror handling

[motosharpley]

Set up error handling in app.js

[jkwening]

This is issue is becoming more of a priority since the frontend is now setup to make requests to the backend server. We need to standardize how we send response back to the frontend. I've been running into difficulty resolving bugs/issues since there's no consistent structure.

To get the conversation started, I'm going to propose that for all responses we continue to send status code number along with a JSON response of the following structure:

{
  status: 'PREDEFINED_STRING_CONSTANTS',
  error_message: 'detailed information about the reasons behind given status constant
   for those other than "OK"',
  '...': '...whatever objects needed for "OK" response'
}

To start off, I believe we currently need the following status constants:

• "OK": indicates that no errors occurred
• "UNKNOWN_ERROR": indicates a server-side error
• "NOT_FOUND": indicates referenced data such as 'id' for pin/user was not found in respective database
• "REQUEST_DENIED": indicates that request was denied because couldn't authenticate
• "INVALID_REQUEST": indicates that some required param(s) or request body data is missing that is necessary for processing the request
• "ZERO_RESULTS": indicates a request for a count or # of something returned 0 results

Thoughts?

[redragonx]

Can I get assigned to this?

[motosharpley]

I like the idea of having a standardized set of responses I would also like to see a catch-all in app.js that would handle anything that manages to fall all the way through.

[redragonx]

Are there errors that need to be refactored while I'm making this?

[jkwening]

@motosharpley yep - agreed on the catch-all for server app.js. I'm still trying to track down how the server is crashing at certain times due to uncaught error that I can't quite trace back to a source...I'm currently guessing its related to one of the promises or call back instances failing and not being appropriately caught.

@redragonx all controllers need to be refactored to implement the standard appRes.status(#).send(...) code snippets.

Implementation-wise, I was thinking we could use an event driven mechanism to do this. Open to other implementation suggestions, figured I'd share where I would start if I was going to do this. Also, its imperative that the status constants reside as a true constant object and exported for others to use instead of manually coding each snippet - there should be only one source of truth for ease of testing and modifying across the code base. I would suggest putting it in the utils-controller.js module to start and depending how big it gets we may want to migrate it into its own module.

[redragonx]

@jkwening - Okay, this will be likely done in multiple commits or PRs depending on how many controllers there are.

Should we talk about future proofing the data objects? I am not too sure on this yet as I'm looking at the data needed still. Just a thought.

I was thinking of putting the constants in a file called constants.js so it'll be easier to edit. And the logic will be in another file like utils-controller.js

Thoughts?

[jkwening]

@redragonx yep - multiple commits/PR is definitely fine and encouraged...I'm definitely big fan of piecemeal implementation and updates than one massive code update which can be tricking to test and validate.

We currently don't have that many true constants to justify a module just for that. Go ahead and encapsulate the whole error handling into its own module, include status constant object and the logic for now and let's see how it comes together and determine re-packaging later on if needed.

I would suggest first building out the module for standardizing the error handling logic and then move onto refactoring the existing code base to utilize it. I've created a new feature branch called "standard-api-error-handling" for this work so it can be completed in isolation of other changes to minimize effects on other feature works. We should work out of that branch until this is done and tested out, then will merge into development.

[redragonx]

Are there any samples of what api error handling looks like?

I'm not familiar with the app.res(#).send() notation.

[motosharpley]

here is a little snippet of a catchall to help get you started

/ error handling
app.use((req, res, next) => {
  const error = new Error('Not found');
  error.status = 404;
  next(error);
});

app.use((error, req, res, next) => {
  res.status(error.status || 500);
  res.json({
    error: {
      message: error.message
    }
  });
});

[motosharpley]

if you scroll down a bit this thread has examples of different approaches, synchronous, event etc. https://stackoverflow.com/questions/7310521/node-js-best-practice-exception-handling

[redragonx]

Will each constant have it's own snippet? Or am I checking the request for each error type in the catchall?

Can I get a list of endpoints that will expect certain errors?

[redragonx]

Also, what about a logger middleware, any suggestions for that?

Found these resources to help guide me design the api error handling
https://kostasbariotis.com/rest-api-error-handling-with-express-js/
https://github.com/kbariotis/throw.js

[redragonx]

Regarding my last question about the logger, please ignore it. :) I see that we are using the Morgan logger.

[redragonx]

Here are some additional REST api error formats we can look at:

https://nordicapis.com/best-practices-api-error-handling/

I like the Bing and Twitter versions.

[jkwening]

@redragonx the status code constants I was thinking something like this:

export const STATUS = {
  "OK",
  "UNKNOWN_ERROR",
  "NOT_FOUND",
  "REQUEST_DENIED",
  "INVALID_REQUEST",
  "ZERO_RESULTS",
} 

So that the same status code strings are utilized whenever responding to a request.

[redragonx]

Still working on this.

[jkwening]

@redragonx glad to hear you're making progress. We're working out of a feature specific branches, so feel free to make PR request as you have parts of it committed and moving along. As usual, let us know if you have any questions or run into issues.

[redragonx]

@jkwening Thanks, I haven't had much time last week to do much, which is annoying to me at least. I'll try to do PRs but no promises.

[redragonx]

Does anyone have good specific examples of Navi api calls that I can test with? @motosharpley @jkwening

Thanks!

[motosharpley]

@redragonx you can use postman to hit the endpoints and test to see that your getting the expected response back https://www.getpostman.com/