BugFix: Consider only variable and reference classes #104
Conversation
WalkthroughThe pull request introduces extensive modifications to the OPC UA browsing functionality in the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 GitHub Actions: sensorconnectopcua_plugin/browse.go[error] 313-313: Nil pointer dereference in browse function when attempting to access Node.Attribute 🪛 GitHub Actions: tag-processoropcua_plugin/browse.go[error] 313-313: Invalid memory access in browse function causing segmentation violation 🪛 GitHub Actions: s7-plcopcua_plugin/browse.go[error] 313-313: Invalid memory access in browse function causing segmentation fault 🪛 GitHub Actions: nodered-jsopcua_plugin/browse.go[error] 313-313: Invalid memory access in browse function causing segmentation violation 🪛 GitHub Actions: opcua-plcopcua_plugin/browse.go[error] 313-313: Invalid memory access in browse function causing segmentation violation [error] Ginkgo test suite timed out waiting for all parallel processes to report back ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
opcua_plugin/browse.go (3)
284-296: Consider logging errors when fetching referenced nodesIn the
hasNodeReferencedComponentsfunction, if an error occurs when fetching variable or object class references, the error is silently ignored by returningfalse. This could make debugging more difficult. Consider logging the errors to assist with troubleshooting and provide more visibility into potential issues.Apply this diff to add error logging:
func() bool { variableClassRefs, err := n.ReferencedNodes(ctx, id.HasComponent, ua.BrowseDirectionForward, ua.NodeClassVariable, true) if err != nil { + logger.Warnf("Failed to fetch variable class references: %v", err) return false } objectClassRefs, err := n.ReferencedNodes(ctx, id.HasComponent, ua.BrowseDirectionForward, ua.NodeClassObject, true) if err != nil { + logger.Warnf("Failed to fetch object class references: %v", err) return false } if len(variableClassRefs) == 0 && len(objectClassRefs) == 0 { return false } return true }
301-315: Refactor to reduce code duplication inbrowseChildrenV2The
browseChildrenV2function contains duplicated code when fetching variable and object class children. To improve maintainability and readability, consider creating a helper function to fetch and append children based on the node class.Here's how you can refactor the code:
func browseChildrenV2(refType uint32) error { children := make([]NodeBrowser, 0) - variableClassChildren, err := n.Children(ctx, refType, ua.NodeClassVariable) + nodeClasses := []ua.NodeClass{ua.NodeClassVariable, ua.NodeClassObject} + for _, nodeClass := range nodeClasses { + classChildren, err := n.Children(ctx, refType, nodeClass) if err != nil { sendError(ctx, errors.Errorf("Children: %d: %s", refType, err), errChan, logger) return err } - children = append(children, variableClassChildren...) - objectClassChildren, err := n.Children(ctx, refType, ua.NodeClassObject) - if err != nil { - sendError(ctx, errors.Errorf("Children: %d: %s", refType, err), errChan, logger) - return err - } - children = append(children, objectClassChildren...) + children = append(children, classChildren...) + } for _, child := range children { wg.Add(1) go browse(ctx, child, def.Path, level+1, logger, def.NodeID.String(), nodeChan, errChan, wg, browseHierarchicalReferences, opcuaBrowserChan) } return nil }
324-337: Refactor duplicated logic inbrowseChildrenfunctionSimilar to
browseChildrenV2, thebrowseChildrenfunction has duplicated code for fetching variable and object class references. Refactoring this code can enhance maintainability and reduce potential errors due to code duplication.Here's a suggested refactor:
func browseChildren(refType uint32) error { refs := make([]NodeBrowser, 0) - variableClassReferences, err := n.ReferencedNodes(ctx, refType, ua.BrowseDirectionForward, ua.NodeClassVariable, true) - if err != nil { - sendError(ctx, errors.Errorf("References: %d: %s", refType, err), errChan, logger) - return err - } - refs = append(refs, variableClassReferences...) - objectClassReferences, err := n.ReferencedNodes(ctx, refType, ua.BrowseDirectionForward, ua.NodeClassObject, true) - if err != nil { - sendError(ctx, errors.Errorf("References: %d: %s", refType, err), errChan, logger) - return err - } - refs = append(refs, objectClassReferences...) + nodeClasses := []ua.NodeClass{ua.NodeClassVariable, ua.NodeClassObject} + for _, nodeClass := range nodeClasses { + classReferences, err := n.ReferencedNodes(ctx, refType, ua.BrowseDirectionForward, nodeClass, true) + if err != nil { + sendError(ctx, errors.Errorf("References: %d: %s", refType, err), errChan, logger) + return err + } + refs = append(refs, classReferences...) + } logger.Debugf("found %d child refs\n", len(refs)) for _, rn := range refs { wg.Add(1) go browse(ctx, rn, def.Path, level+1, logger, def.NodeID.String(), nodeChan, errChan, wg, browseHierarchicalReferences, opcuaBrowserChan) } return nil }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
opcua_plugin/browse.go (2)
363-366: Consider adding context cancellation check before spawning goroutines.While the goroutine management is generally correct, spawning new goroutines without checking the context could lead to unnecessary work if the context is already cancelled.
Add a context check:
- wg.Add(1) - go browse(ctx, child, def.Path, level+1, logger, def.NodeID.String(), nodeChan, errChan, wg, browseHierarchicalReferences, opcuaBrowserChan) + select { + case <-ctx.Done(): + return ctx.Err() + default: + wg.Add(1) + go browse(ctx, child, def.Path, level+1, logger, def.NodeID.String(), nodeChan, errChan, wg, browseHierarchicalReferences, opcuaBrowserChan) + }
98-101: Improve logging consistency and performance.The logging patterns are inconsistent and could impact performance due to string concatenation. The [IMPORTANT] tag usage is also inconsistent.
Consider using structured logging and consistent formatting:
- logger.Debugf("[IMPORTANT] browse function called with level: %d, path: %s, parentNodeId: %s", level, path, parentNodeId) - logger.Debugf("[IMPORTANT] The namespace for node %s is %s", n.ID().String(), namespace) + logger.Debugf("browse: start level=%d path=%s parentNodeId=%s", level, path, parentNodeId) + logger.Debugf("browse: namespace=%s nodeId=%s", namespace, n.ID().String()) - logger.Debugf("NodeClass attr[0].Status for node %s is %s", n.ID().String(), attrs[0].Status) - logger.Debugf("NodeClass attr[0].Value for node %s is %s", n.ID().String(), attrs[0].Value.Int()) + logger.Debugf("browse: attribute=NodeClass status=%s value=%d nodeId=%s", + attrs[0].Status, attrs[0].Value.Int(), n.ID().String())Also applies to: 148-149, 172-173, 220-221, 255-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
opcua_plugin/browse.go(15 hunks)opcua_plugin/opcua_node_wrapper.go(2 hunks)opcua_plugin/opcua_unittest_test.go(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: main
opcua_plugin/browse.go
[error] 314-314: Multiple goroutines crashed while browsing OPC UA nodes
🔇 Additional comments (2)
opcua_plugin/opcua_node_wrapper.go (2)
25-26: LGTM! Clean interface extension.The addition of the
Node()method to theNodeBrowserinterface is clean and well-defined.
73-75: LGTM! Simple and correct implementation.The implementation of
Node()inOpcuaNodeWrapperis straightforward and correctly returns the underlying node.
| @@ -489,6 +490,7 @@ | |||
| attributes []*ua.DataValue | |||
| browseName *ua.QualifiedName | |||
| referenceNodes map[uint32][]NodeBrowser | |||
| node *opcua.Node | |||
There was a problem hiding this comment.
💡 Codebase verification
Add tests for the Node() method in MockOpcuaNodeWraper
The codebase analysis confirms that while the Node() method is implemented, it lacks test coverage. Other methods in MockOpcuaNodeWraper are properly tested, but there are no test cases verifying the behavior of the Node() method.
- Add test cases in
opcua_unittest_test.goto verify:- Node() returns the correct node when set
- Node() returns nil when not set (current default behavior)
🔗 Analysis chain
Add tests for the new Node() method.
While the implementation is correct, there are no tests verifying the behavior of the new Node() method in the mock wrapper.
Consider adding test cases to verify:
- Node() returns the correct node when set
- Node() returns nil when not set
Also applies to: 573-575
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any tests for the Node() method
rg -A 5 "TestMockOpcuaNodeWraper.*Node" || echo "No tests found for Node() method"
Length of output: 116
Script:
#!/bin/bash
# Find MockOpcuaNodeWraper implementation
ast-grep --pattern 'type MockOpcuaNodeWraper struct {
$$$
}
func ($_ *MockOpcuaNodeWraper) Node() $_ {
$$$
}'
# Find any test usage of MockOpcuaNodeWraper
rg -A 5 "MockOpcuaNodeWraper" opcua_plugin/opcua_unittest_test.go
Length of output: 2571
| logger.Debugf("[IMPORTANT] The description is nil. So setting the node class to object for node %s", n.ID().String()) | ||
| def.NodeClass = ua.NodeClassObject // by setting it as an object, we will not subscribe to it | ||
| // no need to return here, as we can continue without the Description |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error handling for attribute access failures.
The code inconsistently handles attribute access failures. In some cases, it sets NodeClass to Object as a fallback, which might hide actual issues. This pattern is repeated in multiple places with slightly different implementations.
Consider extracting this pattern into a helper function:
+func handleAttributeError(def *NodeDef, logger Logger, attributeName string) {
+ logger.Warnf("Attribute %s is not readable. Setting node class to object", attributeName)
+ def.NodeClass = ua.NodeClassObject
+}
- logger.Debugf("[IMPORTANT] The description is nil. So setting the node class to object for node %s", n.ID().String())
- def.NodeClass = ua.NodeClassObject // by setting it as an object, we will not subscribe to it
+ handleAttributeError(&def, logger, "Description")
- logger.Debugf("[IMPORTANT] The data type is not readable for node %s. So setting the node class to object", n.ID().String())
- def.NodeClass = ua.NodeClassObject // by setting it as an object, we will not subscribe to it
+ handleAttributeError(&def, logger, "DataType")Also applies to: 305-307
| originalNodeClass, err := n.Node().NodeClass(ctx) | ||
| if err != nil { | ||
| logger.Debugf("Error getting the original node class for node %s: %s", n.ID().String(), err) | ||
| } | ||
| logger.Debugf("originalNodeClass for node %s is %s. def.NodeClass after the fallback condition is %s", n.ID().String(), originalNodeClass, def.NodeClass) |
There was a problem hiding this comment.
Fix potential nil pointer dereference causing goroutine crashes.
The direct call to n.Node().NodeClass() can cause a panic if Node() returns nil. This is causing goroutine crashes as reported in the pipeline failures.
Add a nil check before accessing the node:
- originalNodeClass, err := n.Node().NodeClass(ctx)
- if err != nil {
- logger.Debugf("Error getting the original node class for node %s: %s", n.ID().String(), err)
- }
- logger.Debugf("originalNodeClass for node %s is %s. def.NodeClass after the fallback condition is %s", n.ID().String(), originalNodeClass, def.NodeClass)
+ if node := n.Node(); node != nil {
+ if originalNodeClass, err := node.NodeClass(ctx); err != nil {
+ logger.Debugf("Error getting the original node class for node %s: %s", n.ID().String(), err)
+ } else {
+ logger.Debugf("originalNodeClass for node %s is %s. def.NodeClass after the fallback condition is %s", n.ID().String(), originalNodeClass, def.NodeClass)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| originalNodeClass, err := n.Node().NodeClass(ctx) | |
| if err != nil { | |
| logger.Debugf("Error getting the original node class for node %s: %s", n.ID().String(), err) | |
| } | |
| logger.Debugf("originalNodeClass for node %s is %s. def.NodeClass after the fallback condition is %s", n.ID().String(), originalNodeClass, def.NodeClass) | |
| if node := n.Node(); node != nil { | |
| if originalNodeClass, err := node.NodeClass(ctx); err != nil { | |
| logger.Debugf("Error getting the original node class for node %s: %s", n.ID().String(), err) | |
| } else { | |
| logger.Debugf("originalNodeClass for node %s is %s. def.NodeClass after the fallback condition is %s", n.ID().String(), originalNodeClass, def.NodeClass) | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: main
[error] 314-314: Multiple goroutines crashed while browsing OPC UA nodes
Description
ua.NodeClassVariable | ua.NodeClassObject. This leads to a bug since the code tries to do abitwise ORoperation resulting to the value of0x03. But if you look at the enums ofNodeClassthere is no nodeclass with the value of3. (See the nodeclass image below). This results in an incorrect browsing whenBrowseHierarchicalReferences is trueImage showing the result of Bitwise OR operation
ua.NodeclassVariable | ua.NodeClassObjectOPCUA Browser Records with the PR Change
OPCUA Browser Records before the Change
Summary by CodeRabbit