refactor: get rid of apache TApplicationException (cloudwego#1389)

client/middlewares.go

@@ -20,10 +20,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"reflect"
+	"strings"
 	"time"
 
-	"github.com/apache/thrift/lib/go/thrift"
-
 	"github.com/cloudwego/kitex/internal"
 	"github.com/cloudwego/kitex/pkg/discovery"
 	"github.com/cloudwego/kitex/pkg/endpoint"
@@ -159,13 +159,27 @@ func newIOErrorHandleMW(errHandle func(context.Context, error) error) endpoint.M
 	}
 }
 
+func isRemoteErr(err error) bool {
+	if err == nil {
+		return false
+	}
+	switch err.(type) {
+	// for thrift、KitexProtobuf, actually check *remote.TransError is enough
+	case *remote.TransError, protobuf.PBError:
+		return true
+	default:
+		// case thrift.TApplicationException ?
+		// XXX: we'd like to get rid of apache pkg, should be ok to check by type name
+		// for thrift v0.13.0, it's "*thrift.tApplicationException"
+	}
+	return strings.HasSuffix(reflect.TypeOf(err).String(), "ApplicationException")
+}
+
 // DefaultClientErrorHandler is Default ErrorHandler for client
 // when no ErrorHandler is specified with Option `client.WithErrorHandler`, this ErrorHandler will be injected.
 // for thrift、KitexProtobuf, >= v0.4.0 wrap protocol error to TransError, which will be more friendly.
 func DefaultClientErrorHandler(ctx context.Context, err error) error {
-	switch err.(type) {
-	// for thrift、KitexProtobuf, actually check *remote.TransError is enough
-	case *remote.TransError, thrift.TApplicationException, protobuf.PBError:
+	if isRemoteErr(err) {
 		// Add 'remote' prefix to distinguish with local err.
 		// Because it cannot make sure which side err when decode err happen
 		return kerrors.ErrRemoteOrNetwork.WithCauseAndExtraMsg(err, "remote")
@@ -176,9 +190,7 @@ func DefaultClientErrorHandler(ctx context.Context, err error) error {
 // ClientErrorHandlerWithAddr is ErrorHandler for client, which will add remote addr info into error
 func ClientErrorHandlerWithAddr(ctx context.Context, err error) error {
 	addrStr := getRemoteAddr(ctx)
-	switch err.(type) {
-	// for thrift、KitexProtobuf, actually check *remote.TransError is enough
-	case *remote.TransError, thrift.TApplicationException, protobuf.PBError:
+	if isRemoteErr(err) {
 		// Add 'remote' prefix to distinguish with local err.
 		// Because it cannot make sure which side err when decode err happen
 		extraMsg := "remote"

client/middlewares_test.go

@@ -22,7 +22,6 @@ import (
 	"net"
 	"testing"
 
-	"github.com/apache/thrift/lib/go/thrift"
 	"github.com/golang/mock/gomock"
 
 	"github.com/cloudwego/kitex/internal/mocks"
@@ -33,6 +32,7 @@ import (
 	"github.com/cloudwego/kitex/pkg/endpoint"
 	"github.com/cloudwego/kitex/pkg/event"
 	"github.com/cloudwego/kitex/pkg/kerrors"
+	"github.com/cloudwego/kitex/pkg/protocol/bthrift"
 	"github.com/cloudwego/kitex/pkg/proxy"
 	"github.com/cloudwego/kitex/pkg/remote/codec/protobuf"
 	"github.com/cloudwego/kitex/pkg/remote/trans/nphttp2/status"
@@ -140,32 +140,32 @@ func TestDefaultErrorHandler(t *testing.T) {
 	reqCtx := rpcinfo.NewCtxWithRPCInfo(context.Background(), ri)
 
 	// Test TApplicationException
-	err := DefaultClientErrorHandler(context.Background(), thrift.NewTApplicationException(100, "mock"))
+	err := DefaultClientErrorHandler(context.Background(), bthrift.NewApplicationException(100, "mock"))
 	test.Assert(t, err.Error() == "remote or network error[remote]: mock", err.Error())
-	var te thrift.TApplicationException
+	var te *bthrift.ApplicationException
 	ok := errors.As(err, &te)
 	test.Assert(t, ok)
-	test.Assert(t, te.TypeId() == 100)
+	test.Assert(t, te.TypeID() == 100)
 	// Test TApplicationException with remote addr
-	err = ClientErrorHandlerWithAddr(reqCtx, thrift.NewTApplicationException(100, "mock"))
+	err = ClientErrorHandlerWithAddr(reqCtx, bthrift.NewApplicationException(100, "mock"))
 	test.Assert(t, err.Error() == "remote or network error[remote-"+tcpAddrStr+"]: mock", err.Error())
 	ok = errors.As(err, &te)
 	test.Assert(t, ok)
-	test.Assert(t, te.TypeId() == 100)
+	test.Assert(t, te.TypeID() == 100)
 
 	// Test PbError
 	err = DefaultClientErrorHandler(context.Background(), protobuf.NewPbError(100, "mock"))
 	test.Assert(t, err.Error() == "remote or network error[remote]: mock")
 	var pe protobuf.PBError
 	ok = errors.As(err, &pe)
 	test.Assert(t, ok)
-	test.Assert(t, te.TypeId() == 100)
+	test.Assert(t, te.TypeID() == 100)
 	// Test PbError with remote addr
 	err = ClientErrorHandlerWithAddr(reqCtx, protobuf.NewPbError(100, "mock"))
 	test.Assert(t, err.Error() == "remote or network error[remote-"+tcpAddrStr+"]: mock", err.Error())
 	ok = errors.As(err, &pe)
 	test.Assert(t, ok)
-	test.Assert(t, te.TypeId() == 100)
+	test.Assert(t, te.TypeID() == 100)
 
 	// Test status.Error
 	err = DefaultClientErrorHandler(context.Background(), status.Err(100, "mock"))

client/mocks_test.go

@@ -17,7 +17,7 @@
 package client
 
 import (
-	"github.com/apache/thrift/lib/go/thrift"
+	thrift "github.com/cloudwego/kitex/pkg/protocol/bthrift/apache"
 )
 
 // MockTStruct implements the thrift.TStruct interface.

pkg/protocol/bthrift/exception.go

@@ -29,6 +29,9 @@ type ApplicationException struct {
 	m string
 }
 
+// check interface only. TO BE REMOVED in the future
+var _ thrift.TApplicationException = &ApplicationException{}
+
 // NewApplicationException creates an ApplicationException instance
 func NewApplicationException(t int32, msg string) *ApplicationException {
 	return &ApplicationException{t: t, m: msg}
@@ -40,6 +43,9 @@ func (e *ApplicationException) Msg() string { return e.m }
 // TypeID ...
 func (e *ApplicationException) TypeID() int32 { return e.t }
 
+// TypeId ... for apache ApplicationException compatibility
+func (e *ApplicationException) TypeId() int32 { return e.t }
+
 // BLength returns the len of encoded buffer.
 func (e *ApplicationException) BLength() int {
 	// Msg Field: 1 (type) + 2 (id) + 4(strlen) + len(m)
@@ -48,7 +54,7 @@ func (e *ApplicationException) BLength() int {
 	return (1 + 2 + 4 + len(e.m)) + (1 + 2 + 4) + 1
 }
 
-// Read ...
+// FastRead ...
 func (e *ApplicationException) FastRead(b []byte) (off int, err error) {
 	for i := 0; i < 2; i++ {
 		_, tp, id, l, err := Binary.ReadFieldBegin(b[off:])
@@ -80,7 +86,7 @@ func (e *ApplicationException) FastRead(b []byte) (off int, err error) {
 	return off, nil
 }
 
-// Write ...
+// FastWrite ...
 func (e *ApplicationException) FastWrite(b []byte) (off int) {
 	off += Binary.WriteFieldBegin(b[off:], "", thrift.STRING, 1)
 	off += Binary.WriteString(b[off:], e.m)
@@ -95,6 +101,57 @@ func (e *ApplicationException) FastWriteNocopy(b []byte, binaryWriter BinaryWrit
 	return e.FastWrite(b)
 }
 
+// Read implements Read interface of TStruct
+// it only supports binary protocol.
+// Deprecated: use FastRead instead
+func (e *ApplicationException) Read(in thrift.TProtocol) error {
+	for {
+		_, ttype, id, err := in.ReadFieldBegin()
+		if err != nil {
+			return err
+		}
+		if ttype == thrift.STOP {
+			break
+		}
+		switch {
+		case id == 1 && ttype == thrift.STRING:
+			e.m, err = in.ReadString()
+			if err != nil {
+				return err
+			}
+		case id == 2 && ttype == thrift.I32:
+			e.t, err = in.ReadI32()
+			if err != nil {
+				return err
+			}
+		default:
+			if err = thrift.SkipDefaultDepth(in, ttype); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}
+
+// Write implements Write interface of TStruct
+// it only supports binary protocol.
+// Deprecated: use FastWrite instead
+func (e *ApplicationException) Write(out thrift.TProtocol) error {
+	if err := out.WriteFieldBegin("message", thrift.STRING, 1); err != nil {
+		return err
+	}
+	if err := out.WriteString(e.m); err != nil {
+		return err
+	}
+	if err := out.WriteFieldBegin("type", thrift.I32, 2); err != nil {
+		return err
+	}
+	if err := out.WriteI32(e.t); err != nil {
+		return err
+	}
+	return out.WriteFieldStop()
+}
+
 // originally from github.com/apache/thrift@v0.13.0/lib/go/thrift/exception.go
 var defaultApplicationExceptionMessage = map[int32]string{
 	thrift.UNKNOWN_APPLICATION_EXCEPTION:  "unknown application exception",

pkg/protocol/bthrift/exception_test.go

@@ -37,11 +37,27 @@ func TestApplicationException(t *testing.T) {
 	test.Assert(t, ex2.TypeID() == 1)
 	test.Assert(t, ex2.Msg() == "t1")
 
-	// compatibility test only, can be removed in the future
+	// =================
+	// the code below, it's for compatibility test only.
+	// it can be removed in the future along with Read/Write method
+
 	trans := thrift.NewTMemoryBufferLen(100)
 	proto := thrift.NewTBinaryProtocol(trans, true, true)
-	ex0 := thrift.NewTApplicationException(1, "t1")
-	err = ex0.Write(proto)
+	ex9 := thrift.NewTApplicationException(1, "t1")
+	err = ex9.Write(proto)
+	test.Assert(t, err == nil, err)
+	test.Assert(t, bytes.Equal(b, trans.Bytes()))
+
+	trans = thrift.NewTMemoryBufferLen(100)
+	proto = thrift.NewTBinaryProtocol(trans, true, true)
+	ex3 := NewApplicationException(1, "t1")
+	err = ex3.Write(proto)
 	test.Assert(t, err == nil, err)
 	test.Assert(t, bytes.Equal(b, trans.Bytes()))
+
+	ex4 := NewApplicationException(0, "")
+	err = ex4.Read(proto)
+	test.Assert(t, err == nil, err)
+	test.Assert(t, ex4.TypeID() == 1)
+	test.Assert(t, ex4.Msg() == "t1")
 }

pkg/remote/codec/thrift/thrift.go

@@ -276,11 +276,11 @@ func getValidData(methodName string, message remote.Message) (interface{}, error
 	transErr, isTransErr := data.(*remote.TransError)
 	if !isTransErr {
 		if err, isError := data.(error); isError {
-			encodeErr := thrift.NewTApplicationException(remote.InternalError, err.Error())
+			encodeErr := bthrift.NewApplicationException(remote.InternalError, err.Error())
 			return encodeErr, nil
 		}
 		return nil, errors.New("exception relay need error type data")
 	}
-	encodeErr := thrift.NewTApplicationException(transErr.TypeID(), transErr.Error())
+	encodeErr := bthrift.NewApplicationException(transErr.TypeID(), transErr.Error())
 	return encodeErr, nil
 }

pkg/remote/codec/thrift/thrift_data.go

@@ -110,7 +110,7 @@ func marshalBasicThriftData(ctx context.Context, tProt thrift.TProtocol, data in
 // UnmarshalThriftException decode thrift exception from tProt
 // If your input is []byte, you can wrap it with `NewBinaryProtocol(remote.NewReaderBuffer(buf))`
 func UnmarshalThriftException(tProt thrift.TProtocol) error {
-	exception := thrift.NewTApplicationException(thrift.UNKNOWN_APPLICATION_EXCEPTION, "")
+	exception := bthrift.NewApplicationException(thrift.UNKNOWN_APPLICATION_EXCEPTION, "")
 	if err := exception.Read(tProt); err != nil {
 		return perrors.NewProtocolErrorWithErrMsg(err, fmt.Sprintf("thrift unmarshal Exception failed: %s", err.Error()))
 	}

pkg/remote/codec/thrift/thrift_data_test.go

@@ -24,6 +24,7 @@ import (
 
 	"github.com/cloudwego/kitex/internal/mocks/thrift/fast"
 	"github.com/cloudwego/kitex/internal/test"
+	"github.com/cloudwego/kitex/pkg/protocol/bthrift"
 	thrift "github.com/cloudwego/kitex/pkg/protocol/bthrift/apache"
 	"github.com/cloudwego/kitex/pkg/remote"
 )
@@ -163,7 +164,7 @@ func TestUnmarshalThriftException(t *testing.T) {
 	transport := thrift.NewTMemoryBufferLen(marshalThriftBufferSize)
 	tProt := thrift.NewTBinaryProtocol(transport, true, true)
 	errMessage := "test: invalid protocol"
-	exc := thrift.NewTApplicationException(thrift.INVALID_PROTOCOL, errMessage)
+	exc := bthrift.NewApplicationException(thrift.INVALID_PROTOCOL, errMessage)
 	err := exc.Write(tProt)
 	test.Assert(t, err == nil, err)
 

pkg/remote/codec/thrift/thrift_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/cloudwego/kitex/internal/mocks"
 	mt "github.com/cloudwego/kitex/internal/mocks/thrift/fast"
 	"github.com/cloudwego/kitex/internal/test"
+	"github.com/cloudwego/kitex/pkg/protocol/bthrift"
 	thrift "github.com/cloudwego/kitex/pkg/protocol/bthrift/apache"
 	"github.com/cloudwego/kitex/pkg/remote"
 	netpolltrans "github.com/cloudwego/kitex/pkg/remote/trans/netpoll"
@@ -209,13 +210,13 @@ func TestException(t *testing.T) {
 
 func TestTransErrorUnwrap(t *testing.T) {
 	errMsg := "mock err"
-	transErr := remote.NewTransError(remote.InternalError, thrift.NewTApplicationException(1000, errMsg))
-	uwErr, ok := transErr.Unwrap().(thrift.TApplicationException)
+	transErr := remote.NewTransError(remote.InternalError, bthrift.NewApplicationException(1000, errMsg))
+	uwErr, ok := transErr.Unwrap().(*bthrift.ApplicationException)
 	test.Assert(t, ok)
 	test.Assert(t, uwErr.TypeId() == 1000)
 	test.Assert(t, transErr.Error() == errMsg)
 
-	uwErr2, ok := errors.Unwrap(transErr).(thrift.TApplicationException)
+	uwErr2, ok := errors.Unwrap(transErr).(*bthrift.ApplicationException)
 	test.Assert(t, ok)
 	test.Assert(t, uwErr2.TypeId() == 1000)
 	test.Assert(t, uwErr2.Error() == errMsg)

pkg/utils/thrift.go

@@ -75,7 +75,7 @@ func (t *ThriftMessageCodec) Decode(b []byte, msg thrift.TStruct) (method string
 		return
 	}
 	if msgType == thrift.EXCEPTION {
-		exception := thrift.NewTApplicationException(thrift.UNKNOWN_APPLICATION_EXCEPTION, "")
+		exception := bthrift.NewApplicationException(thrift.UNKNOWN_APPLICATION_EXCEPTION, "")
 		if err = exception.Read(t.tProt); err != nil {
 			return
 		}
@@ -146,6 +146,5 @@ func UnmarshalError(b []byte) error {
 	if _, err := ex.FastRead(b[off:]); err != nil {
 		return err
 	}
-	// XXX: for compatibility, consider to remove it in the future
-	return thrift.NewTApplicationException(ex.TypeID(), ex.Msg())
+	return ex
 }

server/invoke_test.go

@@ -22,10 +22,9 @@ import (
 	"sync/atomic"
 	"testing"
 
-	"github.com/apache/thrift/lib/go/thrift"
-
 	"github.com/cloudwego/kitex/internal/mocks"
 	"github.com/cloudwego/kitex/internal/test"
+	thrift "github.com/cloudwego/kitex/pkg/protocol/bthrift/apache"
 	"github.com/cloudwego/kitex/pkg/remote/trans/invoke"
 	"github.com/cloudwego/kitex/pkg/utils"
 )