-
Notifications
You must be signed in to change notification settings - Fork 22
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
Include site category data in Readings schema #3884
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new schema, Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #3884 +/- ##
========================================
Coverage 11.74% 11.75%
========================================
Files 114 114
Lines 15254 15255 +1
Branches 274 274
========================================
+ Hits 1792 1793 +1
Misses 13462 13462
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
src/device-registry/utils/scripts/api-response-transformer.js (3)
7-7
: Remove unused dependency to streamline the code.The
is-empty
package is imported but not utilized in the code. Consider removing it to reduce unnecessary dependencies and potential confusion.
33-36
: Enhance 'extractValue' function to handle unexpected line formats gracefully.The
extractValue
function assumes each line contains': '
for splitting. If a line doesn't match this pattern,split(': ')
could result inundefined
values, leading to runtime errors. It's advisable to add a check to handle such cases.Here's how you might modify the function:
const extractValue = (line) => { + if (!line.includes(': ')) { + return { key: '', value: '' }; + } const [key, value] = line.trim().split(': '); return { key: key.trim(), value: value.trim() }; };
100-100
: Clean up commented-out code to maintain clarity.The line
// console.log("the site _id", site._id);
is commented out. Removing unnecessary commented code helps keep the codebase clean and improves readability.src/device-registry/models/Reading.js (3)
23-44
: Consider adding validation rules for critical fieldsThe
categorySchema
structure is well-organized, but could benefit from additional validation rules for critical fields:Consider applying these validations:
const categorySchema = new Schema( { area_name: { type: String }, category: { type: String }, highway: { type: String }, landuse: { type: String }, - latitude: { type: Number }, - longitude: { type: Number }, + latitude: { + type: Number, + min: -90, + max: 90, + required: true + }, + longitude: { + type: Number, + min: -180, + max: 180, + required: true + }, natural: { type: String }, - search_radius: { type: Number }, + search_radius: { + type: Number, + min: 0, + required: true + }, waterway: { type: String }, tags: [ { type: String, trim: true, }, ], }, { _id: false, } );
Line range hint
5-24
: Consider adding indexes for frequently queried fieldsThe schema involves complex aggregation pipelines, particularly in the analytics functions. Adding strategic indexes could significantly improve query performance.
Consider adding these indexes to support the aggregation pipelines:
ReadingsSchema.index({ device_id: 1, time: 1 }, { unique: true }); ReadingsSchema.index({ device: 1, time: 1 }, { unique: true }); ReadingsSchema.index({ site_id: 1, time: 1 }, { unique: true }); +// Add indexes for analytics queries +ReadingsSchema.index({ "pm2_5.value": 1 }); +ReadingsSchema.index({ "pm10.value": 1 }); +ReadingsSchema.index({ "no2.value": 1 });
Line range hint
447-450
: Add null checks and error handling for edge casesThe percentage difference calculations could potentially cause division by zero errors.
Consider adding safeguards:
const calculatePercentageDiff = (current, previous) => { - if (previous === 0) return 0; + if (!current || !previous || previous === 0) return 0; return ((current - previous) / previous) * 100; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/device-registry/models/Reading.js
(2 hunks)src/device-registry/utils/scripts/api-response-transformer.js
(1 hunks)src/device-registry/utils/scripts/bulk-script.js
(2 hunks)
🔇 Additional comments (1)
src/device-registry/models/Reading.js (1)
68-68
: Integration of site_category looks good
The site_category
field is correctly integrated into the SiteDetailsSchema
using the newly defined categorySchema
.
const url = "http://localhost:3000/api/v2/devices/sites/summary"; | ||
const config = { | ||
headers: { | ||
Authorization: "", | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use environment variables for configuration to enhance security and flexibility.
Hardcoding URLs and the Authorization
header can lead to security risks and make the code less adaptable across different environments. Using environment variables or a configuration file allows for better security practices and easier configuration management.
for (let i = 0; i < response.data.sites.length; i += 10) { | ||
const batch = response.data.sites.slice(i, i + 10); | ||
// Process batch of 10 items | ||
batch.forEach(async (site) => { | ||
// console.log("the site _id", site._id); | ||
const url = `http://localhost:3000/api/v2/spatial/categorize_site?latitude=${site.latitude}&longitude=${site.longitude}`; | ||
|
||
//***MAKE THE GET REQUEST */ | ||
axios | ||
.get(url, config) | ||
.then((response) => { | ||
// console.log("PUT response:", response.data); | ||
const apiResponse = response.data; | ||
const requestBody = createRequestBody(apiResponse); | ||
// console.log("requestBody", requestBody); | ||
const updateURL = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`; | ||
const data = { | ||
site_category: requestBody, | ||
}; | ||
|
||
// Make a PUT request | ||
axios | ||
.put(updateURL, data, config) | ||
.then((response) => { | ||
console.log("PUT response:", response.data); | ||
}) | ||
.catch((error) => { | ||
if (error.response) { | ||
console.log(error.response.status); | ||
console.log(error.response.data); | ||
} else { | ||
console.log(error.message); | ||
} | ||
}); | ||
}) | ||
.catch((error) => { | ||
if (error.response) { | ||
console.log(error.response.status); | ||
console.log(error.response.data); | ||
} else { | ||
console.log(error.message); | ||
} | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor asynchronous code to use async/await consistently for improved readability.
In the batch processing loop, you're mixing async
functions with .then()
and .catch()
promise handling. For better readability and error handling, consider refactoring the code to use async/await
throughout.
Here's how you could refactor the code:
// Process batch of items
-batch.forEach(async (site) => {
+for (const site of batch) {
+ try {
+ const url = `http://localhost:3000/api/v2/spatial/categorize_site?latitude=${site.latitude}&longitude=${site.longitude}`;
+
+ // Make the GET request
+ const response = await axios.get(url, config);
+ const apiResponse = response.data;
+ const requestBody = createRequestBody(apiResponse);
+
+ const updateURL = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`;
+ const data = {
+ site_category: requestBody,
+ };
+
+ // Make the PUT request
+ const putResponse = await axios.put(updateURL, data, config);
+ console.log('PUT response:', putResponse.data);
+ } catch (error) {
+ if (error.response) {
+ console.log(error.response.status);
+ console.log(error.response.data);
+ } else {
+ console.log(error.message);
+ }
+ }
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
for (let i = 0; i < response.data.sites.length; i += 10) { | |
const batch = response.data.sites.slice(i, i + 10); | |
// Process batch of 10 items | |
batch.forEach(async (site) => { | |
// console.log("the site _id", site._id); | |
const url = `http://localhost:3000/api/v2/spatial/categorize_site?latitude=${site.latitude}&longitude=${site.longitude}`; | |
//***MAKE THE GET REQUEST */ | |
axios | |
.get(url, config) | |
.then((response) => { | |
// console.log("PUT response:", response.data); | |
const apiResponse = response.data; | |
const requestBody = createRequestBody(apiResponse); | |
// console.log("requestBody", requestBody); | |
const updateURL = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`; | |
const data = { | |
site_category: requestBody, | |
}; | |
// Make a PUT request | |
axios | |
.put(updateURL, data, config) | |
.then((response) => { | |
console.log("PUT response:", response.data); | |
}) | |
.catch((error) => { | |
if (error.response) { | |
console.log(error.response.status); | |
console.log(error.response.data); | |
} else { | |
console.log(error.message); | |
} | |
}); | |
}) | |
.catch((error) => { | |
if (error.response) { | |
console.log(error.response.status); | |
console.log(error.response.data); | |
} else { | |
console.log(error.message); | |
} | |
}); | |
}); | |
for (let i = 0; i < response.data.sites.length; i += 10) { | |
const batch = response.data.sites.slice(i, i + 10); | |
// Process batch of items | |
for (const site of batch) { | |
try { | |
const url = `http://localhost:3000/api/v2/spatial/categorize_site?latitude=${site.latitude}&longitude=${site.longitude}`; | |
// Make the GET request | |
const response = await axios.get(url, config); | |
const apiResponse = response.data; | |
const requestBody = createRequestBody(apiResponse); | |
const updateURL = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`; | |
const data = { | |
site_category: requestBody, | |
}; | |
// Make the PUT request | |
const putResponse = await axios.put(updateURL, data, config); | |
console.log('PUT response:', putResponse.data); | |
} catch (error) { | |
if (error.response) { | |
console.log(error.response.status); | |
console.log(error.response.data); | |
} else { | |
console.log(error.message); | |
} | |
} | |
} |
const url = `http://localhost:3000/api/v2/devices/soft?id=${device._id}`; | ||
const new_api_code = extractBeforeV2IncludingSlash(device.api_code); | ||
|
||
const data = { | ||
network: "airqo", | ||
api_code: new_api_code, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve batch processing reliability and error handling
The current implementation has several potential issues:
- Using
forEach
with async operations doesn't guarantee sequential processing - No rate limiting could overwhelm the API
- No progress tracking for large datasets
Consider this more robust approach:
- batch.forEach(async (device) => {
+ for (const device of batch) {
+ try {
const url = `http://localhost:3000/api/v2/devices/soft?id=${device._id}`;
const new_api_code = extractBeforeV2IncludingSlash(device.api_code);
+ if (!new_api_code) {
+ console.warn(`Skipping device ${device._id}: Invalid api_code`);
+ continue;
+ }
const data = {
api_code: new_api_code,
};
+ await new Promise(resolve => setTimeout(resolve, 100)); // Rate limiting
+ const response = await axios.put(url, data, config);
+ console.log(`Updated device ${device._id}`);
+ } catch (error) {
+ console.error(`Failed to update device ${device._id}:`, error.message);
+ }
- });
+ }
Committable suggestion skipped: line range outside the PR's diff.
/** | ||
* Update each device to have the default network of VISIBLE_DEVICES_ONLY | ||
*/ | ||
|
||
// Make a GET request | ||
const url = "http://localhost:3000/api/v1/devices"; | ||
const url = "http://localhost:3000/api/v2/devices/summary"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use environment configuration for API endpoints
Hardcoding the localhost URL makes the script environment-dependent and could lead to issues in different deployment environments.
Consider using environment variables:
- const url = "http://localhost:3000/api/v2/devices/summary";
+ const BASE_URL = process.env.API_BASE_URL || "http://localhost:3000";
+ const url = `${BASE_URL}/api/v2/devices/summary`;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const url = "http://localhost:3000/api/v2/devices/summary"; | |
const BASE_URL = process.env.API_BASE_URL || "http://localhost:3000"; | |
const url = `${BASE_URL}/api/v2/devices/summary`; |
💡 Codebase verification
Based on the results from our investigation, I can now provide a conclusive response about the API version migration.
Migration from v1 to v2 endpoints is incomplete
- Found commented out v1 route in
src/device-registry/bin/server.js
:// app.use("/api/v1/devices", require("@routes/v1"));
- The v1 routes directory still exists and contains active code
- The v2 endpoint being used in
bulk-script.js
is correct, but the overall migration is not complete
🔗 Analysis chain
Verify complete migration from v1 to v2 endpoints
Let's ensure all v1 endpoints have been properly updated to v2 and that we're using all required v2 parameters.
Also applies to: 37-37
Let me gather more information about the device-registry endpoints to ensure a complete verification.
The previous results show v1 references in device-registry files, and we need to verify if there are any device-specific v1 endpoints still in use. This will help us confirm if the migration is complete and consistent.
Let me check the device-registry routes to verify the complete migration status.
This will help us verify if there are any v1 routes still in use and confirm the migration status of the endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining v1 API references
echo "Checking for v1 API references..."
rg -l "api/v1/" --type js
# Search for v2 endpoint usage patterns to verify consistency
echo "Checking v2 endpoint patterns..."
rg "api/v2/devices/(summary|soft)" --type js -A 2
Length of output: 3023
Script:
#!/bin/bash
# Check for any v1 device endpoints specifically
echo "Checking for v1 device endpoints..."
rg "api/v1/devices" --type js -A 2
# Check the device-registry config and routes for endpoint definitions
echo "Checking device-registry endpoint configurations..."
fd -e js -e ts . src/device-registry/routes --exec cat {}
Length of output: 102549
Script:
#!/bin/bash
# Check device-registry routes for v1 and v2 endpoint definitions
echo "Checking device-registry routes for API versions..."
fd -e js . src/device-registry/routes/v1 --exec cat {}
fd -e js . src/device-registry/routes/v2 --exec cat {}
# Check for any v1 device endpoints in use
echo "Checking for v1 device endpoint usage..."
rg "api/v1/devices" --type js -A 2 src/device-registry/
Length of output: 155385
function extractBeforeV2IncludingSlash(url) { | ||
const v2Index = url.indexOf("v2/"); | ||
|
||
if (v2Index !== -1) { | ||
// Found "v2/" in the string | ||
return url.substring(0, v2Index + 3); // Include "v2/" | ||
} else { | ||
// "v2/" not found, return the original string | ||
return url; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add input validation and edge case handling to the utility function
The function needs to be more robust to handle various edge cases and invalid inputs.
Consider this improved implementation:
function extractBeforeV2IncludingSlash(url) {
+ if (!url || typeof url !== 'string') {
+ return '';
+ }
+
const v2Index = url.indexOf("v2/");
if (v2Index !== -1) {
- // Found "v2/" in the string
return url.substring(0, v2Index + 3); // Include "v2/"
} else {
- // "v2/" not found, return the original string
return url;
}
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
function extractBeforeV2IncludingSlash(url) { | |
const v2Index = url.indexOf("v2/"); | |
if (v2Index !== -1) { | |
// Found "v2/" in the string | |
return url.substring(0, v2Index + 3); // Include "v2/" | |
} else { | |
// "v2/" not found, return the original string | |
return url; | |
} | |
} | |
function extractBeforeV2IncludingSlash(url) { | |
if (!url || typeof url !== 'string') { | |
return ''; | |
} | |
const v2Index = url.indexOf("v2/"); | |
if (v2Index !== -1) { | |
return url.substring(0, v2Index + 3); // Include "v2/" | |
} else { | |
return url; | |
} | |
} |
Description
Include site category data in Readings schema
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
Include site category data in Readings schema
Summary by CodeRabbit
New Features
Bug Fixes
Documentation