Repository: ovn-org/libovsdb
Pull request #290 (2022-02-04 14:26:54)

api: support pointer to slice of Models for List
The whole API is based on Model defined as a pointer to a struct except
the List method which takes a pointer to a slice of structs which
technically is not equivalent to a pointer to a slice of Models. This
causes a lot of user code transforming to and from these two types.
Incidentally, this looks like it might have not been originally intended
looking at the existing bugs on the List api test.

Thus add support for the latter type and keep support of the former for
backward compatibility.

Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>


Patch:
diff --git a/client/api.go b/client/api.go
index 0297ff3..f831cf6 100644
--- a/client/api.go
+++ b/client/api.go
@@ -16,6 +16,7 @@ import (
 type API interface {
 	// List populates a slice of Models objects based on their type
 	// The function parameter must be a pointer to a slice of Models
+	// Models can be structs or pointers to structs
 	// If the slice is null, the entire cache will be copied into the slice
 	// If it has a capacity != 0, only 'capacity' elements will be filled in
 	List(ctx context.Context, result interface{}) error
@@ -104,7 +105,23 @@ func (a api) List(ctx context.Context, result interface{}) error {
 		return &ErrWrongType{resultPtr.Type(), "Expected pointer to slice of valid Models"}
 	}
 
-	table, err := a.getTableFromModel(reflect.New(resultVal.Type().Elem()).Interface())
+	// List accepts a slice of Models that can be either structs or pointer to
+	// structs
+	var appendValue func(reflect.Value)
+	var m model.Model
+	if resultVal.Type().Elem().Kind() == reflect.Ptr {
+		m = reflect.New(resultVal.Type().Elem().Elem()).Interface()
+		appendValue = func(v reflect.Value) {
+			resultVal.Set(reflect.Append(resultVal, v))
+		}
+	} else {
+		m = reflect.New(resultVal.Type().Elem()).Interface()
+		appendValue = func(v reflect.Value) {
+			resultVal.Set(reflect.Append(resultVal, reflect.Indirect(v)))
+		}
+	}
+
+	table, err := a.getTableFromModel(m)
 	if err != nil {
 		return err
 	}
@@ -141,7 +158,7 @@ func (a api) List(ctx context.Context, result interface{}) error {
 		// clone only the models that match the predicate
 		m := model.Clone(row)
 
-		resultVal.Set(reflect.Append(resultVal, reflect.Indirect(reflect.ValueOf(m))))
+		appendValue(reflect.ValueOf(m))
 		i++
 	}
 	return nil


Test patch:
diff --git a/client/api_test.go b/client/api_test.go
index 8c10beb..f9e6e72 100644
--- a/client/api_test.go
+++ b/client/api_test.go
@@ -78,7 +78,7 @@ func TestAPIListSimple(t *testing.T) {
 			initialCap: 1,
 			resultCap:  1,
 			resultLen:  1,
-			content:    lscacheList[0:0],
+			content:    lscacheList,
 			err:        false,
 		},
 		{
@@ -86,15 +86,34 @@ func TestAPIListSimple(t *testing.T) {
 			initialCap: 2,
 			resultCap:  2,
 			resultLen:  2,
-			content:    lscacheList[0:2],
+			content:    lscacheList,
 			err:        false,
 		},
 	}
+	hasDups := func(a interface{}) bool {
+		l := map[string]struct{}{}
+		switch v := a.(type) {
+		case []testLogicalSwitch:
+			for _, i := range v {
+				if _, ok := l[i.Name]; ok {
+					return ok
+				}
+			}
+		case []*testLogicalSwitch:
+			for _, i := range v {
+				if _, ok := l[i.Name]; ok {
+					return ok
+				}
+			}
+		}
+		return false
+	}
 	for _, tt := range test {
 		t.Run(fmt.Sprintf("ApiList: %s", tt.name), func(t *testing.T) {
-			var result []testLogicalSwitch
+			// test List with a pointer to a slice of Models
+			var result []*testLogicalSwitch
 			if tt.initialCap != 0 {
-				result = make([]testLogicalSwitch, tt.initialCap)
+				result = make([]*testLogicalSwitch, 0, tt.initialCap)
 			}
 			api := newAPI(tcache, &discardLogger)
 			err := api.List(context.Background(), &result)
@@ -104,7 +123,28 @@ func TestAPIListSimple(t *testing.T) {
 				assert.Nil(t, err)
 				assert.Lenf(t, result, tt.resultLen, "Length should match expected")
 				assert.Equal(t, cap(result), tt.resultCap, "Capability should match expected")
-				assert.ElementsMatchf(t, tt.content, tt.content, "Content should match")
+				assert.Subsetf(t, tt.content, result, "Result should be a subset of expected")
+				assert.False(t, hasDups(result), "Result should have no duplicates")
+			}
+
+			// test List with a pointer to a slice of structs
+			var resultWithNoPtr []testLogicalSwitch
+			if tt.initialCap != 0 {
+				resultWithNoPtr = make([]testLogicalSwitch, 0, tt.initialCap)
+			}
+			contentNoPtr := make([]testLogicalSwitch, 0, len(tt.content))
+			for i := range tt.content {
+				contentNoPtr = append(contentNoPtr, *tt.content[i].(*testLogicalSwitch))
+			}
+			err = api.List(context.Background(), &resultWithNoPtr)
+			if tt.err {
+				assert.NotNil(t, err)
+			} else {
+				assert.Nil(t, err)
+				assert.Lenf(t, result, tt.resultLen, "Length should match expected")
+				assert.Equal(t, cap(result), tt.resultCap, "Capability should match expected")
+				assert.Subsetf(t, contentNoPtr, resultWithNoPtr, "Result should be a subset of expected")
+				assert.False(t, hasDups(resultWithNoPtr), "Result should have no duplicates")
 			}
 
 		})