Revert "fix(events): tolerate corrupt end_dt + reject end<=start at write time"
This reverts commit 94b169f31c.
This commit is contained in:
@@ -54,7 +54,6 @@ async def create_event():
|
|||||||
end_dt = _parse_dt(data["end_dt"]) if data.get("end_dt") else None
|
end_dt = _parse_dt(data["end_dt"]) if data.get("end_dt") else None
|
||||||
except ValueError:
|
except ValueError:
|
||||||
return jsonify({"error": "Invalid datetime format"}), 400
|
return jsonify({"error": "Invalid datetime format"}), 400
|
||||||
try:
|
|
||||||
event = await events_svc.create_event(
|
event = await events_svc.create_event(
|
||||||
user_id=_get_current_user_id(),
|
user_id=_get_current_user_id(),
|
||||||
title=data["title"],
|
title=data["title"],
|
||||||
@@ -68,8 +67,6 @@ async def create_event():
|
|||||||
project_id=data.get("project_id"),
|
project_id=data.get("project_id"),
|
||||||
reminder_minutes=data.get("reminder_minutes"),
|
reminder_minutes=data.get("reminder_minutes"),
|
||||||
)
|
)
|
||||||
except ValueError as exc:
|
|
||||||
return jsonify({"error": str(exc)}), 400
|
|
||||||
return jsonify(event.to_dict()), 201
|
return jsonify(event.to_dict()), 201
|
||||||
|
|
||||||
|
|
||||||
@@ -109,14 +106,11 @@ async def update_event(event_id: int):
|
|||||||
fields[dt_field] = _parse_dt(data[dt_field])
|
fields[dt_field] = _parse_dt(data[dt_field])
|
||||||
except ValueError:
|
except ValueError:
|
||||||
return jsonify({"error": f"Invalid datetime for {dt_field}"}), 400
|
return jsonify({"error": f"Invalid datetime for {dt_field}"}), 400
|
||||||
try:
|
|
||||||
event = await events_svc.update_event(
|
event = await events_svc.update_event(
|
||||||
user_id=_get_current_user_id(),
|
user_id=_get_current_user_id(),
|
||||||
event_id=event_id,
|
event_id=event_id,
|
||||||
**fields,
|
**fields,
|
||||||
)
|
)
|
||||||
except ValueError as exc:
|
|
||||||
return jsonify({"error": str(exc)}), 400
|
|
||||||
if event is None:
|
if event is None:
|
||||||
return jsonify({"error": "Event not found"}), 404
|
return jsonify({"error": "Event not found"}), 404
|
||||||
return jsonify(event.to_dict())
|
return jsonify(event.to_dict())
|
||||||
|
|||||||
@@ -32,17 +32,7 @@ async def create_event(
|
|||||||
attendees: list[str] | None = None,
|
attendees: list[str] | None = None,
|
||||||
calendar_name: str | None = None,
|
calendar_name: str | None = None,
|
||||||
) -> Event:
|
) -> Event:
|
||||||
"""Create an event in the DB, then fire a CalDAV push task.
|
"""Create an event in the DB, then fire a CalDAV push task."""
|
||||||
|
|
||||||
Raises ValueError if `end_dt` is set but not strictly after `start_dt`.
|
|
||||||
Catching this at write time prevents the "end before start" data state
|
|
||||||
that breaks downstream filters (Fable #160).
|
|
||||||
"""
|
|
||||||
if end_dt is not None and end_dt <= start_dt:
|
|
||||||
raise ValueError(
|
|
||||||
f"end_dt ({end_dt.isoformat()}) must be after start_dt "
|
|
||||||
f"({start_dt.isoformat()}); pass end_dt=None for point events."
|
|
||||||
)
|
|
||||||
uid = str(uuid.uuid4())
|
uid = str(uuid.uuid4())
|
||||||
async with async_session() as session:
|
async with async_session() as session:
|
||||||
event = Event(
|
event = Event(
|
||||||
@@ -97,16 +87,12 @@ async def list_events(
|
|||||||
async with async_session() as session:
|
async with async_session() as session:
|
||||||
# Match strategy:
|
# Match strategy:
|
||||||
# - Recurring events: fetch all, expand via rrule below.
|
# - Recurring events: fetch all, expand via rrule below.
|
||||||
# - Non-recurring with a VALID end_dt (end > start): standard
|
# - Non-recurring with an end_dt: standard overlap — starts before
|
||||||
# overlap — starts before date_to and ends after date_from.
|
# date_to and ends after date_from.
|
||||||
# - Non-recurring with no end_dt OR an invalid end_dt (end <=
|
# - Non-recurring with no end_dt: treat as a point event at
|
||||||
# start; almost always a tool-call mishap): treat as a point
|
# start_dt, include only if start_dt falls within the window.
|
||||||
# event at start_dt and include only if start_dt is in the
|
# (Previously this branch matched any event with a null end_dt,
|
||||||
# window. The fallback for invalid end_dt is critical — without
|
# returning all past events as "happening today".)
|
||||||
# it, an end_dt accidentally set in the past makes the event
|
|
||||||
# vanish from every list_events call regardless of where its
|
|
||||||
# start_dt actually is. Discovered 2026-04-29; see Fable #160
|
|
||||||
# for the structural fix that replaces end_dt with duration.
|
|
||||||
result = await session.execute(
|
result = await session.execute(
|
||||||
select(Event).where(
|
select(Event).where(
|
||||||
Event.user_id == user_id,
|
Event.user_id == user_id,
|
||||||
@@ -116,16 +102,9 @@ async def list_events(
|
|||||||
Event.recurrence.is_(None),
|
Event.recurrence.is_(None),
|
||||||
Event.start_dt <= date_to,
|
Event.start_dt <= date_to,
|
||||||
or_(
|
or_(
|
||||||
and_(
|
|
||||||
Event.end_dt.isnot(None),
|
|
||||||
Event.end_dt > Event.start_dt,
|
|
||||||
Event.end_dt >= date_from,
|
Event.end_dt >= date_from,
|
||||||
),
|
|
||||||
and_(
|
and_(
|
||||||
or_(
|
|
||||||
Event.end_dt.is_(None),
|
Event.end_dt.is_(None),
|
||||||
Event.end_dt <= Event.start_dt,
|
|
||||||
),
|
|
||||||
Event.start_dt >= date_from,
|
Event.start_dt >= date_from,
|
||||||
),
|
),
|
||||||
),
|
),
|
||||||
@@ -141,14 +120,8 @@ async def list_events(
|
|||||||
items.append(event.to_dict())
|
items.append(event.to_dict())
|
||||||
continue
|
continue
|
||||||
|
|
||||||
# Expand recurring event occurrences within [date_from, date_to].
|
# Expand recurring event occurrences within [date_from, date_to]
|
||||||
# Treat end_dt <= start_dt as no-end (invalid data); falling back
|
duration = (event.end_dt - event.start_dt) if event.end_dt else None
|
||||||
# to point events instead of computing a negative duration.
|
|
||||||
duration = (
|
|
||||||
(event.end_dt - event.start_dt)
|
|
||||||
if event.end_dt and event.end_dt > event.start_dt
|
|
||||||
else None
|
|
||||||
)
|
|
||||||
try:
|
try:
|
||||||
rule = rrulestr(event.recurrence, dtstart=event.start_dt, ignoretz=False)
|
rule = rrulestr(event.recurrence, dtstart=event.start_dt, ignoretz=False)
|
||||||
occurrences = rule.between(date_from, date_to, inc=True)
|
occurrences = rule.between(date_from, date_to, inc=True)
|
||||||
@@ -200,13 +173,7 @@ async def search_events(
|
|||||||
|
|
||||||
|
|
||||||
async def update_event(user_id: int, event_id: int, **fields) -> Event | None:
|
async def update_event(user_id: int, event_id: int, **fields) -> Event | None:
|
||||||
"""Partial update. Returns updated event or None if not found.
|
"""Partial update. Returns updated event or None if not found."""
|
||||||
|
|
||||||
Raises ValueError if the resulting state would have `end_dt <= start_dt`.
|
|
||||||
Validation runs against the *post-update* values so updates that fix
|
|
||||||
one side of the constraint without explicitly clearing the other still
|
|
||||||
fail loudly instead of persisting invalid data.
|
|
||||||
"""
|
|
||||||
async with async_session() as session:
|
async with async_session() as session:
|
||||||
result = await session.execute(
|
result = await session.execute(
|
||||||
select(Event).where(Event.id == event_id, Event.user_id == user_id)
|
select(Event).where(Event.id == event_id, Event.user_id == user_id)
|
||||||
@@ -219,17 +186,6 @@ async def update_event(user_id: int, event_id: int, **fields) -> Event | None:
|
|||||||
"location", "color", "recurrence", "project_id", "reminder_minutes"}
|
"location", "color", "recurrence", "project_id", "reminder_minutes"}
|
||||||
# Nullable fields that callers can explicitly set to None to clear
|
# Nullable fields that callers can explicitly set to None to clear
|
||||||
nullable = {"end_dt", "recurrence", "project_id", "reminder_minutes"}
|
nullable = {"end_dt", "recurrence", "project_id", "reminder_minutes"}
|
||||||
|
|
||||||
# Pre-validate the *resulting* start/end against the post-update
|
|
||||||
# state, not against either field's pre-update value alone.
|
|
||||||
new_start = fields["start_dt"] if "start_dt" in fields and fields["start_dt"] is not None else event.start_dt
|
|
||||||
new_end = fields["end_dt"] if "end_dt" in fields else event.end_dt
|
|
||||||
if new_end is not None and new_end <= new_start:
|
|
||||||
raise ValueError(
|
|
||||||
f"end_dt ({new_end.isoformat()}) must be after start_dt "
|
|
||||||
f"({new_start.isoformat()}); pass end_dt=None for point events."
|
|
||||||
)
|
|
||||||
|
|
||||||
for key, value in fields.items():
|
for key, value in fields.items():
|
||||||
if key in allowed and (value is not None or key in nullable):
|
if key in allowed and (value is not None or key in nullable):
|
||||||
setattr(event, key, value)
|
setattr(event, key, value)
|
||||||
|
|||||||
@@ -213,7 +213,6 @@ async def create_event_tool(*, user_id, arguments, **_ctx):
|
|||||||
proj = await resolve_project(user_id, project_name)
|
proj = await resolve_project(user_id, project_name)
|
||||||
if proj:
|
if proj:
|
||||||
project_id = proj.id
|
project_id = proj.id
|
||||||
try:
|
|
||||||
event = await events_create_event(
|
event = await events_create_event(
|
||||||
user_id=user_id,
|
user_id=user_id,
|
||||||
title=arguments.get("title", "Untitled Event"),
|
title=arguments.get("title", "Untitled Event"),
|
||||||
@@ -230,8 +229,6 @@ async def create_event_tool(*, user_id, arguments, **_ctx):
|
|||||||
attendees=arguments.get("attendees"),
|
attendees=arguments.get("attendees"),
|
||||||
calendar_name=arguments.get("calendar_name"),
|
calendar_name=arguments.get("calendar_name"),
|
||||||
)
|
)
|
||||||
except ValueError as exc:
|
|
||||||
return {"success": False, "error": str(exc)}
|
|
||||||
return {"success": True, "type": "event", "data": event.to_dict()}
|
return {"success": True, "type": "event", "data": event.to_dict()}
|
||||||
|
|
||||||
|
|
||||||
@@ -363,10 +360,7 @@ async def update_event_tool(*, user_id, arguments, **_ctx):
|
|||||||
return {"success": False, "error": f"Invalid end: {exc}"}
|
return {"success": False, "error": f"Invalid end: {exc}"}
|
||||||
if end_dt is not None:
|
if end_dt is not None:
|
||||||
fields["end_dt"] = end_dt
|
fields["end_dt"] = end_dt
|
||||||
try:
|
|
||||||
updated = await events_update_event(user_id=user_id, event_id=event_to_update.id, **fields)
|
updated = await events_update_event(user_id=user_id, event_id=event_to_update.id, **fields)
|
||||||
except ValueError as exc:
|
|
||||||
return {"success": False, "error": str(exc)}
|
|
||||||
if updated is None:
|
if updated is None:
|
||||||
return {"success": False, "error": "Event not found or update failed."}
|
return {"success": False, "error": "Event not found or update failed."}
|
||||||
return {"success": True, "type": "event_updated", "data": updated.to_dict()}
|
return {"success": True, "type": "event_updated", "data": updated.to_dict()}
|
||||||
|
|||||||
@@ -124,92 +124,6 @@ async def test_update_event_fires_caldav_push():
|
|||||||
assert mock_task.called
|
assert mock_task.called
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
|
||||||
async def test_create_event_rejects_end_before_start():
|
|
||||||
"""Write-side validation: end_dt <= start_dt must raise rather than
|
|
||||||
persisting invalid data. Discovered 2026-04-29: a tool-call mishap
|
|
||||||
left an event with end_dt 32 days BEFORE start_dt; the bad data then
|
|
||||||
made the event invisible to the upcoming-list filter. Catching at
|
|
||||||
the boundary prevents the same shape from recurring."""
|
|
||||||
from fabledassistant.services.events import create_event
|
|
||||||
start = datetime(2026, 5, 1, 12, 0, tzinfo=timezone.utc)
|
|
||||||
end_before = datetime(2026, 3, 30, 12, 0, tzinfo=timezone.utc)
|
|
||||||
with pytest.raises(ValueError, match="must be after start_dt"):
|
|
||||||
await create_event(
|
|
||||||
user_id=1, title="Bad",
|
|
||||||
start_dt=start, end_dt=end_before,
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
|
||||||
async def test_create_event_rejects_end_equal_to_start():
|
|
||||||
"""Equal start/end is also invalid (zero-duration); pass end=None
|
|
||||||
for point events instead."""
|
|
||||||
from fabledassistant.services.events import create_event
|
|
||||||
same = datetime(2026, 5, 1, 12, 0, tzinfo=timezone.utc)
|
|
||||||
with pytest.raises(ValueError, match="must be after start_dt"):
|
|
||||||
await create_event(
|
|
||||||
user_id=1, title="Zero",
|
|
||||||
start_dt=same, end_dt=same,
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
|
||||||
async def test_update_event_rejects_post_state_with_end_before_start():
|
|
||||||
"""update_event must validate against the *post-update* state, not
|
|
||||||
just the field passed in. A user changing only start_dt to a value
|
|
||||||
after the existing end_dt would otherwise sneak past validation."""
|
|
||||||
mock_event = _make_mock_event() # start = 2026-03-25 10:00, end = 11:00
|
|
||||||
mock_session = _make_mock_session()
|
|
||||||
mock_result = MagicMock()
|
|
||||||
mock_result.scalar_one_or_none.return_value = mock_event
|
|
||||||
mock_session.execute = AsyncMock(return_value=mock_result)
|
|
||||||
|
|
||||||
with patch("fabledassistant.services.events.async_session") as mock_cls:
|
|
||||||
mock_cls.return_value = mock_session
|
|
||||||
from fabledassistant.services.events import update_event
|
|
||||||
# Push start_dt past the existing end_dt (without touching end_dt)
|
|
||||||
with pytest.raises(ValueError, match="must be after start_dt"):
|
|
||||||
await update_event(
|
|
||||||
user_id=1, event_id=1,
|
|
||||||
start_dt=datetime(2026, 3, 25, 12, 0, tzinfo=timezone.utc),
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
|
||||||
async def test_list_events_returns_event_with_invalid_end_dt():
|
|
||||||
"""Filter robustness: an event whose end_dt landed before start_dt
|
|
||||||
(corrupt data state from earlier bugs) must still surface in the
|
|
||||||
upcoming list when its start_dt is in range. Without this, the
|
|
||||||
event becomes invisible everywhere except via direct id lookup."""
|
|
||||||
mock_event = _make_mock_event()
|
|
||||||
# Corrupt state: end is 32 days BEFORE start
|
|
||||||
mock_event.start_dt = datetime(2026, 5, 1, 12, 0, tzinfo=timezone.utc)
|
|
||||||
mock_event.end_dt = datetime(2026, 3, 30, 12, 0, tzinfo=timezone.utc)
|
|
||||||
mock_event.to_dict.return_value = {
|
|
||||||
"id": 1, "title": "Corrupt event",
|
|
||||||
"start_dt": mock_event.start_dt.isoformat(),
|
|
||||||
"end_dt": mock_event.end_dt.isoformat(),
|
|
||||||
}
|
|
||||||
mock_session = _make_mock_session()
|
|
||||||
mock_result = MagicMock()
|
|
||||||
mock_result.scalars.return_value.all.return_value = [mock_event]
|
|
||||||
mock_session.execute = AsyncMock(return_value=mock_result)
|
|
||||||
|
|
||||||
with patch("fabledassistant.services.events.async_session") as mock_cls:
|
|
||||||
mock_cls.return_value = mock_session
|
|
||||||
from fabledassistant.services.events import list_events
|
|
||||||
# Window covers start_dt (May 1) but not end_dt (March 30).
|
|
||||||
results = await list_events(
|
|
||||||
user_id=1,
|
|
||||||
date_from=datetime(2026, 4, 29, tzinfo=timezone.utc),
|
|
||||||
date_to=datetime(2026, 5, 27, tzinfo=timezone.utc),
|
|
||||||
)
|
|
||||||
# Pre-fix: filter would exclude this event. Post-fix: present.
|
|
||||||
assert len(results) == 1
|
|
||||||
assert results[0]["id"] == 1
|
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_tools_calendar_always_available():
|
async def test_tools_calendar_always_available():
|
||||||
"""Calendar tools must appear in get_tools_for_user even without CalDAV."""
|
"""Calendar tools must appear in get_tools_for_user even without CalDAV."""
|
||||||
|
|||||||
Reference in New Issue
Block a user