Repository: gopcua/opcua
Pull request #512 (2021-11-24 08:06:18)

ua: add support for resolving namespace urls
This patch adds support for resolving node ids with namespace URLs (`nsu=`). Since I don't want to break the `ParseNodeID()` function I've created `ResolveNodeID()` function which takes a resolver for the lookup. 

If we are OK with breaking `ParseNodeID()` then we can roll this into one function. 

magiconair: Still need to add tests.
kung-foo: Should we provide a "default" resolver? Or at least a helper? I have this bit of code in one of our util packages:

```go
func ReadNamespaces(client *opcua.Client) ([]string, error) {
	node := client.Node(ua.NewNumericNodeID(0, id.Server_NamespaceArray))
	v, err := node.Value()
	if err != nil {
		return nil, err
	}

	ns, ok := v.Value().([]string)
	if !ok {
		return nil, errors.Errorf("invalid namespace value type. id=%d, type=%T", v.Type(), v.Value())
	}

	return ns, nil
}
```
magiconair: Yes, absolutely. I'll add it.
magiconair: I guess we want to cache the namespace array
siscia: Sorry, maybe I missunderstand the whole point of the ParseNodeId function.

But if we try to 

```go
node, err := ua.ParseNodeId("ns=12;s=foo")
```
where the namespace = 12 is not defined, does the `ParseNodeId` function returns an error?

Why should it be different for `nsu`? 

I am missing some OPCUA specs?


kung-foo: @siscia The idea is that instead of `ns=5;s=Counter1` you could use `nsu=http://www.prosysopc.com/OPCUA/SimulationNodes;s=Counter1`. To resolve the url to 5, you need to query the `Server_NamespaceArray`.

`ParseNodeId` does not _validate_ any of the values, just the structure. So it will send whatever is in there to the server.

@magiconair what happens if we just send an `nsu=...` node it to the server?

magiconair: Thx. I'll add some tests and then merge it.
magiconair: After thinking about this I think this approach is wrong. I would like to change `ParseNodeID(s string)` to `ParseNodeID(s string, ns []string)` and make the resolution entirely static. The client should then maintain the namespace array by fetching it on connection and with an optional refresh timer as an option. Opinions?
magiconair: This is what I should have done right from the start but hindsight is always 20-20. Allow for parsing all node ids in `ParseNodeID` and move the responsibility of fetching and caching the namespace array to the caller. This is also why I would feel comfortable in breaking `ParseNodeID` at this point since it feels like the right change and it is easy to fix.
magiconair: Both approaches would not make it possible to have `nsu=` in JSON node ids though. :/
magiconair: Hmm, going down this route also means to pass the `ns` array into every lookup function everywhere. this doesn't look promising. But only passing it into the `ParseNodeID` function means that we only support `nsu=` ids for a subset of cases. I think to make this work we probably need to hide some magic like a global namespace list. 
siscia: What about adding `ParseNodeIDWithNamespaces(s string, ns []string)`

However, I would like to point out that `NodeID`s and `ExtendedNodeID`s (the one with `nsu=...`) are different things.

I am not sure hiding them under the same object is the best choice.
magiconair: I think I have a better approach. We basically leave the `ParseNodeID` function as is but hide it behind a global node id parser which can be swapped out. 

```go
func init() {
	parser.Store(&NamespaceParser{})
}

// parser stores the global node id parser.
// It is guaranteed that there will always be a node id parser.
var parser atomic.Value // NodeIDParser

// RegisterNodeIDParser configures a new global node id parser.
func RegisterNodeIDParser(p NodeIDParser) {
	if p == nil {
		panic("NodeIDParser must not be nil")
	}
	parser.Store(p)
}

// NodeIDParser defines the interface for a node id parser.
type NodeIDParser interface {
	Parse(s string) (*NodeID, error)
}

// NamespaceParser provides a node id parser which is aware of
// server namespaces.
type NamespaceParser struct {
	Namespaces []string
}

func (p *NamespaceParser) Parse(s string) (*NodeID, error) {
	return resolveNodeID(s, p.Namespaces)
}

// MustParseNodeID returns a node id from a string definition
// if it is parseable by ParseNodeID. Otherwise, the function
// panics.
func MustParseNodeID(s string) *NodeID {
	id, err := ParseNodeID(s)
	if err != nil {
		panic(err.Error())
	}
	return id
}

// ParseNodeID returns a node id from a string definition of the format
// '{ns,nsu}=<namespace>;{s,i,b,g}=<identifier>'.
//
// The 's=' prefix can be omitted for string node ids in namespace 0.
//
// For numeric ids the smallest possible type which can store the namespace
// and id value is returned.
//
// It depends on the implementation of the registered node id parser whether
// the function is capable of resolving namespace uris.
func ParseNodeID(s string) (*NodeID, error) {
	return parser.Load().(NodeIDParser).Parse(s)
}

func resolveNodeID(s string, nsu []string) (*NodeID, error) {
...
}
```
magiconair: Then we change the `client.Dial` as follows and boom :

```patch
diff --git a/client.go b/client.go
index eb79f54..a7b9588 100644
--- a/client.go
+++ b/client.go
@@ -480,11 +480,23 @@ func (c *Client) Dial(ctx context.Context) error {
 
 	c.sechan, err = uasc.NewSecureChannel(c.endpointURL, c.conn, c.cfg.sechan, c.sechanErr)
 	if err != nil {
-		_ = c.conn.Close()
+		c.conn.Close()
 		return err
 	}
 
-	return c.sechan.Open(ctx)
+	if err := c.sechan.Open(ctx); err != nil {
+		c.conn.Close()
+		return err
+	}
+
+	ns, err := c.NamespaceArray()
+	if err != nil {
+		c.conn.Close()
+		return err
+	}
+
+	ua.RegisterNodeIDParser(&ua.NamespaceParser{Namespaces: ns})
+	return nil
 }
 
 // Close closes the session and the secure channel.
```
magiconair: For auto-refresh of the namespaces we add an option which kicks off a go routine in the background to fetch the namespace array on a regular basis.

@kung-foo @siscia what do you think?
magiconair: So where the smart client approach fails if you have a program with multiple OPC/UA connections since they would clobber each others' namespaces. So we should make the auto-initialising of namespaces optional but enable it by default. 
magiconair: pushing in a minute

kung-foo: A couple of thoughts (in no particular order):

* I don't like globals like the registry. In this case, it fails to allow for multiple client connections in the same process.
* Since the namespace index (according to the spec) is _only_ valid during the lifetime of the current connection, then it sorta implies that funcs like `ParseNodeID` should be on the client class.
* we (my company) have had a few discussions internally about how we store node ids. right now, we just do something like browse -> store node ids in db. But these node ids always have a ns index and not the actual namespace. 
kung-foo: I checked our codebase to see where we use `ua.ParseNodeID`. There are two places where we use it, and there is a client connection "nearby" in both cases.
kung-foo: In the main opcua repo, we allow raw node ids (i.e. just strings) in the `Subscription.AddNodes(...)` API. There we call `us.ParseNodeID` as a convenience. But, even in that case, there is client conn available.

This has me leaning towards moving `ParseNodeID` to the client, and optionally maintaining a static helper function that explicitly takes a ns slice.
magiconair: Yeah, that might make even more sense.
magiconair: But how would that work for JSON decoding?
magiconair: However, this also means that you cannot use `nsu=` for services and extension object types unless we move them into the client as well. 
kung-foo: I've never touched any JSON stuff (with regards to OPC-UA) so I'm not familiar with the use case.

Should the `NodeID` struct contain the namespace uri?
magiconair: I guess that would be a reasonable solution.
magiconair: Should we then change `ParseNodeID` to contain the `ns` list?
kung-foo: virtual whiteboard:

```
// tag with numeric id, may or may not work downstream in api calls
ua.ParseNodeID("ns=2;s=Tag1") ->  NodeID(ns=2, s=Tag1)

// guarantees (or returns an error) that the numeric ns at least exists, though "Tag1" may or may not 
conn.ParseNodeID("ns=2;s=Tag1") ->  NodeID(ns=2, nsu=urn:my-server, s=Tag1)

// guarantees (or returns an error) that the ns exists. "Tag1" _probably_ exists assuming nothing changed with regards to the tag itself
conn.ParseNodeID("nsu=urn:my-server;s=Tag1") -> NodeID(ns=1337, nsu=urn:my-server, s=Tag1)

// current api
ua.ParseNodeID("nsu=urn:my-server;s=Tag1") -> Err

// new api
ua.ParseNodeID("nsu=urn:my-server;s=Tag1", client.NamespaceArray()) -> NodeID(ns=1337, nsu=urn:my-server, s=Tag1)

// current approach
conn.Browse() -> NodeID(ns=2, s=Tag1).String() -> "ns=2;s=Tag1"

// better approach? essentially we always fillout the nsu field when possible
conn.Browse() -> NodeID(ns=2, nsu=urn:my-server, s=Tag1).String() -> "nsu=urn:my-server;s=Tag1"

// current api (returns ns index is which is not guaranteed the moment the connection closes)
NodeID(ns=2, s=Tag1).String() -> "ns=2;s=Tag1"

// updated string api (returns nsu _iff_ it is set)
NodeID(ns=1337, nsu=urn:my-server, s=Tag1).String() -> "nsu=urn:my-server;s=Tag1"
```
kung-foo: @siscia 
> What about adding ParseNodeIDWithNamespaces(s string, ns []string)
> However, I would like to point out that NodeIDs and ExtendedNodeIDs (the one with nsu=...) are different things.
> I am not sure hiding them under the same object is the best choice.

I guess from my perspective, the `NodeID` (with only a ns index) is an internal, not-valid-outside-this-conn object. Whereas the `ExtendedNodeID` is the one I end reading and writing with.

So I'd _almost_ be ok with saying the `ExtendedNodeID` _is_ the `NodeID`, and that the ns-index-only is just a private bit that no client sees.

But that's probably too opinionated for this lib... 
siscia: So, the real important details that was contributed in the discussion in the fact that the namespace array is valid only as long as the OPCUA session.

This means that the client should not worry about cache invalidation, as long as it uses a single session.

It is also telling us that we should treat the `nsu` as first class citizens, since each time we store, a nodeid with a namespace encoded, is a possible source of problem in the future.

But something like 

`ParseExtendedNodeId(s) *ExtendedNodeID` and then provide a method:
`(e *ExtendedNodeID) NodeID(ns []string) *NodeID`

Does not work?

I really don't like to force the parsing of node IDs to go through a connection.

It forces us to have a connection to validate that a string is a valid NodeID, that I find wrong as we are forcing a string to be a valid nodeid in a specific amount of time and when there is an OPCUA server close by.
magiconair: Namespaces can change during the session
siscia: From the spec:

```
NamespaceArray defines an array of namespace URIs. This Variable is also referred as namespace table. The indexes into the namespace table are referred to as NamespaceIndexes. NamespaceIndexes are used in NodeIds in OPC UA Services, rather than the longer namespace URI. Index 0 is reserved for the OPC UA namespace, and index 1 is reserved for the local Server. Clients may read the entire namespace table or they may read individual entries in the namespace table. The Server shall not modify or delete entries of the namespace table while any client has an open session to the Server, because clients may cache the namespace table. A Server may add entries to the namespace table even if clients are connected to the Server. It is recommended that Servers not change the indexes of the namespace table but only add entries, because the client may cache NodeIds using the indexes. Nevertheless, it might not always be possible for Servers to avoid changing indexes in the namespace table. Clients that cache NamespaceIndexes of NodeIds should always check when starting a session to verify that the cached NamespaceIndexes have not changed.
```

https://reference.opcfoundation.org/v104/Core/docs/Part5/6.3.1/

So yes it can change during a session, but it should not delete entries.
kung-foo: > It forces us to have a connection to validate that a string is a valid NodeID, that I find wrong as we are forcing a string to be a valid nodeid in a specific amount of time and when there is an OPCUA server close by.

Is it possible to validate a NodeID without a session? We can validate that it (assuming a string) syntactically represents a node. I guess it is the difference between parsing a node id and validating/resolving.
magiconair: I think what we are struggling with is in the fact that a NodeID does not support namespace URIs. It only supports numeric namespace indexes. So from that perspective adding support for namespaces to the `ParseNodeID` function seems wrong. And maybe that is why we are struggling with this. 

https://reference.opcfoundation.org/v104/Core/docs/Part6/5.2.2/#5.2.2.9

Namespace URIs only make sense in the context of a session since they have to be resolved to a numeric id before they can be used. So maybe we should leave `ParseNodeID` the way it is and don't support `nsu=` but instead add a `ParseExpandedNodeID` function for the static parsing and a `client.Namespaces()` function which returns the list of cached namespaces.
magiconair: Something like this:

```go
// ParseNodeID returns a node id from a string definition of the format
// 'ns=<namespace>;{s,i,b,g}=<identifier>'.
//
// The 's=' prefix can be omitted for string node ids in namespace 0.
//
// For numeric ids the smallest possible type which can store the namespace
// and id value is returned.
//
// Namespace URIs are not supported since NodeID cannot store them. If you need
// to support namespace URIs use ParseExpandedNodeID.
func ParseNodeID(s string) (*NodeID, error) {
	id, err := ParseExpandedNodeID(s, nil)
	if err != nil {
		return nil, err
	}
	if id.NamespaceURI != "" {
		return nil, errors.Errorf("namespace uris are not supported")
	}
	return id.NodeID, nil
}

// ParseExpandedNodeID returns a node id from a string definition of the format
// '{ns,nsu}=<namespace>;{s,i,b,g}=<identifier>'.
//
// The 's=' prefix can be omitted for string node ids in namespace 0.
//
// For numeric ids the smallest possible type which can store the namespace
// and id value is returned.
//
// Namespace URIs are resolved to ids from the provided list of namespaces.
func ParseExpandedNodeID(s string, ns []string) (*ExpandedNodeID, error) {
...
}
```
magiconair: @kung-foo and @siscia could you please have another look?
magiconair: Make sure you look at the `Files` section in the review tab to see the current list of changes to `main` and not just the delta to the previous iteration.
kung-foo: ugh. :facepalm: I never noticed there was an _actual_ `ExpandedNodeID` struct in the lib. I thought this was all hypothetical... Ignore most of my comments today...
siscia: I think it looks good!
magiconair: I'll flatten the changes and work on your comments @kung-foo. I'll add the option to update the namespaces back again. Also need to find out why the tests are failing all of a sudden.
siscia: Yesterday I though more about this.

And I am not able to see why the interface I proposed above was not liked.

Unfortunately, `ExtendedNodeID` where introduces as an after though to the OPC/UA implementation (or at least it seems like) and I don't think that it makes any benefit in trying to hide the fact that those are different.

I would really like to understand why the interface:

```go
ParseExtendedNodeId(s) (*ExtendedNodeID, err)
(e *ExtendedNodeID) NodeID(ns []string) (*NodeID, err)
```

was not considered.

In case of defaults nodeids, the ParseExtendedNodeId function will return a zero value ExtendedNodeID with only the NodeID field set.

In the case of an ExtendedNodeId `nsu=...` the function will return the ExtendedNodeID without verifying that the namespace exists.

Finally the method will allow to jump back to the standard `NodeID` if the namespace match, otherwise will raise an error.
magiconair: Afterthought or not, `ExpandedNodeID` is part of the standard and it exists to support node ids with namespace URIs. By using two different parsing functions we make it clear that they are different. 

I think the main issue with what you're proposing is that it might break existing code since you could no longer rely on the public `NodeID` field to be valid. Also, using a two step process to resolve a string to a final node id seems too complex. I think users know whether they deal with `nsu` or not. And if they are unsure they have to use `ExpandedNodeID` from the start. 

It also feels wrong to me to make the `ExpandedNodeID` responsible for resolving a node id uri. It should be just a data container and all logic is usually handled in helper functions. @kung-foo what do you think?
magiconair: Rebasing to `main`
magiconair: Found the bug which made the tests fail. 
magiconair: I'll squash the changes into a single commit. We don't need the path on how we got here.


Patch:
diff --git a/client.go b/client.go
index b0e8193..776624d 100644
--- a/client.go
+++ b/client.go
@@ -126,6 +126,9 @@ type Client struct {
 	// state of the client
 	state atomic.Value // ConnState
 
+	// list of cached namespaces on the server
+	namespaces atomic.Value // []string
+
 	// monitorOnce ensures only one connection monitor is running
 	monitorOnce sync.Once
 
@@ -158,6 +161,7 @@ func NewClient(endpoint string, opts ...Option) *Client {
 	c.publishTimeout.Store(uasc.MaxTimeout)
 	c.pauseSubscriptions(context.Background())
 	c.state.Store(Closed)
+	c.namespaces.Store([]string{})
 	return &c
 }
 
@@ -185,13 +189,15 @@ func (c *Client) Connect(ctx context.Context) (err error) {
 	if err := c.Dial(ctx); err != nil {
 		return err
 	}
+
 	s, err := c.CreateSession(c.cfg.session)
 	if err != nil {
-		_ = c.Close()
+		c.Close()
 		return err
 	}
+
 	if err := c.ActivateSession(s); err != nil {
-		_ = c.Close()
+		c.Close()
 		return err
 	}
 	c.state.Store(Connected)
@@ -203,6 +209,15 @@ func (c *Client) Connect(ctx context.Context) (err error) {
 		go c.monitorSubscriptions(mctx)
 	})
 
+	// todo(fs): we might need to guard this with an option in case of a broken
+	// todo(fs): server. For the sake of simplicity we left the option out but
+	// todo(fs): see the discussion in https://github.com/gopcua/opcua/pull/512
+	// todo(fs): and you should find a commit that implements this option.
+	if err := c.UpdateNamespaces(); err != nil {
+		c.Close()
+		return err
+	}
+
 	return nil
 }
 
@@ -489,11 +504,16 @@ func (c *Client) Dial(ctx context.Context) error {
 
 	c.sechan, err = uasc.NewSecureChannel(c.endpointURL, c.conn, c.cfg.sechan, c.sechanErr)
 	if err != nil {
-		_ = c.conn.Close()
+		c.conn.Close()
 		return err
 	}
 
-	return c.sechan.Open(ctx)
+	if err := c.sechan.Open(ctx); err != nil {
+		c.conn.Close()
+		return err
+	}
+
+	return nil
 }
 
 // Close closes the session and the secure channel.
@@ -529,6 +549,11 @@ func (c *Client) State() ConnState {
 	return c.state.Load().(ConnState)
 }
 
+// Namespaces returns the currently cached list of namespaces.
+func (c *Client) Namespaces() []string {
+	return c.namespaces.Load().([]string)
+}
+
 // Session returns the active session.
 func (c *Client) Session() *Session {
 	return c.session.Load().(*Session)
@@ -960,6 +985,31 @@ func (c *Client) HistoryReadRawModified(nodes []*ua.HistoryReadValueID, details
 	return res, err
 }
 
+// NamespaceArray returns the list of namespaces registered on the server.
+func (c *Client) NamespaceArray() ([]string, error) {
+	node := c.Node(ua.NewNumericNodeID(0, id.Server_NamespaceArray))
+	v, err := node.Value()
+	if err != nil {
+		return nil, err
+	}
+
+	ns, ok := v.Value().([]string)
+	if !ok {
+		return nil, errors.Errorf("error fetching namespace array. id=%d, type=%T", v.Type(), v.Value())
+	}
+	return ns, nil
+}
+
+// UpdateNamespaces updates the list of cached namespaces from the server.
+func (c *Client) UpdateNamespaces() error {
+	ns, err := c.NamespaceArray()
+	if err != nil {
+		return err
+	}
+	c.namespaces.Store(ns)
+	return nil
+}
+
 // safeAssign implements a type-safe assign from T to *T.
 func safeAssign(t, ptrT interface{}) error {
 	if reflect.TypeOf(t) != reflect.TypeOf(ptrT).Elem() {
diff --git a/ua/expanded_node_id.go b/ua/expanded_node_id.go
index 2ac2572..f513fcf 100644
--- a/ua/expanded_node_id.go
+++ b/ua/expanded_node_id.go
@@ -4,6 +4,15 @@
 
 package ua
 
+import (
+	"encoding/base64"
+	"math"
+	"strconv"
+	"strings"
+
+	"github.com/gopcua/opcua/errors"
+)
+
 // ExpandedNodeID extends the NodeID structure by allowing the NamespaceURI to be
 // explicitly specified instead of using the NamespaceIndex. The NamespaceURI is optional.
 // If it is specified, then the NamespaceIndex inside the NodeID shall be ignored.
@@ -20,17 +29,18 @@ func (a ExpandedNodeID) String() string {
 }
 
 // NewExpandedNodeID creates a new ExpandedNodeID.
-func NewExpandedNodeID(hasURI, hasIndex bool, nodeID *NodeID, uri string, idx uint32) *ExpandedNodeID {
+func NewExpandedNodeID(nodeID *NodeID, uri string, idx uint32) *ExpandedNodeID {
 	e := &ExpandedNodeID{
 		NodeID:      nodeID,
 		ServerIndex: idx,
 	}
 
-	if hasURI {
+	if uri != "" {
 		e.NodeID.SetURIFlag()
 		e.NamespaceURI = uri
 	}
-	if hasIndex {
+
+	if idx > 0 {
 		e.NodeID.SetIndexFlag()
 	}
 
@@ -51,6 +61,34 @@ func NewFourByteExpandedNodeID(ns uint8, id uint16) *ExpandedNodeID {
 	}
 }
 
+// NewNumericExpandedNodeID creates a numeric expanded node id.
+func NewNumericExpandedNodeID(ns uint16, id uint32) *ExpandedNodeID {
+	return &ExpandedNodeID{
+		NodeID: NewNumericNodeID(ns, id),
+	}
+}
+
+// NewStringExpandedNodeID creates a string expanded node id.
+func NewStringExpandedNodeID(ns uint16, id string) *ExpandedNodeID {
+	return &ExpandedNodeID{
+		NodeID: NewStringNodeID(ns, id),
+	}
+}
+
+// NewGUIDExpandedNodeID creates a GUID expanded node id.
+func NewGUIDExpandedNodeID(ns uint16, id string) *ExpandedNodeID {
+	return &ExpandedNodeID{
+		NodeID: NewGUIDNodeID(ns, id),
+	}
+}
+
+// NewByteStringExpandedNodeID creates a byte string expanded node id.
+func NewByteStringExpandedNodeID(ns uint16, id []byte) *ExpandedNodeID {
+	return &ExpandedNodeID{
+		NodeID: NewByteStringNodeID(ns, id),
+	}
+}
+
 func (e *ExpandedNodeID) Decode(b []byte) (int, error) {
 	buf := NewBuffer(b)
 	e.NodeID = new(NodeID)
@@ -86,3 +124,108 @@ func (e *ExpandedNodeID) HasNamespaceURI() bool {
 func (e *ExpandedNodeID) HasServerIndex() bool {
 	return e.NodeID.EncodingMask()>>6&0x1 == 1
 }
+
+// ParseExpandedNodeID returns a node id from a string definition of the format
+// '{ns,nsu}=<namespace>;{s,i,b,g}=<identifier>'.
+//
+// The 's=' prefix can be omitted for string node ids in namespace 0.
+//
+// For numeric ids the smallest possible type which can store the namespace
+// and id value is returned.
+//
+// Namespace URIs are resolved to ids from the provided list of namespaces.
+func ParseExpandedNodeID(s string, ns []string) (*ExpandedNodeID, error) {
+	if s == "" {
+		return NewTwoByteExpandedNodeID(0), nil
+	}
+
+	var nsval, idval string
+
+	p := strings.SplitN(s, ";", 3)
+	switch len(p) {
+	case 1:
+		nsval, idval = "ns=0", p[0]
+	case 2:
+		nsval, idval = p[0], p[1]
+	default:
+		return nil, errors.Errorf("invalid node id: %s", s)
+	}
+
+	// parse namespace
+	var nsid uint16
+	var nsu string
+	switch {
+	case strings.HasPrefix(nsval, "nsu="):
+		if ns == nil {
+			return nil, errors.Errorf("namespace urls require a server NamespaceArray")
+		}
+
+		nsuval := strings.TrimPrefix(nsval, "nsu=")
+		ok := false
+		for id, uri := range ns {
+			if uri == nsuval {
+				nsid, nsu = uint16(id), uri
+				ok = true
+				break
+			}
+		}
+		if !ok {
+			return nil, errors.Errorf("namespace uri %s not found in the server NamespaceArray %#v", nsval, ns)
+		}
+
+	case strings.HasPrefix(nsval, "ns="):
+		n, err := strconv.Atoi(nsval[3:])
+		if err != nil {
+			return nil, errors.Errorf("invalid namespace id: %s", s)
+		}
+		if n < 0 || n > math.MaxUint16 {
+			return nil, errors.Errorf("namespace id out of range (0..65535): %s", s)
+		}
+		nsid = uint16(n)
+
+	default:
+		return nil, errors.Errorf("invalid node id: %s", s)
+	}
+
+	// parse identifier
+	switch {
+	case strings.HasPrefix(idval, "i="):
+		id, err := strconv.ParseUint(idval[2:], 10, 64)
+		if err != nil {
+			return nil, errors.Errorf("invalid numeric id: %s", s)
+		}
+		switch {
+		case nsid == 0 && id < 256:
+			return NewExpandedNodeID(NewTwoByteNodeID(byte(id)), "", 0), nil
+		case nsid < 256 && id < math.MaxUint16:
+			return NewExpandedNodeID(NewFourByteNodeID(byte(nsid), uint16(id)), nsu, 0), nil
+		case id <= math.MaxUint32:
+			return NewExpandedNodeID(NewNumericNodeID(nsid, uint32(id)), nsu, 0), nil
+		default:
+			return nil, errors.Errorf("numeric id out of range (0..2^32-1): %s", s)
+		}
+
+	case strings.HasPrefix(idval, "s="):
+		return NewExpandedNodeID(NewStringNodeID(nsid, idval[2:]), nsu, 0), nil
+
+	case strings.HasPrefix(idval, "g="):
+		n := NewGUIDNodeID(nsid, idval[2:])
+		if n == nil || n.StringID() == "" {
+			return nil, errors.Errorf("invalid guid node id: %s", s)
+		}
+		return NewExpandedNodeID(n, nsu, 0), nil
+
+	case strings.HasPrefix(idval, "b="):
+		b, err := base64.StdEncoding.DecodeString(idval[2:])
+		if err != nil {
+			return nil, errors.Errorf("invalid opaque node id: %s", s)
+		}
+		return NewExpandedNodeID(NewByteStringNodeID(nsid, b), nsu, 0), nil
+
+	case strings.HasPrefix(idval, "ns="):
+		return nil, errors.Errorf("invalid node id: %s", s)
+
+	default:
+		return NewExpandedNodeID(NewStringNodeID(nsid, idval), nsu, 0), nil
+	}
+}
diff --git a/ua/node_id.go b/ua/node_id.go
index 6340487..594829d 100644
--- a/ua/node_id.go
+++ b/ua/node_id.go
@@ -9,8 +9,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"math"
-	"strconv"
-	"strings"
 
 	"github.com/gopcua/opcua/errors"
 )
@@ -94,90 +92,25 @@ func MustParseNodeID(s string) *NodeID {
 // ParseNodeID returns a node id from a string definition of the format
 // 'ns=<namespace>;{s,i,b,g}=<identifier>'.
 //
-// For string node ids the 's=' prefix can be omitted.
+// The 's=' prefix can be omitted for string node ids in namespace 0.
 //
 // For numeric ids the smallest possible type which can store the namespace
 // and id value is returned.
 //
-// Namespace URLs 'nsu=' are not supported since they require a lookup.
+// Namespace URIs are not supported since NodeID cannot store them. If you need
+// to support namespace URIs use ParseExpandedNodeID.
 func ParseNodeID(s string) (*NodeID, error) {
-	if s == "" {
-		return NewTwoByteNodeID(0), nil
+	id, err := ParseExpandedNodeID(s, nil)
+	if err != nil {
+		return nil, err
 	}
-
-	var nsval, idval string
-
-	p := strings.SplitN(s, ";", 2)
-	switch len(p) {
-	case 1:
-		nsval, idval = "ns=0", p[0]
-	case 2:
-		nsval, idval = p[0], p[1]
-	default:
-		return nil, errors.Errorf("invalid node id: %s", s)
+	if id.HasNamespaceURI() {
+		return nil, errors.Errorf("namespace uris are not supported. use `ua.ParseExpandedNodeID`")
 	}
-
-	// parse namespace
-	var ns uint16
-	switch {
-	case strings.HasPrefix(nsval, "nsu="):
-		return nil, errors.Errorf("namespace urls are not supported: %s", s)
-
-	case strings.HasPrefix(nsval, "ns="):
-		n, err := strconv.Atoi(nsval[3:])
-		if err != nil {
-			return nil, errors.Errorf("invalid namespace id: %s", s)
-		}
-		if n < 0 || n > math.MaxUint16 {
-			return nil, errors.Errorf("namespace id out of range (0..65535): %s", s)
-		}
-		ns = uint16(n)
-
-	default:
-		return nil, errors.Errorf("invalid node id: %s", s)
-	}
-
-	// parse identifier
-	switch {
-	case strings.HasPrefix(idval, "i="):
-		id, err := strconv.ParseUint(idval[2:], 10, 64)
-		if err != nil {
-			return nil, errors.Errorf("invalid numeric id: %s", s)
-		}
-		switch {
-		case ns == 0 && id < 256:
-			return NewTwoByteNodeID(byte(id)), nil
-		case ns < 256 && id < math.MaxUint16:
-			return NewFourByteNodeID(byte(ns), uint16(id)), nil
-		case id <= math.MaxUint32:
-			return NewNumericNodeID(ns, uint32(id)), nil
-		default:
-			return nil, errors.Errorf("numeric id out of range (0..2^32-1): %s", s)
-		}
-
-	case strings.HasPrefix(idval, "s="):
-		return NewStringNodeID(ns, idval[2:]), nil
-
-	case strings.HasPrefix(idval, "g="):
-		n := NewGUIDNodeID(ns, idval[2:])
-		if n == nil || n.StringID() == "" {
-			return nil, errors.Errorf("invalid guid node id: %s", s)
-		}
-		return n, nil
-
-	case strings.HasPrefix(idval, "b="):
-		b, err := base64.StdEncoding.DecodeString(idval[2:])
-		if err != nil {
-			return nil, errors.Errorf("invalid opaque node id: %s", s)
-		}
-		return NewByteStringNodeID(ns, b), nil
-
-	case strings.HasPrefix(idval, "ns="):
-		return nil, errors.Errorf("invalid node id: %s", s)
-
-	default:
-		return NewStringNodeID(ns, idval), nil
+	if id.HasServerIndex() {
+		return nil, errors.Errorf("server index is not supported. use `ua.ParseExpandedNodeID`")
 	}
+	return id.NodeID, nil
 }
 
 // EncodingMask returns the encoding mask field including the


Test patch:
diff --git a/ua/expanded_node_id_test.go b/ua/expanded_node_id_test.go
index dfd186a..de868de 100644
--- a/ua/expanded_node_id_test.go
+++ b/ua/expanded_node_id_test.go
@@ -5,52 +5,40 @@
 package ua
 
 import (
+	"math"
+	"reflect"
 	"testing"
+
+	"github.com/gopcua/opcua/errors"
 )
 
 func TestExpandedNodeID(t *testing.T) {
 	cases := []CodecTestCase{
 		{
-			Name: "Without optional fields",
-			Struct: NewExpandedNodeID(
-				false, false,
-				NewTwoByteNodeID(0xff),
-				"", 0,
-			),
+			Name:   "Without optional fields",
+			Struct: NewExpandedNodeID(NewTwoByteNodeID(0xff), "", 0),
 			Bytes: []byte{
 				0x00, 0xff,
 			},
 		},
 		{
-			Name: "With NamespaceURI",
-			Struct: NewExpandedNodeID(
-				true, false,
-				NewTwoByteNodeID(0xff),
-				"foobar", 0,
-			),
+			Name:   "With NamespaceURI",
+			Struct: NewExpandedNodeID(NewTwoByteNodeID(0xff), "foobar", 0),
 			Bytes: []byte{
 				0x80, 0xff, 0x06, 0x00, 0x00, 0x00, 0x66, 0x6f,
 				0x6f, 0x62, 0x61, 0x72,
 			},
 		},
 		{
-			Name: "With ServerIndex",
-			Struct: NewExpandedNodeID(
-				false, true,
-				NewTwoByteNodeID(0xff),
-				"", 32768,
-			),
+			Name:   "With ServerIndex",
+			Struct: NewExpandedNodeID(NewTwoByteNodeID(0xff), "", 32768),
 			Bytes: []byte{ // With ServerIndex
 				0x40, 0xff, 0x00, 0x80, 0x00, 0x00,
 			},
 		},
 		{
-			Name: "With NamespaceURI and ServerIndex",
-			Struct: NewExpandedNodeID(
-				true, true,
-				NewTwoByteNodeID(0xff),
-				"foobar", 32768,
-			),
+			Name:   "With NamespaceURI and ServerIndex",
+			Struct: NewExpandedNodeID(NewTwoByteNodeID(0xff), "foobar", 32768),
 			Bytes: []byte{
 				0xc0, 0xff, 0x06, 0x00, 0x00, 0x00, 0x66, 0x6f,
 				0x6f, 0x62, 0x61, 0x72, 0x00, 0x80, 0x00, 0x00,
@@ -59,3 +47,62 @@ func TestExpandedNodeID(t *testing.T) {
 	}
 	RunCodecTest(t, cases)
 }
+
+func TestParseExpandedNodeID(t *testing.T) {
+	cases := []struct {
+		s   string
+		ns  []string
+		n   *ExpandedNodeID
+		err error
+	}{
+		// happy flows (same as for ParseNodeID)
+		{s: "", n: NewTwoByteExpandedNodeID(0)},
+		{s: "ns=0;i=1", n: NewTwoByteExpandedNodeID(1)},
+		{s: "i=1", n: NewTwoByteExpandedNodeID(1)},
+		{s: "i=2253", n: NewFourByteExpandedNodeID(0, 2253)},
+		{s: "ns=1;i=2", n: NewFourByteExpandedNodeID(1, 2)},
+		{s: "ns=256;i=2", n: NewNumericExpandedNodeID(256, 2)},
+		{s: "ns=1;i=65536", n: NewNumericExpandedNodeID(1, 65536)},
+		{s: "ns=65535;i=65536", n: NewNumericExpandedNodeID(65535, 65536)},
+		{s: "ns=2;i=4294967295", n: NewNumericExpandedNodeID(2, math.MaxUint32)},
+		{s: "ns=1;g=5eac051c-c313-43d7-b790-24aa2c3cfd37", n: NewGUIDExpandedNodeID(1, "5eac051c-c313-43d7-b790-24aa2c3cfd37")},
+		{s: "ns=1;b=YWJj", n: NewByteStringExpandedNodeID(1, []byte{'a', 'b', 'c'})},
+		{s: "ns=1;s=a", n: NewStringExpandedNodeID(1, "a")},
+		{s: "ns=1;a", n: NewStringExpandedNodeID(1, "a")},
+
+		// error flows (same as ParseNodeID)
+		{s: "abc=0;i=2", err: errors.New("invalid node id: abc=0;i=2")},
+		{s: "ns=0;i=1;s=2", err: errors.New("invalid node id: ns=0;i=1;s=2")},
+		{s: "ns=0", err: errors.New("invalid node id: ns=0")},
+		{s: "nsu=abc;i=1", err: errors.New("namespace urls require a server NamespaceArray")},
+		{s: "ns=65536;i=1", err: errors.New("namespace id out of range (0..65535): ns=65536;i=1")},
+		{s: "ns=abc;i=1", err: errors.New("invalid namespace id: ns=abc;i=1")},
+		{s: "ns=1;i=abc", err: errors.New("invalid numeric id: ns=1;i=abc")},
+		{s: "ns=1;i=4294967296", err: errors.New("numeric id out of range (0..2^32-1): ns=1;i=4294967296")},
+		{s: "ns=1;g=x", err: errors.New("invalid guid node id: ns=1;g=x")},
+		{s: "ns=1;b=aW52YWxp%ZA==", err: errors.New("invalid opaque node id: ns=1;b=aW52YWxp%ZA==")},
+
+		// nsu happy flows
+		{s: "nsu=abc;i=2", ns: []string{"", "abc"}, n: NewExpandedNodeID(NewFourByteNodeID(1, 2), "abc", 0)},
+		{s: "nsu=abc;i=65536", ns: []string{"", "abc"}, n: NewExpandedNodeID(NewNumericNodeID(1, 65536), "abc", 0)},
+		{s: "nsu=abc;b=YWJj", ns: []string{"", "abc"}, n: NewExpandedNodeID(NewByteStringNodeID(1, []byte{'a', 'b', 'c'}), "abc", 0)},
+		{s: "nsu=abc;a", ns: []string{"", "abc"}, n: NewExpandedNodeID(NewStringNodeID(1, "a"), "abc", 0)},
+		{s: "nsu=abc;s=a", ns: []string{"", "abc"}, n: NewExpandedNodeID(NewStringNodeID(1, "a"), "abc", 0)},
+
+		// nsu error flows
+		{s: "nsu=abc;i=2253", ns: []string{}, err: errors.New("namespace uri nsu=abc not found in the server NamespaceArray []string{}")},
+		{s: "nsu=abc;i=2253", ns: []string{"", "def", "xyz"}, err: errors.New(`namespace uri nsu=abc not found in the server NamespaceArray []string{"", "def", "xyz"}`)},
+	}
+
+	for _, c := range cases {
+		t.Run(c.s, func(t *testing.T) {
+			n, err := ParseExpandedNodeID(c.s, c.ns)
+			if got, want := err, c.err; !errors.Equal(got, want) {
+				t.Fatalf("got error %v want %v", got, want)
+			}
+			if got, want := n, c.n; !reflect.DeepEqual(got, want) {
+				t.Fatalf("\ngot  %#v\nwant %#v", got, want)
+			}
+		})
+	}
+}
diff --git a/ua/node_id_test.go b/ua/node_id_test.go
index 422c459..34442db 100644
--- a/ua/node_id_test.go
+++ b/ua/node_id_test.go
@@ -154,8 +154,10 @@ func TestParseNodeID(t *testing.T) {
 		{s: "ns=1;a", n: NewStringNodeID(1, "a")},
 
 		// error flows
+		{s: "abc=0;i=2", err: errors.New("invalid node id: abc=0;i=2")},
+		{s: "ns=0;i=1;s=2", err: errors.New("invalid node id: ns=0;i=1;s=2")},
 		{s: "ns=0", err: errors.New("invalid node id: ns=0")},
-		{s: "nsu=abc;i=1", err: errors.New("namespace urls are not supported: nsu=abc;i=1")},
+		{s: "nsu=abc;i=1", err: errors.New("namespace urls require a server NamespaceArray")},
 		{s: "ns=65536;i=1", err: errors.New("namespace id out of range (0..65535): ns=65536;i=1")},
 		{s: "ns=abc;i=1", err: errors.New("invalid namespace id: ns=abc;i=1")},
 		{s: "ns=1;i=abc", err: errors.New("invalid numeric id: ns=1;i=abc")},
diff --git a/ua/variant_test.go b/ua/variant_test.go
index 5864c56..df072d6 100644
--- a/ua/variant_test.go
+++ b/ua/variant_test.go
@@ -206,12 +206,8 @@ func TestVariant(t *testing.T) {
 			},
 		},
 		{
-			Name: "ExpandedNodeID",
-			Struct: MustVariant(NewExpandedNodeID(
-				true, false,
-				NewTwoByteNodeID(0xff),
-				"foobar", 0,
-			)),
+			Name:   "ExpandedNodeID",
+			Struct: MustVariant(NewExpandedNodeID(NewTwoByteNodeID(0xff), "foobar", 0)),
 			Bytes: []byte{
 				// variant encoding mask
 				0x12,