CZ-NIC / yangson

GNU Lesser General Public License v3.0
53 stars 20 forks source link

Notification.from_raw() difficult to use. #78

Open kwatsen opened 3 years ago

kwatsen commented 3 years ago

Looking at the test_xml_notification pytest added to test_models.py in my xml-improved fork of yangson.

Notice that it works okay when from_raw() is passed this object:

{
    "testb:leafO" : True
}

But not this (more common) object:

{
    "testb:noA" : {
        "leafO" : True
    }
}

I understand that this is how Yangson generally works, but I wish that it were more like RESTCONF, whereby the object passed in and out of API calls has its top-level node being the node itself (not just it's contents).

The reason I care is because clients will always send notifications (i.e., via RFC 8040 SSE or the https-notif draft) with the top-level node existing, and all default-namespace settings are applied there. As it stands, the server-logic needs to float the default namespace down a level, without clobbering any already set default namespaces.

BTW, I tried toggling the allow_nodata boolean parameter, but it had no effect. To be honest, I'm still unclear what the allow_nodata is supposed to do. Maybe @HRogge can say? Is it, by chance, to allow exactly what I want (i.e., to toggle whether if the top-level node is included or not?)

FWIW, this issue also applies to InputNode and OutputNode, but they are both under RpcActionNode, whose from_raw() gives the expected behavior (i.e., a document with top-level node like {prefix}input. That said, it does also allow a document to have both an 'input' and 'output' simultaneously, a scenario that should never occur in normal operation.

Please respond quickly, as I'm hoping to get the xml-improved fork merged into master early next week. I'll submit a PR once I tidy up a few things, like adding a test_error pytest, etc.

HRogge commented 3 years ago

allow_no_data was a "hotfix" for the issue that Input/Output nodes of RPCs are not data-nodes... so "get_data_child()" would skip this nodes which have to be present when parsing a RPC request. When I first requested making Input/OutputNode derive from DataNode it was not accepted, so I got this "switch" merged.

it seems we forgot to remove the allow_nodata hotfix when we decided to derive Input/OutputNode from DataNode. I just tested it with my Restconf-Code, I don't need allow_nodata anymore for RPC access.

I have yet to look into NotificationNode and its codepath.

HRogge commented 3 years ago

To do what you want we would need the equivalent of the "is_root" parameter for from_raw(). this tells "from_xml()" that is should encode the current node, not its children... the rest is then solved by recursion.

kwatsen commented 3 years ago

I was looking to close this issue, but noticed that the test_xml_notification pytest in test_models.py was rather sparse and so spent some time trying to improve it...

As a recap, the goal of this test is to roundtrip (JSON --> XML --> JSON) a notification in order to exercise all the various bits.

Unfortunately, I wasn't able to do much of anything, as I continually ran into NonexistentSchemaNode: testb:noA under / and SchemaError: [/] member-not-allowed: testb:noA exceptions.

PS: the following sections should be removed now:

    ###################
    # JSON - RESTCONF # 
    ###################
    <snip/>

    ######################
    # JSON - HTTPS-NOTIF #
    ######################
    <snip/>