From 246ded3df25b03d5ff1e64c952fb89e80b3c2c5a Mon Sep 17 00:00:00 2001 From: Ilya Ozherelyev Date: Thu, 7 Nov 2024 15:46:01 +0100 Subject: [PATCH 1/7] [common] Add unit test for convertions --- internal/common/convert.go | 27 ++++---- internal/common/convert_test.go | 36 +++++++++++ internal/common/thrift_util.go | 54 ++++------------ internal/common/thrift_util_test.go | 96 +++++++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 53 deletions(-) create mode 100644 internal/common/convert_test.go create mode 100644 internal/common/thrift_util_test.go diff --git a/internal/common/convert.go b/internal/common/convert.go index 0646de53f..2f30788c7 100644 --- a/internal/common/convert.go +++ b/internal/common/convert.go @@ -38,55 +38,60 @@ func Int64Ceil(v float64) int64 { // Int32Ptr makes a copy and returns the pointer to an int32. func Int32Ptr(v int32) *int32 { - return &v + return PtrOf(v) } // Float64Ptr makes a copy and returns the pointer to a float64. func Float64Ptr(v float64) *float64 { - return &v + return PtrOf(v) } // Int64Ptr makes a copy and returns the pointer to an int64. func Int64Ptr(v int64) *int64 { - return &v + return PtrOf(v) } // StringPtr makes a copy and returns the pointer to a string. func StringPtr(v string) *string { - return &v + return PtrOf(v) } // BoolPtr makes a copy and returns the pointer to a string. func BoolPtr(v bool) *bool { - return &v + return PtrOf(v) } // TaskListPtr makes a copy and returns the pointer to a TaskList. func TaskListPtr(v s.TaskList) *s.TaskList { - return &v + return PtrOf(v) } // DecisionTypePtr makes a copy and returns the pointer to a DecisionType. func DecisionTypePtr(t s.DecisionType) *s.DecisionType { - return &t + return PtrOf(t) } // EventTypePtr makes a copy and returns the pointer to a EventType. func EventTypePtr(t s.EventType) *s.EventType { - return &t + return PtrOf(t) } // QueryTaskCompletedTypePtr makes a copy and returns the pointer to a QueryTaskCompletedType. func QueryTaskCompletedTypePtr(t s.QueryTaskCompletedType) *s.QueryTaskCompletedType { - return &t + return PtrOf(t) } // TaskListKindPtr makes a copy and returns the pointer to a TaskListKind. func TaskListKindPtr(t s.TaskListKind) *s.TaskListKind { - return &t + return PtrOf(t) } // QueryResultTypePtr makes a copy and returns the pointer to a QueryResultType. func QueryResultTypePtr(t s.QueryResultType) *s.QueryResultType { - return &t + return PtrOf(t) +} + +// PtrOf makes a copy and returns the pointer to a value. +func PtrOf[T any](v T) *T { + return &v } diff --git a/internal/common/convert_test.go b/internal/common/convert_test.go new file mode 100644 index 000000000..dee93d443 --- /dev/null +++ b/internal/common/convert_test.go @@ -0,0 +1,36 @@ +package common + +import ( + s "go.uber.org/cadence/.gen/go/shared" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestPtrOf(t *testing.T) { + assert.Equal(t, "a", *PtrOf("a")) + assert.Equal(t, 1, *PtrOf(1)) + assert.Equal(t, int32(1), *PtrOf(int32(1))) + assert.Equal(t, int64(1), *PtrOf(int64(1))) + assert.Equal(t, float64(1.1), *PtrOf(float64(1.1))) + assert.Equal(t, true, *PtrOf(true)) +} + +func TestPtrHelpers(t *testing.T) { + assert.Equal(t, int32(1), *Int32Ptr(1)) + assert.Equal(t, int64(1), *Int64Ptr(1)) + assert.Equal(t, float64(1.1), *Float64Ptr(1.1)) + assert.Equal(t, true, *BoolPtr(true)) + assert.Equal(t, "a", *StringPtr("a")) + assert.Equal(t, s.TaskList{Name: PtrOf("a")}, *TaskListPtr(s.TaskList{Name: PtrOf("a")})) + assert.Equal(t, s.DecisionTypeScheduleActivityTask, *DecisionTypePtr(s.DecisionTypeScheduleActivityTask)) + assert.Equal(t, s.EventTypeWorkflowExecutionStarted, *EventTypePtr(s.EventTypeWorkflowExecutionStarted)) + assert.Equal(t, s.QueryTaskCompletedTypeCompleted, *QueryTaskCompletedTypePtr(s.QueryTaskCompletedTypeCompleted)) + assert.Equal(t, s.TaskListKindNormal, *TaskListKindPtr(s.TaskListKindNormal)) + assert.Equal(t, s.QueryResultTypeFailed, *QueryResultTypePtr(s.QueryResultTypeFailed)) +} + +func TestCeilHelpers(t *testing.T) { + assert.Equal(t, int32(2), Int32Ceil(1.1)) + assert.Equal(t, int64(2), Int64Ceil(1.1)) +} diff --git a/internal/common/thrift_util.go b/internal/common/thrift_util.go index 34763293f..0e4a16c10 100644 --- a/internal/common/thrift_util.go +++ b/internal/common/thrift_util.go @@ -27,19 +27,13 @@ import ( "github.com/apache/thrift/lib/go/thrift" ) -// TSerialize is used to serialize thrift TStruct to []byte -func TSerialize(ctx context.Context, t thrift.TStruct) (b []byte, err error) { - return thrift.NewTSerializer().Write(ctx, t) -} - // TListSerialize is used to serialize list of thrift TStruct to []byte -func TListSerialize(ts []thrift.TStruct) (b []byte, err error) { +func TListSerialize(ts []thrift.TStruct) ([]byte, error) { if ts == nil { - return + return nil, nil } t := thrift.NewTSerializer() - t.Transport.Reset() // NOTE: we don't write any markers as thrift by design being a streaming protocol doesn't // recommend writing length. @@ -48,26 +42,11 @@ func TListSerialize(ts []thrift.TStruct) (b []byte, err error) { ctx := context.Background() for _, v := range ts { if e := v.Write(ctx, t.Protocol); e != nil { - err = thrift.PrependError("error writing TStruct: ", e) - return + return nil, thrift.PrependError("error writing TStruct: ", e) } } - if err = t.Protocol.Flush(ctx); err != nil { - return - } - - if err = t.Transport.Flush(ctx); err != nil { - return - } - - b = t.Transport.Bytes() - return -} - -// TDeserialize is used to deserialize []byte to thrift TStruct -func TDeserialize(ctx context.Context, t thrift.TStruct, b []byte) (err error) { - return thrift.NewTDeserializer().Read(ctx, t, b) + return t.Transport.Bytes(), t.Protocol.Flush(ctx) } // TListDeserialize is used to deserialize []byte to list of thrift TStruct @@ -94,13 +73,8 @@ func TListDeserialize(ts []thrift.TStruct, b []byte) (err error) { func IsUseThriftEncoding(objs []interface{}) bool { // NOTE: our criteria to use which encoder is simple if all the types are serializable using thrift then we use // thrift encoder. For everything else we default to gob. - - if len(objs) == 0 { - return false - } - - for i := 0; i < len(objs); i++ { - if !IsThriftType(objs[i]) { + for _, obj := range objs { + if !IsThriftType(obj) { return false } } @@ -111,14 +85,9 @@ func IsUseThriftEncoding(objs []interface{}) bool { func IsUseThriftDecoding(objs []interface{}) bool { // NOTE: our criteria to use which encoder is simple if all the types are de-serializable using thrift then we use // thrift decoder. For everything else we default to gob. - - if len(objs) == 0 { - return false - } - - for i := 0; i < len(objs); i++ { - rVal := reflect.ValueOf(objs[i]) - if rVal.Kind() != reflect.Ptr || !IsThriftType(reflect.Indirect(rVal).Interface()) { + for _, obj := range objs { + rVal := reflect.ValueOf(obj) + if rVal.Kind() != reflect.Ptr || !IsThriftType(obj) { return false } } @@ -133,6 +102,7 @@ func IsThriftType(v interface{}) bool { if reflect.ValueOf(v).Kind() != reflect.Ptr { return false } - t := reflect.TypeOf((*thrift.TStruct)(nil)).Elem() - return reflect.TypeOf(v).Implements(t) + return reflect.TypeOf(v).Implements(tStructType) } + +var tStructType = reflect.TypeOf((*thrift.TStruct)(nil)).Elem() diff --git a/internal/common/thrift_util_test.go b/internal/common/thrift_util_test.go new file mode 100644 index 000000000..aac10ae94 --- /dev/null +++ b/internal/common/thrift_util_test.go @@ -0,0 +1,96 @@ +package common + +import ( + "context" + "testing" + + "github.com/apache/thrift/lib/go/thrift" + "github.com/stretchr/testify/assert" +) + +func TestTListSerialize(t *testing.T) { + t.Run("nil", func(t *testing.T) { + data, err := TListSerialize(nil) + assert.NoError(t, err) + assert.Nil(t, data) + }) + t.Run("normal", func(t *testing.T) { + ts := []thrift.TStruct{ + &mockThriftStruct{Field1: "value1", Field2: 1}, + &mockThriftStruct{Field1: "value2", Field2: 2}, + } + + _, err := TListSerialize(ts) + assert.NoError(t, err) + }) +} + +func TestTListDeserialize(t *testing.T) { + ts := []thrift.TStruct{ + &mockThriftStruct{}, + &mockThriftStruct{}, + } + + data, err := TListSerialize(ts) + assert.NoError(t, err) + + err = TListDeserialize(ts, data) + assert.NoError(t, err) +} + +func TestIsUseThriftEncoding(t *testing.T) { + ts := []interface{}{ + &mockThriftStruct{}, + &mockThriftStruct{}, + } + + result := IsUseThriftEncoding(ts) + assert.True(t, result) + + ts = []interface{}{ + &mockThriftStruct{}, + "string", + } + + result = IsUseThriftEncoding(ts) + assert.False(t, result) +} + +func TestIsUseThriftDecoding(t *testing.T) { + ts := []interface{}{ + &mockThriftStruct{}, + &mockThriftStruct{}, + } + + assert.True(t, IsUseThriftDecoding(ts)) + + ts = []interface{}{ + &mockThriftStruct{}, + "string", + } + + assert.False(t, IsUseThriftDecoding(ts)) +} + +func TestIsThriftType(t *testing.T) { + assert.True(t, IsThriftType(&mockThriftStruct{})) + + assert.False(t, IsThriftType(mockThriftStruct{})) +} + +type mockThriftStruct struct { + Field1 string + Field2 int +} + +func (m *mockThriftStruct) Read(ctx context.Context, iprot thrift.TProtocol) error { + return nil +} + +func (m *mockThriftStruct) Write(ctx context.Context, oprot thrift.TProtocol) error { + return nil +} + +func (m *mockThriftStruct) String() string { + return "" +} From dc73a110d99a7ad8f644c40b102345cc3f174055 Mon Sep 17 00:00:00 2001 From: Ilya Ozherelyev Date: Thu, 7 Nov 2024 15:55:20 +0100 Subject: [PATCH 2/7] simplify the change --- internal/common/thrift_util.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/common/thrift_util.go b/internal/common/thrift_util.go index 0e4a16c10..90ecc334f 100644 --- a/internal/common/thrift_util.go +++ b/internal/common/thrift_util.go @@ -86,8 +86,7 @@ func IsUseThriftDecoding(objs []interface{}) bool { // NOTE: our criteria to use which encoder is simple if all the types are de-serializable using thrift then we use // thrift decoder. For everything else we default to gob. for _, obj := range objs { - rVal := reflect.ValueOf(obj) - if rVal.Kind() != reflect.Ptr || !IsThriftType(obj) { + if !IsThriftType(obj) { return false } } From 8caf34badb31992863c9cdc70253d872fa02529e Mon Sep 17 00:00:00 2001 From: Ilya Ozherelyev Date: Thu, 7 Nov 2024 16:34:18 +0100 Subject: [PATCH 3/7] fmt and copyright --- internal/common/convert_test.go | 23 ++++++++++++++++++++++- internal/common/thrift_util_test.go | 20 ++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/internal/common/convert_test.go b/internal/common/convert_test.go index dee93d443..9569a2154 100644 --- a/internal/common/convert_test.go +++ b/internal/common/convert_test.go @@ -1,9 +1,30 @@ +// Copyright (c) 2017-2021 Uber Technologies Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + package common import ( - s "go.uber.org/cadence/.gen/go/shared" "testing" + s "go.uber.org/cadence/.gen/go/shared" + "github.com/stretchr/testify/assert" ) diff --git a/internal/common/thrift_util_test.go b/internal/common/thrift_util_test.go index aac10ae94..95c57591e 100644 --- a/internal/common/thrift_util_test.go +++ b/internal/common/thrift_util_test.go @@ -1,3 +1,23 @@ +// Copyright (c) 2017-2021 Uber Technologies Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + package common import ( From 3acf602bffabc488cf69a9f47c2fc8d1bf1b239e Mon Sep 17 00:00:00 2001 From: Ilya Ozherelyev Date: Thu, 7 Nov 2024 18:51:54 +0100 Subject: [PATCH 4/7] revert IsUseThriftDecoding --- internal/common/thrift_util.go | 3 ++- internal/common/thrift_util_test.go | 24 ++++++++++++++---------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/internal/common/thrift_util.go b/internal/common/thrift_util.go index 90ecc334f..15c454e03 100644 --- a/internal/common/thrift_util.go +++ b/internal/common/thrift_util.go @@ -86,7 +86,8 @@ func IsUseThriftDecoding(objs []interface{}) bool { // NOTE: our criteria to use which encoder is simple if all the types are de-serializable using thrift then we use // thrift decoder. For everything else we default to gob. for _, obj := range objs { - if !IsThriftType(obj) { + rVal := reflect.ValueOf(obj) + if rVal.Kind() != reflect.Ptr || !IsThriftType(reflect.Indirect(rVal).Interface()) { return false } } diff --git a/internal/common/thrift_util_test.go b/internal/common/thrift_util_test.go index 95c57591e..775866151 100644 --- a/internal/common/thrift_util_test.go +++ b/internal/common/thrift_util_test.go @@ -77,19 +77,23 @@ func TestIsUseThriftEncoding(t *testing.T) { } func TestIsUseThriftDecoding(t *testing.T) { - ts := []interface{}{ - &mockThriftStruct{}, - &mockThriftStruct{}, - } + t.Run("success", func(t *testing.T) { + ts := []interface{}{ + &mockThriftStruct{}, + &mockThriftStruct{}, + } - assert.True(t, IsUseThriftDecoding(ts)) + assert.True(t, IsUseThriftDecoding(ts)) + }) - ts = []interface{}{ - &mockThriftStruct{}, - "string", - } + t.Run("fail", func(t *testing.T) { + ts := []interface{}{ + &mockThriftStruct{}, + "string", + } - assert.False(t, IsUseThriftDecoding(ts)) + assert.False(t, IsUseThriftDecoding(ts)) + }) } func TestIsThriftType(t *testing.T) { From 5501cc57534783d4aa10c5203b8deda03c88d538 Mon Sep 17 00:00:00 2001 From: Ilya Ozherelyev Date: Thu, 7 Nov 2024 19:12:28 +0100 Subject: [PATCH 5/7] fix the test --- internal/common/thrift_util_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/common/thrift_util_test.go b/internal/common/thrift_util_test.go index 775866151..e6e705e43 100644 --- a/internal/common/thrift_util_test.go +++ b/internal/common/thrift_util_test.go @@ -78,9 +78,11 @@ func TestIsUseThriftEncoding(t *testing.T) { func TestIsUseThriftDecoding(t *testing.T) { t.Run("success", func(t *testing.T) { + str1 := &mockThriftStruct{} + str2 := &mockThriftStruct{} ts := []interface{}{ - &mockThriftStruct{}, - &mockThriftStruct{}, + &str1, + &str2, } assert.True(t, IsUseThriftDecoding(ts)) From d727131d60464993072937e388afea84b17922b3 Mon Sep 17 00:00:00 2001 From: Ilya Ozherelyev Date: Fri, 8 Nov 2024 11:41:48 +0100 Subject: [PATCH 6/7] address review comments --- internal/common/convert_test.go | 2 +- internal/common/thrift_util.go | 6 ++ internal/common/thrift_util_test.go | 94 +++++++++++++++++++---------- 3 files changed, 68 insertions(+), 34 deletions(-) diff --git a/internal/common/convert_test.go b/internal/common/convert_test.go index 9569a2154..f09c90191 100644 --- a/internal/common/convert_test.go +++ b/internal/common/convert_test.go @@ -40,7 +40,7 @@ func TestPtrOf(t *testing.T) { func TestPtrHelpers(t *testing.T) { assert.Equal(t, int32(1), *Int32Ptr(1)) assert.Equal(t, int64(1), *Int64Ptr(1)) - assert.Equal(t, float64(1.1), *Float64Ptr(1.1)) + assert.Equal(t, 1.1, *Float64Ptr(1.1)) assert.Equal(t, true, *BoolPtr(true)) assert.Equal(t, "a", *StringPtr("a")) assert.Equal(t, s.TaskList{Name: PtrOf("a")}, *TaskListPtr(s.TaskList{Name: PtrOf("a")})) diff --git a/internal/common/thrift_util.go b/internal/common/thrift_util.go index 15c454e03..730127e62 100644 --- a/internal/common/thrift_util.go +++ b/internal/common/thrift_util.go @@ -71,6 +71,9 @@ func TListDeserialize(ts []thrift.TStruct, b []byte) (err error) { // IsUseThriftEncoding checks if the objects passed in are all encoded using thrift. func IsUseThriftEncoding(objs []interface{}) bool { + if len(objs) == 0 { + return false + } // NOTE: our criteria to use which encoder is simple if all the types are serializable using thrift then we use // thrift encoder. For everything else we default to gob. for _, obj := range objs { @@ -83,6 +86,9 @@ func IsUseThriftEncoding(objs []interface{}) bool { // IsUseThriftDecoding checks if the objects passed in are all de-serializable using thrift. func IsUseThriftDecoding(objs []interface{}) bool { + if len(objs) == 0 { + return false + } // NOTE: our criteria to use which encoder is simple if all the types are de-serializable using thrift then we use // thrift decoder. For everything else we default to gob. for _, obj := range objs { diff --git a/internal/common/thrift_util_test.go b/internal/common/thrift_util_test.go index e6e705e43..a73647455 100644 --- a/internal/common/thrift_util_test.go +++ b/internal/common/thrift_util_test.go @@ -59,43 +59,71 @@ func TestTListDeserialize(t *testing.T) { } func TestIsUseThriftEncoding(t *testing.T) { - ts := []interface{}{ - &mockThriftStruct{}, - &mockThriftStruct{}, - } - - result := IsUseThriftEncoding(ts) - assert.True(t, result) - - ts = []interface{}{ - &mockThriftStruct{}, - "string", + for _, tc := range []struct { + name string + input []interface{} + expected bool + }{ + { + name: "nil", + input: nil, + expected: false, + }, + { + name: "success", + input: []interface{}{ + &mockThriftStruct{}, + &mockThriftStruct{}, + }, + expected: true, + }, + { + name: "fail", + input: []interface{}{ + &mockThriftStruct{}, + PtrOf("string"), + }, + expected: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, IsUseThriftEncoding(tc.input)) + }) } - - result = IsUseThriftEncoding(ts) - assert.False(t, result) } func TestIsUseThriftDecoding(t *testing.T) { - t.Run("success", func(t *testing.T) { - str1 := &mockThriftStruct{} - str2 := &mockThriftStruct{} - ts := []interface{}{ - &str1, - &str2, - } - - assert.True(t, IsUseThriftDecoding(ts)) - }) - - t.Run("fail", func(t *testing.T) { - ts := []interface{}{ - &mockThriftStruct{}, - "string", - } - - assert.False(t, IsUseThriftDecoding(ts)) - }) + for _, tc := range []struct { + name string + input []interface{} + expected bool + }{ + { + name: "nil", + input: nil, + expected: false, + }, + { + name: "success", + input: []interface{}{ + PtrOf(&mockThriftStruct{}), + PtrOf(&mockThriftStruct{}), + }, + expected: true, + }, + { + name: "fail", + input: []interface{}{ + PtrOf(&mockThriftStruct{}), + PtrOf("string"), + }, + expected: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, IsUseThriftDecoding(tc.input)) + }) + } } func TestIsThriftType(t *testing.T) { From a215fbc2fcad7d24b138c388110d7ed278482d5d Mon Sep 17 00:00:00 2001 From: Ilya Ozherelyev Date: Fri, 8 Nov 2024 12:43:38 +0100 Subject: [PATCH 7/7] Empty-Commit