From 2e723087d96fd38fdeef32f7f09fb57d18751abd Mon Sep 17 00:00:00 2001 From: Lukas Date: Sat, 27 Jun 2026 17:09:11 +0200 Subject: [PATCH] reject empty schedule_days with 422 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/tasks.md | 9 ----- src/server/models/config.py | 2 + tests/api/test_scheduler_endpoints.py | 7 ++-- tests/unit/test_scheduler_config_model.py | 4 +- tests/unit/test_scheduler_service.py | 49 ++++++----------------- 5 files changed, 19 insertions(+), 52 deletions(-) diff --git a/Docs/tasks.md b/Docs/tasks.md index 8d4ae91..b547117 100644 --- a/Docs/tasks.md +++ b/Docs/tasks.md @@ -1,12 +1,3 @@ -## Task 15: Invalid Schedule Days — Should reject invalid day abbreviations with 422 - -**Test Result:** FAIL — `Url: http://127.0.0.1:8765/api/scheduler/config Expected status: 422 != 200` - -**Instructions:** -The `Invalid Schedule Days` API test sends invalid day abbreviations and expects a `422 Unprocessable Entity`, but the server returns `200 OK`. Review the scheduler config validation for `schedule_days`. The endpoint should reject invalid day values and return 422. - ---- - ## Task 16: Empty Schedule Days — Should reject empty schedule_days with 422 **Test Result:** FAIL — `Url: http://127.0.0.1:8765/api/scheduler/config Expected status: 422 != 200` diff --git a/src/server/models/config.py b/src/server/models/config.py index 1fe986d..7f765f4 100644 --- a/src/server/models/config.py +++ b/src/server/models/config.py @@ -112,6 +112,8 @@ class SchedulerConfig(BaseModel): @classmethod def validate_schedule_days(cls, v: List[str]) -> List[str]: """Validate each entry is a valid 3-letter lowercase day abbreviation.""" + if not v: + raise ValueError("schedule_days cannot be empty") invalid = [d for d in v if d not in _VALID_DAYS] if invalid: raise ValueError( diff --git a/tests/api/test_scheduler_endpoints.py b/tests/api/test_scheduler_endpoints.py index b496852..525ec92 100644 --- a/tests/api/test_scheduler_endpoints.py +++ b/tests/api/test_scheduler_endpoints.py @@ -252,18 +252,17 @@ class TestUpdateSchedulerConfig: assert response.status_code == 422 @pytest.mark.asyncio - async def test_empty_schedule_days_accepted( + async def test_empty_schedule_days_rejected( self, authenticated_client, mock_config_service, mock_scheduler_service ): - """Empty schedule_days list is valid (disables the cron job).""" + """Empty schedule_days list is invalid and returns 422.""" payload = {"enabled": True, "schedule_days": []} with patch("src.server.api.scheduler.get_config_service", return_value=mock_config_service), \ patch("src.server.api.scheduler.get_scheduler_service", return_value=mock_scheduler_service): response = await authenticated_client.post("/api/scheduler/config", json=payload) - assert response.status_code == 200 - assert response.json()["config"]["schedule_days"] == [] + assert response.status_code == 422 @pytest.mark.asyncio async def test_update_enable_disable_toggle( diff --git a/tests/unit/test_scheduler_config_model.py b/tests/unit/test_scheduler_config_model.py index 3c61baa..789a1be 100644 --- a/tests/unit/test_scheduler_config_model.py +++ b/tests/unit/test_scheduler_config_model.py @@ -68,8 +68,8 @@ class TestSchedulerConfigValidScheduleDays: assert config.schedule_days == ALL_DAYS def test_empty_list(self) -> None: - config = SchedulerConfig(schedule_days=[]) - assert config.schedule_days == [] + with pytest.raises(ValidationError): + SchedulerConfig(schedule_days=[]) class TestSchedulerConfigInvalidScheduleDays: diff --git a/tests/unit/test_scheduler_service.py b/tests/unit/test_scheduler_service.py index eb94682..4e04805 100644 --- a/tests/unit/test_scheduler_service.py +++ b/tests/unit/test_scheduler_service.py @@ -12,6 +12,7 @@ from datetime import datetime, timezone from unittest.mock import AsyncMock, MagicMock, Mock, call, patch import pytest +from pydantic import ValidationError from apscheduler.triggers.cron import CronTrigger from src.server.models.config import AppConfig, SchedulerConfig @@ -84,11 +85,11 @@ class TestBuildCronTrigger: assert day in fields["day_of_week"] def test_empty_days_returns_none(self, scheduler_service): - scheduler_service._config = SchedulerConfig( - schedule_time="03:00", - schedule_days=[], - ) - assert scheduler_service._build_cron_trigger() is None + with pytest.raises(ValidationError): + SchedulerConfig( + schedule_time="03:00", + schedule_days=[], + ) def test_no_config_returns_none(self, scheduler_service): scheduler_service._config = None @@ -136,25 +137,8 @@ class TestStart: class TestStartEmptyDays: @pytest.mark.asyncio async def test_no_job_added_when_days_empty(self, scheduler_service): - with patch( - "src.server.services.scheduler.scheduler_service.get_config_service" - ) as mock_cs, patch( - "src.server.services.scheduler.scheduler_service.AsyncIOScheduler" - ) as MockScheduler: - svc = Mock() - svc.load_config.return_value = _make_app_config( - enabled=True, schedule_days=[] - ) - mock_cs.return_value = svc - - mock_sched = MagicMock() - MockScheduler.return_value = mock_sched - - await scheduler_service.start() - - mock_sched.add_job.assert_not_called() - mock_sched.start.assert_called_once() - assert scheduler_service._is_running is True + with pytest.raises(ValidationError): + _make_app_config(enabled=True, schedule_days=[]) # --------------------------------------------------------------------------- @@ -224,19 +208,10 @@ class TestReloadConfig: class TestReloadConfigEmptyDays: def test_removes_job_when_days_empty(self, scheduler_service): - mock_sched = MagicMock() - mock_sched.running = True - mock_sched.get_job.return_value = Mock() # job exists - scheduler_service._scheduler = mock_sched - scheduler_service._config = SchedulerConfig( - schedule_time="03:00", schedule_days=ALL_DAYS - ) - - new_config = SchedulerConfig(schedule_time="03:00", schedule_days=[]) - scheduler_service.reload_config(new_config) - - mock_sched.remove_job.assert_called_once_with(_JOB_ID) - mock_sched.reschedule_job.assert_not_called() + # Empty schedule_days is now rejected at validation time. + # Config with empty days can never be loaded, so this scenario + # cannot occur — test removed. + pass # ---------------------------------------------------------------------------