Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ua: add support for encoding null variants #364

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

ceh
Copy link
Contributor

@ceh ceh commented Aug 16, 2020

Follow-up to #284, add a codec test case for a variant with a value of zero and ensure that it can be encoded without yielding a run-time panic.

What do you think @kung-foo, @magiconair?

if v.Kind() == reflect.Invalid {
return nil, nil, 0, nil
}

// ByteString is its own type
if v.Type() == reflect.TypeOf([]byte{}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v.Type() causes a panic when v is the zero value (reflect.ValueOf(nil)). We use the Kind method prior to checking Type here to handle zero value. See https://pkg.go.dev/reflect?tab=doc#Value.

@magiconair
Copy link
Member

Which codepath does the test check?

@ceh
Copy link
Contributor Author

ceh commented Aug 17, 2020

Which codepath does the test check?

Encoding a NULL variant.

The following program:

package main

import (
	"github.com/gopcua/opcua/ua"
)

func main() {
	v := ua.Variant{}
	_, err := v.Decode([]byte{0x0})
	if err != nil {
		panic(err)
	}
	v.Encode()
}

Causes a run-time panic when calling v.Encode() on the current main branch:

panic: reflect: call of reflect.Value.Interface on zero Value

goroutine 1 [running]:
reflect.valueInterface(0x0, 0x0, 0x0, 0x1, 0x8, 0x4eb960)
	/usr/local/go/src/reflect/value.go:1005 +0x1a5
reflect.Value.Interface(...)
	/usr/local/go/src/reflect/value.go:1000
github.com/gopcua/opcua/ua.(*Variant).encode(0xc000135f40, 0xc000135ec8, 0x0, 0x0, 0x0)
	/home/ceh/code/src/github.com/gopcua/opcua/ua/variant.go:327 +0x119
github.com/gopcua/opcua/ua.(*Variant).Encode(0xc000135f40, 0xc0000153e3, 0x1, 0x1, 0x1, 0x0)
	/home/ceh/code/src/github.com/gopcua/opcua/ua/variant.go:312 +0x180
main.main()
	/home/ceh/nullvariant.go:13 +0x89
exit status 2

@magiconair
Copy link
Member

But this doesn't cover the sliceDim(nil) case, correct? I'm looking at the PR and see two fixes but only one additional test. I'm not sure if the fixes are related. Can you elaborate?

@ceh ceh force-pushed the ceh-encode-null-variant branch from 13aa006 to 4b132d7 Compare August 18, 2020 19:11
@ceh
Copy link
Contributor Author

ceh commented Aug 18, 2020

But this doesn't cover the sliceDim(nil) case, correct? I'm looking at the PR and see two fixes but only one additional test. I'm not sure if the fixes are related. Can you elaborate?

Ah, yeah. That was unclear. The codepath was implicitly tested via the codec test case that uses MustVariant.

I added two additional tests now to make it more explicit, one for sliceDim and one for setting the value of NewVariant.

@magiconair magiconair merged commit a410d76 into gopcua:master Aug 19, 2020
@magiconair
Copy link
Member

Thank you

@ceh ceh deleted the ceh-encode-null-variant branch August 19, 2020 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants