CZ-NIC / yangson

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

Support dictionary in JSON for yang list statement #117

Open stephan-hof opened 2 years ago

stephan-hof commented 2 years ago

Dear maintainers of the yangson library, I would like to hear your opinions about the following feature.

Assume the following definition in a yang file.

container parent {
  list children {
    key name;
    leaf name {
      type string;
    }
    leaf age {
      type uint32;
    }
  }
}   

Currently yangson demands that the corresponding JSON looks like this:

{
    'parent': {
        'children': [
           {'name': 'joe', age: 1}
           {'name': 'jane', age: 3}
        ]
    }
}

What do you think to additionally support also a JSON like this:

{
    'parent': {
        'children': {
            'joe': {'age': 1},
            'jane': {'age': 3}
        }
    }
}

We are mostly using yang for human readable config files (as yaml files). So we prefer our users to write this in their yaml file.

parent:
    children:
       joe:
         age:1
      jane:
         age: 3

Using a dictionary for 'list statement' seems in sync with the yang specification. Since yang demand that each item of a list has a unique key (specified via the key keyword). So if you think this would make sense I'll try to come up with a pull request.

llhotka commented 2 years ago

This is incompatible with JSON data representation as defined in RFC 7951 and cannot be used in RESTCONF either. It would be possible to write a special export method to produce such a JSON, but switching completely to this representation isn't possible.

stephan-hof commented 2 years ago

Hi, thanks for the quick reply.

but switching completely to this representation isn't possible.

This makes a lot of sense and it was never an intention of me to switch the format entirely. Let me try to make my use case clearer: We have configuration files for our software. They are not related to NETCONF, RESTCONF more like a configuration file for traefik. Those files are written/changed by humans, so a slightly more compact syntax is desirable. In this software I'm using yangson (roughly) like this:

dm = yangson.DataModel(...)

with open(location) as fd:
   daemon_configuration = yaml.safe_load(fd)
   inst = dm.from_raw(daemon_configuration)

My proposal would that from_raw (or more general at instance construction) both styles are allowed:

After a brief look at the code a change to yangson.schemanode.ListNode.from_raw could be done to support two styles of the incoming raw_value.

llhotka commented 2 years ago

I don't want to complicate the from_raw method, it is supposed to read parsed RFC 7951 format. Note that it is not as easy as it seems:

stephan-hof commented 2 years ago

The change I had in mind was on ListNode.form_raw itself. As far as I can see this gets only called for yang list statements. Up to my knowledge yang list statements mandate that each item in the list must have a unique key. So I must admit I'm not sure what you mean with non-config lists needn't have any keys.

Regarding the order. Since python3.7 python dictionaries preserve order. Furthermore in older versions there is collections.OrderedDict and yaml/json libraries support using that instead for parsing. For us order preserving is pretty important as well, so we use these described mechanics already in other places and it works as expected.

The code change is not so hard as it seems. From the top of my head it would be a change like this:

class ListNode(SequenceNode, InternalNode):
   .....  existing code for that class ...

    def from_raw(self, rval, jptr):
        return super(self).from_raw(self._auto_convert_dict(rval), jptr)

    def _auto_convert_dict(self, rval):
        if not isinstance(rval, dict):
            return rval 

        as_list = [] 
        for key, value in rval.items():
            key = key if isinstance(key, (tuple, list)) else [key]
            for (idx, (field_name, module_name)) in enumerate(self.keys):
                value[field_name] = key[idx]
            as_list.append(value)

        return as_list

Having said all this. I would understand if you don't agree with adding this to yangson. Since it adds some 'special' behaviour to the incoming JSON which is not covered by the RFC. Just wanted to check if the maintainers see it as a possibility before I start thinking about workarounds like:

llhotka commented 2 years ago

The change I had in mind was on ListNode.form_raw itself. As far as I can see this gets only called for yang list statements. Up to my knowledge yang list statements mandate that each item in the list must have a unique key. So I must admit I'm not sure what you mean with non-config lists needn't have any keys.

Sec. 7.8.2 in RFC 7950: The "key" statement, which MUST be present if the list represents configuration and MAY be present otherwise, ....

Regarding the order. Since python3.7 python dictionaries preserve order. Furthermore in older versions there is collections.OrderedDict and yaml/json libraries support using that instead for parsing. For us order preserving is pretty important as well, so we use these described mechanics already in other places and it works as expected.

Insertion order, yes. There is no way though to e.g. insert a new entry between two existing entries.