freeconf / yang

Standards-based management for Golang microservices
Apache License 2.0
38 stars 15 forks source link

Improve list-format handling, especially for union-list subtypes #86

Closed HRogge closed 10 months ago

HRogge commented 1 year ago

This PR cleans up the handling of the "list-variant" flag of the format type by replacing the magic number with an internal constant and adding another helper.

It also enforces that the union-types of a union-list are automatically converted to list-types.

dhubler commented 11 months ago
  1. magic number improvement looks good, would like a simple test on that. can that be in separate MR
  2. there is a rogue change for reflection in there. if intentional, that would need to be separated out into new MR with test as well
  3. newValue, it already knows result is a list, why would need bool to force anything. a test would help me see where there is a failing
HRogge commented 11 months ago

1/2 You are right, this needs to be splitup further... and I have to check where the change in "reflection" is coming from.

3) the problem happens when using the IETF "ip address" union in a leaf-list field... from the perspective of the yang-file, its a union-type with two pattern-matching string types... putting it into a leaf-list makes it a union-list with two string types (but not string lists), which means the wrong conversion is called when reading the field content from string representation. Thats why its (in my opinion) necessary to convert the subtypes of a union-list forcefully to list-types.

I will work on a couple of new PRs to split this apart, we can close this one when I created the new ones (or when the new ones are merged).

HRogge commented 11 months ago

Magic number improvement has been put into its own PR: #91 I will have to look into your own fix for the union-list problem (I think you solution is correct) and have to dive into the "PUT on root element" thing again.

HRogge commented 10 months ago

I tested the current master with my code and it still produce an error when writing the the UnionList... but I have a bit of trouble reproducing it in a limited testcase... hopefully have one this week.

HRogge commented 10 months ago

Okay, finally got it... this is a test for nodeutil/json_rdr_test.go, which fails in the current master... it does NOT fail if the "ip-prefix" type is inlined (instead of using the typedef).

My "fix" for it is definitely at the wrong place, I (now) expect some detail is missing in the YANG parser.

func TestJsonRdrTypedefUnionList(t *testing.T) {
    mstr := `
    module x {
        revision 0;
        typedef ip-prefix {
            type union {
                type ipv4-prefix;
            }
        }

        typedef ipv4-prefix {
            type string;
        }

        container y {
            list node {
                leaf-list ip {
                    type ip-prefix;
                }
            }
        }
    }
        `
    m, err := parser.LoadModuleFromString(nil, mstr)
    if err != nil {
        t.Fatal(err)
    }
    tests := []struct {
        in  string
        out string
    }{
        {
            in:  `{"y":{"node":[{"ip":["10.0.0.1","10.0.0.2"]}]}}`,
            out: `{"y":{"node":[{"ip":["10.0.0.1","10.0.0.2"]}]}}`,
        },
    }
    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)
    }
}
HRogge commented 10 months ago

To explain the problem I see with my code a little bit more... when I use the typdef (that contains a union) with a leaf-list, the format-value of the types inside the unionlist type are NOT list-types... which causes the issue the testcase detects.

HRogge commented 10 months ago

I managed to trim down the testcase even more:

func TestJsonRdrTypedefUnionList(t *testing.T) {
    mstr := `
    module x {
        revision 0;
        typedef ip-prefix {
            type union {
                type string;
            }
        }
        leaf-list ip {
            type ip-prefix;
        }
    }
        `
    m, err := parser.LoadModuleFromString(nil, mstr)
    if err != nil {
        t.Fatal(err)
    }
    tests := []struct {
        in  string
        out string
    }{
        {
            in:  `{"ip":["10.0.0.1","10.0.0.2"]}`,
            out: `{"ip":["10.0.0.1","10.0.0.2"]}`,
        },
    }
    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)
    }
}
HRogge commented 10 months ago

I think #93 is the right way to fix the problem with the unionlist subtypes.