Repository: ovn-org/libovsdb (go)

Issue:
Field filtering on Monitors does not work
With
```
// Only Monitor Required SBDB tables to reduce memory overhead
chassisPrivate := sbdb.ChassisPrivate{}
_, err = c.Monitor(ctx,
    c.NewMonitor(
	    client.WithTable(&chassisPrivate, &chassisPrivate.Name),
    ),
)
```
we get
`field pointer does not correspond to orm struct`

This is because the model passed in `WithTable` is not used later on to build the `TableMonitor`. Instead, a new model is built from the mapper for which the field pointer passed does not apply.

Pull request description:
monitor: Use strings instead of interfaces for Fields
When monitoring certain fields of a table, the model is
needed since their values are obtained as an offset.
Since the model is not kept in TableMonitor, this change
will convert the fields to strings sooner.

Fixes #303

Signed-off-by: Flavio Fernandes <flaviof@redhat.com>

Patch:
diff --git a/client/monitor.go b/client/monitor.go
index fe95d15..65b38ac 100644
--- a/client/monitor.go
+++ b/client/monitor.go
@@ -65,18 +65,43 @@ type TableMonitor struct {
 	Condition model.Condition
 	// Fields are the fields in the model to monitor
 	// If none are supplied, all fields will be used
-	Fields []interface{}
+	Fields []string
+}
+
+func fieldsAsStrings(dbModel model.DatabaseModel, m model.Model, fields []interface{}) ([]string, error) {
+	var columns []string
+
+	if len(fields) == 0 {
+		return columns, nil
+	}
+	data, err := dbModel.NewModelInfo(m)
+	if err != nil {
+		return nil, fmt.Errorf("unable to obtain info from model %v: %v", m, err)
+	}
+	for _, f := range fields {
+		column, err := data.ColumnByPtr(f)
+		if err != nil {
+			return nil, fmt.Errorf("unable to obtain column from model %v: %v", data, err)
+		}
+		columns = append(columns, column)
+	}
+	return columns, nil
 }
 
 func WithTable(m model.Model, fields ...interface{}) MonitorOption {
 	return func(o *ovsdbClient, monitor *Monitor) error {
-		tableName := o.primaryDB().model.FindTable(reflect.TypeOf(m))
+		dbModel := o.primaryDB().model
+		tableName := dbModel.FindTable(reflect.TypeOf(m))
 		if tableName == "" {
 			return fmt.Errorf("object of type %s is not part of the ClientDBModel", reflect.TypeOf(m))
 		}
+		fieldsStr, err := fieldsAsStrings(dbModel, m, fields)
+		if err != nil {
+			return err
+		}
 		tableMonitor := TableMonitor{
 			Table:  tableName,
-			Fields: fields,
+			Fields: fieldsStr,
 		}
 		monitor.Tables = append(monitor.Tables, tableMonitor)
 		return nil
@@ -85,14 +110,19 @@ func WithTable(m model.Model, fields ...interface{}) MonitorOption {
 
 func WithConditionalTable(m model.Model, condition model.Condition, fields ...interface{}) MonitorOption {
 	return func(o *ovsdbClient, monitor *Monitor) error {
-		tableName := o.primaryDB().model.FindTable(reflect.TypeOf(m))
+		dbModel := o.primaryDB().model
+		tableName := dbModel.FindTable(reflect.TypeOf(m))
 		if tableName == "" {
 			return fmt.Errorf("object of type %s is not part of the ClientDBModel", reflect.TypeOf(m))
 		}
+		fieldsStr, err := fieldsAsStrings(dbModel, m, fields)
+		if err != nil {
+			return err
+		}
 		tableMonitor := TableMonitor{
 			Table:     tableName,
 			Condition: condition,
-			Fields:    fields,
+			Fields:    fieldsStr,
 		}
 		monitor.Tables = append(monitor.Tables, tableMonitor)
 		return nil
diff --git a/mapper/mapper.go b/mapper/mapper.go
index 21cecc1..7b76200 100644
--- a/mapper/mapper.go
+++ b/mapper/mapper.go
@@ -319,16 +319,10 @@ func (m Mapper) equalIndexes(one, other *Info, indexes ...string) (bool, error)
 // NewMonitorRequest returns a monitor request for the provided tableName
 // If fields is provided, the request will be constrained to the provided columns
 // If no fields are provided, all columns will be used
-func (m *Mapper) NewMonitorRequest(data *Info, fields []interface{}) (*ovsdb.MonitorRequest, error) {
+func (m *Mapper) NewMonitorRequest(data *Info, fields []string) (*ovsdb.MonitorRequest, error) {
 	var columns []string
 	if len(fields) > 0 {
-		for _, f := range fields {
-			column, err := data.ColumnByPtr(f)
-			if err != nil {
-				return nil, err
-			}
-			columns = append(columns, column)
-		}
+		columns = append(columns, fields...)
 	} else {
 		for c := range data.Metadata.TableSchema.Columns {
 			columns = append(columns, c)


Test patch:
diff --git a/client/monitor_test.go b/client/monitor_test.go
index b1c28d3..a5336f6 100644
--- a/client/monitor_test.go
+++ b/client/monitor_test.go
@@ -1,8 +1,11 @@
 package client
 
 import (
+	"encoding/json"
 	"testing"
 
+	"github.com/ovn-org/libovsdb/model"
+	"github.com/ovn-org/libovsdb/ovsdb"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -17,3 +20,33 @@ func TestWithTable(t *testing.T) {
 
 	assert.Equal(t, 1, len(m.Tables))
 }
+
+func populateClientModel(t *testing.T, client *ovsdbClient) {
+	var s ovsdb.DatabaseSchema
+	err := json.Unmarshal([]byte(schema), &s)
+	assert.NoError(t, err)
+	clientDBModel, err := model.NewClientDBModel("Open_vSwitch", map[string]model.Model{
+		"Bridge":       &Bridge{},
+		"Open_vSwitch": &OpenvSwitch{},
+	})
+	assert.NoError(t, err)
+	dbModel, errs := model.NewDatabaseModel(s, clientDBModel)
+	assert.Empty(t, errs)
+	client.primaryDB().model = dbModel
+	assert.NoError(t, err)
+}
+
+func TestWithTableAndFields(t *testing.T) {
+	client, err := newOVSDBClient(defDB)
+	assert.NoError(t, err)
+	populateClientModel(t, client)
+
+	m := newMonitor()
+	ovs := OpenvSwitch{}
+	opt := WithTable(&ovs, &ovs.Bridges, &ovs.CurCfg)
+	err = opt(client, m)
+	assert.NoError(t, err)
+
+	assert.Equal(t, 1, len(m.Tables))
+	assert.ElementsMatch(t, []string{"bridges", "cur_cfg"}, m.Tables[0].Fields)
+}
diff --git a/mapper/mapper_test.go b/mapper/mapper_test.go
index 5510614..2cefede 100644
--- a/mapper/mapper_test.go
+++ b/mapper/mapper_test.go
@@ -1124,7 +1124,7 @@ func TestNewMonitorRequest(t *testing.T) {
 	mr, err := mapper.NewMonitorRequest(info, nil)
 	require.NoError(t, err)
 	assert.ElementsMatch(t, mr.Columns, []string{"name", "config", "composed_1", "composed_2", "int1", "int2"})
-	mr2, err := mapper.NewMonitorRequest(info, []interface{}{&testTable.Int1, &testTable.MyName})
+	mr2, err := mapper.NewMonitorRequest(info, []string{"int1", "name"})
 	require.NoError(t, err)
 	assert.ElementsMatch(t, mr2.Columns, []string{"int1", "name"})
 }