fix(flashcards): handle invalid rating types and values in reviewFlashcard (#1199) - #1805
fix(flashcards): handle invalid rating types and values in reviewFlashcard (#1199)#1805suhaniiz wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughFlashcard reviews now normalize and validate numeric or labeled ratings before SM-2 processing. Adzuna country configuration and request values are trimmed and lowercased before URL construction. ChangesFlashcard rating validation
Adzuna country normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/controllers/flashcardController.js`:
- Around line 5-9: Update calculateSM2 so both numeric 5 and string "5" enter
the existing easy/score-5 branch instead of falling through to score 3; preserve
ALLOWED_RATINGS and add tests covering both accepted forms and their expected
interval/efactor results.
- Around line 192-196: In the review request handler containing the rating
validation and Flashcard.findOne call, validate the incoming id as a valid
ObjectId before querying. Return the existing client-error response for
malformed ids and only execute Flashcard.findOne with validated ids, preserving
the current review flow for valid requests.
In `@backend/controllers/jobController.js`:
- Line 7: Validate ADZUNA_COUNTRY after normalization against
ALLOWED_ADZUNA_COUNTRIES and fall back to the established safe country when
unsupported. Ensure the validated constant is used by fetchFromAdzuna’s default
parameter and any endpoint construction, including the direct caller path that
bypasses getJobs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17f7b5b0-eb80-40f1-b853-978946b4f6ff
📒 Files selected for processing (2)
backend/controllers/flashcardController.jsbackend/controllers/jobController.js
| const ALLOWED_RATINGS = new Set([ | ||
| "again", "hard", "medium", "good", "easy", | ||
| "1", "2", "3", "4", "5", | ||
| 1, 2, 3, 4, 5 | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Map rating 5 to the score-5 path.
ALLOWED_RATINGS accepts both "5" and 5, but calculateSM2 has no branch for either value. Both inputs fall through to score = 3, which persists incorrect interval and efactor values for a valid rating. Add both forms to the easy branch and test them.
Proposed fix
- else if (normalizedRating === "easy" || normalizedRating === "4" || normalizedRating === 4) score = 5;
+ else if (
+ normalizedRating === "easy" ||
+ normalizedRating === "4" ||
+ normalizedRating === 4 ||
+ normalizedRating === "5" ||
+ normalizedRating === 5
+ ) score = 5;Also applies to: 18-22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/flashcardController.js` around lines 5 - 9, Update
calculateSM2 so both numeric 5 and string "5" enter the existing easy/score-5
branch instead of falling through to score 3; preserve ALLOWED_RATINGS and add
tests covering both accepted forms and their expected interval/efactor results.
| // 4. Check if rating is in allowed list | ||
| if (!ALLOWED_RATINGS.has(normalizedRating)) { | ||
| return res.status(400).json({ | ||
| success: false, | ||
| message: "Rating is required. Supported values: 'again', 'hard', 'good', 'easy'.", | ||
| message: `Invalid rating '${rating}'. Supported values: 'again', 'hard', 'medium', 'good', 'easy' or numbers 1-5.`, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 10 'validateReviewFlashcard|ObjectId\.isValid|isValidObjectId|Flashcard\.findOne|reviewFlashcard' .Repository: Canopus-Labs/PrepPilot
Length of output: 20637
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- controller review handler ---'
sed -n '162,238p' backend/controllers/flashcardController.js
printf '%s\n' '--- flashcard validator and routes ---'
sed -n '1,45p' backend/Input_validators/ValidateFlashcard.js
sed -n '1,35p' backend/routes/flashcardRoutes.js
printf '%s\n' '--- error handling and app registration ---'
rg -n -C 8 'errorHandler|err, req, res, next|app\.use|flashcardRoutes|/api/flashcards' backend --glob '*.js'
printf '%s\n' '--- dependency versions ---'
rg -n -C 2 '"mongoose"|"express"' backend/package.json package.json 2>/dev/null || trueRepository: Canopus-Labs/PrepPilot
Length of output: 19382
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
controller = Path("backend/controllers/flashcardController.js").read_text()
validator = Path("backend/Input_validators/ValidateFlashcard.js").read_text()
review = controller[controller.index("const reviewFlashcard"):controller.index("const deleteFlashcard")]
query_pos = review.index("Flashcard.findOne({ _id: id, userId })")
prefix = review[:query_pos]
has_id_guard = bool(re.search(
r"(?:isValidObjectId|ObjectId\.isValid)\s*\(\s*id\s*\)", prefix
))
catch_returns_500 = bool(re.search(
r"catch\s*\([^)]*\)\s*\{[\s\S]*?res\.status\(500\)",
review
))
validator_checks_id = bool(re.search(
r"\bid\b[\s\S]{0,300}(?:isValidObjectId|ObjectId\.isValid)",
validator
))
print({
"review_query_present": True,
"object_id_guard_before_review_query": has_id_guard,
"review_catch_returns_500": catch_returns_500,
"review_validator_checks_id": validator_checks_id,
})
assert not has_id_guard
assert catch_returns_500
assert not validator_checks_id
PYRepository: Canopus-Labs/PrepPilot
Length of output: 308
🌐 Web query:
Mongoose 8 findOne invalid ObjectId cast error CastError query
💡 Result:
In Mongoose 8, a CastError occurring during a findOne or similar query indicates that Mongoose failed to convert a provided value into the data type defined in your schema for that specific path [1][2]. When querying by _id, Mongoose automatically expects an ObjectId; if you provide a string that is not a valid 24-character hex string, Mongoose will throw a CastError before the query is ever sent to MongoDB [3][1]. To resolve or handle this error, you can use the following strategies: 1. Validate the Input: Before executing the query, check if the provided string is a valid ObjectId using mongoose.Types.ObjectId.isValid(id) [4][5][6]. If it is invalid, you can handle it immediately (e.g., return null or a 400 Bad Request error) without running the database operation [6][7]. if (!mongoose.Types.ObjectId.isValid(id)) { return null; // Or handle as an invalid request } const doc = await MyModel.findOne({ _id: id }); 2. Catch the Error: Use standard try/catch blocks with async/await to catch the CastError during execution [7]. Since the error is a specific Mongoose class, you can verify it using instance checks [8][1][2]. try { const doc = await MyModel.findOne({ _id: id }); } catch (err) { if (err instanceof mongoose.Error.CastError) { // Handle the cast error specifically (e.g., return null) } } 3. Schema Configuration (If _id is not an ObjectId): If your _id is intended to be a different type (such as a string or number), ensure your schema explicitly defines it as such [9]. Mongoose will only attempt to cast to ObjectId if the schema path is configured as an ObjectId (which is the default for _id) [9][3]. const schema = new mongoose.Schema({ _id: String // Explicitly set to String to avoid ObjectId casting }); If you are using sanitizeFilter (mongoose.set('sanitizeFilter', true)), be aware that this can sometimes cause unexpected CastErrors when using query operators ($lt, $gt) if the inputs are not properly trusted [10]. In such cases, wrapping the query object or values with mongoose.trusted may be required [10].
Citations:
- 1: https://mongoosejs.com/docs/8.x/docs/tutorials/query_casting.html
- 2: https://mongoosejs.com/docs/8.x/docs/api/error.html
- 3: If findOne({_id}) doesn't match any result, does it throw an Error instead of null? Automattic/mongoose#4060
- 4: https://stackoverflow.com/questions/69971873/why-findone-id-is-not-working-properly-in-mongoose-6-012
- 5: https://stackoverflow.com/questions/17223517/mongoose-casterror-cast-to-objectid-failed-for-value-object-object-at-path
- 6: https://stackoverflow.com/questions/14940660/whats-mongoose-error-cast-to-objectid-failed-for-value-xxx-at-path-id
- 7: https://stackoverflow.com/questions/49204683/how-to-make-a-mongoose-find-query-return-null-instead-of-throwing-a-casterror-wh
- 8: https://mongoosejs.com/docs/5.x/docs/tutorials/query_casting.html
- 9: https://stackoverflow.com/questions/72247761/mongoose-findone-query-still-trying-to-cast-to-objectid
- 10: Unexpected CastError on Date fields with $gt/$lt operators in find() query Automattic/mongoose#15634
Preserve ObjectId validation for review requests.
A malformed id reaches Flashcard.findOne({ _id: id, userId }) without validation. Mongoose raises a cast error, and the catch block returns 500. Add an ObjectId check before the query.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/flashcardController.js` around lines 192 - 196, In the
review request handler containing the rating validation and Flashcard.findOne
call, validate the incoming id as a valid ObjectId before querying. Return the
existing client-error response for malformed ids and only execute
Flashcard.findOne with validated ids, preserving the current review flow for
valid requests.
| const ADZUNA_APP_ID = process.env.ADZUNA_APP_ID; | ||
| const ADZUNA_API_KEY = process.env.ADZUNA_API_KEY; | ||
| const ADZUNA_COUNTRY = process.env.ADZUNA_COUNTRY || "in"; | ||
| const ADZUNA_COUNTRY = (process.env.ADZUNA_COUNTRY || "in").toLowerCase().trim(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate ADZUNA_COUNTRY before constructing requests.
Line 7 only trims and lowercases the environment value. An unsupported value such as USA becomes usa and remains invalid. The direct fetchFromAdzuna(normalizedRole) caller uses this value through the default parameter, so Lines 38-39 can construct an unsupported Adzuna endpoint without the ALLOWED_ADZUNA_COUNTRIES check in getJobs. Validate the configured value against the allowed list and use a safe validated fallback.
Also applies to: 38-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/jobController.js` at line 7, Validate ADZUNA_COUNTRY
after normalization against ALLOWED_ADZUNA_COUNTRIES and fall back to the
established safe country when unsupported. Ensure the validated constant is used
by fetchFromAdzuna’s default parameter and any endpoint construction, including
the direct caller path that bypasses getJobs.
|
@suhaniiz Address coderabbit suggestions |
2 participants
📝 Pull Request Description
Related Issue
Closes #1199
Summary
Fixed an unhandled
TypeErrorinreviewFlashcardwhere passing non-primitive data types (e.g. objects{"rating": {}}, arrays{"rating": []}) or unsupported rating strings caused runtime exceptions and a500 Internal Server Error.Added strict type checking and value whitelisting at the controller level so that invalid or malformed inputs return a clean
400 Bad Requestwith an informative error message beforecalculateSM2is called.Type of Change
How Has This Been Tested?
Tested endpoint
PUT /api/flashcards/:id/reviewvia Postman/cURL with the following cases:{"rating": {"invalid": "object"}}and{"rating": [1, 2]}-> Returns400 Bad Request(previously triggered 500 error).{"rating": "super_easy"}-> Returns400 Bad Request.{}and{"rating": null}-> Returns400 Bad Request.{"rating": "good"},{"rating": "HARD"}, and{"rating": 3}-> Returns200 OKand updates SM-2 parameters correctly.Screenshots (if applicable)
N/A (Backend API fix)
Checklist
Summary
reviewFlashcardvalidation for rating types and supported values.400 Bad Requestfor missing, invalid, or unsupported ratings.calculateSM2.jobController.js.