From b7de07ae6981864e87954e28b69552de959a79f5 Mon Sep 17 00:00:00 2001 From: Wen Bo Li <50884368+wenovus@users.noreply.github.com> Date: Mon, 24 Jul 2023 14:36:27 -0700 Subject: [PATCH] Change internal JSON Generation for OrderedMaps to be backwards-compatible (#894) * Fix BuildEmptyTree for ordered maps * Fix internal JSON --- ygot/render.go | 157 ++++++++++-------- ygot/render_exported_test.go | 36 ++-- ...son_orderedmap_container_internal.json-txt | 22 +-- 3 files changed, 120 insertions(+), 95 deletions(-) diff --git a/ygot/render.go b/ygot/render.go index 5cb47ff4..d7f1bbbf 100644 --- a/ygot/render.go +++ b/ygot/render.go @@ -19,13 +19,13 @@ import ( "errors" "fmt" "reflect" - "sort" "strings" "github.com/openconfig/gnmi/errlist" "github.com/openconfig/gnmi/value" "github.com/openconfig/ygot/internal/yreflect" "github.com/openconfig/ygot/util" + "golang.org/x/exp/slices" "google.golang.org/protobuf/encoding/prototext" "google.golang.org/protobuf/proto" @@ -1442,67 +1442,59 @@ func keyValue(v reflect.Value, prependModuleNameIref bool) (any, error) { return name, nil } -// mapJSON takes an input reflect.Value containing a map, and -// constructs the representation for JSON marshalling that corresponds to it. -// The module within which the map is defined is specified by the parentMod -// argument. -func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (any, error) { - var errs errlist.List - mapKeyMap := map[string]reflect.Value{} - // Order of elements determines the order in which keys will be processed. - var mapKeys []string +// mapKeyToJSONString converts a map key to a string to be used in JSON output. +func mapKeyToJSONString(k reflect.Value, args jsonOutputConfig) (string, error) { switch args.jType { case RFC7951: - // YANG lists are marshalled into a JSON object array for IETF - // JSON. We handle the keys in alphabetical order to ensure that - // deterministic ordering is achieved in the output JSON. - for _, k := range field.MapKeys() { - keyval, err := keyValue(k, false) - if err != nil { - errs.Add(fmt.Errorf("invalid enumerated key: %v", err)) - continue - } - kn := fmt.Sprintf("%v", keyval) - mapKeys = append(mapKeys, kn) - mapKeyMap[kn] = k + k, err := keyValue(k, false) + if err != nil { + return "", fmt.Errorf("invalid enumerated key: %v", err) } + return fmt.Sprintf("%v", k), nil case Internal: // In non-IETF JSON, then we output a list as a JSON object. The keys // are stored as strings. - for _, k := range field.MapKeys() { - var kn string - switch k.Kind() { - case reflect.Struct: - // Handle the case of a multikey list. - var kp []string - for j := 0; j < k.NumField(); j++ { - keyval, err := keyValue(k.Field(j), false) - if err != nil { - errs.Add(fmt.Errorf("invalid enumerated key: %v", err)) - continue - } - kp = append(kp, fmt.Sprintf("%v", keyval)) - } - kn = strings.Join(kp, " ") - case reflect.Int64: - keyval, err := keyValue(k, false) + switch k.Kind() { + case reflect.Struct: + var errs errlist.List + // Handle the case of a multikey list. + var kp []string + for j := 0; j < k.NumField(); j++ { + keyval, err := keyValue(k.Field(j), false) if err != nil { errs.Add(fmt.Errorf("invalid enumerated key: %v", err)) continue } - kn = fmt.Sprintf("%v", keyval) - default: - kn = fmt.Sprintf("%v", k.Interface()) + kp = append(kp, fmt.Sprintf("%v", keyval)) + } + if errs.Err() != nil { + return "", errs.Err() + } + return strings.Join(kp, " "), nil + case reflect.Int64: + keyval, err := keyValue(k, false) + if err != nil { + return "", fmt.Errorf("invalid enumerated key: %v", err) } - mapKeys = append(mapKeys, kn) - mapKeyMap[kn] = k + return fmt.Sprintf("%v", keyval), nil + default: + return fmt.Sprintf("%v", k.Interface()), nil } default: - return nil, fmt.Errorf("unknown JSON type: %v", args.jType) + return "", fmt.Errorf("unknown JSON type: %v", args.jType) } - sort.Strings(mapKeys) +} + +// mapValuePair represents a map value pair with the key as a string. +type mapValuePair struct { + k string + v reflect.Value +} - if len(mapKeys) == 0 { +// mapValuePairsToJSON converts the given ordered pair of map values to JSON +// according to the JSON format option. +func mapValuePairsToJSON(pairs []mapValuePair, parentMod string, args jsonOutputConfig) (any, error) { + if len(pairs) == 0 { // empty list should be encoded as empty list if args.jType == RFC7951 { return []any{}, nil @@ -1510,6 +1502,8 @@ func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (any, return nil, nil } + var errs errlist.List + // Build the output that we expect. Since there is a difference between the IETF // and non-IETF forms, we simply choose vals to be any, and then type assert // it later on. Since t cannot mutuate through this function we can guarantee that @@ -1524,11 +1518,10 @@ func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (any, default: return nil, fmt.Errorf("invalid JSON format specified: %v", args.jType) } - for _, kn := range mapKeys { - k := mapKeyMap[kn] - goStruct, ok := field.MapIndex(k).Interface().(GoStruct) + for _, pair := range pairs { + goStruct, ok := pair.v.Interface().(GoStruct) if !ok { - errs.Add(fmt.Errorf("cannot map struct %v, invalid GoStruct", field)) + errs.Add(fmt.Errorf("cannot map struct %v, invalid GoStruct", pair.v.Interface())) continue } @@ -1542,19 +1535,46 @@ func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (any, case RFC7951: vals = append(vals.([]any), val) case Internal: - vals.(map[string]any)[kn] = val + vals.(map[string]any)[pair.k] = val default: errs.Add(fmt.Errorf("invalid JSON type: %v", args.jType)) continue } } - if errs.Err() != nil { return nil, errs.Err() } return vals, nil } +// mapJSON takes an input reflect.Value containing a map, and +// constructs the representation for JSON marshalling that corresponds to it. +// The module within which the map is defined is specified by the parentMod +// argument. +func mapJSON(field reflect.Value, parentMod string, args jsonOutputConfig) (any, error) { + var errs errlist.List + // Order of elements determines the order in which keys will be processed. + var pairs []mapValuePair + + iter := field.MapRange() + for iter.Next() { + kn, err := mapKeyToJSONString(iter.Key(), args) + if err != nil { + errs.Add(err) + continue + } + pairs = append(pairs, mapValuePair{k: kn, v: iter.Value()}) + } + slices.SortFunc(pairs, func(a, b mapValuePair) bool { return a.k < b.k }) + + js, err := mapValuePairsToJSON(pairs, parentMod, args) + errs.Add(err) + if errs.Err() != nil { + return nil, errs.Err() + } + return js, nil +} + // jsonValue takes a reflect.Value which represents a struct field and // constructs the representation that can be used to marshal the field to JSON. // The module within which the value is defined is specified by the parentMod string, @@ -1581,21 +1601,26 @@ func jsonValue(field reflect.Value, parentMod string, args jsonOutputConfig) (an errs.Add(err) } case reflect.Ptr: - if _, ok := field.Interface().(GoOrderedMap); ok { - // This is an ordered-map for YANG "ordered-by user" lists. - valuesMethod, err := yreflect.MethodByName(field, "Values") - if err != nil { - return nil, err - } - ret := valuesMethod.Call(nil) - if got, wantReturnN := len(ret), 1; got != wantReturnN { - return nil, fmt.Errorf("method Values() doesn't have expected number of return values, got %v, want %v", got, wantReturnN) + if om, ok := field.Interface().(GoOrderedMap); ok { + var pairs []mapValuePair + if err := yreflect.RangeOrderedMap(om, func(k reflect.Value, v reflect.Value) bool { + kn, err := mapKeyToJSONString(k, args) + if err != nil { + errs.Add(err) + return true + } + pairs = append(pairs, mapValuePair{k: kn, v: v}) + return true + }); err != nil { + errs.Add(err) } - values := ret[0] - if gotKind := values.Type().Kind(); gotKind != reflect.Slice { - return nil, fmt.Errorf("method Values() did not return a slice value, got %v", gotKind) + + js, err := mapValuePairsToJSON(pairs, parentMod, args) + errs.Add(err) + if errs.Err() != nil { + return nil, errs.Err() } - return jsonValue(values, parentMod, args) + return js, nil } switch field.Elem().Kind() { diff --git a/ygot/render_exported_test.go b/ygot/render_exported_test.go index 7f127096..d37817bc 100644 --- a/ygot/render_exported_test.go +++ b/ygot/render_exported_test.go @@ -363,8 +363,8 @@ func TestConstructJSONOrderedMap(t *testing.T) { }, wantInternal: map[string]any{ "ordered-multikeyed-lists": map[string]any{ - "ordered-multikeyed-list": []any{ - map[string]any{ + "ordered-multikeyed-list": map[string]any{ + "foo 42": map[string]any{ "key1": "foo", "key2": uint64(42), "config": map[string]any{ @@ -376,7 +376,7 @@ func TestConstructJSONOrderedMap(t *testing.T) { "ro-value": "foo-state-val", }, }, - map[string]any{ + "bar 42": map[string]any{ "key1": "bar", "key2": uint64(42), "config": map[string]any{ @@ -388,7 +388,7 @@ func TestConstructJSONOrderedMap(t *testing.T) { "ro-value": "bar-state-val", }, }, - map[string]any{ + "baz 84": map[string]any{ "key1": "baz", "key2": uint64(84), "config": map[string]any{ @@ -454,8 +454,8 @@ func TestConstructJSONOrderedMap(t *testing.T) { }, wantInternal: map[string]any{ "ordered-multikeyed-lists": map[string]any{ - "ordered-multikeyed-list": []any{ - map[string]any{ + "ordered-multikeyed-list": map[string]any{ + "foo 42": map[string]any{ "key1": "foo", "key2": uint64(42), "config": map[string]any{ @@ -465,7 +465,7 @@ func TestConstructJSONOrderedMap(t *testing.T) { "ro-value": "foo-state-val", }, }, - map[string]any{ + "bar 42": map[string]any{ "key1": "bar", "key2": uint64(42), "state": map[string]any{ @@ -473,7 +473,7 @@ func TestConstructJSONOrderedMap(t *testing.T) { "ro-value": "bar-state-val", }, }, - map[string]any{ + "baz 84": map[string]any{ "key1": "baz", "key2": uint64(84), "config": map[string]any{ @@ -560,22 +560,22 @@ func TestEncodeTypedValueOrderedMap(t *testing.T) { }{{ name: "ordered list type", inVal: ctestschema.GetOrderedMap(t), - want: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonVal{JsonVal: []byte(`[ - { - "config": { - "key": "foo", - "value": "foo-val" - }, - "key": "foo" - }, - { + want: &gnmipb.TypedValue{Value: &gnmipb.TypedValue_JsonVal{JsonVal: []byte(`{ + "bar": { "config": { "key": "bar", "value": "bar-val" }, "key": "bar" + }, + "foo": { + "config": { + "key": "foo", + "value": "foo-val" + }, + "key": "foo" } -]`)}}, +}`)}}, }, { name: "ordered list type - ietf json", inVal: ctestschema.GetOrderedMap(t), diff --git a/ygot/testdata/emitjson_orderedmap_container_internal.json-txt b/ygot/testdata/emitjson_orderedmap_container_internal.json-txt index 5263e6d4..802266f3 100644 --- a/ygot/testdata/emitjson_orderedmap_container_internal.json-txt +++ b/ygot/testdata/emitjson_orderedmap_container_internal.json-txt @@ -1,27 +1,27 @@ { "ordered-lists": { - "ordered-list": [ - { - "config": { - "key": "foo", - "value": "foo-val" - }, - "key": "foo" - }, - { + "ordered-list": { + "bar": { "config": { "key": "bar", "value": "bar-val" }, "key": "bar" }, - { + "baz": { "config": { "key": "baz", "value": "baz-val" }, "key": "baz" + }, + "foo": { + "config": { + "key": "foo", + "value": "foo-val" + }, + "key": "foo" } - ] + } } }