Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/unit/tasks/conductor/sonic/test_validator.py" line_range="54" />
<code_context>
+ ), errors
+
+
+def test_union_leafref_accepts_either_target():
+ """VLAN_MEMBER.port is a union of PORT and PORTCHANNEL leafrefs."""
+ config = {
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for leafrefs that resolve via non-key target fields (inner row values, not row keys).
Right now tests only cover leafrefs whose targets are found via row keys or leaf-lists, but `_collect_target_keysets` also pulls values from non-key fields in the row dict. Please add a test where `target_field` is a non-key field and the referenced value appears only in that inner field (e.g., `TUNNEL.src_ip -> PEER_SWITCH.address_ipv4` with mismatched row keys but matching inner field), and assert the validator accepts it. This will exercise and protect the non-key-path logic in `_collect_target_keysets`.
</issue_to_address>
### Comment 2
<location path="tests/unit/tasks/conductor/sonic/test_validator.py" line_range="85" />
<code_context>
+ assert "PORT.name" in msg and "PORTCHANNEL.name" in msg
+
+
+def test_leaf_list_of_leafrefs_checks_each_element():
+ config = {
+ "BUFFER_PROFILE": {"p1": {}, "p2": {}},
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test where target tables exist but are empty, to confirm missing references are still flagged.
The `_check_leafrefs` docstring states that configs missing any declared target tables should still flag unresolvable references. Current tests cover happy paths and some failures but not the case where target tables exist and are empty. Please add a test with a source table containing leafref values and empty target tables (e.g., empty `PORT` or `BUFFER_PROFILE`) and assert that leafref errors are raised, to ensure `_collect_target_keysets` / `_value_in_any_target` don’t treat empty targets as a success case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ), errors | ||
|
|
||
|
|
||
| def test_union_leafref_accepts_either_target(): |
There was a problem hiding this comment.
suggestion (testing): Add a test for leafrefs that resolve via non-key target fields (inner row values, not row keys).
Right now tests only cover leafrefs whose targets are found via row keys or leaf-lists, but _collect_target_keysets also pulls values from non-key fields in the row dict. Please add a test where target_field is a non-key field and the referenced value appears only in that inner field (e.g., TUNNEL.src_ip -> PEER_SWITCH.address_ipv4 with mismatched row keys but matching inner field), and assert the validator accepts it. This will exercise and protect the non-key-path logic in _collect_target_keysets.
| assert "PORT.name" in msg and "PORTCHANNEL.name" in msg | ||
|
|
||
|
|
||
| def test_leaf_list_of_leafrefs_checks_each_element(): |
There was a problem hiding this comment.
suggestion (testing): Consider a test where target tables exist but are empty, to confirm missing references are still flagged.
The _check_leafrefs docstring states that configs missing any declared target tables should still flag unresolvable references. Current tests cover happy paths and some failures but not the case where target tables exist and are empty. Please add a test with a source table containing leafref values and empty target tables (e.g., empty PORT or BUFFER_PROFILE) and assert that leafref errors are raised, to ensure _collect_target_keysets / _value_in_any_target don’t treat empty targets as a success case.
Per-table Pydantic validation only confirms each row is well-formed; YANG leafref semantics — a leaf must point at an existing value in another path — were treated as plain strings, so a PORTCHANNEL_MEMBER referencing a non-existent PORT silently passed. Catch these before pushing to the device. - tools/sonic_yang_to_pydantic.py walks every leafref (including unions and leaf-lists), parses the XPath to (target_table, target_field), and emits a registry alongside the per-table schemas. Relative paths and predicate- bearing XPaths are deliberately skipped (issue out-of-scope). - osism/tasks/conductor/sonic/_generated/_leafrefs.py is the committed output (137 deduplicated constraints across 205 tables); regenerated via the tool. - validator.py runs a post-pass after row validation: for each constraint it walks rows in source_table and checks that each value resolves to one of the targets. Unions succeed if any target accepts; leaf-lists are checked per element; single-key lists fall back to the row key when the field is not present in the row dict. Composite-key parsing remains out of scope. - Failures surface as ValidationError(table=<source>, path="<row>.<field>") so the existing text/JSON report stays consistent. Closes #2252 AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>
Per-table Pydantic validation only confirms each row is well-formed; YANG leafref semantics — a leaf must point at an existing value in another path — were treated as plain strings, so a PORTCHANNEL_MEMBER referencing a non-existent PORT silently passed. Catch these before pushing to the device.
Closes #2252
AI-assisted: Claude Code