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

New issue

Closed

opened 2023-08-29 16:05:07 +00:00 by nemith · 0 comments

nemith commented

2023-08-29 16:05:07 +00:00

(Migrated from github.com)

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

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)
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

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.
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"
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.

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 a`Body []byte `xml:,innerxml` to capture the remaining nodes to allowed for deferred xml deoding. Even the `Reply` struct has a method called `Decode` used to decode the `Body` into a Go struct. This was very much modeled after jsons `json.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 ```xml <rpc-reply message-id="101""> <foo>foo</foo> <bar>42</bar> </rpc-reply> ``` ```go 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 <rpc-reply> 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.