Repository: ovn-org/libovsdb
Pull request #119 (2021-05-19 13:38:22)

ovsdb: Improve Monitor Select Handling
As pointed out in #87 we don't handle MonitorSelect as defined
in RFC7047. If a field is nil, it should be intrepretted as true.

This commit adds differentiates between the type used for marshalling
vs. the type exposed in the API. It provides an easy way of generating
MonitorSelect objects using bools, and provides functions for
determining the value of a field. E.g `Initial()` will return
true if nil or set to true, or false if set to false.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>

dave-tucker: @roytman please take a look. I felt it was easier to quickly open a PR than to explain what I had in mind in #87.
This should solve for RFC compliance while keeping the API clean.
coveralls: ## Pull Request Test Coverage Report for [Build 857042579](https://coveralls.io/builds/39819420)

* **51** of **53**   **(96.23%)**  changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.5%**) to **72.442%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [ovsdb/monitor_select.go](https://coveralls.io/builds/39819420/source?filename=ovsdb%2Fmonitor_select.go#L81) | 49 | 51 | 96.08%
<!-- | **Total:** | **51** | **53** | **96.23%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/39819420/badge)](https://coveralls.io/builds/39819420) |
| :-- | --: |
| Change from base [Build 856811939](https://coveralls.io/builds/39816734): |  0.5% |
| Covered Lines: | 1777 |
| Relevant Lines: | 2453 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/client/client.go b/client/client.go
index 153a749..7606fde 100644
--- a/client/client.go
+++ b/client/client.go
@@ -271,12 +271,8 @@ func (ovs OvsdbClient) MonitorAll(jsonContext interface{}) error {
 		}
 		requests[table] = ovsdb.MonitorRequest{
 			Columns: columns,
-			Select: ovsdb.MonitorSelect{
-				Initial: true,
-				Insert:  true,
-				Delete:  true,
-				Modify:  true,
-			}}
+			Select:  ovsdb.NewDefaultMonitorSelect(),
+		}
 	}
 	return ovs.Monitor(jsonContext, requests)
 }
diff --git a/ovsdb/monitor_select.go b/ovsdb/monitor_select.go
new file mode 100644
index 0000000..b97e062
--- /dev/null
+++ b/ovsdb/monitor_select.go
@@ -0,0 +1,88 @@
+package ovsdb
+
+import "encoding/json"
+
+// MonitorSelect represents a monitor select according to RFC7047
+type MonitorSelect struct {
+	initial *bool
+	insert  *bool
+	delete  *bool
+	modify  *bool
+}
+
+// NewMonitorSelect returns a new MonitorSelect with the provided values
+func NewMonitorSelect(initial, insert, delete, modify bool) *MonitorSelect {
+	return &MonitorSelect{
+		initial: &initial,
+		insert:  &insert,
+		delete:  &delete,
+		modify:  &modify,
+	}
+}
+
+// NewDefaultMonitorSelect returns a new MonitorSelect with default values
+func NewDefaultMonitorSelect() *MonitorSelect {
+	return NewMonitorSelect(true, true, true, true)
+}
+
+// Initial returns whether or not an initial response will be sent
+func (m MonitorSelect) Initial() bool {
+	if m.initial == nil {
+		return true
+	}
+	return *m.initial
+}
+
+// Insert returns whether we will receive updates for inserts
+func (m MonitorSelect) Insert() bool {
+	if m.insert == nil {
+		return true
+	}
+	return *m.insert
+}
+
+// Delete returns whether we will receive updates for deletions
+func (m MonitorSelect) Delete() bool {
+	if m.delete == nil {
+		return true
+	}
+	return *m.delete
+}
+
+// Modify returns whether we will receive updates for modifications
+func (m MonitorSelect) Modify() bool {
+	if m.modify == nil {
+		return true
+	}
+	return *m.modify
+}
+
+type monitorSelect struct {
+	Initial *bool `json:"initial,omitempty"`
+	Insert  *bool `json:"insert,omitempty"`
+	Delete  *bool `json:"delete,omitempty"`
+	Modify  *bool `json:"modify,omitempty"`
+}
+
+func (m MonitorSelect) MarshalJSON() ([]byte, error) {
+	ms := monitorSelect{
+		Initial: m.initial,
+		Insert:  m.insert,
+		Delete:  m.delete,
+		Modify:  m.modify,
+	}
+	return json.Marshal(ms)
+}
+
+func (m *MonitorSelect) UnmarshalJSON(data []byte) error {
+	var ms monitorSelect
+	err := json.Unmarshal(data, &ms)
+	if err != nil {
+		return err
+	}
+	m.initial = ms.Initial
+	m.insert = ms.Insert
+	m.delete = ms.Delete
+	m.modify = ms.Modify
+	return nil
+}
diff --git a/ovsdb/notation.go b/ovsdb/notation.go
index fc9a051..338339a 100644
--- a/ovsdb/notation.go
+++ b/ovsdb/notation.go
@@ -53,16 +53,8 @@ type MonitorRequests struct {
 
 // MonitorRequest represents a monitor request according to RFC7047
 type MonitorRequest struct {
-	Columns []string      `json:"columns,omitempty"`
-	Select  MonitorSelect `json:"select,omitempty"`
-}
-
-// MonitorSelect represents a monitor select according to RFC7047
-type MonitorSelect struct {
-	Initial bool `json:"initial,omitempty"`
-	Insert  bool `json:"insert,omitempty"`
-	Delete  bool `json:"delete,omitempty"`
-	Modify  bool `json:"modify,omitempty"`
+	Columns []string       `json:"columns,omitempty"`
+	Select  *MonitorSelect `json:"select,omitempty"`
 }
 
 // TableUpdates is a collection of TableUpdate entries


Test patch:
diff --git a/client/ovs_integration_test.go b/client/ovs_integration_test.go
index cb4c2bc..112574d 100644
--- a/client/ovs_integration_test.go
+++ b/client/ovs_integration_test.go
@@ -498,12 +498,8 @@ func TestMonitorCancelIntegration(t *testing.T) {
 	requests := make(map[string]ovsdb.MonitorRequest)
 	requests["Bridge"] = ovsdb.MonitorRequest{
 		Columns: []string{"name"},
-		Select: ovsdb.MonitorSelect{
-			Initial: true,
-			Insert:  true,
-			Delete:  true,
-			Modify:  true,
-		}}
+		Select:  ovsdb.NewDefaultMonitorSelect(),
+	}
 
 	err = ovs.Monitor(monitorID, requests)
 	if err != nil {
diff --git a/ovsdb/monitor_select_test.go b/ovsdb/monitor_select_test.go
new file mode 100644
index 0000000..8a31ba2
--- /dev/null
+++ b/ovsdb/monitor_select_test.go
@@ -0,0 +1,103 @@
+package ovsdb
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestNewMonitorSelect(t *testing.T) {
+	ms := NewMonitorSelect(true, false, true, false)
+	assert.True(t, ms.Initial(), "initial")
+	assert.False(t, ms.Insert(), "insert")
+	assert.True(t, ms.Delete(), "delete")
+	assert.False(t, ms.Modify(), "modify")
+}
+
+func TestNewDefaultMonitorSelect(t *testing.T) {
+	ms := NewDefaultMonitorSelect()
+	assert.True(t, ms.Initial(), "initial")
+	assert.True(t, ms.Insert(), "insert")
+	assert.True(t, ms.Delete(), "delete")
+	assert.True(t, ms.Modify(), "modify")
+}
+
+func TestMonitorSelectInitial(t *testing.T) {
+	tt := true
+	f := false
+	ms1 := MonitorSelect{initial: nil}
+	ms2 := MonitorSelect{initial: &tt}
+	ms3 := MonitorSelect{initial: &f}
+	assert.True(t, ms1.Initial(), "nil")
+	assert.True(t, ms2.Initial(), "true")
+	assert.False(t, ms3.Initial(), "false")
+}
+
+func TestMonitorSelectInsert(t *testing.T) {
+	tt := true
+	f := false
+	ms1 := MonitorSelect{insert: nil}
+	ms2 := MonitorSelect{insert: &tt}
+	ms3 := MonitorSelect{insert: &f}
+	assert.True(t, ms1.Insert(), "nil")
+	assert.True(t, ms2.Insert(), "true")
+	assert.False(t, ms3.Insert(), "false")
+}
+
+func TestMonitorSelectDelete(t *testing.T) {
+	tt := true
+	f := false
+	ms1 := MonitorSelect{delete: nil}
+	ms2 := MonitorSelect{delete: &tt}
+	ms3 := MonitorSelect{delete: &f}
+	assert.True(t, ms1.Delete(), "nil")
+	assert.True(t, ms2.Delete(), "true")
+	assert.False(t, ms3.Delete(), "false")
+}
+
+func TestMonitorSelectModify(t *testing.T) {
+	tt := true
+	f := false
+	ms1 := MonitorSelect{modify: nil}
+	ms2 := MonitorSelect{modify: &tt}
+	ms3 := MonitorSelect{modify: &f}
+	assert.True(t, ms1.Modify(), "nil")
+	assert.True(t, ms2.Modify(), "true")
+	assert.False(t, ms3.Modify(), "false")
+}
+
+func TestMonitorSelectMarshalUnmarshalJSON(t *testing.T) {
+	tests := []struct {
+		name string
+		ms   *MonitorSelect
+		want string
+	}{
+		{
+			"nil",
+			&MonitorSelect{},
+			`{}`,
+		},
+		{
+			"default",
+			NewDefaultMonitorSelect(),
+			`{"delete":true, "initial":true, "insert":true, "modify":true}`,
+		},
+		{
+			"falsey",
+			NewMonitorSelect(false, false, false, false),
+			`{"delete":false, "initial":false, "insert":false, "modify":false}`,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := json.Marshal(tt.ms)
+			assert.Nil(t, err)
+			assert.JSONEq(t, tt.want, string(got))
+			var ms2 MonitorSelect
+			err = json.Unmarshal(got, &ms2)
+			assert.Nil(t, err)
+			assert.Equal(t, tt.ms, &ms2)
+		})
+	}
+}
diff --git a/ovsdb/rpc_test.go b/ovsdb/rpc_test.go
index def3da6..e6547ec 100644
--- a/ovsdb/rpc_test.go
+++ b/ovsdb/rpc_test.go
@@ -53,12 +53,7 @@ func TestNewMonitorArgs(t *testing.T) {
 	value := 1
 	r := MonitorRequest{
 		Columns: []string{"name", "ports", "external_ids"},
-		Select: MonitorSelect{
-			Initial: true,
-			Insert:  true,
-			Delete:  true,
-			Modify:  true,
-		},
+		Select:  NewDefaultMonitorSelect(),
 	}
 	requests := make(map[string]MonitorRequest)
 	requests["Bridge"] = r