[SUGGESTION] ENH: streamline controller dependency injection in routes and tests #66

MarcosMulari · 2025-12-07T23:19:58Z

Changes

Core Implementation

Create FastAPI dependency providers for all controllers (RocketController, MotorController, EnvironmentController, FlightController)
Implement singleton pattern using @lru_cache(maxsize=1) for thread-safe controller reuse
Add type aliases (RocketControllerDep, MotorControllerDep, etc.) using Annotated[Controller, Depends(...)]
Improves performance by avoiding controller re-instantiation on every request

Routes Refactoring

Replace manual controller instantiation with dependency injection across all endpoints:
- Environment routes (CRUD + simulate + binary)
- Motor routes (CRUD + simulate + binary)
- Rocket routes (CRUD + simulate + binary + motor-reference flows)
- Flight routes (CRUD + simulate + binary + environment/rocket updates + references)
Maintains backward compatibility with existing API contracts

Test Updates

Update test fixtures to patch src.dependencies controllers instead of src.routes
Use AsyncMock for all controller methods (they are async functions)
Clear lru_cache before and after each test to ensure fresh mock instances
Pattern: patch('src.dependencies.ControllerClass')

Related Files

src/dependencies.py - Dependency providers
src/routes/environment.py
src/routes/motor.py
src/routes/rocket.py
src/routes/flight.py
tests/unit/test_routes/test_environments_route.py
tests/unit/test_routes/test_motors_route.py
tests/unit/test_routes/test_rockets_route.py
tests/unit/test_routes/test_flights_route.py

Testing

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 client instead 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:
- FastAPI request parsing and validation
- Dependency injection resolution
- Route handler execution
- Response serialization
- HTTP status codes
Real-world scenarios: The endpoint test simulates actual HTTP usage, catching issues that direct function calls would miss:
- Pydantic validation errors
- Request body parsing
- Response formatting
Test isolation: Mocking the controller ensures we're testing the route layer independently without touching the actual database/services

Summary by CodeRabbit

Documentation
- Added a Windows Users note describing a uvloop installation/runtime issue and recommending Docker as a workaround.
Refactor
- Route handlers now use a centralized dependency-injection provider for controller singletons, simplifying controller management and improving maintainability.
Tests
- Unit tests updated to use asynchronous mocks and to clear provider caches around fixtures to align with the new DI approach.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

- 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

…n to README

coderabbitai · 2025-12-07T23:20:07Z

Warning

Rate limit exceeded

@GabrielBarberini has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 1 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between c212b9f and d8b43e3.

📒 Files selected for processing (2)

src/routes/rocket.py (7 hunks)
tests/unit/test_routes/test_motors_route.py (2 hunks)

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a dependency-injection layer (src/dependencies.py) with LRU-cached singleton controller factories and FastAPI Annotated dependencies; updates route handlers to accept injected controllers; updates unit tests to patch at the dependencies layer and use AsyncMock; README adds a Windows note about uvloop.

Changes

Cohort / File(s)	Summary
Documentation `README.md`	Added "Windows Users" section describing `uvloop` installation/runtime issue on Windows and recommending Docker as a workaround.
Dependency injection module `src/dependencies.py`	New module: LRU-cached singleton factory functions `get_rocket_controller`, `get_motor_controller`, `get_environment_controller`, `get_flight_controller` and FastAPI Annotated dependency aliases `RocketControllerDep`, `MotorControllerDep`, `EnvironmentControllerDep`, `FlightControllerDep`.
Routes — Environment `src/routes/environment.py`	Route handlers now accept `controller: EnvironmentControllerDep`; removed local `EnvironmentController()` instantiation; import switched to `src.dependencies`.
Routes — Flight `src/routes/flight.py`	All flight endpoints now accept `controller: FlightControllerDep`; removed local `FlightController()` instantiation; import switched to `src.dependencies`.
Routes — Motor `src/routes/motor.py`	All motor endpoints now accept `controller: MotorControllerDep`; removed local `MotorController()` instantiation; import switched to `src.dependencies`.
Routes — Rocket `src/routes/rocket.py`	All rocket endpoints now accept `controller: RocketControllerDep`; removed local `RocketController()` instantiation; import switched to `src.dependencies`.
Unit tests — route fixtures `tests/unit/test_routes/test_environments_route.py`, `.../test_flights_route.py`, `.../test_motors_route.py`, `.../test_rockets_route.py`	Tests patch controller classes at `src.dependencies` instead of route modules, use `AsyncMock` for controller instances (assigning `mock_class.return_value`), and call `get_<controller>_controller.cache_clear()` around fixtures to reset the LRU cache.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Areas needing extra attention:
- src/dependencies.py: validate lru_cache usage and that returned instances match expected controller interfaces (sync vs async method expectations).
- Routes: confirm FastAPI Annotated / Depends typings and parameter ordering across endpoints; ensure no endpoint missed.
- Tests: check patches target src.dependencies, AsyncMock behavior, and that cache_clear() calls properly isolate fixtures (one fixture shows duplicated yield).
- Resource lifecycle: if controllers manage connections/resources, verify startup/shutdown/cleanup are still handled.

Possibly related PRs

API V3 Refactoring #44 — Earlier refactor introducing controllers/routes/interfaces that this change adapts to dependency injection and singleton factories.

Suggested reviewers

aasitvora99
shantanu-sardesai
Gui-FernandesBR
phmbressan

Poem

🐰 I hopped through code with a curious nose,
Cached controllers snug where dependency grows,
Routes sip their helpers from a tidy well,
Tests mock the hops — all is working well,
I nibble a carrot and watch CI compose. 🎩

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: streamlining controller dependency injection across routes and tests through a new dependencies module and refactored imports.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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 text language 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 MotorController at the dependencies module level

Uses AsyncMock for async controller methods

Clears the LRU cache before and after the test to ensure isolation

One minor observation: line 1 still imports Mock which appears unused in this file (only AsyncMock is used throughout).
-from unittest.mock import patch, AsyncMock, Mock
+from unittest.mock import patch, AsyncMock
tests/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, Mock is imported on line 1 but only AsyncMock appears to be used.
-from unittest.mock import patch, Mock, AsyncMock
+from unittest.mock import patch, AsyncMock
tests/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)

Mock is imported on line 1 but only AsyncMock is used
         mock_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: Mock is imported on line 1 but only AsyncMock is used.
-from unittest.mock import patch, Mock, AsyncMock
+from unittest.mock import patch, AsyncMock
src/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_cache operations are thread-safe, there's a subtle race condition: if multiple threads call get_rocket_controller() simultaneously on the first invocation before the cache is populated, multiple RocketController() 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9ed5aa4 and 24a0dfc.

📒 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.py
tests/unit/test_routes/test_rockets_route.py
tests/unit/test_routes/test_motors_route.py
src/routes/motor.py
src/routes/flight.py
src/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.py
tests/unit/test_routes/test_rockets_route.py
tests/unit/test_routes/test_flights_route.py
tests/unit/test_routes/test_motors_route.py
src/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.py
src/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_controller is 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, and Depends for FastAPI dependency injection.

57-62: LGTM! Well-defined type aliases for dependency injection.

The Annotated type 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 EnvironmentControllerDep for 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 MotorControllerDep for 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 RocketControllerDep for 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 FlightControllerDep for 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

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

GabrielBarberini

Excellent contribution! We used to have stateful controllers in the past and ever since we refactored it with the interface reflection it became stateless and we totally missed the opportunity to introduce dep. inj

Great job

GabrielBarberini · 2025-12-13T14:48:36Z

src/dependencies.py

+from src.controllers.environment import EnvironmentController
+from src.controllers.flight import FlightController
+
+@lru_cache(maxsize=1)


@MarcosMulari what would you say to the idea of using simple @cache instead?

Since there are no args here maxsize=1 will act as a Noop and cache === lru_cache(maxsize=None)

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (2)

tests/unit/test_routes/test_environments_route.py (1)
13-14: Remove extra blank line.

Two consecutive blank lines appear before the import statement. Standard convention is one blank line between import groups.

Apply this diff:
 )
-
 
 from src.dependencies import get_environment_controller
tests/unit/test_routes/test_rockets_route.py (1)
22-23: Remove extra blank line.

Two consecutive blank lines appear before the import statement. Standard convention is one blank line between import groups.

Apply this diff:
 )
-
 
 from src.dependencies import get_rocket_controller

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 24a0dfc and 7980854.

📒 Files selected for processing (3)

src/routes/flight.py (10 hunks)
tests/unit/test_routes/test_environments_route.py (2 hunks)
tests/unit/test_routes/test_rockets_route.py (2 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

tests/{unit,integration}/**/*.py