I'm writing a function that will be exposed as an API via Google Cloud Functions. It takes the users coordinates, looks up the location name via an API, then using that information calls another API, then returns that data.
My question is about how best to handle errors. I have so far implemented something like as described in the code below, but I'm not sure it's good or "right"...
// index.js
const { getCityByLatLon } = require('./location.js');
const { getWeatherByCity } = require('./weather.js');
module.exports.getWeather = async (req, res) => {
try {
// Validate request - implementation not important
} catch (e) {
// Don't care about logging bad input anywhere, just send error to consumer
return res.status(422).json({ error: e.message });
}
const { lat, lon } = req.query;
try {
const city = await getCityByLatLon(lat, lon);
const weather = await getWeatherByCity(city);
res.json(weather);
} catch (e) {
// Need to return an error here...
// Should it be the message that either getCityByLatLon or getWeatherByCity
// gives, or should it be handled a different way?
res.status(500).json({ error: e.message });
}
};
// location.js
module.exports.getCityByLatLon = (lat, lon) => {
const mapsClient = new Client(); // Implementation not important
try {
const result = await mapsClient.reverseGeocode(lat, lon);
if (result.status !== 'OK') {
// We can't handle this, but should log it and tell the caller
// that something went wrong
throw new Error(result.error_message);
}
if (result.data.length < 1) {
throw new Error('No results');
}
return result.data[0].city;
} catch (e) {
// I think I want to log this exception and store it somewhere
logger.log(e);
// And throw a "nice" error which doesn't give info on the underlying
// error, for the API response to return
throw new Error('Failed to get City');
}
};
// weather.js
module.exports.getWeatherByCity = (city) => {
const weatherClient = new Client(); // Implementation not important
try {
const result = await weatherClient.fetchNextWeek(city);
if (result.status !== 'OK') {
// We can't handle this, but should log it and tell the caller
// that something went wrong
throw new Error(result.error_message);
}
return result.data;
} catch (e) {
// I think I want to log this exception and store it somewhere
logger.log(e);
// And throw a "nice" error which doesn't give info on the underlying
// error, for the API response to return
throw new Error('Failed to get Weather');
}
};
Is this really a good practice? I thought it may have been, as getCityByLatLon and getWeatherByCity don't expose implementation specific errors. But, WebStorm made me think again, by showing an inspection ('throw' of exception caught locally) because I'm throwing inside a try block, then handling it immediately.
The common approach should be to pass the error to the topmost function in the chain, this helps in clean code implementation and avoiding too many try/catch in your code. In your case, it is the responsibility of the caller to log the message, as functions are expected to fail. Adding a log statement to each and every function is not ideal as it may cause duplicate error messages to appear in your logging system.
Consider a case where the caller is also printing the error inside the catch block, in this case, the errors get printed twice in your console.
The common approach is to throw errors, and let the caller decide what to do with the errors.
A more common and robust approach would be to add an error middleware to your request router to handle the error logging in your system. You can attach an error middleware to your express request router as follows:
import { Router } from "express";
const router = Router();
// ... your routes in the format router("/").get()
router.use((err, req, res, next) => {
// we have got an error, we can log it
console.log(err);
res.status(500).json({ error: e.message });
});
Now, in your controller, you could use something like this
module.exports.getWeather = async (req, res, next) => {
try {
// Validate request - implementation not important
} catch (e) {
// Don't care about logging bad input anywhere, just send error to
consumer
return res.status(422).json({ error: e.message });
}
const { lat, lon } = req.query;
try {
const city = await getCityByLatLon(lat, lon);
const weather = await getWeatherByCity(city);
res.json(weather);
} catch (e) {
// Need to return an error here...
// Should it be the message that either getCityByLatLon or
getWeatherByCity
// gives, or should it be handled a different way?
return next(err); // this forces your design to use only one type of response payload for all kind of 500 errors. The schema of your object remains the same. Also, your errors will get logged as well.
}
};