fix: Add recursive validation for nested OPC-UA structures#87
Conversation
Fix validation error when adding Function Block instances to OPC-UA address space. The previous validation only checked the first level of fields, causing errors like "Invalid datatype 'TON'" for nested FB instances. Changes: - Add recursive validate_field_datatypes() that only validates leaf fields (those without nested children) - Complex types (FBs, nested structs) are skipped, only their leaf children are validated - Add missing IEC 61131-3 base types to VALID_DATATYPES: SINT, USINT, UINT, UDINT, ULINT, LREAL, WORD, DWORD, LWORD The validation now mirrors the recursive pattern used for index collection (collect_field_indices). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a validation error that occurred when adding Function Block instances with nested structures to the OPC-UA address space. The validation was incorrectly rejecting intermediate complex types (like TON, TOF) instead of only validating leaf field datatypes.
Changes:
- Added recursive validation function that skips complex types and only validates leaf fields
- Extended VALID_DATATYPES to include missing IEC 61131-3 base types (SINT, USINT, UINT, UDINT, ULINT, LREAL, WORD, DWORD, LWORD)
- Reorganized VALID_DATATYPES with categorized comments for better maintainability
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Helper to validate datatypes recursively for nested fields | ||
| # Only leaf fields (those without nested children) are validated | ||
| def validate_field_datatypes( | ||
| fields: List[VariableField], | ||
| struct_node_id: str, | ||
| plugin_name: str | ||
| ) -> None: |
There was a problem hiding this comment.
The helper function validate_field_datatypes is defined inside the model validator method. Consider defining it at module level or as a private function to improve testability and reusability.
| # TIME-related types (IEC 61131-3) | ||
| # Time-related types | ||
| "TIME", "DATE", "TOD", "DT", | ||
| # Legacy/alternative names (for backward compatibility) |
There was a problem hiding this comment.
The comment indicates these are 'Legacy/alternative names' but doesn't specify which standard types they map to. Consider documenting that INT32 maps to DINT and FLOAT maps to REAL for clarity.
| # Legacy/alternative names (for backward compatibility) | |
| # Legacy/alternative names (for backward compatibility): | |
| # INT32 -> DINT, FLOAT -> REAL |
thiagoralves
left a comment
There was a problem hiding this comment.
Review Comments
1. Missing null-check on datatype (potential bug)
At line 494 in opcua_config_model.py:
if field.datatype.upper() not in VALID_DATATYPES:If field.datatype is None or empty, this will raise AttributeError. While the data model shows datatype: str (non-optional), defensive coding would be safer.
Suggestion:
if not field.datatype or field.datatype.upper() not in VALID_DATATYPES:2. Error messages lack full path for deeply nested fields
For deeply nested structures, the error message only shows:
Invalid datatype 'X' for field 'ET' in struct 'PLC.main.CONTROLLER'
For a structure like CONTROLLER.TON0.ET, this doesn't show the intermediate path, which makes debugging harder.
Suggestion - pass and accumulate the path:
def validate_field_datatypes(fields, struct_node_id, plugin_name, path=""):
for field in fields:
current_path = f"{path}.{field.name}" if path else field.name
if field.fields:
validate_field_datatypes(field.fields, struct_node_id, plugin_name, current_path)
else:
if field.datatype.upper() not in VALID_DATATYPES:
raise ValueError(
f"Invalid datatype '{field.datatype}' for field '{current_path}' "
f"in struct '{struct_node_id}' in plugin '{plugin_name}'. "
f"Valid types: {sorted(VALID_DATATYPES)}"
)Overall the fix is correct and addresses the validation bug properly. These are minor improvements to consider.
- Add null-check on datatype for defensive coding - Include full field path in error messages for easier debugging (e.g., "TON0.ET" instead of just "ET") Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Problem
When adding a Function Block instance (e.g.,
IRRIGATION_MAIN_CONTROLLER0) to the OPC-UA variables list, the runtime failed with:The validation was checking the datatype of intermediate complex types (
TON,TOF, custom FBs) instead of only validating the leaf fields.Root Cause
The datatype validation only checked the first level of fields:
But nested structures require recursive validation that skips complex types (those with nested
fields) and only validates leaf fields.Solution
Added recursive
validate_field_datatypes()that mirrors the existingcollect_field_indices()pattern:fields-> recurse into children (don't validate parent datatype)Added missing IEC 61131-3 base types to
VALID_DATATYPES:SINT,USINT,UINT,UDINT,ULINT(integer types)LREAL(double-precision float)WORD,DWORD,LWORD(bit string types)Test plan
Generated with Claude Code