Add RFC7951JSONConfig.PrependModuleNameNonRootTopLevel flag
#648
Conversation
wenovus
commented
Apr 18, 2022
wenovus
commented
``` // PrependModuleNameNonRootTopLevel specifies that the top-level field // names of a non-root ValidatedGoStruct being marshalled should always // have their belonging-module names prepended in the JSON output, // instead of only being present when its namespace differs from the // namespace of its parent. // NOTE: This flag is used to preserve [email protected] behaviour in // code that depends on it. This will be deprecated in the future. PrependModuleNameNonRootTopLevel bool ```
| // namespace of its parent. | ||
| // NOTE: This flag is used to preserve [email protected] behaviour in | ||
| // code that depends on it. This will be deprecated in the future. | ||
| PrependModuleNameNonRootTopLevel bool |
There was a problem hiding this comment.
Should this be SkipPrependModNameAtRoot or something - since I think by default we want to have the fixed behaviour (I think?). That being said, I'm OK with leaving it this way which is 'safer' but means that users need to do something to avoid the bug (which seems undesirable).
There was a problem hiding this comment.
Since the flag is by default false I'm interpreting the "default" to be not prepending, which is the standard behaviour. Sorry is the naming a bit confusing?
I agree that since the default ygot behaviour is nonstandard, I see this closer to a bug fix rather than an API improvement. So I also support putting the nonstandard behaviour behind the flag.
There was a problem hiding this comment.
Yes -- sorry, I misinterpreted the naming of the flag. PrependXXX with a default false makes sense to me. I'm OK with this change -- and just add to the release notes that we resolved this bug.
There was a problem hiding this comment.
So - sorry - we need to change the name here to SkipXXX rather than PrependXXX I believe.
There was a problem hiding this comment.
Ok, so if it's SkipXXX doesn't that mean by default we don't skip, and that means the module name is still there as before?
There was a problem hiding this comment.
Hah - we're tying ourselves in knots here.
Explicitly:
SkipPrependModuleNameNonRootTopLevel- default false - means don't skip prepending it, which means we do add it (i.e., default is fixed behaviour).PrependModuleNameNonRootTopLevel- default false - means don't prepend it, which means we leave the output unchanged.
The 2nd option is the "safe" one whereby users do not see a default change. We agreed (I think) that this is not preferable because this is a bug, not a feature being added.
The former option is one where users that want the buggy behaviour need to explicitly opt-in, which seems to be what we agree on. Hence my preference for the 'Skip' option.
That being said - let's make the flag really clear - and we can just say OmitModuleNameForTopLevelEntity. In this case, Omit... being false means that it is not omitted (i.e., it is added).
I suggest a comment like:
OmitModuleNameForTopLevelEntity, when set to true, allows direct children of a specific
container to not have their module name appended in JSON output. This behaviour is
incorrect according to RFC7951, but ygot releases prior to v0.17.0 implemented this
as their default behaviour. The default (false) behaviour is to be compliant with
RFC7951.
NB: this flag may be removed in future releases.
There was a problem hiding this comment.
Ok, so the fixed behaviour to me is when we DON'T prepend the module name. The output is changed. Currently, we prepend module names for non-root top-level names unconditionally, the fixed behaviour is to only prepend when the name resides in a different namespace than the parent.
So, I'm still thinking it should be PrependModuleNameNonRootTopLevel.
There was a problem hiding this comment.
Hmm...
So this made me go back and look at the specification again. In RFC7951 section 4:
A namespace-qualified member name MUST be used for all members of a
top-level JSON object and then also whenever the namespaces of the
data node and its parent node are different. In all other cases, the
simple form of the member name MUST be used.
So, in this case, my interpretation is that the top-level object does have to contain all the namespace qualification, because this says "do it at the top-level AND then only append the module if the node is in the same namespace as the parent".
This makes me think we should just revert the previous change. For some reason I was misremembering what the defect was originally.
What was the context for originally opening #637 -- since it seems the user in that bug here has the opposite problem.
There was a problem hiding this comment.
I think the crux of the issue is what is meant by all members of a top-level JSON object, and specifically what is a top-level JSON object.
We have two competing interpretations:
- All top-level members of any JSON object.
- All top-level members of only a JSON object at the top-level of the YANG hierarchy.
Now, if 1. were the correct interpretation, then I am confused by how top-level JSON object refers to any JSON object.
Now, if 2. were the correct interpretation, then "top-level" means the top-level of the YANG hierarchy, which while not crystal clear, is acceptable IMO; also, by "members", it means the top-level, or immediate members of a JSON object, which also while not crystal clear, is also acceptable IMO.
Thus, I think 2. makes more sense than 1, and so I'm thinking the fix was correct.
I agree that the user's bug comment was confusing, I did try to clarify to the user in my subsequent comment. The reason this bug was filed was actually from somebody else complaining about too much prepending, I've sent you the link to that offline.
There was a problem hiding this comment.
Thanks for the explanation here -- I actually think the original bug report is wrong :-), and that (1) in your example is correct.
I dug around and looked at rfc8040 (restconf) which has some examples of a GET being done at a non-root path in its appendix here.
In this case, the resource being requested is GET /restconf/data/ietf-yang-library:modules-state -- modules-state is a non-root-level container defined mode here: https://github.com/YangModels/yang/blob/main/vendor/cisco/xe/1681/ietf-yang-library.yang#L205 which does have its name prepended, some of the other examples (e.g., this POST example also uses the module name for a non-root level entity.
My view is that we should revert the change that was made to changes this functionality. Somehow I interpreted this incorrectly before - apologies.
|
@wenovus -- is this request dead at this point? I don't see that we concluded that we needed a change, and it looks like we didn't get asked again? |
I forgot the background for this PR. I actually think we did the wrong thing here, and agree with your last comment that #637 was a wrong bug report. I can see that prepending the module makes sense because it helps to distinguish between same-named nodes in different namespaces. I feel like this hasn't caused an issue because in OpenConfig we forbid the occurrence of same-named nodes in different namespaces. I'm ok with just closing this PR, or if there is a need, reverting the fix for #637 as you suggested and then adding a flag for the current behaviour. |
|
@robshakir to make sure you received the ping on the previous comment. |
