Repository navigation
Conversation
1af29b8 to
1914689
Compare
lllyfeng
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The overall YANG Library integration looks useful, but I found several issues that should be addressed before merging:
IdentityRefStringValueCodecImpl now accepts values even when restriction.evaluate() returns false or throws an exception. This changes validation globally and allows invalid identityref values with a complete schema. Lenient handling should be opt-in and limited to genuinely missing modules.
ListDataJsonCodec downgrades a missing list key to a warning and returns null, silently dropping the entire list entry. This can cause data loss while parsing appears successful. Missing keys should remain errors by default.
YangAbstractDataContainer.validateChildren() now accepts inactive nodes, including nodes disabled by if-feature or deviate not-supported. This weakens validation for every caller, not only partial YANG Library schemas. Please preserve strict validation by default and expose lenient behavior through an explicit option.
The second-pass cross-module augment resolution occurs after the original module.build() errors have already been merged into the validation result. Even if the target is resolved during the second pass, the earlier MISSING_TARGET error remains, so validation may still fail.
When a specific revision is requested, findYangFile() falls back from name@revision.yang to name.yang without verifying the module’s actual revision. This can load the wrong revision while the attached YangSchema reports the requested one.
There are also no new regression tests, and the PR currently has no CI status. The description only mentions a build with tests skipped. Please add tests covering invalid identityrefs, inactive nodes, missing list keys, cross-module augment ordering, and revision mismatches.
For these reasons, I recommend requesting changes before merging.
|
The requested review changes have been addressed and verified. Regression tests pass, and the branch is ready for re-review. |
lllyfeng
left a comment
There was a problem hiding this comment.
Thanks for the updates — identityref validation, missing list key handling, revision fallback and the new tests/CI all look good now. A few items from the previous review are still open, so requesting changes again:
-
Lenient mode is controlled by a static ThreadLocal (LenientValidationOptions). Please pass it as an explicit option/parameter instead; global thread-local state is fragile with thread pools and async callers.
-
CaseImpl/ChoiceImpl unconditionally add children whose if-feature is disabled, and CaseImpl swallows errors from addSchemaNodeChild (e.g. duplicate nodes). This weakens schema validation for all users. Please keep the existing behavior and surface the errors.
-
The cross-module augment test uses module names like "01-auger"/"02-target", which are not valid YANG identifiers, so the augment never resolves. With valid names, main already handles this case. Please fix the fixtures and show a case that actually needs the re-resolution logic, or drop it.
-
Please split out unrelated changes (YangStructure extending TreeNode, BaseImpl/IdentityImpl changes, ValidatorResultBuilder.getRecords returning a mutable list, debug logging in YangYinParser) into separate PRs, and update the PR description.
-
main has moved (#50, #52, #53 merged, including schema path changes), so please rebase and re-run the tests.
Thanks!
- Replace System.out.println with slf4j logger in YangLibraryParser, YangYinParser, YangSchemaContextImpl, YangAbstractDataContainer - Add slf4j-api dependency to yangkit-parser and yangkit-model-impl pom.xml - Remove unused PARSE_LOCK from YangYinParser - Remove unused YANG_LIB_NS constant from YangLibraryParser - Remove unused retry counter vars in YangSchemaContextImpl augment pass - Fix FileInputStream resource leak in YangLibraryParser (try-with-resources) - Remove unused ParsedModule.namespace field - Fix IdentityRefStringValueCodecImpl to use slf4j instead of java.util.logging - Remove duplicate BASE keyword registration in YangStatementImplRegister - Add YangDataUtil.getSchemaNodeContainerForDocument() method - Add missing YangDataUtil import in JsonCodecUtil - Add null guard on getMainModule().getNamespace().getUri() in JsonCodecUtil - Replace NetGauzeYangKafkaConsumer with clean App5YangLibrary example (no Kafka/SR concepts in yangkit; demonstrates all 3 parseFromYangLibrary overloads)
Make lenient handling strictly opt-in and limited to genuinely partial schemas, preserving strict validation by default: - Add LenientValidationOptions (thread-local, strict default). Inactive (if-feature/deviated) nodes, unresolvable identityref/leafref, and missing modules are accepted only while lenient is enabled. - IdentityRefStringValueCodecImpl / IdentityRefJsonCodec: an invalid identityref against a complete schema is always an error (no lenient acceptance); leniency remains only for missing modules. - ListDataJsonCodec: a missing list key stays an ERROR in both modes (was downgraded to a WARNING, silently dropping the entry). - YangSchemaContextImpl: second-pass cross-module augment resolution removes now-stale MISSING_TARGET records. - YangLibraryParser.findYangFile: no unversioned fallback when a revision is requested; checkRevisionMatch verifies the file's declared revision. Add regression tests: LenientValidationBehaviorTest (invalid identityref, inactive node, missing list key, cross-module augment ordering) and YangLibraryRevisionMatchTest (revision match/mismatch).
18e3f1e to
fb61e17
Compare
|
Addressed the latest review requests in commit |
lllyfeng
left a comment
There was a problem hiding this comment.
Thanks — this addresses the previous review well: explicit LenientValidationOptions instead of the ThreadLocal, Case/Choice now propagate addSchemaNodeChild results, augment re-resolution and the unrelated changes are gone. A few remaining items before merge:
- YangSchemaContext.java:24/26 — please make getValidationOptions()/setValidationOptions() default methods (e.g. return LenientValidationOptions.STRICT) so external implementations don't break.
- YangStatementImplRegister.java:72 — the BASE registration change (GRAMMAR+SCHEMA_BUILD) belongs to #55, which actually depends on it; please move it there and leave the registrations untouched here.
- JsonCodecUtil.java:561 — the comment about "missing list key downgraded to a WARNING" no longer matches the code.
- Minor: indentation at ChoiceImpl.java:322; missing trailing newline in LenientValidationOptions.java.
|
Follow-up to the latest review: |
lllyfeng
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — default methods on YangSchemaContext, BASE registration moved out, stale comment fixed. Full reactor build/tests pass locally on top of main. Minor nit (non-blocking): indentation around ChoiceImpl.java:320-322. LGTM.
Summary
Adds RFC 8525 YANG Library parsing and opt-in lenient handling for partial schemas. The parser builds a schema context from the library XML and module directory; schema and data processing use the validation policy attached to that context.
Validation behavior
LenientValidationOptions.LENIENT, for example viaYangLibraryParser.parse(xmlFile, modulesDir, options)or the correspondingYangYinParser.parseFromYangLibraryoverload. The immutable option is stored on the schema context; there is no static or thread-local state.YangSchemaContext.getValidationOptions()defaults toSTRICT, and its setter is a default no-op, preserving compatibility for external implementations.CaseImplmerges child insertion validation results.Review follow-up
BASESCHEMA_BUILDregistration, was split into PR #55. This PR keeps BASE registered forGRAMMARonly.ValidatorResultBuilder.getRecords()addition andYangYinParserdebug logging were dropped. The list-key severity comment now matches the behavior.Verification
Rebased onto
yang-central/mainat573a6f18(including merged PRs #50, #52, and #53). Fullmvn testpasses across the reactor, including lenient-validation and revision-match regressions.