Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RFC7951JSONConfig.PrependModuleNameNonRootTopLevel flag #648

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add RFC7951JSONConfig.PrependModuleNameNonRootTopLevel flag #648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions ygot/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -916,6 +916,14 @@ type RFC7951JSONConfig struct {
// elements that are defined within a different YANG module than their
// parent.
AppendModuleName bool
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So - sorry - we need to change the name here to SkipXXX rather than PrependXXX I believe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@robshakir robshakir Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. All top-level members of any JSON object.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// PrependModuleNameIdentityref determines whether the module name is
// prepended to identityref values. AppendModuleName (should be named
// PrependModuleName) subsumes and overrides this flag.
Expand Down Expand Up @@ -951,7 +959,11 @@ func (*RFC7951JSONConfig) IsMarshal7951Arg() {}
// to JSON described by RFC7951. The supplied args control options corresponding
// to the method by which JSON is marshalled.
func ConstructIETFJSON(s ValidatedGoStruct, args *RFC7951JSONConfig) (map[string]interface{}, error) {
return structJSON(s, s.ΛBelongingModule(), jsonOutputConfig{
var parentMod string
if args == nil || !args.PrependModuleNameNonRootTopLevel {
parentMod = s.ΛBelongingModule()
}
return structJSON(s, parentMod, jsonOutputConfig{
jType: RFC7951,
rfc7951Config: args,
})
Expand Down Expand Up @@ -1001,8 +1013,10 @@ func Marshal7951(d interface{}, args ...Marshal7951Arg) ([]byte, error) {
}
}
var parentMod string
if s, ok := d.(ValidatedGoStruct); ok {
parentMod = s.ΛBelongingModule()
if rfcCfg == nil || !rfcCfg.PrependModuleNameNonRootTopLevel {
if s, ok := d.(ValidatedGoStruct); ok {
parentMod = s.ΛBelongingModule()
}
}
j, err := jsonValue(reflect.ValueOf(d), parentMod, jsonOutputConfig{
jType: RFC7951,
Expand Down
116 changes: 101 additions & 15 deletions ygot/render_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1948,17 +1948,18 @@ func (t *unmarshalableJSON) UnmarshalJSON(d []byte) error {

func TestConstructJSON(t *testing.T) {
tests := []struct {
name string
in ValidatedGoStruct
inAppendMod bool
inPrependModIref bool
inRewriteModuleNameRules map[string]string
inPreferShadowPath bool
wantIETF map[string]interface{}
wantInternal map[string]interface{}
wantSame bool
wantErr bool
wantJSONErr bool
name string
in ValidatedGoStruct
inAppendMod bool
inPrependModIref bool
inPrependModNonRootTopLevel bool
inRewriteModuleNameRules map[string]string
inPreferShadowPath bool
wantIETF map[string]interface{}
wantInternal map[string]interface{}
wantSame bool
wantErr bool
wantJSONErr bool
}{{
name: "invalidGoStruct",
in: &invalidGoStructChild{
Expand Down Expand Up @@ -2798,6 +2799,50 @@ func TestConstructJSON(t *testing.T) {
"f5": "hat",
},
},
}, {
name: "module append example with prepending top-level module names",
in: &ietfRenderExample{
F1: String("foo"),
F2: String("bar"),
F3: &ietfRenderExampleChild{
F4: String("baz"),
F5: String("hat"),
},
F6: String("mat"),
F7: String("bat"),
},
inAppendMod: true,
inPrependModNonRootTopLevel: true,
wantIETF: map[string]interface{}{
"f1mod:f1": "foo",
"f1mod:config": map[string]interface{}{
"f2mod:f6": "mat",
},
"f2mod:config": map[string]interface{}{
"f2": "bar",
"f3mod:f7": "bat",
},
"f1mod:f3": map[string]interface{}{
"f42mod:config": map[string]interface{}{
"f4": "baz",
},
"f5": "hat",
},
},
wantInternal: map[string]interface{}{
"f1": "foo",
"config": map[string]interface{}{
"f2": "bar",
"f6": "mat",
"f7": "bat",
},
"f3": map[string]interface{}{
"config": map[string]interface{}{
"f4": "baz",
},
"f5": "hat",
},
},
}, {
name: "list at root",
in: &listAtRoot{
Expand Down Expand Up @@ -2967,10 +3012,11 @@ func TestConstructJSON(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name+" ConstructIETFJSON", func(t *testing.T) {
gotietf, err := ConstructIETFJSON(tt.in, &RFC7951JSONConfig{
AppendModuleName: tt.inAppendMod,
PrependModuleNameIdentityref: tt.inPrependModIref,
RewriteModuleNames: tt.inRewriteModuleNameRules,
PreferShadowPath: tt.inPreferShadowPath,
AppendModuleName: tt.inAppendMod,
PrependModuleNameNonRootTopLevel: tt.inPrependModNonRootTopLevel,
PrependModuleNameIdentityref: tt.inPrependModIref,
RewriteModuleNames: tt.inRewriteModuleNameRules,
PreferShadowPath: tt.inPreferShadowPath,
})
if (err != nil) != tt.wantErr {
t.Fatalf("ConstructIETFJSON(%v): got unexpected error: %v, want error %v", tt.in, err, tt.wantErr)
Expand Down Expand Up @@ -3859,6 +3905,15 @@ func TestMarshal7951(t *testing.T) {
&RFC7951JSONConfig{AppendModuleName: true},
},
want: `{"f1":"hello"}`,
}, {
desc: "append module names with non-root top-level requested",
in: &ietfRenderExample{
F1: String("hello"),
},
inArgs: []Marshal7951Arg{
&RFC7951JSONConfig{AppendModuleName: true, PrependModuleNameNonRootTopLevel: true},
},
want: `{"f1mod:f1":"hello"}`,
}, {
desc: "complex children with module name prepend request",
in: &ietfRenderExample{
Expand Down Expand Up @@ -3889,6 +3944,37 @@ func TestMarshal7951(t *testing.T) {
"test",
42
]
}`,
}, {
desc: "complex children with module name prepend request with non-root top-level module prepend request",
in: &ietfRenderExample{
F2: String("bar"),
MixedList: []interface{}{EnumTestVALONE, "test", 42},
EnumList: map[EnumTest]*ietfRenderExampleEnumList{
EnumTestVALONE: {Key: EnumTestVALONE},
},
},
inArgs: []Marshal7951Arg{
&RFC7951JSONConfig{AppendModuleName: true, PrependModuleNameNonRootTopLevel: true},
JSONIndent(" "),
},
want: `{
"f1mod:enum-list": [
{
"config": {
"key": "foo:VAL_ONE"
},
"key": "foo:VAL_ONE"
}
],
"f1mod:mixed-list": [
"foo:VAL_ONE",
"test",
42
],
"f2mod:config": {
"f2": "bar"
}
}`,
}, {
desc: "complex children with PrependModuleNameIdentityref=true",
Expand Down