From 1e8449fbcd8acf1c90a7a6f16207dfd92696c47b Mon Sep 17 00:00:00 2001 From: Sean Condon Date: Fri, 11 Sep 2020 10:41:31 +0100 Subject: [PATCH 1/2] Modelregistry: fix bug in ExtractPaths Also removed a duplicate function --- .../jsonvalues/convertJsonTd2_test.go | 5 +- pkg/modelregistry/jsonvalues/jsonToValues.go | 71 ++++++++++--------- .../jsonvalues/jsonToValues_test.go | 19 ----- .../testdata/sample-testdevice2-opstate.json | 2 +- pkg/modelregistry/modelregistry.go | 48 +++++++++++-- pkg/modelregistry/modelregistry_test.go | 39 ++++++---- pkg/northbound/gnmi/set.go | 21 +++--- pkg/northbound/gnmi/set_test.go | 2 +- 8 files changed, 126 insertions(+), 81 deletions(-) diff --git a/pkg/modelregistry/jsonvalues/convertJsonTd2_test.go b/pkg/modelregistry/jsonvalues/convertJsonTd2_test.go index 33813f023..444e05d64 100644 --- a/pkg/modelregistry/jsonvalues/convertJsonTd2_test.go +++ b/pkg/modelregistry/jsonvalues/convertJsonTd2_test.go @@ -215,10 +215,13 @@ func Test_DecomposeJSONWithPathsTd2OpState(t *testing.T) { "/cont1b-state/list2b[index1=101][index2=102]/leaf3c", "/cont1b-state/list2b[index1=101][index2=103]/leaf3c", "/cont1b-state/list2b[index1=101][index2=102]/leaf3d", - "/cont1b-state/list2b[index1=101][index2=103]/leaf3d", "/cont1b-state/cont2c/leaf3b": assert.Equal(t, pathValue.GetValue().GetType(), devicechange.ValueType_STRING, pathValue.Path) assert.Equal(t, len(pathValue.GetValue().GetTypeOpts()), 0) + case "/cont1b-state/list2b[index1=101][index2=103]/leaf3d": + assert.Equal(t, pathValue.GetValue().GetType(), devicechange.ValueType_STRING, pathValue.Path) + strVal := (*devicechange.TypedString)(pathValue.GetValue()).String() + assert.Equal(t, "IDTYPE2", strVal) // Should do the conversion fron "2" to "IDTYPE2" case "/cont1b-state/leaf2d": assert.Equal(t, pathValue.GetValue().GetType(), devicechange.ValueType_UINT, pathValue.Path) diff --git a/pkg/modelregistry/jsonvalues/jsonToValues.go b/pkg/modelregistry/jsonvalues/jsonToValues.go index f0b1ffc7e..7b66d879f 100644 --- a/pkg/modelregistry/jsonvalues/jsonToValues.go +++ b/pkg/modelregistry/jsonvalues/jsonToValues.go @@ -21,13 +21,11 @@ import ( devicechange "github.com/onosproject/onos-config/api/types/change/device" "github.com/onosproject/onos-config/pkg/modelregistry" "math" - "regexp" "sort" "strconv" "strings" ) -const matchOnIndex = `(\[.*?]).*?` const ( slash = "/" equals = "=" @@ -36,8 +34,6 @@ const ( colon = ":" ) -var rOnIndex = regexp.MustCompile(matchOnIndex) - type indexValue struct { name string value *devicechange.TypedValue @@ -90,7 +86,7 @@ func extractValuesWithPaths(f interface{}, parentPath string, for _, obj := range objs { isIndex := false for i, idxName := range indexNames { - if removePathIndices(obj.Path) == fmt.Sprintf("%s/%s", removePathIndices(parentPath), idxName) { + if modelregistry.RemovePathIndices(obj.Path) == fmt.Sprintf("%s/%s", modelregistry.RemovePathIndices(parentPath), idxName) { indices = append(indices, indexValue{name: idxName, value: obj.Value, order: i}) isIndex = true break @@ -234,6 +230,7 @@ func handleAttribute(value interface{}, parentPath string, modelROpaths modelreg var ok bool var pathElem *modelregistry.ReadWritePathElem var subPath *modelregistry.ReadOnlyAttrib + var enum map[int]string var err error pathElem, modelPath, ok = findModelRwPathNoIndices(modelRWpaths, parentPath) if !ok { @@ -246,8 +243,10 @@ func handleAttribute(value interface{}, parentPath string, modelROpaths modelreg return nil, fmt.Errorf("unable to locate %s in model", parentPath) } modeltype = subPath.Datatype + enum = subPath.Enum } else { modeltype = pathElem.ValueType + //enum = pathElem. } var typedValue *devicechange.TypedValue switch modeltype { @@ -255,7 +254,31 @@ func handleAttribute(value interface{}, parentPath string, modelROpaths modelreg var stringVal string switch valueTyped := value.(type) { case string: - stringVal = valueTyped + if len(enum) > 0 { + // Compare values and convert from digit if necessary + for k, v := range enum { + if v == valueTyped { + stringVal = valueTyped + break + } else if fmt.Sprintf("%d", k) == valueTyped { + stringVal = v + break + } + } + if stringVal == "" { + enumOpts := make([]string, len(enum)*2) + i := 0 + for k, v := range enum { + enumOpts[i*2] = fmt.Sprintf("%d", k) + enumOpts[i*2+1] = v + i++ + } + return nil, fmt.Errorf("value %s for %s does not match any enumerated value %s", + valueTyped, parentPath, strings.Join(enumOpts, ";")) + } + } else { + stringVal = valueTyped + } case float64: stringVal = fmt.Sprintf("%g", value) case bool: @@ -392,9 +415,9 @@ func handleAttributeLeafList(modeltype devicechange.ValueType, func findModelRwPathNoIndices(modelRWpaths modelregistry.ReadWritePathMap, searchpath string) (*modelregistry.ReadWritePathElem, string, bool) { - searchpathNoIndices := removePathIndices(searchpath) + searchpathNoIndices := modelregistry.RemovePathIndices(searchpath) for path, value := range modelRWpaths { - if removePathIndices(path) == searchpathNoIndices { + if modelregistry.RemovePathIndices(path) == searchpathNoIndices { pathWithNumericalIdx, err := insertNumericalIndices(path, searchpath) if err != nil { return nil, fmt.Sprintf("could not replace wildcards in model path with numerical ids %v", err), false @@ -408,7 +431,7 @@ func findModelRwPathNoIndices(modelRWpaths modelregistry.ReadWritePathMap, func findModelRoPathNoIndices(modelROpaths modelregistry.ReadOnlyPathMap, searchpath string) (*modelregistry.ReadOnlyAttrib, string, bool) { - searchpathNoIndices := removePathIndices(searchpath) + searchpathNoIndices := modelregistry.RemovePathIndices(searchpath) for path, value := range modelROpaths { for subpath, subpathValue := range value { var fullpath string @@ -417,7 +440,7 @@ func findModelRoPathNoIndices(modelROpaths modelregistry.ReadOnlyPathMap, } else { fullpath = fmt.Sprintf("%s%s", path, subpath) } - if removePathIndices(fullpath) == searchpathNoIndices { + if modelregistry.RemovePathIndices(fullpath) == searchpathNoIndices { pathWithNumericalIdx, err := insertNumericalIndices(fullpath, searchpath) if err != nil { return nil, fmt.Sprintf("could not replace wildcards in model path with numerical ids %v", err), false @@ -447,13 +470,13 @@ func stripNamespace(path string) string { func indicesOfPath(modelROpaths modelregistry.ReadOnlyPathMap, modelRWpaths modelregistry.ReadWritePathMap, searchpath string) []string { - searchpathNoIndices := removePathIndices(searchpath) + searchpathNoIndices := modelregistry.RemovePathIndices(searchpath) // First search through the RW paths for path := range modelRWpaths { - pathNoIndices := removePathIndices(path) + pathNoIndices := modelregistry.RemovePathIndices(path) // Find a short path if pathNoIndices[:strings.LastIndex(pathNoIndices, slash)] == searchpathNoIndices { - return extractIndexNames(path) + return modelregistry.ExtractIndexNames(path) } } @@ -466,10 +489,10 @@ func indicesOfPath(modelROpaths modelregistry.ReadOnlyPathMap, } else { fullpath = fmt.Sprintf("%s%s", path, subpath) } - pathNoIndices := removePathIndices(fullpath) + pathNoIndices := modelregistry.RemovePathIndices(fullpath) // Find a short path if pathNoIndices[:strings.LastIndex(pathNoIndices, slash)] == searchpathNoIndices { - return extractIndexNames(fullpath) + return modelregistry.ExtractIndexNames(fullpath) } } } @@ -477,24 +500,6 @@ func indicesOfPath(modelROpaths modelregistry.ReadOnlyPathMap, return []string{} } -func removePathIndices(path string) string { - jsonMatches := rOnIndex.FindAllStringSubmatch(path, -1) - for _, m := range jsonMatches { - path = strings.ReplaceAll(path, m[1], "") - } - return path -} - -func extractIndexNames(path string) []string { - indexNames := make([]string, 0) - jsonMatches := rOnIndex.FindAllStringSubmatch(path, -1) - for _, m := range jsonMatches { - idxName := m[1][1:strings.LastIndex(m[1], "=")] - indexNames = append(indexNames, idxName) - } - return indexNames -} - func insertNumericalIndices(modelPath string, jsonPath string) (string, error) { jsonParts := strings.Split(jsonPath, slash) modelParts := strings.Split(modelPath, slash) diff --git a/pkg/modelregistry/jsonvalues/jsonToValues_test.go b/pkg/modelregistry/jsonvalues/jsonToValues_test.go index 29c25d72c..8dbc75e4b 100644 --- a/pkg/modelregistry/jsonvalues/jsonToValues_test.go +++ b/pkg/modelregistry/jsonvalues/jsonToValues_test.go @@ -162,25 +162,6 @@ func Test_indicesOfPath(t *testing.T) { assert.Equal(t, "severity", indices[2]) } -func Test_removePathIndices(t *testing.T) { - const jsonPath = "/p/q/r[10]/s/t[20]/u/v[30]/w" - const jsonPathRemovedIdx = "/p/q/r/s/t/u/v/w" - noIndices := removePathIndices(jsonPath) - assert.Equal(t, jsonPathRemovedIdx, noIndices) -} - -func Test_extractIndexNames(t *testing.T) { - const modelPath = "/p/q/r[a=*]/s/t[b=*][c=*]/u/v[d=*][e=*][f=*]/w" - indexNames := extractIndexNames(modelPath) - assert.Equal(t, 6, len(indexNames)) - assert.Equal(t, "a", indexNames[0]) - assert.Equal(t, "b", indexNames[1]) - assert.Equal(t, "c", indexNames[2]) - assert.Equal(t, "d", indexNames[3]) - assert.Equal(t, "e", indexNames[4]) - assert.Equal(t, "f", indexNames[5]) -} - func Test_insertNumericalIndices(t *testing.T) { const modelPath = "/p/q/r[a=*]/s/t[b=*][c=*]/u/v[d=*][e=*][f=*]/w" const jsonPath = "/p/q/r[10]/s/t[20]/u/v[30]/w" diff --git a/pkg/modelregistry/jsonvalues/testdata/sample-testdevice2-opstate.json b/pkg/modelregistry/jsonvalues/testdata/sample-testdevice2-opstate.json index 4b586824e..986cdad81 100644 --- a/pkg/modelregistry/jsonvalues/testdata/sample-testdevice2-opstate.json +++ b/pkg/modelregistry/jsonvalues/testdata/sample-testdevice2-opstate.json @@ -16,7 +16,7 @@ "index1": 101, "index2": 103, "leaf3c": "Second mock Value", - "leaf3d": "IDTYPE2" + "leaf3d": "2" } ], "leaf2d": 10001, diff --git a/pkg/modelregistry/modelregistry.go b/pkg/modelregistry/modelregistry.go index 5b4e5491f..64d3014e7 100644 --- a/pkg/modelregistry/modelregistry.go +++ b/pkg/modelregistry/modelregistry.go @@ -57,11 +57,15 @@ const ( GetStateExplicitRoPathsExpandWildcards ) +// MatchOnIndex - regexp to find indices in paths names +const MatchOnIndex = `(\[.*?]).*?` + // ReadOnlyAttrib is the known metadata about a Read Only leaf type ReadOnlyAttrib struct { Datatype devicechange.ValueType Description string Units string + Enum map[int]string } // ReadOnlySubPathMap abstracts the read only subpath @@ -70,6 +74,8 @@ type ReadOnlySubPathMap map[string]ReadOnlyAttrib // ReadOnlyPathMap abstracts the read only path type ReadOnlyPathMap map[string]ReadOnlySubPathMap +var rOnIndex = regexp.MustCompile(MatchOnIndex) + // JustPaths extracts keys from a read only path map func (ro ReadOnlyPathMap) JustPaths() []string { keys := make([]string, 0) @@ -112,6 +118,7 @@ type ReadWritePathElem struct { Default string Range []string Length []string + Enum map[int]string } // ReadWritePathMap is a map of ReadWrite paths a their metadata @@ -265,6 +272,11 @@ func ExtractPaths(deviceEntry *yang.Entry, parentState yang.TriState, parentPath if err != nil { log.Errorf(err.Error()) } + var enum map[int]string + if dirEntry.Type.Kind == yang.Yidentityref { + enum = handleIdentity(dirEntry.Type) + } + tObj.Enum = enum if parentState == yang.TSFalse { leafMap, ok := readOnlyPaths[parentPath] if !ok { @@ -293,6 +305,7 @@ func ExtractPaths(deviceEntry *yang.Entry, parentState yang.TriState, parentPath Default: dirEntry.Default, Range: ranges, Length: lengths, + Enum: enum, } readWritePaths[itemPath] = rwElem } @@ -345,7 +358,14 @@ func ExtractPaths(deviceEntry *yang.Entry, parentState yang.TriState, parentPath // Need to copy the index of the list across to the RO list too roIdxName := k[:strings.LastIndex(k, "/")] roIdxSubPath := k[strings.LastIndex(k, "/"):] - if roIdxName == itemPath { + indices := ExtractIndexNames(itemPath[strings.LastIndex(itemPath, "/"):]) + kIsIdxAttr := false + for _, idx := range indices { + if roIdxSubPath == fmt.Sprintf("/%s", idx) { + kIsIdxAttr = true + } + } + if roIdxName == itemPath && kIsIdxAttr { roIdx := ReadOnlyAttrib{ Datatype: v.ValueType, Description: v.Description, @@ -374,15 +394,24 @@ func ExtractPaths(deviceEntry *yang.Entry, parentState yang.TriState, parentPath // RemovePathIndices removes the index value from a path to allow it to be compared to a model path func RemovePathIndices(path string) string { - const indexPattern = `=.*?]` - rname := regexp.MustCompile(indexPattern) - indices := rname.FindAllStringSubmatch(path, -1) + indices := rOnIndex.FindAllStringSubmatch(path, -1) for _, i := range indices { - path = strings.Replace(path, i[0], "=*]", 1) + path = strings.Replace(path, i[0], "", 1) } return path } +// ExtractIndexNames - get an ordered array of index names +func ExtractIndexNames(path string) []string { + indexNames := make([]string, 0) + jsonMatches := rOnIndex.FindAllStringSubmatch(path, -1) + for _, m := range jsonMatches { + idxName := m[1][1:strings.LastIndex(m[1], "=")] + indexNames = append(indexNames, idxName) + } + return indexNames +} + func formatName(dirEntry *yang.Entry, isList bool, parentPath string, subpathPrefix string) string { parentAndSubPath := parentPath if subpathPrefix != "/" { @@ -464,3 +493,12 @@ func toValueType(entry *yang.YangType, isLeafList bool) (devicechange.ValueType, return devicechange.ValueType_STRING, nil } } + +func handleIdentity(yangType *yang.YangType) map[int]string { + identityMap := make(map[int]string) + identityMap[0] = "UNSET" + for i, val := range yangType.IdentityBase.Values { + identityMap[i+1] = val.Name + } + return identityMap +} diff --git a/pkg/modelregistry/modelregistry_test.go b/pkg/modelregistry/modelregistry_test.go index 005e768af..5fac8fa01 100644 --- a/pkg/modelregistry/modelregistry_test.go +++ b/pkg/modelregistry/modelregistry_test.go @@ -661,24 +661,27 @@ func Test_Ntp(t *testing.T) { } func Test_RemovePathIndices(t *testing.T) { - assert.Equal(t, - RemovePathIndices("/cont1b-state"), - "/cont1b-state") + assert.Equal(t, "/cont1b-state", + RemovePathIndices("/cont1b-state")) - assert.Equal(t, - RemovePathIndices("/cont1b-state/list2b[index=test1]/index"), - "/cont1b-state/list2b[index=*]/index") + assert.Equal(t, "/cont1b-state/list2b/index", + RemovePathIndices("/cont1b-state/list2b[index=test1]/index")) - assert.Equal(t, - RemovePathIndices("/cont1b-state/list2b[index=test1,test2]/index"), - "/cont1b-state/list2b[index=*]/index") + assert.Equal(t, "/cont1b-state/list2b/index", + RemovePathIndices("/cont1b-state/list2b[index=test1,test2]/index")) - assert.Equal(t, - RemovePathIndices("/cont1b-state/list2b[index=test1]/index/t3[name=5]"), - "/cont1b-state/list2b[index=*]/index/t3[name=*]") + assert.Equal(t, "/cont1b-state/list2b/index/t3", + RemovePathIndices("/cont1b-state/list2b[index=test1]/index/t3[name=5]")) } +func Test_RemovePathIndices2(t *testing.T) { + const jsonPath = "/p/q/r[10]/s/t[20]/u/v[30]/w" + const jsonPathRemovedIdx = "/p/q/r/s/t/u/v/w" + noIndices := RemovePathIndices(jsonPath) + assert.Equal(t, jsonPathRemovedIdx, noIndices) +} + func Test_formatName1(t *testing.T) { dirEntry1 := yang.Entry{ Name: "testname", @@ -695,3 +698,15 @@ func Test_formatName2(t *testing.T) { } assert.Equal(t, formatName(&dirEntry1, true, "/testpath/testpath2", ""), "/testpath/testpath2/testname[name=*]") } + +func Test_extractIndexNames(t *testing.T) { + const modelPath = "/p/q/r[a=*]/s/t[b=*][c=*]/u/v[d=*][e=*][f=*]/w" + indexNames := ExtractIndexNames(modelPath) + assert.Equal(t, 6, len(indexNames)) + assert.Equal(t, "a", indexNames[0]) + assert.Equal(t, "b", indexNames[1]) + assert.Equal(t, "c", indexNames[2]) + assert.Equal(t, "d", indexNames[3]) + assert.Equal(t, "e", indexNames[4]) + assert.Equal(t, "f", indexNames[5]) +} diff --git a/pkg/northbound/gnmi/set.go b/pkg/northbound/gnmi/set.go index e77023d14..70586c7f4 100644 --- a/pkg/northbound/gnmi/set.go +++ b/pkg/northbound/gnmi/set.go @@ -292,13 +292,15 @@ func (s *Server) formatUpdateOrReplace(prefix *gnmi.Path, u *gnmi.Update, "Model Plugin not available for target %s", target) } - correctedValues, err := jsonvalues.DecomposeJSONWithPaths(path, jsonVal, nil, rwPaths) + pathValues, err := jsonvalues.DecomposeJSONWithPaths(path, jsonVal, nil, rwPaths) if err != nil { - log.Warn("Json value in Set could not be parsed", err) + log.Warnf("Json value in Set could not be parsed %v", err) return nil, err } - - for _, cv := range correctedValues { + if len(pathValues) == 0 { + log.Warnf("no pathValues found for %s in %v", path, string(jsonVal)) + } + for _, cv := range pathValues { updates[cv.Path] = cv.GetValue() } } else { @@ -373,16 +375,17 @@ func compareRoPaths(path string, model modelregistry.ReadOnlyPathMap) error { for ropath, subpaths := range model { // Search through for list indices and replace with generic modelPath := modelregistry.RemovePathIndices(path) - if strings.HasPrefix(modelPath, ropath) { + ropathNoIdx := modelregistry.RemovePathIndices(ropath) + if strings.HasPrefix(modelPath, ropathNoIdx) { for s := range subpaths { - fullpath := ropath + fullpath := ropathNoIdx if s != "/" { - fullpath = fmt.Sprintf("%s%s", ropath, s) + fullpath = fmt.Sprintf("%s%s", ropathNoIdx, s) } if fullpath == modelPath { return fmt.Errorf("contains a change to a "+ - "read only path %s. Rejected. %s, %s, %s, %s", - path, modelPath, ropath, s, fullpath) + "read only path %s. Rejected. %s, %s, %s, %s, %s", + path, modelPath, ropath, ropathNoIdx, s, fullpath) } } } diff --git a/pkg/northbound/gnmi/set_test.go b/pkg/northbound/gnmi/set_test.go index 736ad0954..f36f5f677 100644 --- a/pkg/northbound/gnmi/set_test.go +++ b/pkg/northbound/gnmi/set_test.go @@ -521,7 +521,7 @@ func TestSet_checkForReadOnly(t *testing.T) { err := server.checkForReadOnly("device-1", "TestDevice", "1.0.0", updateT1, make([]string, 0)) assert.NilError(t, err, "unexpected error on T1") err = server.checkForReadOnly("device-2", "TestDevice", "1.0.0", updateT2, make([]string, 0)) - assert.Error(t, err, `update contains a change to a read only path /cont1a/cont2a/leaf2c. Rejected. /cont1a/cont2a/leaf2c, /cont1a/cont2a/leaf2c, /, /cont1a/cont2a/leaf2c`) + assert.Error(t, err, `update contains a change to a read only path /cont1a/cont2a/leaf2c. Rejected. /cont1a/cont2a/leaf2c, /cont1a/cont2a/leaf2c, /cont1a/cont2a/leaf2c, /, /cont1a/cont2a/leaf2c`) } // Tests giving a new device without specifying a type From f0f3bd21486a2e6f8cd083a1d2c1a6a14473dd64 Mon Sep 17 00:00:00 2001 From: Sean Condon Date: Fri, 11 Sep 2020 21:30:06 +0100 Subject: [PATCH 2/2] Addressing review comment --- pkg/modelregistry/jsonvalues/jsonToValues.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/modelregistry/jsonvalues/jsonToValues.go b/pkg/modelregistry/jsonvalues/jsonToValues.go index 7b66d879f..0da0f1a60 100644 --- a/pkg/modelregistry/jsonvalues/jsonToValues.go +++ b/pkg/modelregistry/jsonvalues/jsonToValues.go @@ -246,7 +246,6 @@ func handleAttribute(value interface{}, parentPath string, modelROpaths modelreg enum = subPath.Enum } else { modeltype = pathElem.ValueType - //enum = pathElem. } var typedValue *devicechange.TypedValue switch modeltype {