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.

Sign up for GitHub

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

Jump to bottom

Remove "error" field from errors. #1098

Closed
cyantarek opened this issue Dec 14, 2019 · 7 comments
Closed

Remove "error" field from errors. #1098

cyantarek opened this issue Dec 14, 2019 · 7 comments
Labels
breaking change Will require a major version increment help wanted
Milestone
2.0

Comments

@cyantarek
Copy link 

body := &errorBody{
		Error:   s.Message(),
		Message: s.Message(),
		Code:    int32(s.Code()),
		Details: s.Proto().GetDetails(),
}

Here (link)

The Error and Message both field got s.Message(), I think it's not clean to duplicate the same thing. I don't know if there's any special intention behind it.

Please let us know. Thanks.
@cyantarek cyantarek changed the title Why errors have bothe "error" and "message" field that contains redundant data? Why errors have both "error" and "message" field that contains same (redundant) data? Dec 14, 2019
@johanbrandhorst
Copy link
Collaborator

Hi @cyantarek. The message field was added in #718 to make the error output of the gRPC-Gateway consistent with the Google RPC Status definitions JSON representation: https://github.com/googleapis/googleapis/blob/master/google/rpc/status.proto#L80. Unfortunately, because we already had error defined in our responses, we couldn't remove it without breaking backwards compatibility. So we've had to keep both. We hope to be able to remove the error field in a future 2.0 release (though there is currently no roadmap for when such a release might happen).

Hope that explains it!

@johanbrandhorst
Copy link
Collaborator

We want to fix this for v2

@johanbrandhorst johanbrandhorst added this to the 2.0 milestone Apr 20, 2020
@johanbrandhorst johanbrandhorst changed the title Why errors have both "error" and "message" field that contains same (redundant) data? Remove "error" field from errors. Apr 20, 2020
@johanbrandhorst johanbrandhorst added breaking change Will require a major version increment help wanted v2 labels Apr 20, 2020
@johanbrandhorst johanbrandhorst mentioned this issue Apr 20, 2020
Closed
13 tasks
@srenatus
Copy link
Contributor

Since it's a breaking change anyways, are we to just remove the field in internal/errors.proto, renumbering the others?

@johanbrandhorst
Copy link
Collaborator

Sure thing

@johanbrandhorst
Copy link
Collaborator

Actually, not sure we even need errors.proto anymore. We probably will just marshal the status.Status directly.

@srenatus
Copy link
Contributor

I've started looking into this. I'll include the status change. :)

@srenatus srenatus mentioned this issue Apr 26, 2020
Merged
johanbrandhorst pushed a commit that referenced this issue Apr 28, 2020
@srenatus
#1098: remove error field from error messages (#1242)
5ece275 
* runtime/errors: remove 'error' field, use gRPC status instead

Signed-off-by: Stephan Renatus <srenatus@chef.io>

* internal/errors.proto: regenerate

Signed-off-by: Stephan Renatus <srenatus@chef.io>

* protoc-gen-swagger: use google.rpc.Status instead of .grpc.gateway.runtime.Error

Signed-off-by: Stephan Renatus <srenatus@chef.io>

* protoc-gen-swagger: rename AddStreamError -> AddErrorDefs, add to tests

There are probably better ways to write this in than calling
`AddErrorDefs` everywhere.

Signed-off-by: Stephan Renatus <srenatus@chef.io>

* run "bazel run :gazelle"

Signed-off-by: Stephan Renatus <srenatus@chef.io>

* rebuild all the things

Note: I've also run

    find examples -name "*runtime_error*" -delete

before. It seems like those are not captured by any of the *clean make
targets.

Signed-off-by: Stephan Renatus <srenatus@chef.io>

* runtime/errors: fix comment, use status.Convert

Signed-off-by: Stephan Renatus <srenatus@chef.io>

* integration_test: port error status test to proto_error_test.go

Signed-off-by: Stephan Renatus <srenatus@chef.io>
johanbrandhorst added a commit that referenced this issue Apr 30, 2020
@johanbrandhorst
runtime: replace StreamError with status.Status
858b295 
Following on from #1242, this replace the StreamError
with a status.Status type. Also, remove the ability to configure
the stream error handler. The existing handler was specific to the
old type, and we can add something better back in later
if necessary.

Fixes #1098
@johanbrandhorst johanbrandhorst mentioned this issue Apr 30, 2020
Merged
johanbrandhorst added a commit that referenced this issue May 2, 2020
@johanbrandhorst
runtime: replace StreamError with status.Status
53da419 
Following on from #1242, this replace the StreamError
with a status.Status type. Also, remove the ability to configure
the stream error handler. The existing handler was specific to the
old type, and we can add something better back in later
if necessary.

Fixes #1098
@johanbrandhorst
Copy link
Collaborator

This is done in v2.

@idoqo idoqo mentioned this issue Mar 14, 2022
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking change Will require a major version increment help wanted
Projects
None yet
Milestone
2.0
Development

No branches or pull requests
3 participants
@srenatus @johanbrandhorst @cyantarek