Broken functionality of LYD_PARSE_STORE_ONLY flag on union type

[steweg]

Hi,

I when I accidentally pull latest version of devel/master, I noticed that some of the features that worked for me before stopped working. I have pin point it to commit b89b5ae. I am kind of surprised to see that even saying that it is fixing some issue in different project. Unfortunately there is no unit test which would provide some level of details in how this potential issue can happen or what it causes to that other project

Anyway on my side, now I have a problem because when I use arbitrary value which shall work or was working before, I am getting errors that my value is not valid due to pattern, length etc... which was exactly what I wanted to avoid by using this special flag. I fully agree that in case of union type that selected subtype might be wrong, but this is kind of expected as the value that was given with STORE_ONLY is just kind of arbitrary or temporary one. So it is also expected to be replaced with completely valid once available, or not being used at all in the final set.

I am not sure for what reason someone else is using this FLAG, but current state is definitely not ok as union types are now exception in behavior when compared with other built-in types.

So can you please share some light to root cause, so we can find some reasonable alternative that will work for all parties involved? Thanks

[michalvasko]

Sorry about that, the use-case is described in the referenced issue but I understand it may not be easy to understand. So, what happens is that validated data are stored in a JSON file and then tried to be efficiently loaded using all the flags including LYD_PARSE_STORE_ONLY. That worked fine for all the types except for the union because it then simply used the first suitable type instead of using the validated type.

The use-case is quite different from yours but I am sure both can be supported. The first solution that comes to mind is adding another flag that would affect only how the union values are stored. What do you think, do you have a better idea?

[steweg]

I was thinking about adjusting the logic in a way that it will try to perform validation in first round and if all types will fail as a last resort with LYD_PARSE_STORE_ONLY option it will fallback to store it to any suitable type. If that would fails again, than return error to the user.

The second idea was that it will store it temporarily under any suitable type and during the validation it will change it to the correct one. But this one depends on whether in your other usecase you are performing any validation at all after parsing or not. I guess that you are not doing that.

[michalvasko]

Okay, the first solution is fine with me so please test it and then I will commit it.

union.patch

[steweg]

I have created PR #2429 using your code and my unit tests

[steweg]

If possible, can you please backport this into version 3? libyang-python project is still using version 3 and it doesn't seem to be easy migration to version 4, so it would be beneficial to have also version 3 with this bugfix for some time. Thanks

[michalvasko]

Thanks for the tests (but seems there is a bug in them currently) but we generally do not backport anything. All the changes are made in the devel branch that is merged into the master on a release. Even if this change gets into master, it will be moved to SO4 on the next release.

[steweg]

Yeah, I have fixed the test itself, but there seems to be errors which are not related to our commit (memory leaks in various tests etc...).