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

More browsing #200

Merged
merged 4 commits into from
Sep 24, 2019
Merged

More browsing #200

merged 4 commits into from
Sep 24, 2019

Conversation

magiconair
Copy link
Member

This is a patch to improve browsing with a somewhat more complex example which works for my current use-case. I think it is time to get this merged. I'll clean this up and then press the button.

return nil, err
}
for _, r := range res {
nodes = append(nodes, n.c.Node(r.NodeID.NodeID))
Copy link
Collaborator

@alexbrdn alexbrdn May 27, 2019

Choose a reason for hiding this comment

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

I wrote a browse example along the same lines to explore a little and have identified a problem with using this pattern: client.Node(reference.NodeID.NodeID).
The ExpandedNodeId in reference.NodeID may have been actually expanded and contains a namespace uri and 0 as namespace index (as per the spec). Now, assigning reference.NodeID.NodeID to a Node's id copies the mask which contains (in my case) NamespaceUri flag and ServerIndex flag. Encoding this NodeID in the BrowseRequest writes 0xC0 as DataEncoding byte into the stream. And the Server responds with StatusBadDecodingError, because the stream doesn't contain the promised namespace URI and ServerIndex and actually the ExpandedNodeId is not allowed in BrowseRequest anyway.

You can test this by browsing the Prosys OPC UA Simulation Server in default configuration. Funny thing is, the namespace that gets expanded is the OPC UA core namespace and AFAICT it's only this namespace that gets expanded. Nevertheless, I see this as a problem with the API as the Reference.NodeID.NodeID allows one to get hold of a NodeID, which is has a mask a NodeID is not supposed to have.

A possible solution would be to make ExpandedNodeID.nodeID private and add a function ExpandedNodeID.NodeID() to safely convert to NodeID or return an error. One problem I see is one would need a lookup to resolve a namespace URI to an index.

Maybe this needs to be extracted into an issue of its own.

Copy link
Member

Choose a reason for hiding this comment

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

ran into this exact issue yesterday

Copy link
Member

Choose a reason for hiding this comment

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

a super quick'n'dirty work around I used:

diff --git a/node.go b/node.go
index 5adf925..00fc9fd 100644
--- a/node.go
+++ b/node.go
@@ -156,6 +156,9 @@ func (n *Node) ReferencedNodes(refs uint32, dir ua.BrowseDirection, mask ua.Node
                return nil, err
        }
        for _, r := range res {
+               r.NodeID.NodeID.ClearIndexFlag()
+               r.NodeID.NodeID.ClearURIFlag()
+
                nodes = append(nodes, n.c.Node(r.NodeID.NodeID))
        }
        return nodes, nil
diff --git a/ua/node_id.go b/ua/node_id.go
index 8533e8c..d178f47 100644
--- a/ua/node_id.go
+++ b/ua/node_id.go
@@ -179,6 +179,10 @@ func (n *NodeID) SetURIFlag() {
        n.mask |= 0x80
 }
 
+func (n *NodeID) ClearURIFlag() {
+       n.mask &^= 0x80
+}
+
 // IndexFlag returns whether the Index flag is set in EncodingMask.
 func (n *NodeID) IndexFlag() bool {
        return n.mask&0x40 == 0x40
@@ -189,6 +193,10 @@ func (n *NodeID) SetIndexFlag() {
        n.mask |= 0x40
 }
 
+func (n *NodeID) ClearIndexFlag() {
+       n.mask &^= 0x40
+}
+
 // Namespace returns the namespace id. For two byte node ids
 // this will always be zero.
 func (n *NodeID) Namespace() uint16 {

@magiconair
Copy link
Member Author

Thanks @kung-foo and @alexbrdn. I'll try to wrap up the other open issues so that we can move forward.

@magiconair
Copy link
Member Author

rebased to master

@magiconair magiconair added this to the v0.1.6 milestone Aug 13, 2019
@magiconair magiconair self-assigned this Aug 13, 2019
@AndreySerebrovC2M
Copy link

Hello! Will you merge it some day?

@magiconair
Copy link
Member Author

Hey @AndreySerebrovC2M, I'll clean this up and merge it. Thanks for reminding me.

examples/browse/browse.go Outdated Show resolved Hide resolved
@magiconair magiconair changed the title WIP: More browsing More browsing Sep 24, 2019
@magiconair
Copy link
Member Author

This has been sitting here for too long. There is some overlap with #266 but I'll sort that out. Merging this now.

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.

4 participants