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

Add message field to the error message emitted by grpc-gateway #718

Merged
merged 3 commits into from
Aug 3, 2018

Conversation

ffredsh
Copy link
Contributor

@ffredsh ffredsh commented Aug 2, 2018

This is to make it more consistent with the Status proto message in order to make the error more consistent between grpc and json.

We currently use grpc-gateway for local developement and ESP in production and having to deal with the message in two different fields is a bit annoying. This makes the behaviour of grpc-gateway a bit more consistent with ESP.

@codecov-io
Copy link

codecov-io commented Aug 2, 2018

Codecov Report

Merging #718 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #718      +/-   ##
=========================================
+ Coverage   56.28%   56.3%   +0.01%     
=========================================
  Files          30      30              
  Lines        3061    3062       +1     
=========================================
+ Hits         1723    1724       +1     
  Misses       1167    1167              
  Partials      171     171
Impacted Files Coverage Δ
runtime/errors.go 45.83% <100%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42fa202...1283180. Read the comment docs.

@@ -66,6 +66,9 @@ var (

type errorBody struct {
Error string `protobuf:"bytes,1,name=error" json:"error"`
// This is to make the error more compatible with users that expect errors to be Status objects. It should be
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@achew22 achew22 merged commit 7951e5b into grpc-ecosystem:master Aug 3, 2018
@ti
Copy link
Contributor

ti commented Oct 11, 2018

this is really ugly. how to fix "error" and "message" repeat problem ?
before this code:

{
    "error": "something error",
    "code": 3
}

after:

{
    "error": "something error",
    "message": "something error",
    "code": 3
}

should web restore the code?

@ti
Copy link
Contributor

ti commented Oct 11, 2018

you can just use to fix it before:

type Error struct {
	Message string   `json:"error,omitempty"`

@ffredsh

@johanbrandhorst
Copy link
Collaborator

I agree, but unfortunately we have to keep the old "error" field for backwards compatibility purposes. We'll want to remove it come 2.0.

@ti
Copy link
Contributor

ti commented Oct 11, 2018

@johanbrandhorst web should not remove the "error" field in http response json, the "error" field is defined for compatible for some rfc documents,
For exp:
https://tools.ietf.org/html/rfc6749#page-45

more:
the "error" means some "enmu text" for the front apps, but he "message" just means some error details, if you want add it, you should add the "message" to the 2.0

@ffredsh

@johanbrandhorst
Copy link
Collaborator

I apologize, but you have confused me a bit. Your PR suggested that we should remove the message in 1.x, which is unacceptable as we have releases with it in. Are you now saying we definitely must keep the error field in 2.0 and remove the message again because error is declared as a response field by RFC6749 (Oath Framework)?

@ti
Copy link
Contributor

ti commented Oct 12, 2018

This change if really affect our current restful document for open apis 😢

@ti
Copy link
Contributor

ti commented Oct 12, 2018

@ffredsh

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
…ecosystem#718)

* Add message field to the error message emitted by grpc-gateway to make it more consistent with Status
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants