From e27d10e817e4d9a4d5d37cef4e16890923860ffb Mon Sep 17 00:00:00 2001 From: Frank Schroeder Date: Mon, 9 Dec 2024 15:02:03 +0100 Subject: [PATCH 1/3] use stretchr/testify for the CodecTest --- go.mod | 8 +++++++- go.sum | 10 ++++++++++ ua/codec_test.go | 6 +++--- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 6e9a0c2d..24e3ec82 100644 --- a/go.mod +++ b/go.mod @@ -5,11 +5,17 @@ go 1.22.0 require ( github.com/google/uuid v1.3.0 github.com/pascaldekloe/goe v0.1.1 + github.com/stretchr/testify v1.10.0 golang.org/x/exp v0.0.0-20241204233417-43b7b7cde48d golang.org/x/term v0.8.0 ) -require golang.org/x/sys v0.8.0 // indirect +require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + golang.org/x/sys v0.8.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect +) retract ( v0.2.5 // https://github.com/gopcua/opcua/issues/538 diff --git a/go.sum b/go.sum index 2a3bdd41..f1d7d4bb 100644 --- a/go.sum +++ b/go.sum @@ -1,10 +1,20 @@ +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/pascaldekloe/goe v0.1.1 h1:Ah6WQ56rZONR3RW3qWa2NCZ6JAVvSpUcoLBaOmYFt9Q= github.com/pascaldekloe/goe v0.1.1/go.mod h1:KSyfaxQOh0HZPjDP1FL/kFtbqYqrALJTaMafFUIccqU= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= +github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= golang.org/x/exp v0.0.0-20241204233417-43b7b7cde48d h1:0olWaB5pg3+oychR51GUVCEsGkeCU/2JxjBgIo4f3M0= golang.org/x/exp v0.0.0-20241204233417-43b7b7cde48d/go.mod h1:qj5a5QZpwLU2NLQudwIN5koi3beDhSAlJwa67PuM98c= golang.org/x/sys v0.8.0 h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU= golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.8.0 h1:n5xxQn2i3PC0yLAbjTpNT85q/Kgzcr2gIoX9OrJUols= golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/ua/codec_test.go b/ua/codec_test.go index 24fe252a..14e20aae 100644 --- a/ua/codec_test.go +++ b/ua/codec_test.go @@ -10,7 +10,7 @@ import ( "reflect" "testing" - "github.com/pascaldekloe/goe/verify" + "github.com/stretchr/testify/require" ) // CodecTestCase describes a test case for a encoding and decoding an @@ -49,7 +49,7 @@ func RunCodecTest(t *testing.T, cases []CodecTestCase) { if typ.Kind() == reflect.Slice { v = v.Elem() } - verify.Values(t, "", v.Interface(), c.Struct) + require.Equal(t, c.Struct, v.Interface()) }) t.Run("encode", func(t *testing.T) { @@ -57,7 +57,7 @@ func RunCodecTest(t *testing.T, cases []CodecTestCase) { if err != nil { t.Fatal(err) } - verify.Values(t, "", b, c.Bytes) + require.Equal(t, c.Bytes, b) }) }) } From 3c34d35ed93e12222566d98de9125f2d529f983c Mon Sep 17 00:00:00 2001 From: Frank Schroeder Date: Mon, 9 Dec 2024 15:04:32 +0100 Subject: [PATCH 2/3] Issue #678: Fix Variant to handle nil slices Fix the Variant encoder and decoder to handle nil slices correctly by setting the length to -1. Closes #678 --- ua/datatypes_test.go | 20 ++++++++++++ ua/variant.go | 78 ++++++++++++++++++++++++++++---------------- ua/variant_test.go | 23 ++++++------- 3 files changed, 80 insertions(+), 41 deletions(-) diff --git a/ua/datatypes_test.go b/ua/datatypes_test.go index 180a59ea..54a488d6 100644 --- a/ua/datatypes_test.go +++ b/ua/datatypes_test.go @@ -73,6 +73,26 @@ func TestDataValue(t *testing.T) { 0x80, 0x3b, 0xe8, 0xb3, 0x92, 0x4e, 0xd4, 0x01, }, }, + { + Name: "value with nil slice, source timestamp, server timestamp", + Struct: &DataValue{ + EncodingMask: 0x0d, + Value: MustVariant([]string(nil)), + SourceTimestamp: time.Date(2018, time.September, 17, 14, 28, 29, 112000000, time.UTC), + ServerTimestamp: time.Date(2018, time.September, 17, 14, 28, 29, 112000000, time.UTC), + }, + Bytes: []byte{ + // EncodingMask + 0x0d, + // Value + 0x8c, // type + 0xff, 0xff, 0xff, 0xff, // value + // SourceTimestamp + 0x80, 0x3b, 0xe8, 0xb3, 0x92, 0x4e, 0xd4, 0x01, + // SeverTimestamp + 0x80, 0x3b, 0xe8, 0xb3, 0x92, 0x4e, 0xd4, 0x01, + }, + }, } RunCodecTest(t, cases) } diff --git a/ua/variant.go b/ua/variant.go index 60fda05d..b621dfa0 100644 --- a/ua/variant.go +++ b/ua/variant.go @@ -141,19 +141,29 @@ func (m *Variant) Decode(b []byte) (int, error) { // read flattened array elements n := int(m.arrayLength) - if n < 0 || n > MaxVariantArrayLength { + if n > MaxVariantArrayLength { return buf.Pos(), StatusBadEncodingLimitsExceeded } + // get the type for the slice + sliceType := reflect.SliceOf(typ) + if m.Type() == TypeIDByte { + sliceType = reflect.TypeOf(ByteArray{}) + } + var vals reflect.Value - switch m.Type() { - case TypeIDByte: - vals = reflect.MakeSlice(reflect.TypeOf(ByteArray{}), n, n) + switch { + // decode a nil slice + case n == -1: + vals = reflect.Zero(reflect.MakeSlice(sliceType, 0, 0).Type()) + m.value = vals.Interface() + + // decode a slice with values default: - vals = reflect.MakeSlice(reflect.SliceOf(typ), n, n) - } - for i := 0; i < n; i++ { - vals.Index(i).Set(reflect.ValueOf(m.decodeValue(buf))) + vals = reflect.MakeSlice(sliceType, n, n) + for i := 0; i < n; i++ { + vals.Index(i).Set(reflect.ValueOf(m.decodeValue(buf))) + } } // check for dimensions of multi-dimensional array @@ -416,9 +426,11 @@ var errUnbalancedSlice = errors.New("unbalanced multi-dimensional array") // sliceDim determines the element type, dimensions and the total length // of a one or multi-dimensional slice. -func sliceDim(v reflect.Value) (typ reflect.Type, dim []int32, count int32, err error) { +// +// If the value is a nil slice then count is -1. +func sliceDim(val reflect.Value) (typ reflect.Type, dim []int32, count int32, err error) { // null type - if v.Kind() == reflect.Invalid { + if val.Kind() == reflect.Invalid { return nil, nil, 0, nil } @@ -430,54 +442,64 @@ func sliceDim(v reflect.Value) (typ reflect.Type, dim []int32, count int32, err // array of Byte. // // https://github.com/gopcua/opcua/issues/463 - if v.Type() == reflect.TypeOf([]byte{}) && v.Type() != reflect.TypeOf(ByteArray{}) { - return v.Type(), nil, 1, nil + if val.Type() == reflect.TypeOf([]byte{}) && val.Type() != reflect.TypeOf(ByteArray{}) { + return val.Type(), nil, 1, nil } // element type - if v.Kind() != reflect.Slice { - return v.Type(), nil, 1, nil + if val.Kind() != reflect.Slice { + return val.Type(), nil, 1, nil + } + + // nil array + if val.IsNil() { + return val.Type().Elem(), nil, -1, nil } // empty array - if v.Len() == 0 { - return v.Type().Elem(), append([]int32{0}, dim...), 0, nil + if val.Len() == 0 { + return val.Type().Elem(), append([]int32{0}, dim...), 0, nil } // check that inner slices all have the same length - if v.Index(0).Kind() == reflect.Slice { - for i := 0; i < v.Len(); i++ { - if v.Index(i).Len() != v.Index(0).Len() { + if val.Index(0).Kind() == reflect.Slice { + for i := 0; i < val.Len(); i++ { + if val.Index(i).Len() != val.Index(0).Len() { return nil, nil, 0, errUnbalancedSlice } } } // recurse to inner slice or element type - typ, dim, count, err = sliceDim(v.Index(0)) + typ, dim, count, err = sliceDim(val.Index(0)) if err != nil { return nil, nil, 0, err } - return typ, append([]int32{int32(v.Len())}, dim...), count * int32(v.Len()), nil + return typ, append([]int32{int32(val.Len())}, dim...), count * int32(val.Len()), nil } // set sets the value and updates the flags according to the type. func (m *Variant) set(v interface{}) error { + val := reflect.ValueOf(v) + // set array length and dimensions if value is a slice - et, dim, count, err := sliceDim(reflect.ValueOf(v)) + et, dim, count, err := sliceDim(val) if err != nil { return err } - if len(dim) > 0 { - m.mask |= VariantArrayValues + switch { + case len(dim) > 1: + m.mask |= VariantArrayValues | VariantArrayDimensions m.arrayLength = count - } - - if len(dim) > 1 { - m.mask |= VariantArrayDimensions m.arrayDimensionsLength = int32(len(dim)) m.arrayDimensions = dim + + case len(dim) > 0 || count == -1: + m.mask |= VariantArrayValues + m.arrayLength = count + m.arrayDimensionsLength = 0 + m.arrayDimensions = nil } typeid, ok := variantTypeToTypeID[et] diff --git a/ua/variant_test.go b/ua/variant_test.go index df072d68..37df8b09 100644 --- a/ua/variant_test.go +++ b/ua/variant_test.go @@ -477,6 +477,16 @@ func TestVariant(t *testing.T) { 0x01, 0x00, 0x00, 0x00, }, }, + { + Name: "[]string(nil)", + Struct: MustVariant([]string(nil)), + Bytes: []byte{ + // variant encoding mask + 0x8c, + // array length + 0xff, 0xff, 0xff, 0xff, + }, + }, } RunCodecTest(t, cases) } @@ -543,19 +553,6 @@ func TestArray(t *testing.T) { t.Fatalf("got error %#v want %#v", got, want) } }) - t.Run("length negative", func(t *testing.T) { - b := []byte{ - // variant encoding mask - 0x87, - // array length - 0xff, 0xff, 0xff, 0xff, // -1 - } - - _, err := Decode(b, MustVariant([]uint32{0})) - if got, want := err, StatusBadEncodingLimitsExceeded; !errors.Equal(got, want) { - t.Fatalf("got error %#v want %#v", got, want) - } - }) t.Run("length too big", func(t *testing.T) { b := []byte{ // variant encoding mask From ad13a259fd88f0697eeb2fb2615001ec0b6e058d Mon Sep 17 00:00:00 2001 From: Frank Schroeder Date: Wed, 11 Dec 2024 10:44:39 +0100 Subject: [PATCH 3/3] simplify --- ua/variant.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ua/variant.go b/ua/variant.go index b621dfa0..8b16b2a2 100644 --- a/ua/variant.go +++ b/ua/variant.go @@ -480,10 +480,8 @@ func sliceDim(val reflect.Value) (typ reflect.Type, dim []int32, count int32, er // set sets the value and updates the flags according to the type. func (m *Variant) set(v interface{}) error { - val := reflect.ValueOf(v) - // set array length and dimensions if value is a slice - et, dim, count, err := sliceDim(val) + et, dim, count, err := sliceDim(reflect.ValueOf(v)) if err != nil { return err }