UnionLists overwrite list/leaf type of subtypes

[HRogge]

It seems my fix for unionlists of subtypes not becoming lists had a sideeffect overwriting the subtype itself ( see #60 ).

I am not completely sure how we should to fix this, but at least I have a testcase that demonstrates the problem.

diff --git a/nodeutil/json_rdr_test.go b/nodeutil/json_rdr_test.go
index 2b638a6..3d1def8 100644
--- a/nodeutil/json_rdr_test.go
+++ b/nodeutil/json_rdr_test.go
@@ -124,6 +124,46 @@ func TestJsonRdrTypedefUnionList(t *testing.T) {
        }
 }

+func TestJsonRdrTypedefMixedUnion(t *testing.T) {
+       mstr := `
+    module x {
+        revision 0;
+        typedef ip-prefix {
+            type union {
+                type string;
+            }
+        }
+        leaf-list ips {
+            type ip-prefix;
+        }
+        leaf ip {
+                type ip-prefix;
+        }
+}
+        `
+       m, err := parser.LoadModuleFromString(nil, mstr)
+       if err != nil {
+               t.Fatal(err)
+       }
+       tests := []struct {
+               in  string
+               out string
+       }{
+               {
+                       in:  `{"ips":["10.0.0.1","10.0.0.2"],"ip":"10.0.0.3"}`,
+                       out: `{"ips":["10.0.0.1","10.0.0.2"],"ip":"10.0.0.3"}`,
+               },
+       }
+       for _, json := range tests {
+               t.Log(json.in)
+               actual, err := WriteJSON(node.NewBrowser(m, ReadJSON(json.in)).Root())
+               if err != nil {
+                       t.Error(err)
+               }
+               fc.AssertEqual(t, json.out, actual)
+       }
+}
+
 func TestNumberParse(t *testing.T) {
        moduleStr := `
 module json-test {

[dhubler]

nuggets. if i get a chance, i'll take a look

On Fri, Dec 1, 2023 at 4:08 AM Henning Rogge @.***> wrote:

It seems my fix for unionlists of subtypes not becoming lists had a sideeffect overwriting the subtype itself ( see #60 https://github.com/freeconf/yang/issues/60 ).

I am not completely sure how we should to fix this, but at least I have a testcase that demonstrates the problem.

diff --git a/nodeutil/json_rdr_test.go b/nodeutil/json_rdr_test.go index 2b638a6..3d1def8 100644 --- a/nodeutil/json_rdr_test.go +++ b/nodeutil/json_rdr_test.go @@ -124,6 +124,46 @@ func TestJsonRdrTypedefUnionList(t *testing.T) { } }

+func TestJsonRdrTypedefMixedUnion(t *testing.T) {

• mstr := `
• module x {
• revision 0;
• typedef ip-prefix {
• type union {
• type string;
• }
• }
• leaf-list ips {
• type ip-prefix;
• }
• leaf ip {
• type ip-prefix;
• } +}
• `
• m, err := parser.LoadModuleFromString(nil, mstr)
• if err != nil {
• t.Fatal(err)
• }
• tests := []struct {
• in string
• out string
• }{
• {
• in: {"ips":["10.0.0.1","10.0.0.2"],"ip":"10.0.0.3"},
• out: {"ips":["10.0.0.1","10.0.0.2"],"ip":"10.0.0.3"},
• },
• }
• for _, json := range tests {
• t.Log(json.in)
• actual, err := WriteJSON(node.NewBrowser(m, ReadJSON(json.in)).Root())
• if err != nil {
• t.Error(err)
• }
• fc.AssertEqual(t, json.out, actual)
• } +}

• func TestNumberParse(t *testing.T) { moduleStr := ` module json-test {

— Reply to this email directly, view it on GitHub https://github.com/freeconf/yang/issues/96, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACA7QDGZG7ZHHDW7J6TNLYHGM73AVCNFSM6AAAAABACQK6IOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGAZDANBTGQYDSNI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[HRogge]

I looked a bit more in this issue and I think the "fix" for the UnionList in meta/compile.go compileType() line 241 is responsible.

It solves he case that a pre-defined type is used in a UnionList by changing the type to a list, but this doesn't work if the type is also used as a non-list type.

Maybe it would be better to check if the subtypes of a UnionList are lists in 311 (same file) and if not duplicate the type (instead of modifying the original type)...

[HRogge]

I tried to duplicate the type (by just copying the object, changing the format to list/no-list) and then set it to the unionTypes slice of the parent, but it doesn't seem to work...

diff --git a/meta/compile.go b/meta/compile.go
index 2a7e6ef..18ca39b 100644
--- a/meta/compile.go
+++ b/meta/compile.go
@@ -239,9 +239,6 @@ func (c *compiler) compileType(y *Type, parent Leafable, isUnion bool) error {
                return errors.New("no type set on " + SchemaPath(parent))
        }
        if int(y.format) != 0 {
-               if _, isList := parent.(*LeafList); isList && !y.format.IsList() {
-                       y.format = y.format.List()
-               }
                return nil
        }
        var builtinType bool
@@ -307,10 +304,19 @@ func (c *compiler) compileType(y *Type, parent Leafable, isUnion bool) error {
                if len(y.unionTypes) == 0 {
                        return errors.New(SchemaPath(parent) + " - unions need at least one type")
                }
-               for _, u := range y.unionTypes {
+               for i, u := range y.unionTypes {
                        if err := c.compileType(u, parent, true); err != nil {
                                return err
                        }
+                       if !u.format.IsList() && y.format == val.FmtUnionList {
+                               cpy := *u
+                               cpy.format = cpy.format.List()
+                               y.unionTypes[i] = &cpy
+                       } else if u.format.IsList() && y.format == val.FmtUnion {
+                               cpy := *u
+                               cpy.format = cpy.format.Single()
+                               y.unionTypes[i] = &cpy
+                       }
                }
        } else if len(y.unionTypes) > 0 {
                return errors.New(SchemaPath(parent) + " - embedded types are only for union types")

[HRogge]

I dug a little bit deeper into the issue...

I think the problem arises from the fact that the meta.compileType() function is allowed to convert a type from "single" to "list" variant if the parent is a leaf-list (meta/compile.go line 242-244 and 303-305). This is okay for built-in types and inline subtypes, but it can lead to issues for named type definitions of unions that are also used elsewhere. Because these have pointers to their union-types, which means we can have multiple types which have pointers to the same unionTypes.

I have a patch that solves the testcase above, but it breaks the parser_samples_test.go because it copies all unionTypes in the meta/core.go function mixin().

I will send a Draft-PR so you can have a look at the code and tell me what you think about it. Maybe I will find a way to fix the break later.

[HRogge]

created PR #97

[dhubler]

yeah, this seems serious. i have your branch and will look at it later today.

On Tue, Dec 5, 2023 at 3:17 AM Henning Rogge @.***> wrote:

created PR #97 https://github.com/freeconf/yang/pull/97

— Reply to this email directly, view it on GitHub https://github.com/freeconf/yang/issues/96#issuecomment-1840232319, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACA7R5ARB4WVZFRYTCB2LYH3KBTAVCNFSM6AAAAABACQK6IOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBQGIZTEMZRHE . You are receiving this because you commented.Message ID: @.***>