Skip to content

Commit 5b5d873

Browse files
wasd96040501wasd96040501
committed
feat(generic_http_thrift): fail on nil value for required field
1 parent 4e1dbe9 commit 5b5d873

6 files changed

Lines changed: 102 additions & 16 deletions

File tree

‎pkg/generic/httpthrift_codec.go‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -41,19 +41,20 @@ type HTTPRequest = descriptor.HTTPRequest
4141
type HTTPResponse = descriptor.HTTPResponse
4242

4343
type httpThriftCodec struct {
44-
svcDsc atomic.Value // *idl
45-
provider DescriptorProvider
46-
binaryWithBase64 bool
47-
convOpts conv.Options // used for dynamicgo conversion
48-
convOptsWithThriftBase conv.Options // used for dynamicgo conversion with EnableThriftBase turned on
49-
dynamicgoEnabled bool
50-
useRawBodyForHTTPResp bool
51-
svcName string
44+
svcDsc atomic.Value // *idl
45+
provider DescriptorProvider
46+
binaryWithBase64 bool
47+
convOpts conv.Options // used for dynamicgo conversion
48+
convOptsWithThriftBase conv.Options // used for dynamicgo conversion with EnableThriftBase turned on
49+
dynamicgoEnabled bool
50+
useRawBodyForHTTPResp bool
51+
failOnNilValueForRequiredField bool
52+
svcName string
5253
}
5354

5455
func newHTTPThriftCodec(p DescriptorProvider, opts *Options) *httpThriftCodec {
5556
svc := <-p.Provide()
56-
c := &httpThriftCodec{provider: p, binaryWithBase64: false, dynamicgoEnabled: false, useRawBodyForHTTPResp: opts.useRawBodyForHTTPResp, svcName: svc.Name}
57+
c := &httpThriftCodec{provider: p, binaryWithBase64: false, dynamicgoEnabled: false, useRawBodyForHTTPResp: opts.useRawBodyForHTTPResp, failOnNilValueForRequiredField: opts.failOnNilValueForRequiredField, svcName: svc.Name}
5758
if dp, ok := p.(GetProviderOption); ok && dp.Option().DynamicGoEnabled {
5859
c.dynamicgoEnabled = true
5960

@@ -95,6 +96,7 @@ func (c *httpThriftCodec) configureHTTPRequestWriter(writer *thrift.WriteHTTPReq
9596
if c.dynamicgoEnabled {
9697
writer.SetDynamicGo(&c.convOpts, &c.convOptsWithThriftBase)
9798
}
99+
writer.SetFailOnNilValueForRequiredField(c.failOnNilValueForRequiredField)
98100
}
99101

100102
func (c *httpThriftCodec) configureHTTPResponseReader(reader *thrift.ReadHTTPResponse) {

‎pkg/generic/httpthrift_codec_test.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,41 @@ func TestHttpThriftCodecWithDynamicGo(t *testing.T) {
109109
test.Assert(t, ok)
110110
}
111111

112+
func TestHttpThriftCodecWithFailOnNilValueForRequired(t *testing.T) {
113+
// without dynamicgo
114+
p, err := NewThriftFileProvider("./http_test/idl/binary_echo.thrift")
115+
test.Assert(t, err == nil)
116+
gOpts := &Options{dynamicgoConvOpts: DefaultHTTPDynamicGoConvOpts, failOnNilValueForRequiredField: true}
117+
htc := newHTTPThriftCodec(p, gOpts)
118+
test.Assert(t, !htc.dynamicgoEnabled)
119+
test.Assert(t, !htc.useRawBodyForHTTPResp)
120+
test.Assert(t, htc.failOnNilValueForRequiredField)
121+
test.DeepEqual(t, htc.convOpts, conv.Options{})
122+
test.DeepEqual(t, htc.convOptsWithThriftBase, conv.Options{})
123+
defer htc.Close()
124+
test.Assert(t, htc.Name() == "HttpThrift")
125+
126+
req := &HTTPRequest{Request: getStdHttpRequest()}
127+
// wrong
128+
method, err := htc.getMethod("test")
129+
test.Assert(t, err.Error() == "req is invalid, need descriptor.HTTPRequest" && method == nil)
130+
// right
131+
method, err = htc.getMethod(req)
132+
test.Assert(t, err == nil && method.Name == "BinaryEcho")
133+
test.Assert(t, method.StreamingMode == serviceinfo.StreamingNone)
134+
test.Assert(t, htc.svcName == "ExampleService")
135+
136+
rw := htc.getMessageReaderWriter()
137+
_, ok := rw.(error)
138+
test.Assert(t, !ok)
139+
140+
rw = htc.getMessageReaderWriter()
141+
_, ok = rw.(thrift.MessageWriter)
142+
test.Assert(t, ok)
143+
_, ok = rw.(thrift.MessageReader)
144+
test.Assert(t, ok)
145+
}
146+
112147
func getStdHttpRequest() *http.Request {
113148
body := map[string]interface{}{
114149
"msg": []byte("hello"),

‎pkg/generic/option.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ type Options struct {
4141
dynamicgoConvOpts conv.Options
4242
// flag to set whether to store http resp body into HTTPResponse.RawBody
4343
useRawBodyForHTTPResp bool
44+
// will return error when field is required but input value is nil
45+
failOnNilValueForRequiredField bool
4446
}
4547

4648
type Option struct {
@@ -68,3 +70,10 @@ func UseRawBodyForHTTPResp(enable bool) Option {
6870
opt.useRawBodyForHTTPResp = enable
6971
}}
7072
}
73+
74+
// will return error when field is required but input value is nil
75+
func WithFailOnNilValueForRequiredField(enable bool) Option {
76+
return Option{F: func(opt *Options) {
77+
opt.failOnNilValueForRequiredField = enable
78+
}}
79+
}

‎pkg/generic/thrift/http.go‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,12 @@ func NewHTTPReaderWriter(svc *descriptor.ServiceDescriptor) *HTTPReaderWriter {
4343

4444
// WriteHTTPRequest implement of MessageWriter
4545
type WriteHTTPRequest struct {
46-
svc *descriptor.ServiceDescriptor
47-
binaryWithBase64 bool
48-
convOpts conv.Options // used for dynamicgo conversion
49-
convOptsWithThriftBase conv.Options // used for dynamicgo conversion with EnableThriftBase turned on
50-
dynamicgoEnabled bool
46+
svc *descriptor.ServiceDescriptor
47+
binaryWithBase64 bool
48+
convOpts conv.Options // used for dynamicgo conversion
49+
convOptsWithThriftBase conv.Options // used for dynamicgo conversion with EnableThriftBase turned on
50+
dynamicgoEnabled bool
51+
failOnNilValueForRequiredField bool // will return error when field is required but input value is nil
5152
}
5253

5354
var (
@@ -71,6 +72,10 @@ func (w *WriteHTTPRequest) SetBinaryWithBase64(enable bool) {
7172
w.binaryWithBase64 = enable
7273
}
7374

75+
func (w *WriteHTTPRequest) SetFailOnNilValueForRequiredField(enable bool) {
76+
w.failOnNilValueForRequiredField = enable
77+
}
78+
7479
// SetDynamicGo ...
7580
func (w *WriteHTTPRequest) SetDynamicGo(convOpts, convOptsWithThriftBase *conv.Options) {
7681
w.convOpts = *convOpts
@@ -94,7 +99,7 @@ func (w *WriteHTTPRequest) originalWrite(ctx context.Context, out bufiox.Writer,
9499
requestBase = nil
95100
}
96101
bw := thrift.NewBufferWriter(out)
97-
err = wrapStructWriter(ctx, req, bw, fn.Request, &writerOption{requestBase: requestBase, binaryWithBase64: w.binaryWithBase64})
102+
err = wrapStructWriter(ctx, req, bw, fn.Request, &writerOption{requestBase: requestBase, binaryWithBase64: w.binaryWithBase64, failOnNilValueForRequiredField: w.failOnNilValueForRequiredField})
98103
bw.Recycle()
99104
return err
100105
}

‎pkg/generic/thrift/write.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ type writerOption struct {
3535
requestBase *base.Base // request base from metahandler
3636
// decoding Base64 to binary
3737
binaryWithBase64 bool
38+
// will return error when field is required but input value is nil
39+
failOnNilValueForRequiredField bool
3840
}
3941

4042
type writer func(ctx context.Context, val interface{}, out *thrift.BufferWriter, t *descriptor.TypeDescriptor, opt *writerOption) error
@@ -778,6 +780,9 @@ func writeHTTPRequest(ctx context.Context, val interface{}, out *thrift.BufferWr
778780

779781
if v == nil {
780782
if !field.Optional {
783+
if opt != nil && opt.failOnNilValueForRequiredField {
784+
return fmt.Errorf("value of field [%s] is nil", name)
785+
}
781786
if err := out.WriteFieldBegin(field.Type.Type.ToThriftTType(), int16(field.ID)); err != nil {
782787
return err
783788
}

‎pkg/generic/thrift/write_test.go‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1358,6 +1358,36 @@ func Test_writeHTTPRequest(t *testing.T) {
13581358
},
13591359
false,
13601360
},
1361+
{
1362+
"writeStructRequiredFail",
1363+
args{
1364+
val: &descriptor.HTTPRequest{
1365+
Body: map[string]interface{}{"hello": nil},
1366+
},
1367+
1368+
t: &descriptor.TypeDescriptor{
1369+
Type: descriptor.STRUCT,
1370+
Key: &descriptor.TypeDescriptor{Type: descriptor.STRING},
1371+
Elem: &descriptor.TypeDescriptor{Type: descriptor.STRING},
1372+
Struct: &descriptor.StructDescriptor{
1373+
Name: "Demo",
1374+
FieldsByName: map[string]*descriptor.FieldDescriptor{
1375+
"hello": {
1376+
Name: "hello",
1377+
ID: 1,
1378+
Required: true,
1379+
Type: &descriptor.TypeDescriptor{Type: descriptor.STRING},
1380+
HTTPMapping: descriptor.DefaultNewMapping("hello"),
1381+
},
1382+
},
1383+
},
1384+
},
1385+
opt: &writerOption{
1386+
failOnNilValueForRequiredField: true,
1387+
},
1388+
},
1389+
true,
1390+
},
13611391
}
13621392
for _, tt := range tests {
13631393
t.Run(tt.name, func(t *testing.T) {
@@ -1434,7 +1464,7 @@ func getReqPbBody() (proto.Message, error) {
14341464
path := "main.proto"
14351465
content := `
14361466
package kitex.test.server;
1437-
1467+
14381468
message BizReq {
14391469
optional int32 user_id = 1;
14401470
optional string user_name = 2;

0 commit comments

Comments
 (0)