src/services/flight.py (2)

93-95: Prefer TypeError for type validation failure.

The static analysis correctly identifies that TypeError is more appropriate than ValueError when validating that the deserialized object is of the expected type.

Proposed fix

         if not isinstance(flight, RocketPyFlight):
-            raise ValueError("File does not contain a RocketPy Flight object")
+            raise TypeError(
+                "Deserialized object is not a RocketPy Flight"
+            )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/flight.py` around lines 93 - 95, Replace the inappropriate
ValueError raised when the deserialized object is not the expected type: in the
method that checks "if not isinstance(flight, RocketPyFlight)" (the code that
currently raises ValueError("File does not contain a RocketPy Flight object")
and returns cls(flight=flight)), change the exception to TypeError and update
the message if desired (e.g., indicate the object is not a RocketPyFlight) so
type validation failures raise TypeError instead of ValueError.

---

296-320: Consider documenting the rationale for default fins fallback.

The default_fins with n=0 (zero fins) serves as a schema-satisfying placeholder when no fins are extracted. While this works, consider adding a brief comment explaining this is a fallback for flights where aerodynamic surfaces weren't fully captured, to aid future maintainers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/flight.py` around lines 296 - 320, The code creates a
placeholder default_fins = [Fins(..., n=0, ...)] used when fins_list is empty
but has no explanatory note; add a concise inline comment (or a brief docstring
above the block) next to default_fins explaining that this is an intentional
schema-satisfying fallback representing "no fins" (n=0) so RocketModel.fins
always has a list when fins extraction fails, and mention any implications for
downstream aerodynamic calculations that should treat n=0 as "no aerodynamic
surfaces." Reference the default_fins variable, the Fins constructor usage, and
the RocketModel(..., fins=...) assignment when placing the comment.

src/controllers/flight.py (1)

203-215: Consider partial failure handling for multi-model persistence.

The import persists Environment, Motor, Rocket, and Flight sequentially without a transaction. If persistence fails mid-way (e.g., Rocket creation fails), earlier models (Environment, Motor) become orphaned in the database with no referencing Flight.

For an MVP this is acceptable, but consider documenting this limitation or adding cleanup logic for robustness.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/controllers/flight.py` around lines 203 - 215, The import currently calls
flight_service.extract_models() and then sequentially persists EnvironmentModel,
MotorModel, RocketModel, and FlightModel via self._persist_model(), which can
leave orphaned records on mid-way failure; either wrap the multi-model
persistence in a DB transaction so all four inserts commit or roll back
atomically, or add compensating cleanup logic that deletes any successfully
created models if a later persist raises an exception (ensure cleanup references
the created ids and handles partial failures), and update FlightImported
construction to only occur after successful commit; relevant symbols:
extract_models, _persist_model, EnvironmentModel, MotorModel, RocketModel,
FlightModel, FlightImported.

tests/unit/test_routes/test_flights_route.py (1)

567-571: Tighten the success-message assertion to protect the API contract.

The current check ("imported" in message) is permissive and won’t catch stale wording like “from binary”.

✅ Suggested test refinement

     assert body['flight_id'] == 'f1'
     assert body['rocket_id'] == 'r1'
     assert body['motor_id'] == 'm1'
     assert body['environment_id'] == 'e1'
-    assert 'imported' in body['message'].lower()
+    assert body['message'] == "Flight successfully imported from .rpy file"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_routes/test_flights_route.py` around lines 567 - 571, The
success-message assertion is too permissive; replace the loose substring check
on body['message'] with a precise contract assertion to prevent regressions
(e.g., instead of assert 'imported' in body['message'].lower(), assert the exact
expected string using the returned id like assert body['message'] == f"Flight
{body['flight_id']} imported." or match whatever canonical message the API
returns). Update the assertion in tests/unit/test_routes/test_flights_route.py
to compare the full message string using body['flight_id'] (and other IDs if
part of the canonical message) so the test fails on any wording change.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/services/flight.py:465

• The generated notebook hard-codes language_info.version to 3.12.0. If the API runtime uses a different Python version, this metadata becomes inaccurate and can confuse users/debugging. Consider omitting the version field or deriving it from sys.version_info at generation time.

                "language_info": {
                    "name": "python",
                    "version": "3.12.0",
                },

src/views/flight.py:156

• FlightImported.message says the flight was imported "from binary", but this PR switches imports to JSON-based .rpy files. This user-facing message should be updated to refer to .rpy/JSON (or just "file") to avoid misleading API consumers.

class FlightImported(ApiBaseView):
    message: str = "Flight successfully imported from binary"
    flight_id: str
    rocket_id: str

src/routes/flight.py:190

• import_flight_from_rpy declares controller: FlightControllerDep = None, unlike the other routes in this module which rely on dependency injection without a default. Dropping the = None keeps the signature consistent and avoids implying the dependency is optional.

async def import_flight_from_rpy(
    file: UploadFile = File(...),
    controller: FlightControllerDep = None,
) -> FlightImported:

src/routes/flight.py:204

• This endpoint reads the entire uploaded file into memory with await file.read(). For potentially large .rpy uploads this can cause high memory usage or worker OOM. Consider enforcing an upload size limit (e.g., via server/middleware) and/or streaming/reading with a capped buffer before passing to the decoder.

    with tracer.start_as_current_span("import_flight_from_rpy"):
        content = await file.read()
        return await controller.import_flight_from_rpy(content)

tests/unit/test_routes/test_flights_route.py (1)

545-602: Add a test for oversized .rpy uploads (HTTP 413).

The route now enforces a 10 MB cap, but this branch isn’t covered yet.

💡 Proposed test

+def test_import_flight_from_rpy_payload_too_large(mock_controller_instance):
+    oversized = b"a" * (10 * 1024 * 1024 + 1)
+    response = client.post(
+        '/flights/upload',
+        files={'file': ('large.rpy', oversized, 'application/json')},
+    )
+    assert response.status_code == 413
+    assert response.json() == {
+        'detail': 'Uploaded .rpy file exceeds 10 MB limit.'
+    }
+    mock_controller_instance.import_flight_from_rpy.assert_not_called()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_routes/test_flights_route.py` around lines 545 - 602, Add a
test to cover the 10 MB upload cap by creating a new function (e.g.,
test_import_flight_from_rpy_oversized) that posts a file larger than 10 * 1024 *
1024 bytes to '/flights/upload' (use a bytes object like b'a' * (10 * 1024 *
1024 + 1)), asserts response.status_code == 413, and verifies
mock_controller_instance.import_flight_from_rpy was not called; place it
alongside the existing tests (test_import_flight_from_rpy,
test_import_flight_from_rpy_invalid, test_import_flight_from_rpy_server_error)
so the oversized branch is covered.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/services/flight.py (1)

179-197: ⚠️ Potential issue | 🔴 Critical

Unfixed critical bug: grain_fields built unconditionally will crash on LIQUID/GENERIC motors.

This issue was flagged in a previous review and acknowledged, but the fix was not applied. Accessing motor.grain_number, motor.grain_density, etc. at lines 179-189 will raise AttributeError for LiquidMotor and GenericMotor instances that lack these attributes.

Move grain_fields construction inside the SOLID | HYBRID branch:

Proposed fix

-        grain_fields = {
-            "grain_number": motor.grain_number,
-            "grain_density": motor.grain_density,
-            "grain_outer_radius": motor.grain_outer_radius,
-            "grain_initial_inner_radius": (motor.grain_initial_inner_radius),
-            "grain_initial_height": motor.grain_initial_height,
-            "grain_separation": motor.grain_separation,
-            "grains_center_of_mass_position": (
-                motor.grains_center_of_mass_position
-            ),
-            "throat_radius": motor.throat_radius,
-        }
-
         match kind:
-            case MotorKinds.SOLID:
-                data |= grain_fields
-            case MotorKinds.HYBRID:
-                data |= grain_fields
+            case MotorKinds.SOLID | MotorKinds.HYBRID:
+                data |= {
+                    "grain_number": motor.grain_number,
+                    "grain_density": motor.grain_density,
+                    "grain_outer_radius": motor.grain_outer_radius,
+                    "grain_initial_inner_radius": (
+                        motor.grain_initial_inner_radius
+                    ),
+                    "grain_initial_height": motor.grain_initial_height,
+                    "grain_separation": motor.grain_separation,
+                    "grains_center_of_mass_position": (
+                        motor.grains_center_of_mass_position
+                    ),
+                    "throat_radius": motor.throat_radius,
+                }
+                if kind is MotorKinds.HYBRID:
+                    data["tanks"] = FlightService._extract_tanks(motor)
-                data["tanks"] = FlightService._extract_tanks(motor)
             case MotorKinds.LIQUID:

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/flight.py` around lines 179 - 197, The code constructs
grain_fields unconditionally using attributes like motor.grain_number which will
raise AttributeError for LiquidMotor/GenericMotor; move the grain_fields
construction inside the branch that handles solids/hybrids so those attributes
are only accessed when kind is MotorKinds.SOLID or MotorKinds.HYBRID, then merge
into data (and add tanks via FlightService._extract_tanks only for HYBRID);
update the match block to build grain_fields locally within the SOLID and HYBRID
cases rather than before the match.

src/services/flight.py (1)

93-101: Consider catching JSONDecodeError for consistent error reporting.

If the input bytes are not valid JSON, json.loads(content) at line 93 will raise JSONDecodeError. Consider wrapping in a try/except to provide a consistent ValueError with a user-friendly message, matching the pattern at line 101.

+        try:
+            data = json.loads(content)
+        except json.JSONDecodeError as exc:
+            raise ValueError("Invalid .rpy file: malformed JSON") from exc
-        data = json.loads(content)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/services/flight.py` around lines 93 - 101, The initial
json.loads(content) can raise json.JSONDecodeError and should be caught so the
function raises a consistent ValueError; wrap the first json.loads(content) (and
optionally the subsequent json.loads(..., cls=RocketPyDecoder)) in a try/except
that catches json.JSONDecodeError and re-raises a ValueError with a clear
user-friendly message (e.g., "Input is not valid JSON" or similar) before
proceeding to validate the resulting object as a RocketPyFlight; update handling
around the RocketPyDecoder / RocketPyFlight check to preserve the existing
ValueError pattern.

tests/unit/test_routes/test_flights_route.py (1)

545-656: Add one real integration test for these new endpoints.

These cases only verify route wiring against mocked controllers, so they will not catch regressions in multipart parsing, upload-limit enforcement, or notebook payload generation. Please add a focused tests/integration/ case for /flights/upload and /flights/{id}/notebook alongside these unit checks.

As per coding guidelines, "Place pure-function tests in tests/unit/ and API or database flows in tests/integration/."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_routes/test_flights_route.py` around lines 545 - 656, Add
real integration tests under tests/integration that exercise the actual FastAPI
app (not mocked controllers) for the two endpoints: POST /flights/upload and GET
/flights/{id}/notebook. For /flights/upload create a test (e.g.,
test_integration_import_flight_from_rpy) that posts a real multipart file with
valid .rpy content and asserts a 201, verifies response body contains
flight_id/rocket_id/motor_id/environment_id and that large payloads (>10MB)
return 413 and do not create a flight; for /flights/{id}/notebook create a test
(e.g., test_integration_get_flight_notebook) that requests an existing flight
id, asserts 200, content-type application/x-ipynb+json and that returned JSON
has nbformat and cells, and also cover 404 for missing ids; use the real app
instance/TestClient and any real persistence or test DB fixtures your suite
provides so multipart parsing, upload-limit enforcement and notebook generation
are actually exercised.