Page MenuHomeVyOS Platform

get_config_dict includes node name as key only for tag and leaf nodes
Closed, ResolvedPublic

Description

The dict returned by get_config_dict will have a different form depending on the node type: for a non-tag or non-leaf node the configuration under the node will be returned; for a tag node, the configuration including the tag node (xml) name will be returned. This is not necessarily a bug, and makes a certain sense --- the question is whether we want to normalize this behaviour, in line with @thomas-mangin 's suggestion in T2636 to normalize the form of values of multi-valued nodes. We should consider this now, as get_config_dict is moving to general use, hence priority is listed as 'high'. A quick example for clarification:

c.get_config_dict('test some-tag-node')
{'some-tag-node': {'some0': {'just-a-node': {'multiple-value': ['one', 'two'], 'single-value': 'val'}}}}

c.get_config_dict('test some-tag-node some0 just-a-node')
{'multiple-value': ['one', 'two'], 'single-value': 'val'}

This for the configuration:

test {
     some-tag-node some0 {
         just-a-node {
             multiple-value one
             multiple-value two
             single-value val
         }
     }
 }

Details

Difficulty level
Unknown (require assessment)
Version
vyos-1.3
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)

Event Timeline

jestabro triaged this task as High priority.Mon, Jun 29, 9:18 PM
jestabro created this task.
jestabro created this object in space S1 VyOS Public.
jestabro updated the task description. (Show Details)Mon, Jun 29, 9:24 PM
jestabro updated the task description. (Show Details)
jestabro added a comment.EditedTue, Jun 30, 1:44 AM

One compelling idea is to move the implementation of the path argument in get_config_dict out of show_config, and replace it with a function to get a sub dictionary from the full get_config_dict. An example implementation is below. This would not only normalize the result, independent of idiosyncrasies of the current backend, but also future proof the form for the switch to vyconf. Secondly, it would allow us to remove the dependency on show_config itself, since that call is already made at Config initialization. Thirdly, it would allow direct use for off-line config instances, namely those for testing and offload to daemon.

We would have to survey current uses to confirm consistency.

Example implementation; needs testing:

def get_sub_dict(d, path):
    if isinstance(path, str):
        path = re.split(r'\s+', path)
    elif isinstance(path, list):  
        pass
    else:
        raise TypeError("Path must be a whitespace-separated string or a list")
    if not path or not isinstance(d, dict):
        return None
    k = path[0]
    if k not in d.keys():
        return None
    c = {k: d[k]}
    path = path[1:]
    if not path:
        return c
    elif not isinstance(c[k], dict):
        return None
    return get_sub_dict(c[k], path)

This gives the same form for an arbitrary node. Example within a config session to compare (current behaviour in task description):

>>> from vyos.config import Config
>>> c = Config()
>>> d = c.get_config_dict()
>>> get_sub_dict(d, 'test some-tag-node')
{'some-tag-node': {'some0': {'just-a-node': {'multiple-value': ['one', 'two'], 'single-value': 'val'}}}}
>>> get_sub_dict(d, 'test some-tag-node some0 just-a-node')
{'just-a-node': {'multiple-value': ['one', 'two'], 'single-value': 'val'}}
jestabro updated the task description. (Show Details)Tue, Jun 30, 2:04 AM
thomas-mangin added a comment.EditedTue, Jun 30, 7:57 AM

Thank you for this explaining what is happening. I would indeed rather tag were used as keys.

Also if asking for a tag, I believe it is more intuitive to return the content of the tag and not the data indexed by that tag:
https://github.com/vyos/vyos-1x/pull/484/files#diff-58fc6df0879046cf057488b9a641ca6eR261

so the behaviour I would suggest would be (using John's example):

c.get_config_dict(['test', 'some-tag-node'])
{'some0': {'just-a-node': {'multiple-value': ['one', 'two'], 'single-value': 'val'}}}

Also in my code when I expect path to be a list, I call it lpath. I use path to indicate that the code could be a string or a

We should change over time all the code which uses strings for path to lists to not have type instrospection and that get_conf_dict should only accept list, all the current callers do.

Just as a comment, I would not return None but {} for missing nodes, as otherwise you can not use multiple dict.get(key,{}) to traverse the full structure and check that the last element exists.

pasik added a subscriber: pasik.Tue, Jun 30, 8:14 AM
jestabro added a comment.EditedTue, Jun 30, 3:44 PM

The reason to return the data indexed by a node name: leaf nodes; the data is not a dict. If one wants consistency, then data of a node is returned indexed. This function does one specific thing: return a sub-dictionary from a dictionary, so

get_sub_dict(d, path) -> Dict[str, Any]

This is also consistent with the behaviour of show_config(path, ...) on leaf and tag nodes, which is a nice benefit, as we can consider it as a drop-in replacement.

Along the same lines: this function has one job: get a sub dict of an existing dict --- it knows nothing of the internal logic of the xml, configs, etc. Consequently, it returns None for nonsensical input or paths non-existent in the provided dict. We will have functions that are aware of the full xml definitions; this is not that function.

Thanks as always for the input, @thomas-mangin I will open a new task with a focused title, to discuss get_sub_dict and testing/issues with current use.

Okay, I will revise that: ... Consequently it returns None for nonsensical input and {} for non-existent paths ...

That make sense; thanks @thomas-mangin !

jestabro renamed this task from get_config_dict includes node as key only for tag and leaf nodes to get_config_dict includes node name as key only for tag and leaf nodes.Wed, Jul 1, 5:12 PM