addressing a runtime error for listing cohorts #4040
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to two main files: Changes
Possibly related PRs
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 #4040 +/- ##
========================================
Coverage 11.73% 11.73%
========================================
Files 114 114
Lines 15349 15349
Branches 319 319
========================================
Hits 1801 1801
Misses 13548 13548
|
|
Device registry changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/device-registry/models/Cohort.js (1)
Line range hint
217-226: Consider optimizing the aggregation pipeline performance.The current implementation might face performance issues with large datasets:
- The
allowDiskUse(true)indicates potential memory pressure- The lookup operation followed by grouping could be expensive
Consider these optimizations:
- Add index on the
cohortsfield in the devices collection to optimize the lookup- If possible, limit the fields in lookup to reduce memory usage
- Consider pagination to limit the result set size
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/device-registry/models/Cohort.js(1 hunks)src/device-registry/utils/scripts/new-bulk-script.js(1 hunks)
🔇 Additional comments (1)
src/device-registry/models/Cohort.js (1)
Line range hint 217-226: LGTM! The MongoDB aggregation syntax is now correct.
The change from .groups() to .group() fixes the runtime error. This is the correct MongoDB aggregation stage syntax.
| const NETWORK_MAPPINGS = { | ||
| // iqair: "permian-health", | ||
| // usembassy: "us-embassy", | ||
| // urbanbetter: "urban-better", | ||
| // kcca: "kcca", | ||
| // airqo: "airqo", | ||
| // Add more mappings as needed | ||
| }; |
There was a problem hiding this comment.
Network mappings are all commented out.
All network mappings are commented out, which will cause all devices to be assigned to the "unknown" group.
Please uncomment and verify the required network mappings:
const NETWORK_MAPPINGS = {
- // iqair: "permian-health",
- // usembassy: "us-embassy",
- // urbanbetter: "urban-better",
- // kcca: "kcca",
- // airqo: "airqo",
+ iqair: "permian-health",
+ usembassy: "us-embassy",
+ urbanbetter: "urban-better",
+ kcca: "kcca",
+ airqo: "airqo",
// Add more mappings as needed
};📝 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 NETWORK_MAPPINGS = { | |
| // iqair: "permian-health", | |
| // usembassy: "us-embassy", | |
| // urbanbetter: "urban-better", | |
| // kcca: "kcca", | |
| // airqo: "airqo", | |
| // Add more mappings as needed | |
| }; | |
| const NETWORK_MAPPINGS = { | |
| iqair: "permian-health", | |
| usembassy: "us-embassy", | |
| urbanbetter: "urban-better", | |
| kcca: "kcca", | |
| airqo: "airqo", | |
| // Add more mappings as needed | |
| }; |
| const url = "http://localhost:3000/api/v2/devices/sites"; | ||
| const config = { | ||
| headers: { | ||
| Authorization: "", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Security concern: Hardcoded URL and empty Authorization header.
The script contains security vulnerabilities:
- Hardcoded localhost URL
- Empty Authorization header
Consider this safer implementation:
-const url = "http://localhost:3000/api/v2/devices/sites";
-const config = {
- headers: {
- Authorization: "",
- },
-};
+const url = process.env.API_URL || "http://localhost:3000/api/v2/devices/sites";
+const config = {
+ headers: {
+ Authorization: process.env.API_TOKEN,
+ },
+};📝 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/sites"; | |
| const config = { | |
| headers: { | |
| Authorization: "", | |
| }, | |
| }; | |
| const url = process.env.API_URL || "http://localhost:3000/api/v2/devices/sites"; | |
| const config = { | |
| headers: { | |
| Authorization: process.env.API_TOKEN, | |
| }, | |
| }; |
| for (let i = 0; i < response.data.sites.length; i += 10) { | ||
| const batch = response.data.sites.slice(i, i + 10); | ||
|
|
||
| for (const site of batch) { | ||
| const group = NETWORK_MAPPINGS[site.network] || DEFAULT_GROUP; | ||
|
|
||
| if (group !== DEFAULT_GROUP) { | ||
| const url = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`; | ||
| const data = { groups: [group] }; | ||
| // console.log("the data:"); | ||
| // console.dir(data); | ||
|
|
||
| try { | ||
| // Uncomment if you want to make the PUT request | ||
| const putResponse = await axios.put(url, data, config); | ||
| console.log("PUT response:", putResponse.data); | ||
| } catch (error) { | ||
| console.error("PUT error:", error.message); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and batch processing.
The current batch processing implementation:
- Lacks retry mechanism for failed requests
- Doesn't track failed updates
- Could benefit from concurrent processing
Consider this enhanced implementation:
- for (let i = 0; i < response.data.sites.length; i += 10) {
- const batch = response.data.sites.slice(i, i + 10);
-
- for (const site of batch) {
+ const processBatch = async (sites) => {
+ const results = await Promise.allSettled(
+ sites.map(async (site) => {
const group = NETWORK_MAPPINGS[site.network] || DEFAULT_GROUP;
if (group !== DEFAULT_GROUP) {
const url = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`;
const data = { groups: [group] };
try {
- const putResponse = await axios.put(url, data, config);
+ const putResponse = await axios.put(url, data, config).catch(async (error) => {
+ if (error.response?.status === 429) {
+ await new Promise(resolve => setTimeout(resolve, 1000));
+ return axios.put(url, data, config);
+ }
+ throw error;
+ });
console.log("PUT response:", putResponse.data);
+ return { success: true, site: site._id };
} catch (error) {
console.error("PUT error:", error.message);
+ return { success: false, site: site._id, error: error.message };
}
}
+ return { success: true, site: site._id, skipped: true };
+ })
+ );
+ return results;
+ };
+ const BATCH_SIZE = 10;
+ const failedUpdates = [];
+
+ for (let i = 0; i < response.data.sites.length; i += BATCH_SIZE) {
+ const batch = response.data.sites.slice(i, i + BATCH_SIZE);
+ const results = await processBatch(batch);
+
+ results.forEach(result => {
+ if (result.status === 'rejected' || !result.value.success) {
+ failedUpdates.push(result.value || result.reason);
+ }
+ });
+
+ // Add a small delay between batches to prevent rate limiting
+ await new Promise(resolve => setTimeout(resolve, 100));
}
+
+ if (failedUpdates.length > 0) {
+ console.error('Failed updates:', failedUpdates);
+ }📝 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); | |
| for (const site of batch) { | |
| const group = NETWORK_MAPPINGS[site.network] || DEFAULT_GROUP; | |
| if (group !== DEFAULT_GROUP) { | |
| const url = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`; | |
| const data = { groups: [group] }; | |
| // console.log("the data:"); | |
| // console.dir(data); | |
| try { | |
| // Uncomment if you want to make the PUT request | |
| const putResponse = await axios.put(url, data, config); | |
| console.log("PUT response:", putResponse.data); | |
| } catch (error) { | |
| console.error("PUT error:", error.message); | |
| } | |
| } | |
| } | |
| } | |
| const processBatch = async (sites) => { | |
| const results = await Promise.allSettled( | |
| sites.map(async (site) => { | |
| const group = NETWORK_MAPPINGS[site.network] || DEFAULT_GROUP; | |
| if (group !== DEFAULT_GROUP) { | |
| const url = `http://localhost:3000/api/v2/devices/sites?id=${site._id}`; | |
| const data = { groups: [group] }; | |
| try { | |
| const putResponse = await axios.put(url, data, config).catch(async (error) => { | |
| if (error.response?.status === 429) { | |
| await new Promise(resolve => setTimeout(resolve, 1000)); | |
| return axios.put(url, data, config); | |
| } | |
| throw error; | |
| }); | |
| console.log("PUT response:", putResponse.data); | |
| return { success: true, site: site._id }; | |
| } catch (error) { | |
| console.error("PUT error:", error.message); | |
| return { success: false, site: site._id, error: error.message }; | |
| } | |
| } | |
| return { success: true, site: site._id, skipped: true }; | |
| }) | |
| ); | |
| return results; | |
| }; | |
| const BATCH_SIZE = 10; | |
| const failedUpdates = []; | |
| for (let i = 0; i < response.data.sites.length; i += BATCH_SIZE) { | |
| const batch = response.data.sites.slice(i, i + BATCH_SIZE); | |
| const results = await processBatch(batch); | |
| results.forEach(result => { | |
| if (result.status === 'rejected' || !result.value.success) { | |
| failedUpdates.push(result.value || result.reason); | |
| } | |
| }); | |
| // Add a small delay between batches to prevent rate limiting | |
| await new Promise(resolve => setTimeout(resolve, 100)); | |
| } | |
| if (failedUpdates.length > 0) { | |
| console.error('Failed updates:', failedUpdates); | |
| } |
Description
addressing a runtime error for listing cohorts
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
addressing a runtime error for listing cohorts
Summary by CodeRabbit
New Features
Bug Fixes