#58 Added [Feature] - Added Input Validation and Improved Error Handl… #98
Conversation
…rror Handling for Appointment API
WalkthroughA single file (Backend/routes/appointments.py) has been refactored to introduce structured error handling via a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (6)
Backend/routes/appointments.py (6)
13-14: Consider deferring logging configuration to the application level.Calling
logging.basicConfig()at module import time can override or conflict with application-level logging configuration. If the Flask app or other modules configure logging differently, this module-level setup may cause unexpected behavior.Consider removing
logging.basicConfig()here and letting the main application configure logging centrally, or guard it with a check.
40-47: Time validation is case-sensitive—consider accepting lowercase am/pm.The regex
(AM|PM)only matches uppercase. Users submitting"10:30 am"will get a validation error. Consider making the pattern case-insensitive:def _validate_is_time(self, is_time, field, value): """ The rule 'is_time': True enforces HH:MM AM/PM. """ if is_time: - pattern = r"^(0[1-9]|1[0-2]):[0-5][0-9]\s?(AM|PM)$" - if not re.match(pattern, value): + pattern = r"^(0[1-9]|1[0-2]):[0-5][0-9]\s?(AM|PM)$" + if not re.match(pattern, value, re.IGNORECASE): self._error(field, "Invalid time format. Expected HH:MM AM/PM")
119-140: Add specific error handling for invalid date format.Time errors get a specific message, but date format errors fall through to the generic handler. For consistent UX, add a specific check:
# Specific time format error if "time" in errors: # Our custom message is already "Invalid time format. Expected HH:MM AM/PM" return False, make_error( "Invalid time format. Expected HH:MM AM/PM", 400 ) + # Specific date format error + if "date" in errors: + return False, make_error( + "Invalid date format. Expected YYYY-MM-DD", + 400 + ) + # Specific status value error if "status" in errors:
163-164:close_db(db)parameter is ignored—clarify intent.Looking at
close_dbindb/db.py, the function pops from Flask'sgobject and ignores its parameter. Passingdbhere is misleading. This works becauseopen_db()stores the connection ing.db, but the code reads as if it's closing the passed connection.Consider either:
- Call
close_db()without arguments (clearer intent)- Or call
db.close()directly if you want explicit controlThis applies to all routes using
close_db(db)in their finally blocks.
230-233: Consider returning 201 and the created resource ID.For REST semantics,
POSTcreating a resource typically returns201 Created. Returning the new appointment ID helps clients reference the created resource:+ new_id = db.execute("SELECT last_insert_rowid()").fetchone()[0] return jsonify({ "status": "success", - "message": "Appointment added successfully" - }), 200 + "message": "Appointment added successfully", + "id": new_id + }), 201
107-117: Use Cerberus error codes instead of fragile string matching for required field detection.Checking for
"required field"as a substring in error messages is brittle and couples to Cerberus's internal message format. Use the built-in error code instead:from cerberus import errors missing_fields = [ field for field, errs in v.document_error_tree.items() if errors.REQUIRED_FIELD in errs ]This is the documented way to access validation errors in Cerberus and avoids dependency on message text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Backend/routes/appointments.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Backend/routes/appointments.py (1)
Backend/db/db.py (2)
open_db(8-13)close_db(15-18)
🔇 Additional comments (5)
Backend/routes/appointments.py (5)
17-22: LGTM!Clean helper function providing consistent structured error responses across all routes.
50-83: LGTM!Well-structured schema with appropriate constraints for required and optional fields.
86-94: LGTM!Safe JSON parsing with appropriate error handling for malformed or missing request bodies.
167-192: LGTM!Clean implementation with proper ID validation and 404 handling for non-existent appointments.
305-340: LGTM!Clean delete implementation with proper existence check before deletion.
| appointment_status = data.get("status", "Pending") | ||
| description = data.get("description", "") or "" | ||
| description = description.strip() |
There was a problem hiding this comment.
Default values may unintentionally overwrite existing data.
When status is omitted, it defaults to "Pending", which could reset a "Completed" appointment. Similarly, omitting description defaults to "", erasing existing content.
Since the schema marks these as optional, users may expect omitting them preserves existing values. Consider either:
- Require all fields on update (change schema or use a separate update schema)
- Preserve existing values when fields are omitted:
- appointment_status = data.get("status", "Pending")
- description = data.get("description", "") or ""
+ # Preserve existing values if not provided
+ appointment_status = data.get("status") or dict(existing_appointment)["appointment_status"]
+ description = data.get("description")
+ if description is None:
+ description = dict(existing_appointment)["content"](This requires moving these lines after the existence check.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Backend/routes/appointments.py around lines 259-261, the current defaulting
of appointment_status and description will overwrite existing values when fields
are omitted; instead, move this logic after you verify the appointment exists
and then preserve existing values by using the existing appointment fields as
defaults (e.g., status = data.get("status", existing.status) and description =
data.get("description", existing.description)), and only call strip() when
description is not None; alternatively implement/validate a separate update
schema that requires fields or explicitly distinguishes omitted vs null values.
|
Include testing evidences |
1 similar comment
|
Include testing evidences |
2 participants
🔒 Improved Input Validation & Error Handling for Appointment API
Closes # 58
📝 Description
This PR enhances the Appointment API by introducing robust input validation and standardized error handling. It ensures that all incoming data is properly validated before interacting with the database, improving both reliability and security
🔧 Changes Made
✨ Added Cerberus validation for appointment fields
🚫 Unknown/extra fields are rejected during validation
📌 Improved structured and descriptive error responses:
🛡 Wrapped all DB operations in try/except for safer failure handling
🧰 Updated logs for easier debugging and API traceability
🧹 General cleanup and code quality improvements
✅ Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.