From d8a885f39c07a3a907eccbe316c60da96427932d Mon Sep 17 00:00:00 2001 From: Yi Duan Date: Wed, 8 Jan 2025 16:43:34 +0800 Subject: [PATCH] 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 --- conv/j2t/conv_test.go | 66 ++++++++++++------- conv/t2j/conv_test.go | 22 +++++++ conv/t2j/impl.go | 16 ++--- .../workflows => scripts}/go_latest_test.yml | 0 testdata/idl/example3.thrift | 2 +- testdata/idl/example4.thrift | 2 +- thrift/annotation/anno_mapping.go | 12 ++-- thrift/annotation/http_mapping_test.go | 18 ++++- thrift/idl.go | 3 + 9 files changed, 101 insertions(+), 40 deletions(-) rename {.github/workflows => scripts}/go_latest_test.yml (100%) diff --git a/conv/j2t/conv_test.go b/conv/j2t/conv_test.go index 7b85d045..624ad94b 100644 --- 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 index 83966927..19805846 100644 --- 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 index 6e51a018..72f8aeb1 100644 --- 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 b/scripts/go_latest_test.yml similarity index 100% rename from .github/workflows/go_latest_test.yml rename to scripts/go_latest_test.yml diff --git a/testdata/idl/example3.thrift b/testdata/idl/example3.thrift index 561f46ba..6b0a3379 100644 --- 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 index a0e2b2b3..e8c2a746 100644 --- 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 index c1d11f95..acca1c43 100644 --- 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 index b8f90b8b..30659aae 100644 --- 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 index 62885b4f..236000b5 100644 --- 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.