fix(api): address code review findings (#66)

- Raise HTTP 500 if create_async returns None (DB failure on POST)
- Capture bool result from update_async/delete_async; raise HTTP 500
  on False; only clear cache after confirmed success
- Fix DELETE test to self-create player in Arrange (no inter-test dep)
- Replace all(UUID(...)) with explicit _is_valid_uuid() validator
- Redeclare PlayerResponseModel fields with id first (controls JSON order)
- Add type hints and Google-style docstrings to HyphenatedUUID methods;
  rename unused dialect params to _dialect
- Align service logger with main.py: getLogger("uvicorn.error") with
  reference comment (Kludex/uvicorn#562)
- Replace logger.error(f"...") with logger.exception("...: %s", error)
  in all three SQLAlchemyError handlers
- Replace EN dashes with ASCII hyphens in seed_002 log/argparse strings
- Replace logger.error with logger.exception in seed_001/seed_002
  sqlite3.Error handlers

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: nanotaboada

models/player_model.py

@@ -66,12 +66,35 @@ class PlayerRequestModel(MainModel):
     starting11: Optional[bool]
 
 
-class PlayerResponseModel(PlayerRequestModel):
+class PlayerResponseModel(MainModel):
     """
     Pydantic model representing a football Player with a UUID for Retrieve operations.
 
     Attributes:
-        id (UUID): The unique identifier for the Player (UUID v4).
+        id (UUID): The unique identifier for the Player (UUID v4 for API-created
+            records, UUID v5 for migration-seeded records).
+        first_name (str): The first name of the Player.
+        middle_name (Optional[str]): The middle name of the Player, if any.
+        last_name (str): The last name of the Player.
+        date_of_birth (Optional[str]): The date of birth of the Player, if provided.
+        squad_number (int): The unique squad number assigned to the Player.
+        position (str): The playing position of the Player.
+        abbr_position (Optional[str]): The abbreviated form of the Player's position,
+        if any.
+        team (Optional[str]): The team to which the Player belongs, if any.
+        league (Optional[str]): The league where the team plays, if any.
+        starting11 (Optional[bool]): Indicates if the Player is in the starting 11,
+        if provided.
     """
 
     id: UUID
+    first_name: str
+    middle_name: Optional[str]
+    last_name: str
+    date_of_birth: Optional[str]
+    squad_number: int
+    position: str
+    abbr_position: Optional[str]
+    team: Optional[str]
+    league: Optional[str]
+    starting11: Optional[bool]

routes/player_route.py

@@ -69,6 +69,11 @@ async def post_async(
     if existing:
         raise HTTPException(status_code=status.HTTP_409_CONFLICT)
     player = await player_service.create_async(async_session, player_model)
+    if player is None:
+        raise HTTPException(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail="Failed to create the Player due to a database error.",
+        )
     await simple_memory_cache.clear(CACHE_KEY)
     return player
 
@@ -198,7 +203,12 @@ async def put_async(
     player = await player_service.retrieve_by_id_async(async_session, player_id)
     if not player:
         raise HTTPException(status_code=status.HTTP_404_NOT_FOUND)
-    await player_service.update_async(async_session, player_id, player_model)
+    updated = await player_service.update_async(async_session, player_id, player_model)
+    if not updated:
+        raise HTTPException(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail="Failed to update the Player due to a database error.",
+        )
     await simple_memory_cache.clear(CACHE_KEY)
 
 
@@ -229,5 +239,10 @@ async def delete_async(
     player = await player_service.retrieve_by_id_async(async_session, player_id)
     if not player:
         raise HTTPException(status_code=status.HTTP_404_NOT_FOUND)
-    await player_service.delete_async(async_session, player_id)
+    deleted = await player_service.delete_async(async_session, player_id)
+    if not deleted:
+        raise HTTPException(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail="Failed to delete the Player due to a database error.",
+        )
     await simple_memory_cache.clear(CACHE_KEY)

schemas/player_schema.py

@@ -6,6 +6,7 @@
 Used for async database CRUD operations in the application.
 """
 
+from typing import Optional, Union
 from uuid import UUID, uuid4
 from sqlalchemy import Column, String, Integer, Boolean, TypeDecorator
 from databases.player_database import Base
@@ -20,14 +21,35 @@ class HyphenatedUUID(TypeDecorator):
     impl = String(36)
     cache_ok = True
 
-    def process_bind_param(self, value, dialect):
+    def process_bind_param(
+        self, value: Optional[Union[UUID, str]], _dialect
+    ) -> Optional[str]:
+        """Convert a UUID or string to a hyphenated UUID string for storage.
+
+        Args:
+            value: A UUID object, a UUID string, or None.
+            _dialect: The SQLAlchemy dialect (unused).
+
+        Returns:
+            A hyphenated UUID string (e.g. '550e8400-e29b-41d4-a716-446655440000'),
+            or None if value is None.
+        """
         if value is None:
             return None
         if isinstance(value, UUID):
             return str(value)
         return str(UUID(str(value)))
 
-    def process_result_value(self, value, dialect):
+    def process_result_value(self, value: Optional[str], _dialect) -> Optional[UUID]:
+        """Convert a stored hyphenated UUID string back to a Python UUID object.
+
+        Args:
+            value: A hyphenated UUID string, or None.
+            _dialect: The SQLAlchemy dialect (unused).
+
+        Returns:
+            A Python UUID object, or None if value is None.
+        """
         if value is None:
             return None
         return UUID(value)

services/player_service.py

@@ -25,7 +25,8 @@
 from models.player_model import PlayerRequestModel
 from schemas.player_schema import Player
 
-logger = logging.getLogger("uvicorn")
+# https://github.com/encode/uvicorn/issues/562
+logger = logging.getLogger("uvicorn.error")
 
 # Create -----------------------------------------------------------------------
 
@@ -52,7 +53,7 @@ async def create_async(
         await async_session.refresh(player)
         return player
     except SQLAlchemyError as error:
-        logger.error(f"Error trying to create the Player: {error}")
+        logger.exception("Error trying to create the Player: %s", error)
         await async_session.rollback()
         return None
 
@@ -146,7 +147,7 @@ async def update_async(
         await async_session.commit()
         return True
     except SQLAlchemyError as error:
-        logger.error(f"Error trying to update the Player: {error}")
+        logger.exception("Error trying to update the Player: %s", error)
         await async_session.rollback()
         return False
 
@@ -171,6 +172,6 @@ async def delete_async(async_session: AsyncSession, player_id: UUID) -> bool:
         await async_session.commit()
         return True
     except SQLAlchemyError as error:
-        logger.error(f"Error trying to delete the Player: {error}")
+        logger.exception("Error trying to delete the Player: %s", error)
         await async_session.rollback()
         return False

tests/test_main.py

@@ -22,6 +22,16 @@
 
 PATH = "/players/"
 
+
+def _is_valid_uuid(value: str) -> bool:
+    """Return True if value is a well-formed UUID string, False otherwise."""
+    try:
+        UUID(value)
+        return True
+    except ValueError:
+        return False
+
+
 # GET /health/ -----------------------------------------------------------------
 
 
@@ -70,7 +80,9 @@ def test_request_get_players_response_body_each_player_has_uuid(client):
     response = client.get(PATH)
     # Assert
     players = response.json()
-    assert all(UUID(player["id"]) for player in players)  # UUID v5 (migration-seeded)
+    assert all(
+        _is_valid_uuid(player["id"]) for player in players
+    )  # UUID v5 (migration-seeded)
 
 
 # GET /players/{player_id} -----------------------------------------------------
@@ -229,9 +241,10 @@ def test_request_delete_player_id_unknown_response_status_not_found(client):
 
 def test_request_delete_player_id_existing_response_status_no_content(client):
     """DELETE /players/{player_id} with existing UUID returns 204 No Content"""
-    # Arrange
-    squad_number = nonexistent_player().squad_number  # created by POST test
-    lookup_response = client.get(PATH + "squadnumber/" + str(squad_number))
+    # Arrange — create the player to be deleted, then resolve its UUID
+    player = nonexistent_player()
+    client.post(PATH, json=player.__dict__)
+    lookup_response = client.get(PATH + "squadnumber/" + str(player.squad_number))
     player_id = lookup_response.json()["id"]
     # Act
     response = client.delete(PATH + str(player_id))

tools/seed_001_starting_eleven.py

@@ -47,7 +47,7 @@
     {
         "id": "b04965e6-a9bb-591f-8f8a-1adcb2c8dc39",
         "firstName": "Emiliano",
-        "middleName": "",
+        "middleName": None,
         "lastName": "Martínez",
         "dateOfBirth": "1992-09-02T00:00:00.000Z",
         "squadNumber": 23,
@@ -280,7 +280,7 @@ def run(db_path: Path) -> None:
 
     except sqlite3.Error as exc:
         conn.rollback()
-        logger.error("Migration failed: %s", exc)
+        logger.exception("Migration failed: %s", exc)
         sys.exit(1)
     finally:
         conn.close()

tools/seed_002_substitutes.py

@@ -271,7 +271,7 @@ def run(db_path: Path) -> None:
 
         if _already_migrated(conn):
             logger.info(
-                "Migration 002 already applied – all substitute UUIDs present. "
+                "Migration 002 already applied - all substitute UUIDs present. "
                 "Skipping."
             )
             return
@@ -280,19 +280,19 @@ def run(db_path: Path) -> None:
         conn.executemany(INSERT_SQL, SUBSTITUTES)
         conn.commit()
 
-        logger.info("Migration 002 – Substitutes completed successfully.")
+        logger.info("Migration 002 - Substitutes completed successfully.")
 
     except sqlite3.Error as exc:
         conn.rollback()
-        logger.error("Migration failed: %s", exc)
+        logger.exception("Migration failed: %s", exc)
         sys.exit(1)
     finally:
         conn.close()
 
 
 def _parse_args() -> argparse.Namespace:
     parser = argparse.ArgumentParser(
-        description="Migration 002 – seed substitute players with UUID PKs."
+        description="Migration 002 - seed substitute players with UUID PKs."
     )
     parser.add_argument(
         "--db-path",