TimAagaard
commented
2 years ago @MartinsOnuoha Just a status update. I figured out what was wrong with the population/filter route. you run a find() on the data to get the population data for a specified year. If the year isn't found, it returns undefined and crashes when you try to sort the data based on the undefined value. I fixed this by wrapping the whole route function in a try / catch block, adding the 'next' param to the route function, and on catch(err) I run next(err), which will pass it to express' built in error handler and cause a 500 response. I also made it so that when find doesn't find anything it replaces it with {year: year, value: -1} and then afterwards use filter() to remove any data that has -1 as the population value.
So, now it will return empty data instead of crashing if there is no population data for a given year....and if there are any other ways it could crash it will now just fail gracefully, returning an HTTP 500 and if not in production mode, will dump a stack trace to the console.
Over the next couple days I hope to have all routes handled like this.
I recommend a 3-prong approach to fixing this issue.
1) Wrap all route functions in a try / catch block that runs next() in the catch block to forward everything along to Express' error handler.
2) Deprecate all POST routes in favor of GET routes. POST routes aren't idempotent, GET routes are. Your POST routes are idempotent because they don't alter anything. For backwards compatibility purposes (and because, lets face it, it's nicer to send an object with names rather than remember the order of values in a GET route), I propose that we move the functionality of the POST routes to GET routes and make redirect() the POST routes to the GET routes. This will set us up nicely for step 3
3) Caching. Cache all GET routes. A simple key/value store of the request route / response object. Throw some middleware in there that checks whether the cache holds the response and returns the value if it does, if not next() it on through to the actual route.
I would send a PR for each step.
Let me know if that sounds like a good plan to you.
Owner @TimAagaard
1) I just broke your API, sorry. If you run /api/v0.1/countries/population/filter with:
The app hard crashes. I double checked locally and Node crashes. (So if you're around and awake you should restart the app)
2) If you run the same route with the values in the documentation, it runs fine
3) If you change the year to 2020 in situation 2, the route runs forever and never returns, which would cause Heroku's router to send back a 503 because your dyno is stuck processing. I don't know if its actually stuck, or taking a really long time but it's been running for over 10 minutes locally without returning - no errors from Node.
This is just one route that I checked because I thought it had the potential to be expensive.
I'm going to test all of these routes tonight and try to break them. Then I'm going to attempt to fix them.
If you don't have any objections we can start off by wrapping all the routes in a try/catch block that just returns an HTTP 500 if the catch block gets triggered. That will prevent hard crashes.
I would also recommend implementing default values for all parameters.
Beyond that, it's going to be profiling the routes for time constraints. Anything over 30s is going to cause a 503 on Heroku.
We could also implement caching to speed up repeated routes if you don't already have that.
Originally posted by @TimAagaard in https://github.com/MartinsOnuoha/countriesNowAPI/issues/90#issuecomment-1062015186