Feature: issue #385 - Resctructured to follow bigger apps layout #388
Conversation
WalkthroughThe changes reorganize source code imports and references to consistently use the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI App (src.main)
participant Player Router (src.routes.player)
participant Player Service (src.services.player)
participant Player Model (src.models.player)
participant Database
Client->>FastAPI App (src.main): HTTP request (e.g., GET /players/{player_id})
FastAPI App (src.main)->>Player Router: Route dispatch
Player Router->>Player Service: Call service function (e.g., get_player)
Player Service->>Player Model: ORM/database interaction
Player Model->>Database: Query/Update
Database-->>Player Model: Result
Player Model-->>Player Service: Model instance/data
Player Service-->>Player Router: Data/response
Player Router-->>FastAPI App (src.main): Response
FastAPI App (src.main)-->>Client: HTTP response
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)src/routes/player.py (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
app/services/player.py (1)
18-19: Update to absolute imports for service dependencies
Switching imports toapp.models.player.PlayerModelandapp.schemas.player.Playeraligns with the new package layout. Consider aliasing the schema import (e.g.,PlayerSchema) to avoid confusion with the ORM class.app/routes/player.py (4)
35-35: ExtractedPLAYER_TITLEconstantNice DRY improvement. For consistency, consider extracting the squad-number title into its own constant as well.
61-66: Enhance POST/players/to return created resourceCurrently the endpoint returns a 201 with no body. For better REST semantics, you could add a
response_model=PlayerModeland return the newly created object (or include aLocationheader). For example:@router.post( - "/players/", - status_code=status.HTTP_201_CREATED, + "/players/", + status_code=status.HTTP_201_CREATED, + response_model=PlayerModel, ) ... await player.create_async(async_session, player_model) await simple_memory_cache.clear(CACHE_KEY) + return player_model
107-127: Add descriptivedetailon 404 in GET by IDRaising a bare 404 is fine, but including a
detailmessage improves client feedback:if not player_res: - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND) + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Player with id={player_id} not found" + )🧰 Tools
🪛 Ruff (0.11.9)
108-108: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
155-160: Factor out squad-number title into constantFor consistency with
PLAYER_TITLE, consider definingPLAYER_SQUAD_TITLE = "The Squad Number of the Player"at the top and using it here:- squad_number: int = Path(..., title="The Squad Number of the Player"), + squad_number: int = Path(..., title=PLAYER_SQUAD_TITLE),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
storage/players-sqlite3.dbis excluded by!**/*.db
📒 Files selected for processing (7)
Dockerfile(2 hunks)app/main.py(2 hunks)app/routes/health.py(1 hunks)app/routes/player.py(7 hunks)app/schemas/player.py(1 hunks)app/services/player.py(1 hunks)tests/conftest.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/services/player.py (2)
app/models/player.py (1)
PlayerModel(33-63)app/schemas/player.py (1)
Player(13-43)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (10)
tests/conftest.py (1)
4-4: Update import to reflect new project layout
Import path updated to use theapppackage. This aligns with the restructured codebase.app/schemas/player.py (1)
10-10: Use absolute import for Base
Switch to absolute import fromapp.databases.player. EnsureBaseis exported from that module.app/routes/health.py (1)
10-13: Rename router instance and update decorator
Renamedapi_routertorouterand updated the route decorator to use@router.get. This improves naming consistency across all route modules.app/main.py (2)
15-15: Use absolute imports for route modules
Importing fromapp.routesaligns with the restructured package layout.
38-39: Include routers from refactored modules
Updatedinclude_routercalls toplayer.routerandhealth.router. Verify that each router defines its ownprefixandtagsas expected.Dockerfile (1)
81-81: Verify thatapppackage is present for the CMD pathYou updated the CMD to
uvicorn app.main:app. After adjusting the COPY step, ensure that/app/app/main.py(and the rest of theapp/directory) exists in the image so this command resolves correctly.app/routes/player.py (4)
25-27: Imports updated to absoluteapp.namespaceGood alignment with the new project layout—
app.databases.player,app.models.player, andapp.services.playernow reflect the package structure. Just confirm that each module and its__init__.py(or namespace package) are in place.
29-29: Router renamed fromapi_routertorouterThis naming follows FastAPI conventions and matches the changes in
app/main.py. No further action needed.
173-195: PUT/players/{player_id}behavior is correctThe handler checks existence, updates the record, clears the cache, and implicitly returns 204 No Content. This aligns with the API contract.
🧰 Tools
🪛 Ruff (0.11.9)
174-174: Do not perform function call
Bodyin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
175-175: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
207-226: DELETE/players/{player_id}behavior is correctThe handler verifies the entity exists, performs deletion, clears the cache, and returns 204 No Content. Well-implemented.
🧰 Tools
🪛 Ruff (0.11.9)
208-208: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
Dockerfile
Outdated
| # Copy application source code | ||
| COPY app/main.py ./ | ||
| COPY app/databases/ ./databases/ | ||
| COPY app/models/ ./models/ | ||
| COPY app/routes/ ./routes/ | ||
| COPY app/schemas/ ./schemas/ | ||
| COPY app/services/ ./services/ | ||
|
|
There was a problem hiding this comment.
Critical: Dockerfile flattens app package, breaking imports
The current COPY commands break the app package hierarchy by installing subdirectories directly under /app. Since the code uses imports like app.main and app.models, there must be a top-level app directory in the image. Without it, uvicorn app.main:app will fail at runtime.
Please replace the individual COPY instructions with one that preserves the package:
- # Copy application source code
- COPY app/main.py ./
- COPY app/databases/ ./databases/
- COPY app/models/ ./models/
- COPY app/routes/ ./routes/
- COPY app/schemas/ ./schemas/
- COPY app/services/ ./services/
+ # Copy entire app package with correct ownership
+ COPY --chown=fastapi:fastapi app/ ./app/🤖 Prompt for AI Agents
In Dockerfile lines 48 to 55, the current COPY commands flatten the app package
by copying subdirectories directly into the image root, breaking imports like
app.main. To fix this, replace all individual COPY commands with a single COPY
that copies the entire app directory into the image, preserving the app package
hierarchy so that imports remain valid at runtime.
✅ Addressed in commit 589e8e6
app/routes/player.py
Outdated
| players = await simple_memory_cache.get(CACHE_KEY) | ||
| response.headers["X-Cache"] = "HIT" | ||
| if not players: | ||
| players = await player_service.retrieve_all_async(async_session) | ||
| players = await player.retrieve_all_async(async_session) | ||
| await simple_memory_cache.set(CACHE_KEY, players, ttl=CACHE_TTL) | ||
| response.headers["X-Cache"] = "MISS" | ||
| return players |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix cache-miss logic to distinguish empty lists
Using if not players: treats an empty list as a miss. Switch to a None check so that an empty result still counts as a valid cache hit:
- players = await simple_memory_cache.get(CACHE_KEY)
- response.headers["X-Cache"] = "HIT"
- if not players:
+ players = await simple_memory_cache.get(CACHE_KEY)
+ response.headers["X-Cache"] = "HIT"
+ if players is None:
players = await player.retrieve_all_async(async_session)
await simple_memory_cache.set(CACHE_KEY, players, ttl=CACHE_TTL)
response.headers["X-Cache"] = "MISS"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| players = await simple_memory_cache.get(CACHE_KEY) | |
| response.headers["X-Cache"] = "HIT" | |
| if not players: | |
| players = await player_service.retrieve_all_async(async_session) | |
| players = await player.retrieve_all_async(async_session) | |
| await simple_memory_cache.set(CACHE_KEY, players, ttl=CACHE_TTL) | |
| response.headers["X-Cache"] = "MISS" | |
| return players | |
| players = await simple_memory_cache.get(CACHE_KEY) | |
| response.headers["X-Cache"] = "HIT" | |
| if players is None: | |
| players = await player.retrieve_all_async(async_session) | |
| await simple_memory_cache.set(CACHE_KEY, players, ttl=CACHE_TTL) | |
| response.headers["X-Cache"] = "MISS" | |
| return players |
🤖 Prompt for AI Agents
In app/routes/player.py around lines 90 to 96, the cache-miss logic incorrectly
treats an empty list as a cache miss by using 'if not players:'. Change this
condition to explicitly check for None instead, so that an empty list is
considered a valid cache hit and does not trigger a cache refresh.
DimitriZC
force-pushed
the
feature/issue-385-Restructure-to-bigger-apps-layout
branch
from
ef83ac0 to
52b47c7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #388 +/- ##
==========================================
+ Coverage 89.65% 90.90% +1.25%
==========================================
Files 3 4 +1
Lines 116 132 +16
==========================================
+ Hits 104 120 +16
Misses 12 12
🚀 New features to boost your workflow:
|
|
DimitriZC
changed the title
This PR changes the project structure following the FastAPI layout documentation for Bigger Applications (the optional change was not implemented). Black and flake8 verifications were executed, all test passed after the structural changes.
This change is
Summary by CodeRabbit