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

Make internal server error new fallback error type #1092

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

georgeboot
Copy link

@georgeboot georgeboot commented Dec 18, 2024

Currently, all errors become a 400 Bad Request, unless an explicit error type is set. This is not ideal, as a bad request should not be retried by a client since it's the client that is at fault (as is for al 4xx codes). A better generic 'fallback' error would be a 500 internal server error, as it indicates something went wrong, and sending another attempt might very well succeed.

This pull requests changes the currenty behaviour by returning a 500 on all non-explicit errors.

_ => (
StatusCode::INTERNAL_SERVER_ERROR,
ErrorDetail::new("internal_server_error", "Internal Server Error"),
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have an InternalServer enum in this match. Replace the INTERNAL_SERVER_ERROR variant with InternalServerError.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the oversight. Fixed

@kaplanelad
Copy link
Contributor

why did you changed the response?

@georgeboot
Copy link
Author

why did you changed the response?

I had to implement the bad request error which carries a reason, and then saw that internal server error had some sort of enum style error message and the same but formatted in the description, so figured it was probably prototyped and never fully implemented, so I did it.

@georgeboot georgeboot requested a review from kaplanelad January 7, 2025 07:20
),
_ => (
StatusCode::INTERNAL_SERVER_ERROR,
ErrorDetail::with_reason("Internal Server Error"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the original response

ErrorDetail::new("internal_server_error", "Internal Server Error"),

Why did you change it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to implement the bad request error which carries a reason, and then saw that internal server error had some sort of enum style error message and the same but formatted in the description, so figured it was probably prototyped and never fully implemented, so I added the change to this PR. Happy to submit it in another PR if you like.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 500 responses should follow this format:

{
    "error":"internal_server_error",
    "description":"Internal Server Error"
}

instead of the format you changed it to:

{
    "error":"Internal Server Error"
}

Copy link
Author

@georgeboot georgeboot Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why the specific format though? The error message looks very arbitrary, the description contains the same as the error itself, where other error types don't have that. The HTTP error code already communicates the error variant for matching, so the body does not need to machine readable. Or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe all errors should follow this format, as it provides a clear, standardized error code for API responses.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants