[SUGGESTION] ENH: streamline controller dependency injection in routes and tests #66
Conversation
- Create FastAPI dependency providers for all controllers - Implement singleton pattern using lru_cache for thread-safety - Add type aliases (RocketControllerDep, MotorControllerDep, etc.) - Improves performance by avoiding controller re-instantiation on every request References: - FastAPI Dependencies: https://fastapi.tiangolo.com/tutorial/dependencies/
- Replace manual FlightController instantiation with FlightControllerDep - Apply DI pattern across all flight endpoints (CRUD + simulate + binary) - Maintains backward compatibility with existing API contracts
- Replace manual RocketController instantiation with RocketControllerDep - Apply DI pattern to all rocket endpoints including motor-reference flows
- Replace manual MotorController instantiation with MotorControllerDep - Apply DI pattern to all motor endpoints (CRUD + simulate + binary)
- Replace manual EnvironmentController instantiation with EnvironmentControllerDep - Complete DI migration across all API routes - All controllers now use singleton pattern for optimal performance
- Replace patch from src.routes to src.dependencies for controller mocking - Use AsyncMock for all controller methods (they are async functions) - Clear lru_cache before and after each test to ensure fresh mock instances
WalkthroughThis PR introduces a dependency injection pattern for controller management. A new dependencies module creates singleton controller factories using LRU caching and provides Annotated type aliases for FastAPI injection. Route handlers across all four resource types (environment, flight, motor, rocket) are updated to receive injected controllers instead of instantiating them locally. Test fixtures are updated to mock at the dependencies layer and manage cache state. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
MarcosMulari
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
README.md (1)
18-21: Specify language for fenced code block.The code block at line 19 should specify a language identifier. This improves syntax highlighting and complies with markdown linting standards.
Apply this diff to add the
textlanguage identifier:-``` +```text RuntimeError: uvloop does not support Windows at the moment -``` +```tests/unit/test_routes/test_motors_route.py (1)
28-45: Well-structured fixture for DI-based controller mocking.The fixture correctly:
- Patches
MotorControllerat the dependencies module level- Uses
AsyncMockfor async controller methods- Clears the LRU cache before and after the test to ensure isolation
One minor observation: line 1 still imports
Mockwhich appears unused in this file (onlyAsyncMockis used throughout).-from unittest.mock import patch, AsyncMock, Mock +from unittest.mock import patch, AsyncMocktests/unit/test_routes/test_rockets_route.py (2)
22-25: Minor formatting issue: extra blank line in import block.Line 23 contains only whitespace which creates inconsistent formatting.
) - - + from src.dependencies import get_rocket_controller -
77-96: LGTM! Fixture correctly implements the DI mocking pattern.The fixture follows the same well-structured approach as the motors test, properly handling cache clearing for test isolation.
Similar to motors,
Mockis imported on line 1 but onlyAsyncMockappears to be used.-from unittest.mock import patch, Mock, AsyncMock +from unittest.mock import patch, AsyncMocktests/unit/test_routes/test_environments_route.py (1)
28-46: LGTM! Fixture correctly implements the DI mocking pattern.The fixture properly handles cache clearing for test isolation.
Minor observations:
- Lines 40-41 have two consecutive blank lines (formatting inconsistency)
Mockis imported on line 1 but onlyAsyncMockis usedmock_class.return_value = mock_controller - - + get_environment_controller.cache_clear()tests/unit/test_routes/test_flights_route.py (1)
47-68: LGTM! Comprehensive fixture for flight controller mocking.The fixture correctly mocks all flight controller methods including:
- Standard CRUD operations (
post_flight,get_flight_by_id,put_flight_by_id,delete_flight_by_id)- Simulation and binary endpoints (
get_flight_simulation,get_rocketpy_flight_binary)- Reference-based operations (
create_flight_from_references,update_flight_from_references)- Partial update operations (
update_environment_by_flight_id,update_rocket_by_flight_id)Same minor note:
Mockis imported on line 1 but onlyAsyncMockis used.-from unittest.mock import patch, Mock, AsyncMock +from unittest.mock import patch, AsyncMocksrc/dependencies.py (2)
11-22: Clarify thread-safety claim in docstring.The docstring states "Using lru_cache ensures thread-safe singleton behavior," which is not entirely accurate. While
lru_cacheoperations are thread-safe, there's a subtle race condition: if multiple threads callget_rocket_controller()simultaneously on the first invocation before the cache is populated, multipleRocketController()instances could be created (only one will be cached).In practice, this is acceptable for FastAPI since controllers are typically stateless (as documented), and FastAPI's dependency system handles this gracefully. However, consider clarifying the docstring to avoid confusion.
""" Provides a singleton RocketController instance. - The controller is stateless and can be safely reused across requests. - Using lru_cache ensures thread-safe singleton behavior. + The controller is stateless and can be safely reused across requests, + so even in the unlikely event of duplicate instantiation during startup, + behavior remains correct. Returns: RocketController: Shared controller instance for rocket operations. """
25-55: Inconsistent docstrings across factory functions.
get_rocket_controller()includes explanation about statelessness and reuse, but the other three factories (get_motor_controller,get_environment_controller,get_flight_controller) have minimal docstrings. Consider harmonizing them for consistency.src/routes/flight.py (2)
2-2: Clarify the primary benefit of dependency injection.The docstring mentions "improved performance," but DI's main benefits are testability, maintainability, and lifecycle management rather than performance. Consider revising to reflect the actual benefits.
Apply this diff:
-Flight routes with dependency injection for improved performance. +Flight routes
78-78: Remove trailing whitespace.Several lines contain trailing whitespace which should be removed for consistency with project formatting standards.
Apply this diff:
with tracer.start_as_current_span("read_flight"): return await controller.get_flight_by_id(flight_id) - + @router.put("/{flight_id}", status_code=204)return await controller.update_flight_from_references( flight_id, payload ) - + @router.delete("/{flight_id}", status_code=204)response_class=Response, ) - async def get_rocketpy_flight_binary(rocket=rocket, ) - + @router.get("/{flight_id}/simulate")with tracer.start_as_current_span("get_flight_simulation"): return await controller.get_flight_simulation(flight_id) - +Also applies to: 119-119, 146-146, 212-212, 227-227
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(1 hunks)src/dependencies.py(1 hunks)src/routes/environment.py(6 hunks)src/routes/flight.py(10 hunks)src/routes/motor.py(5 hunks)src/routes/rocket.py(7 hunks)tests/unit/test_routes/test_environments_route.py(2 hunks)tests/unit/test_routes/test_flights_route.py(2 hunks)tests/unit/test_routes/test_motors_route.py(2 hunks)tests/unit/test_routes/test_rockets_route.py(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-02-25T21:53:37.291Z
Learnt from: GabrielBarberini
Repo: RocketPy-Team/Infinity-API PR: 44
File: src/controllers/interface.py:49-56
Timestamp: 2025-02-25T21:53:37.291Z
Learning: In the Infinity-API architecture, controller methods are intentionally named with the model name included (e.g., `get_environment_by_id` rather than just `get_by_id`) to maintain a convention over configuration approach, enable future automatic route generation, prevent method name collisions, and provide self-documenting code.
Applied to files:
src/routes/environment.py
📚 Learning: 2024-11-15T15:12:21.314Z
Learnt from: GabrielBarberini
Repo: RocketPy-Team/Infinity-API PR: 38
File: lib/routes/motor.py:75-75
Timestamp: 2024-11-15T15:12:21.314Z
Learning: When modifying API route paths in `lib/routes/motor.py`, changing from `/rocketpy/{motor_id}` to `/{motor_id}/rocketpy` is acceptable when no external consumers are affected. It's acceptable to introduce this breaking change if the team has decided to adopt the new approach, provided that the `README` and related documentation are updated accordingly.
Applied to files:
src/routes/environment.pytests/unit/test_routes/test_rockets_route.pytests/unit/test_routes/test_motors_route.pysrc/routes/motor.pysrc/routes/flight.pysrc/routes/rocket.py
📚 Learning: 2024-12-07T11:50:08.415Z
Learnt from: GabrielBarberini
Repo: RocketPy-Team/Infinity-API PR: 41
File: tests/test_routes/test_flights_route.py:122-125
Timestamp: 2024-12-07T11:50:08.415Z
Learning: In the project's pytest test files (e.g., `tests/test_routes/test_flights_route.py`), fixtures like `stub_rocket` and `stub_flight` are function-scoped and can be safely modified within tests without causing side effects.
Applied to files:
tests/unit/test_routes/test_environments_route.pytests/unit/test_routes/test_rockets_route.pytests/unit/test_routes/test_flights_route.pytests/unit/test_routes/test_motors_route.pysrc/routes/flight.py
📚 Learning: 2024-11-16T00:03:14.224Z
Learnt from: GabrielBarberini
Repo: RocketPy-Team/Infinity-API PR: 38
File: tests/test_routes/test_motors_route.py:42-100
Timestamp: 2024-11-16T00:03:14.224Z
Learning: In `tests/test_routes/test_motors_route.py`, focus on assessing whether the API has the capability of validating the input schema, rather than testing the extension of Pydantic validations (e.g., testing empty request payloads).
Applied to files:
tests/unit/test_routes/test_motors_route.pysrc/routes/motor.py
🧬 Code graph analysis (9)
src/routes/environment.py (2)
src/services/environment.py (3)
environment(42-43)environment(46-47)EnvironmentService(11-71)src/controllers/environment.py (3)
get_rocketpy_environment_binary(23-41)EnvironmentController(10-61)__init__(19-20)
tests/unit/test_routes/test_environments_route.py (3)
src/dependencies.py (1)
get_environment_controller(37-44)src/routes/environment.py (2)
get_environment_simulation(133-144)get_rocketpy_environment_binary(106-129)src/controllers/environment.py (3)
get_environment_simulation(44-61)get_rocketpy_environment_binary(23-41)EnvironmentController(10-61)
tests/unit/test_routes/test_rockets_route.py (1)
src/dependencies.py (1)
get_rocket_controller(12-22)
tests/unit/test_routes/test_flights_route.py (3)
src/dependencies.py (1)
get_flight_controller(48-55)src/routes/flight.py (4)
get_flight_simulation(215-226)get_rocketpy_flight_binary(147-168)create_flight_from_references(47-62)update_flight_from_references(99-118)src/controllers/flight.py (5)
get_flight_simulation(146-164)get_rocketpy_flight_binary(125-143)create_flight_from_references(53-62)update_flight_from_references(65-78)FlightController(18-164)
src/dependencies.py (5)
src/controllers/rocket.py (1)
RocketController(17-97)src/controllers/motor.py (1)
MotorController(10-59)src/controllers/environment.py (1)
EnvironmentController(10-61)src/controllers/flight.py (1)
FlightController(18-164)src/services/rocket.py (1)
RocketService(26-260)
tests/unit/test_routes/test_motors_route.py (3)
src/dependencies.py (1)
get_motor_controller(26-33)src/routes/motor.py (1)
get_motor_simulation(129-140)src/controllers/motor.py (1)
get_motor_simulation(44-59)
src/routes/motor.py (1)
src/controllers/motor.py (3)
get_rocketpy_motor_binary(23-41)get_motor_simulation(44-59)MotorController(10-59)
src/routes/flight.py (1)
src/controllers/flight.py (5)
create_flight_from_references(53-62)get_rocketpy_flight_binary(125-143)get_flight_simulation(146-164)FlightController(18-164)update_flight_from_references(65-78)
src/routes/rocket.py (5)
src/repositories/rocket.py (1)
create_rocket(21-22)src/services/rocket.py (2)
rocket(99-100)rocket(103-104)src/models/rocket.py (2)
RocketModel(25-111)RocketWithMotorReferenceRequest(147-161)src/views/rocket.py (2)
RocketCreated(67-69)RocketRetrieved(72-74)src/controllers/rocket.py (2)
create_rocket_from_motor_reference(41-46)get_rocketpy_rocket_binary(61-76)
🪛 markdownlint-cli2 (0.18.1)
README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (9)
tests/unit/test_routes/test_motors_route.py (1)
12-14: LGTM! Import changes align with the new DI pattern.The import of
get_motor_controlleris necessary for cache management in the test fixture.tests/unit/test_routes/test_environments_route.py (1)
13-15: LGTM! Import structure is correct.tests/unit/test_routes/test_flights_route.py (1)
18-20: LGTM! Import structure is correct.src/dependencies.py (2)
1-9: LGTM! Clean imports for the DI module.Appropriate use of
functools.lru_cache,Annotated, andDependsfor FastAPI dependency injection.
57-62: LGTM! Well-defined type aliases for dependency injection.The
Annotatedtype aliases provide clean, reusable dependency declarations for route handlers, following FastAPI best practices.src/routes/environment.py (1)
14-14: Dependency injection pattern correctly applied.All environment endpoints consistently use
EnvironmentControllerDepfor controller injection. The pattern is clean and uniform across CRUD, binary, and simulation endpoints.Also applies to: 32-32, 47-47, 63-63, 83-83, 108-108, 135-135
src/routes/motor.py (1)
14-14: Dependency injection pattern correctly applied.All motor endpoints consistently use
MotorControllerDepfor controller injection, matching the pattern established in environment routes.Also applies to: 30-33, 45-48, 60-64, 79-82, 104-107, 129-132
src/routes/rocket.py (1)
17-17: Dependency injection pattern correctly applied.All rocket endpoints, including the motor-reference flows, consistently use
RocketControllerDepfor controller injection.Also applies to: 33-36, 48-48, 64-67, 79-83, 101-101, 119-122, 144-147, 169-172
src/routes/flight.py (1)
16-16: Dependency injection pattern correctly applied.All flight endpoints, including reference-based and partial-update flows, consistently use
FlightControllerDepfor controller injection.Also applies to: 32-35, 49-49, 66-68, 80-84, 102-102, 121-124, 147-150, 173-176, 193-197, 215-218
There was a problem hiding this comment.
Pull request overview
This PR refactors the application architecture to implement dependency injection for all controllers (Rocket, Motor, Environment, Flight), improving performance by using singleton instances instead of recreating controllers on every request. The implementation uses FastAPI's Depends mechanism with @lru_cache(maxsize=1) for thread-safe controller reuse.
- Created centralized
src/dependencies.pywith dependency provider functions and type aliases - Updated all route handlers across 4 route modules to accept controllers via dependency injection
- Refactored all route tests to properly mock controllers at the dependency layer and clear lru_cache for test isolation
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/dependencies.py |
New file defining singleton dependency providers for all controllers with lru_cache pattern |
src/routes/environment.py |
Replaced manual EnvironmentController instantiation with EnvironmentControllerDep injection |
src/routes/motor.py |
Replaced manual MotorController instantiation with MotorControllerDep injection |
src/routes/rocket.py |
Replaced manual RocketController instantiation with RocketControllerDep injection |
src/routes/flight.py |
Replaced manual FlightController instantiation with FlightControllerDep injection; updated docstring |
tests/unit/test_routes/test_environments_route.py |
Updated mocks to patch src.dependencies, use AsyncMock, and clear lru_cache |
tests/unit/test_routes/test_motors_route.py |
Updated mocks to patch src.dependencies, use AsyncMock, and clear lru_cache |
tests/unit/test_routes/test_rockets_route.py |
Updated mocks to patch src.dependencies, use AsyncMock, and clear lru_cache |
tests/unit/test_routes/test_flights_route.py |
Updated mocks to patch src.dependencies, use AsyncMock, and clear lru_cache |
README.md |
Added Windows-specific guidance about uvloop compatibility issue with Docker recommendation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
There was a problem hiding this comment.
Inconsistent spacing: this line uses 4 spaces for indentation instead of the blank line pattern used elsewhere. For consistency with the rest of the file, consider using a blank line without trailing spaces here.
| return await controller.get_flight_simulation(flight_id) | ||
|
|
There was a problem hiding this comment.
Inconsistent spacing: this line uses 4 spaces for indentation instead of the blank line pattern used elsewhere. For consistency with the rest of the file, consider using a blank line without trailing spaces here.
| return await controller.get_flight_simulation(flight_id) | |
| return await controller.get_flight_simulation(flight_id) |
| RocketView, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Extra whitespace on this line. The line contains only spaces with no meaningful content.
|
|
||
|
|
||
| get_environment_controller.cache_clear() | ||
|
|
||
| yield mock_controller | ||
|
|
There was a problem hiding this comment.
Extra blank line that should be removed for consistency with other test files. The pattern in other test files (motors, rockets, flights) shows no blank line between mock_class.return_value = mock_controller and get_*_controller.cache_clear().
| get_environment_controller.cache_clear() | |
| yield mock_controller | |
| get_environment_controller.cache_clear() | |
| yield mock_controller | |
| yield mock_controller | |
| return await controller.get_flight_by_id(flight_id) | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Inconsistent spacing: this line uses 4 spaces for indentation instead of the blank line pattern used elsewhere. For consistency with the rest of the file, consider using a blank line without trailing spaces here.
| ) | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Inconsistent spacing: this line uses 4 spaces for indentation instead of the blank line pattern used elsewhere. For consistency with the rest of the file, consider using a blank line without trailing spaces here.
1 participant
Changes
Core Implementation
@lru_cache(maxsize=1)for thread-safe controller reuseAnnotated[Controller, Depends(...)]Routes Refactoring
Test Updates
src.dependenciescontrollers instead ofsrc.routesAsyncMockfor all controller methods (they are async functions)lru_cachebefore and after each test to ensure fresh mock instancespatch('src.dependencies.ControllerClass')Related Files
src/dependencies.py- Dependency providerssrc/routes/environment.pysrc/routes/motor.pysrc/routes/rocket.pysrc/routes/flight.pytests/unit/test_routes/test_environments_route.pytests/unit/test_routes/test_motors_route.pytests/unit/test_routes/test_rockets_route.pytests/unit/test_routes/test_flights_route.pyTesting
All route tests updated and passing with the new dependency injection pattern. Controllers are properly mocked and isolated for unit testing.
Testing Strategy Discussion
Initial confusion: I didn't understand why tests call the endpoint through the
clientinstead of directly testing the controller function. Seemed redundant at first.What I learned: This is actually an intentional integration test pattern with specific benefits:
End-to-end verification: By calling through
client.post(), we test the entire request pipeline:Real-world scenarios: The endpoint test simulates actual HTTP usage, catching issues that direct function calls would miss:
Test isolation: Mocking the controller ensures we're testing the route layer independently without touching the actual database/services
Summary by CodeRabbit
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.