runtime error fix for limit input validation on best air quality endpoint #3837
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on enhancing the validation logic for query parameters in the Express.js route handlers within the 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 #3837 +/- ##
========================================
Coverage 11.77% 11.77%
========================================
Files 114 114
Lines 15205 15205
Branches 274 274
========================================
Hits 1791 1791
Misses 13414 13414
|
|
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 (1)
src/device-registry/routes/v2/readings.js (1)
86-88: Consider aligning skip validation with middleware behaviorThe route validation for
skipparameter could be better aligned with thevalidatePaginationmiddleware behavior. Currently:
- Route validation allows non-negative integers
- Middleware sets skip to 0 if it's negative or NaN
While not critical, consistency in validation would improve code maintainability.
Consider updating the validation to explicitly document the middleware behavior:
query("skip") .optional() .trim() .notEmpty() .withMessage("skip cannot be empty if provided") .bail() .isInt({ min: 0 }) - .withMessage("skip must be a non-negative integer") + .withMessage("skip must be a non-negative integer (defaults to 0)") .toInt(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/device-registry/routes/v2/readings.js(1 hunks)
🔇 Additional comments (2)
src/device-registry/routes/v2/readings.js (2)
69-69: Good addition of the bail() method!
The addition of .bail() improves the validation flow by stopping further checks if the language parameter is empty, which enhances error handling clarity.
77-79: Verify error handling for the validation changes
Let's ensure the validation changes properly handle various edge cases and prevent runtime errors.
Also applies to: 86-88
| .bail() | ||
| .isInt({ min: 0 }) | ||
| .withMessage("limit must be a non-negative integer") |
There was a problem hiding this comment.
Critical: Inconsistent limit validation between middleware and route
There's a validation conflict between:
- The route validation that allows non-negative integers (including 0)
- The
validatePaginationmiddleware that:- Sets minimum to 1
- Sets default to 1000
- Sets maximum to 2000
This inconsistency could lead to confusing behavior where a limit of 0 passes the route validation but gets modified by the middleware.
Consider aligning the validations by updating the route validation to match the middleware constraints:
query("limit")
.optional()
.trim()
.notEmpty()
.withMessage("limit cannot be empty if provided")
.bail()
- .isInt({ min: 0 })
- .withMessage("limit must be a non-negative integer")
+ .isInt({ min: 1, max: 2000 })
+ .withMessage("limit must be between 1 and 2000")
.toInt(),📝 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.
| .bail() | |
| .isInt({ min: 0 }) | |
| .withMessage("limit must be a non-negative integer") | |
| .bail() | |
| .isInt({ min: 1, max: 2000 }) | |
| .withMessage("limit must be between 1 and 2000") |
…he middleware constraints
|
Device registry changes in this PR available for preview here |
Description
Fixing the runtime for limit input validation on best air quality endpoint
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Additional Notes
Fixing the runtime for limit input validation on best air quality endpoint
Summary by CodeRabbit
Bug Fixes
limit(now accepts values between 1 and 2000) andskip(now requires a non-negative integer).Chores