Let validate() treat RpcActionNode 'input' like 'config' and 'output' like 'nonconfig'

[kwatsen]

I have a routine that attempts to convert any raw text string received from a client to a Python object:

def encoded_str_to_obj(
    estr: str,
    enc: Encoding,
    dm: yangson.datamodel.DataModel,
    sn: yangson.schemanode.SchemaNode,
) -> dict:
    """
    Deserialize string to object...and validate it!

    Parameters:
      estr: the encoded string to deserialized
      enc:  the encoding to use (XML or JSON)
      sn:   the Yangson DataModel object to use
 
    Returns:
      a Python obj for the deserialized string

    <snip/>

It mostly works, but fails on RpcActionNode when the YANG defines an output statement containing mandatory true leaves, because the client-input only contains the input child. As an example, see the conveyed-information leaf here.

If, for the purpose of validation, the input could be treated like "config" and the output like "nonconfig", then a content_type parameter could be added to this routine to enable it to not throw an exception because the output statement is missing...

[llhotka]

Perhaps you can validate it against the InputNode?

[llhotka]

@kwatsen, on a more general note: I did some tests and I think that working with RPC payloads (and possibly notifications) is awkward in Yangson. It make little sense to me to validate RPCs together with config and state data, but actually also input and output data of the same RPC together. So what seems to be necessary is to treat the data model not as a single schema but as multiple schemas like this:

• main schema for config and state data
• two schemas for each RPC (one for input, another for output)
• one schema for each notification

So the DataModel class could have not just one schema member, as it is now, but (i) main_schema and (ii) a dictionary of RPC and notification schemas with RPC/notification names as dictionary keys. For every piece of instance data, one would have to specify the schema to use (main_schema being the default). I think it shouldn't be too difficult to implement, and it could probably be made also almost perfectly backwards compatible.

I have two questions for you:

1. What do you think about this arrangement?
2. What is the best form/encapsulation of RPC payload data for submitting them to Yangson?

[kwatsen]

Hi Lada, thank you for looking into this more.

To answer your first question, I looked into using InputNode, but the code would be rather hacky (a bunch of special cases). Case in point, this is my 2nd or 3rd time looking into this issue and each time my "solution" is to instead disable validation 🫣

FWIW, below is a snippet of code that calls the aforementioned encoded_str_to_obj() function:

# get HTTP request body
request_body_text = await request.text()

# ascertain if `XML` or `JSON` from Content-Type
request_content_encoding = utils.Encoding[request.headers["Content-Type"].rsplit("+", 1)[1].upper()]

# get shemanode for this RPC
sn = self.dm.get_schema_node("/ietf-sztp-bootstrap-server:get-bootstrapping-data")

# convert body for a Python object
request_body = utils.encoded_str_to_obj(request_body_text, request_content_encoding, self.dm, sn)

[kwatsen]

Regarding the questions in your second response, here are my thoughts:

1. Would the arrangement support must or when expressions? Note the accessible tree from RFC 7950, Section 6.4.1:

   o  If the XPath expression is defined in a substatement to a
      "notification" statement, the accessible tree is the notification
      instance, all state data in the server, and the running
      configuration datastore.  If the notification is defined on the
      top level in a module, then the root node has the node
      representing the notification being defined and all top-level data
      nodes in all modules as children.  Otherwise, the root node has
      all top-level data nodes in all modules as children.

   o  If the XPath expression is defined in a substatement to an "input"
      statement in an "rpc" or "action" statement, the accessible tree
      is the RPC or action operation instance, all state data in the
      server, and the running configuration datastore.  The root node
      has top-level data nodes in all modules as children.
      Additionally, for an RPC, the root node also has the node
      representing the RPC operation being defined as a child.  The node
      representing the operation being defined has the operation's input
      parameters as children.

   o  If the XPath expression is defined in a substatement to an
      "output" statement in an "rpc" or "action" statement, the
      accessible tree is the RPC or action operation instance, all state
      data in the server, and the running configuration datastore.  The
      root node has top-level data nodes in all modules as children.
      Additionally, for an RPC, the root node also has the node
      representing the RPC operation being defined as a child.  The node
      representing the operation being defined has the operation's
      output parameters as children.

2. I'm unsure what you mean, some examples might help. That said, this seems related to NETCONF-based RPC/action replies cannot be parsed #140, where I was suggesting normalization between NC/RC.

[llhotka]

Ad 1. XPath context is actually more of an issue for instance data – if you want to validate RPC input/output payload containing XPath references to config or state data, then you also have to supply the main data tree somehow. It is OK e.g. for a RESTCONF server but not so much for offline validation, which is probably what you are doing now. I am not sure what typical use cases for RPC payload validation might be.

Ad 2. Several encapsulation formats are possible, starting from

{
    "modulename:rpcname": {
        "input": {
            ...
        }
        "output": {
            ...
        }
    }
}

and a few other options:

• either input or output inside module name:rpcname but not both (I am not sure if it makes sense to validate both at the same time)
• either input or output but without the encapsulating modulename:rpcname. Here we have to specify the RPC operation separately.
• just the contents of input or output. We have to separately specify the RPC operation, and also whether it is input or output. This could be a problem for XML, some encapsulating element would be needed anyway.

Which one do you prefer?

[kwatsen]

Ad 1. Actually, I am writing a RESTCONF server. That said, Yangson should be mindful of the needs of clients too. I understand the need for instance data, but my comment was more regarding Yangson being able to validate the must and when expressions (if it does at all), as it seems that would require access the config/state schema tree as well?

Ad 2. Thank you for presenting the three options. My favorite is option 2.

PROs:

• implemented by pytests test_rpc, test_xml_rpc and test_xml_action, which bodes well for backwards compatibility.
• seamlessly handles XML with more than one top-level element.
• 1-1 with RESTCONF payloads, which I think are better than NETCONF payloads. I hope this is fixed via this NC-next issue.

CONs:

• one could argue that option (1) is better because it is more consistent with how Yangson supports notifications (i.e., having an envelop identifying the notification)
• one could argue that option (2) is inconsistent with how Yangson does not expect an envelop for config/state data trees (i.e., there's no envelop called, e.g., "data-tree"). Disclaimer, for XML, the pytests show an envelop called "content-data" using the xmlns ietf-yang-instance-data.
• one could argue that option (3) is better because it hides even more of the difference between NC and RC.

[llhotka]

I also like option (2) best. I will now do some more testing and will try to come up with improvements that could make RPC validation easier.

BTW, I found out that one of the tests for RPC (test_c.py, added by @HRogge) is based on an invalid YANG module yang-modules/testc/[email protected] that contains an RPC definition inside a container, which is not permitted:

container rpcs {
  rpc rpcB {
  }
}

I actually don't know what this test is good for, so I plan to remove it.