Fix spurious cascading errors on when-false nodes in multi-error validation#2528
Open
szabolcs-szekely wants to merge 4 commits into
Open
Fix spurious cascading errors on when-false nodes in multi-error validation#2528szabolcs-szekely wants to merge 4 commits into
szabolcs-szekely wants to merge 4 commits into
Conversation
…errors Mark when-false nodes with a new LYD_WHEN_FALSE flag so XPath evaluation treats them as non-existent (LY_ENOT) instead of incomplete (LY_EINCOMPLETE). This prevents the internal-error crash in lyd_validate_dummy_when and spurious cascading errors on must, leafref, and child when expressions under LYD_VALIDATE_MULTI_ERROR. Clean up node_types and node_when entries for descendants of a when-false node so leafref validation and child when evaluations do not run against a logically non-existent subtree. Add regression tests covering the when/must cross-reference scenario and the leafref-inside-when-false-container case. Fixes CESNET#2516
Add a fourth sub-case to test_when_must_cross_ref that places the when-false node on a list with a key and references it through a [key='...'] predicate in a sibling must constraint. This routes the XPath step through moveto_node_hash_child rather than moveto_node_check, exercising the second LYD_WHEN_FALSE branch in src/xpath.c explicitly. The data deliberately sets item[name='a']/value to 'expected' so that a regression in the hash-lookup branch would cause the must to pass silently. Both the when error and the must failure must be reported. Fixes CESNET#2516
Address review findings on the LYD_WHEN_FALSE handling. - Initialize the return code before skipping a when-false node in lyd_validate_final_r so a when-false first sibling no longer reads an uninitialized value. - Clear a stale LYD_WHEN_FALSE mark when a when later resolves to true so XPath no longer treats a now-valid node as non-existent on revalidation. - Skip container default handling for when-false nodes in addition to their child validation. - Add a nested when-in-when regression test confirming the node_when descendant cleanup suppresses the spurious child when error.
michalvasko
requested changes
Jun 4, 2026
Member
michalvasko
left a comment
michalvasko
left a comment
There was a problem hiding this comment.
Okay, I suppose, since it does not add too much code. But like I said before, this feature was added later and the original design did not anticipate it so it is expected it does not work perfectly. For a proper implementation, the validation would need significant refactoring, which seems not worth it.
Comment on lines
+439
to
+476
| /* invalid data; mark the when as resolved-to-false so subsequent XPath | ||
| * evaluations on this node return "no match" instead of LY_EINCOMPLETE */ | ||
| node->flags |= LYD_WHEN_FALSE; | ||
| /* remove descendants from node_types so their leafrefs are not validated | ||
| * against the when-false subtree, which would produce spurious cascading | ||
| * errors (mirrors the cleanup done by lyd_validate_autodel_node_del) */ | ||
| if (node_types && node_types->count) { | ||
| struct lyd_node *iter; | ||
| uint32_t idx; | ||
|
|
||
| LYD_TREE_DFS_BEGIN(node, iter) { | ||
| if ((iter->schema->nodetype & LYD_NODE_TERM) && | ||
| LYSC_GET_TYPE_PLG(((struct lysc_node_leaf *)iter->schema)->type->plugin_ref)->validate_tree && | ||
| ly_set_contains(node_types, iter, &idx)) { | ||
| ly_set_rm_index(node_types, idx, NULL); | ||
| } | ||
| LYD_TREE_DFS_END(node, iter); | ||
| } | ||
| } | ||
| /* remove descendants from node_when so their own when conditions are not | ||
| * evaluated; a when-false subtree is logically non-existent, so child | ||
| * whens are moot and would otherwise produce spurious cascading errors */ | ||
| if (node_when->count > 1) { | ||
| struct lyd_node *iter; | ||
| uint32_t idx; | ||
|
|
||
| count = node_when->count; | ||
| LYD_TREE_DFS_BEGIN(node, iter) { | ||
| if ((iter != node) && ly_set_contains(node_when, iter, &idx)) { | ||
| ly_set_rm_index_ordered(node_when, idx, NULL); | ||
| } | ||
| LYD_TREE_DFS_END(node, iter); | ||
| } | ||
| if (count > node_when->count) { | ||
| /* descendants were removed, refresh the iteration index */ | ||
| ly_set_contains(node_when, node, &i); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
With this code, the function is too long, please put this into a separate function. Also, it may impact performance so why not use the new flag LYD_WHEN_FALSE only for LYD_VALIDATE_MULTI_ERROR validation?
Author
There was a problem hiding this comment.
I separated the function and attached the LYD_WHEN_FALSE flag only for the LYD_VALIDATE_MULTI_ERROR mode with commit f78b246
szabolcs-szekely
added a commit
to nokia/CESNET-libyang
that referenced
this pull request
Jun 5, 2026
Address the maintainer review on PR CESNET#2528. - Move the when-false marking and node_types/node_when descendant cleanup out of lyd_validate_unres_when into a dedicated lyd_validate_when_false helper to keep the caller short. - Apply the LYD_WHEN_FALSE handling only for LYD_VALIDATE_MULTI_ERROR. In single-error validation the first when failure stops validation, so the flag and the cleanup loops are needless work. Co-authored-by: Cursor <cursoragent@cursor.com>
Address the maintainer review on PR CESNET#2528. - Move the when-false marking and node_types/node_when descendant cleanup out of lyd_validate_unres_when into a dedicated lyd_validate_when_false helper to keep the caller short. - Apply the LYD_WHEN_FALSE handling only for LYD_VALIDATE_MULTI_ERROR. In single-error validation the first when failure stops validation, so the flag and the cleanup loops are needless work.
szabolcs-szekely
force-pushed
the
fix/when-false-xpath-multi-error
branch
from
7b2acbf to
f78b246
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relates to #2516.
We want to use yanglint's multi-error mode (
LYD_VALIDATE_MULTI_ERROR) for testing. Building on the crash fix in ff3cd62, awhen-false node was still indistinguishable from a not-yet-evaluatedwhen: XPath returnedLY_EINCOMPLETEfor it, andmust/leafref/child-whenran against the logically non-existent subtree, producing spurious cascading errors.Changes:
LYD_WHEN_FALSEflag to mark awhen-false node distinctly from a not-yet-evaluated one, cleared again if thewhenlater resolves to true.LYD_WHEN_FALSEnodes as non-existent (LY_ENOT) instead ofLY_EINCOMPLETE.must/obsolete/child/container-default checks for these nodes and removes their descendants fromnode_types/node_when.when/mustcross-reference (incl. keyed-list hash lookup), the leafref-inside-when-false-container case, and a nestedwhen-in-whencase.