OpenHands icon indicating copy to clipboard operation
OpenHands copied to clipboard

Standardize Error Response Format Across API

Open amanape opened this issue 10 months ago • 8 comments

What problem or use case are you trying to solve? Currently, error responses across the API are inconsistent. Some endpoints return {"error": "some message"}, while others might have different structures. This inconsistency will make it difficult to parse and handle errors reliably.

Describe the UX of the solution you'd like All API error responses should follow a standardized format, making it easier for frontend and other consumers to handle errors consistently. The response should include:

  • ~~A code field to categorize the type of error~~
  • An ~~message~~ error field with a human-readable explanation
  • A status_code field that provides a unique identifier for the error type

Example:

{
  "code": 404, // This is probably unnecessary since JSONReponse can already set the code outside this object
  "message": "Settings not found",
  "status_code": "settings_not_found"
}

IMO the status: "error" might also be unnecessary since this data will be contained in an AxiosError object. Thoughts?

Do you have thoughts on the technical implementation?

  • Introduce a helper function or utility that constructs and returns JSON error responses in a consistent format
  • Replace all instances of JSONResponse(content={"error": "some message"}) with the new standardized response

Describe alternatives you've considered

Additional context

amanape avatar Feb 07 '25 16:02 amanape

I agree we have needlessly diverging structure on these, great that you're addressing it. This looks fine as far as it goes but some of these errors have msg_id for translation, so how does that factor in?

Are status codes mappable to msg_id?

raymyers avatar Feb 07 '25 16:02 raymyers

Are status codes mappable to msg_id?

With the existing method, yes. Each error would return a single message ID, and in turn for a single error translation. This means we can always map { some_error_message: "SOME_TRANSLATION$KEY" } since there won't be (and maybe shouldn't be) two different i18n keys for the same error message.

If we want to translate error messages displayed on the client, this should be fine. My thoughts though are to avoid returning i18n keys from the server. Assume a different frontend and the returned i18n could become useless to that frontend.

amanape avatar Feb 07 '25 17:02 amanape

Assume a different frontend and the returned i18n could become useless to that frontend.

That's a very good point! Indeed we have some in the backend. And I was about to introduce two lil' ones. 😇 😅

'settings_not_found' would be mapped to settings_not_found$KEY?

IMO the status: "error" might also be unnecessary since this data will be contained in an AxiosError object. Thoughts?

Idk, "error" instead of "message" makes sense to distinguish them easily ("message" is overloaded immensely), and, as you said, perhaps for another UI. 🤔

enyst avatar Feb 07 '25 17:02 enyst

'settings_not_found' would be mapped to settings_not_found$KEY?

Using https://github.com/All-Hands-AI/OpenHands/blob/93d2e4a338adcaa8acaa602adad14364abca821f/openhands/server/routes/manage_conversations.py#L152-L170 as an example:

settings_not_found would map to CONFIGURATION$SETTINGS_NOT_FOUND missing_llm_api_key would map to STATUS$ERROR_LLM_AUTHENTICATION

I am for returning additional codes as they will allow us to easily handle specific errors in complex ways, but not have them in any way dependant on the FE. (it would also imply that the developer working on the server should be aware of the messages and the translations defined on the frontend, which is another negative)

Idk, "error" instead of "message" makes sense to distinguish them easily ("message" is overloaded immensely), and, as you said, perhaps for another UI. 🤔

Agreed

amanape avatar Feb 07 '25 17:02 amanape

IMO the status: "error" might also be unnecessary since this data will be contained in an AxiosError object. Thoughts?

The errors from the server are not always an AxiosError, sometimes it comes through the websocket connection, in ws-client-provider. So you'll want to decide if that's in the scope of what you're standardizing.

raymyers avatar Feb 07 '25 18:02 raymyers

After discussing with Robert, we decided that instead of introducing new syntax for status_code, the existing i18n keys provide a suitable syntax for status_code, regardless of whether they are used for translations. This would also eliminate the need to map between different status code formats and their corresponding translations.

In which case, it might be beneficial to slightly adjust the current syntax to make it more suitable for error codes. For example, instead of STATUS$ERROR_LLM_AUTHENTICATION, we could use STATUS_ERROR_LLM_AUTHENTICATION or simply ERROR_LLM_AUTHENTICATION. Note: This change would also require updating the i18n keys accordingly.

The proposed response format would be:

{
  "error": "Settings not found",
  "status_code": "SETTINGS_NOT_FOUND" // Assuming the corresponding i18n key is renamed
}

Open questions:

  • Should we include a code property? (e.g., "code": 404)
  • status_code vs. msg_id—I feel that status_code is more suitable for the error response.

Thoughts?

amanape avatar Feb 11 '25 20:02 amanape

This issue is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions[bot] avatar Mar 14 '25 02:03 github-actions[bot]

This issue is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions[bot] avatar Apr 15 '25 02:04 github-actions[bot]

This issue was closed because it has been stalled for over 30 days with no activity.

github-actions[bot] avatar Apr 23 '25 02:04 github-actions[bot]