Repository navigation
feat: strict anydata validation with payload schema context - #50
Conversation
Non-empty anydata content now requires a matching payload schema context resolved via AnydataValidationSupport. On resolution failure the outer anydata node is created without a payload value and an operation-failed record is added; the enclosing document schema is never used as a fallback, and content is never silently discarded. Primitive anydata payloads (string/number/boolean/null/array) produce ErrorTag.BAD_ELEMENT records per libyang's structured-input rule for anydata, while preserving Yangkit's validation result format. Validating the enclosing document recursively validates the nested payload document, returning standard yangkit validation records for mandatory, must, cardinality, unique, type, and leafref constraints with paths rooted beneath the enclosing anydata node. Applies to the JSON, XML, CBOR, and Protocol Buffers data codecs. Updates tests and module READMEs accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lllyfeng
left a comment
There was a problem hiding this comment.
Hi @Deepya1502, welcome, and thanks for this substantial contribution! Recursive validation of anydata payloads, with paths rebased under the anydata node, is something we've wanted for a while. Using the payload document's own schema context when serializing is also a real bug fix.
I checked out the branch, merged it with current main, and ran the full build. Everything compiles and all tests pass. CI hasn't started yet because the workflow needs approval for first-time contributors; I'll approve that separately so the checks can run.
My main concern is backward compatibility:
-
A missing payload schema should not be an error by default. RFC 7950 §7.10 describes anydata as data "for which the data model is not known at module design time", and says knowing it later is possible but "not required". With this PR, any non-empty anydata parsed without a resolver produces an Error-level
operation-failedrecord, and the content is dropped. I confirmed this breaks existing callers: the originalJsonCodecNotificationsTestfrommainfails 2/5 on this branch (yp:datastore-contentsandyp:valuefrom YANG-Push). The PR only passes because it changes that test to supply a resolver. Could we make the strict behavior opt-in? For example, addAnydataValidationOptions#requirePayloadSchema(boolean)(defaultfalse). When it's off, keep today's fallback to the enclosing schema context. When it's on, use the newoperation-failedbehavior. Then please revert the change toJsonCodecNotificationsTest, so it keeps guarding the default behavior. -
Rejecting primitive anydata is fine. RFC 7951 §5.5 says anydata is encoded like a container (a name/object pair), so rejecting strings, numbers, booleans, null and arrays in JSON and CBOR is spec-compliant. Please mention it in the PR description as a behavior change so we can call it out in the release notes.
-
Please split out the unrelated changes. The XML codec recursion refactor,
addExistingListKey, and theuniquenull-schema-path fix look useful, but they're independent of anydata and don't have their own tests. Separate PRs would make them much easier to review and bisect.
Details are in the inline comments. Thanks again, happy to help iterate on this!
Make missing payload schemas an opt-in strict error, restore notification compatibility tests, and assert rebased validation paths. Remove unrelated XML and unique-schema-path changes from this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks @Deepya1502 for the quick turnaround, and for splitting the unrelated changes into #51, #52 and #53. That makes review much easier. The opt-in A few remaining items:
Merge order: #50 first. Then please rebase #51, #52 and #53, since they conflict with #50 and with each other. |
Deepya1502
left a comment
There was a problem hiding this comment.
All the review comments are addressed, please review.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the feedback @lllyfeng. Updated the PR description, I added a short comment in AnyDataDataImpl#buildAnydataPath explaining that a root parent has no enclosing path to prepend. I also updated AnydataValidationContextResolver with a default isRequirePayloadSchema() method, overridden it in AnydataValidationOptions , and changed schema resolution to use the interface method. This allows custom resolvers to enable strict mode while preserving the default fallback behavior and lambda compatibility. Added a JSON codec regression test for a custom strict resolver; the targeted test passed. |
lllyfeng
left a comment
There was a problem hiding this comment.
Thanks @Deepya1502, all review points are addressed and CI is green. Approving and merging.
Summary
nullanydata values withBAD_ELEMENT, as required by RFC 7951 §5.5.The branch is rebased onto the current yang-central/yangkit main.