Encode nil node id as zero

[magiconair]

This patch encodes a nil NodeID as a two byte zero node id.

See #506

[magiconair]

Pinging the other OPC/UA experts on the team about this change.

[kung-foo]

not sure... seems like encoding a nil node id should panic. In other words, I would be surprised when it didn't panic since a nil node seems like an error on my part.

However, I see this:

A null NodeId has special meaning. For example, many services defined in OPC 10000-4 define special behaviour if a null NodeId is passed as a parameter. Each IdType has a set of identifier values that represent a null NodeId. These values are summarised in Table 24.

https://reference.opcfoundation.org/v104/Core/docs/Part3/8.2.4/

So maybe there is a valid reason. But in that case, I'd rather have a NewNullNodeID() func.

[magiconair]

Having a function still means that you have to set it everywhere and Encode would still panic if you forget it. What would be the benefit of the function?

[kung-foo]

It's the surprise factor. I'd be surprised if it didn't panic, and I might be surprised if it returned unexpected data because there was a special "null" node id generated (unexpectedly) behind the scenes. I just don't like surprises (birthday parties excepted). Where would we document that any where and for any reason a node id is nil, it will still (sorta) work? I would not expect a user to dig in and look at NodeID.Encode...

[magiconair]

Since the concept of a nil NodeID doesn't exist and is sent as zero value I think this should be in the Encode method. This is how the protocol works and how we translate it to Go. TBH, I always considered it odd that I have to provide the zero value explicitly if all I want to do is to send the default and was usually surprised by the panic. Yes, setting the zero value is more explicit but it also feels like initialising a struct with 0 and false which you wouldn't do in Go.

What I'm struggling with is whether this should stop at the NodeID. The BrowseRequest for example also has an optional ViewDescription. Why do I have to provide a &ua.ViewDescription{ViewID: ua.NewTwoByteNodeID(0)}if all I want is to omit it? That's also exactly the thing the original author was struggling with and what the defaults helper functions provide. Maybe optional fields should be serialised to their zero value when set to nil.

[magiconair]

But it is also this inconsistency which makes me a bit reluctant about this PR in particular since we should either address all optional fields or none of them but not just a select few.

[magiconair]

Lets park this one.

[magiconair]

I'll close this one since there is no agreement.