support for bulk update of devices #4047
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request introduce a new method for bulk updating devices to the device controller and the device model, along with a new route and validation for handling these updates. The Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #4047 +/- ##
===========================================
- Coverage 11.73% 11.68% -0.05%
===========================================
Files 114 114
Lines 15349 15427 +78
Branches 319 319
===========================================
+ Hits 1801 1803 +2
- Misses 13548 13624 +76
|
|
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 (5)
src/device-registry/utils/create-device.js (2)
729-729: Remove unused variable 'providedDeviceIds'The variable
providedDeviceIdsis declared but not used in the function. Removing it will clean up the code and prevent any confusion.Apply this diff:
- const providedDeviceIds = new Set(deviceIds.map((id) => id.toString()));
756-756: Ensure correct merging of filtersUsing
Object.assign(filter, additionalFilter);could potentially overwrite existing properties in thefilterobject. To avoid unintended side effects, consider merging the filters carefully, possibly using the spread operator or combining them with$and.Example:
- Object.assign(filter, additionalFilter); + const combinedFilter = { $and: [filter, additionalFilter] };src/device-registry/models/Device.js (1)
671-671: Remove unnecessarynewoption inupdateManyThe
newoption is not applicable toupdateManyoperations in Mongoose; it is used withfindOneAndUpdate. Removing it will clarify the code and prevent confusion.Apply this diff:
{ - new: true, runValidators: true, ...opts, }src/device-registry/controllers/create-device.js (2)
800-800: Consider enhancing debug logging.To improve debugging capabilities, consider logging the request payload size and validation details.
logText("the BULK update operation starts...."); +logObject("bulk update payload", { size: req.body.length });
826-830: Consider enriching the success response.The success response could be more informative by including summary statistics of the bulk update operation.
return res.status(status).json({ message: result.message, success: true, - bulk_update_notes: result.data, + bulk_update_notes: result.data, + summary: { + total: result.data.length, + successful: result.data.filter(note => note.success).length, + failed: result.data.filter(note => !note.success).length + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/device-registry/controllers/create-device.js(1 hunks)src/device-registry/models/Device.js(1 hunks)src/device-registry/routes/v2/devices.js(2 hunks)src/device-registry/utils/create-device.js(1 hunks)src/device-registry/validators/device.validators.js(2 hunks)
🔇 Additional comments (4)
src/device-registry/utils/create-device.js (1)
788-788:
Fix string interpolation in error logging statement
The error logging statement on line 788 uses single quotes, which do not support template literals in JavaScript. To correctly interpolate the error message, please use backticks.
Apply this diff to fix the issue:
- logger.error('🐛🐛 Bulk Update Error: ${error.message}');
+ logger.error(`🐛🐛 Bulk Update Error: ${error.message}`);Likely invalid or redundant comment.
src/device-registry/routes/v2/devices.js (1)
129-135: Bulk update route added appropriately
The new route for bulk updating devices is correctly defined with the necessary validations and controller method. This enhances the API's capabilities effectively.
src/device-registry/validators/device.validators.js (1)
773-799: Ensure allowed fields in updateData are consistent
Double-check that the allowedFields array includes all valid fields and is consistent with the fields validated in validateUpdateDevice. This ensures that only permissible fields are updated.
src/device-registry/controllers/create-device.js (1)
798-852: LGTM! The implementation follows consistent patterns.
The new updateManyDevicesOnPlatform method follows the established controller patterns for error handling, request processing, and response formatting. The implementation is clean and maintainable.
| ...responseFromBulkModifyDevices, | ||
| metadata: { | ||
| totalDevicesUpdated: responseFromBulkModifyDevices.modifiedCount, |
There was a problem hiding this comment.
Correct property access for modifiedCount in response metadata
The modifiedCount property is nested under data in the response from bulkModify. Access it using responseFromBulkModifyDevices.data.modifiedCount to ensure accurate metadata.
Apply this diff:
- totalDevicesUpdated: responseFromBulkModifyDevices.modifiedCount,
+ totalDevicesUpdated: responseFromBulkModifyDevices.data.modifiedCount,📝 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.
| ...responseFromBulkModifyDevices, | |
| metadata: { | |
| totalDevicesUpdated: responseFromBulkModifyDevices.modifiedCount, | |
| ...responseFromBulkModifyDevices, | |
| metadata: { | |
| totalDevicesUpdated: responseFromBulkModifyDevices.data.modifiedCount, |
src/device-registry/models/Device.js
Outdated
| message: "No devices were updated", | ||
| data: { | ||
| modifiedCount: 0, | ||
| matchedCount: bulkUpdateResult.matchedCount, |
There was a problem hiding this comment.
Correct matchedCount property in response data
The bulkUpdateResult from updateMany does not have a matchedCount property. Use bulkUpdateResult.n to get the number of matched documents.
Apply this diff:
- matchedCount: bulkUpdateResult.matchedCount,
+ matchedCount: bulkUpdateResult.n,📝 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.
| matchedCount: bulkUpdateResult.matchedCount, | |
| matchedCount: bulkUpdateResult.n, |
| .exists() | ||
| .withMessage("updateData must be provided in the request body") | ||
| .bail() | ||
| .isObject() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace .isObject() with a custom validator
The .isObject() method is not a standard validator in express-validator. Using a custom validator ensures that updateData is correctly validated as an object.
Apply this diff:
- .isObject()
- .withMessage("updateData must be an object")
+ .custom((value) => {
+ if (typeof value !== 'object' || Array.isArray(value) || value === null) {
+ throw new Error('updateData must be an object');
+ }
+ return true;
+ })📝 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.
| .isObject() | |
| .custom((value) => { | |
| if (typeof value !== 'object' || Array.isArray(value) || value === null) { | |
| throw new Error('updateData must be an object'); | |
| } | |
| return true; | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
src/device-registry/models/Device.js (1)
682-683: Update response structure for MongoDB compatibilityThe
nModifiedproperty is deprecated in newer MongoDB versions. Consider usingmodifiedCountinstead.data: { - modifiedCount: bulkUpdateResult.nModified, - matchedCount: bulkUpdateResult.n, + modifiedCount: bulkUpdateResult.modifiedCount, + matchedCount: bulkUpdateResult.matchedCount, },src/device-registry/validators/device.validators.js (2)
739-740: Extract MAX_BULK_UPDATE_DEVICES to constantsMove the magic number to a configuration constant for better maintainability.
+const MAX_BULK_UPDATE_DEVICES = 30; - const MAX_BULK_UPDATE_DEVICES = 30; if (value.length > MAX_BULK_UPDATE_DEVICES) {
777-803: Extract allowed fields to a reusable constantThe allowed fields list should be extracted to a reusable constant to maintain DRY principle and ensure consistency across the codebase.
+const ALLOWED_DEVICE_UPDATE_FIELDS = [ + "visibility", + "long_name", + // ... other fields +]; - const allowedFields = [ - "visibility", - "long_name", - // ... other fields - ]; + const allowedFields = ALLOWED_DEVICE_UPDATE_FIELDS;src/device-registry/controllers/create-device.js (2)
800-801: Enhance logging for bulk operationsAdd structured logging with operation metadata for better monitoring and debugging.
- logText("the BULK update operation starts...."); + logger.info("Bulk update operation started", { + deviceCount: req.body.deviceIds.length, + tenant: req.query.tenant, + updateFields: Object.keys(req.body.updateData) + });
815-819: Add request tracking for bulk operationsConsider adding request tracking for bulk operations to help with monitoring and debugging.
Consider implementing:
- Request correlation IDs
- Operation timing metrics
- Progress tracking for large bulk updates
src/device-registry/utils/create-device.js (4)
749-757: Consider adding index hint for bulk operations.While the filter construction is correct, for large-scale operations, consider adding an index hint to optimize the query performance.
const filter = { _id: { $in: deviceIds }, + $hint: { _id: 1 } // Add index hint for better performance };
766-776: Consider implementing batch processing for large updates.For better scalability, consider implementing batch processing when dealing with a large number of devices. This would help manage memory usage and prevent potential timeouts.
// Perform bulk update +const BATCH_SIZE = 1000; +const batches = []; +for (let i = 0; i < deviceIds.length; i += BATCH_SIZE) { + const batchIds = deviceIds.slice(i, i + BATCH_SIZE); + const batchFilter = { ...filter, _id: { $in: batchIds } }; + batches.push( + DeviceModel(tenant).bulkModify( + { + filter: batchFilter, + update: updateData, + opts, + }, + next + ) + ); +} +const results = await Promise.all(batches);
779-786: Enhance response metadata with timing information.Consider adding execution timing information to the metadata for monitoring and optimization purposes.
return { ...responseFromBulkModifyDevices, metadata: { totalDevicesUpdated: responseFromBulkModifyDevices.data.modifiedCount, requestedDeviceIds: deviceIds, existingDeviceIds: Array.from(existingDeviceIds), + executionTimeMs: Date.now() - startTime, // Add execution time }, };
787-796: Enhance error logging for bulk operations.The error handling is good, but consider adding more context about the failed operation in the logs.
-logger.error(`🐛🐛 Bulk Update Error: ${error.message}`); +logger.error(`🐛🐛 Bulk Update Error for ${deviceIds.length} devices: ${error.message}`, { + tenant, + deviceCount: deviceIds.length, + errorStack: error.stack +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/device-registry/controllers/create-device.js(1 hunks)src/device-registry/models/Device.js(1 hunks)src/device-registry/utils/create-device.js(1 hunks)src/device-registry/validators/device.validators.js(2 hunks)
🔇 Additional comments (2)
src/device-registry/utils/create-device.js (2)
716-747: Robust device existence validation implementation.
The implementation thoroughly validates device existence with detailed error reporting. The use of Sets for comparison is an efficient approach.
759-764: Verify the impact of runValidators on bulk performance.
The runValidators: true option ensures data integrity but might impact performance for large updates. Consider adding a warning in the documentation about potential performance implications for bulk operations.
✅ Verification successful
The previous pattern didn't work well with ast-grep, so I'm using ripgrep to search for Schema definitions and any existing documentation about validation performance in bulk operations. This will help us understand the validation rules in place and any documented performance considerations.
I'm searching for actual bulk update operations and any performance-related documentation in the codebase, particularly around the file in question. This will help us understand if there are any existing patterns or documented performance considerations for bulk operations with validation.
The runValidators option is safe to use in this bulk update context.
Based on the codebase analysis:
- The update operation is used in a controlled context with a specific filter and limited scope
- The schema validations are relatively simple, mostly consisting of:
- Basic type checks (String, Number, Boolean)
- Required field validations
- Trim operations
- Simple enum validations
The validation overhead is minimal for these types of validations, and the data integrity benefits outweigh any minor performance impact. The codebase shows consistent use of validation across models, indicating it's a deliberate architectural choice for maintaining data quality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential validation rules that might impact performance
ast-grep --pattern 'const $schema = new Schema({
$$$
})'
Length of output: 60
Script:
#!/bin/bash
# Let's try a different approach to find mongoose schema definitions and validation rules
rg -A 10 "new Schema\("
# Also search for any documentation about bulk operations and validation
rg -i "runValidators.*bulk|bulk.*runValidators"
# Search for any performance-related comments or documentation
rg -i "performance.*validation|validation.*performance"
Length of output: 67382
Script:
#!/bin/bash
# Let's search for any bulk operations in the codebase
rg -B 2 -A 2 "updateMany|update\(\{.*\}\s*,\s*\{.*\}\s*,\s*\{.*runValidators.*\}\)"
# Also search for any mongoose update operations with options
rg -B 2 -A 2 "update.*\{.*\}.*\{.*\}.*\{.*\}"
# Look for any performance-related comments or documentation in the file
rg -C 5 "performance|optimization" src/device-registry/utils/create-device.js
Length of output: 46454
| if (bulkUpdateResult.nModified > 0) { | ||
| return { | ||
| success: true, | ||
| message: `Successfully modified ${bulkUpdateResult.nModified} devices`, | ||
| data: { | ||
| modifiedCount: bulkUpdateResult.nModified, | ||
| matchedCount: bulkUpdateResult.n, | ||
| }, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } else { | ||
| return { | ||
| success: false, | ||
| message: "No devices were updated", | ||
| data: { | ||
| modifiedCount: 0, | ||
| matchedCount: bulkUpdateResult.n, | ||
| }, | ||
| status: httpStatus.NOT_FOUND, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using transactions for atomic updates
The bulk update operation should be wrapped in a transaction to ensure atomicity across multiple document updates. This prevents partial updates in case of failures.
if (bulkUpdateResult.nModified > 0) {
+ const session = await mongoose.startSession();
+ try {
+ await session.withTransaction(async () => {
return {
success: true,
message: `Successfully modified ${bulkUpdateResult.nModified} devices`,
data: {
modifiedCount: bulkUpdateResult.nModified,
matchedCount: bulkUpdateResult.n,
},
status: httpStatus.OK,
};
+ });
+ } finally {
+ session.endSession();
+ }
}Committable suggestion skipped: line range outside the PR's diff.
|
Device registry changes in this PR available for preview here |
Description
support for bulk update of devices
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
PUT: {{baseUrl}}/api/v2/devices/bulkAPI Documentation Updated?
Summary by CodeRabbit
New Features
/bulkendpoint.Bug Fixes