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

remove expanded flag when return Nodes or NodeID #619

Closed
wants to merge 3 commits into from

Conversation

huskar-t
Copy link

when returning Nodes or NodeID there is no longer NamespaceURI and ServerIndex information so the expanded flag should be removed

@huskar-t
Copy link
Author

I think this is the reason for #550.
ReadRequest.NodesToRead[*].NodeID and BrowseRequest.NodesToBrowse.NodesToBrowse[*].NodeID contain the expanded flag and therefore cause the request error.
@magiconair

@huskar-t
Copy link
Author

@kung-foo @magiconair Please review

@kung-foo
Copy link
Member

I think I took a look at this a few years ago. See #200 (comment)

The reason I never merged it was because I am reluctant to just internally clear flags. I would need to know that that there is never a case where the flags should be set.

@huskar-t
Copy link
Author

@kung-foo
In the ReferencedNodesWithContext method

for _, r := range res {
		r.NodeID.NodeID.RemoveExpandedFlag()
		nodes = append(nodes, n.c.Node(r.NodeID.NodeID))
	}

where r.NodeID is the ExpandedNodeID returned in the prosys simulator with the ServerIndex flag. So r.NodeID.NodeID is returned with the ServerIndex flag.
In the AttributesWithContext function

	for _, id := range attrID {
		rv := &ua.ReadValueID{NodeID: n.ID, AttributeID: id}
		req.NodesToRead = append(req.NodesToRead, rv)
	}

if n.ID with ServerIndex flag.The nodeID Encode method writes the mask directly to it, which sends an incorrect ReadRequest

func (n *NodeID) Encode() ([]byte, error) {
	buf := NewBuffer(nil)
	buf.WriteByte(byte(n.mask))

In the browse example, the error occurs as follows:
1.

refs, err := n.ReferencedNodesWithContext(ctx, refType, ua.BrowseDirectionForward, ua.NodeClassAll, true)

ReferencedNodesWithContext return nodes with ServerIndex flag
2.

attrs, err := n.AttributesWithContext(ctx, ua.AttributeIDNodeClass, ua.AttributeIDBrowseName, ua.AttributeIDDescription, ua.AttributeIDAccessLevel, ua.AttributeIDDataType)

An incorrect ReadRequest was sent

@huskar-t
Copy link
Author

I think if the return contains NodeID but not ExpandedNodeID, then the expanded flag of NodeID should be cleared

@huskar-t
Copy link
Author

@magiconair @kung-foo Is this pull request rejected?

@huskar-t
Copy link
Author

maybe you can use prosysopc simulator to try to browse

@kung-foo
Copy link
Member

@huskar-t These changes can take time to merge since we have very few e2e integration tests to verify that this doesn't break expected behavior. I don't have much time this week, but will probably get a chance next week.

@huskar-t
Copy link
Author

@kung-foo Any progress?

@huskar-t
Copy link
Author

huskar-t commented Jun 5, 2023

@magiconair @kung-foo If there are any issues or concerns about my code, please do not hesitate to let me know. I am happy to work on any necessary changes or improvements to get it merged.

@sruehl
Copy link
Contributor

sruehl commented Aug 28, 2023

I think the analysis from @huskar-t is correct. I attached some wireshark screenshot in #682 which illustrates the problem.

@kung-foo
Copy link
Member

Fixed in #683

@kung-foo kung-foo closed this Aug 29, 2023
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.

3 participants