adding input validations for the devices update routes #3707
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new constant Changes
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 #3707 +/- ##
========================================
Coverage 11.59% 11.59%
========================================
Files 108 108
Lines 14871 14875 +4
Branches 274 274
========================================
+ Hits 1724 1725 +1
- Misses 13147 13150 +3
|
|
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: 1
🧹 Outside diff range and nitpick comments (3)
src/device-registry/models/Device.js (1)
Line range hint
249-257: Excellent addition of duplicate checking for thecohortsarray!The pre-save hook effectively prevents data inconsistency by identifying and handling duplicate values in the
cohortsarray. The logic is sound and efficient.To further enhance error handling, consider using a custom error class or the
HttpErrorclass that's already imported in this file. This would provide more consistent error handling across the application.Here's a suggested improvement:
deviceSchema.pre("save", function(next) { // ... (existing code) // Check for duplicate values in the grids array const duplicateValues = this.cohorts.filter( (value, index, self) => self.indexOf(value) !== index ); if (duplicateValues.length > 0) { - const error = new Error("Duplicate values found in cohorts array."); - return next(error); + return next(new HttpError("Bad Request", httpStatus.BAD_REQUEST, { + message: "Duplicate values found in cohorts array.", + duplicates: duplicateValues + })); } return next(); });This change would provide more structured error information and maintain consistency with other error handling in the application.
src/device-registry/routes/v2/devices.js (2)
1046-1062: Excellent addition of validation for device_codes!The new validation for the
device_codesfield is a great improvement to ensure data integrity. It correctly checks if the field is an array and not empty when provided.A small suggestion to enhance code readability:
Consider using the
isArray()method instead of a custom function:body("device_codes") .optional() - .custom((value) => { - return Array.isArray(value); - }) + .isArray() .withMessage("the device_codes should be an array if provided") .bail() .notEmpty() .withMessage("the device_codes should not be empty if provided"),This change would make the validation more concise and easier to understand at a glance.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1049-1049: src/device-registry/routes/v2/devices.js#L1049
Added line #L1049 was not covered by tests
1251-1261: Great addition of category validation!The new validation for the
categoryfield is a valuable improvement. It ensures that the field, when provided, is not empty and contains one of the allowed values: "bam", "lowcost", or "gas".A small suggestion for consistency:
Consider updating the error message to match the case of the allowed values:
body("category") .optional() .notEmpty() .withMessage("category should not be empty IF provided") .bail() .trim() .toLowerCase() .isIn(["bam", "lowcost", "gas"]) .withMessage( - "the category value is not among the expected ones which include: LOWCOST, GAS and BAM" + "the category value is not among the expected ones which include: lowcost, gas and bam" ),This change would make the error message consistent with the actual allowed values, which are in lowercase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/device-registry/models/Device.js (2 hunks)
- src/device-registry/routes/v2/devices.js (4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/device-registry/routes/v2/devices.js
[warning] 1049-1049: src/device-registry/routes/v2/devices.js#L1049
Added line #L1049 was not covered by tests
[warning] 1438-1438: src/device-registry/routes/v2/devices.js#L1438
Added line #L1438 was not covered by tests
[warning] 1736-1736: src/device-registry/routes/v2/devices.js#L1736
Added line #L1736 was not covered by tests
🔇 Additional comments (4)
src/device-registry/models/Device.js (3)
37-41: Excellent addition of theDEVICE_CATEGORIESconstant!The use of
Object.freeze()ensures immutability, which is a great practice. The categories are well-defined and align perfectly with the PR objectives. The naming convention is consistent and follows best practices.
204-208: Great update to thecategoryfield indeviceSchema!The use of
DEVICE_CATEGORIESfor theenumproperty ensures consistency with the defined categories. Setting a default value of "lowcost" is a sensible choice for the most common use case. These changes significantly enhance data integrity by restricting thecategoryfield to predefined values.
Line range hint
1-1000: Overall, excellent enhancements to the Device model!The changes in this file significantly improve data integrity and consistency in the device model. The introduction of the
DEVICE_CATEGORIESconstant, along with the updatedcategoryfield in the schema, enhances the robustness of the device update functionality. The addition of duplicate checking in the pre-save hook for thecohortsarray adds an extra layer of data validation.These modifications align perfectly with the PR objectives of implementing input validations for the device update routes. They provide a solid foundation for ensuring data consistency and preventing errors during device updates.
Great job on these improvements!
src/device-registry/routes/v2/devices.js (1)
Line range hint
1046-1752: Summary: Excellent enhancements to input validationOverall, the changes in this file significantly improve the input validation for device update operations. The new validations for
device_codesandcategoryfields are consistently implemented across different routes, enhancing data integrity and providing clearer feedback to API users.A few points to consider:
- The suggested refactorings in previous comments would further improve code maintainability.
- The static analysis tool indicates that some of the new lines are not covered by tests.
To address the test coverage issue, please run the following command to identify which specific test files need to be updated:
Once you've identified the relevant test files, consider adding test cases that specifically target the new validations for
device_codesandcategoryfields. This will ensure that these important additions are properly tested and maintain high code quality.Great work on improving the robustness of the device update functionality!
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1736-1736: src/device-registry/routes/v2/devices.js#L1736
Added line #L1736 was not covered by tests
| body("device_codes") | ||
| .optional() | ||
| .custom((value) => { | ||
| return Array.isArray(value); | ||
| }) | ||
| .withMessage("the device_codes should be an array if provided") | ||
| .bail() | ||
| .notEmpty() | ||
| .withMessage("the device_codes should not be empty if provided"), | ||
| body("category") | ||
| .optional() | ||
| .notEmpty() | ||
| .withMessage("the category should not be empty if provided") | ||
| .bail() | ||
| .trim() | ||
| .toLowerCase() | ||
| .isIn(["bam", "lowcost", "gas"]) | ||
| .withMessage( | ||
| "the category value is not among the expected ones which include: lowcost, bam and gas" | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent validation for soft update route - well done!
The addition of validations for device_codes and category fields in the soft update route is excellent. This ensures consistency across different update operations and improves data integrity.
However, I noticed that these validations are duplicated from the previous update route. To improve maintainability:
Consider extracting these common validations into a separate function or middleware. This would reduce code duplication and make future updates easier. Here's a possible approach:
const validateDeviceUpdateFields = [
body("device_codes")
.optional()
.isArray()
.withMessage("the device_codes should be an array if provided")
.bail()
.notEmpty()
.withMessage("the device_codes should not be empty if provided"),
body("category")
.optional()
.notEmpty()
.withMessage("the category should not be empty if provided")
.bail()
.trim()
.toLowerCase()
.isIn(["bam", "lowcost", "gas"])
.withMessage(
"the category value is not among the expected ones which include: lowcost, bam and gas"
),
// ... other common validations
];
// Then use it in your routes:
router.put("/", ..., validateDeviceUpdateFields, ...);
router.put("/soft", ..., validateDeviceUpdateFields, ...);This refactoring would make the code more DRY (Don't Repeat Yourself) and easier to maintain.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1736-1736: src/device-registry/routes/v2/devices.js#L1736
Added line #L1736 was not covered by tests
Description
adding input validations for the devices update routes
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
adding input validations for the devices update routes
Summary by CodeRabbit
New Features
device_codesandcategoryto device update routes, enhancing data management.Bug Fixes
Documentation