-
Notifications
You must be signed in to change notification settings - Fork 5.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Standardize Error Response Format Across API #6650
Comments
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? |
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 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. |
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
Idk, "error" instead of "message" makes sense to distinguish them easily ("message" is overloaded immensely), and, as you said, perhaps for another UI. 🤔 |
Using OpenHands/openhands/server/routes/manage_conversations.py Lines 152 to 170 in 93d2e4a
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)
Agreed |
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. |
After discussing with Robert, we decided that instead of introducing new syntax for 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 The proposed response format would be: {
"error": "Settings not found",
"status_code": "SETTINGS_NOT_FOUND" // Assuming the corresponding i18n key is renamed
} Open questions:
Thoughts? |
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:
Acode
field to categorize the type of errormessage
error
field with a human-readable explanationstatus_code
field that provides a unique identifier for the error typeExample:
IMO the
status: "error"
might also be unnecessary since this data will be contained in anAxiosError
object. Thoughts?Do you have thoughts on the technical implementation?
JSONResponse(content={"error": "some message"})
with the new standardized responseDescribe alternatives you've considered
Additional context
The text was updated successfully, but these errors were encountered: