From 7965a6adf1822652818961dc2fec778eed192875 Mon Sep 17 00:00:00 2001 From: Yi Duan Date: Mon, 10 Jul 2023 21:33:54 +0800 Subject: [PATCH] opt: only write StructEnd while `WriteEmpty()` (#28) * opt: only write StructEnd while `WriteEmpty()` * opt: support quoted go.tag * adjust test case --- go.mod | 1 + go.sum | 2 + internal/util/strings.go | 38 +++++++++----- internal/util/strings_test.go | 72 ++++++++++++++++++++++++++ testdata/h2t_conv_test.go | 5 +- thrift/annotation/anno_mapping.go | 9 ++-- thrift/annotation/anno_mapping_test.go | 3 +- thrift/binary.go | 18 +------ 8 files changed, 107 insertions(+), 41 deletions(-) create mode 100644 internal/util/strings_test.go diff --git a/go.mod b/go.mod index 401c05b6..84daaead 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/cloudwego/kitex v0.6.1 github.com/cloudwego/thriftgo v0.2.11 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc + github.com/fatih/structtag v1.2.0 github.com/iancoleman/strcase v0.2.0 github.com/klauspost/cpuid/v2 v2.2.4 github.com/stretchr/testify v1.8.2 diff --git a/go.sum b/go.sum index 88c0045c..8b4e5e28 100644 --- a/go.sum +++ b/go.sum @@ -64,6 +64,8 @@ github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymF github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4= github.com/envoyproxy/go-control-plane v0.9.9-0.20201210154907-fd9021fe5dad/go.mod h1:cXg6YxExXjJnVBQHBLXeUAgxn2UodCpnH306RInaBQk= github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c= +github.com/fatih/structtag v1.2.0 h1:/OdNE99OxoI/PqaW/SuSK9uxxT3f/tcSZgon/ssNSx4= +github.com/fatih/structtag v1.2.0/go.mod h1:mBJUNpUnHmRKrKlQQlmCrh5PuhftFbNv8Ys4/aAZl94= github.com/fogleman/gg v1.2.1-0.20190220221249-0403632d5b90/go.mod h1:R/bRT+9gY/C5z7JzPU0zXsXHKM4/ayA+zqcVNZzPa1k= github.com/fogleman/gg v1.3.0/go.mod h1:R/bRT+9gY/C5z7JzPU0zXsXHKM4/ayA+zqcVNZzPa1k= github.com/go-fonts/dejavu v0.1.0/go.mod h1:4Wt4I4OU2Nq9asgDCteaAaWZOV24E+0/Pwo0gppep4g= diff --git a/internal/util/strings.go b/internal/util/strings.go index 95d165d9..c1e7d5b0 100644 --- a/internal/util/strings.go +++ b/internal/util/strings.go @@ -17,11 +17,10 @@ package util import ( - "fmt" - "strconv" "strings" "github.com/cloudwego/dynamicgo/meta" + "github.com/fatih/structtag" "github.com/iancoleman/strcase" ) @@ -54,18 +53,33 @@ func ToLowerCamelCase(name string) string { } func SplitTagOptions(tag string) (ret []string, err error) { - kv := strings.Trim(tag, " ") - i := strings.Index(kv, ":") - if i <= 0 { - return nil, fmt.Errorf("invalid go tag: %s", tag) - } - ret = append(ret, kv[:i]) - v, err := strconv.Unquote(kv[i+1:]) + tags, err := structtag.Parse(tag) if err != nil { - return ret, err + // try replace `\"` + tag = strings.Replace(tag, `\"`, `"`, -1) + tags, err = structtag.Parse(tag) + if err != nil { + return nil, err + } + } + // kv := strings.Trim(tag, "\n\b\t\r") + // i := strings.Index(kv, ":") + // if i <= 0 { + // return nil, fmt.Errorf("invalid go tag: %s", tag) + // } + // ret = append(ret, kv[:i]) + // val := strings.Trim(kv[i+1:], "\n\b\t\r") + + // v, err := strconv.Unquote(kv[i+1:]) + // if err != nil { + // return ret, err + // } + // vs := strings.Split(v, ",") + // ret = append(ret, vs...) + for _, t := range tags.Tags() { + ret = append(ret, t.Key) + ret = append(ret, t.Name) } - vs := strings.Split(v, ",") - ret = append(ret, vs...) return ret, nil } diff --git a/internal/util/strings_test.go b/internal/util/strings_test.go new file mode 100644 index 00000000..0c62cd00 --- /dev/null +++ b/internal/util/strings_test.go @@ -0,0 +1,72 @@ +/** + * Copyright 2023 CloudWeGo Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package util + +import ( + "reflect" + "testing" +) + +func TestSplitTagOptions(t *testing.T) { + type args struct { + tag string + } + tests := []struct { + name string + args args + wantRet []string + wantErr bool + }{ + { + name: "single", + args: args{ + tag: `json:"k1,omitempty"`, + }, + wantRet: []string{"json", "k1"}, + wantErr: false, + }, + { + name: "multi", + args: args{ + tag: `json:"k1,omitempty" thrift:"k2,xxx"`, + }, + wantRet: []string{"json", "k1", "thrift", "k2"}, + wantErr: false, + }, + { + name: "quoted", + args: args{ + tag: `json:\"k1,omitempty\"`, + }, + wantRet: []string{"json", "k1"}, + wantErr: false, + }, + + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotRet, err := SplitTagOptions(tt.args.tag) + if (err != nil) != tt.wantErr { + t.Errorf("SplitTagOptions() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(gotRet, tt.wantRet) { + t.Errorf("SplitTagOptions() = %v, want %v", gotRet, tt.wantRet) + } + }) + } +} diff --git a/testdata/h2t_conv_test.go b/testdata/h2t_conv_test.go index c03fafb8..ec6d9ab1 100644 --- a/testdata/h2t_conv_test.go +++ b/testdata/h2t_conv_test.go @@ -97,10 +97,7 @@ func TestHTTP2ThriftConv_Do(t *testing.T) { Arguments: general.Struct{ 1: general.Struct{ 1: int32(7749), - 255: general.Struct{ - 1: int32(0), - 2: general.List(nil), - }, + 255: general.Struct{}, }, }, }, diff --git a/thrift/annotation/anno_mapping.go b/thrift/annotation/anno_mapping.go index 573a4919..c1d11f95 100644 --- a/thrift/annotation/anno_mapping.go +++ b/thrift/annotation/anno_mapping.go @@ -44,7 +44,7 @@ func (m goTagMapper) Map(ctx context.Context, anns []parser.Annotation, desc int } switch kv[0] { case "json": - if err := handleGoJSON(kv, &ret); err != nil { + if err := handleGoJSON(kv[1], &ret); err != nil { return nil, nil, err } } @@ -53,13 +53,10 @@ func (m goTagMapper) Map(ctx context.Context, anns []parser.Annotation, desc int return } -func handleGoJSON(kv []string, ret *[]parser.Annotation) error { - if len(kv) < 2 { - return fmt.Errorf("invalid json tag: %v", kv) - } +func handleGoJSON(name string, ret *[]parser.Annotation) error { *ret = append(*ret, parser.Annotation{ Key: APIKeyName, - Values: []string{kv[1]}, + Values: []string{name}, }) return nil } diff --git a/thrift/annotation/anno_mapping_test.go b/thrift/annotation/anno_mapping_test.go index 35a01ab3..09e6b252 100644 --- a/thrift/annotation/anno_mapping_test.go +++ b/thrift/annotation/anno_mapping_test.go @@ -72,8 +72,7 @@ func TestGoTagJSON(t *testing.T) { string ExampleMethod(1: Base req) } `, "ExampleMethod") - require.Error(t, err) - println(err.Error()) + require.NoError(t, err) } func TestApiKey(t *testing.T) { diff --git a/thrift/binary.go b/thrift/binary.go index a1efe7cf..9eb6e24b 100644 --- a/thrift/binary.go +++ b/thrift/binary.go @@ -502,23 +502,7 @@ func (p *BinaryProtocol) WriteEmpty(desc *TypeDescriptor) error { } return p.WriteMapEnd() case STRUCT: - if err := p.WriteStructBegin(""); err != nil { - return err - } - for _, f := range desc.Struct().Fields() { - if f.Required() == OptionalRequireness { - continue - } - if err := p.WriteFieldBegin(f.Name(), f.Type().Type(), f.ID()); err != nil { - return err - } - if err := p.WriteEmpty(f.Type()); err != nil { - return err - } - if err := p.WriteFieldEnd(); err != nil { - return err - } - } + // NOTICE: to avoid self-cycled type dead loop here, just write empty struct return p.WriteStructEnd() default: return errors.New("invalid type")