Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

capture the full raw response for Reply/Notificaiton #63

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nemith
Copy link
Owner

@nemith nemith commented Aug 29, 2023

While working with an internal netconf service I discovered that OKResp and other structs were not being parsed properly. This is because xml (and thus encoding/xml) is document focused. An XML document has single root. This means the "defered" parsing of the <rpc-reply> body doesn't work as you need a proper root node (not just a value) as well you cannot support multiple nodes.

To fix this, begrudgingly, this change changes the API to to not just expose the Body to the call but the entire <rpc-reply> to be parsed. The same goes for <notification>.

This could mean slightly larger allocations, but really.... it doesn't matter. The majority of the data would be inside the Body value anyway so adding the wrapping <rpc-reply> doesn't add much.

I think there is room in a v2 to explore a full stream based solution here, but not needed for this. It does make the full io.Reader/io.Writer of RFC6242 kinda.. worthless now, but perhaps this will make things like netconf over QUIC possible. We will see.

Closes: #62

Breaking changes

  • Reply.Body and Notification.Body are now gone. In their place there is a Raw() and String() methods on both structs to return the raw data and a string representation.
  • OkResp was renamed to OKReply
  • GetConfigReply was reworked to assume as the base.

I think there is future work to make a <rpc-reply> drop in for structs to enforce the root node, but this works for now.

@nemith nemith added the breaking-change For PRs that bust the API. To be used for cutting releases label Aug 29, 2023
@nemith nemith force-pushed the nemith/full_msg branch 4 times, most recently from ab4ae13 to 7d95be1 Compare August 31, 2023 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change For PRs that bust the API. To be used for cutting releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reply deferred Body parsing doesn't work with multiple elements under the root.
1 participant