Narrow exception scope lending

[Arnav-0206]

Closes #12217

Refactor

Technical

This PR narrows the exception scope in openlibrary/core/lending.py by replacing broad except Exception blocks with more specific exception types, as indicated by existing TODO comments.

• Replaced with:

  • httpx.HTTPError for httpx network operations
  • requests.RequestException for requests-based calls
  • JSONDecodeError for JSON parsing

• Removed corresponding TODO comments where addressed

• Preserved expected behavior for network/API failures

• Kept the broad exception in sync_loan() intentionally, as it handles infogami-related errors without a specific exception type

Testing

• Ran the project locally using Docker
• Verified no change in expected behavior for network/API calls
• Confirmed unrelated exceptions (e.g., TypeError, KeyError) are no longer silently swallowed

Screenshot

Not applicable (no UI changes)

Stakeholders

[Copilot]

Pull request overview

Refactors openlibrary/core/lending.py to narrow previously broad exception handling around IA/network calls and JSON parsing, aligning behavior with the intent of existing TODOs while avoiding swallowing unrelated errors.

Changes:

• Replaced several except Exception blocks with targeted network/JSON exceptions (httpx.HTTPError, requests.RequestException, JSONDecodeError).
• Removed corresponding TODO comments where the exception scope was narrowed.
• Kept the broad exception in sync_loan() intentionally.

[Copilot]

Same as the prior except: if .json() fails for the IA loan lookup, httpx will commonly raise ValueError/json.JSONDecodeError rather than simplejson.errors.JSONDecodeError. Consider extending the caught decode exception types here too so malformed JSON is handled consistently.

[Copilot]

httpx.Response.json() raises ValueError/json.JSONDecodeError on invalid JSON (see similar handling in openlibrary/core/ia.py). Catching simplejson.errors.JSONDecodeError here likely won’t catch decode failures from httpx, changing behavior from returning {'error': 'request_timeout'} to raising. Consider catching json.JSONDecodeError (and/or ValueError) alongside httpx.HTTPError (or switch to requests/simplejson decoding consistently).

[Copilot]

resp.json() here is from httpx (via ia.session), which typically raises ValueError/json.JSONDecodeError on invalid JSON rather than simplejson.errors.JSONDecodeError. This means JSON decode failures may now escape this handler and bubble up. Consider catching json.JSONDecodeError (and/or ValueError) alongside httpx.HTTPError.

[Copilot]

ia.session is an httpx.Client, so .json() decode failures will usually raise ValueError/json.JSONDecodeError rather than simplejson.errors.JSONDecodeError. With the narrowed except, invalid JSON may now raise instead of returning None. Consider catching json.JSONDecodeError (and/or ValueError) here too.

[Copilot]

Both calls to _get_ia_loan ultimately rely on httpx responses; decode failures from .json() typically raise ValueError/json.JSONDecodeError, not simplejson.errors.JSONDecodeError. These handlers may stop catching malformed JSON and allow exceptions to bubble up. Consider catching json.JSONDecodeError (and/or ValueError) in these except clauses as well.

[mekarpeles]

It would have been nice if discussed before PR opened; the blanket exceptions weren't great but the question becomes whether these exceptions will catch all the errors we care about (did we trade a working catch-all for something too specific)

[Arnav-0206]

Thank you @mekarpeles for the feedback!
Sure thing, I will make the PR the way you suggested.

Regarding the question of whether these cover all the error types we really care about:

For the IA_Lending_API._post hook (line 1125): Limited this to requests.RequestException. As this is the universal base class for the requests library, it will safely catch all the timeouts, connection errors, and HTTP errors without hiding native Python exceptions (like KeyError or TypeError).

For the ia.session.get().json() calls: Pinpointed httpx.HTTPError (which is effectively the base class for all network/timeout/status exceptions in httpx) along with a fallback to JSONDecodeError in case the IA endpoint sends us a corrupted response body.

For the database sync_loan call (line 815): Specifically kept except Exception as a decent catch all since infogami throws very broad errors on document update conflicts.

I have one question though: if we want to do an exhaustive test to cover all the possible cases, does that mean writing a test for every single exception and error type from libs we use? Will that be overkill?

I am totally determined to make this absolutely right. If you think httpx.HTTPError is still too risky/specific for the lending checks, I would be very glad to extend it to any base exception you consider safest for production. Tell me what you want me to do and I will change the PR accordingly :D

[RayBB]

Closing as this does not follow our contributing guidelines (https://docs.openlibrary.org/developers/CONTRIBUTING.html)

In this case, you're opening a PR before you're assigned to an issue. It's important that you're assigned to ensure it's an issue that staff are prioritizing to review and work on. If no issue exists you can create it.

Thanks for understanding and we welcome you to find another issue follow the process linked above.