From 0073779088392233aa5c850b8164b111b55d7e61 Mon Sep 17 00:00:00 2001 From: nicowre Date: Tue, 30 Jun 2026 13:18:50 +0200 Subject: [PATCH] feat(course-filters): tighten PATCH/POST contract (required name, forbid extras) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - POST and PATCH bodies now reject unknown keys (extra="forbid") so a typo or a stale frontend surfaces as 422 instead of being silently dropped (Pydantic's default). - PATCH.name is now REQUIRED, not Optional. The only editable field is name, and a body without it is a no-op — accepting {} hid client bugs that would only show up in prod. When a second editable field is added later, relax this back to Optional + add an at-least-one-set validator. - Service simplified: the unreachable empty-update branch is gone. - Tests: +4 cases (extra-fields rejected on POST/PATCH, empty PATCH body returns 422, same-name PATCH is a valid no-op that still hits update()). --- src/schemas/course_filter.py | 29 ++++++++--- src/services/course_filter_service.py | 12 ++--- tests/api/test_course_filter_routes.py | 69 ++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 14 deletions(-) diff --git a/src/schemas/course_filter.py b/src/schemas/course_filter.py index ec763c8..ce0ebcd 100644 --- a/src/schemas/course_filter.py +++ b/src/schemas/course_filter.py @@ -1,6 +1,5 @@ """Course filter schemas for request/response validation.""" from datetime import datetime -from typing import Optional from pydantic import BaseModel, ConfigDict, Field, field_validator @@ -18,25 +17,39 @@ def _strip(cls, v: str) -> str: raise ValueError("name must not be blank") return v - model_config = ConfigDict(json_schema_extra={"example": {"name": "SQL"}}) + # ``extra="forbid"`` lets us reject typos / stale clients up-front instead + # of silently dropping unknown keys (Pydantic's default). + model_config = ConfigDict( + extra="forbid", + json_schema_extra={"example": {"name": "SQL"}}, + ) class CourseFilterUpdate(BaseModel): - """Schema for renaming a course filter.""" + """Schema for renaming a course filter. + + Today the only editable field is ``name`` — and it is REQUIRED here, not + optional. Rationale: a PATCH with no editable field is a no-op, and + silently accepting ``{}`` lets buggy clients ship a deploy that „works" + in CI and surprises us in prod. If a second editable field is added later, + relax this back to ``Optional`` and add a model-level ``at-least-one-set`` + validator. + """ - name: Optional[str] = Field(None, description="Neuer Filter-String", min_length=1, max_length=255) + name: str = Field(..., description="Neuer Filter-String", min_length=1, max_length=255) @field_validator("name") @classmethod - def _strip(cls, v: Optional[str]) -> Optional[str]: - if v is None: - return v + def _strip(cls, v: str) -> str: v = v.strip() if not v: raise ValueError("name must not be blank") return v - model_config = ConfigDict(json_schema_extra={"example": {"name": "SQL Grundlagen"}}) + model_config = ConfigDict( + extra="forbid", + json_schema_extra={"example": {"name": "SQL Grundlagen"}}, + ) class CourseFilterResponse(BaseModel): diff --git a/src/services/course_filter_service.py b/src/services/course_filter_service.py index cfac42b..a722835 100644 --- a/src/services/course_filter_service.py +++ b/src/services/course_filter_service.py @@ -61,12 +61,12 @@ def create_filter(self, data: CourseFilterCreate) -> CourseFilter: def update_filter(self, filter_id: UUID, data: CourseFilterUpdate) -> CourseFilter: instance = self.get_filter(filter_id) - update_data = data.model_dump(exclude_unset=True) - if not update_data: - return instance + # ``name`` is required at the schema level, so ``new_name`` is always + # present and non-blank here. A no-op (same name) still flows through + # so ``updated_at`` advances — that's a fine default for PATCH. + new_name = data.name - new_name = update_data.get("name") - if new_name and new_name != instance.name: + if new_name != instance.name: existing = self.repo.get_by_name(new_name) if existing and existing.id != instance.id: raise ConflictException( @@ -74,7 +74,7 @@ def update_filter(self, filter_id: UUID, data: CourseFilterUpdate) -> CourseFilt ) try: - updated = self.repo.update(filter_id, **update_data) + updated = self.repo.update(filter_id, name=new_name) except IntegrityError as e: self.db.rollback() logger.warning( diff --git a/tests/api/test_course_filter_routes.py b/tests/api/test_course_filter_routes.py index 3070b16..9bf0bcd 100644 --- a/tests/api/test_course_filter_routes.py +++ b/tests/api/test_course_filter_routes.py @@ -359,3 +359,72 @@ def test_delete_filter_lecturer_returns_403(): response = client.delete(f"/api/v1/course-filters/{uuid4()}") assert response.status_code == 403 + + +# ── strict schema: required + no extra fields ───────────────────────────────── + + +def test_create_filter_rejects_extra_fields(): + """``extra="forbid"`` blocks unknown keys so a typo / stale client surfaces + as 422 instead of being silently dropped.""" + app.dependency_overrides[get_current_user] = _admin_user + app.dependency_overrides[get_db] = lambda: MagicMock() + + response = client.post( + "/api/v1/course-filters", + json={"name": "SQL", "color": "red"}, + ) + assert response.status_code == 422 + + +def test_update_filter_empty_body_returns_422(): + """``name`` is required on PATCH — an empty body is not a valid no-op.""" + app.dependency_overrides[get_current_user] = _admin_user + app.dependency_overrides[get_db] = lambda: MagicMock() + + response = client.patch(f"/api/v1/course-filters/{uuid4()}", json={}) + assert response.status_code == 422 + + +def test_update_filter_rejects_extra_fields(): + """Same forbid-extra contract on PATCH so frontend doesn't accidentally + POST fields that look editable but aren't.""" + app.dependency_overrides[get_current_user] = _admin_user + app.dependency_overrides[get_db] = lambda: MagicMock() + + response = client.patch( + f"/api/v1/course-filters/{uuid4()}", + json={"name": "x", "id": "spoofed"}, + ) + assert response.status_code == 422 + + +def test_update_filter_to_same_name_succeeds(): + """No-op rename (same name) is intentionally allowed — flows through and + bumps ``updated_at``. The duplicate-check skips because the existing row + IS the same row.""" + fid = str(uuid4()) + existing = _make_filter("SQL", fid=fid) + + repo = MagicMock() + repo.get_by_id.return_value = existing + # If the service mistakenly hit get_by_name here, it would short-circuit + # to 409 because existing.id matches. Make sure it never gets called. + repo.get_by_name.side_effect = AssertionError( + "get_by_name must not be called when name is unchanged" + ) + repo.update.return_value = existing + + app.dependency_overrides[get_current_user] = _admin_user + app.dependency_overrides[get_db] = lambda: MagicMock() + + with patch( + "src.services.course_filter_service.CourseFilterRepository", + return_value=repo, + ): + response = client.patch( + f"/api/v1/course-filters/{fid}", json={"name": "SQL"} + ) + + assert response.status_code == 200 + repo.update.assert_called_once()