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

Reply deferred Body parsing doesn't work with multiple elements under the root. #62

Open
nemith opened this issue Aug 29, 2023 · 0 comments · May be fixed by #63
Open

Reply deferred Body parsing doesn't work with multiple elements under the root. #62

nemith opened this issue Aug 29, 2023 · 0 comments · May be fixed by #63
Labels
bug Something isn't working help wanted Extra attention is needed investigation Needs some time to invesigate an isssue

Comments

@nemith
Copy link
Owner

nemith commented Aug 29, 2023

The <rpc-reply> element defined in RFC6241 has really two uses.

  1. The session/netconf library uses to parse an incoming type (reply vs notification, etc) as well as data about the reply it self (message-id)
  2. Anyone calling custom RPCs against the netconf server reads the reply out of it.

So far this has been done with the Reply object by added aBody []byte xml:,innerxmlto capture the remaining nodes to allowed for deferred xml deoding. Even theReplystruct has a method calledDecodeused to decode theBodyinto a Go struct. This was very much modeled after jsonsjson.RawMessage` API and assumed to work similarly.

This works 90% of the time as the reply message is a single element and a single element looks like document to XML. This, however, breaks when there are multiple elements.

Multiple nodes

For example the following doesn't work: Assume the netconf server replies with the message

   <rpc-reply message-id="101"">
       <foo>foo</foo>
       <bar>42</bar>
   </rpc-reply>
reply, err := session.Do(ctx.TODO(), req)

var resp struct {
    Foo  string `xml:"foo"`
    Bar  int    `xml"int"`
} 

if err := reply.Decode(&resp); err != nil { /* ... *}

Because encoding/xml expects a document (i.e a single root element) Decode method on Reply doesn't DO ANYTHING and just silently fails. The fields inside resp are of the zero type and nothing happens.

The RFC explicitly allows for this behavior:

https://www.rfc-editor.org/rfc/rfc6241#section-4.2 (emphasis mine):

The response data is encoded as one or more child elements to the element.

This also comes up in a lot of Junos RPCs that may return a <ok/> along with data elements (even though it's a bit redundant and against the RF

Single nodes as well

This also just breaks with a single element. We have defined in the library the following

type OKResp struct {
	OK ExtantBool `xml:"ok"`
}

If passed the same way into reply.Decode() (or session.Call()) the<ok/> is never parsed and is always false. This only really works if the resp structure has a XMLNode defined or the name of the struct itself is the single xml node.

Potential fixes

  1. The easy solution is to allow the user to re-decode the entire rpc-reply message. Elements <rpc-error> can be omitted if the user doesn't want to parse it. This is probably the direction to go in and gives the most flexibility while fitting the original goals of this library.

  2. some sort of fancy "rewrapping" of objects to make them documents to allow for parsing (i.e adding a fake root node) to the input data and the parsing data to appease the decoder. This is probably doable but also needs a lot of testing around corner cases to work properly and is a bit too "weird"

  3. Nothing. We return the bytes up to the caller to handle parsing (i.e parsing multiple "documents"). This seems the least useful and more hostile to the users of the library..

Either way I am glad I ran into this issue now as it will be a breaking API change to solve it.

@nemith nemith added bug Something isn't working help wanted Extra attention is needed investigation Needs some time to invesigate an isssue labels Aug 29, 2023
@nemith nemith linked a pull request Aug 29, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed investigation Needs some time to invesigate an isssue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant