feat(framework): Handle rejection errors in fetch flows#6754
feat(framework): Handle rejection errors in fetch flows#6754mohammadnaseri wants to merge 4 commits intomainfrom
Conversation
mohammadnaseri
requested review from
danieljanes,
panh99 and
tanertopal
as code owners
There was a problem hiding this comment.
Pull request overview
Adds explicit handling for Platform API compatibility rejections (HTTP 412) in request_download_link by extracting structured error details and surfacing a more informative ValueError message to callers.
Changes:
- Add a dedicated HTTP 412 branch in
request_download_linkto parsedetailpayloads (dict or string) and format a user-facing error. - Extend the existing table-driven tests to include a 412 “incompatible” scenario with structured error details.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
framework/py/flwr/supercore/utils.py |
Implements HTTP 412-specific error parsing/formatting before the generic not resp.ok handler. |
framework/py/flwr/supercore/utils_test.py |
Adds a new table-driven scenario validating the new 412 incompatibility error surface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if resp.status_code == 412: | ||
| try: | ||
| error_message = resp.json().get("detail") | ||
| except ValueError: |
There was a problem hiding this comment.
resp.json().get("detail") assumes resp.json() returns a dict; if the server returns a non-object JSON value (e.g., list/string) this will raise AttributeError and bypass the intended 412 handling. Consider parsing JSON into a variable and only calling .get when it’s a dict (otherwise treat it as the detail payload or fall back to the default incompatibility message).
| }, | ||
| "raises": "incompatible with this app version", | ||
| }, | ||
| { |
There was a problem hiding this comment.
The new 412 handling introduces multiple branches (invalid JSON, detail as string, missing/empty detail, etc.), but the added scenario only covers the structured-dict case. Add table cases to exercise the other branches so regressions in the 412 parsing/formatting logic are caught by tests.
| { | |
| { | |
| "name": "http_412_detail_string", | |
| "fake_resp": { | |
| "ok": False, | |
| "status": 412, | |
| "json": { | |
| "detail": "simple 412 error", | |
| }, | |
| }, | |
| "raises": "simple 412 error", | |
| }, | |
| { | |
| "name": "http_412_missing_detail", | |
| "fake_resp": { | |
| "ok": False, | |
| "status": 412, | |
| "json": {}, | |
| "text": "precondition failed (no detail)", | |
| }, | |
| "raises": "status 412", | |
| }, | |
| { | |
| "name": "http_412_empty_detail", | |
| "fake_resp": { | |
| "ok": False, | |
| "status": 412, | |
| "json": { | |
| "detail": "", | |
| }, | |
| "text": "precondition failed (empty detail)", | |
| }, | |
| "raises": "status 412", | |
| }, | |
| { | |
| "name": "http_412_invalid_json", | |
| "fake_resp": { | |
| "ok": False, | |
| "status": 412, | |
| # Simulate a non-JSON/invalid-JSON body by omitting `json` | |
| "text": "<html>Precondition Failed</html>", | |
| }, | |
| "raises": "status 412", | |
| }, | |
| { |
github-actions
bot
added
the
Maintainer
Handle platform compatibility rejections (HTTP 412) in fetch flows by parsing structured error details.