Revert: status: FromError: return entire error message text for wrapped errors

[XSAM]

Use case(s) - what problem will this feature solve?

The current implementation of status.FromError has two issues:

original status message is changed with errors.Join

Considering this code

package main

import (
	"errors"
	"log/slog"

	"google.golang.org/grpc/status"
)

func main() {
	stats := status.New(400, "status error")
	statusErr := stats.Err()
	wrappedErr := errors.Join(statusErr, errors.New("wrapper"))

	statsFromErr, _ := status.FromError(statusErr)
	statsFromWrappedErr, _ := status.FromError(wrappedErr)

	slog.Info("stats", "msg", stats.Message())
	slog.Info("stats from error", "msg", statsFromErr.Message())
	slog.Info("stats from wrapped error", "msg", statsFromWrappedErr.Message())
}

The output is something like

INFO stats msg="status error"
INFO stats from error msg="status error"
INFO stats from wrapped error msg="rpc error: code = Code(400) desc = status error\nwrapper"

You will see the original message didn't change by calling status.FromError(statusErr), which is expected.

But, the original message status error become rpc error: code = Code(400) desc = status error\nwrapper after joining a wrapper error errors.Join(statusErr, errors.New("wrapper")). The status after this becomes not the original one.

The new message not just include the message wrapper from wrapper (this is strange), but also include the a rendered status string string rpc error: code = Code(400) desc = . I am not sure including a rendered string in the Message field of the status itself make sense.

Even worse, when I called statsFromWrappedErr.String(). I got

rpc error: code = Code(400) desc = rpc error: code = Code(400) desc = status error\nwrapper

This behavior looks strange and has a code smell.

---

For those who relies on this current behavior, they could always use this to append the error string after calling status.FromError.

p := statsFromErr.Proto()
p.Message = statusErr.Error()
newStats := status.FromProto(p)

But, for those who want their original status, they can only fork their own version of status.FormError since the status returned by status.FormError has been contaminated.

Inconsistent behavior

The second issue is about consistency.

While it can retrieve original status from an error that implements grpcstatus interface

grpc-go/status/status.go

Lines 101 to 111 in 4cedec4

	if gs, ok := err.(grpcstatus); ok {
	grpcStatus := gs.GRPCStatus()
	if grpcStatus == nil {
	// Error has status nil, which maps to codes.OK. There
	// is no sensible behavior for this, so we turn it into
	// an error with codes.Unknown and discard the existing
	// status.
	return New(codes.Unknown, err.Error()), false
	}
	return grpcStatus, true
	}

It return a modified status from an error that also implements grpcstatus interface but wrapped

grpc-go/status/status.go

Lines 112 to 125 in 4cedec4

	var gs grpcstatus
	if errors.As(err, &gs) {
	grpcStatus := gs.GRPCStatus()
	if grpcStatus == nil {
	// Error wraps an error that has status nil, which maps
	// to codes.OK. There is no sensible behavior for this,
	// so we turn it into an error with codes.Unknown and
	// discard the existing status.
	return New(codes.Unknown, err.Error()), false
	}
	p := grpcStatus.Proto()
	p.Message = err.Error()
	return status.FromProto(p), true
	}

These two serve a similar purpose (find the status) by different way but returning different results. This seems inconsistent.

Proposed Solution

revert #6150

Additional Context

I am happy to create a PR for this.

[dfawley]

But, the original message status error become rpc error: code = Code(400) desc = status error\nwrapper after joining a wrapper error errors.Join(statusErr, errors.New("wrapper")). The status after this becomes not the original one.

It return a modified status from an error that also implements grpcstatus interface but wrapped

I believe I looked into some other libraries and saw similar behavior before we made this change. It is a bit unfortunate that the new Message contains the status code again, but I don't think there's any other way to get the wrapped error text without that happening.

Aside from theoretical problems or a dislike for the result, do you have any real problems that this creates?

[XSAM]

It is a bit unfortunate that the new Message contains the status code again, but I don't think there's any other way to get the wrapped error text without that happening.

I am not sure why injecting a wrapped error text would become a feature of status.FromError in the first place, especially err.Error() can always return the wrapped error text.

do you have any real problems that this creates?

If a counter-intuitive result and inconsistent behavior are not considered real problems, then I don't have real problems that this creates.