Feature/centralized error handling#182
Conversation
📝 WalkthroughWalkthroughCentralizes environment-aware error handling with new exception classes and a decorator; updates many backend routes to raise these errors and use PATCH for partial updates; adds validation utilities and agent endpoints in app bootstrap; updates frontend calls to PATCH; adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as "Route Handler"
participant Decor as "handle_db_errors\n(Decorator)"
participant ErrH as "Error Handler\n(create_error_response)"
participant DB as "Database / Cache"
participant Response
Client->>Route: HTTP Request (GET/POST/PATCH/DELETE)
activate Route
Route->>DB: validate / query / mutate
alt Success
DB-->>Route: data / ack
Route-->>Client: 200/201 Success Response
else Exception (MissingFieldError / NotFoundError / sqlite)
Route-->>Decor: exception propagates
Decor->>ErrH: select handler based on exception
ErrH->>ErrH: check current_app.config['ENV']
alt ENV == development
ErrH-->>Response: detailed JSON (dev_message + details)
else ENV == production
ErrH-->>Response: generic JSON (prod_message)
end
Response-->>Client: 4xx/5xx Error Response
end
deactivate Route
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Backend/routes/symptoms.py (1)
15-33: Inaccurate missing field reporting.The condition
if not (week and symptom)correctly checks if either field is missing, butMissingFieldError(['week_number', 'symptom'])always reports both fields as missing regardless of which one is actually absent. This provides misleading feedback to API consumers.Proposed fix
- if not (week and symptom): - raise MissingFieldError(['week_number', 'symptom']) + required = ['week_number', 'symptom'] + missing = [field for field in required if not data.get(field)] + if missing: + raise MissingFieldError(missing)Backend/app.py (1)
227-233: Index route has unused import and incomplete implementation.The
index()function imports and callsget_tasks()but doesn't use the result. It only returnsappointment_db. This appears to be incomplete or debug code.🐛 Suggested cleanup
`@app.route`('/') def index(): from routes.appointments import get_appointments - from routes.tasks import get_tasks appointment_db = get_appointments() - task_db = get_tasks() return appointment_db
🤖 Fix all issues with AI agents
In `@Backend/app.py`:
- Around line 236-237: The app currently calls app.run(host='0.0.0.0',
port=port, debug=True) which forces debug mode in all environments; change this
to compute debug from app.config['ENV'] (e.g., debug = app.config['ENV'] ==
'development') and pass that debug flag to app.run, and ensure port (from
os.getenv("DEV_PORT", 5000) / os.getenv("PROD_PORT", 8000)) is converted to an
integer before use; update the invocation of app.run(...) accordingly so debug
is False in production.
- Around line 21-23: The module currently calls argparse.parse_args() at import
time (the parser variable and args = parser.parse_args()), which breaks WSGI
deployments; move the argparse.ArgumentParser creation and args =
parser.parse_args() call into the if __name__ == '__main__': block so parsing
happens only when the script is executed directly, and update any code that uses
args (e.g., server start logic) to read from the moved args inside that main
block.
In `@Backend/error_handling/error_classes.py`:
- Around line 1-7: The message attribute in MissingFieldError.__init__ uses an
unnecessary f-string prefix; update the assignment in class MissingFieldError
(method __init__) to use a plain string ("Missing required fields") or
interpolate self.field_names into the f-string (e.g., f"Missing required fields:
{self.field_names}") to remove the unused f-prefix and satisfy linting.
In `@Backend/routes/blood_pressure.py`:
- Around line 52-65: The two GET routes share the same variable segment and
collide; update the route decorators to use distinct URL prefixes so Flask can
dispatch correctly: change the decorator for get_bp_logs_by_week to use
'/blood_pressure/week/<int:week>' and change the decorator for the single-entry
handler (the function that fetches by id, e.g., get_bp_log or similar) to use
'/blood_pressure/entry/<int:id>'; keep the handler names (get_bp_logs_by_week
and the id-fetching function) and their logic unchanged, only update their
`@bp_bp.route`(...) patterns and any code that constructs links to these
endpoints.
In `@Backend/routes/profile.py`:
- Around line 88-124: The handler update_profile currently assumes a profile row
exists and uses profile['lmp'] etc., causing a crash when profile is None; add a
null check right after profile = db.execute(...).fetchone() and if profile is
None return a 404 or appropriate error JSON (e.g., {"error":"profile not
found"}) before accessing profile fields, or alternatively initialize fallback
defaults from request when profile is missing; ensure subsequent references to
profile (profile['lmp'], profile['cycleLength'], profile['periodLength'],
profile['age'], profile['weight'], profile['user_location']) are only used after
this guard so calculate_due_date(lmp, cycleLength) and the UPDATE statement
operate with validated values.
In `@Backend/routes/tasks.py`:
- Around line 57-64: The UPDATE query in routes/tasks.py is incorrectly
defaulting isOptional and isAppointmentMade to False on partial updates; change
the db.execute parameter calls to use data.get('isOptional', task['isOptional'])
and data.get('isAppointmentMade', task['isAppointmentMade']) so these flags
preserve existing values when not provided in the incoming data (same pattern
used for title/content/etc.), keeping the rest of the db.execute call and
db.commit intact.
- Around line 95-105: The two DB operations in move_to_appointment (the 'UPDATE
tasks SET isAppointmentMade = ? WHERE id = ?' and the 'INSERT INTO appointments
(...) VALUES (...)') are committed separately, breaking atomicity; change the
logic to perform both statements within a single transaction and call
db.commit() once after both execute calls (or begin a transaction and rollback
on exception), ensuring you handle exceptions to rollback if the INSERT fails so
the task update is not persisted alone.
In `@Backend/routes/weight.py`:
- Around line 89-92: The update_weight endpoint incorrectly treats
validate_week_number and validate_weight_value return values as booleans; those
validators return dicts with a "status" key like log_weight does, so update
update_weight to inspect the validators' ["status"] fields (e.g., if not
validate_week_number(week_number)["status"] or if not
validate_weight_value(weight)["status"]) and return the corresponding error
payload/message from the validator when status is False, mirroring the pattern
used in log_weight.
In `@Backend/utils.py`:
- Around line 17-34: validate_medicine_data currently skips validation for empty
strings because it checks truthiness of name/dose; change the checks to use "is
not None" and validate the local variables instead of indexing data (use name
and dose), e.g., for name: if name is not None and (not isinstance(name, str) or
len(name.strip()) == 0): errors["name"] = "Medicine name must be a non-empty
string." and similarly for dose using dose variable; keep the existing
week_number handling with validate_week_number and populate
errors["week_number"] from week_result as before.
- Around line 2-12: The validate_bp_data function currently indexes
data['week_number'], data['systolic'], and data['diastolic'] directly which
raises KeyError for PATCH/partial payloads; change the logic to first check
presence of each key (e.g., if 'week_number' in data / if 'systolic' in data /
if 'diastolic' in data) and only perform the type and range validations when the
key exists (or use data.get(...) and check for None), adding an appropriate
error message to errors when validation fails; keep the validation rules
(positive int for week_number, 50< systolic <300, 30< diastolic <200) and return
the errors dict as before.
In `@Frontend/src/services/ActionExecutor.js`:
- Around line 245-249: The rollbackUpdateAppointment function is calling the
wrong endpoint/method (PUT on /appointments/{id}) which the backend doesn’t
support; update rollbackUpdateAppointment to send a PATCH to
/update_appointment/{id} (same shape as update call) and use the same JSON
body/headers pattern as the existing updateAppointment logic so the undo uses
the correct endpoint and avoids 405 errors.
🧹 Nitpick comments (6)
Backend/routes/profile.py (1)
21-56: Inconsistent error handling pattern.The validation mixes
MissingFieldError(lines 33-34) with inlinejsonifyerror responses (lines 36-44). For consistency with the centralized error handling approach, consider creating a custom validation error class or using a validation utility like other routes do withvalidate_bp_dataandvalidate_medicine_data.Backend/routes/tasks.py (2)
1-5: Unused imports can be cleaned up.
sqlite3(line 1) andclose_db(line 3) are imported but not used in this file. The@handle_db_errorsdecorator handles sqlite3 exceptions internally, andclose_dbis managed by the app teardown context.♻️ Suggested cleanup
-import sqlite3 -from flask import Blueprint, jsonify, request -from db.db import open_db,close_db +from flask import Blueprint, jsonify, request +from db.db import open_db from error_handling.error_classes import MissingFieldError, NotFoundError from error_handling.handlers import handle_db_errors
77-77: Consider using PATCH for consistency with other update routes.The PR changes update endpoints from PUT to PATCH across the codebase, but
move_to_appointmentstill uses PUT. While PUT may be semantically appropriate here (creating a new resource from an existing one), consider whether PATCH or POST would be more consistent with the PR's conventions.Backend/app.py (1)
68-74: Unused parametereinhandle_unsupported_media.The exception parameter is not used, unlike other error handlers that use it for environment-aware messaging. Consider using it for consistency or prefixing with underscore to indicate intentional non-use.
♻️ Suggested fix
`@app.errorhandler`(UnsupportedMediaType) # This handles cases where Content-Type is not application/json # This error is raised by the get_json() method of the request object -def handle_unsupported_media(e): +def handle_unsupported_media(_e): return jsonify({ "error": "Content-Type must be application/json" }), 415Backend/routes/discharge.py (1)
34-36: Consider extracting duplicatedb_pathcomputation.The
db_pathconstruction and agent retrieval logic is duplicated acrossadd_discharge_log,update_discharge_log, anddelete_discharge_log. This could be extracted to a helper or module-level constant.♻️ Suggested refactor
Add at module level:
DB_PATH = os.path.join(os.path.dirname(os.path.dirname(__file__)), "db", "database.db")Then replace each occurrence:
- db_path = os.path.join(os.path.dirname(os.path.dirname(__file__)), "db", "database.db") - agent = get_agent(db_path) + agent = get_agent(DB_PATH)Also applies to: 90-92, 109-111
Backend/routes/weight.py (1)
43-45: Consider extracting duplicatedb_pathand agent retrieval logic.Similar to
discharge.py, thedb_pathcomputation andget_agent()call are duplicated. This could be consolidated.Also applies to: 105-107, 123-125
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Backend/routes/profile.py`:
- Around line 25-33: The route handlers set_profile and update_profile call
request.get_json() then immediately use data.get(), which will raise
AttributeError when get_json() returns None; add a guard like the /agent route:
after calling request.get_json() check if data is falsy (None) and return a 400
Bad Request with a clear JSON error before any data.get() calls, and apply the
same guard in both set_profile and update_profile so missing/empty JSON payloads
produce 400 validation errors instead of triggering the generic 500 handler.
🧹 Nitpick comments (4)
Backend/routes/profile.py (1)
36-39: Keep validation errors within centralized error formatting.These inline
return jsonify(...), 400branches bypass the environment-aware error formatting you just centralized. Prefer raising a custom validation exception or using the shared error helper so error payloads stay consistent across environments.Also applies to: 41-44, 104-107, 109-112
Backend/utils.py (1)
2-14: Validation logic improved, but falsy values still skip checks.Using
data.get()prevents KeyError, which is good. However, the patternif data.get('systolic') and ...means a value of0would skip validation entirely. While0is outside the valid ranges anyway, consider using explicitis not Nonechecks for consistency:systolic = data.get('systolic') if systolic is not None and (not isinstance(systolic, int) or not (50 < systolic < 300)):This makes intent clearer and aligns with how
validate_medicine_datahandlesweek_number.Backend/routes/tasks.py (1)
27-43: Return 201 for successful resource creation.The
add_taskendpoint returns200on success, butlog_weightreturns201for creation. For consistency and REST semantics, POST operations that create resources should return201 Created.♻️ Proposed fix
- return jsonify({"status": "success", "message": "Task added"}), 200 + return jsonify({"status": "success", "message": "Task added"}), 201Backend/app.py (1)
68-74: Remove unused parameter or use it in the response.The
eparameter inhandle_unsupported_media(e)is unused. Either remove it or include relevant details from the exception in development mode for consistency with other error handlers.♻️ Proposed fix (option 1: use the parameter)
`@app.errorhandler`(UnsupportedMediaType) -# This handles cases where Content-Type is not application/json -# This error is raised by the get_json() method of the request object -def handle_unsupported_media(e): - return jsonify({ - "error": "Content-Type must be application/json" - }), 415 +def handle_unsupported_media(e): + """Handle cases where Content-Type is not application/json.""" + mode = app.config.get('ENV', 'development') + if mode == 'development': + return jsonify({ + "error": "Content-Type must be application/json", + "details": e.description + }), 415 + return jsonify({ + "error": "Content-Type must be application/json" + }), 415
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Backend/routes/profile.py`:
- Around line 118-121: The UPDATE statement in profile.py (the db.execute call
that sets dueDate, user_location, lmp, cycleLength, periodLength, age, weight)
is missing a WHERE clause and will update every row; modify that query to
include a WHERE condition (e.g., "WHERE id = ?" or "WHERE user_id = ?") and pass
the corresponding id/user_id into the parameter tuple so only the intended
profile row is updated (ensure the identifier used matches the table PK/column
and any surrounding function handling e.g., the route handler that receives the
profile id).
🧹 Nitpick comments (3)
Backend/routes/profile.py (3)
1-1: Unused import.
sqlite3is imported but not used in this file. The database errors are handled by the@handle_db_errorsdecorator which importsOperationalErrorandDatabaseErrordirectly.Suggested fix
-import sqlite3 from flask import Blueprint, jsonify, request
26-46: Inconsistent error handling approach.The PR objective is to centralize error handling, but this function mixes inline
jsonifyerror responses (lines 27, 39, 41, 46) with the centralizedMissingFieldErrorexception (line 36). Consider using custom exception classes consistently for all validation errors to align with the PR's goal.For example, you could introduce a
ValidationErrorclass for type/format validation failures, or extendMissingFieldErrorusage to cover these cases.
55-57: Duplicated db_path construction.The database path construction and agent retrieval pattern is repeated in three places. Consider extracting this to a helper function or module-level constant.
Suggested refactor
Add at module level:
DB_PATH = os.path.join(os.path.dirname(os.path.dirname(__file__)), "db", "database.db") def _update_profile_cache(operation: str): agent = get_agent(DB_PATH) agent.update_cache(data_type="profile", operation=operation)Then in each handler:
- db_path = os.path.join(os.path.dirname(os.path.dirname(__file__)), "db", "database.db") - agent = get_agent(db_path) - agent.update_cache(data_type="profile", operation="create") + _update_profile_cache("create")Also applies to: 82-84, 125-127
|
@aneesafatima |
2 participants
Closes #171
📝 Description
This PR implements a comprehensive error handling system for the Flask application, introducing environment-based error responses, custom error classes, and unified error handling across all routes. The implementation includes significant bug fixes discovered during testing and establishes consistent patterns for error management throughout the codebase.
Key Improvements:
MissingFieldErrorandNotFoundErrorclasses🔧 Changes Made
1. Environment Configuration & CLI Support
python app.py --env developmentorpython app.py --env productionapp.config['ENV']for application-wide accessDEV_PORTenv var (defaults to 5000)PROD_PORTenv var (defaults to 8000)2. Custom Error Classes
Created custom exception classes in the error handling module:
3. Global Error Handlers
Registered five comprehensive error handlers on the Flask app instance:
MissingFieldErrorhandler: Returns missing field details in dev, generic message in prodNotFoundErrorhandler: Returns resource-specific 404 responsesBadRequesthandler: Catches malformed JSON and invalid requests (triggered byrequest.get_json())UnsupportedMediaTypehandler: Returns 415 when Content-Type is not application/jsonExceptionhandler: Catches all unhandled exceptions with environment-aware responses4. Database Error Handling
@handle_db_errorsdecorator to wrap all routes performing database operationsOperationalErrorandDatabaseErrorfrom SQLite3close_db()calls: Application already uses@app.teardown_appcontextfor cleanup5. Request Handling Improvements
request.jsonwithrequest.get_json()throughout the applicationrequest.get_json()automatically raisesBadRequestfor malformed JSONrequest.jsonsilently accepts invalid data, leading to runtime errors6. Input Validation Utilities
Created
utils.pywith validation functions:validate_bp_data(data): Validates blood pressure entriesweek_number: Must be positive integersystolic: Must be integer between 50-300diastolic: Must be integer between 30-200validate_medicine_data(data): Validates medicine entriesweek_number: Validates usingvalidate_week_number()name: Must be non-empty stringdose: Must be non-empty stringvalidate_week_number(week): Ensures week is integer between 1-52validate_weight_value(weight): Ensures weight is positive number up to 1000kg7. HTTP Method Corrections
8. Route Conflict Resolution
Fixed duplicate route patterns that caused Flask routing conflicts:
Weight Routes:
/weight/<int:id>used for both ideal weight and week-based weight/weight/<int:id>for ideal weight operations/weight/week/<int:id>for week-based weight operationsMedicine Routes:
/medicine/<int:id>caused similar conflicts9. Critical Bug Fixes
Bug #1: Missing Validation in Create Operations
Bug #2: Incomplete SQL Parameters
Bug #3: Incorrect Default Values in Updates
10. Error Response Utilities
Created helper function for consistent error formatting:
11. Status Code Improvements
12. Documentation Updates
setup.mdwith environment-specific run commands:python app.py --env developmentpython app.py --env production📷 Screenshots or Visual Changes (if applicable)
[Used Incorrect Query format for testing]:
[Output in development while using a wrong query]:
[Deleting Appointment with incorrect ID in dev mode]:
[Provided Incorrect JSON format]:
[Gave Incorrect blood pressure details in dev mode]:
[Gave Incorrect blood pressure details in prod mode]:
🧪 Testing Performed
Extensive testing was conducted across all modified routes:
get_json()🏗️ Architecture Improvements
🤝 Collaboration
N/A
✅ Checklist
📋 Additional Notes for Reviewers
This PR represents a significant refactoring of the application's error handling system. While extensive, every change addresses either:
The changes are compatible with existing functionality while providing a robust foundation for future development. All modifications have been tested to ensure existing features continue to work as expected.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.