fix:(thrift) only enable api.body fast-path on demands (#84)

* fix:(thrift) `api.body` shoudn't change field alias by default (for json-generic) * fix test * test * test * add test * ci: remove go_latest test
cloudwego · Jan 8, 2025 · d8a885f · d8a885f
1 parent 55704ea
commit d8a885f
Show file tree

Hide file tree

Showing 9 changed files with 101 additions and 40 deletions.
diff --git a/conv/j2t/conv_test.go b/conv/j2t/conv_test.go
@@ -552,29 +552,51 @@ func TestNullJSON2Thrift(t *testing.T) {
 }
 
 func TestApiBody(t *testing.T) {
-	desc := getExampleDescByName("ApiBodyMethod", true, thrift.Options{})
-	data := []byte(`{"code":1024,"InnerCode":{}}`)
-	cv := NewBinaryConv(conv.Options{
-		EnableHttpMapping:            true,
-		WriteDefaultField:            true,
-		ReadHttpValueFallback:        true,
-		TracebackRequredOrRootFields: true,
+	t.Run("http", func(t *testing.T) {
+		desc := getExampleDescByName("ApiBodyMethod", true, thrift.Options{
+			ApiBodyFastPath: true,
+		})
+		data := []byte(`{"code":1024,"Code":2048,"InnerCode":{}}`)
+		cv := NewBinaryConv(conv.Options{
+			EnableHttpMapping:            true,
+			WriteDefaultField:            true,
+			ReadHttpValueFallback:        true,
+			TracebackRequredOrRootFields: true,
+		})
+		ctx := context.Background()
+		req, err := stdh.NewRequest("POST", "http://localhost:8888/root", bytes.NewBuffer(data))
+		require.Nil(t, err)
+		req.Header.Set("Content-Type", "application/json")
+		r, _ := http.NewHTTPRequestFromStdReq(req)
+		ctx = context.WithValue(ctx, conv.CtxKeyHTTPRequest, r)
+		out, err := cv.Do(ctx, desc, data)
+		require.Nil(t, err)
+		act := example3.NewExampleApiBody()
+		_, err = act.FastRead(out)
+		require.Nil(t, err)
+		require.Equal(t, int64(1024), act.Code)
+		require.Equal(t, int16(1024), act.Code2)
+		require.Equal(t, int64(1024), act.InnerCode.C1)
+		require.Equal(t, int16(0), act.InnerCode.C2)
+	})
+	t.Run("not http", func(t *testing.T) {
+		desc := getExampleDescByName("ApiBodyMethod", true, thrift.Options{
+			ApiBodyFastPath: false,
+		})
+		data := []byte(`{"code":1024,"Code":2048,"InnerCode":{"C1":1,"code":2}}`)
+		cv := NewBinaryConv(conv.Options{
+			WriteDefaultField: true,
+		})
+		out, err := cv.Do(context.Background(), desc, data)
+		require.Nil(t, err)
+		act := example3.NewExampleApiBody()
+		_, err = act.FastRead(out)
+		require.Nil(t, err)
+		require.Equal(t, int64(2048), act.Code)
+		require.Equal(t, int16(1024), act.Code2)
+		require.Equal(t, int64(1), act.InnerCode.C1)
+		require.Equal(t, int16(2), act.InnerCode.C2)
 	})
-	ctx := context.Background()
-	req, err := stdh.NewRequest("POST", "http://localhost:8888/root", bytes.NewBuffer(data))
-	require.Nil(t, err)
-	req.Header.Set("Content-Type", "application/json")
-	r, err := http.NewHTTPRequestFromStdReq(req)
-	ctx = context.WithValue(ctx, conv.CtxKeyHTTPRequest, r)
-	out, err := cv.Do(ctx, desc, data)
-	require.Nil(t, err)
-	act := example3.NewExampleApiBody()
-	_, err = act.FastRead(out)
-	require.Nil(t, err)
-	require.Equal(t, int64(1024), act.Code)
-	require.Equal(t, int16(1024), act.Code2)
-	require.Equal(t, int64(1024), act.InnerCode.C1)
-	require.Equal(t, int16(0), act.InnerCode.C2)
 }
 
 func TestError(t *testing.T) {

diff --git a/conv/t2j/conv_test.go b/conv/t2j/conv_test.go
@@ -285,6 +285,28 @@ func TestAGWBodyDynamic(t *testing.T) {
 	require.Equal(t, (`{"Int64":1,"Xjson":"{\"b\":1}"}`), string(out))
 }
 
+func TestAPIBody(t *testing.T) {
+	cv := NewBinaryConv(conv.Options{
+		EnableValueMapping: true,
+	})
+	desc := GetDescByName("ApiBodyMethod", false)
+	exp := example3.NewExampleApiBody()
+	exp.Code = 1
+	exp.Code2 = 2
+	exp.InnerCode = new(example3.InnerCode)
+	exp.InnerCode.C1 = 31
+	exp.InnerCode.C2 = 32
+	exp.InnerCode.C3 = []*example3.InnerCode{
+		{C1: 41, C2: 42},
+	}
+	ctx := context.Background()
+	in := make([]byte, exp.BLength())
+	_ = exp.FastWriteNocopy(in, nil)
+	out, err := cv.Do(ctx, desc, in)
+	require.NoError(t, err)
+	require.Equal(t, `{"Code":1,"code":2,"InnerCode":{"C1":31,"code":32,"C3":[{"C1":41,"code":42,"C3":[]}]}}`, string(out))
+}
+
 func TestException(t *testing.T) {
 	cv := NewBinaryConv(conv.Options{
 		ConvertException: true,

diff --git a/conv/t2j/impl.go b/conv/t2j/impl.go
@@ -73,7 +73,7 @@ func (self *BinaryConv) readResponseBase(ctx context.Context, p *thrift.BinaryPr
 func (self *BinaryConv) do(ctx context.Context, src []byte, desc *thrift.TypeDescriptor, out *[]byte, resp http.ResponseSetter) (err error) {
 	//NOTICE: output buffer must be larger than src buffer
 	rt.GuardSlice(out, len(src)*_GUARD_SLICE_FACTOR)
-	
+
 	var p = thrift.BinaryProtocol{
 		Buf: src,
 	}
@@ -395,7 +395,7 @@ func (self *BinaryConv) handleUnsets(b *thrift.RequiresBitmap, desc *thrift.Stru
 		var ok = false
 		if hms := field.HTTPMappings(); self.opts.EnableHttpMapping && hms != nil {
 			// make a default thrift value
-			p := thrift.BinaryProtocol{Buf: make([]byte, 0, conv.DefaulHttpValueBufferSizeForJSON)};
+			p := thrift.BinaryProtocol{Buf: make([]byte, 0, conv.DefaulHttpValueBufferSizeForJSON)}
 			if err := p.WriteDefaultOrEmpty(field); err != nil {
 				return wrapError(meta.ErrWrite, fmt.Sprintf("encoding field '%s' default value failed", field.Name()), err)
 			}
@@ -517,10 +517,10 @@ func (self *BinaryConv) writeHttpValue(ctx context.Context, resp http.ResponseSe
 	var thriftVal []byte
 	var jsonVal []byte
 	var textVal []byte
-	
+
 	for _, hm := range field.HTTPMappings() {
 		var val []byte
-		enc := hm.Encoding();
+		enc := hm.Encoding()
 
 		if enc == meta.EncodingThriftBinary {
 			// raw encoding, check if raw value is set
@@ -543,10 +543,10 @@ func (self *BinaryConv) writeHttpValue(ctx context.Context, resp http.ResponseSe
 					return false, unwrapError(fmt.Sprintf("reading thrift value of '%s' failed, thrift pos:%d", field.Name(), p.Read), err)
 				}
 				val = tmp
-				textVal = val 
+				textVal = val
 			} else {
 				val = textVal
-			} 
+			}
 		} else if self.opts.UseKitexHttpEncoding {
 			// kitex http encoding fallback
 			if textVal == nil {
@@ -558,7 +558,7 @@ func (self *BinaryConv) writeHttpValue(ctx context.Context, resp http.ResponseSe
 				val = textVal
 			} else {
 				val = textVal
-			} 
+			}
 		} else if enc == meta.EncodingJSON {
 			// for nested type, convert it to a new JSON string
 			if jsonVal == nil {
@@ -572,7 +572,7 @@ func (self *BinaryConv) writeHttpValue(ctx context.Context, resp http.ResponseSe
 			} else {
 				val = jsonVal
 			}
-			
+
 		} else {
 			return false, wrapError(meta.ErrConvert, fmt.Sprintf("unsuported http-value encoding %v of field '%s'", enc, field.Name()), nil)
 		}

diff --git a/.github/workflows/go_latest_test.yml → scripts/go_latest_test.yml b/.github/workflows/go_latest_test.yml → scripts/go_latest_test.yml
diff --git a/testdata/idl/example3.thrift b/testdata/idl/example3.thrift
@@ -49,7 +49,7 @@ struct ExampleReq {
 }
 
 struct ExampleResp {
-    1: string Msg (api.body = "msg"),
+    1: string Msg (api.key = "msg"),
     2: optional double Cookie (api.cookie = "cookie"),
     3: required i32 Status (api.http_code = "status"),
     4: optional bool Header (api.header = "heeader"),

diff --git a/testdata/idl/example4.thrift b/testdata/idl/example4.thrift
@@ -11,7 +11,7 @@ struct GetFavoriteReq {
     255: required Favorite Base,
 }
 struct GetFavoriteResp {
-    1: required Favorite Favorite (api.body="favorite"),
+    1: required Favorite Favorite (api.key="favorite"),
     255: required base.BaseResp BaseResp,
 }
 

diff --git a/thrift/annotation/anno_mapping.go b/thrift/annotation/anno_mapping.go
@@ -71,13 +71,13 @@ func (m apiBodyMapper) Map(ctx context.Context, anns []parser.Annotation, desc i
 		if len(ann.Values) != 1 {
 			return nil, nil, errors.New("api.body must have a value")
 		}
-		ret = append(ret, parser.Annotation{
-			Key:    APIKeyName,
-			Values: []string{ann.Values[0]},
-		})
 		isRoot := ctx.Value(thrift.CtxKeyIsBodyRoot)
-		// special fast-path: if the field is at body root, we don't need to add api.body
-		if isRoot != nil && isRoot.(bool) {
+		// special fast-path: if the field is at body root, we don't need to add api.body to call GetMapBody()
+		if opts.ApiBodyFastPath && isRoot != nil && isRoot.(bool) {
+			ret = append(ret, parser.Annotation{
+				Key:    APIKeyName,
+				Values: []string{ann.Values[0]},
+			})
 			continue
 		} else {
 			ret = append(ret, parser.Annotation{

diff --git a/thrift/annotation/http_mapping_test.go b/thrift/annotation/http_mapping_test.go
@@ -36,6 +36,18 @@ func GetDescFromContent(content string, method string) (*thrift.FunctionDescript
 	return p.Functions()[method], nil
 }
 
+func GetDescFromContentWithOptions(content string, method string, opts thrift.Options) (*thrift.FunctionDescriptor, error) {
+	path := "a/b/main.thrift"
+	includes := map[string]string{
+		path: content,
+	}
+	p, err := opts.NewDescritorFromContent(context.Background(), path, content, includes, true)
+	if err != nil {
+		return nil, err
+	}
+	return p.Functions()[method], nil
+}
+
 func TestAPIQuery(t *testing.T) {
 	fn, err := GetDescFromContent(`
 	namespace go kitex.test.server
@@ -81,7 +93,7 @@ func TestAPIRawUri(t *testing.T) {
 }
 
 func TestAPIBody(t *testing.T) {
-	fn, err := GetDescFromContent(`
+	fn, err := GetDescFromContentWithOptions(`
 	namespace go kitex.test.server
 	struct Base {
 		1: required Base2 f1
@@ -95,7 +107,9 @@ func TestAPIBody(t *testing.T) {
 	service InboxService {
 		string ExampleMethod(1: Base req)
 	}
-	`, "ExampleMethod")
+	`, "ExampleMethod", thrift.Options{
+		ApiBodyFastPath: true,
+	})
 	if err != nil {
 		t.Fatal(err)
 	}

diff --git a/thrift/idl.go b/thrift/idl.go
@@ -94,6 +94,9 @@ type Options struct {
 	//      `// path := /a/b/c.thrift` will got ["/a/b/c.thrift"]
 	// NOTICE: at present, only StructDescriptor.Annotations() can get this
 	PutThriftFilenameToAnnotation bool
+
+	// ApiBodyFastPath indicates `api.body` will change alias-name of root field, which can avoid search http-body on them
+	ApiBodyFastPath bool
 }
 
 // NewDefaultOptions creates a default Options.