From 3832932601de374fbc772924bd268c3f57f09b73 Mon Sep 17 00:00:00 2001 From: Heinrich Chan Date: Tue, 9 Jun 2026 22:38:56 +0800 Subject: [PATCH 1/2] feat: Add DELETE /persons, separate update schema --- http/api.http | 8 ++- pyfastapi/controllers/person.py | 11 ++- pyfastapi/repositories/person.py | 5 +- pyfastapi/schemas/__init__.py | 4 +- pyfastapi/schemas/person.py | 10 +++ pyfastapi/services/person.py | 10 ++- tests/api_tests/test_e2e.py | 6 +- tests/api_tests/test_persons.py | 36 +++++++--- .../services/test_person_service.py | 67 ++++++++++++++++--- 9 files changed, 127 insertions(+), 30 deletions(-) diff --git a/http/api.http b/http/api.http index 310deab..5f27d36 100644 --- a/http/api.http +++ b/http/api.http @@ -34,7 +34,7 @@ Content-Type: application/json ### -### Update a person +### Update a person (upsert) PUT {{host}}/persons/1 Content-Type: application/json @@ -46,6 +46,12 @@ Content-Type: application/json ### +### Delete a person +DELETE {{host}}/persons/1 +Accept: application/json + +### + ### List all countries (paginated) GET {{host}}/countries Accept: application/json diff --git a/pyfastapi/controllers/person.py b/pyfastapi/controllers/person.py index 0934134..eb9b2bb 100644 --- a/pyfastapi/controllers/person.py +++ b/pyfastapi/controllers/person.py @@ -42,5 +42,14 @@ def update_person( service: Annotated[PersonService, Depends()] ) -> Response: logger.debug(f"body {body}") - service.update_person(int(id_), body) + service.update_or_create_person(int(id_), body) + return Response(status_code=204) + + +@router.delete("/{id_}") +def delete_person( + id_: Annotated[int, Path(title="person id")], + service: Annotated[PersonService, Depends()] +) -> Response: + service.delete_person(int(id_)) return Response(status_code=204) diff --git a/pyfastapi/repositories/person.py b/pyfastapi/repositories/person.py index aaf76cf..c0cc855 100644 --- a/pyfastapi/repositories/person.py +++ b/pyfastapi/repositories/person.py @@ -1,7 +1,6 @@ from toolz .functoolz import compose # type: ignore from fastapi_pagination import LimitOffsetPage from fastapi_pagination.ext.sqlalchemy import paginate -from sqlalchemy.dialects.sqlite import insert from sqlalchemy.orm import joinedload from sqlalchemy.sql import select @@ -30,6 +29,7 @@ def create_new_person(self, person: Person) -> Person: return person def update_or_create_person(self, person: Person) -> None: + from sqlalchemy.dialects.sqlite import insert stmt = insert(Person).values( id=person.id, first_name=person.first_name, @@ -45,3 +45,6 @@ def update_or_create_person(self, person: Person) -> None: } ) self.db.execute(stmt) + + def delete_person(self, person: Person) -> None: + self.db.delete(person) diff --git a/pyfastapi/schemas/__init__.py b/pyfastapi/schemas/__init__.py index fdf0102..d88641d 100644 --- a/pyfastapi/schemas/__init__.py +++ b/pyfastapi/schemas/__init__.py @@ -1,8 +1,8 @@ from .continent import ContinentSchema from .country import CountrySchema, CountryListSchema, QueryCountrySchema, SortCountryEnum -from .person import PersonListSchema, PersonCreateSchema, PersonSchema, QueryPersonSchema, SortPersonEnum +from .person import PersonListSchema, PersonCreateSchema, PersonUpdateSchema, PersonSchema, QueryPersonSchema, SortPersonEnum __all__ = [ "ContinentSchema", "CountrySchema", "CountryListSchema", "PersonSchema", "PersonListSchema", "PersonCreateSchema", - "QueryCountrySchema", "QueryPersonSchema", "SortCountryEnum", "SortPersonEnum" + "PersonUpdateSchema", "QueryCountrySchema", "QueryPersonSchema", "SortCountryEnum", "SortPersonEnum" ] diff --git a/pyfastapi/schemas/person.py b/pyfastapi/schemas/person.py index 5793a61..4252154 100644 --- a/pyfastapi/schemas/person.py +++ b/pyfastapi/schemas/person.py @@ -27,6 +27,16 @@ class PersonCreateSchema(PersonBaseSchema): country_code: str +class PersonUpdateSchema(BaseModel): + first_name: str | None = None + last_name: str | None = None + country_code: str | None = None + + model_config = { + "from_attributes": True + } + + class QueryPersonSchema(BaseModel): last_name: str | None = None first_name: str | None = None diff --git a/pyfastapi/services/person.py b/pyfastapi/services/person.py index 4424416..c500429 100644 --- a/pyfastapi/services/person.py +++ b/pyfastapi/services/person.py @@ -44,7 +44,7 @@ def create_person(self, body: PersonCreateSchema) -> Person: self.db.refresh(new_person) return new_person - def update_person(self, id_: int, body: PersonCreateSchema) -> None: + def update_or_create_person(self, id_: int, body: PersonCreateSchema) -> None: if not self.country_repo.get_country(body.country_code): raise CountryNotFoundError(body.country_code) @@ -56,3 +56,11 @@ def update_person(self, id_: int, body: PersonCreateSchema) -> None: person.id = id_ self.person_repo.update_or_create_person(person) self.db.commit() + + def delete_person(self, id_: int) -> None: + person = self.person_repo.get_person(id_) + if not person: + raise PersonNotFoundError(id_) + + self.person_repo.delete_person(person) + self.db.commit() diff --git a/tests/api_tests/test_e2e.py b/tests/api_tests/test_e2e.py index e2a4f5d..bcce3af 100644 --- a/tests/api_tests/test_e2e.py +++ b/tests/api_tests/test_e2e.py @@ -98,8 +98,7 @@ def test_get_person_not_found(init_db: None, client: TestClient) -> None: def test_update_person_nonexistent_id_still_returns_204(init_db: None, client: TestClient, db_session: Session) -> None: """ - merge() semantics mean updating a non-existent ID may INSERT instead of 404. - Document current behavior so regressions are visible. + PUT semantics mean updating a non-existent ID creates the resource (upsert). """ data = { "first_name": "New", @@ -107,10 +106,9 @@ def test_update_person_nonexistent_id_still_returns_204(init_db: None, client: T "country_code": "PH", } response = client.put("/persons/99999", json=data) - # Current implementation returns 204 regardless because merge() can create. assert response.status_code == 204 - # Verify merge() actually created the row in the database + # Verify the upsert actually created the row in the database person_from_db: Person = db_session.execute( select(Person).where(Person.id == 99999) ).scalar_one() diff --git a/tests/api_tests/test_persons.py b/tests/api_tests/test_persons.py index 0a6cee4..0170d6d 100644 --- a/tests/api_tests/test_persons.py +++ b/tests/api_tests/test_persons.py @@ -196,7 +196,6 @@ def test_update_person(init_db: None, client: TestClient, db_session: Session) - def test_upsert_create(init_db: None, client: TestClient, db_session: Session) -> None: - # Use an ID that doesn't exist in seed data (seed data has 13 persons, IDs likely 1-13) new_id = 999 first_name = "Upsert" last_name = "Create" @@ -207,15 +206,12 @@ def test_upsert_create(init_db: None, client: TestClient, db_session: Session) - "country_code": country_code, } - # Verify it doesn't exist stmt_check = select(Person).where(Person.id == new_id) assert db_session.execute(stmt_check).scalar_one_or_none() is None - # Perform PUT (UPSERT) response = client.put(f"/persons/{new_id}", json=data) assert response.status_code == 204 - # Verify it was created person_from_db = db_session.execute(stmt_check).scalar_one() assert person_from_db.id == new_id assert person_from_db.first_name == first_name @@ -224,7 +220,6 @@ def test_upsert_create(init_db: None, client: TestClient, db_session: Session) - def test_upsert_update(init_db: None, client: TestClient, db_session: Session) -> None: - # Use an existing ID from seed data existing_id = 1 first_name = "Upsert" last_name = "Update" @@ -235,19 +230,42 @@ def test_upsert_update(init_db: None, client: TestClient, db_session: Session) - "country_code": country_code, } - # Verify it exists and is different stmt_check = select(Person).where(Person.id == existing_id) person_before = db_session.execute(stmt_check).scalar_one() assert person_before.first_name != first_name - # Perform PUT (UPSERT) response = client.put(f"/persons/{existing_id}", json=data) assert response.status_code == 204 - # Verify it was updated - db_session.expire_all() # Ensure we get fresh data + db_session.expire_all() person_after = db_session.execute(stmt_check).scalar_one() assert person_after.id == existing_id assert person_after.first_name == first_name assert person_after.last_name == last_name assert person_after.country_code == country_code + + +def test_delete_person(init_db: None, client: TestClient, db_session: Session) -> None: + person_id = 1 + # Verify the person exists before deletion + stmt_check = select(Person).where(Person.id == person_id) + person_before = db_session.execute(stmt_check).scalar_one() + assert person_before is not None + + response = client.delete(f"/persons/{person_id}") + assert response.status_code == 204 + + # Verify the person no longer exists + person_after = db_session.execute(stmt_check).scalar_one_or_none() + assert person_after is None + + # Verify total count decreased by 1 + count: int = db_session.execute(select(func.count()).select_from(Person)).scalar_one() + assert count == NUM_SEED_DATA - 1 + + +def test_delete_person_not_found(init_db: None, client: TestClient, db_session: Session) -> None: + person_id = 999 + response = client.delete(f"/persons/{person_id}") + assert response.status_code == 404 + assert response.json() == {"detail": f"Person {person_id} not found"} diff --git a/tests/unit_tests/services/test_person_service.py b/tests/unit_tests/services/test_person_service.py index 1ce2ac7..b59e315 100644 --- a/tests/unit_tests/services/test_person_service.py +++ b/tests/unit_tests/services/test_person_service.py @@ -26,7 +26,9 @@ def mock_db() -> MagicMock: @pytest.fixture -def person_service(mock_person_repo: MagicMock, mock_country_repo: MagicMock, mock_db: MagicMock) -> PersonService: +def person_service( + mock_person_repo: MagicMock, mock_country_repo: MagicMock, mock_db: MagicMock +) -> PersonService: return PersonService(mock_person_repo, mock_country_repo, mock_db) @@ -41,7 +43,9 @@ def test_get_persons(self, person_service: PersonService, mock_person_repo: Magi mock_person_repo.get_persons.assert_called_once_with(q, sort) assert result == mock_person_repo.get_persons.return_value - def test_get_person_success(self, person_service: PersonService, mock_person_repo: MagicMock) -> None: + def test_get_person_success( + self, person_service: PersonService, mock_person_repo: MagicMock + ) -> None: person_id = 1 expected_person = Person(id=person_id, first_name="John", last_name="Doe") mock_person_repo.get_person.return_value = expected_person @@ -51,7 +55,9 @@ def test_get_person_success(self, person_service: PersonService, mock_person_rep mock_person_repo.get_person.assert_called_once_with(person_id) assert result == expected_person - def test_get_person_not_found(self, person_service: PersonService, mock_person_repo: MagicMock) -> None: + def test_get_person_not_found( + self, person_service: PersonService, mock_person_repo: MagicMock + ) -> None: person_id = 999 mock_person_repo.get_person.return_value = None @@ -62,7 +68,11 @@ def test_get_person_not_found(self, person_service: PersonService, mock_person_r assert str(person_id) in exc.value.message def test_create_person_success( - self, person_service: PersonService, mock_person_repo: MagicMock, mock_country_repo: MagicMock, mock_db: MagicMock + self, + person_service: PersonService, + mock_person_repo: MagicMock, + mock_country_repo: MagicMock, + mock_db: MagicMock, ) -> None: body = PersonCreateSchema(first_name="Jane", last_name="Doe", country_code="US") mock_country_repo.get_country.return_value = MagicMock(spec=Country) @@ -78,7 +88,9 @@ def test_create_person_success( mock_db.refresh.assert_called_once_with(created_person) assert result == created_person - def test_create_person_country_not_found(self, person_service: PersonService, mock_country_repo: MagicMock) -> None: + def test_create_person_country_not_found( + self, person_service: PersonService, mock_country_repo: MagicMock + ) -> None: body = PersonCreateSchema(first_name="Jane", last_name="Doe", country_code="XX") mock_country_repo.get_country.return_value = None @@ -87,18 +99,21 @@ def test_create_person_country_not_found(self, person_service: PersonService, mo mock_country_repo.get_country.assert_called_once_with("XX") - def test_update_person_success( - self, person_service: PersonService, mock_person_repo: MagicMock, mock_country_repo: MagicMock, mock_db: MagicMock + def test_update_or_create_person_success( + self, + person_service: PersonService, + mock_person_repo: MagicMock, + mock_country_repo: MagicMock, + mock_db: MagicMock, ) -> None: person_id = 1 body = PersonCreateSchema(first_name="Jane", last_name="Doe", country_code="US") mock_country_repo.get_country.return_value = MagicMock(spec=Country) - person_service.update_person(person_id, body) + person_service.update_or_create_person(person_id, body) mock_country_repo.get_country.assert_called_once_with("US") mock_person_repo.update_or_create_person.assert_called_once() - # Verify the person object passed to repo has correct ID args, _ = mock_person_repo.update_or_create_person.call_args passed_person = args[0] assert passed_person.id == person_id @@ -106,12 +121,42 @@ def test_update_person_success( mock_db.commit.assert_called_once() - def test_update_person_country_not_found(self, person_service: PersonService, mock_country_repo: MagicMock) -> None: + def test_update_or_create_person_country_not_found( + self, person_service: PersonService, mock_country_repo: MagicMock + ) -> None: person_id = 1 body = PersonCreateSchema(first_name="Jane", last_name="Doe", country_code="XX") mock_country_repo.get_country.return_value = None with pytest.raises(CountryNotFoundError): - person_service.update_person(person_id, body) + person_service.update_or_create_person(person_id, body) mock_country_repo.get_country.assert_called_once_with("XX") + + def test_delete_person_success( + self, + person_service: PersonService, + mock_person_repo: MagicMock, + mock_db: MagicMock, + ) -> None: + person_id = 1 + existing_person = Person(id=person_id, first_name="John", last_name="Doe", country_code="US") + mock_person_repo.get_person.return_value = existing_person + + person_service.delete_person(person_id) + + mock_person_repo.get_person.assert_called_once_with(person_id) + mock_person_repo.delete_person.assert_called_once_with(existing_person) + mock_db.commit.assert_called_once() + + def test_delete_person_not_found( + self, person_service: PersonService, mock_person_repo: MagicMock + ) -> None: + person_id = 999 + mock_person_repo.get_person.return_value = None + + with pytest.raises(PersonNotFoundError) as exc: + person_service.delete_person(person_id) + + assert exc.value.status_code == 404 + assert str(person_id) in exc.value.message From 276c914ebe9e0ef232a848440750396cf00f6023 Mon Sep 17 00:00:00 2001 From: Heinrich Chan Date: Fri, 12 Jun 2026 17:42:49 +0800 Subject: [PATCH 2/2] implement kimi suggestions --- README.md | 2 ++ pyfastapi/controllers/person.py | 4 ++-- pyfastapi/repositories/person.py | 2 +- pyfastapi/schemas/__init__.py | 4 ++-- pyfastapi/schemas/person.py | 10 ---------- 5 files changed, 7 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index d0597ff..4626ec4 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,8 @@ Sample backend API built with **FastAPI**, **SQLAlchemy 2.x**, and **Alembic**. - **No trailing slashes on collection endpoints.** Routes are registered as `/persons`, `/countries`, and `/continents`. Accessing the trailing-slash variant (e.g., `/persons/`) will receive a `307 Temporary Redirect` to the canonical path. - **Sort syntax**: Prefix with `-` for descending (e.g., `-name`), `+` or no prefix for ascending. - **Filter syntax**: Query parameters are mapped to schema fields. String fields use `ILIKE` (`%value%`); others use exact equality. +- **`PUT /persons/{id}` is an upsert.** If the ID exists it is updated; if it does not exist a new person is created with that ID. Returns `204 No Content`. +- **`DELETE /persons/{id}`** removes the person and returns `204 No Content`. Returns `404 Not Found` if the person does not exist. ## Data Model diff --git a/pyfastapi/controllers/person.py b/pyfastapi/controllers/person.py index eb9b2bb..330e853 100644 --- a/pyfastapi/controllers/person.py +++ b/pyfastapi/controllers/person.py @@ -35,7 +35,7 @@ def create_person(body: PersonCreateSchema, service: Annotated[PersonService, De return service.create_person(body) -@router.put("/{id_}") +@router.put("/{id_}", status_code=204) def update_person( id_: Annotated[int, Path(title="person id")], body: PersonCreateSchema, @@ -46,7 +46,7 @@ def update_person( return Response(status_code=204) -@router.delete("/{id_}") +@router.delete("/{id_}", status_code=204) def delete_person( id_: Annotated[int, Path(title="person id")], service: Annotated[PersonService, Depends()] diff --git a/pyfastapi/repositories/person.py b/pyfastapi/repositories/person.py index c0cc855..0c417fd 100644 --- a/pyfastapi/repositories/person.py +++ b/pyfastapi/repositories/person.py @@ -1,6 +1,7 @@ from toolz .functoolz import compose # type: ignore from fastapi_pagination import LimitOffsetPage from fastapi_pagination.ext.sqlalchemy import paginate +from sqlalchemy.dialects.sqlite import insert from sqlalchemy.orm import joinedload from sqlalchemy.sql import select @@ -29,7 +30,6 @@ def create_new_person(self, person: Person) -> Person: return person def update_or_create_person(self, person: Person) -> None: - from sqlalchemy.dialects.sqlite import insert stmt = insert(Person).values( id=person.id, first_name=person.first_name, diff --git a/pyfastapi/schemas/__init__.py b/pyfastapi/schemas/__init__.py index d88641d..fdf0102 100644 --- a/pyfastapi/schemas/__init__.py +++ b/pyfastapi/schemas/__init__.py @@ -1,8 +1,8 @@ from .continent import ContinentSchema from .country import CountrySchema, CountryListSchema, QueryCountrySchema, SortCountryEnum -from .person import PersonListSchema, PersonCreateSchema, PersonUpdateSchema, PersonSchema, QueryPersonSchema, SortPersonEnum +from .person import PersonListSchema, PersonCreateSchema, PersonSchema, QueryPersonSchema, SortPersonEnum __all__ = [ "ContinentSchema", "CountrySchema", "CountryListSchema", "PersonSchema", "PersonListSchema", "PersonCreateSchema", - "PersonUpdateSchema", "QueryCountrySchema", "QueryPersonSchema", "SortCountryEnum", "SortPersonEnum" + "QueryCountrySchema", "QueryPersonSchema", "SortCountryEnum", "SortPersonEnum" ] diff --git a/pyfastapi/schemas/person.py b/pyfastapi/schemas/person.py index 4252154..5793a61 100644 --- a/pyfastapi/schemas/person.py +++ b/pyfastapi/schemas/person.py @@ -27,16 +27,6 @@ class PersonCreateSchema(PersonBaseSchema): country_code: str -class PersonUpdateSchema(BaseModel): - first_name: str | None = None - last_name: str | None = None - country_code: str | None = None - - model_config = { - "from_attributes": True - } - - class QueryPersonSchema(BaseModel): last_name: str | None = None first_name: str | None = None