Skip to content

Add JSON parsing support for YANG structures - #54

Open
Deepya1502 wants to merge 3 commits into
yang-central:mainfrom
Deepya1502:pr/yang-structure-json-support
Open

Deepya1502 wants to merge 3 commits into
yang-central:mainfrom
Deepya1502:pr/yang-structure-json-support

Conversation

@Deepya1502

@Deepya1502 Deepya1502 commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Separates YANG Structure JSON handling from the YANG Library integration PR, per review feedback on #48.

  • Keep YangStructure unchanged; it does not extend TreeNode. Structure members are resolved through its existing SchemaNodeContainer.getSchemaNodeChild() API.
  • YangStructureDataJsonCodec creates the structure data node only. YangStructureMessageJsonCodec decodes declared structure fields once, while message-specific content parsing handles notification payload fields.
  • Unknown structure members are reported as validation errors rather than silently ignored.

Verification

Tests cover valid structure messages, invalid eventTime with exactly one error, and an unknown structure member. A clean mvn test passes across the full reactor after rebasing onto current main.

@lllyfeng lllyfeng left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting this out. Two concerns:

  1. YangStructureDataJsonCodec.java:30-33 builds every field via JsonCodecUtil.buildChildData, but YangStructureMessageJsonCodec.parseHeader (L85) already builds the structure's fields. Result is double parsing: a notification with eventTime "not-a-date" now yields two identical BAD_ELEMENT records (one on main). Please avoid the double build, and report unknown fields instead of silently skipping them.
  2. YangStructure.java:7 now extends TreeNode — this changes getTreeNodeChild/XPath behavior for structures. Please explain the motivation, or drop it if not required.
    Please add unit tests covering: valid structure data, an invalid leaf value (exactly one error), and an unknown member.

@Deepya1502

Copy link
Copy Markdown
Contributor Author

Addressed the review: structure fields are decoded only once, unknown structure members now produce an error, and notification tests verify one error for invalid eventTime plus unknown-member handling. Full mvn test passes. The TreeNode extension is retained because the structure parser needs schema-child lookup for declared members.

@lllyfeng lllyfeng left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — structure fields are now decoded once (verified: invalid eventTime yields exactly one error), unknown members are reported, and the new tests cover both. One question before merging: removing TreeNode from YangStructure.java:7 still passes the full json-codec test suite locally, so the need isn't demonstrated by tests. Since it changes XPath/getTreeNodeChild behavior for structures, could you either add a test that requires it or drop it?

@Deepya1502
Deepya1502 force-pushed the pr/yang-structure-json-support branch from 0d75e6f to 55c58b1 Compare October 13, 2026 14:01
@Deepya1502

Copy link
Copy Markdown
Contributor Author

Addressed the latest review: YangStructure no longer extends TreeNode; its existing SchemaNodeContainer API resolves declared members. Structure fields are decoded once, unknown members are errors, and tests cover valid input, exactly one invalid-eventTime error, and an unknown member. Clean full-reactor mvn test passes after rebase.

@Deepya1502 Deepya1502 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed review comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants