Skip to content

FEAT-1: Add weather app backend functionality to sample app #1

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.

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 3 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
65 changes: 61 additions & 4 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
const express = require('express')
const weather = require('./src/weatherAPI.js')
const {requestCityFromIP, requestCoordsFromIP} = require('./src/geoipAPI.js')
const anonymizeip = require('./src/anonymizeip.js')
var path = require('path');

const PORT = process.env.PORT || 3000
const HOST = '0.0.0.0'

// !* Edit here for demos
const RELEASE_NO = 'PROD-134'
const RELEASE_NO = 'DEMO-2'

const api = express()

Expand All @@ -14,10 +17,64 @@ api.set('view engine', 'pug');

api.use(express.static(path.join(__dirname, 'public')));

// Define the main route for the sample app. When the homepage of the sample app
// is requested, the server will first determine the client's geolocation based
// on the client's IP address.
api.get('/', (req, res) => {
res.render('index', {
release_no: RELEASE_NO
})

const clientIP = req.ip
const renderValues = {
release_no: RELEASE_NO,
ipAddr: anonymizeip(clientIP),
}

requestCityFromIP(clientIP).then((coords) => {
console.log('Coords obj:', coords)

if ("regionName" in coords && "city" in coords && "country" in coords) {
renderValues.locationName = `${coords.city}, ${coords.regionName}, ${coords.country}`
} else if ("country" in coords) {
if ("city" in coords) {
renderValues.locationName = `${coords.city}, ${coords.country}`
} else if ("region" in coords) {
renderValues.locationName = `${coords.regionName}, ${coords.country}`
} else {
renderValues.locationName = coords.country
}
} else if ("city" in coords) {
renderValues.locationName = coords.city
} else {
renderValues.locationName = coords.regionName
}

// By default, display the weather in Toronto.
return weather("43.7001", "-79.4163", "America/Toronto")
}).then((weatherData) => {
// renderValues.forecast = JSON.stringify(weatherData)
renderValues.forecast = weatherData
res.render('index', renderValues)
})
})

api.get('/geolocate', (req, res) => {
// If the geolocation permission is denied on the client side, the client
// will send a request to `/geolocate` to get the estimated coordinates
// of the client's IP address. This will then return the coordinates to the
// client, which will use them to call the weather API as it normally would.
geoip(req.ip).then((coords) => {
res.json(coords)
})
})

api.get('/weather', (req, res) => {
const queryParams = req.query
if (!queryParams.lat || !queryParams.lon || !queryParams.timezone) {
res.status(400).send('Missing query parameters. All of the following are required: lat, lon, timezone')
}

weather(queryParams.lat, queryParams.lon, queryParams.timezone).then((weatherData) => {
res.json(weatherData)
})
})

api.listen(PORT, HOST)
Copy link

Choose a reason for hiding this comment

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

Here are several potential issues in the given code.

  1. Misplaced comment: At the top of the api.get('/') interaction, there's a note that this is a first response to calculate a client's geolocation from its IP. However, directly under that, the client IP address is being anonymized. The anonymization process may strip some valuable data needed to ascertain geolocation accurately.

  2. Error handling: There're no catch blocks for promise rejections (e.g., weather API failures), which could lead to unhandled promise rejections and crash the server if an error event isn't properly caught. To prevent such issues, promises should ideally have a .catch() block attached to handle errors gracefully.

  3. Weather location hard-coded: The logic seems to assume that all clients are located in Toronto by default ("43.7001", "-79.4163", "America/Toronto"). This might not be representative of the actual client base, especially considering that previous code attempts to calculate location based on an IP address.

  4. GeoIP confusion: In api.get('/geolocate'), 'geoip(req.ip)is being used instead ofrequestCoordsFromIP(req.ip). Since the earlier imports include requestCoordsFromIP`, it looks like maybe 'geoip' was a typo.

  5. Lack of validation: The '/weather' endpoint doesn't have any validation before hitting the 'weather' function. The latitude, longitude, and timezone query parameters need to have some format check or validation before use.

  6. Redundancy: Looking at this code snippet, the app tries to get geolocation data twice - once when loading the root route /, and again when the /geolocate endpoint is hit. If getting the geolocation data is costly in time or resources, duplicating the activity might present performance issues.

  7. Variable name mismatch: There's a mismatch in checking the key "region" in one case and using it as "coords.regionName" later. This might be a typo which can lead to undefined value.

  8. Incomplete response: If queryParams.lat, queryParams.lon, queryParams.timezone are not provided in the '/weather' endpoint, an error message is sent immediately without terminating the function - potentially leading to additional code execution and errors.

  9. Stripped IP address for geolocation: The IP is anonymized before being used to determine geolocation. If too much information is stripped, geolocation may fail or be inaccurate.

Expand Down
Loading