feat(api): replace integer PK with UUID v4 for Player entity (#66)#492
Merged
nanotaboada merged 4 commits intomasterfrom Feb 20, 2026
Merged
feat(api): replace integer PK with UUID v4 for Player entity (#66)#492nanotaboada merged 4 commits intomasterfrom
nanotaboada merged 4 commits intomasterfrom
Conversation
- Add HyphenatedUUID custom TypeDecorator storing UUIDs as CHAR(36) in
SQLite and returning uuid.UUID objects in Python
- Split PlayerModel into PlayerRequestModel (no id, for POST/PUT) and
PlayerResponseModel (includes id: UUID, for GET/POST responses)
- Update all route path parameters and service signatures from int to UUID
- Add retrieve_by_squad_number_async() service method; expose via
GET /players/squadnumber/{squad_number} route
- Change POST conflict detection from ID lookup to squad_number uniqueness
- Return created Player from create_async() after session refresh
- Add migration_001_starting_eleven.py: recreates players table with UUID
PK and seeds 11 starters with deterministic UUID v5 values
- Add migration_002_substitutes.py: seeds 14 substitutes with deterministic
UUID v5 values (requires migration 001 to run first)
- Update player_stub.py with UUID-based fixtures; unknown_player() now uses
a nil-style UUID instead of an integer
- Update test suite to assert UUID presence and version (UUID v4 for
API-created records); DELETE resolves target UUID via squad number lookup
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
WalkthroughMigrates Player PK from integer to UUID (HyphenatedUUID TypeDecorator), splits PlayerModel into PlayerRequestModel/PlayerResponseModel, updates routes/services/tests to use UUIDs, adds two idempotent tools/ seed scripts, and excludes tools/ from linting/coverage/formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as "API Route"
participant Service as "Player Service"
participant DB as "SQLite DB"
Client->>API: POST /players (PlayerRequestModel)
API->>Service: create_async(async_session, PlayerRequestModel)
Service->>DB: INSERT player (id: uuid4 generated or provided)
DB-->>Service: inserted row (id: UUID)
Service-->>API: return Player (ORM instance)
API-->>Client: 201 Created (PlayerResponseModel with id: UUID)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #492 +/- ##
============================================
+ Coverage 89.65% 100.00% +10.34%
============================================
Files 3 3
Lines 116 110 -6
============================================
+ Hits 104 110 +6
+ Misses 12 0 -12
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
tools/seed_001_starting_eleven.py (1)
281-284: Considerlogging.exceptionfor full traceback on failure.
logging.erroronly logs the stringified exception.logging.exceptionwould additionally include the traceback, which is more helpful when debugging a failed migration.Proposed fix
except sqlite3.Error as exc: conn.rollback() - logger.error("Migration failed: %s", exc) + logger.exception("Migration failed: %s", exc) sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/seed_001_starting_eleven.py` around lines 281 - 284, Replace the logger.error call in the sqlite3 exception handler so the full traceback is recorded: inside the except sqlite3.Error as exc block (where conn.rollback() and sys.exit(1) are called), use logger.exception (or logger.error with exc_info=True) instead of logger.error("Migration failed: %s", exc) so the traceback is included in the logs to aid debugging.models/player_model.py (1)
69-77:idwill serialize after all inherited fields in JSON responses.Because Pydantic v2 orders parent fields before child fields,
idwill appear at the end of the JSON output. If you prefer the conventionalid-first ordering, you could override field ordering or redeclare the fields. This is purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/player_model.py` around lines 69 - 77, PlayerResponseModel currently inherits fields from PlayerRequestModel so the inherited fields appear before id in serialized JSON; to force id to appear first, redeclare the fields in PlayerResponseModel with id as the first attribute (i.e., list id: UUID first and then copy the field declarations from PlayerRequestModel into PlayerResponseModel) or alternatively implement a custom model serializer on PlayerResponseModel that returns a dict with id as the first key; reference PlayerResponseModel, PlayerRequestModel and id when making the change.services/player_service.py (2)
28-28: Decouple logger from Uvicorn — uselogging.getLogger(__name__)
logging.getLogger("uvicorn")borrows Uvicorn's logger, which is only configured when the app runs under Uvicorn. In tests, CLI tools, or other ASGI servers the logger is unconfigured and error messages are silently dropped.♻️ Proposed fix
-logger = logging.getLogger("uvicorn") +logger = logging.getLogger(__name__)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/player_service.py` at line 28, The module-level logger in services/player_service.py is incorrectly tied to Uvicorn via logging.getLogger("uvicorn"); change it to use the module's own logger by calling logging.getLogger(__name__) so logs work in tests/CLI/other ASGI servers; update the logger assignment (the logger variable) accordingly and ensure any subsequent logging calls in this file use that logger.
54-57: Uselogging.exceptioninstead oflogging.errorin allexceptblocks
logging.exception()emits at ERROR level and automatically attaches the current exception's traceback (exc_info=True). Usinglogging.error()discards the stack trace, making DB failures harder to diagnose. Applies to lines 55, 149, and 174 (Ruff TRY400).Additionally, consider using
%slazy formatting instead of f-strings in log calls to avoid unnecessary string interpolation when the log level is filtered out.♻️ Proposed fix (same pattern for all three except blocks)
- logger.error(f"Error trying to create the Player: {error}") + logger.exception("Error trying to create the Player: %s", error)- logger.error(f"Error trying to update the Player: {error}") + logger.exception("Error trying to update the Player: %s", error)- logger.error(f"Error trying to delete the Player: %s", error) + logger.exception("Error trying to delete the Player: %s", error)Also applies to: 148-151, 173-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/player_service.py` around lines 54 - 57, Replace the logger.error calls in the except blocks in services/player_service.py with logger.exception so the traceback is included (e.g., in the except SQLAlchemyError in the Player creation flow and the two other except blocks around lines where rollback is called); also switch from f-string interpolation to lazy %-style formatting (e.g., logger.exception("Error trying to create the Player: %s", error)) while keeping the await async_session.rollback() and return None behavior unchanged so the exception is logged with its stack trace and performance-friendly formatting.tools/seed_002_substitutes.py (2)
274-283: Replace EN DASH (–) with ASCII hyphen-minus (-) in log stringsRuff RUF001 flags lines 274, 283 (and 295 in
_parse_args) for using the Unicode EN DASH character–(U+2013). Log consumers and search tools expect ASCII-.♻️ Proposed fix
- "Migration 002 already applied – all substitute UUIDs present. " + "Migration 002 already applied - all substitute UUIDs present. " ... - logger.info("Migration 002 – Substitutes completed successfully.") + logger.info("Migration 002 - Substitutes completed successfully.")Apply the same change to
_parse_argsat line 295:- description="Migration 002 – seed substitute players with UUID PKs." + description="Migration 002 - seed substitute players with UUID PKs."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/seed_002_substitutes.py` around lines 274 - 283, Replace the Unicode EN DASH (U+2013) with a plain ASCII hyphen-minus (-) in the migration's log messages and the argument parser: update the two logger.info strings that currently contain "Migration 002 – Substitutes completed successfully." and "Migration 002 already applied – all substitute UUIDs present. Skipping." to use "-" instead of "–", and make the same replacement in the `_parse_args` function where an EN DASH is used; this ensures all log and help strings use the ASCII hyphen-minus character.
285-288: Uselogging.exceptioninstead oflogging.errorinsideexceptblock
logging.exception()is the idiomatic choice inside anexceptblock — it records the message at ERROR level AND automatically attaches the current exception's traceback (exc_info=True), which is essential for diagnosing seed failures.♻️ Proposed fix
- logger.error("Migration failed: %s", exc) + logger.exception("Migration failed: %s", exc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/seed_002_substitutes.py` around lines 285 - 288, In the except sqlite3.Error as exc block, replace the call to logger.error("Migration failed: %s", exc) with logger.exception(...) so the traceback is included; specifically, keep conn.rollback() and sys.exit(1) but change logger.error to logger.exception (e.g., logger.exception("Migration failed")) to automatically log exc_info and the traceback for the exception variable exc.schemas/player_schema.py (1)
23-33: Add type hints and Google-style docstrings to TypeDecorator methodsBoth
process_bind_paramandprocess_result_valuelack type annotations and docstrings, violating the project's coding guidelines ("Type hints are required everywhere" and "Use Google-style docstrings for modules, classes, and functions"). Thedialectparameter can be prefixed with_to suppress theARG002/unused-argumentlinter warning while preserving the interface contract.♻️ Proposed fix
- def process_bind_param(self, value, dialect): + def process_bind_param(self, value: UUID | str | None, _dialect: object) -> str | None: + """Convert a Python UUID (or UUID string) to a hyphenated string for storage.""" 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: str | None, _dialect: object) -> UUID | None: + """Convert a stored hyphenated UUID string back to a Python UUID object.""" if value is None: return None return UUID(value)As per coding guidelines: "Type hints are required everywhere — functions, variables, and return types" and "Use Google-style docstrings for modules, classes, and functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/player_schema.py` around lines 23 - 33, Add explicit type hints and Google-style docstrings to the TypeDecorator methods: update process_bind_param(self, value, dialect) to annotate value as Optional[Union[UUID, str]] and return Optional[str], rename the unused dialect parameter to _dialect to avoid ARG002, and add a Google-style docstring describing Args and Returns; similarly annotate process_result_value(self, value, dialect) to accept Optional[str] and return Optional[UUID], rename dialect to _dialect, and add a Google-style docstring describing Args and Returns; ensure you import Optional and Union from typing and UUID from uuid so the annotations resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@routes/player_route.py`:
- Around line 198-203: The route currently discards the boolean result of
player_service.update_async and player_service.delete_async so DB failures that
return False still yield HTTP 204; change the PUT and DELETE handlers to capture
the return value from update_async(player_id, player_model) and
delete_async(player_id), and if the result is False raise an HTTPException with
a 500 (or other appropriate) status_code and a brief error message; keep the
existing retrieve_by_id_async check and cache-clear logic
(simple_memory_cache.clear(CACHE_KEY)) but only clear the cache after a
successful update/delete. Ensure you reference the exact symbols update_async,
delete_async, retrieve_by_id_async, simple_memory_cache.clear, and
HTTPException/status to locate and implement the checks.
- Around line 66-73: player_service.create_async can return None on DB failure;
modify the route to check the returned value from player = await
player_service.create_async(async_session, player_model) and if player is None
raise a controlled HTTPException (e.g., status.HTTP_500_INTERNAL_SERVER_ERROR
with a descriptive message or choose status.HTTP_409_CONFLICT if you prefer to
indicate duplicate) instead of returning None; only call await
simple_memory_cache.clear(CACHE_KEY) after you confirm player is not None so the
cache is cleared only on successful creation.
In `@tests/test_main.py`:
- Around line 67-73: The test
test_request_get_players_response_body_each_player_has_uuid is currently using
all(UUID(player["id"]) ...) which is misleading because UUID(...) either returns
a truthy object or raises; change the assertion to explicitly validate IDs
instead of relying on truthiness: implement a small validator that tries to
construct UUID(player["id"]) and returns True on success and False on
ValueError, then assert all(validator(player["id"]) for player in players).
Update the assertion in the test (referencing the players variable and PATH) to
use that validator so malformed IDs cause a failing assertion rather than an
exception.
- Around line 230-238:
test_request_delete_player_id_existing_response_status_no_content relies on
state created by another test (uses nonexistent_player().squad_number and
assumes a player exists), causing failures when run in isolation; modify this
test so its Arrange phase creates the player it will delete (call the POST
endpoint via client to create a player and capture the returned id) or use a
dedicated fixture that seeds and tears down the player, then perform the DELETE
against that id (refer to the test function name
test_request_delete_player_id_existing_response_status_no_content, the helper
nonexistent_player, and PATH to locate the code to change).
In `@tools/seed_001_starting_eleven.py`:
- Around line 46-190: In the STARTING_ELEVEN seed data the "middleName" for
Emiliano Martínez (id "b04965e6-a9bb-591f-8f8a-1adcb2c8dc39") is an empty string
while all other players without a middle name use None; change that entry's
"middleName" from "" to None to standardize representation and avoid downstream
checks that rely on is None.
---
Nitpick comments:
In `@models/player_model.py`:
- Around line 69-77: PlayerResponseModel currently inherits fields from
PlayerRequestModel so the inherited fields appear before id in serialized JSON;
to force id to appear first, redeclare the fields in PlayerResponseModel with id
as the first attribute (i.e., list id: UUID first and then copy the field
declarations from PlayerRequestModel into PlayerResponseModel) or alternatively
implement a custom model serializer on PlayerResponseModel that returns a dict
with id as the first key; reference PlayerResponseModel, PlayerRequestModel and
id when making the change.
In `@schemas/player_schema.py`:
- Around line 23-33: Add explicit type hints and Google-style docstrings to the
TypeDecorator methods: update process_bind_param(self, value, dialect) to
annotate value as Optional[Union[UUID, str]] and return Optional[str], rename
the unused dialect parameter to _dialect to avoid ARG002, and add a Google-style
docstring describing Args and Returns; similarly annotate
process_result_value(self, value, dialect) to accept Optional[str] and return
Optional[UUID], rename dialect to _dialect, and add a Google-style docstring
describing Args and Returns; ensure you import Optional and Union from typing
and UUID from uuid so the annotations resolve.
In `@services/player_service.py`:
- Line 28: The module-level logger in services/player_service.py is incorrectly
tied to Uvicorn via logging.getLogger("uvicorn"); change it to use the module's
own logger by calling logging.getLogger(__name__) so logs work in
tests/CLI/other ASGI servers; update the logger assignment (the logger variable)
accordingly and ensure any subsequent logging calls in this file use that
logger.
- Around line 54-57: Replace the logger.error calls in the except blocks in
services/player_service.py with logger.exception so the traceback is included
(e.g., in the except SQLAlchemyError in the Player creation flow and the two
other except blocks around lines where rollback is called); also switch from
f-string interpolation to lazy %-style formatting (e.g., logger.exception("Error
trying to create the Player: %s", error)) while keeping the await
async_session.rollback() and return None behavior unchanged so the exception is
logged with its stack trace and performance-friendly formatting.
In `@tools/seed_001_starting_eleven.py`:
- Around line 281-284: Replace the logger.error call in the sqlite3 exception
handler so the full traceback is recorded: inside the except sqlite3.Error as
exc block (where conn.rollback() and sys.exit(1) are called), use
logger.exception (or logger.error with exc_info=True) instead of
logger.error("Migration failed: %s", exc) so the traceback is included in the
logs to aid debugging.
In `@tools/seed_002_substitutes.py`:
- Around line 274-283: Replace the Unicode EN DASH (U+2013) with a plain ASCII
hyphen-minus (-) in the migration's log messages and the argument parser: update
the two logger.info strings that currently contain "Migration 002 – Substitutes
completed successfully." and "Migration 002 already applied – all substitute
UUIDs present. Skipping." to use "-" instead of "–", and make the same
replacement in the `_parse_args` function where an EN DASH is used; this ensures
all log and help strings use the ASCII hyphen-minus character.
- Around line 285-288: In the except sqlite3.Error as exc block, replace the
call to logger.error("Migration failed: %s", exc) with logger.exception(...) so
the traceback is included; specifically, keep conn.rollback() and sys.exit(1)
but change logger.error to logger.exception (e.g., logger.exception("Migration
failed")) to automatically log exc_info and the traceback for the exception
variable exc.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/player_service.py (1)
135-145:⚠️ Potential issue | 🟡 Minor
update_asyncassumesplayeris notNone— potentialAttributeError.
async_session.get(Player, player_id)at line 135 can returnNone. While the route layer pre-checks existence, there's a TOCTOU gap: a concurrent DELETE between the route's check and this call would cause an unhandledAttributeErroron line 136. The same issue exists indelete_asyncat line 169–170.Adding a
Noneguard keeps the service layer self-contained:🛡️ Proposed fix for update_async
player = await async_session.get(Player, player_id) + if player is None: + return False player.first_name = player_model.first_name🛡️ Proposed fix for delete_async
player = await async_session.get(Player, player_id) + if player is None: + return False await async_session.delete(player)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/player_service.py` around lines 135 - 145, In update_async and delete_async, async_session.get(Player, player_id) can return None causing AttributeError if a concurrent delete occurred; add a None guard after the get: check if player is None and handle it by raising the same NotFound (or custom) exception the route expects (or return a suitable error value) so the service layer is self-contained. Locate the async_session.get call in update_async and delete_async, perform an if player is None: raise NotFoundError("Player not found") (or propagate the existing exception type used elsewhere) before accessing player attributes or calling session.delete. Ensure the exception type/message matches the rest of the service/route error handling.
🧹 Nitpick comments (4)
services/player_service.py (1)
64-78: Missing return type annotation onretrieve_all_async.All other service functions have return type annotations. This one should too, per the coding guidelines requiring type hints everywhere.
♻️ Proposed fix
-async def retrieve_all_async(async_session: AsyncSession): +async def retrieve_all_async(async_session: AsyncSession) -> list[Player]:As per coding guidelines: "Type hints are required everywhere — functions, variables, and return types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/player_service.py` around lines 64 - 78, The function retrieve_all_async is missing a return type annotation; update its signature to include an explicit return type (for example -> list[Player] or -> Sequence[Player]) to match other service functions and the project's typing rules, and add any necessary typing imports (e.g., from typing import List or Sequence) at the top of the module; keep the function body unchanged and ensure the declared return type matches players = result.scalars().all() which yields a list of Player instances.models/player_model.py (1)
69-100:PlayerResponseModelduplicates all fields fromPlayerRequestModel— consider inheritance.
PlayerResponseModelrepeats every field definition fromPlayerRequestModel, only addingid: UUID. Inheriting fromPlayerRequestModelwould eliminate the duplication and ensure both models stay in sync.♻️ Proposed refactor
-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 for API-created - records, UUID v5 for migration-seeded records). - first_name (str): The first name of the Player. - ... - """ - - 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] +class PlayerResponseModel(PlayerRequestModel): + """ + Pydantic model representing a football Player with a UUID for Retrieve operations. + + Inherits all fields from PlayerRequestModel and adds: + id (UUID): The unique identifier for the Player (UUID v4 for API-created + records, UUID v5 for migration-seeded records). + """ + + id: UUID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/player_model.py` around lines 69 - 100, PlayerResponseModel duplicates all fields from PlayerRequestModel; change PlayerResponseModel to inherit from PlayerRequestModel (instead of MainModel) and only declare the additional id: UUID field there (e.g., class PlayerResponseModel(PlayerRequestModel): id: UUID), keeping imports and any validators from MainModel in PlayerRequestModel so behavior remains the same; remove the duplicated field declarations from PlayerResponseModel so the two stay in sync.tools/seed_001_starting_eleven.py (1)
245-286: Connection and pragmas are set up outside thetryblock.If the PRAGMA statements (lines 252–253) or any unexpected exception inside
try(non-sqlite3.Error) are raised,conn.close()infinallywon't execute for the PRAGMA case sinceconnis assigned but thetryhasn't been entered yet. Consider wrapping the connection setup itself in thetry/finally, or using a context manager.This is a minor robustness concern for a CLI tool, so feel free to defer.
♻️ Suggested improvement
conn = sqlite3.connect(db_path) - conn.execute("PRAGMA journal_mode=WAL") - conn.execute("PRAGMA foreign_keys=ON") - try: + conn.execute("PRAGMA journal_mode=WAL") + conn.execute("PRAGMA foreign_keys=ON") + id_type = _id_column_type(conn)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/seed_001_starting_eleven.py` around lines 245 - 286, The connection and PRAGMA calls are created outside the try/finally, so if those PRAGMA calls raise an exception the finally block that calls conn.close() may not run; wrap the sqlite3.connect(...) and the subsequent PRAGMA statements inside the existing try block (or use a context manager such as "with sqlite3.connect(db_path) as conn:"), then run _id_column_type(conn), _backup(db_path), CREATE_TABLE_SQL execution, INSERT_SQL executemany with STARTING_ELEVEN, commit/rollback logic and the existing exception handling inside that try so conn is always closed; keep the logic and logging in run() and preserve references to _id_column_type, _already_migrated, _backup, CREATE_TABLE_SQL, INSERT_SQL, and STARTING_ELEVEN.tests/test_main.py (1)
242-252: DELETE test now self-creates its target — good isolation improvement.This addresses the prior review's concern about inter-test ordering dependency. One minor gap: the POST at line 246 isn't asserted for success. If the POST unexpectedly fails (e.g., 409 because a prior test run left stale data), the subsequent GET may still succeed, masking the issue. Consider adding an assertion on the create step.
♻️ Proposed tweak
player = nonexistent_player() - client.post(PATH, json=player.__dict__) + create_response = client.post(PATH, json=player.__dict__) + assert create_response.status_code in (201, 409) # created or already exists lookup_response = client.get(PATH + "squadnumber/" + str(player.squad_number)) + assert lookup_response.status_code == 200 player_id = lookup_response.json()["id"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_main.py` around lines 242 - 252, The test creates a player but never asserts the POST succeeded; capture the POST result (replace the bare client.post(...) with create_response = client.post(PATH, json=player.__dict__)) and assert the create_response returned a successful status (e.g., assert create_response.status_code in (200, 201) or assert create_response.status_code == 201) before proceeding to the GET/DELETE steps so a failed create won't silently mask test failures in test_request_delete_player_id_existing_response_status_no_content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@models/player_model.py`:
- Around line 57-66: PlayerRequestModel and PlayerResponseModel declare several
Optional fields (middle_name, date_of_birth, abbr_position, team, league,
starting11) but lack explicit defaults, which makes them required under Pydantic
v2; update each of those Optional fields in both PlayerRequestModel and
PlayerResponseModel to include a default of = None so they become truly
optional/omissible in incoming JSON (e.g., change middle_name: Optional[str] to
middle_name: Optional[str] = None, and do the same for date_of_birth,
abbr_position, team, league, and starting11).
---
Outside diff comments:
In `@services/player_service.py`:
- Around line 135-145: In update_async and delete_async,
async_session.get(Player, player_id) can return None causing AttributeError if a
concurrent delete occurred; add a None guard after the get: check if player is
None and handle it by raising the same NotFound (or custom) exception the route
expects (or return a suitable error value) so the service layer is
self-contained. Locate the async_session.get call in update_async and
delete_async, perform an if player is None: raise NotFoundError("Player not
found") (or propagate the existing exception type used elsewhere) before
accessing player attributes or calling session.delete. Ensure the exception
type/message matches the rest of the service/route error handling.
---
Nitpick comments:
In `@models/player_model.py`:
- Around line 69-100: PlayerResponseModel duplicates all fields from
PlayerRequestModel; change PlayerResponseModel to inherit from
PlayerRequestModel (instead of MainModel) and only declare the additional id:
UUID field there (e.g., class PlayerResponseModel(PlayerRequestModel): id:
UUID), keeping imports and any validators from MainModel in PlayerRequestModel
so behavior remains the same; remove the duplicated field declarations from
PlayerResponseModel so the two stay in sync.
In `@services/player_service.py`:
- Around line 64-78: The function retrieve_all_async is missing a return type
annotation; update its signature to include an explicit return type (for example
-> list[Player] or -> Sequence[Player]) to match other service functions and the
project's typing rules, and add any necessary typing imports (e.g., from typing
import List or Sequence) at the top of the module; keep the function body
unchanged and ensure the declared return type matches players =
result.scalars().all() which yields a list of Player instances.
In `@tests/test_main.py`:
- Around line 242-252: The test creates a player but never asserts the POST
succeeded; capture the POST result (replace the bare client.post(...) with
create_response = client.post(PATH, json=player.__dict__)) and assert the
create_response returned a successful status (e.g., assert
create_response.status_code in (200, 201) or assert create_response.status_code
== 201) before proceeding to the GET/DELETE steps so a failed create won't
silently mask test failures in
test_request_delete_player_id_existing_response_status_no_content.
In `@tools/seed_001_starting_eleven.py`:
- Around line 245-286: The connection and PRAGMA calls are created outside the
try/finally, so if those PRAGMA calls raise an exception the finally block that
calls conn.close() may not run; wrap the sqlite3.connect(...) and the subsequent
PRAGMA statements inside the existing try block (or use a context manager such
as "with sqlite3.connect(db_path) as conn:"), then run _id_column_type(conn),
_backup(db_path), CREATE_TABLE_SQL execution, INSERT_SQL executemany with
STARTING_ELEVEN, commit/rollback logic and the existing exception handling
inside that try so conn is always closed; keep the logic and logging in run()
and preserve references to _id_column_type, _already_migrated, _backup,
CREATE_TABLE_SQL, INSERT_SQL, and STARTING_ELEVEN.
- 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
- Remove inline comments from .coveragerc exclude_lines (configparser
treats them as part of the regex, preventing pragma: no cover from
matching)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
nanotaboada
force-pushed
the
feat/uuid-primary-key
branch
from
e98d6b2 to
1bd2e6b
Compare
- Add = None defaults to Optional fields in PlayerRequestModel and PlayerResponseModel (Pydantic v2 requires explicit defaults to make fields omissible; Optional[str] alone only allows None when provided) - Add # pragma: no cover to SQLAlchemyError handlers in player_service.py (DB failure paths that require a live SQLAlchemy error to exercise) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/player_service.py (1)
64-78: 🛠️ Refactor suggestion | 🟠 MajorMissing return type hint on
retrieve_all_async.All other service functions have return type annotations. This one should too.
♻️ Proposed fix
-async def retrieve_all_async(async_session: AsyncSession): +async def retrieve_all_async(async_session: AsyncSession) -> list[Player]:As per coding guidelines: "Type hints are required everywhere — functions, variables, and return types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/player_service.py` around lines 64 - 78, Add a return type annotation to retrieve_all_async so it matches other service functions — e.g., change the signature to async def retrieve_all_async(async_session: AsyncSession) -> list[Player]: and ensure you import the appropriate typing (list or List) if not already present; this uses the existing AsyncSession parameter and Player model referenced in the function.tests/test_main.py (1)
242-252:⚠️ Potential issue | 🟡 MinorAssert the player creation to eliminate test order dependency.
The POST on line 246 can silently fail with 409 Conflict if
nonexistent_player()was already created by an earlier test. The test still passes because the lookup finds the existing player, but this creates fragile order-dependent behavior.♻️ Proposed fix — assert creation and extract UUID directly from response
player = nonexistent_player() - client.post(PATH, json=player.__dict__) - lookup_response = client.get(PATH + "squadnumber/" + str(player.squad_number)) + create_response = client.post(PATH, json=player.__dict__) + assert create_response.status_code == 201, "Player must be created for DELETE test" + player_id = create_response.json()["id"] - player_id = lookup_response.json()["id"]The POST endpoint returns the created player with its generated UUID in the response, so this fix also eliminates the redundant lookup call and makes the test self-contained.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_main.py` around lines 242 - 252, Change the test_request_delete_player_id_existing_response_status_no_content to assert that the initial POST (client.post(PATH, json=player.__dict__)) succeeded and to extract the created player's UUID from that POST response instead of doing a separate lookup; specifically, call client.post and assert its status_code is 201 (or the expected create status), get player_id from post_response.json()["id"], then call client.delete(PATH + str(player_id)) and assert 204—this removes the fragile dependency on nonexistent_player() already existing and eliminates the extra client.get lookup.
🧹 Nitpick comments (3)
models/player_model.py (1)
69-100:PlayerResponseModelduplicates all fields fromPlayerRequestModel— consider inheritance.
PlayerResponseModelrepeats every field fromPlayerRequestModeland only addsid: UUID. This violates DRY and means any field change must be applied in two places.♻️ Proposed refactor
-class PlayerResponseModel(MainModel): - """ - Pydantic model representing a football Player with a UUID for Retrieve operations. - ... - """ - - 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] +class PlayerResponseModel(PlayerRequestModel): + """ + Pydantic model representing a football Player with a UUID for Retrieve operations. + + Inherits all fields from PlayerRequestModel and adds: + id (UUID): The unique identifier for the Player (UUID v4 for API-created + records, UUID v5 for migration-seeded records). + """ + + id: UUID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/player_model.py` around lines 69 - 100, PlayerResponseModel duplicates all fields from PlayerRequestModel; refactor PlayerResponseModel to inherit from PlayerRequestModel (and MainModel if needed) and only declare the additional id: UUID field, removing the repeated field declarations so future changes to PlayerRequestModel propagate automatically; ensure the class definition still subclasses MainModel (or preserve any validators) by using multiple inheritance or adjusting base class order accordingly and run tests to confirm behavior.services/player_service.py (2)
158-177:delete_asyncsimilarly lacks a null guard beforeasync_session.delete(player).If
playerisNone,async_session.delete(None)will raise. Add a guard consistent with the suggested fix forupdate_async.♻️ Proposed fix
player = await async_session.get(Player, player_id) + if player is None: + return False await async_session.delete(player)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/player_service.py` around lines 158 - 177, The delete_async function calls async_session.delete(player) without checking if player is None; add a null guard like in update_async: fetch player via async_session.get(Player, player_id), if player is None log a not-found message (using logger) and return False (or appropriate not-found result) without calling async_session.delete or commit, otherwise proceed to delete, commit, and handle SQLAlchemyError with rollback; reference the delete_async function, Player model, async_session, logger, and SQLAlchemyError when locating where to add the guard.
135-152:update_asyncwill raiseAttributeErrorifplayer_iddoesn't exist — no null guard.
async_session.get(Player, player_id)can returnNone. Line 136 immediately accessesplayer.first_namewithout a check. The route currently guards this, but the service is unsafe if called from any other context. Same pattern applies todelete_async(line 169-170).♻️ Proposed fix
player = await async_session.get(Player, player_id) + if player is None: + return False player.first_name = player_model.first_name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/player_service.py` around lines 135 - 152, The update_async flow calls async_session.get(Player, player_id) and immediately dereferences player attributes, which will raise AttributeError if no row is found; add a null guard after the call (e.g., if not player) and handle the missing player consistently (return False or raise a NotFoundError), then only set player.first_name/... and commit; apply the same pattern to delete_async where async_session.get(Player, player_id) is used (check for None before deleting) so both update_async and delete_async safely handle nonexistent player IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 74-78: Add a bullet under the "Fixed" section of the changelog
documenting that create_async() now returns the created Player instance after
performing a session refresh (previous behavior returned nothing), so readers
know this behavioral fix; mention it as a succinct entry like "create_async()
now returns the created Player after session refresh (was previously not
returning the created object)".
- Around line 47-52: Add an "Added" entry under the [Unreleased] section
documenting the new public API: the GET /players/squadnumber/{squad_number}
endpoint and its implementation function retrieve_by_squad_number_async(), e.g.
a short bullet stating the route and that it returns a player by squad number;
ensure the entry is placed in the "Added" subsection and follows the project's
CHANGELOG style/format.
---
Outside diff comments:
In `@services/player_service.py`:
- Around line 64-78: Add a return type annotation to retrieve_all_async so it
matches other service functions — e.g., change the signature to async def
retrieve_all_async(async_session: AsyncSession) -> list[Player]: and ensure you
import the appropriate typing (list or List) if not already present; this uses
the existing AsyncSession parameter and Player model referenced in the function.
In `@tests/test_main.py`:
- Around line 242-252: Change the
test_request_delete_player_id_existing_response_status_no_content to assert that
the initial POST (client.post(PATH, json=player.__dict__)) succeeded and to
extract the created player's UUID from that POST response instead of doing a
separate lookup; specifically, call client.post and assert its status_code is
201 (or the expected create status), get player_id from
post_response.json()["id"], then call client.delete(PATH + str(player_id)) and
assert 204—this removes the fragile dependency on nonexistent_player() already
existing and eliminates the extra client.get lookup.
---
Duplicate comments:
In `@models/player_model.py`:
- Around line 57-66: PlayerRequestModel and PlayerResponseModel declare several
Optional[...] fields without default values which makes them required under
Pydantic v2; update the field definitions for middle_name, date_of_birth,
abbr_position, team, league, and starting11 in both models by assigning a
default of None (e.g., middle_name: Optional[str] = None) so they become truly
optional and clients can omit them or send null.
---
Nitpick comments:
In `@models/player_model.py`:
- Around line 69-100: PlayerResponseModel duplicates all fields from
PlayerRequestModel; refactor PlayerResponseModel to inherit from
PlayerRequestModel (and MainModel if needed) and only declare the additional id:
UUID field, removing the repeated field declarations so future changes to
PlayerRequestModel propagate automatically; ensure the class definition still
subclasses MainModel (or preserve any validators) by using multiple inheritance
or adjusting base class order accordingly and run tests to confirm behavior.
In `@services/player_service.py`:
- Around line 158-177: The delete_async function calls
async_session.delete(player) without checking if player is None; add a null
guard like in update_async: fetch player via async_session.get(Player,
player_id), if player is None log a not-found message (using logger) and return
False (or appropriate not-found result) without calling async_session.delete or
commit, otherwise proceed to delete, commit, and handle SQLAlchemyError with
rollback; reference the delete_async function, Player model, async_session,
logger, and SQLAlchemyError when locating where to add the guard.
- Around line 135-152: The update_async flow calls async_session.get(Player,
player_id) and immediately dereferences player attributes, which will raise
AttributeError if no row is found; add a null guard after the call (e.g., if not
player) and handle the missing player consistently (return False or raise a
NotFoundError), then only set player.first_name/... and commit; apply the same
pattern to delete_async where async_session.get(Player, player_id) is used
(check for None before deleting) so both update_async and delete_async safely
handle nonexistent player IDs.
- Add -> List[Player] return type to retrieve_all_async (List imported) - Add None guard to update_async to prevent AttributeError on missing player - Move PRAGMA calls inside try block in seed_001 so conn.close() always runs - Remove duplicate client.delete() call in DELETE test (caused 404 assertion) - Re-seed storage/players-sqlite3.db with hyphenated UUIDs (previous commit stored UUIDs without hyphens, breaking HyphenatedUUID lookups) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores