From 9c48d8f60ee89a255b82f19c78017e4511f35f02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=94=90=E5=83=A7=E6=B4=97=E5=8F=91=E7=94=A8=E9=A3=98?= =?UTF-8?q?=E6=9F=94?= <2906266135@qq.com> Date: Mon, 10 Aug 2026 19:13:01 +0800 Subject: [PATCH 1/3] feat(schedule): add cloud schedule persistence --- backend/alembic/env.py | 2 +- .../20260810_0003_schedule_storage_v3.py | 382 ++++++++++++++++++ backend/src/timeflow/data/__init__.py | 4 +- backend/src/timeflow/data/models.py | 182 +++++++-- .../timeflow/data/repositories/__init__.py | 5 + .../timeflow/data/repositories/schedule.py | 252 ++++++++++++ backend/tests/test_schedule_repository.py | 124 ++++++ backend/tests/test_schedule_schema.py | 69 +++- 8 files changed, 959 insertions(+), 61 deletions(-) create mode 100644 backend/alembic/versions/20260810_0003_schedule_storage_v3.py create mode 100644 backend/src/timeflow/data/repositories/__init__.py create mode 100644 backend/src/timeflow/data/repositories/schedule.py create mode 100644 backend/tests/test_schedule_repository.py diff --git a/backend/alembic/env.py b/backend/alembic/env.py index d1cb440..b53a74d 100644 --- a/backend/alembic/env.py +++ b/backend/alembic/env.py @@ -6,7 +6,7 @@ from alembic import context from timeflow.data.database import Base -from timeflow.data.models import Schedule # noqa: F401 +from timeflow.data.models import Schedule, ScheduleOccurrenceOverride # noqa: F401 from timeflow.infrastructure.settings import get_settings config = context.config diff --git a/backend/alembic/versions/20260810_0003_schedule_storage_v3.py b/backend/alembic/versions/20260810_0003_schedule_storage_v3.py new file mode 100644 index 0000000..4759773 --- /dev/null +++ b/backend/alembic/versions/20260810_0003_schedule_storage_v3.py @@ -0,0 +1,382 @@ +"""Align schedule storage with the v3.10 architecture. + +Revision ID: 20260810_0003 +Revises: 20260729_0002 +Create Date: 2026-08-10 +""" + +from collections.abc import Sequence + +import sqlalchemy as sa + +from alembic import op + +revision: str = "20260810_0003" +down_revision: str | Sequence[str] | None = "20260729_0002" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def _create_v3_schedules_table() -> None: + op.create_table( + "schedules_v3", + sa.Column("id", sa.String(length=64), primary_key=True, nullable=False), + sa.Column("account_id", sa.String(length=64), nullable=False), + sa.Column("schedule_type", sa.String(length=16), nullable=False), + sa.Column( + "schedule_kind", + sa.String(length=16), + nullable=False, + server_default="once", + ), + sa.Column("title", sa.String(length=255), nullable=False), + sa.Column( + "is_all_day", + sa.Boolean(), + nullable=False, + server_default=sa.text("false"), + ), + sa.Column("start_time", sa.DateTime(timezone=True), nullable=True), + sa.Column("end_time", sa.DateTime(timezone=True), nullable=True), + sa.Column("timezone", sa.String(length=64), nullable=False), + sa.Column("recurrence_rule", sa.String(length=512), nullable=True), + sa.Column("location_name", sa.String(length=255), nullable=True), + sa.Column("latitude", sa.Numeric(precision=9, scale=6), nullable=True), + sa.Column("longitude", sa.Numeric(precision=9, scale=6), nullable=True), + sa.Column("reminder_type", sa.String(length=32), nullable=True), + sa.Column("reminder_trigger_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("reminder_offset_minutes", sa.Integer(), nullable=True), + sa.Column("reminder_strength", sa.String(length=16), nullable=True), + sa.Column("reminder_disposition_state", sa.String(length=16), nullable=True), + sa.Column("status", sa.String(length=16), nullable=False), + sa.Column("revision", sa.BigInteger(), nullable=False), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), + sa.Column("updated_at", sa.DateTime(timezone=True), nullable=False), + sa.Column("deleted_at", sa.DateTime(timezone=True), nullable=True), + sa.CheckConstraint( + "schedule_type IN ('time', 'location')", + name="ck_schedules_schedule_type", + ), + sa.CheckConstraint( + "schedule_kind IN ('once', 'recurring')", + name="ck_schedules_schedule_kind", + ), + sa.CheckConstraint( + "status IN ('active', 'deleted')", + name="ck_schedules_status", + ), + sa.CheckConstraint( + "reminder_type IS NULL OR reminder_type IN " + "('at_time', 'before_start', 'arrive_location', " + "'return_to_recorded_location')", + name="ck_schedules_reminder_type", + ), + sa.CheckConstraint( + "reminder_strength IS NULL OR reminder_strength IN ('low', 'medium', 'high')", + name="ck_schedules_reminder_strength", + ), + sa.CheckConstraint( + "reminder_disposition_state IS NULL OR reminder_disposition_state = 'confirmed'", + name="ck_schedules_reminder_disposition_state", + ), + sa.CheckConstraint("revision > 0", name="ck_schedules_revision_positive"), + sa.CheckConstraint( + "latitude IS NULL OR latitude BETWEEN -90 AND 90", + name="ck_schedules_latitude_range", + ), + sa.CheckConstraint( + "longitude IS NULL OR longitude BETWEEN -180 AND 180", + name="ck_schedules_longitude_range", + ), + sa.CheckConstraint( + "(schedule_type = 'time' AND start_time IS NOT NULL) " + "OR (schedule_type = 'location' AND start_time IS NULL " + "AND latitude IS NOT NULL AND longitude IS NOT NULL AND is_all_day = false)", + name="ck_schedules_schedule_type_requirements", + ), + sa.CheckConstraint( + "(schedule_kind = 'once' AND recurrence_rule IS NULL) " + "OR (schedule_kind = 'recurring' AND schedule_type = 'time' " + "AND recurrence_rule IS NOT NULL)", + name="ck_schedules_recurrence_requirements", + ), + sa.CheckConstraint( + "is_all_day = false " + "OR (schedule_type = 'time' AND start_time IS NOT NULL AND end_time IS NOT NULL)", + name="ck_schedules_all_day_requirements", + ), + sa.CheckConstraint( + "end_time IS NULL OR start_time IS NOT NULL", + name="ck_schedules_end_requires_start", + ), + sa.CheckConstraint( + "reminder_offset_minutes IS NULL OR reminder_offset_minutes >= 0", + name="ck_schedules_reminder_offset_nonnegative", + ), + sa.CheckConstraint( + "(reminder_type IS NULL AND reminder_trigger_at IS NULL " + "AND reminder_offset_minutes IS NULL AND reminder_strength IS NULL " + "AND reminder_disposition_state IS NULL) " + "OR (reminder_type IS NOT NULL AND reminder_strength IS NOT NULL)", + name="ck_schedules_reminder_presence", + ), + sa.CheckConstraint( + "reminder_type IS NULL " + "OR (reminder_type = 'at_time' AND reminder_trigger_at IS NOT NULL " + "AND reminder_offset_minutes IS NULL) " + "OR (reminder_type = 'before_start' AND reminder_trigger_at IS NULL " + "AND reminder_offset_minutes IS NOT NULL) " + "OR (reminder_type IN ('arrive_location', 'return_to_recorded_location') " + "AND reminder_trigger_at IS NULL AND reminder_offset_minutes IS NULL " + "AND latitude IS NOT NULL AND longitude IS NOT NULL)", + name="ck_schedules_reminder_configuration", + ), + sa.CheckConstraint( + "(status = 'active' AND deleted_at IS NULL) " + "OR (status = 'deleted' AND deleted_at IS NOT NULL)", + name="ck_schedules_deleted_at", + ), + ) + + +def _create_occurrence_overrides_table() -> None: + op.create_table( + "schedule_occurrence_overrides", + sa.Column("id", sa.String(length=64), primary_key=True, nullable=False), + sa.Column( + "schedule_id", + sa.String(length=64), + sa.ForeignKey("schedules.id", ondelete="RESTRICT"), + nullable=False, + ), + sa.Column("occurrence_start", sa.DateTime(timezone=True), nullable=False), + sa.Column("action", sa.String(length=16), nullable=False), + sa.Column( + "replacement_schedule_id", + sa.String(length=64), + sa.ForeignKey("schedules.id", ondelete="RESTRICT"), + nullable=True, + ), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), + sa.Column("updated_at", sa.DateTime(timezone=True), nullable=False), + sa.CheckConstraint( + "action IN ('cancel', 'replace')", + name="ck_schedule_occurrence_overrides_action", + ), + sa.CheckConstraint( + "(action = 'cancel' AND replacement_schedule_id IS NULL) " + "OR (action = 'replace' AND replacement_schedule_id IS NOT NULL)", + name="ck_schedule_occurrence_overrides_replacement", + ), + sa.UniqueConstraint( + "schedule_id", + "occurrence_start", + name="uq_schedule_occurrence_overrides_schedule_occurrence", + ), + ) + op.create_index( + "ix_schedule_occurrence_overrides_replacement_schedule_id", + "schedule_occurrence_overrides", + ["replacement_schedule_id"], + ) + + +def upgrade() -> None: + """Migrate the legacy schedule rows and create occurrence overrides.""" + op.drop_index("ix_schedules_system_alarm_ref_id", table_name="schedules") + op.drop_index("ix_schedules_system_schedule_ref_id", table_name="schedules") + op.drop_index("ix_schedules_user_status_start_time", table_name="schedules") + + _create_v3_schedules_table() + op.create_index( + "ix_schedules_account_status_start_time", + "schedules_v3", + ["account_id", "status", "start_time"], + ) + op.create_index( + "ix_schedules_account_revision", + "schedules_v3", + ["account_id", "revision"], + ) + + op.execute( + sa.text( + """ + INSERT INTO schedules_v3 ( + id, account_id, schedule_type, schedule_kind, title, is_all_day, + start_time, end_time, timezone, recurrence_rule, location_name, + latitude, longitude, reminder_type, reminder_trigger_at, + reminder_offset_minutes, reminder_strength, + reminder_disposition_state, status, revision, created_at, + updated_at, deleted_at + ) + SELECT + left(id, 64), + left(user_id, 64), + schedule_type, + 'once', + left(title, 255), + false, + NULLIF(start_time, '')::timestamptz, + NULLIF(end_time, '')::timestamptz, + left(COALESCE(NULLIF(timezone, ''), 'UTC'), 64), + NULL, + left(location_name, 255), + latitude::numeric(9, 6), + longitude::numeric(9, 6), + NULL, + NULL, + NULL, + NULL, + NULL, + CASE WHEN status = 'deleted' THEN 'deleted' ELSE 'active' END, + 1, + COALESCE(NULLIF(created_at, '')::timestamptz, now()), + COALESCE(NULLIF(updated_at, '')::timestamptz, now()), + CASE + WHEN status = 'deleted' + THEN COALESCE(NULLIF(updated_at, '')::timestamptz, now()) + ELSE NULL + END + FROM schedules + """ + ) + ) + + op.drop_table("schedules") + op.rename_table("schedules_v3", "schedules") + _create_occurrence_overrides_table() + + +def downgrade() -> None: + """Restore the legacy MVP schedule shape while retaining core row data.""" + op.create_table( + "schedules_v2", + sa.Column("id", sa.Text(), primary_key=True, nullable=False), + sa.Column("user_id", sa.Text(), nullable=False), + sa.Column("source_mode", sa.Text(), nullable=False), + sa.Column("schedule_type", sa.Text(), nullable=False), + sa.Column("status", sa.Text(), nullable=False), + sa.Column("title", sa.Text(), nullable=False), + sa.Column("notes", sa.Text(), nullable=True), + sa.Column("start_time", sa.Text(), nullable=True), + sa.Column("end_time", sa.Text(), nullable=True), + sa.Column("timezone", sa.Text(), nullable=True), + sa.Column("location_name", sa.Text(), nullable=True), + sa.Column("location_address", sa.Text(), nullable=True), + sa.Column("latitude", sa.Float(), nullable=True), + sa.Column("longitude", sa.Float(), nullable=True), + sa.Column("geofence_radius_meters", sa.Integer(), nullable=False), + sa.Column("geofence_armed", sa.Integer(), nullable=False), + sa.Column("time_remind_offset_minutes", sa.Integer(), nullable=False), + sa.Column("time_triggered_at", sa.Text(), nullable=True), + sa.Column("geo_triggered_at", sa.Text(), nullable=True), + sa.Column("system_schedule_ref_id", sa.Text(), nullable=True), + sa.Column("system_alarm_ref_id", sa.Text(), nullable=True), + sa.Column("created_at", sa.Text(), nullable=False), + sa.Column("updated_at", sa.Text(), nullable=False), + sa.CheckConstraint( + "source_mode IN ('manual', 'voice')", + name="ck_schedules_source_mode", + ), + sa.CheckConstraint( + "schedule_type IN ('time', 'location')", + name="ck_schedules_schedule_type", + ), + sa.CheckConstraint( + "status IN ('scheduled', 'done', 'deleted')", + name="ck_schedules_status", + ), + sa.CheckConstraint( + "geofence_armed IN (0, 1)", + name="ck_schedules_geofence_armed", + ), + sa.CheckConstraint( + "geofence_radius_meters > 0", + name="ck_schedules_geofence_radius_positive", + ), + sa.CheckConstraint( + "time_remind_offset_minutes >= 0", + name="ck_schedules_time_remind_offset_nonnegative", + ), + sa.CheckConstraint( + "latitude IS NULL OR latitude BETWEEN -90 AND 90", + name="ck_schedules_latitude_range", + ), + sa.CheckConstraint( + "longitude IS NULL OR longitude BETWEEN -180 AND 180", + name="ck_schedules_longitude_range", + ), + sa.CheckConstraint( + "(schedule_type = 'time' AND start_time IS NOT NULL) " + "OR (schedule_type = 'location' AND start_time IS NULL " + "AND latitude IS NOT NULL AND longitude IS NOT NULL)", + name="ck_schedules_schedule_type_requirements", + ), + sa.CheckConstraint( + "end_time IS NULL OR start_time IS NOT NULL", + name="ck_schedules_end_requires_start", + ), + ) + + op.execute( + sa.text( + """ + INSERT INTO schedules_v2 ( + id, user_id, source_mode, schedule_type, status, title, notes, + start_time, end_time, timezone, location_name, location_address, + latitude, longitude, geofence_radius_meters, geofence_armed, + time_remind_offset_minutes, time_triggered_at, geo_triggered_at, + system_schedule_ref_id, system_alarm_ref_id, created_at, updated_at + ) + SELECT + id, + account_id, + 'voice', + schedule_type, + CASE WHEN status = 'deleted' THEN 'deleted' ELSE 'scheduled' END, + title, + NULL, + start_time::text, + end_time::text, + timezone, + location_name, + NULL, + latitude::double precision, + longitude::double precision, + 100, + 0, + 0, + NULL, + NULL, + NULL, + NULL, + created_at::text, + updated_at::text + FROM schedules + """ + ) + ) + + op.drop_index( + "ix_schedule_occurrence_overrides_replacement_schedule_id", + table_name="schedule_occurrence_overrides", + ) + op.drop_table("schedule_occurrence_overrides") + op.drop_table("schedules") + op.rename_table("schedules_v2", "schedules") + op.create_index( + "ix_schedules_user_status_start_time", + "schedules", + ["user_id", "status", "start_time"], + ) + op.create_index( + "ix_schedules_system_schedule_ref_id", + "schedules", + ["system_schedule_ref_id"], + ) + op.create_index( + "ix_schedules_system_alarm_ref_id", + "schedules", + ["system_alarm_ref_id"], + ) diff --git a/backend/src/timeflow/data/__init__.py b/backend/src/timeflow/data/__init__.py index cbdf38a..8cccc51 100644 --- a/backend/src/timeflow/data/__init__.py +++ b/backend/src/timeflow/data/__init__.py @@ -1,6 +1,6 @@ """Database models and primitives for TimeFlow.""" from timeflow.data.database import Base -from timeflow.data.models import Schedule +from timeflow.data.models import Schedule, ScheduleOccurrenceOverride -__all__ = ["Base", "Schedule"] +__all__ = ["Base", "Schedule", "ScheduleOccurrenceOverride"] diff --git a/backend/src/timeflow/data/models.py b/backend/src/timeflow/data/models.py index 6a41d98..13d9c7f 100644 --- a/backend/src/timeflow/data/models.py +++ b/backend/src/timeflow/data/models.py @@ -1,40 +1,57 @@ """SQLAlchemy models for TimeFlow business data.""" -from sqlalchemy import CheckConstraint, Float, Index, Integer, Text +from datetime import datetime +from decimal import Decimal + +from sqlalchemy import ( + BigInteger, + Boolean, + CheckConstraint, + DateTime, + ForeignKey, + Index, + Integer, + Numeric, + String, + UniqueConstraint, +) from sqlalchemy.orm import Mapped, mapped_column from timeflow.data.database import Base class Schedule(Base): - """Business schedule persisted by the backend.""" + """Cloud-authoritative schedule persisted by the backend.""" __tablename__ = "schedules" __table_args__ = ( - CheckConstraint( - "source_mode IN ('manual', 'voice')", - name="ck_schedules_source_mode", - ), CheckConstraint( "schedule_type IN ('time', 'location')", name="ck_schedules_schedule_type", ), CheckConstraint( - "status IN ('scheduled', 'done', 'deleted')", + "schedule_kind IN ('once', 'recurring')", + name="ck_schedules_schedule_kind", + ), + CheckConstraint( + "status IN ('active', 'deleted')", name="ck_schedules_status", ), CheckConstraint( - "geofence_armed IN (0, 1)", - name="ck_schedules_geofence_armed", + "reminder_type IS NULL OR reminder_type IN " + "('at_time', 'before_start', 'arrive_location', " + "'return_to_recorded_location')", + name="ck_schedules_reminder_type", ), CheckConstraint( - "geofence_radius_meters > 0", - name="ck_schedules_geofence_radius_positive", + "reminder_strength IS NULL OR reminder_strength IN ('low', 'medium', 'high')", + name="ck_schedules_reminder_strength", ), CheckConstraint( - "time_remind_offset_minutes >= 0", - name="ck_schedules_time_remind_offset_nonnegative", + "reminder_disposition_state IS NULL OR reminder_disposition_state = 'confirmed'", + name="ck_schedules_reminder_disposition_state", ), + CheckConstraint("revision > 0", name="ck_schedules_revision_positive"), CheckConstraint( "latitude IS NULL OR latitude BETWEEN -90 AND 90", name="ck_schedules_latitude_range", @@ -46,46 +63,127 @@ class Schedule(Base): CheckConstraint( "(schedule_type = 'time' AND start_time IS NOT NULL) " "OR (schedule_type = 'location' AND start_time IS NULL " - "AND latitude IS NOT NULL AND longitude IS NOT NULL)", + "AND latitude IS NOT NULL AND longitude IS NOT NULL AND is_all_day = false)", name="ck_schedules_schedule_type_requirements", ), + CheckConstraint( + "(schedule_kind = 'once' AND recurrence_rule IS NULL) " + "OR (schedule_kind = 'recurring' AND schedule_type = 'time' " + "AND recurrence_rule IS NOT NULL)", + name="ck_schedules_recurrence_requirements", + ), + CheckConstraint( + "is_all_day = false " + "OR (schedule_type = 'time' AND start_time IS NOT NULL AND end_time IS NOT NULL)", + name="ck_schedules_all_day_requirements", + ), CheckConstraint( "end_time IS NULL OR start_time IS NOT NULL", name="ck_schedules_end_requires_start", ), + CheckConstraint( + "reminder_offset_minutes IS NULL OR reminder_offset_minutes >= 0", + name="ck_schedules_reminder_offset_nonnegative", + ), + CheckConstraint( + "(reminder_type IS NULL AND reminder_trigger_at IS NULL " + "AND reminder_offset_minutes IS NULL AND reminder_strength IS NULL " + "AND reminder_disposition_state IS NULL) " + "OR (reminder_type IS NOT NULL AND reminder_strength IS NOT NULL)", + name="ck_schedules_reminder_presence", + ), + CheckConstraint( + "reminder_type IS NULL " + "OR (reminder_type = 'at_time' AND reminder_trigger_at IS NOT NULL " + "AND reminder_offset_minutes IS NULL) " + "OR (reminder_type = 'before_start' AND reminder_trigger_at IS NULL " + "AND reminder_offset_minutes IS NOT NULL) " + "OR (reminder_type IN ('arrive_location', 'return_to_recorded_location') " + "AND reminder_trigger_at IS NULL AND reminder_offset_minutes IS NULL " + "AND latitude IS NOT NULL AND longitude IS NOT NULL)", + name="ck_schedules_reminder_configuration", + ), + CheckConstraint( + "(status = 'active' AND deleted_at IS NULL) " + "OR (status = 'deleted' AND deleted_at IS NOT NULL)", + name="ck_schedules_deleted_at", + ), Index( - "ix_schedules_user_status_start_time", - "user_id", + "ix_schedules_account_status_start_time", + "account_id", "status", "start_time", ), - Index("ix_schedules_system_schedule_ref_id", "system_schedule_ref_id"), - Index("ix_schedules_system_alarm_ref_id", "system_alarm_ref_id"), + Index("ix_schedules_account_revision", "account_id", "revision"), ) - id: Mapped[str] = mapped_column(Text, primary_key=True) - user_id: Mapped[str] = mapped_column(Text, nullable=False) - source_mode: Mapped[str] = mapped_column(Text, nullable=False) - schedule_type: Mapped[str] = mapped_column(Text, nullable=False) - status: Mapped[str] = mapped_column(Text, nullable=False) - title: Mapped[str] = mapped_column(Text, nullable=False) - notes: Mapped[str | None] = mapped_column(Text, nullable=True) - start_time: Mapped[str | None] = mapped_column(Text, nullable=True) - end_time: Mapped[str | None] = mapped_column(Text, nullable=True) - timezone: Mapped[str | None] = mapped_column(Text, nullable=True) - location_name: Mapped[str | None] = mapped_column(Text, nullable=True) - location_address: Mapped[str | None] = mapped_column(Text, nullable=True) - latitude: Mapped[float | None] = mapped_column(Float, nullable=True) - longitude: Mapped[float | None] = mapped_column(Float, nullable=True) - geofence_radius_meters: Mapped[int] = mapped_column(Integer, nullable=False) - geofence_armed: Mapped[int] = mapped_column(Integer, nullable=False) - time_remind_offset_minutes: Mapped[int] = mapped_column(Integer, nullable=False) - time_triggered_at: Mapped[str | None] = mapped_column(Text, nullable=True) - geo_triggered_at: Mapped[str | None] = mapped_column(Text, nullable=True) - system_schedule_ref_id: Mapped[str | None] = mapped_column(Text, nullable=True) - system_alarm_ref_id: Mapped[str | None] = mapped_column(Text, nullable=True) - created_at: Mapped[str] = mapped_column(Text, nullable=False) - updated_at: Mapped[str] = mapped_column(Text, nullable=False) + id: Mapped[str] = mapped_column(String(64), primary_key=True) + account_id: Mapped[str] = mapped_column(String(64), nullable=False) + schedule_type: Mapped[str] = mapped_column(String(16), nullable=False) + schedule_kind: Mapped[str] = mapped_column( + String(16), nullable=False, default="once", server_default="once" + ) + title: Mapped[str] = mapped_column(String(255), nullable=False) + is_all_day: Mapped[bool] = mapped_column( + Boolean, nullable=False, default=False, server_default="false" + ) + start_time: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) + end_time: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) + timezone: Mapped[str] = mapped_column(String(64), nullable=False) + recurrence_rule: Mapped[str | None] = mapped_column(String(512), nullable=True) + location_name: Mapped[str | None] = mapped_column(String(255), nullable=True) + latitude: Mapped[Decimal | None] = mapped_column(Numeric(9, 6), nullable=True) + longitude: Mapped[Decimal | None] = mapped_column(Numeric(9, 6), nullable=True) + reminder_type: Mapped[str | None] = mapped_column(String(32), nullable=True) + reminder_trigger_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) + reminder_offset_minutes: Mapped[int | None] = mapped_column(Integer, nullable=True) + reminder_strength: Mapped[str | None] = mapped_column(String(16), nullable=True) + reminder_disposition_state: Mapped[str | None] = mapped_column(String(16), nullable=True) + status: Mapped[str] = mapped_column(String(16), nullable=False) + revision: Mapped[int] = mapped_column(BigInteger, nullable=False) + created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False) + updated_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False) + deleted_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True), nullable=True) + + +class ScheduleOccurrenceOverride(Base): + """Persisted exception for one occurrence of a recurring schedule.""" + + __tablename__ = "schedule_occurrence_overrides" + __table_args__ = ( + CheckConstraint( + "action IN ('cancel', 'replace')", + name="ck_schedule_occurrence_overrides_action", + ), + CheckConstraint( + "(action = 'cancel' AND replacement_schedule_id IS NULL) " + "OR (action = 'replace' AND replacement_schedule_id IS NOT NULL)", + name="ck_schedule_occurrence_overrides_replacement", + ), + UniqueConstraint( + "schedule_id", + "occurrence_start", + name="uq_schedule_occurrence_overrides_schedule_occurrence", + ), + Index( + "ix_schedule_occurrence_overrides_replacement_schedule_id", + "replacement_schedule_id", + ), + ) + + id: Mapped[str] = mapped_column(String(64), primary_key=True) + schedule_id: Mapped[str] = mapped_column( + String(64), ForeignKey("schedules.id", ondelete="RESTRICT"), nullable=False + ) + occurrence_start: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False) + action: Mapped[str] = mapped_column(String(16), nullable=False) + replacement_schedule_id: Mapped[str | None] = mapped_column( + String(64), ForeignKey("schedules.id", ondelete="RESTRICT"), nullable=True + ) + created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False) + updated_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False) -__all__ = ["Schedule"] +__all__ = ["Schedule", "ScheduleOccurrenceOverride"] diff --git a/backend/src/timeflow/data/repositories/__init__.py b/backend/src/timeflow/data/repositories/__init__.py new file mode 100644 index 0000000..3ee8d0a --- /dev/null +++ b/backend/src/timeflow/data/repositories/__init__.py @@ -0,0 +1,5 @@ +"""Concrete database repositories.""" + +from timeflow.data.repositories.schedule import ScheduleRepository + +__all__ = ["ScheduleRepository"] diff --git a/backend/src/timeflow/data/repositories/schedule.py b/backend/src/timeflow/data/repositories/schedule.py new file mode 100644 index 0000000..0447aa3 --- /dev/null +++ b/backend/src/timeflow/data/repositories/schedule.py @@ -0,0 +1,252 @@ +"""SQLAlchemy persistence adapter for schedules and occurrence overrides.""" + +from decimal import Decimal + +from sqlalchemy import select, update +from sqlalchemy.orm import Session + +from timeflow.business.calendar.contracts import ( + OccurrenceOverrideAction, + ReminderDispositionState, + ReminderStrength, + ReminderType, + ScheduleKind, + ScheduleOccurrenceOverrideSnapshot, + ScheduleSnapshot, + ScheduleStatus, + ScheduleType, +) +from timeflow.data.models import Schedule, ScheduleOccurrenceOverride + + +class ScheduleRepository: + """Account-scoped persistence primitives used by the schedule service. + + The repository flushes changes but never commits or rolls back. Transaction + ownership stays with the later application service implementation. + """ + + def __init__(self, session: Session) -> None: + self._session = session + + def add_schedule(self, snapshot: ScheduleSnapshot) -> ScheduleSnapshot: + """Insert one cloud schedule without committing the surrounding transaction.""" + model = Schedule(**_schedule_values(snapshot)) + self._session.add(model) + self._session.flush() + return _to_schedule_snapshot(model) + + def get_schedule( + self, + *, + account_id: str, + schedule_id: str, + include_deleted: bool = False, + ) -> ScheduleSnapshot | None: + """Return one schedule only when it belongs to the requested account.""" + statement = select(Schedule).where( + Schedule.account_id == account_id, + Schedule.id == schedule_id, + ) + if not include_deleted: + statement = statement.where(Schedule.status == ScheduleStatus.ACTIVE.value) + + model = self._session.scalar(statement) + return None if model is None else _to_schedule_snapshot(model) + + def list_schedules( + self, + *, + account_id: str, + include_deleted: bool = False, + ) -> tuple[ScheduleSnapshot, ...]: + """List schedules for exactly one account in deterministic order.""" + statement = select(Schedule).where(Schedule.account_id == account_id) + if not include_deleted: + statement = statement.where(Schedule.status == ScheduleStatus.ACTIVE.value) + statement = statement.order_by(Schedule.start_time, Schedule.created_at, Schedule.id) + + return tuple(_to_schedule_snapshot(model) for model in self._session.scalars(statement)) + + def update_schedule( + self, + *, + snapshot: ScheduleSnapshot, + expected_revision: int, + ) -> ScheduleSnapshot | None: + """Conditionally replace mutable persisted fields using optimistic revision.""" + values = _schedule_values(snapshot) + values.pop("id") + values.pop("account_id") + statement = ( + update(Schedule) + .where( + Schedule.id == snapshot.id, + Schedule.account_id == snapshot.account_id, + Schedule.revision == expected_revision, + ) + .values(**values) + .returning(Schedule) + ) + model = self._session.scalars(statement).one_or_none() + return None if model is None else _to_schedule_snapshot(model) + + def add_occurrence_override( + self, + *, + account_id: str, + snapshot: ScheduleOccurrenceOverrideSnapshot, + ) -> ScheduleOccurrenceOverrideSnapshot | None: + """Insert an override only for schedules owned by the requested account.""" + if not self._schedule_belongs_to_account(account_id, snapshot.schedule_id): + return None + if snapshot.replacement_schedule_id is not None and not self._schedule_belongs_to_account( + account_id, snapshot.replacement_schedule_id + ): + return None + + model = ScheduleOccurrenceOverride( + id=snapshot.id, + schedule_id=snapshot.schedule_id, + occurrence_start=snapshot.occurrence_start, + action=snapshot.action.value, + replacement_schedule_id=snapshot.replacement_schedule_id, + created_at=snapshot.created_at, + updated_at=snapshot.updated_at, + ) + self._session.add(model) + self._session.flush() + return _to_override_snapshot(model) + + def get_occurrence_override( + self, + *, + account_id: str, + override_id: str, + ) -> ScheduleOccurrenceOverrideSnapshot | None: + """Return one override through an account-scoped schedule join.""" + statement = ( + select(ScheduleOccurrenceOverride) + .join(Schedule, Schedule.id == ScheduleOccurrenceOverride.schedule_id) + .where( + Schedule.account_id == account_id, + ScheduleOccurrenceOverride.id == override_id, + ) + ) + model = self._session.scalar(statement) + return None if model is None else _to_override_snapshot(model) + + def list_occurrence_overrides( + self, + *, + account_id: str, + schedule_id: str | None = None, + ) -> tuple[ScheduleOccurrenceOverrideSnapshot, ...]: + """List overrides whose recurring schedules belong to one account.""" + statement = ( + select(ScheduleOccurrenceOverride) + .join(Schedule, Schedule.id == ScheduleOccurrenceOverride.schedule_id) + .where(Schedule.account_id == account_id) + ) + if schedule_id is not None: + statement = statement.where(ScheduleOccurrenceOverride.schedule_id == schedule_id) + statement = statement.order_by( + ScheduleOccurrenceOverride.occurrence_start, + ScheduleOccurrenceOverride.id, + ) + return tuple(_to_override_snapshot(model) for model in self._session.scalars(statement)) + + def _schedule_belongs_to_account(self, account_id: str, schedule_id: str) -> bool: + statement = select(Schedule.id).where( + Schedule.account_id == account_id, + Schedule.id == schedule_id, + ) + return self._session.scalar(statement) is not None + + +def _schedule_values(snapshot: ScheduleSnapshot) -> dict[str, object]: + """Map the framework-independent snapshot to ORM column values.""" + return { + "id": snapshot.id, + "account_id": snapshot.account_id, + "schedule_type": snapshot.schedule_type.value, + "schedule_kind": snapshot.schedule_kind.value, + "title": snapshot.title, + "is_all_day": snapshot.is_all_day, + "start_time": snapshot.start_time, + "end_time": snapshot.end_time, + "timezone": snapshot.timezone, + "recurrence_rule": snapshot.recurrence_rule, + "location_name": snapshot.location_name, + "latitude": None if snapshot.latitude is None else Decimal(str(snapshot.latitude)), + "longitude": None if snapshot.longitude is None else Decimal(str(snapshot.longitude)), + "reminder_type": None if snapshot.reminder_type is None else snapshot.reminder_type.value, + "reminder_trigger_at": snapshot.reminder_trigger_at, + "reminder_offset_minutes": snapshot.reminder_offset_minutes, + "reminder_strength": ( + None if snapshot.reminder_strength is None else snapshot.reminder_strength.value + ), + "reminder_disposition_state": ( + None + if snapshot.reminder_disposition_state is None + else snapshot.reminder_disposition_state.value + ), + "status": snapshot.status.value, + "revision": snapshot.revision, + "created_at": snapshot.created_at, + "updated_at": snapshot.updated_at, + "deleted_at": snapshot.deleted_at, + } + + +def _to_schedule_snapshot(model: Schedule) -> ScheduleSnapshot: + """Map one ORM row to the shared final cloud snapshot contract.""" + return ScheduleSnapshot( + id=model.id, + account_id=model.account_id, + schedule_type=ScheduleType(model.schedule_type), + schedule_kind=ScheduleKind(model.schedule_kind), + title=model.title, + is_all_day=model.is_all_day, + timezone=model.timezone, + status=ScheduleStatus(model.status), + revision=model.revision, + created_at=model.created_at, + updated_at=model.updated_at, + start_time=model.start_time, + end_time=model.end_time, + recurrence_rule=model.recurrence_rule, + location_name=model.location_name, + latitude=None if model.latitude is None else float(model.latitude), + longitude=None if model.longitude is None else float(model.longitude), + reminder_type=None if model.reminder_type is None else ReminderType(model.reminder_type), + reminder_trigger_at=model.reminder_trigger_at, + reminder_offset_minutes=model.reminder_offset_minutes, + reminder_strength=( + None if model.reminder_strength is None else ReminderStrength(model.reminder_strength) + ), + reminder_disposition_state=( + None + if model.reminder_disposition_state is None + else ReminderDispositionState(model.reminder_disposition_state) + ), + deleted_at=model.deleted_at, + ) + + +def _to_override_snapshot( + model: ScheduleOccurrenceOverride, +) -> ScheduleOccurrenceOverrideSnapshot: + """Map one occurrence override row to the shared snapshot contract.""" + return ScheduleOccurrenceOverrideSnapshot( + id=model.id, + schedule_id=model.schedule_id, + occurrence_start=model.occurrence_start, + action=OccurrenceOverrideAction(model.action), + replacement_schedule_id=model.replacement_schedule_id, + created_at=model.created_at, + updated_at=model.updated_at, + ) + + +__all__ = ["ScheduleRepository"] diff --git a/backend/tests/test_schedule_repository.py b/backend/tests/test_schedule_repository.py new file mode 100644 index 0000000..fa42ca6 --- /dev/null +++ b/backend/tests/test_schedule_repository.py @@ -0,0 +1,124 @@ +"""Repository tests for account isolation and optimistic persistence primitives.""" + +from collections.abc import Generator +from dataclasses import replace +from datetime import UTC, datetime + +import pytest +from sqlalchemy import create_engine +from sqlalchemy.orm import Session + +from timeflow.business.calendar import ( + OccurrenceOverrideAction, + ScheduleKind, + ScheduleOccurrenceOverrideSnapshot, + ScheduleSnapshot, + ScheduleStatus, + ScheduleType, +) +from timeflow.data.database import Base +from timeflow.data.repositories import ScheduleRepository + + +@pytest.fixture +def session() -> Generator[Session, None, None]: + """Return an isolated SQLAlchemy session for repository behavior tests.""" + engine = create_engine("sqlite+pysqlite:///:memory:") + Base.metadata.create_all(engine) + with Session(engine) as database_session: + yield database_session + + +def _schedule( + schedule_id: str, + account_id: str, + *, + revision: int = 1, +) -> ScheduleSnapshot: + now = datetime.now(UTC) + return ScheduleSnapshot( + id=schedule_id, + account_id=account_id, + schedule_type=ScheduleType.TIME, + schedule_kind=ScheduleKind.ONCE, + title=f"Schedule {schedule_id}", + is_all_day=False, + timezone="Asia/Shanghai", + status=ScheduleStatus.ACTIVE, + revision=revision, + created_at=now, + updated_at=now, + start_time=now, + ) + + +def test_schedule_reads_are_account_scoped(session: Session) -> None: + """An account can never load rows owned by another account.""" + repository = ScheduleRepository(session) + repository.add_schedule(_schedule("schedule-a", "account-a")) + repository.add_schedule(_schedule("schedule-b", "account-b")) + + assert repository.get_schedule(account_id="account-a", schedule_id="schedule-b") is None + account_schedules = repository.list_schedules(account_id="account-a") + assert [snapshot.id for snapshot in account_schedules] == ["schedule-a"] + + +def test_schedule_update_requires_matching_account_and_revision(session: Session) -> None: + """The persistence update is a single optimistic conditional statement.""" + repository = ScheduleRepository(session) + original = repository.add_schedule(_schedule("schedule-a", "account-a")) + updated = replace(original, title="Updated", revision=2, updated_at=datetime.now(UTC)) + + assert repository.update_schedule(snapshot=updated, expected_revision=0) is None + persisted = repository.update_schedule(snapshot=updated, expected_revision=1) + + assert persisted is not None + assert persisted.title == "Updated" + assert persisted.revision == 2 + + +def test_deleted_schedules_are_hidden_by_default(session: Session) -> None: + """Soft-deleted rows remain available only to explicit snapshot queries.""" + repository = ScheduleRepository(session) + original = repository.add_schedule(_schedule("schedule-a", "account-a")) + deleted = replace( + original, + status=ScheduleStatus.DELETED, + revision=2, + updated_at=datetime.now(UTC), + deleted_at=datetime.now(UTC), + ) + assert repository.update_schedule(snapshot=deleted, expected_revision=1) is not None + + assert repository.get_schedule(account_id="account-a", schedule_id="schedule-a") is None + assert ( + repository.get_schedule( + account_id="account-a", + schedule_id="schedule-a", + include_deleted=True, + ) + is not None + ) + + +def test_occurrence_overrides_are_account_scoped(session: Session) -> None: + """Override writes and reads follow ownership through their parent schedule.""" + repository = ScheduleRepository(session) + parent = repository.add_schedule(_schedule("series-a", "account-a")) + repository.add_schedule(_schedule("series-b", "account-b")) + now = datetime.now(UTC) + override = ScheduleOccurrenceOverrideSnapshot( + id="override-a", + schedule_id=parent.id, + occurrence_start=now, + action=OccurrenceOverrideAction.CANCEL, + created_at=now, + updated_at=now, + ) + + assert repository.add_occurrence_override(account_id="account-b", snapshot=override) is None + assert repository.add_occurrence_override(account_id="account-a", snapshot=override) == override + assert repository.list_occurrence_overrides(account_id="account-b") == () + account_overrides = repository.list_occurrence_overrides(account_id="account-a") + assert [snapshot.id for snapshot in account_overrides] == ["override-a"] + assert account_overrides[0].action is OccurrenceOverrideAction.CANCEL diff --git a/backend/tests/test_schedule_schema.py b/backend/tests/test_schedule_schema.py index dddec23..06f952e 100644 --- a/backend/tests/test_schedule_schema.py +++ b/backend/tests/test_schedule_schema.py @@ -1,33 +1,70 @@ -"""Schema tests for the MVP schedules table.""" +"""Schema tests for the v3.10 schedule persistence model.""" -from timeflow.data.models import Schedule +from typing import cast +from sqlalchemy import Table -def test_schedule_table_has_mvp_columns() -> None: - """The schedules table exposes the fields required by the MVP design.""" +from timeflow.data.models import Schedule, ScheduleOccurrenceOverride + +def test_schedule_table_matches_cloud_snapshot_storage_fields() -> None: + """The cloud table stores only authoritative schedule and reminder fields.""" assert list(Schedule.__table__.columns.keys()) == [ "id", - "user_id", - "source_mode", + "account_id", "schedule_type", - "status", + "schedule_kind", "title", - "notes", + "is_all_day", "start_time", "end_time", "timezone", + "recurrence_rule", "location_name", - "location_address", "latitude", "longitude", - "geofence_radius_meters", - "geofence_armed", - "time_remind_offset_minutes", - "time_triggered_at", - "geo_triggered_at", - "system_schedule_ref_id", - "system_alarm_ref_id", + "reminder_type", + "reminder_trigger_at", + "reminder_offset_minutes", + "reminder_strength", + "reminder_disposition_state", + "status", + "revision", "created_at", "updated_at", + "deleted_at", ] + + +def test_schedule_table_keeps_device_runtime_state_out_of_cloud_storage() -> None: + """Geofence, snooze, and next-trigger state belong only to client SQLite.""" + columns = set(Schedule.__table__.columns.keys()) + + assert columns.isdisjoint( + { + "geofence_armed", + "next_trigger_at", + "snoozed_until", + "sync_status", + "time_triggered_at", + "geo_triggered_at", + } + ) + + +def test_occurrence_override_table_matches_v3_contract() -> None: + """Only exceptional recurring occurrences are persisted.""" + assert list(ScheduleOccurrenceOverride.__table__.columns.keys()) == [ + "id", + "schedule_id", + "occurrence_start", + "action", + "replacement_schedule_id", + "created_at", + "updated_at", + ] + + occurrence_table = cast(Table, ScheduleOccurrenceOverride.__table__) + constraint_names = {constraint.name for constraint in occurrence_table.constraints} + assert "uq_schedule_occurrence_overrides_schedule_occurrence" in constraint_names + assert "ck_schedule_occurrence_overrides_replacement" in constraint_names From b321857f0aa42e3d0201cfdb9056faef72567b3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=94=90=E5=83=A7=E6=B4=97=E5=8F=91=E7=94=A8=E9=A3=98?= =?UTF-8?q?=E6=9F=94?= <2906266135@qq.com> Date: Tue, 11 Aug 2026 14:37:16 +0800 Subject: [PATCH 2/3] fix(schedule): address persistence integrity review --- .../20260810_0003_schedule_storage_v3.py | 36 ++++++++++++++++--- .../timeflow/data/repositories/schedule.py | 33 ++++++++++++++--- backend/tests/test_schedule_repository.py | 20 +++++++++++ backend/tests/test_schedule_schema.py | 19 ++++++++++ 4 files changed, 99 insertions(+), 9 deletions(-) diff --git a/backend/alembic/versions/20260810_0003_schedule_storage_v3.py b/backend/alembic/versions/20260810_0003_schedule_storage_v3.py index 4759773..0db2beb 100644 --- a/backend/alembic/versions/20260810_0003_schedule_storage_v3.py +++ b/backend/alembic/versions/20260810_0003_schedule_storage_v3.py @@ -183,6 +183,25 @@ def _create_occurrence_overrides_table() -> None: def upgrade() -> None: """Migrate the legacy schedule rows and create occurrence overrides.""" + op.execute( + sa.text( + """ + DO $$ + BEGIN + IF EXISTS ( + SELECT 1 + FROM schedules + WHERE char_length(id) > 64 OR char_length(user_id) > 64 + ) THEN + RAISE EXCEPTION + 'Cannot migrate schedules: id and user_id must not exceed 64 characters'; + END IF; + END + $$ + """ + ) + ) + op.drop_index("ix_schedules_system_alarm_ref_id", table_name="schedules") op.drop_index("ix_schedules_system_schedule_ref_id", table_name="schedules") op.drop_index("ix_schedules_user_status_start_time", table_name="schedules") @@ -211,8 +230,8 @@ def upgrade() -> None: updated_at, deleted_at ) SELECT - left(id, 64), - left(user_id, 64), + id, + user_id, schedule_type, 'once', left(title, 255), @@ -229,12 +248,14 @@ def upgrade() -> None: NULL, NULL, NULL, - CASE WHEN status = 'deleted' THEN 'deleted' ELSE 'active' END, + -- v3 cannot represent legacy `done`; keep completed rows + -- non-active rather than resurrecting them as active schedules. + CASE WHEN status IN ('done', 'deleted') THEN 'deleted' ELSE 'active' END, 1, COALESCE(NULLIF(created_at, '')::timestamptz, now()), COALESCE(NULLIF(updated_at, '')::timestamptz, now()), CASE - WHEN status = 'deleted' + WHEN status IN ('done', 'deleted') THEN COALESCE(NULLIF(updated_at, '')::timestamptz, now()) ELSE NULL END @@ -249,7 +270,12 @@ def upgrade() -> None: def downgrade() -> None: - """Restore the legacy MVP schedule shape while retaining core row data.""" + """Restore the legacy shape without reactivating completed rows. + + The v3 status model cannot distinguish a legacy ``done`` row from a deleted + row after upgrade, so both are restored as ``deleted`` rather than turning a + previously completed schedule back into ``scheduled``. + """ op.create_table( "schedules_v2", sa.Column("id", sa.Text(), primary_key=True, nullable=False), diff --git a/backend/src/timeflow/data/repositories/schedule.py b/backend/src/timeflow/data/repositories/schedule.py index 0447aa3..9766f20 100644 --- a/backend/src/timeflow/data/repositories/schedule.py +++ b/backend/src/timeflow/data/repositories/schedule.py @@ -75,9 +75,6 @@ def update_schedule( expected_revision: int, ) -> ScheduleSnapshot | None: """Conditionally replace mutable persisted fields using optimistic revision.""" - values = _schedule_values(snapshot) - values.pop("id") - values.pop("account_id") statement = ( update(Schedule) .where( @@ -85,7 +82,7 @@ def update_schedule( Schedule.account_id == snapshot.account_id, Schedule.revision == expected_revision, ) - .values(**values) + .values(**_schedule_update_values(snapshot)) .returning(Schedule) ) model = self._session.scalars(statement).one_or_none() @@ -199,6 +196,34 @@ def _schedule_values(snapshot: ScheduleSnapshot) -> dict[str, object]: } +def _schedule_update_values(snapshot: ScheduleSnapshot) -> dict[str, object]: + """Map only fields that the update contract permits callers to replace.""" + values = _schedule_values(snapshot) + mutable_fields = ( + "schedule_type", + "schedule_kind", + "title", + "is_all_day", + "start_time", + "end_time", + "timezone", + "recurrence_rule", + "location_name", + "latitude", + "longitude", + "reminder_type", + "reminder_trigger_at", + "reminder_offset_minutes", + "reminder_strength", + "reminder_disposition_state", + "status", + "revision", + "updated_at", + "deleted_at", + ) + return {field: values[field] for field in mutable_fields} + + def _to_schedule_snapshot(model: Schedule) -> ScheduleSnapshot: """Map one ORM row to the shared final cloud snapshot contract.""" return ScheduleSnapshot( diff --git a/backend/tests/test_schedule_repository.py b/backend/tests/test_schedule_repository.py index fa42ca6..bb0bb60 100644 --- a/backend/tests/test_schedule_repository.py +++ b/backend/tests/test_schedule_repository.py @@ -77,6 +77,26 @@ def test_schedule_update_requires_matching_account_and_revision(session: Session assert persisted.revision == 2 +def test_schedule_update_preserves_immutable_creation_time(session: Session) -> None: + """A replacement snapshot cannot rewrite the persisted creation timestamp.""" + repository = ScheduleRepository(session) + original = repository.add_schedule(_schedule("schedule-a", "account-a")) + caller_created_at = datetime(2020, 1, 1, tzinfo=UTC) + updated = replace( + original, + title="Updated", + revision=2, + created_at=caller_created_at, + updated_at=datetime.now(UTC), + ) + + persisted = repository.update_schedule(snapshot=updated, expected_revision=1) + + assert persisted is not None + assert persisted.created_at == original.created_at.replace(tzinfo=None) + assert persisted.created_at != caller_created_at.replace(tzinfo=None) + + def test_deleted_schedules_are_hidden_by_default(session: Session) -> None: """Soft-deleted rows remain available only to explicit snapshot queries.""" repository = ScheduleRepository(session) diff --git a/backend/tests/test_schedule_schema.py b/backend/tests/test_schedule_schema.py index 06f952e..5b6837b 100644 --- a/backend/tests/test_schedule_schema.py +++ b/backend/tests/test_schedule_schema.py @@ -1,11 +1,16 @@ """Schema tests for the v3.10 schedule persistence model.""" +from pathlib import Path from typing import cast from sqlalchemy import Table from timeflow.data.models import Schedule, ScheduleOccurrenceOverride +MIGRATION_SOURCE = ( + Path(__file__).parents[1] / "alembic" / "versions" / "20260810_0003_schedule_storage_v3.py" +).read_text(encoding="utf-8") + def test_schedule_table_matches_cloud_snapshot_storage_fields() -> None: """The cloud table stores only authoritative schedule and reminder fields.""" @@ -68,3 +73,17 @@ def test_occurrence_override_table_matches_v3_contract() -> None: constraint_names = {constraint.name for constraint in occurrence_table.constraints} assert "uq_schedule_occurrence_overrides_schedule_occurrence" in constraint_names assert "ck_schedule_occurrence_overrides_replacement" in constraint_names + + +def test_schedule_migration_rejects_overlong_legacy_identifiers() -> None: + """Legacy ownership identifiers are rejected instead of silently truncated.""" + assert "char_length(id) > 64" in MIGRATION_SOURCE + assert "char_length(user_id) > 64" in MIGRATION_SOURCE + assert "left(id, 64)" not in MIGRATION_SOURCE + assert "left(user_id, 64)" not in MIGRATION_SOURCE + + +def test_schedule_migration_keeps_completed_legacy_rows_non_active() -> None: + """A migration round trip must not resurrect a completed legacy schedule.""" + assert MIGRATION_SOURCE.count("status IN ('done', 'deleted')") == 2 + assert "CASE WHEN status = 'deleted' THEN 'deleted' ELSE 'scheduled' END" in MIGRATION_SOURCE From 448cfc8cf91259b8137e3a859e6c8528358062ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=94=90=E5=83=A7=E6=B4=97=E5=8F=91=E7=94=A8=E9=A3=98?= =?UTF-8?q?=E6=9F=94?= <2906266135@qq.com> Date: Tue, 11 Aug 2026 16:02:39 +0800 Subject: [PATCH 3/3] fix(schedule): enforce repository revision semantics --- .github/workflows/ci.yml | 8 + .../timeflow/data/repositories/__init__.py | 7 +- .../timeflow/data/repositories/schedule.py | 94 +++++++- backend/tests/conftest.py | 33 +++ .../test_postgres_schedule_repository.py | 215 ++++++++++++++++++ backend/tests/test_postgres_schema.py | 28 --- backend/tests/test_schedule_repository.py | 142 +++++++++++- 7 files changed, 483 insertions(+), 44 deletions(-) create mode 100644 backend/tests/conftest.py create mode 100644 backend/tests/test_postgres_schedule_repository.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f1c85c2..d836b52 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -84,6 +84,7 @@ jobs: --health-retries 10 env: TIMEFLOW_DATABASE_URL: postgresql+psycopg://timeapp:timeapp@127.0.0.1:5432/timeapp + TIMEFLOW_TEST_DATABASE_URL: postgresql+psycopg://timeapp:timeapp@127.0.0.1:5432/timeapp steps: - uses: actions/checkout@v4 @@ -104,6 +105,13 @@ jobs: - name: Apply migrations (upgrade head) run: uv run alembic upgrade head + - name: PostgreSQL schema and repository integration tests + run: >- + uv run pytest + tests/test_postgres_schema.py + tests/test_postgres_schedule_repository.py + --no-cov + - name: Verify downgrade path (downgrade base) run: uv run alembic downgrade base diff --git a/backend/src/timeflow/data/repositories/__init__.py b/backend/src/timeflow/data/repositories/__init__.py index 3ee8d0a..5635556 100644 --- a/backend/src/timeflow/data/repositories/__init__.py +++ b/backend/src/timeflow/data/repositories/__init__.py @@ -1,5 +1,8 @@ """Concrete database repositories.""" -from timeflow.data.repositories.schedule import ScheduleRepository +from timeflow.data.repositories.schedule import ( + ScheduleRepository, + ScheduleRevisionConflictError, +) -__all__ = ["ScheduleRepository"] +__all__ = ["ScheduleRepository", "ScheduleRevisionConflictError"] diff --git a/backend/src/timeflow/data/repositories/schedule.py b/backend/src/timeflow/data/repositories/schedule.py index 9766f20..76961a4 100644 --- a/backend/src/timeflow/data/repositories/schedule.py +++ b/backend/src/timeflow/data/repositories/schedule.py @@ -1,5 +1,6 @@ """SQLAlchemy persistence adapter for schedules and occurrence overrides.""" +from datetime import datetime from decimal import Decimal from sqlalchemy import select, update @@ -19,6 +20,27 @@ from timeflow.data.models import Schedule, ScheduleOccurrenceOverride +class ScheduleRevisionConflictError(RuntimeError): + """An account-owned schedule no longer has the expected revision.""" + + __slots__ = ("actual_revision", "expected_revision", "schedule_id") + + def __init__( + self, + *, + schedule_id: str, + expected_revision: int, + actual_revision: int, + ) -> None: + super().__init__( + f"Schedule {schedule_id!r} revision conflict: " + f"expected {expected_revision}, found {actual_revision}" + ) + self.schedule_id = schedule_id + self.expected_revision = expected_revision + self.actual_revision = actual_revision + + class ScheduleRepository: """Account-scoped persistence primitives used by the schedule service. @@ -74,7 +96,12 @@ def update_schedule( snapshot: ScheduleSnapshot, expected_revision: int, ) -> ScheduleSnapshot | None: - """Conditionally replace mutable persisted fields using optimistic revision.""" + """Atomically replace mutable fields and increment the persisted revision. + + Returns ``None`` when the schedule is absent from the account. Raises + ``ScheduleRevisionConflictError`` when the account owns the schedule but + its current revision differs from ``expected_revision``. + """ statement = ( update(Schedule) .where( @@ -82,11 +109,29 @@ def update_schedule( Schedule.account_id == snapshot.account_id, Schedule.revision == expected_revision, ) - .values(**_schedule_update_values(snapshot)) + .values( + **_schedule_update_values(snapshot), + revision=Schedule.revision + 1, + ) .returning(Schedule) ) model = self._session.scalars(statement).one_or_none() - return None if model is None else _to_schedule_snapshot(model) + if model is not None: + return _to_schedule_snapshot(model) + + actual_revision = self._session.scalar( + select(Schedule.revision).where( + Schedule.id == snapshot.id, + Schedule.account_id == snapshot.account_id, + ) + ) + if actual_revision is not None: + raise ScheduleRevisionConflictError( + schedule_id=snapshot.id, + expected_revision=expected_revision, + actual_revision=actual_revision, + ) + return None def add_occurrence_override( self, @@ -133,6 +178,46 @@ def get_occurrence_override( model = self._session.scalar(statement) return None if model is None else _to_override_snapshot(model) + def update_occurrence_override( + self, + *, + account_id: str, + schedule_id: str, + occurrence_start: datetime, + action: OccurrenceOverrideAction, + replacement_schedule_id: str | None, + updated_at: datetime, + ) -> ScheduleOccurrenceOverrideSnapshot | None: + """Update the existing override identified by its schedule occurrence. + + This persistence operation deliberately does not increment the parent + schedule revision. The application service must call ``update_schedule`` + with the expected revision in the same Session transaction so a conflict + rolls back both aggregate changes together. + """ + if not self._schedule_belongs_to_account(account_id, schedule_id): + return None + if replacement_schedule_id is not None and not self._schedule_belongs_to_account( + account_id, replacement_schedule_id + ): + return None + + statement = ( + update(ScheduleOccurrenceOverride) + .where( + ScheduleOccurrenceOverride.schedule_id == schedule_id, + ScheduleOccurrenceOverride.occurrence_start == occurrence_start, + ) + .values( + action=action.value, + replacement_schedule_id=replacement_schedule_id, + updated_at=updated_at, + ) + .returning(ScheduleOccurrenceOverride) + ) + model = self._session.scalars(statement).one_or_none() + return None if model is None else _to_override_snapshot(model) + def list_occurrence_overrides( self, *, @@ -217,7 +302,6 @@ def _schedule_update_values(snapshot: ScheduleSnapshot) -> dict[str, object]: "reminder_strength", "reminder_disposition_state", "status", - "revision", "updated_at", "deleted_at", ) @@ -274,4 +358,4 @@ def _to_override_snapshot( ) -__all__ = ["ScheduleRepository"] +__all__ = ["ScheduleRepository", "ScheduleRevisionConflictError"] diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py new file mode 100644 index 0000000..bb85ae9 --- /dev/null +++ b/backend/tests/conftest.py @@ -0,0 +1,33 @@ +"""Shared pytest fixtures for disposable PostgreSQL integration tests.""" + +import os +from collections.abc import Iterator + +import pytest +import sqlalchemy as sa +from sqlalchemy import Engine +from sqlalchemy.engine import Connection + + +@pytest.fixture(scope="session") +def postgres_engine() -> Iterator[Engine]: + """Connect only when an explicit disposable integration database is supplied.""" + database_url = os.getenv("TIMEFLOW_TEST_DATABASE_URL") + if database_url is None: + pytest.skip("TIMEFLOW_TEST_DATABASE_URL is not set") + engine = sa.create_engine(database_url) + try: + yield engine + finally: + engine.dispose() + + +@pytest.fixture +def postgres_connection(postgres_engine: Engine) -> Iterator[Connection]: + """Roll back integration-test data after each test.""" + with postgres_engine.connect() as connection: + transaction = connection.begin() + try: + yield connection + finally: + transaction.rollback() diff --git a/backend/tests/test_postgres_schedule_repository.py b/backend/tests/test_postgres_schedule_repository.py new file mode 100644 index 0000000..e609cee --- /dev/null +++ b/backend/tests/test_postgres_schedule_repository.py @@ -0,0 +1,215 @@ +"""PostgreSQL integration tests for the schedule persistence adapter.""" + +from collections.abc import Iterator +from dataclasses import replace +from datetime import UTC, datetime + +import pytest +import sqlalchemy as sa +from sqlalchemy import Engine +from sqlalchemy.engine import Connection +from sqlalchemy.orm import Session + +from timeflow.business.calendar import ( + OccurrenceOverrideAction, + ScheduleKind, + ScheduleOccurrenceOverrideSnapshot, + ScheduleSnapshot, + ScheduleStatus, + ScheduleType, +) +from timeflow.data.models import Account, ScheduleOccurrenceOverride +from timeflow.data.repositories import ScheduleRepository, ScheduleRevisionConflictError + + +@pytest.fixture +def postgres_session(postgres_connection: Connection) -> Iterator[Session]: + """Join the shared rollback transaction through a test-owned savepoint.""" + with Session( + bind=postgres_connection, + join_transaction_mode="create_savepoint", + ) as session: + yield session + + +def _seed_account(session: Session, account_id: str) -> None: + now = datetime.now(UTC) + session.add( + Account( + id=account_id, + username=f"{account_id}@example.com", + password_hash="test-password-hash", + created_at=now, + updated_at=now, + ) + ) + session.flush() + + +def _schedule( + schedule_id: str, + account_id: str, + *, + revision: int = 1, +) -> ScheduleSnapshot: + now = datetime.now(UTC) + return ScheduleSnapshot( + id=schedule_id, + account_id=account_id, + schedule_type=ScheduleType.TIME, + schedule_kind=ScheduleKind.ONCE, + title=f"Schedule {schedule_id}", + is_all_day=False, + timezone="Asia/Shanghai", + status=ScheduleStatus.ACTIVE, + revision=revision, + created_at=now, + updated_at=now, + start_time=now, + ) + + +def test_postgres_repository_insert_and_update_return_final_revision( + postgres_session: Session, +) -> None: + """PostgreSQL RETURNING exposes the row after its atomic revision increment.""" + _seed_account(postgres_session, "account-a") + repository = ScheduleRepository(postgres_session) + inserted = repository.add_schedule(_schedule("schedule-a", "account-a", revision=5)) + + assert inserted.revision == 5 + + caller_snapshot = replace( + inserted, + title="Updated", + revision=1, + updated_at=datetime.now(UTC), + ) + updated = repository.update_schedule(snapshot=caller_snapshot, expected_revision=5) + + assert updated is not None + assert updated.title == "Updated" + assert updated.revision == 6 + assert updated.created_at == inserted.created_at + + +def test_postgres_repository_reports_conflict_and_preserves_account_isolation( + postgres_session: Session, +) -> None: + """A stale owner gets a conflict while another account observes no target row.""" + _seed_account(postgres_session, "account-a") + _seed_account(postgres_session, "account-b") + repository = ScheduleRepository(postgres_session) + inserted = repository.add_schedule(_schedule("schedule-a", "account-a", revision=5)) + stale = replace(inserted, title="Stale", revision=100, updated_at=datetime.now(UTC)) + + with pytest.raises(ScheduleRevisionConflictError) as raised: + repository.update_schedule(snapshot=stale, expected_revision=4) + + assert raised.value.actual_revision == 5 + + wrong_owner = replace(stale, account_id="account-b") + assert repository.update_schedule(snapshot=wrong_owner, expected_revision=5) is None + persisted = repository.get_schedule(account_id="account-a", schedule_id=inserted.id) + assert persisted is not None + assert persisted.title == inserted.title + assert persisted.revision == 5 + + +def test_postgres_repository_updates_one_unique_occurrence_override( + postgres_session: Session, +) -> None: + """One occurrence can change action without duplicating its unique key.""" + _seed_account(postgres_session, "account-a") + repository = ScheduleRepository(postgres_session) + parent = repository.add_schedule(_schedule("series-a", "account-a", revision=5)) + replacement = repository.add_schedule(_schedule("replacement-a", "account-a")) + now = datetime.now(UTC) + original = ScheduleOccurrenceOverrideSnapshot( + id="override-a", + schedule_id=parent.id, + occurrence_start=now, + action=OccurrenceOverrideAction.CANCEL, + created_at=now, + updated_at=now, + ) + repository.add_occurrence_override(account_id="account-a", snapshot=original) + + updated = repository.update_occurrence_override( + account_id="account-a", + schedule_id=parent.id, + occurrence_start=now, + action=OccurrenceOverrideAction.REPLACE, + replacement_schedule_id=replacement.id, + updated_at=datetime.now(UTC), + ) + + assert updated is not None + assert updated.id == original.id + assert updated.action is OccurrenceOverrideAction.REPLACE + assert updated.replacement_schedule_id == replacement.id + persisted_parent = repository.get_schedule(account_id="account-a", schedule_id=parent.id) + assert persisted_parent is not None + assert persisted_parent.revision == 5 + + duplicate = replace(original, id="override-duplicate") + with pytest.raises(sa.exc.IntegrityError): + with postgres_session.begin_nested(): + repository.add_occurrence_override(account_id="account-a", snapshot=duplicate) + + +def test_postgres_repository_and_database_enforce_override_ownership_and_fk( + postgres_session: Session, +) -> None: + """Repository ownership checks complement the PostgreSQL foreign key.""" + _seed_account(postgres_session, "account-a") + _seed_account(postgres_session, "account-b") + repository = ScheduleRepository(postgres_session) + parent = repository.add_schedule(_schedule("series-a", "account-a")) + other_account_replacement = repository.add_schedule(_schedule("replacement-b", "account-b")) + now = datetime.now(UTC) + cross_account = ScheduleOccurrenceOverrideSnapshot( + id="override-cross-account", + schedule_id=parent.id, + occurrence_start=now, + action=OccurrenceOverrideAction.REPLACE, + replacement_schedule_id=other_account_replacement.id, + created_at=now, + updated_at=now, + ) + + assert ( + repository.add_occurrence_override(account_id="account-a", snapshot=cross_account) is None + ) + + with pytest.raises(sa.exc.IntegrityError): + with postgres_session.begin_nested(): + postgres_session.add( + ScheduleOccurrenceOverride( + id="override-missing-parent", + schedule_id="missing-series", + occurrence_start=now, + action=OccurrenceOverrideAction.CANCEL.value, + replacement_schedule_id=None, + created_at=now, + updated_at=now, + ) + ) + postgres_session.flush() + + +def test_postgres_repository_respects_caller_transaction_rollback( + postgres_engine: Engine, +) -> None: + """Repository flushes are discarded when the owning transaction rolls back.""" + account_id = "account-rollback" + schedule_id = "schedule-rollback" + with Session(postgres_engine) as session: + _seed_account(session, account_id) + repository = ScheduleRepository(session) + repository.add_schedule(_schedule(schedule_id, account_id)) + session.rollback() + + with Session(postgres_engine) as verification_session: + repository = ScheduleRepository(verification_session) + assert repository.get_schedule(account_id=account_id, schedule_id=schedule_id) is None diff --git a/backend/tests/test_postgres_schema.py b/backend/tests/test_postgres_schema.py index 90897cb..9182338 100644 --- a/backend/tests/test_postgres_schema.py +++ b/backend/tests/test_postgres_schema.py @@ -1,7 +1,5 @@ """PostgreSQL integration contract for the cloud schedule schema.""" -import os -from collections.abc import Iterator from datetime import UTC, datetime from typing import Any @@ -100,32 +98,6 @@ def _canonical_type(column_type: sa.types.TypeEngine[object]) -> str: raise AssertionError(f"Unexpected reflected type: {column_type!r}") -@pytest.fixture(scope="module") -def postgres_engine() -> Iterator[Engine]: - """Connect only when an explicit disposable integration database is supplied.""" - - database_url = os.getenv("TIMEFLOW_TEST_DATABASE_URL") - if database_url is None: - pytest.skip("TIMEFLOW_TEST_DATABASE_URL is not set") - engine = sa.create_engine(database_url) - try: - yield engine - finally: - engine.dispose() - - -@pytest.fixture -def postgres_connection(postgres_engine: Engine) -> Iterator[Connection]: - """Roll back behavior-test data after each test.""" - - with postgres_engine.connect() as connection: - transaction = connection.begin() - try: - yield connection - finally: - transaction.rollback() - - def _account_values(account_id: str = "acct-test") -> dict[str, Any]: now = datetime(2026, 8, 10, tzinfo=UTC) return { diff --git a/backend/tests/test_schedule_repository.py b/backend/tests/test_schedule_repository.py index 0ed1097..2c8cbff 100644 --- a/backend/tests/test_schedule_repository.py +++ b/backend/tests/test_schedule_repository.py @@ -18,7 +18,7 @@ ) from timeflow.data.database import Base from timeflow.data.models import Account -from timeflow.data.repositories import ScheduleRepository +from timeflow.data.repositories import ScheduleRepository, ScheduleRevisionConflictError @pytest.fixture @@ -78,18 +78,71 @@ def test_schedule_reads_are_account_scoped(session: Session) -> None: assert [snapshot.id for snapshot in account_schedules] == ["schedule-a"] -def test_schedule_update_requires_matching_account_and_revision(session: Session) -> None: - """The persistence update is a single optimistic conditional statement.""" +def test_schedule_update_atomically_increments_revision(session: Session) -> None: + """The database revision advances regardless of the caller snapshot value.""" repository = ScheduleRepository(session) - original = repository.add_schedule(_schedule("schedule-a", "account-a")) - updated = replace(original, title="Updated", revision=2, updated_at=datetime.now(UTC)) + original = repository.add_schedule(_schedule("schedule-a", "account-a", revision=5)) + unchanged_revision = replace( + original, + title="Updated once", + revision=5, + updated_at=datetime.now(UTC), + ) - assert repository.update_schedule(snapshot=updated, expected_revision=0) is None - persisted = repository.update_schedule(snapshot=updated, expected_revision=1) + persisted = repository.update_schedule(snapshot=unchanged_revision, expected_revision=5) + + assert persisted is not None + assert persisted.title == "Updated once" + assert persisted.revision == 6 + + lower_revision = replace( + persisted, + title="Updated twice", + revision=1, + updated_at=datetime.now(UTC), + ) + persisted_again = repository.update_schedule(snapshot=lower_revision, expected_revision=6) + + assert persisted_again is not None + assert persisted_again.title == "Updated twice" + assert persisted_again.revision == 7 + + +def test_schedule_update_raises_explicit_revision_conflict(session: Session) -> None: + """A stale expected revision is distinguishable from a missing schedule.""" + repository = ScheduleRepository(session) + original = repository.add_schedule(_schedule("schedule-a", "account-a", revision=5)) + updated = replace(original, title="Stale update", revision=100, updated_at=datetime.now(UTC)) + + with pytest.raises(ScheduleRevisionConflictError) as raised: + repository.update_schedule(snapshot=updated, expected_revision=4) + assert raised.value.schedule_id == original.id + assert raised.value.expected_revision == 4 + assert raised.value.actual_revision == 5 + persisted = repository.get_schedule(account_id="account-a", schedule_id=original.id) assert persisted is not None - assert persisted.title == "Updated" - assert persisted.revision == 2 + assert persisted.title == original.title + assert persisted.revision == 5 + + +def test_schedule_update_cannot_cross_account_boundary(session: Session) -> None: + """An otherwise valid revision cannot update another account's schedule.""" + repository = ScheduleRepository(session) + original = repository.add_schedule(_schedule("schedule-a", "account-a", revision=5)) + wrong_owner = replace( + original, + account_id="account-b", + title="Cross-account update", + revision=100, + updated_at=datetime.now(UTC), + ) + + assert repository.update_schedule(snapshot=wrong_owner, expected_revision=5) is None + persisted = repository.get_schedule(account_id="account-a", schedule_id=original.id) + assert persisted is not None + assert persisted.title == original.title + assert persisted.revision == 5 def test_schedule_update_preserves_immutable_creation_time(session: Session) -> None: @@ -157,3 +210,74 @@ def test_occurrence_overrides_are_account_scoped(session: Session) -> None: account_overrides = repository.list_occurrence_overrides(account_id="account-a") assert [snapshot.id for snapshot in account_overrides] == ["override-a"] assert account_overrides[0].action is OccurrenceOverrideAction.CANCEL + + +def test_occurrence_override_can_be_updated_without_hidden_revision_change( + session: Session, +) -> None: + """Override persistence changes one unique occurrence but not its parent aggregate.""" + repository = ScheduleRepository(session) + parent = repository.add_schedule(_schedule("series-a", "account-a", revision=5)) + replacement = repository.add_schedule(_schedule("replacement-a", "account-a")) + now = datetime.now(UTC) + original = ScheduleOccurrenceOverrideSnapshot( + id="override-a", + schedule_id=parent.id, + occurrence_start=now, + action=OccurrenceOverrideAction.CANCEL, + created_at=now, + updated_at=now, + ) + repository.add_occurrence_override(account_id="account-a", snapshot=original) + + updated = repository.update_occurrence_override( + account_id="account-a", + schedule_id=parent.id, + occurrence_start=now, + action=OccurrenceOverrideAction.REPLACE, + replacement_schedule_id=replacement.id, + updated_at=datetime.now(UTC), + ) + + assert updated is not None + assert updated.id == original.id + assert updated.action is OccurrenceOverrideAction.REPLACE + assert updated.replacement_schedule_id == replacement.id + assert updated.created_at.replace(tzinfo=UTC) == original.created_at + persisted_parent = repository.get_schedule(account_id="account-a", schedule_id=parent.id) + assert persisted_parent is not None + assert persisted_parent.revision == 5 + + +def test_occurrence_override_update_is_account_scoped(session: Session) -> None: + """An account cannot modify an override through another account's parent.""" + repository = ScheduleRepository(session) + parent = repository.add_schedule(_schedule("series-a", "account-a")) + now = datetime.now(UTC) + original = ScheduleOccurrenceOverrideSnapshot( + id="override-a", + schedule_id=parent.id, + occurrence_start=now, + action=OccurrenceOverrideAction.CANCEL, + created_at=now, + updated_at=now, + ) + repository.add_occurrence_override(account_id="account-a", snapshot=original) + + assert ( + repository.update_occurrence_override( + account_id="account-b", + schedule_id=parent.id, + occurrence_start=now, + action=OccurrenceOverrideAction.REPLACE, + replacement_schedule_id=None, + updated_at=datetime.now(UTC), + ) + is None + ) + persisted = repository.get_occurrence_override( + account_id="account-a", + override_id=original.id, + ) + assert persisted is not None + assert persisted.action is OccurrenceOverrideAction.CANCEL