Skip to content

fix: robust API error handling for invalid thresholds & ranking stats#548

Open
ayushshukla1807 wants to merge 1 commit into
hatnote:masterfrom
ayushshukla1807:fix/threshold-assert-and-ranking-stats-response
Open

fix: robust API error handling for invalid thresholds & ranking stats#548
ayushshukla1807 wants to merge 1 commit into
hatnote:masterfrom
ayushshukla1807:fix/threshold-assert-and-ranking-stats-response

Conversation

@ayushshukla1807

Copy link
Copy Markdown
Contributor

This PR hardens API failure boundaries by addressing two separate issues:

  1. montage/rdb.py: Replaced assert 0.0 <= threshold <= 1.0 with an explicit InvalidAction. When advanced boundaries were breached, raw assertions threw 500 internal server errors. Now it cleanly issues a 400 Bad Request.

  2. montage/juror_endpoints.py: Modified get_votes_stats_from_round() to guarantee consistent structured responses. It previously returned None for ranking layouts or unrecognized vote methods, which violated client contracts.

@lgelauff

lgelauff commented May 2, 2026

Copy link
Copy Markdown
Collaborator

Sounds like a reasonable change and improvement. Did you exhaustively check for side effects? Test locally? How?

@ayushshukla1807

Copy link
Copy Markdown
Contributor Author

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 lgelauff left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 check

One 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants