fix(backend): skip malformed calendar meetings instead of 500ing the whole list#8865
Conversation
…whole list GET /v1/calendar/meetings built [CalendarMeetingContext(**m) for m in meetings], and the model has required no-default fields (calendar_event_id, title, start_time, duration_minutes). A single malformed stored meeting raised a ValidationError that propagated out of the comprehension and 500'd the whole request, so one bad record hid every other meeting the user has. Add CalendarMeetingContext.from_records, which parses each record and skips the ones that fail validation (reporting them via on_error), and use it in the list endpoint so a bad record is logged and skipped instead of failing the request. Test: tests/unit/test_calendar_meeting_from_records.py (valid parsed, malformed skipped without losing valid ones, missing-callback tolerated, empty).
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 3/5
- In
backend/routers/calendar_meetings.py, theon_errorhandler logs rawValidationErrortext, which can includeinput_valuefrom malformed meeting payloads; merging as-is risks leaking sensitive user content into application logs. Switch to a sanitized/redacted error log (or structured fields that exclude raw input) before merging to de-risk privacy exposure.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Good fix — thanks @ZachL111. The poison-record problem is real and the One concrete improvement before merge: the on_error=lambda record, exc: logger.warning(
'Skipping malformed calendar meeting for uid=%s event_id=%s: %s',
uid, record.get('calendar_event_id'), type(exc).__name__,
),Flagging for maintainer review on the data-handling point above before merge; otherwise the implementation looks good to me. |
…r meeting A pydantic ValidationError's str() renders the failing field's input_value, which for a calendar meeting can be sensitive user content (title, participant emails, meeting link, notes). Log calendar_event_id plus the exception class name instead, so a bad record is still locatable without writing user content into application logs (addresses review feedback).
|
Thanks David, good catch on the logging. Applied your exact suggestion in 0cc1903: the on_error now logs uid, calendar_event_id, and the exception class name (type(exc).name), so a bad record is still locatable without the ValidationError str() writing the meeting title, participant emails, link, or notes into the logs. |
kodjima33
left a comment
There was a problem hiding this comment.
Backend bug fix w/ tests: skip malformed calendar meeting instead of 500ing the whole list. Established poison-record pattern.
Summary
Fixes a poison-record bug: one malformed stored calendar meeting made
GET /v1/calendar/meetingsreturn 500, hiding every other meeting the user has.Root cause
The endpoint built the response with an unguarded list comprehension:
CalendarMeetingContexthas required no-default fields (calendar_event_id,title,start_time,duration_minutes). If any stored meeting record is missing one of those or has a bad type,CalendarMeetingContext(**m)raises aValidationError, which propagates out of the comprehension and 500s the whole request. So a single corrupt record takes down the entire list, not just itself.Fix
Add a
CalendarMeetingContext.from_recordsfactory that parses each record and skips the ones that fail validation, reporting each skipped record through anon_errorcallback. The list endpoint uses it, logging and skipping a bad record instead of failing the request.Testing
tests/unit/test_calendar_meeting_from_records.py: valid records are parsed, malformed records are skipped without losing the valid ones (and are reported viaon_error), a missing callback is tolerated, and empty input returns an empty list. The factory is pure, so the test needs no I/O.PR status
Mergeable, no conflicts. Additive resilience: valid records return exactly as before; only malformed records change from 500-the-whole-list to skip-and-log.