The data returned by get_config_dict should always have the same format to prevent any function using it to have to check if one element is a list or a string.

When written T2522, I came across this issue (but the other way round: when trying to render the data on screen from my own configuration class). When there is only one element in the list, it is better for the reader of the configuration of the class if the data is presented as one element.

So I believe showConfig is doing the right thing when generating a configuration for human consumption. It is however not doing what should when parsed to generate a structure to be used for automation.

As the data about which node is now available via the xml code within the repo, it would be quite trivial to iterate the data in get_config_dict and convert any string into a list of one element with that string.
This work would only be required once per call to showConfig and therefore not taxing performance-wise.

Unless someone object I propose to do just that as all the code currently should already work with a list (therefore this should not break anything) and will allow removing the "string" case down the line.

The value vs list is not produced by showConfig, but rather due to matching in configtree. We'll let @dmbaturin consider if this is an issue or not, but I imagine a jinja macro could address the problem where it occurs in template generation.

showConfig produces the following for multi-nodes:

test {
    something one
    something two
}

Yes, so there is no way to know at parsing time (unless you have two elements or check the XML) that this is a multi-element. Something "computer-friendly" would generate:

test {
    something [one]
}

test {
    something [one two]
}

which I am sure we all agree is not as good as what we have now.

But you make an excellent point, @thomas-mangin ; it may be best to normalize that within config_dict itself, now that we have the xml processing available ...

From John on github:

@thomas-mangin this is a nice solution, and since I know that @c-po has been waiting on it, I'm open to merging as is, but I think we should revisit directly to address two matters:
(1) c-po's comment
(2) have multi_to_list applied not to the cached dict, but only at output, as is done with key_mangling; this would mean that multi_to_list would only recurse once, namely, for the final subdict containing the value.

I understand your reasoning regarding (2), of course, as we've discussed the 'big picture' regarding caching recently: caching the root dict was a great suggestion of yours, however, there are two reasons to not recurse through the root_dict_cache: (a) the cost far outweighs the benefit when running as a standalone script outside of the daemon; as you know, we are preserving the orthogonality of running with or without the daemon (necessary in some cases; desired in all, so as to not foreclose on the next round of development); (b) the root_dict_cache is the 'raw' form, and representational tweaks should be the last step before output --- even under a daemon, it is rare that specific values are used twice.

As written, it is mildly expensive: 10ms for a path of depth 5.

Finally, I appreciate the clean up of config; that is needed, but in the future, these should be independent commits in the PR, especially as it is critical code. c-po reminds of best practices here, and I try to remind myself to practice it: even if one's code is bug-free, regressions are always possible, and it is easier to address with atomic commits.

Regarding (1) which is about changing the default. No issues, I was trying to break any code until someone had more time than me to test it, as the devil is always in the details.

Performance seems to be more important than I initially thought: I always assumed that the data would remain cached between call for the configuration of a single module (and therefore cache would be easy to add later on). The code does indeed perform quite a lot of tree traversing which is not the cheapest action so I looked at having all the xml is_ functions, including is_multi, using data generated at parse time (vs run time as it is now) and storing them like @c-po likes (joining the name with dots). It would allow checking for the answer with a single key lookup.

For this approach to be efficient the path need to not be analysed so it means that the benefit can only be achieved if key mangling and tagNode name are not in the path.

Currently, the mangling code is also generic, but only (minus to underscore is performed). Adding this genericity is another complication and slow down. A discussion on phabricator on how to best refactor this may be worth while.

However, For using the path without tagNodes. The current code does offer the option to ignore them if the option with_tag is set. If @c-po is happy to use the is_multi / and defaults having removed the tagNodes, then I can continue in that direction.

Could you share an example how to ise that? I then can test it in my code.

@thomas-mangin in further tests, I've seen wide variability in timing tests, independent of caching, with the original quote being the high-end. That will need to be investigated, but I think performance should not be considered a road-block for now.

Rather I suggest that you consider a path relative version of your current patch, and drop the interaction with dict_cache; I've ran basic tests on a version linked below --- all I've done is moved the call within get_config_dict. If something like this seems reasonable, and you want to set the default multi=True, I can help test and we can merge.

https://github.com/jestabro/vyos-1x/compare/orig-PR485...jestabro:local-PR485

Resolved in PR536.