Skip to content

Commit 032a448

Browse files
committed
BUG: answer 422 instead of 500 for GenericMotor null fields, RocketPy ValueErrors, malformed ids; accept motorless rockets
Replaying the Oct 2026 production P0s against a local instance showed four opaque 500s and a non-standard 422 body: - GENERIC motors with chamber_radius/height/position or propellant_initial_mass = None constructed fine and crashed on first use (Function(None) in center_of_propellant_mass). MotorService now raises 422 "Generic motor requires: a, b" naming every missing field. - RocketPy ValueErrors (overfilled tank, etc.) were wrapped as 500; the controller handler now maps them to 422 with the message. - ObjectId("undefined") raised InvalidId -> 500; find_by_id returns None (404 upstream), update/delete raise 404. - The RequestValidationError handler returned a bare string; it now returns FastAPI's standard {"detail": [{loc, msg, type}]}. RocketModel.motor is now Optional: a rocket saved before its motor exists keeps rocketpy's EmptyMotor (draws fine); rocket/flight simulate answer 422 "Rocket has no motor yet" via RocketService.require_motor(). Tests: 199 passing (18 new). Solution write-up in docs/solutions.
1 parent 2b80bd4 commit 032a448

14 files changed

Lines changed: 515 additions & 34 deletions

File tree

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
---
2+
title: Opaque 500s on drawing-geometry and simulate from GenericMotor null fields, RocketPy ValueErrors and malformed ids
3+
date: 2026-10-18
4+
category: runtime-errors
5+
module: services/motor, controllers/interface, repositories/interface, api
6+
problem_type: runtime_error
7+
component: service_object
8+
severity: critical
9+
symptoms:
10+
- "GET /motors/{id}/drawing-geometry, /rockets/{id}/drawing-geometry, /rockets/{id}/simulate and /flights/{id}/simulate return HTTP 500 'Unexpected error' for any GENERIC motor saved with chamber_position (or chamber_radius / chamber_height / propellant_initial_mass) = null"
11+
- "app.log: AttributeError: 'NoneType' object has no attribute 'shape' inside rocketpy motor.center_of_propellant_mass -> Function(self.chamber_position)"
12+
- "Liquid motor with an overfilled tank: RocketPy ValueError at MassFlowRateBasedTank construction surfaced as 500 instead of the message"
13+
- "PUT /rockets/undefined (jarvis client bug) -> bson InvalidId -> 500 'Unexpected error ocurred'"
14+
- "422 bodies were a bare JSON string ('RequestValidationError: [...]') so jarvis showed 'Unknown validation error'"
15+
- "jarvis Discord alerting escalated every one of these as P0 INTERNAL_SERVER_ERROR"
16+
root_cause: missing_validation
17+
resolution_type: code_fix
18+
related_components:
19+
- src/services/motor.py
20+
- src/controllers/interface.py
21+
- src/repositories/interface.py
22+
- src/api.py
23+
- rocketpy.GenericMotor
24+
tags:
25+
- rocketpy
26+
- generic-motor
27+
- error-handling
28+
- http-422
29+
- validation
30+
- objectid
31+
- jarvis
32+
---
33+
34+
## Problem
35+
36+
Between 2026-10-16 and 2026-10-18 the jarvis instrumentation channel received
37+
dozens of P0 alerts on `motor.drawingGeometry`, `rocket.drawingGeometry`,
38+
`rocket.simulate`, `flight.simulate` and `rocket.update`. Replaying the
39+
production entities (fetched from `api.rocketpy.org`) against a local
40+
Infinity showed four distinct faults, all answered with an opaque 500.
41+
42+
1. **GenericMotor null fields.** `GenericMotor(chamber_position=None, ...)`
43+
constructs without complaint and only crashes when `center_of_propellant_mass`
44+
is first evaluated (`Function(None)` → `'NoneType' has no attribute 'shape'`).
45+
jarvis' COTS import fills chamber radius/height/propellant from the catalogue
46+
but never a chamber position, so every COTS-based GENERIC motor hit this.
47+
2. **RocketPy `ValueError` treated as a server fault.** An overfilled tank, a
48+
bad parachute trigger, inconsistent geometry — RocketPy raises `ValueError`
49+
with a good message; the controller wrapper turned it into 500.
50+
3. **Malformed ids.** `ObjectId("undefined")` raises `InvalidId`, which the
51+
repository wrapper turned into 500.
52+
4. **Non-standard 422 body.** The `RequestValidationError` handler returned
53+
`parse_error(exc)` (a string) as the whole body, so clients looking for
54+
`detail` found nothing.
55+
56+
## Solution
57+
58+
- `MotorService.from_motor_model`, GENERIC branch: collect
59+
`burn_time, chamber_radius, chamber_height, chamber_position,
60+
propellant_initial_mass`; if any is `None` raise
61+
`HTTPException(422, "Generic motor requires: a, b")`. Same "requires:" shape
62+
as the tank-kind guard so jarvis can paint field-level errors.
63+
- `controller_exception_handler`: `except ValueError` → 422 with the
64+
whitespace-collapsed message (logged at warning). Only genuine
65+
programming errors stay 500.
66+
- `repository_exception_handler`: let `HTTPException` through;
67+
`find_by_id` returns `None` for an invalid id (→ 404 upstream);
68+
`update_by_id` / `delete_by_id` raise 404 via `_object_id_or_404`.
69+
- `validation_exception_handler`: respond with FastAPI's standard
70+
`{"detail": [{"loc", "msg", "type"}]}` (dropping `input`/`ctx`, which is
71+
what made the original author stringify the error).
72+
73+
Follow-up (same day): `RocketModel.motor` is now `Optional`. A rocket saved
74+
before its motor exists goes on the wire with `motor: null`, Infinity keeps the
75+
`EmptyMotor` rocketpy constructs every `Rocket` with (drawing already handled
76+
it), and `RocketService.require_motor()` answers 422 "Rocket has no motor yet"
77+
on rocket/flight simulate. jarvis sends `null` until `ids.motorId` is set, so
78+
the placeholder GENERIC motor never reaches Infinity any more.
79+
80+
## Prevention
81+
82+
- Any RocketPy constructor argument with no default must be validated at the
83+
API boundary; "constructs fine, crashes on first use" is the pattern to
84+
look for (RocketPy's lazy `Function` properties).
85+
- Keep `ValueError` → 422 in the controller wrapper; it is RocketPy's
86+
user-input error channel.
87+
- Run `python3 -I /Users/aasitvora/Desktop/chintu/regression-suite/run.py`
88+
before changing a model or service: it covers all 17 official designs and
89+
replays these incidents.

‎src/api.py‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,21 @@ async def validation_exception_handler(
8686
):
8787
exc_str = parse_error(exc)
8888
logger.error("%s: %s", request, exc_str)
89+
# Answer in FastAPI's standard `{"detail": [{loc, msg, type}]}` shape.
90+
# `input` and `ctx` are dropped: `ctx` can carry raw exception objects
91+
# that are not JSON serialisable (the reason this handler originally
92+
# stringified the whole error), and `input` echoes the request body.
93+
detail = [
94+
{
95+
"loc": list(error.get("loc", ())),
96+
"msg": error.get("msg", ""),
97+
"type": error.get("type", ""),
98+
}
99+
for error in exc.errors()
100+
]
89101
return JSONResponse(
90-
content=exc_str, status_code=status.HTTP_422_UNPROCESSABLE_ENTITY
102+
content={"detail": detail},
103+
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
91104
)
92105

93106

‎src/controllers/interface.py‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,17 @@ async def wrapper(self, *args, **kwargs):
2222
)
2323
except HTTPException as e:
2424
raise e from e
25+
except ValueError as e:
26+
# RocketPy raises ValueError for physically invalid input
27+
# (overfilled tank, bad parachute trigger, inconsistent
28+
# geometry). That is the client's design, not a server fault:
29+
# return it as 422 with the message instead of a bare 500.
30+
detail = " ".join(str(e).split())
31+
logger.warning(f"{method.__name__}: Invalid input {detail}")
32+
raise HTTPException(
33+
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
34+
detail=detail,
35+
) from e
2536
except Exception as e:
2637
logger.exception(f"{method.__name__}: Unexpected error {e}")
2738
raise HTTPException(

‎src/models/rocket.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,11 @@ class RocketModel(ApiBaseModel):
2626
NAME: ClassVar = "rocket"
2727
METHODS: ClassVar = ("POST", "GET", "PUT", "DELETE")
2828

29-
# Required parameters
30-
motor: MotorModel
29+
# A rocket may be created before its motor exists (jarvis saves the
30+
# airframe first). `None` maps to rocketpy's `EmptyMotor`, which a
31+
# Rocket carries by default: the rocket persists and draws, and the
32+
# simulate endpoints answer 422 until a real motor is attached.
33+
motor: Optional[MotorModel] = None
3134
radius: float
3235
mass: float
3336
motor_position: float

‎src/repositories/interface.py‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
from fastapi import HTTPException, status
1818
from bson import ObjectId
19+
from bson.errors import InvalidId
1920

2021
from src import logger
2122
from src.secrets import Secrets
@@ -61,6 +62,10 @@ async def wrapper(self, *args, **kwargs):
6162
f"{method.__name__} - Repository not initialized: {e}"
6263
)
6364
raise
65+
except HTTPException:
66+
# Deliberate client-facing errors raised inside a repository
67+
# method (e.g. malformed id → 404) must not be re-wrapped as 500.
68+
raise
6469
except Exception as e:
6570
logger.exception(
6671
f"{method.__name__} - caught unexpected error: {e}"
@@ -258,13 +263,20 @@ async def insert(self, data: dict):
258263
async def update_by_id(self, data: dict, *, data_id: str):
259264
collection = self.get_collection()
260265
assert self.model.model_validate(data)
261-
await collection.update_one({"_id": ObjectId(data_id)}, {"$set": data})
266+
object_id = self._object_id_or_404(data_id)
267+
await collection.update_one({"_id": object_id}, {"$set": data})
262268
return self
263269

264270
@repository_exception_handler
265271
async def find_by_id(self, *, data_id: str):
266272
collection = self.get_collection()
267-
read_data = await collection.find_one({"_id": ObjectId(data_id)})
273+
try:
274+
object_id = ObjectId(data_id)
275+
except InvalidId:
276+
# A malformed id can never match a document; treat it exactly
277+
# like a missing one so the controller answers 404, not 500.
278+
return None
279+
read_data = await collection.find_one({"_id": object_id})
268280
if read_data:
269281
parsed_model = self.model.model_validate(read_data)
270282
parsed_model.set_id(str(read_data["_id"]))
@@ -274,9 +286,24 @@ async def find_by_id(self, *, data_id: str):
274286
@repository_exception_handler
275287
async def delete_by_id(self, *, data_id: str):
276288
collection = self.get_collection()
277-
await collection.delete_one({"_id": ObjectId(data_id)})
289+
await collection.delete_one({"_id": self._object_id_or_404(data_id)})
278290
return self
279291

292+
def _object_id_or_404(self, data_id: str) -> ObjectId:
293+
"""Parse a path id, answering 404 for a malformed one.
294+
295+
The jarvis client was observed issuing PUT /rockets/undefined; bson
296+
raises InvalidId for that, which the generic handler used to turn
297+
into a 500 "Unexpected error ocurred".
298+
"""
299+
try:
300+
return ObjectId(data_id)
301+
except InvalidId as exc:
302+
raise HTTPException(
303+
status_code=status.HTTP_404_NOT_FOUND,
304+
detail=f"{self.model.NAME} not found",
305+
) from exc
306+
280307
@repository_exception_handler
281308
async def find_by_query(self, query: dict):
282309
collection = self.get_collection()

‎src/services/flight.py‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,9 @@ def from_flight_model(cls, flight: FlightModel) -> Self:
6161
rocketpy_env = EnvironmentService.from_env_model(
6262
flight.environment
6363
).environment
64-
rocketpy_rocket = RocketService.from_rocket_model(flight.rocket).rocket
64+
rocket_service = RocketService.from_rocket_model(flight.rocket)
65+
rocket_service.require_motor()
66+
rocketpy_rocket = rocket_service.rocket
6567
rocketpy_flight = RocketPyFlight(
6668
rocket=rocketpy_rocket,
6769
environment=rocketpy_env,
@@ -518,9 +520,7 @@ def get_flight_kml(self) -> bytes:
518520
Returns:
519521
bytes (UTF-8 encoded KML)
520522
"""
521-
with tempfile.NamedTemporaryFile(
522-
suffix=".kml", delete=False
523-
) as tmp:
523+
with tempfile.NamedTemporaryFile(suffix=".kml", delete=False) as tmp:
524524
tmp_path = tmp.name
525525
try:
526526
FlightDataExporter(self.flight).export_kml(file_name=tmp_path)

‎src/services/motor.py‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -236,14 +236,32 @@ def from_motor_model(cls, motor: MotorModel) -> Self:
236236
**optional_params,
237237
)
238238
case _:
239-
# GenericMotor requires burn_time even though it's optional
240-
# for the other motor kinds — surface the constraint at the
241-
# API boundary instead of letting rocketpy raise a
242-
# confusing stack trace deeper in construction.
243-
if motor.burn_time is None:
239+
# GenericMotor has no defaults for burn_time or the chamber
240+
# fields. A None here does not fail at construction — it
241+
# blows up later as `Function(None)` inside
242+
# center_of_propellant_mass when the motor is drawn, added
243+
# to a rocket, or flown. Surface the constraint at the API
244+
# boundary as a 422 that names every missing field, in the
245+
# same "requires: a, b" shape the tank_kind guard uses so
246+
# clients can paint field-level errors.
247+
generic_params = {
248+
'burn_time': motor.burn_time,
249+
'chamber_radius': motor.chamber_radius,
250+
'chamber_height': motor.chamber_height,
251+
'chamber_position': motor.chamber_position,
252+
'propellant_initial_mass': motor.propellant_initial_mass,
253+
}
254+
missing = [
255+
key
256+
for key, value in generic_params.items()
257+
if value is None
258+
]
259+
if missing:
244260
raise HTTPException(
245261
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
246-
detail="burn_time is required for generic motors.",
262+
detail=(
263+
"Generic motor requires: " + ', '.join(missing)
264+
),
247265
)
248266
# nozzle_position is already forwarded via motor_core when
249267
# the client supplied it; GenericMotor's own default (0)

‎src/services/rocket.py‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
Tail as RocketPyTail,
1414
)
1515
from rocketpy.rocket.aero_surface.generic_surface import GenericSurface
16+
from rocketpy.motors.empty_motor import EmptyMotor
1617

1718
from fastapi import HTTPException, status
1819

@@ -62,10 +63,13 @@ def from_rocket_model(cls, rocket: RocketModel) -> Self:
6263
center_of_mass_without_motor=rocket.center_of_mass_without_motor,
6364
coordinate_system_orientation=rocket.coordinate_system_orientation,
6465
)
65-
rocketpy_rocket.add_motor(
66-
MotorService.from_motor_model(rocket.motor).motor,
67-
rocket.motor_position,
68-
)
66+
# No motor yet: keep the EmptyMotor rocketpy constructs the Rocket
67+
# with. Drawing works; the simulate paths refuse via require_motor.
68+
if rocket.motor is not None:
69+
rocketpy_rocket.add_motor(
70+
MotorService.from_motor_model(rocket.motor).motor,
71+
rocket.motor_position,
72+
)
6973

7074
# RailButtons
7175
if rocket.rail_buttons:
@@ -124,12 +128,23 @@ def get_rocket_simulation(self) -> RocketSimulation:
124128
Returns:
125129
RocketSimulation
126130
"""
131+
self.require_motor()
127132
encoded_attributes = collect_attributes(
128133
self.rocket, [RocketSimulation, MotorSimulation]
129134
)
130135
rocket_simulation = RocketSimulation(**encoded_attributes)
131136
return rocket_simulation
132137

138+
def require_motor(self) -> None:
139+
"""Refuse to simulate a rocket that still carries the EmptyMotor."""
140+
if isinstance(self._rocket.motor, EmptyMotor):
141+
raise HTTPException(
142+
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
143+
detail=(
144+
"Rocket has no motor yet. Save a motor before simulating."
145+
),
146+
)
147+
133148
def get_rocket_binary(self) -> bytes:
134149
"""
135150
Get the binary representation of the rocket.

‎src/views/rocket.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ class RocketSimulation(MotorSimulation):
9494

9595
class RocketView(RocketModel):
9696
rocket_id: Optional[str] = None
97-
motor: MotorView
97+
motor: Optional[MotorView] = None
9898

9999

100100
class RocketDrawingGeometry(ApiBaseView):

‎tests/unit/test_controllers/test_controller_interface.py‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,10 @@ async def method(self, model, *args, **kwargs):
8181

8282
@pytest.mark.asyncio
8383
async def test_controller_exception_handler_unexpected_exception(stub_model):
84+
# ValueError is now a deliberate 422 (see the test below); use a
85+
# RuntimeError as the representative programming error.
8486
async def method(self, model, *args, **kwargs):
85-
raise ValueError('Test Error')
87+
raise RuntimeError('Test Error')
8688

8789
wrapped_method = controller_exception_handler(method)
8890
with patch('src.controllers.interface.logger') as mock_logger:
@@ -225,3 +227,23 @@ async def test_controller_interface_delete_model(stub_controller, stub_model):
225227
)
226228
== 'Deleted'
227229
)
230+
231+
232+
@pytest.mark.asyncio
233+
async def test_controller_exception_handler_value_error_is_422(stub_model):
234+
# RocketPy raises ValueError for physically invalid input (overfilled
235+
# tank, bad parachute trigger, ...). That is a client error, not a
236+
# server fault: surface the message as 422 instead of a bare 500.
237+
async def method(self, model, *args, **kwargs):
238+
raise ValueError(
239+
"The tank 'Tank' is overfilled. The gas volume is greater than "
240+
"the total volume of the tank.\n\t\tTry increasing the tank height."
241+
)
242+
243+
wrapped_method = controller_exception_handler(method)
244+
with patch('src.controllers.interface.logger') as mock_logger:
245+
with pytest.raises(HTTPException) as exc:
246+
await wrapped_method(None, stub_model)
247+
assert exc.value.status_code == status.HTTP_422_UNPROCESSABLE_ENTITY
248+
assert exc.value.detail.startswith("The tank 'Tank' is overfilled")
249+
mock_logger.warning.assert_called_once()

0 commit comments

Comments
 (0)