BUG: 422 instead of 500 for GenericMotor null fields, RocketPy ValueErrors, malformed ids; accept motorless rockets

src/api.py

@@ -86,8 +86,21 @@ async def validation_exception_handler(
 ):
     exc_str = parse_error(exc)
     logger.error("%s: %s", request, exc_str)
+    # Answer in FastAPI's standard `{"detail": [{loc, msg, type}]}` shape.
+    # `input` and `ctx` are dropped: `ctx` can carry raw exception objects
+    # that are not JSON serialisable (the reason this handler originally
+    # stringified the whole error), and `input` echoes the request body.
+    detail = [
+        {
+            "loc": list(error.get("loc", ())),
+            "msg": error.get("msg", ""),
+            "type": error.get("type", ""),
+        }
+        for error in exc.errors()
+    ]
     return JSONResponse(
-        content=exc_str, status_code=status.HTTP_422_UNPROCESSABLE_ENTITY
+        content={"detail": detail},
+        status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
     )
 
 

src/controllers/interface.py

@@ -1,5 +1,6 @@
 import functools
 from typing import List
+from pydantic import ValidationError
 from pymongo.errors import PyMongoError
 from fastapi import HTTPException, status
 
@@ -22,6 +23,27 @@ async def wrapper(self, *args, **kwargs):
             )
         except HTTPException as e:
             raise e from e
+        except ValidationError as e:
+            # pydantic's ValidationError subclasses ValueError, but a model
+            # failing to validate here means a persisted document or a view
+            # is malformed: a server fault, never client input. Keep it on
+            # the generic 500 path so no internal detail reaches the client.
+            logger.exception(f"{method.__name__}: Unexpected error {e}")
+            raise HTTPException(
+                status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+                detail="Unexpected error",
+            ) from e
+        except ValueError as e:
+            # RocketPy raises ValueError for physically invalid input
+            # (overfilled tank, bad parachute trigger, inconsistent
+            # geometry). That is the client's design, not a server fault:
+            # return it as 422 with the message instead of a bare 500.
+            detail = " ".join(str(e).split())
+            logger.warning(f"{method.__name__}: Invalid input {detail}")
+            raise HTTPException(
+                status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
+                detail=detail,
+            ) from e
         except Exception as e:
             logger.exception(f"{method.__name__}: Unexpected error {e}")
             raise HTTPException(

src/models/rocket.py

@@ -26,8 +26,11 @@ class RocketModel(ApiBaseModel):
     NAME: ClassVar = "rocket"
     METHODS: ClassVar = ("POST", "GET", "PUT", "DELETE")
 
-    # Required parameters
-    motor: MotorModel
+    # A rocket may be created before its motor exists (jarvis saves the
+    # airframe first). `None` maps to rocketpy's `EmptyMotor`, which a
+    # Rocket carries by default: the rocket persists and draws, and the
+    # simulate endpoints answer 422 until a real motor is attached.
+    motor: Optional[MotorModel] = None
     radius: float
     mass: float
     motor_position: float

src/repositories/interface.py

@@ -16,6 +16,7 @@
 
 from fastapi import HTTPException, status
 from bson import ObjectId
+from bson.errors import InvalidId
 
 from src import logger
 from src.secrets import Secrets
@@ -61,6 +62,10 @@ async def wrapper(self, *args, **kwargs):
                 f"{method.__name__} - Repository not initialized: {e}"
             )
             raise
+        except HTTPException:
+            # Deliberate client-facing errors raised inside a repository
+            # method (e.g. malformed id → 404) must not be re-wrapped as 500.
+            raise
         except Exception as e:
             logger.exception(
                 f"{method.__name__} - caught unexpected error: {e}"
@@ -258,13 +263,20 @@ async def insert(self, data: dict):
     async def update_by_id(self, data: dict, *, data_id: str):
         collection = self.get_collection()
         assert self.model.model_validate(data)
-        await collection.update_one({"_id": ObjectId(data_id)}, {"$set": data})
+        object_id = self._object_id_or_404(data_id)
+        await collection.update_one({"_id": object_id}, {"$set": data})
         return self
 
     @repository_exception_handler
     async def find_by_id(self, *, data_id: str):
         collection = self.get_collection()
-        read_data = await collection.find_one({"_id": ObjectId(data_id)})
+        try:
+            object_id = ObjectId(data_id)
+        except InvalidId:
+            # A malformed id can never match a document; treat it exactly
+            # like a missing one so the controller answers 404, not 500.
+            return None
+        read_data = await collection.find_one({"_id": object_id})
         if read_data:
             parsed_model = self.model.model_validate(read_data)
             parsed_model.set_id(str(read_data["_id"]))
@@ -274,9 +286,24 @@ async def find_by_id(self, *, data_id: str):
     @repository_exception_handler
     async def delete_by_id(self, *, data_id: str):
         collection = self.get_collection()
-        await collection.delete_one({"_id": ObjectId(data_id)})
+        await collection.delete_one({"_id": self._object_id_or_404(data_id)})
         return self
 
+    def _object_id_or_404(self, data_id: str) -> ObjectId:
+        """Parse a path id, answering 404 for a malformed one.
+
+        The jarvis client was observed issuing PUT /rockets/undefined; bson
+        raises InvalidId for that, which the generic handler used to turn
+        into a 500 "Unexpected error ocurred".
+        """
+        try:
+            return ObjectId(data_id)
+        except InvalidId as exc:
+            raise HTTPException(
+                status_code=status.HTTP_404_NOT_FOUND,
+                detail=f"{self.model.NAME} not found",
+            ) from exc
+
     @repository_exception_handler
     async def find_by_query(self, query: dict):
         collection = self.get_collection()

src/services/flight.py

@@ -61,7 +61,9 @@ def from_flight_model(cls, flight: FlightModel) -> Self:
         rocketpy_env = EnvironmentService.from_env_model(
             flight.environment
         ).environment
-        rocketpy_rocket = RocketService.from_rocket_model(flight.rocket).rocket
+        rocket_service = RocketService.from_rocket_model(flight.rocket)
+        rocket_service.require_motor()
+        rocketpy_rocket = rocket_service.rocket
         rocketpy_flight = RocketPyFlight(
             rocket=rocketpy_rocket,
             environment=rocketpy_env,
@@ -518,9 +520,7 @@ def get_flight_kml(self) -> bytes:
         Returns:
             bytes (UTF-8 encoded KML)
         """
-        with tempfile.NamedTemporaryFile(
-            suffix=".kml", delete=False
-        ) as tmp:
+        with tempfile.NamedTemporaryFile(suffix=".kml", delete=False) as tmp:
             tmp_path = tmp.name
         try:
             FlightDataExporter(self.flight).export_kml(file_name=tmp_path)

src/services/motor.py

@@ -236,14 +236,32 @@ def from_motor_model(cls, motor: MotorModel) -> Self:
                     **optional_params,
                 )
             case _:
-                # GenericMotor requires burn_time even though it's optional
-                # for the other motor kinds — surface the constraint at the
-                # API boundary instead of letting rocketpy raise a
-                # confusing stack trace deeper in construction.
-                if motor.burn_time is None:
+                # GenericMotor has no defaults for burn_time or the chamber
+                # fields. A None here does not fail at construction — it
+                # blows up later as `Function(None)` inside
+                # center_of_propellant_mass when the motor is drawn, added
+                # to a rocket, or flown. Surface the constraint at the API
+                # boundary as a 422 that names every missing field, in the
+                # same "requires: a, b" shape the tank_kind guard uses so
+                # clients can paint field-level errors.
+                generic_params = {
+                    'burn_time': motor.burn_time,
+                    'chamber_radius': motor.chamber_radius,
+                    'chamber_height': motor.chamber_height,
+                    'chamber_position': motor.chamber_position,
+                    'propellant_initial_mass': motor.propellant_initial_mass,
+                }
+                missing = [
+                    key
+                    for key, value in generic_params.items()
+                    if value is None
+                ]
+                if missing:
                     raise HTTPException(
                         status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
-                        detail="burn_time is required for generic motors.",
+                        detail=(
+                            "Generic motor requires: " + ', '.join(missing)
+                        ),
                     )
                 # nozzle_position is already forwarded via motor_core when
                 # the client supplied it; GenericMotor's own default (0)

src/services/rocket.py

@@ -13,6 +13,7 @@
     Tail as RocketPyTail,
 )
 from rocketpy.rocket.aero_surface.generic_surface import GenericSurface
+from rocketpy.motors.empty_motor import EmptyMotor
 
 from fastapi import HTTPException, status
 
@@ -62,10 +63,13 @@ def from_rocket_model(cls, rocket: RocketModel) -> Self:
             center_of_mass_without_motor=rocket.center_of_mass_without_motor,
             coordinate_system_orientation=rocket.coordinate_system_orientation,
         )
-        rocketpy_rocket.add_motor(
-            MotorService.from_motor_model(rocket.motor).motor,
-            rocket.motor_position,
-        )
+        # No motor yet: keep the EmptyMotor rocketpy constructs the Rocket
+        # with. Drawing works; the simulate paths refuse via require_motor.
+        if rocket.motor is not None:
+            rocketpy_rocket.add_motor(
+                MotorService.from_motor_model(rocket.motor).motor,
+                rocket.motor_position,
+            )
 
         # RailButtons
         if rocket.rail_buttons:
@@ -124,12 +128,23 @@ def get_rocket_simulation(self) -> RocketSimulation:
         Returns:
             RocketSimulation
         """
+        self.require_motor()
         encoded_attributes = collect_attributes(
             self.rocket, [RocketSimulation, MotorSimulation]
         )
         rocket_simulation = RocketSimulation(**encoded_attributes)
         return rocket_simulation
 
+    def require_motor(self) -> None:
+        """Refuse to simulate a rocket that still carries the EmptyMotor."""
+        if isinstance(self._rocket.motor, EmptyMotor):
+            raise HTTPException(
+                status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
+                detail=(
+                    "Rocket has no motor yet. Save a motor before simulating."
+                ),
+            )
+
     def get_rocket_binary(self) -> bytes:
         """
         Get the binary representation of the rocket.

src/views/rocket.py

@@ -94,7 +94,7 @@ class RocketSimulation(MotorSimulation):
 
 class RocketView(RocketModel):
     rocket_id: Optional[str] = None
-    motor: MotorView
+    motor: Optional[MotorView] = None
 
 
 class RocketDrawingGeometry(ApiBaseView):

tests/unit/test_controllers/test_controller_interface.py

@@ -2,6 +2,7 @@
 import pytest
 from pymongo.errors import PyMongoError
 from fastapi import HTTPException, status
+from pydantic import BaseModel
 from src.controllers.interface import (
     ControllerBase,
     controller_exception_handler,
@@ -81,8 +82,10 @@ async def method(self, model, *args, **kwargs):
 
 @pytest.mark.asyncio
 async def test_controller_exception_handler_unexpected_exception(stub_model):
+    # ValueError is now a deliberate 422 (see the test below); use a
+    # RuntimeError as the representative programming error.
     async def method(self, model, *args, **kwargs):
-        raise ValueError('Test Error')
+        raise RuntimeError('Test Error')
 
     wrapped_method = controller_exception_handler(method)
     with patch('src.controllers.interface.logger') as mock_logger:
@@ -225,3 +228,47 @@ async def test_controller_interface_delete_model(stub_controller, stub_model):
                 )
                 == 'Deleted'
             )
+
+
+@pytest.mark.asyncio
+async def test_controller_exception_handler_value_error_is_422(stub_model):
+    # RocketPy raises ValueError for physically invalid input (overfilled
+    # tank, bad parachute trigger, ...). That is a client error, not a
+    # server fault: surface the message as 422 instead of a bare 500.
+    async def method(self, model, *args, **kwargs):
+        raise ValueError(
+            "The tank 'Tank' is overfilled. The gas volume is greater than "
+            "the total volume of the tank.\n\t\tTry increasing the tank height."
+        )
+
+    wrapped_method = controller_exception_handler(method)
+    with patch('src.controllers.interface.logger') as mock_logger:
+        with pytest.raises(HTTPException) as exc:
+            await wrapped_method(None, stub_model)
+        assert exc.value.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY
+        assert exc.value.detail.startswith("The tank 'Tank' is overfilled")
+        mock_logger.warning.assert_called_once()
+
+
+@pytest.mark.asyncio
+async def test_controller_exception_handler_pydantic_validation_error_stays_500(
+    stub_model,
+):
+    # pydantic.ValidationError subclasses ValueError. A model failing to
+    # validate inside a controller means persisted data or a view is
+    # malformed (server fault): it must not take the 422 branch and leak the
+    # validation detail to the client.
+    class Strict(BaseModel):
+        radius: float
+
+    async def method(self, model):  # pylint: disable=unused-argument
+        Strict(radius="not a number")
+
+    wrapped_method = controller_exception_handler(method)
+    with patch('src.controllers.interface.logger') as mock_logger:
+        with pytest.raises(HTTPException) as exc:
+            await wrapped_method(None, stub_model)
+        assert exc.value.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR
+        assert exc.value.detail == 'Unexpected error'
+        mock_logger.exception.assert_called_once()
+        mock_logger.warning.assert_not_called()

tests/unit/test_repositories/test_repository_interface.py

@@ -284,3 +284,51 @@ async def test_repository_find_by_query_not_found(
     ):
         assert await stub_repository.find_by_query('mock_query') == []
         mock_db_interface_empty_find.find.assert_called_once_with('mock_query')
+
+
+# ---------------------------------------------------------------------------
+# Malformed ids. The jarvis client was observed calling PUT /rockets/undefined;
+# bson raises InvalidId, which the generic handler turned into a 500. A
+# malformed id is a client error: find returns None (→ 404 upstream),
+# update/delete raise 404 directly.
+# ---------------------------------------------------------------------------
+@pytest.mark.asyncio
+async def test_repository_find_by_invalid_id_returns_none(
+    stub_repository, mock_db_interface
+):
+    with patch(
+        'src.repositories.interface.RepositoryInterface.get_collection',
+        return_value=mock_db_interface,
+    ):
+        assert await stub_repository.find_by_id(data_id='undefined') is None
+        mock_db_interface.find_one.assert_not_called()
+
+
+@pytest.mark.asyncio
+async def test_repository_update_by_invalid_id_is_404(
+    stub_repository, mock_db_interface
+):
+    with patch(
+        'src.repositories.interface.RepositoryInterface.get_collection',
+        return_value=mock_db_interface,
+    ):
+        with pytest.raises(HTTPException) as exc:
+            await stub_repository.update_by_id(
+                data_id='undefined', data={'foo': 'bar'}
+            )
+        assert exc.value.status_code == status.HTTP_404_NOT_FOUND
+        mock_db_interface.update_one.assert_not_called()
+
+
+@pytest.mark.asyncio
+async def test_repository_delete_by_invalid_id_is_404(
+    stub_repository, mock_db_interface
+):
+    with patch(
+        'src.repositories.interface.RepositoryInterface.get_collection',
+        return_value=mock_db_interface,
+    ):
+        with pytest.raises(HTTPException) as exc:
+            await stub_repository.delete_by_id(data_id='undefined')
+        assert exc.value.status_code == status.HTTP_404_NOT_FOUND
+        mock_db_interface.delete_one.assert_not_called()