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

REST API: provide error messages in error responses #66

Merged
merged 4 commits into from
Mar 27, 2023
Merged

Conversation

edigaryev
Copy link
Collaborator

Resolves #53.

Copy link
Contributor

@fkorotkov fkorotkov left a comment

Choose a reason for hiding this comment

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

Have you looked in gin's context built-in Error method? It seems you can attache errors and then in a middleware handle it. Do you think it's more idiomatic? On the other hand we also introduced the Responder abstraction and this PR embraces it. 🤔

@edigaryev
Copy link
Collaborator Author

edigaryev commented Mar 27, 2023

On the other hand we also introduced the Responder abstraction and this PR embraces it.

That's it, by using Responder we get way more explicit return value/error propagation, which reduces potential errors and makes code clearer.

With ctx.Error() (or ctx.AbortWithError()) things will always be more complicated, as you have not forget about return.

@fkorotkov
Copy link
Contributor

SGTM. Then what do you think of just having body as plain text and not JSON? Or at least renaming NewError to NewErrorResponse?

@edigaryev
Copy link
Collaborator Author

Then what do you think of just having body as plain text and not JSON?

I don't see why we'd want that. JSON will enable for better future extensibility, and parsing it is a no-brainer in most languages.

Or at least renaming NewError to NewErrorResponse?

See 32a93d3.

@fkorotkov fkorotkov merged commit 357a042 into main Mar 27, 2023
@fkorotkov fkorotkov deleted the error-message branch March 27, 2023 18:12
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.

Error responses should include error message
2 participants