Skip to content

FIX-3: handle bad responses from remote APIs #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 25, 2024
Merged

FIX-3: handle bad responses from remote APIs #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/geoipAPI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,13 @@ function apiRequest(url) {
let coordsResponse = {}

const geoipReq = http.get(url, (res) => {
// console.log(`STATUS: ${res.statusCode}`)
// console.log(`HEADERS: ${JSON.stringify(res.headers)}`)
res.setEncoding('utf8')

if (res.statusCode !== 200) {
console.error(`API request to 'ip-api.com' failed with status code ${res.statusCode}`)
reject(defaultCoords)
}

let responseData = '';
res.on('data', (chunk) => {
// console.log(`BODY: ${JSON.stringify(chunk)}`)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided patch appears to contain some potential issues:

  1. reject is used but it's unclear where and how this reject function is defined or what defaultCoords is. In the context of promise handlers, typically resolve and reject are declared as arguments to a Promise executor function, which isn't shown here. If 'reject' and 'defaultCoords' are not properly defined elsewhere in your program, or if this code block isn't contained within a new Promise, referencing them will likely cause a ReferenceError.

  2. Verifying HTTP response status code as exactly 200 might be too strict. Other codes in the 2xx range indicate success as well. Therefore, it may break perfectly legitimate responses from the server.

  3. The res.on('data') event handler may continue to receive data chunks and append them to responseData even after an error status code has been received and handled. This can lead to unnecessary processing/work. You might want to abort or "close" the response after detecting a non-success status code.

Without more code in this vicinity (especially regarding how apiRequest is defined), I can't reliably comment on other potential issues.

Expand Down
14 changes: 11 additions & 3 deletions src/weatherAPI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,13 @@ function getWeather(lat, lon, timezone) {
return new Promise((resolve, reject) => {
let weatherResponse = {}
const weatherReq = http.get(url, (res) => {
// console.log(`STATUS: ${res.statusCode}`)
// console.log(`HEADERS: ${JSON.stringify(res.headers)}`)
res.setEncoding('utf8')


if (res.statusCode !== 200) {
reject(new Error(`HTTP ${res.statusCode} ${res.statusMessage}`))
}

let responseData = ''
res.on('data', (chunk) => {
// console.log(`BODY: ${JSON.stringify(chunk)}`)
Expand All @@ -189,6 +192,11 @@ function getWeather(lat, lon, timezone) {
res.on('end', () => {
try {
weatherResponse = JSON.parse(responseData)
console.log("Weather API raw response: ", weatherResponse)

// TODO: Throw an error if the `weatherResponse` object
// is missing the expected keys (i.e. `current`, `daily`)

weatherResponse.current.weather = weatherCodes[weatherResponse.current['weather_code']]
weatherResponse.daily.weather = []
weatherResponse.daily.weather_code.forEach(day => {
Expand All @@ -209,7 +217,7 @@ function getWeather(lat, lon, timezone) {
})

weatherReq.on('error', (e) => {
console.error(`problem with request: ${e.message}`)
console.error(`Problem retrieving weather: ${e.message}`)
reject(e)
})

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch does introduce a number of changes, and while some are certainly improvements, a few potential issues can be spotted:

  1. A TODO comment is included to indicate that an error should be thrown if the weatherResponse object does not contain the expected keys (current, daily). Seeing this drafted in the pull request indicates this important check may not have been implemented yet. Unless this is planned as a separate task, it could ideally be included in the same flow since its intent relates to handling the response.

  2. The new print statement console.log("Weather API raw response: ", weatherResponse) may leak sensitive data by logging the entire API response which might contain some confidential or unnecessary information. Logging of sensitive data could potentially violate privacy standards like the GDPR, and it also might pollute the console logs making it harder to find relevant log information during debugging.

  3. The block:

    weatherResponse.current.weather = weatherCodes[weatherResponse.current['weather_code']]
weatherResponse.daily.weather = []
weatherResponse.daily.weather_code.forEach(day => {

    assumes that weatherResponse will always have a valid value after parsing the JSON. However, as pointed out in point 1, there are currently no checks ensuring that weatherResponse contains the necessary fields. If any of these fields are not present in the response, this piece of code will throw an Error (property of undefined), causing an unhandled promise rejection.

  4. Furthermore, res.statusCode !== 200 is a good start in error checking but it may not cover all error situations correctly. Some other HTTP status codes in 200-299 range are also success statuses. Although they are less commonly used, the best practice would be to check whether the status code is within the range 200-299.

  5. There's an assumed weatherCodes object that seems to map weather_code to weather. This object is not defined or imported within the given changeset, so you want to confirm its existence elsewhere in the code. If it's missing, this could create a ReferenceError.

Please note that some of these points may be false alarms if they are, in fact, handled elsewhere in the full codebase. Nevertheless, it's good practice to ensure such potential problems are addressed right away to maintain clean and reliable code.

Expand Down