[ENG-2320] Reduce cyclomatic complexity of the browse function #119
Conversation
WalkthroughThe pull request introduces significant refactoring to the OPC UA plugin's node browsing functionality, specifically in Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ 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 (5)
opcua_plugin/node_attributes.go (4)
11-19: Consider making usage examples or docstrings clearer.
TheAttributeHandlerstruct is well laid out, but it might be helpful to include usage comments or docstrings in its fields to guide future maintainers and reduce confusion about how each flag (e.g.,affectsNodeClass) should be used in context.
21-59: Consider returning user-friendly errors or status.
handleAttributeStatusreturns an unwrappedattr.Statusor a generic error message in some cases. Consider returning more contextual error messages ifattr.Statusis notua.StatusOKto provide additional information, especially for debugging or logging.
79-161: Consider simplifying high-level flow inprocessNodeAttributes.
While the function is clear in purpose, the repetitive pattern of callinghandleAttributeStatusfor each attribute could be further streamlined using shared logic. If performance isn’t the highest priority here, a small refactor or a loop-based approach might reduce boilerplate.
163-183: Add coverage for unknown DataType cases.
The fallback conversion ingetDataTypeStringreturns a string withns=0and the rawtypeID. Consider logging or returning a specific string to highlight unknown types, so downstream processes can differentiate truly unknown types from ones that map to standard primitives.opcua_plugin/browse.go (1)
137-138: Potential micro-optimization.
Rather than manually checkingpath != ""and concatenating with a dot, you could reuse the existingjoinfunction further below for path assembly, keeping path concatenation consistent and reducing possible logic duplication.- newPath = path + "." + newPath + newPath = join(path, newPath)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
opcua_plugin/browse.go(1 hunks)opcua_plugin/node_attributes.go(1 hunks)opcua_plugin/opcua_unittest_test.go(1 hunks)
🔇 Additional comments (5)
opcua_plugin/node_attributes.go (1)
68-77: Review forced NodeClass assignment on non-readable attributes.
Whenattr.Statusisua.StatusBadNotReadableandhandler.affectsNodeClassis true, the code forcibly sets theNodeClasstoua.NodeClassObject. This might unintentionally mask the node’s actual class. Ensure that this behavior aligns with your domain requirements.opcua_plugin/browse.go (3)
136-136: Good use of sanitize for path construction.
This ensures invalid characters in thebrowseName.Nameare replaced, preventing potential naming or path issues elsewhere.
142-144: Struct field assignments are clear and concise.
InitializingNodeDeffields early in one place promotes clarity, making the code easier to follow and reducing the risk of missing an assignment.
147-148: EnsureprocessNodeAttributeshandles misaligned attributes array.
Though you check the number of attributes elsewhere, confirm in your tests that unexpected array lengths (e.g., fewer or extra attributes) don’t cause index out-of-bounds errors or silent failures.opcua_plugin/opcua_unittest_test.go (1)
118-118: Updated test error message aligns with new error handling logic.
The new message “attribute value is nil for NodeClass” is clearer and more specific, reflecting the revised code’s behavior. Ensure any other tests referencing this message are updated accordingly.
kanapuli
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
opcua_plugin/node_attributes.go (4)
11-19: Consider enhancing field documentation.While the struct is well-documented, consider adding type information to the field comments for better clarity. For example:
// handleOK is the function that handles the OK status // Type: func(value *ua.Variant) error
21-58: Improve error messages and code structure in handleAttributeStatus.Consider these improvements:
- Use more descriptive error messages by including the attribute name in all error cases
- Implement early returns to reduce nesting and improve readability
func handleAttributeStatus( name string, attr *ua.DataValue, def *NodeDef, path string, logger Logger, handler AttributeHandler, ) error { if attr == nil { - return fmt.Errorf("attribute is nil") + return fmt.Errorf("attribute %s is nil", name) } if attr.Status == ua.StatusOK { if attr.Value == nil && handler.requiresValue { return fmt.Errorf("attribute value is nil for %s", name) } return handleOKStatus(attr.Value, handler) } if attr.Status == ua.StatusBadSecurityModeInsufficient { - return errors.New("insufficient security mode") + return fmt.Errorf("insufficient security mode for attribute %s", name) } if attr.Status == ua.StatusBadAttributeIDInvalid { if handler.ignoreInvalidAttr { return nil } - return fmt.Errorf("invalid attribute ID") + return fmt.Errorf("invalid attribute ID for %s", name) } if attr.Status == ua.StatusBadNotReadable { handleNotReadable(def, handler, path, logger) return nil } return fmt.Errorf("unexpected status %v for attribute %s", attr.Status, name)
158-178: Consider enhancing dataTypeMap coverage.
Consider adding support for additional OPC UA data types:
- Int64/UInt64
- ByteString
- XmlElement
- LocalizedText
- QualifiedName
Add validation for the fallback format in getDataTypeString:
func getDataTypeString(typeID uint32) string { if dtype, ok := dataTypeMap[typeID]; ok { return dtype } + if typeID == 0 { + return "unknown" + } return fmt.Sprintf("ns=%d;i=%d", 0, typeID) }
1-178: Well-structured extraction of node attribute handling.The code successfully achieves its goal of reducing cyclomatic complexity by extracting node attribute handling into a separate, well-organized module. The implementation follows Go best practices and provides comprehensive error handling.
The AttributeHandler pattern provides good flexibility and separation of concerns, making the code more maintainable and testable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
opcua_plugin/node_attributes.go(1 hunks)
🔇 Additional comments (1)
opcua_plugin/node_attributes.go (1)
1-9: LGTM! Clean package structure and imports.The package name and imports are well-organized and follow Go conventions.
| if handler.handleNotReadable != nil { | ||
| handler.handleNotReadable() | ||
| } | ||
| logger.Warnf("Access denied for node: %s, continuing...\n", path) |
There was a problem hiding this comment.
be careful about warning, they will prevent deployment in the management console
There was a problem hiding this comment.
The existing code in master branch uses logger.Warnf for this scenario and the status quo is maintained.
opcua_plugin/node_attributes.go
Outdated
| return nil | ||
| }, | ||
| requiresValue: true, | ||
| affectsNodeClass: true, |
There was a problem hiding this comment.
maybe explain for those edge cases with a short comment why it requires a value and it affects the node class. for nodeclass, quite obvious, for description maybe not that obvious
There was a problem hiding this comment.
Comments are added now
|
Also checkout htis nitpick comment from coderabbit. might make debugging in the future easier: |
Addressed |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
opcua_plugin/node_attributes.go (2)
20-57: Consider using constants and early returns for better readability.The error handling is thorough, but the code could be more maintainable with these improvements:
+// OPC UA status constants for better readability +const ( + statusOK = ua.StatusOK + statusBadSecurityMode = ua.StatusBadSecurityModeInsufficient + statusBadAttributeID = ua.StatusBadAttributeIDInvalid + statusBadNotReadable = ua.StatusBadNotReadable +) func handleAttributeStatus( name string, attr *ua.DataValue, def *NodeDef, path string, logger Logger, handler AttributeHandler, ) error { if attr == nil { return fmt.Errorf("attribute is nil for node: %s and attribute: %s", def.NodeID, name) } - if attr.Status == ua.StatusOK { - if attr.Value == nil && handler.requiresValue { - return fmt.Errorf("attribute value is nil for node: %s and attribute: %s", def.NodeID, name) - } - return handleOKStatus(attr.Value, handler) + switch attr.Status { + case statusOK: + if attr.Value == nil && handler.requiresValue { + return fmt.Errorf("attribute value is nil for node: %s and attribute: %s", def.NodeID, name) + } + return handleOKStatus(attr.Value, handler) + case statusBadSecurityMode: + return fmt.Errorf("got insufficient security mode for node: %s and attribute: %s", def.NodeID, name) + case statusBadAttributeID: + if handler.ignoreInvalidAttr { + return nil + } + return fmt.Errorf("invalid attribute ID for node: %s and attribute: %s", def.NodeID, name) + case statusBadNotReadable: + handleNotReadable(def, handler, path, logger) + return nil } - - if attr.Status == ua.StatusBadSecurityModeInsufficient { - return fmt.Errorf("got insufficient security mode for node: %s and attribute: %s", def.NodeID, name) - } - - if attr.Status == ua.StatusBadAttributeIDInvalid { - if handler.ignoreInvalidAttr { - return nil - } - return fmt.Errorf("invalid attribute ID for node: %s and attribute: %s", def.NodeID, name) - } - - if attr.Status == ua.StatusBadNotReadable { - handleNotReadable(def, handler, path, logger) - return nil - } return attr.Status }
157-177: Add documentation for the default format.Consider adding a comment explaining the format used when a type ID is not found in the map.
func getDataTypeString(typeID uint32) string { if dtype, ok := dataTypeMap[typeID]; ok { return dtype } + // Return the OPC UA node ID format (ns=0;i=<typeID>) for unknown types return fmt.Sprintf("ns=%d;i=%d", 0, typeID) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
opcua_plugin/node_attributes.go(1 hunks)opcua_plugin/opcua_unittest_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- opcua_plugin/opcua_unittest_test.go
🔇 Additional comments (5)
opcua_plugin/node_attributes.go (5)
10-18: Well-structured AttributeHandler definition!The struct is well-documented with clear field purposes and follows idiomatic Go naming conventions.
59-65: Clean and focused implementation!The function has appropriate nil checks and follows the single responsibility principle.
67-76: Implementation aligns with existing patterns!The warning log message is maintained as per the discussion in previous reviews, keeping consistency with the master branch.
78-155:⚠️ Potential issueAdd safety checks and constants for attribute indices.
The function needs validation and better maintainability:
+// Node attribute indices for better maintainability +const ( + nodeClassIdx = iota + browseNameIdx + descriptionIdx + accessLevelIdx + dataTypeIdx + numRequiredAttrs +) func processNodeAttributes(attrs []*ua.DataValue, def *NodeDef, path string, logger Logger) error { + // Validate attributes slice length + if len(attrs) < numRequiredAttrs { + return fmt.Errorf("insufficient attributes: got %d, want %d", len(attrs), numRequiredAttrs) + } + // NodeClass - if err := handleAttributeStatus("NodeClass", attrs[0], def, path, logger, nodeClassHandler); err != nil { + if err := handleAttributeStatus("NodeClass", attrs[nodeClassIdx], def, path, logger, nodeClassHandler); err != nil {Apply similar changes for other attribute indices throughout the function.
Likely invalid or redundant comment.
20-177: Verify test coverage for critical functions.The following functions need unit test coverage:
handleAttributeStatushandleOKStatushandleNotReadable✅ Verification successful
Functions are adequately tested through integration tests
The attribute handling functions are thoroughly tested through the Browse functionality tests in
opcua_unittest_test.go. The test suite covers:
- Error handling for nil attributes
- Invalid attribute scenarios
- Not readable attributes
- Security mode issues
- Various node classes and attribute combinations
The integration tests provide good coverage through mock implementations that simulate real OPC UA node behaviors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for attribute handling functions echo "=== Searching for existing test cases ===" rg -A 5 "Test(HandleAttributeStatus|HandleOKStatus|HandleNotReadable)" --type go echo -e "\n=== Checking test coverage ===" go test -coverprofile=coverage.out ./... go tool cover -func=coverage.out | grep -E "handleAttribute|handleOK|handleNotReadable"Length of output: 20979
Script:
#!/bin/bash # Check for test files and any indirect coverage of the target functions echo "=== Listing test files in opcua_plugin ===" fd "_test\.go$" opcua_plugin/ echo -e "\n=== Searching for any tests that might indirectly use these functions ===" rg -l "handleAttribute|processNodeAttributes" opcua_plugin/ --type go echo -e "\n=== Checking function implementations ===" rg "^func (handleAttribute|handleOK|handleNotReadable|processNodeAttributes)" opcua_plugin/ --type goLength of output: 1227
Script:
#!/bin/bash # Check for indirect testing through browse functionality echo "=== Checking for browse-related tests ===" rg -A 10 "func Test.*Browse" opcua_plugin/ --type go echo -e "\n=== Checking unittest file for relevant tests ===" cat opcua_plugin/opcua_unittest_test.go echo -e "\n=== Checking for any test usage of NodeDef struct ===" rg "NodeDef" opcua_plugin/ --type go -C 5Length of output: 59310
Description
The
browsefunction is too long to understand and there are a lot of branch conditions which has an higher cyclomatic complexityCurrent cyclomatic complexity score after refactoring the browse function = 15
Previous cyclomatic complexity score for the browse function = 51
Summary by CodeRabbit
New Features
Refactor
Bug Fixes