fix: robust API error handling for invalid thresholds & ranking stats#548
Conversation
|
Sounds like a reasonable change and improvement. Did you exhaustively check for side effects? Test locally? How? |
|
Tested this by manually triggering invalid thresholds and ranking requests on a local instance. It correctly returns a clean error response instead of just letting the app throw a 500. It's a defensive change—I didn't find any side effects in the existing test suite. |
lgelauff
left a comment
There was a problem hiding this comment.
On montage/rdb.py, the get_rating_advancing_group threshold check:
The fix gets the outcome right (500 → 400), but it's worth noting that get_rating_advancing_group is an internal DAO method — by the time it runs, the threshold could reasonably be expected to already be a trusted value. The original assert might have been intentional: "if this fires, a caller messed up."
The reason the assert was causing a 500 in the first place is that advance_round doesn't validate the range before passing the value down:
threshold = float(request_dict['threshold']) # converts, but no range checkOne possible improvement: validate at the HTTP boundary in advance_round and restore the assert in the DAO as a true invariant. Something like:
threshold = float(request_dict['threshold'])
if not (0.0 <= threshold <= 1.0):
raise InvalidAction('threshold must be between 0.0 and 1.0, not %r' % threshold)That said, moving the check into the DAO as InvalidAction works fine in practice — it's more a question of where the validation responsibility belongs.
This PR hardens API failure boundaries by addressing two separate issues:
montage/rdb.py: Replacedassert 0.0 <= threshold <= 1.0with an explicitInvalidAction. When advanced boundaries were breached, raw assertions threw 500 internal server errors. Now it cleanly issues a 400 Bad Request.montage/juror_endpoints.py: Modifiedget_votes_stats_from_round()to guarantee consistent structured responses. It previously returnedNonefor ranking layouts or unrecognized vote methods, which violated client contracts.