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

More consistent error codes and format #114

Merged
merged 4 commits into from
Nov 21, 2023

Conversation

nortron
Copy link
Contributor

@nortron nortron commented Nov 4, 2023

These 3 commits attempt to make the API more consistent in returning JSON errors with non-2xx HTTP error codes.

  • When requesting an unknown URL or exceeding rate limits the response is returned as plaintext instead of JSON: https://the-one-api.dev/v2/notreal
    • Both should be addressed by the 1st and 3rd commits
  • Error handling in the Book endpoints was not returning a status code so 200 comes back: https://the-one-api.dev/v2/book/123
    • Instead of inserting the 500 status code like the other endpoints currently do I centralized the entire error handling to the final express error handler so if the error message ever needs to change it can happen in one place.
      • Express docs say Starting with Express 5, route handlers and middleware that return a Promise will call next(value) automatically when they reject or throw an error. If this repo is ever ever upgraded to Express 5 (currently on 4.17) or later these new try blocks can be entirely removed, the next() calls will be automatic.

The 4th commit updates a code comment to reflect the latest code.

@MateuszKikmunter
Copy link
Collaborator

MateuszKikmunter commented Nov 6, 2023

Looks good to me, good idea with a centralized error handler. 👍
One thing though - can you pls change notFoundResponse to be just Not Found instead of Endpoint does not exist.

Move the 404 message into a JSON response constant and return
that when something isn't found.
Move all instances of generic error responses to a global Express
handler that returns the stock error message and a 500 status code.
This will also fix book endpoints returning 200 when erroring.
@nortron
Copy link
Contributor Author

nortron commented Nov 6, 2023

@MateuszKikmunter No problem, updated to "Not found" with a lowercase "found" to be consistent with the "Something went wrong" capitalization. Happy to make it all caps if you'd prefer that.

Copy link
Collaborator

@MateuszKikmunter MateuszKikmunter left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for taking my suggestions into consideration.

@MateuszKikmunter
Copy link
Collaborator

@MateuszKikmunter No problem, updated to "Not found" with a lowercase "found" to be consistent with the "Something went wrong" capitalization. Happy to make it all caps if you'd prefer that.

Thanks, looks good to me, now it needs to be approved by @gitfrosh :)

@MateuszKikmunter MateuszKikmunter merged commit f9f74a2 into gitfrosh:release Nov 21, 2023
2 checks passed
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