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

Drop @type from Connect error detail debug string #688

Merged
merged 5 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions error.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ var (
// The [google.golang.org/genproto/googleapis/rpc/errdetails] package contains a
// variety of Protobuf messages commonly used as error details.
type ErrorDetail struct {
pb *anypb.Any
wireJSON string // preserve human-readable JSON
pbAny *anypb.Any
pbInner proto.Message // if nil, must be extracted from pbAny
wireJSON string // preserve human-readable JSON
}

// NewErrorDetail constructs a new error detail. If msg is an *[anypb.Any] then
Expand All @@ -59,13 +60,13 @@ type ErrorDetail struct {
func NewErrorDetail(msg proto.Message) (*ErrorDetail, error) {
// If it's already an Any, don't wrap it inside another.
if pb, ok := msg.(*anypb.Any); ok {
return &ErrorDetail{pb: pb}, nil
return &ErrorDetail{pbAny: pb}, nil
}
pb, err := anypb.New(msg)
if err != nil {
return nil, err
}
return &ErrorDetail{pb: pb}, nil
return &ErrorDetail{pbAny: pb, pbInner: msg}, nil
}

// Type is the fully-qualified name of the detail's Protobuf message (for
Expand All @@ -79,21 +80,26 @@ func (d *ErrorDetail) Type() string {
//
// If we ever want to support remote registries, we can add an explicit
// `TypeURL` method.
return typeNameFromURL(d.pb.GetTypeUrl())
return typeNameFromURL(d.pbAny.GetTypeUrl())
}

// Bytes returns a copy of the Protobuf-serialized detail.
func (d *ErrorDetail) Bytes() []byte {
out := make([]byte, len(d.pb.GetValue()))
copy(out, d.pb.GetValue())
out := make([]byte, len(d.pbAny.GetValue()))
copy(out, d.pbAny.GetValue())
return out
}

// Value uses the Protobuf runtime's package-global registry to unmarshal the
// Detail into a strongly-typed message. Typically, clients use Go type
// assertions to cast from the proto.Message interface to concrete types.
func (d *ErrorDetail) Value() (proto.Message, error) {
return d.pb.UnmarshalNew()
if d.pbInner != nil {
// We clone it so that if the caller mutates the returned value,
// they don't inadvertently corrupt this error detail value.
return proto.Clone(d.pbInner), nil
}
return d.pbAny.UnmarshalNew()
jhump marked this conversation as resolved.
Show resolved Hide resolved
}

// An Error captures four key pieces of information: a [Code], an underlying Go
Expand Down Expand Up @@ -236,7 +242,7 @@ func (e *Error) Meta() http.Header {
func (e *Error) detailsAsAny() []*anypb.Any {
anys := make([]*anypb.Any, 0, len(e.details))
for _, detail := range e.details {
anys = append(anys, detail.pb)
anys = append(anys, detail.pbAny)
}
return anys
}
Expand Down
25 changes: 18 additions & 7 deletions protocol_connect.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"strings"
"time"

"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/anypb"
)

Expand Down Expand Up @@ -1146,15 +1147,18 @@ func (d *connectWireDetail) MarshalJSON() ([]byte, error) {
Value string `json:"value"`
Debug json.RawMessage `json:"debug,omitempty"`
}{
Type: typeNameFromURL(d.pb.GetTypeUrl()),
Value: base64.RawStdEncoding.EncodeToString(d.pb.GetValue()),
Type: typeNameFromURL(d.pbAny.GetTypeUrl()),
Value: base64.RawStdEncoding.EncodeToString(d.pbAny.GetValue()),
}
// Try to produce debug info, but expect failure when we don't have
// descriptors.
var codec protoJSONCodec
debug, err := codec.Marshal(d.pb)
if err == nil && len(debug) > 2 { // don't bother sending `{}`
wire.Debug = json.RawMessage(debug)
msg, err := d.getInner()
if err == nil {
var codec protoJSONCodec
debug, err := codec.Marshal(msg)
if err == nil {
wire.Debug = debug
}
jhump marked this conversation as resolved.
Show resolved Hide resolved
}
return json.Marshal(wire)
jhump marked this conversation as resolved.
Show resolved Hide resolved
}
Expand All @@ -1175,7 +1179,7 @@ func (d *connectWireDetail) UnmarshalJSON(data []byte) error {
return fmt.Errorf("decode base64: %w", err)
}
*d = connectWireDetail{
pb: &anypb.Any{
pbAny: &anypb.Any{
TypeUrl: wire.Type,
Value: decoded,
},
Expand All @@ -1184,6 +1188,13 @@ func (d *connectWireDetail) UnmarshalJSON(data []byte) error {
return nil
}

func (d *connectWireDetail) getInner() (proto.Message, error) {
if d.pbInner != nil {
return d.pbInner, nil
}
return d.pbAny.UnmarshalNew()
}

type connectWireError struct {
Code Code `json:"code"`
Message string `json:"message,omitempty"`
Expand Down
59 changes: 48 additions & 11 deletions protocol_connect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,67 @@ import (
"time"

"connectrpc.com/connect/internal/assert"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/descriptorpb"
"google.golang.org/protobuf/types/known/durationpb"
)

func TestConnectErrorDetailMarshaling(t *testing.T) {
t.Parallel()
detail, err := NewErrorDetail(durationpb.New(time.Second))
assert.Nil(t, err)
data, err := json.Marshal((*connectWireDetail)(detail))
assert.Nil(t, err)
t.Logf("marshaled error detail: %s", string(data))
testCases := []struct {
name string
errorDetail proto.Message
expectDebug any
}{
{
name: "normal",
errorDetail: &descriptorpb.FieldOptions{
Deprecated: proto.Bool(true),
Jstype: descriptorpb.FieldOptions_JS_STRING.Enum(),
},
expectDebug: map[string]any{
"deprecated": true,
"jstype": "JS_STRING",
},
},
{
name: "well-known type with custom JSON",
errorDetail: durationpb.New(time.Second),
expectDebug: "1s", // special JS representation as duration string
},
}
for _, testCase := range testCases {
testCase := testCase
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()

detail, err := NewErrorDetail(testCase.errorDetail)
assert.Nil(t, err)
data, err := json.Marshal((*connectWireDetail)(detail))
assert.Nil(t, err)
t.Logf("marshaled error detail: %s", string(data))

var unmarshaled connectWireDetail
assert.Nil(t, json.Unmarshal(data, &unmarshaled))
assert.Equal(t, unmarshaled.wireJSON, string(data))
assert.Equal(t, unmarshaled.pb, detail.pb)
var unmarshaled connectWireDetail
assert.Nil(t, json.Unmarshal(data, &unmarshaled))
assert.Equal(t, unmarshaled.wireJSON, string(data))
assert.Equal(t, unmarshaled.pbAny, detail.pbAny)

var extractDetails struct {
Debug any `json:"debug"`
}
assert.Nil(t, json.Unmarshal(data, &extractDetails))
assert.Equal(t, extractDetails.Debug, testCase.expectDebug)
})
}
}

func TestConnectErrorDetailMarshalingNoDescriptor(t *testing.T) {
t.Parallel()
raw := `{"type":"acme.user.v1.User","value":"DEADBF",` +
`"debug":{"@type":"acme.user.v1.User","email":"someone@connectrpc.com"}}`
`"debug":{"email":"someone@connectrpc.com"}}`
var detail connectWireDetail
assert.Nil(t, json.Unmarshal([]byte(raw), &detail))
assert.Equal(t, detail.pb.GetTypeUrl(), defaultAnyResolverPrefix+"acme.user.v1.User")
assert.Equal(t, detail.pbAny.GetTypeUrl(), defaultAnyResolverPrefix+"acme.user.v1.User")

_, err := (*ErrorDetail)(&detail).Value()
assert.NotNil(t, err)
Expand Down
2 changes: 1 addition & 1 deletion protocol_grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -793,7 +793,7 @@ func grpcErrorFromTrailer(protobuf Codec, trailer http.Header) *Error {
return errorf(CodeInternal, "server returned invalid protobuf for error details: %w", err)
}
for _, d := range status.GetDetails() {
retErr.details = append(retErr.details, &ErrorDetail{pb: d})
retErr.details = append(retErr.details, &ErrorDetail{pbAny: d})
}
// Prefer the Protobuf-encoded data to the headers (grpc-go does this too).
retErr.code = Code(status.GetCode())
Expand Down
Loading