+
{t('trackingConfig.maxTimesReached').replace('{n}', String(max))}
+ {:else} + + {/if} +{t(field.inlineHelp)}
{/if} - {#if hasError} -{timeListErrors[field.key]}
+ {:else} + {@const inputType = field.type === 'date' ? 'date' + : field.type === 'time' ? 'time' + : 'number'} + + {#if field.inlineHelp} +{t(field.inlineHelp)}
{/if} {/if} diff --git a/packages/server/src/notify_bridge_server/api/tracking_configs.py b/packages/server/src/notify_bridge_server/api/tracking_configs.py index 0bdf70b..e1cf504 100644 --- a/packages/server/src/notify_bridge_server/api/tracking_configs.py +++ b/packages/server/src/notify_bridge_server/api/tracking_configs.py @@ -11,11 +11,66 @@ from ..auth.dependencies import get_current_user from ..database.engine import get_session from ..database.models import TrackingConfig, User from ..services.scheduler import reschedule_immich_dispatch_jobs +from ..services.time_list import TimeListError, normalize_time_list _LOGGER = logging.getLogger(__name__) router = APIRouter(prefix="/api/tracking-configs", tags=["tracking-configs"]) +# Immich dispatch slots that fire on a wall-clock schedule. Each has a +# ``{kind}_enabled`` flag and a ``{kind}_times`` comma-separated HH:MM list. +_DISPATCH_KINDS = ("periodic", "scheduled", "memory") +# TrackingConfig fields holding comma-separated ``HH:MM`` dispatch schedules. +# Normalized (validated, de-duplicated, sorted, capped) on every write so the +# scheduler only ever reads clean values and cron jobs stay deterministic. +_TIME_LIST_FIELDS = tuple(f"{k}_times" for k in _DISPATCH_KINDS) + + +def _normalize_time_fields(values: dict) -> None: + """Canonicalize any ``*_times`` keys present in ``values`` (in place). + + Raises HTTP 422 with a field-scoped message when an entry is malformed or + the list exceeds the per-day cap, so the client surfaces exactly which slot + was rejected instead of the input being silently dropped at schedule time. + """ + for field in _TIME_LIST_FIELDS: + if values.get(field) is None: + continue + try: + values[field] = normalize_time_list(values[field]) + except TimeListError as err: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"{field}: {err}", + ) from err + + +def _validate_enabled_have_times(values: dict, existing: TrackingConfig | None) -> None: + """Reject enabling a dispatch slot with no fire times. + + An enabled slot whose ``{kind}_times`` normalizes to empty would save fine + but the scheduler creates zero cron jobs for it — a silently dead slot that + shows as "enabled" in the UI yet never fires. We fail the write with a 422 + instead. Only kinds the request actually touches (enabled flag or times) are + checked, so unrelated edits to a pre-existing config aren't blocked; the + effective state is the request value merged over ``existing``. + """ + for kind in _DISPATCH_KINDS: + enabled_key, times_key = f"{kind}_enabled", f"{kind}_times" + if enabled_key not in values and times_key not in values: + continue + enabled = values.get( + enabled_key, getattr(existing, enabled_key, False) if existing else False + ) + times = values.get( + times_key, getattr(existing, times_key, "") if existing else "" + ) + if enabled and not (times or "").strip(): + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"{times_key}: add at least one time when {kind} is enabled", + ) + class TrackingConfigCreate(BaseModel): provider_type: str @@ -124,7 +179,10 @@ async def create_config( user: User = Depends(get_current_user), session: AsyncSession = Depends(get_session), ): - config = TrackingConfig(user_id=user.id, **body.model_dump()) + data = body.model_dump() + _normalize_time_fields(data) + _validate_enabled_have_times(data, None) + config = TrackingConfig(user_id=user.id, **data) session.add(config) await session.commit() await session.refresh(config) @@ -150,7 +208,10 @@ async def update_config( session: AsyncSession = Depends(get_session), ): config = await _get(session, config_id, user.id) - for field, value in body.model_dump(exclude_unset=True).items(): + updates = body.model_dump(exclude_unset=True) + _normalize_time_fields(updates) + _validate_enabled_have_times(updates, config) + for field, value in updates.items(): setattr(config, field, value) session.add(config) await session.commit() diff --git a/packages/server/src/notify_bridge_server/services/scheduler.py b/packages/server/src/notify_bridge_server/services/scheduler.py index 39defdd..81620b9 100644 --- a/packages/server/src/notify_bridge_server/services/scheduler.py +++ b/packages/server/src/notify_bridge_server/services/scheduler.py @@ -7,6 +7,8 @@ from zoneinfo import ZoneInfo, ZoneInfoNotFoundError from apscheduler.schedulers.asyncio import AsyncIOScheduler +from .time_list import parse_hhmm_list + _LOGGER = logging.getLogger(__name__) @@ -896,30 +898,6 @@ _IMMICH_DISPATCH_KINDS = ("scheduled", "periodic", "memory") _IMMICH_DISPATCH_PREFIX = "immich_dispatch_" -def _parse_hhmm_list(raw: str) -> list[tuple[int, int]]: - """Parse ``"09:00,18:30"`` → ``[(9, 0), (18, 30)]``, skipping bad entries. - - A typo in one slot must not prevent the others from scheduling — we log - and move on rather than raising. - """ - out: list[tuple[int, int]] = [] - for part in (raw or "").split(","): - part = part.strip() - if not part: - continue - try: - h_str, m_str = part.split(":", 1) - hour, minute = int(h_str), int(m_str) - except ValueError: - _LOGGER.warning("Skipping invalid time literal %r", part) - continue - if not (0 <= hour <= 23 and 0 <= minute <= 59): - _LOGGER.warning("Skipping out-of-range time %r", part) - continue - out.append((hour, minute)) - return out - - async def _run_immich_dispatch(tracker_id: int, kind: str) -> None: """APScheduler entry point — wraps the dispatch helper to swallow errors.""" from .scheduled_dispatch import dispatch_scheduled_for_tracker @@ -994,7 +972,24 @@ async def _load_immich_dispatch_jobs() -> None: if not getattr(tc, f"{kind}_enabled", False): continue times_raw = getattr(tc, f"{kind}_times", "") or "" - for hour, minute in _parse_hhmm_list(times_raw): + parsed = parse_hhmm_list(times_raw) + # Observability for misconfigured/legacy data: warn when some tokens + # were unparseable (the lenient parser drops them silently) and when + # an enabled slot resolves to zero fire times (it will never fire). + raw_tokens = [p for p in times_raw.split(",") if p.strip()] + if len(parsed) < len(raw_tokens): + _LOGGER.warning( + "Tracker %d %s: dropped %d unparseable time(s) from %r", + tracker.id, kind, len(raw_tokens) - len(parsed), times_raw, + ) + if not parsed: + _LOGGER.warning( + "Tracker %d has %s enabled but no valid fire times (%r); " + "slot will not fire", + tracker.id, kind, times_raw, + ) + continue + for hour, minute in parsed: job_id = f"{_IMMICH_DISPATCH_PREFIX}{kind}_{tracker.id}_{hour:02d}{minute:02d}" scheduler.add_job( _run_immich_dispatch, diff --git a/packages/server/src/notify_bridge_server/services/time_list.py b/packages/server/src/notify_bridge_server/services/time_list.py new file mode 100644 index 0000000..75af713 --- /dev/null +++ b/packages/server/src/notify_bridge_server/services/time_list.py @@ -0,0 +1,99 @@ +"""Parsing + normalization for comma-separated ``HH:MM`` dispatch time lists. + +The Immich scheduled / periodic / memory slots fire at one or more wall-clock +times per day, stored on ``TrackingConfig`` as a comma-separated ``HH:MM`` +string (e.g. ``"09:00,18:30"``). Two consumers share this module: + +* the API layer (``api.tracking_configs``) calls :func:`normalize_time_list` + to validate + canonicalize user input before persisting — rejecting malformed + entries, de-duplicating, sorting ascending, and capping the count. +* the scheduler (``services.scheduler``) calls :func:`parse_hhmm_list` to turn a + stored (already-normalized) string into ``(hour, minute)`` tuples, tolerating + legacy or hand-edited values by skipping anything unparseable rather than + letting one bad slot stop the others from firing. +""" + +from __future__ import annotations + +# A generous ceiling: 24 distinct fire times per day is already an unusual +# config. The cap mainly exists so a pathological paste can't spawn hundreds of +# cron jobs for a single tracker. +MAX_DISPATCH_TIMES = 24 + + +class TimeListError(ValueError): + """Raised by :func:`normalize_time_list` on malformed input or over-cap. + + Subclasses ``ValueError`` so callers that only care about "bad input" can + catch the broad type; the API layer catches this specifically to map it to + an HTTP 422 with the human-readable message. + """ + + +def _parse_one(token: str) -> tuple[int, int] | None: + """Parse a single ``HH:MM`` token; return ``None`` if malformed/out-of-range. + + Strict on shape (exactly one ``:``, both parts 1-2 plain ASCII digits) but + lenient on width so ``"9:0"`` parses to ``(9, 0)`` — :func:`normalize_time_list` + re-emits it zero-padded. ``int()`` alone is too permissive here: it would + accept signs (``"+9"``), PEP 515 underscores (``"1_0"``), and non-ASCII + decimal digits (Arabic-Indic ``"٠٩"``), none of which are valid wall-clock + literals — so we gate on ``str.isdigit()``/ASCII before converting. + """ + h_str, sep, m_str = token.partition(":") + if not sep: + return None + for part in (h_str, m_str): + if not (1 <= len(part) <= 2 and part.isascii() and part.isdigit()): + return None + hour, minute = int(h_str), int(m_str) + if not (0 <= hour <= 23 and 0 <= minute <= 59): + return None + return hour, minute + + +def parse_hhmm_list(raw: str) -> list[tuple[int, int]]: + """Lenient parse: ``"09:00,18:30"`` → ``[(9, 0), (18, 30)]``. + + Skips blank/invalid entries rather than raising — the scheduler must keep + firing the valid times even if one slot was hand-edited to garbage. Order is + preserved and duplicates are *not* collapsed (the scheduler keys jobs by + time, so a duplicate simply replaces its own job id). + """ + out: list[tuple[int, int]] = [] + for part in (raw or "").split(","): + part = part.strip() + if not part: + continue + hm = _parse_one(part) + if hm is not None: + out.append(hm) + return out + + +def normalize_time_list(raw: str, *, max_count: int = MAX_DISPATCH_TIMES) -> str: + """Validate + canonicalize a comma-separated ``HH:MM`` list. + + Returns the canonical form: zero-padded, de-duplicated, sorted ascending, + and comma-joined with no spaces — e.g. ``" 9:0, 18:30 ,09:00"`` → + ``"09:00,18:30"``. An empty/whitespace input returns ``""`` (the valid + "no scheduled fires" state). + + Raises :class:`TimeListError` when any entry is not a valid ``HH:MM`` time, + or when the de-duplicated count exceeds ``max_count``. The caller maps this + to an HTTP 422 so the user gets a clear message instead of silent dropping. + """ + seen: set[tuple[int, int]] = set() + for part in (raw or "").split(","): + part = part.strip() + if not part: + continue + hm = _parse_one(part) + if hm is None: + raise TimeListError(f"Invalid time {part!r}; use HH:MM (00:00-23:59)") + seen.add(hm) + if len(seen) > max_count: + raise TimeListError( + f"Too many times ({len(seen)}); at most {max_count} are allowed" + ) + return ",".join(f"{h:02d}:{m:02d}" for h, m in sorted(seen)) diff --git a/packages/server/tests/test_time_list.py b/packages/server/tests/test_time_list.py new file mode 100644 index 0000000..594afdb --- /dev/null +++ b/packages/server/tests/test_time_list.py @@ -0,0 +1,116 @@ +"""Unit tests for the shared HH:MM dispatch time-list parser/normalizer.""" + +from __future__ import annotations + +import pytest + +from notify_bridge_server.services.time_list import ( + MAX_DISPATCH_TIMES, + TimeListError, + normalize_time_list, + parse_hhmm_list, +) + + +class TestNormalizeTimeList: + def test_single_time_passthrough(self): + assert normalize_time_list("09:00") == "09:00" + + def test_zero_pads_short_parts(self): + assert normalize_time_list("9:0") == "09:00" + + def test_trims_surrounding_and_inner_whitespace(self): + assert normalize_time_list(" 09:00 , 18:30 ") == "09:00,18:30" + + def test_sorts_ascending(self): + assert normalize_time_list("18:30,09:00,12:15") == "09:00,12:15,18:30" + + def test_deduplicates(self): + # Duplicates collapse even when written with different padding. + assert normalize_time_list("09:00,9:00,09:00") == "09:00" + + def test_empty_string_returns_empty(self): + assert normalize_time_list("") == "" + + def test_whitespace_only_returns_empty(self): + assert normalize_time_list(" ") == "" + + def test_trailing_comma_ignored(self): + assert normalize_time_list("09:00,") == "09:00" + + def test_midnight_and_last_minute_are_valid(self): + assert normalize_time_list("00:00,23:59") == "00:00,23:59" + + @pytest.mark.parametrize( + "bad", + ["24:00", "09:60", "noon", "9", "09-00", "-1:00", "09:5a", "1:2:3"], + ) + def test_invalid_entry_raises(self, bad): + with pytest.raises(TimeListError): + normalize_time_list(bad) + + @pytest.mark.parametrize( + "bad", + [ + "+9:00", # sign not allowed + "1_0:00", # PEP 515 underscore not allowed + "09:0_0", # underscore in minutes + "٩:00", # Arabic-Indic digit (non-ASCII) + "٠٩:٠٠", # Arabic-Indic "09:00" + "09: 00", # inner whitespace in part + "9 :00", # inner whitespace in part + "009:00", # 3-digit hour part + ], + ) + def test_rejects_non_ascii_or_oddly_shaped_digits(self, bad): + # int() alone would accept these; the parser must not. + with pytest.raises(TimeListError): + normalize_time_list(bad) + + def test_one_bad_entry_rejects_whole_list(self): + # Strict on writes: a single bad slot fails the request rather than + # being silently dropped. + with pytest.raises(TimeListError): + normalize_time_list("09:00,bad,18:30") + + def test_at_cap_is_allowed(self): + times = ",".join(f"{h:02d}:00" for h in range(MAX_DISPATCH_TIMES)) + assert normalize_time_list(times) == times + + def test_over_cap_raises(self): + # 25 distinct times (minute-granular) exceeds the default cap of 24. + times = ",".join(f"00:{m:02d}" for m in range(MAX_DISPATCH_TIMES + 1)) + with pytest.raises(TimeListError): + normalize_time_list(times) + + def test_custom_max_count(self): + with pytest.raises(TimeListError): + normalize_time_list("09:00,10:00,11:00", max_count=2) + + def test_duplicates_do_not_count_against_cap(self): + # Three entries but one distinct → fits under a cap of 1. + assert normalize_time_list("09:00,09:00,9:00", max_count=1) == "09:00" + + +class TestParseHhmmList: + def test_basic(self): + assert parse_hhmm_list("09:00,18:30") == [(9, 0), (18, 30)] + + def test_preserves_order_and_duplicates(self): + # Lenient parser keeps input order and does not collapse duplicates — + # the scheduler keys jobs by time so a dup just replaces its own id. + assert parse_hhmm_list("18:30,09:00,18:30") == [(18, 30), (9, 0), (18, 30)] + + def test_skips_invalid_entries(self): + assert parse_hhmm_list("09:00,bad,18:30") == [(9, 0), (18, 30)] + + def test_empty_returns_empty_list(self): + assert parse_hhmm_list("") == [] + assert parse_hhmm_list(" ") == [] + + def test_skips_out_of_range(self): + assert parse_hhmm_list("24:00,09:00") == [(9, 0)] + + def test_skips_non_ascii_and_oddly_shaped(self): + # Lenient parser drops the odd shapes but keeps the valid neighbour. + assert parse_hhmm_list("+9:00,1_0:00,٠٩:٠٠,18:30") == [(18, 30)] diff --git a/packages/server/tests/test_tracking_config_validation.py b/packages/server/tests/test_tracking_config_validation.py new file mode 100644 index 0000000..7e07c38 --- /dev/null +++ b/packages/server/tests/test_tracking_config_validation.py @@ -0,0 +1,114 @@ +"""Tests for the tracking-config write-time guards. + +Covers the cross-field rule that an *enabled* Immich dispatch slot +(periodic / scheduled / memory) must carry at least one fire time — otherwise +the config saves but the scheduler creates zero cron jobs and the slot is +silently dead. +""" + +from __future__ import annotations + +import pytest +from fastapi import HTTPException + +from notify_bridge_server.api.tracking_configs import ( + _normalize_time_fields, + _validate_enabled_have_times, +) +from notify_bridge_server.database.models import TrackingConfig + + +class TestNormalizeTimeFields: + def test_normalizes_each_field_in_place(self): + values = { + "periodic_times": "18:30, 09:00", + "scheduled_times": "9:0", + "memory_times": "", + } + _normalize_time_fields(values) + assert values["periodic_times"] == "09:00,18:30" + assert values["scheduled_times"] == "09:00" + assert values["memory_times"] == "" + + def test_ignores_absent_fields(self): + values = {"name": "x"} + _normalize_time_fields(values) # no KeyError, no-op + assert values == {"name": "x"} + + def test_malformed_raises_422(self): + with pytest.raises(HTTPException) as exc: + _normalize_time_fields({"scheduled_times": "25:00"}) + assert exc.value.status_code == 422 + assert "scheduled_times" in str(exc.value.detail) + + +class TestValidateEnabledHaveTimes: + # --- create path (existing=None) --- + def test_create_enabled_with_times_ok(self): + _validate_enabled_have_times( + {"scheduled_enabled": True, "scheduled_times": "09:00"}, None + ) + + def test_create_enabled_without_times_raises(self): + with pytest.raises(HTTPException) as exc: + _validate_enabled_have_times( + {"scheduled_enabled": True, "scheduled_times": ""}, None + ) + assert exc.value.status_code == 422 + assert "scheduled_times" in str(exc.value.detail) + + def test_create_disabled_without_times_ok(self): + _validate_enabled_have_times( + {"memory_enabled": False, "memory_times": ""}, None + ) + + def test_all_three_kinds_checked(self): + for kind in ("periodic", "scheduled", "memory"): + with pytest.raises(HTTPException): + _validate_enabled_have_times( + {f"{kind}_enabled": True, f"{kind}_times": ""}, None + ) + + # --- update path (effective state = values merged over existing) --- + def test_update_enable_without_providing_times_uses_existing_empty(self): + existing = TrackingConfig(provider_type="immich", name="t", scheduled_times="") + with pytest.raises(HTTPException): + _validate_enabled_have_times({"scheduled_enabled": True}, existing) + + def test_update_enable_with_existing_times_ok(self): + existing = TrackingConfig( + provider_type="immich", name="t", scheduled_times="09:00" + ) + _validate_enabled_have_times({"scheduled_enabled": True}, existing) + + def test_update_clear_times_while_enabled_in_existing_raises(self): + existing = TrackingConfig( + provider_type="immich", + name="t", + scheduled_enabled=True, + scheduled_times="09:00", + ) + with pytest.raises(HTTPException): + _validate_enabled_have_times({"scheduled_times": ""}, existing) + + def test_update_unrelated_field_does_not_trigger_check(self): + # A pre-existing enabled-but-empty config must not block edits that don't + # touch the slot's enabled flag or times (only touched kinds are checked). + existing = TrackingConfig( + provider_type="immich", + name="t", + scheduled_enabled=True, + scheduled_times="", + ) + _validate_enabled_have_times({"name": "renamed"}, existing) + + def test_update_disabling_slot_clears_requirement(self): + existing = TrackingConfig( + provider_type="immich", + name="t", + scheduled_enabled=True, + scheduled_times="09:00", + ) + _validate_enabled_have_times( + {"scheduled_enabled": False, "scheduled_times": ""}, existing + )