From 1ce1d52ef1a18f1cc28693ac50393968346956b5 Mon Sep 17 00:00:00 2001 From: Dave Tucker Date: Fri, 8 Oct 2021 20:28:06 +0100 Subject: [PATCH] cache: Fix ApplyModifications for pointer fields Signed-off-by: Dave Tucker --- cache/cache.go | 44 +++++++++++++++++++++++++++++++++++++++----- cache/cache_test.go | 28 ++++++++++++++++++++++++++-- ovsdb/updates2.go | 2 -- 3 files changed, 65 insertions(+), 9 deletions(-) diff --git a/cache/cache.go b/cache/cache.go index 80eb589f..e32e5d7a 100644 --- a/cache/cache.go +++ b/cache/cache.go @@ -780,6 +780,7 @@ func (t *TableCache) CreateModel(tableName string, row *ovsdb.Row, uuid string) } // ApplyModifications applies the contents of a RowUpdate2.Modify to a model +// nolint: gocyclo func (t *TableCache) ApplyModifications(tableName string, base model.Model, update ovsdb.Row) error { table := t.mapper.Schema.Table(tableName) if table == nil { @@ -797,19 +798,29 @@ func (t *TableCache) ApplyModifications(tableName string, base model.Model, upda if k == "_uuid" { continue } - value, err := ovsdb.OvsToNative(schema.Column(k), v) + + current, err := info.FieldByColumn(k) + if err != nil { + return err + } + + var value interface{} + value, err = ovsdb.OvsToNative(schema.Column(k), v) + // we can overflow the max of a set with min: 0, max: 1 here because of the update2/update3 notation + // which to replace "foo" with "bar" would send a set with ["foo", "bar"] + if err != nil && schema.Column(k).Type == ovsdb.TypeSet && schema.Column(k).TypeObj.Max() == 1 { + value, err = ovsdb.OvsToNativeSlice(schema.Column(k).TypeObj.Key.Type, v) + } if err != nil { return err } nv := reflect.ValueOf(value) - switch nv.Kind() { + switch reflect.ValueOf(current).Kind() { case reflect.Slice, reflect.Array: // The difference between two sets are all elements that only belong to one of the sets. - // Iterate new values for i := 0; i < nv.Len(); i++ { - // search for match in base values baseValue, err := info.FieldByColumn(k) if err != nil { @@ -837,7 +848,30 @@ func (t *TableCache) ApplyModifications(tableName string, base model.Model, upda } } } - + case reflect.Ptr: + // if NativeToOVS was successful, then simply assign + if nv.Type() == reflect.ValueOf(current).Type() { + err = info.SetField(k, nv.Interface()) + return err + } + // With a pointer type, an update value could be a set with 2 elements [old, new] + if nv.Len() != 2 { + panic("expected a slice with 2 elements") + } + // the new value is the value in the slice which isn't equal to the existing string + for i := 0; i < nv.Len(); i++ { + baseValue, err := info.FieldByColumn(k) + if err != nil { + return err + } + bv := reflect.ValueOf(baseValue) + if nv.Index(i) != bv { + err = info.SetField(k, nv.Index(i).Addr().Interface()) + if err != nil { + return err + } + } + } case reflect.Map: // The difference between two maps are all key-value pairs whose keys appears in only one of the maps, // plus the key-value pairs whose keys appear in both maps but with different values. diff --git a/cache/cache_test.go b/cache/cache_test.go index 14c109a0..411aa785 100644 --- a/cache/cache_test.go +++ b/cache/cache_test.go @@ -1179,7 +1179,9 @@ func TestTableCacheApplyModifications(t *testing.T) { Set []string `ovsdb:"set"` Map map[string]string `ovsdb:"map"` Map2 map[string]string `ovsdb:"map2"` + Ptr *string `ovsdb:"ptr"` } + aEmptySet, _ := ovsdb.NewOvsSet([]string{}) aFooSet, _ := ovsdb.NewOvsSet([]string{"foo"}) aFooBarSet, _ := ovsdb.NewOvsSet([]string{"foo", "bar"}) aFooMap, _ := ovsdb.NewOvsMap(map[string]string{"foo": "bar"}) @@ -1188,6 +1190,10 @@ func TestTableCacheApplyModifications(t *testing.T) { "bar": "baz", "baz": "quux", }) + wallace := "wallace" + aWallaceSet, _ := ovsdb.NewOvsSet([]string{wallace}) + gromit := "gromit" + aWallaceGromitSet, _ := ovsdb.NewOvsSet([]string{wallace, gromit}) tests := []struct { name string update ovsdb.Row @@ -1224,7 +1230,6 @@ func TestTableCacheApplyModifications(t *testing.T) { &testDBModel{Set: []string{"foo"}}, &testDBModel{Set: []string{"bar"}}, }, - { "replace map value", ovsdb.Row{"map": aFooMap}, @@ -1252,6 +1257,24 @@ func TestTableCacheApplyModifications(t *testing.T) { Map2: map[string]string{"foo": "bar"}, }, }, + { + "set optional value", + ovsdb.Row{"ptr": aWallaceSet}, + &testDBModel{Ptr: nil}, + &testDBModel{Ptr: &wallace}, + }, + { + "replace optional value", + ovsdb.Row{"ptr": aWallaceGromitSet}, + &testDBModel{Ptr: &wallace}, + &testDBModel{Ptr: &gromit}, + }, + { + "delete optional value", + ovsdb.Row{"ptr": aEmptySet}, + &testDBModel{Ptr: &wallace}, + &testDBModel{Ptr: nil}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1268,7 +1291,8 @@ func TestTableCacheApplyModifications(t *testing.T) { "value": { "type": "string" }, "set": { "type": { "key": { "type": "string" }, "min": 0, "max": "unlimited" } }, "map": { "type": { "key": "string", "max": "unlimited", "min": 0, "value": "string" } }, - "map2": { "type": { "key": "string", "max": "unlimited", "min": 0, "value": "string" } } + "map2": { "type": { "key": "string", "max": "unlimited", "min": 0, "value": "string" } }, + "ptr": { "type": { "key": { "type": "string" }, "min": 0, "max": 1 } } } } } diff --git a/ovsdb/updates2.go b/ovsdb/updates2.go index 64bd2e80..f694dbc1 100644 --- a/ovsdb/updates2.go +++ b/ovsdb/updates2.go @@ -81,8 +81,6 @@ func (r *RowUpdate2) Merge(new *RowUpdate2) { oMap.GoMap[newK] = newV } } - default: - panic("ARGH!") } } }