Skip to content
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

Open
amanape opened this issue Feb 7, 2025 · 6 comments
Open

Standardize Error Response Format Across API #6650

amanape opened this issue Feb 7, 2025 · 6 comments
Labels
backend Related to backend enhancement New feature or request

Comments

@amanape
Copy link
Member

amanape commented Feb 7, 2025

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 amanape added backend Related to backend enhancement New feature or request labels Feb 7, 2025
@raymyers
Copy link
Contributor

raymyers commented Feb 7, 2025

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?

@amanape
Copy link
Member Author

amanape commented Feb 7, 2025

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.

@enyst
Copy link
Collaborator

enyst commented Feb 7, 2025

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

@amanape
Copy link
Member Author

amanape commented Feb 7, 2025

'settings_not_found' would be mapped to settings_not_found$KEY?

Using

except MissingSettingsError as e:
return JSONResponse(
content={
'status': 'error',
'message': str(e),
'msg_id': 'CONFIGURATION$SETTINGS_NOT_FOUND',
},
status_code=400,
)
except LLMAuthenticationError as e:
return JSONResponse(
content={
'status': 'error',
'message': str(e),
'msg_id': 'STATUS$ERROR_LLM_AUTHENTICATION',
},
status_code=400,
)
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

@raymyers
Copy link
Contributor

raymyers commented Feb 7, 2025

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.

@amanape
Copy link
Member Author

amanape commented Feb 11, 2025

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to backend enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants