From f4ce3bbbe5d6cbc4bbebae5e8f72b342f80e0c02 Mon Sep 17 00:00:00 2001
From: Denis Arh <denis.arh@gmail.com>
Date: Tue, 10 Mar 2020 11:06:21 +0100
Subject: [PATCH] Fix problems with multi-value set/del

---
 compose/service/record.go           |  2 +-
 compose/service/values/sanitizer.go | 13 ++++++++++++-
 compose/types/record_value.go       | 17 +++++++++++++++--
 compose/types/record_value_test.go  | 19 ++++++++++++++++++-
 4 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/compose/service/record.go b/compose/service/record.go
index c83d93c06..203515abc 100644
--- a/compose/service/record.go
+++ b/compose/service/record.go
@@ -459,7 +459,7 @@ func (svc record) Update(upd *types.Record) (rec *types.Record, err error) {
 
 		// Merge new (updated) values with old ones
 		// This way we get list of updated, stale and deleted values
-		// that we can selectively updated in the repository
+		// that we can selectively update in the repository
 		upd.Values = old.Values.Merge(upd.Values)
 
 		if upd.OwnedBy == 0 && old.OwnedBy > 0 {
diff --git a/compose/service/values/sanitizer.go b/compose/service/values/sanitizer.go
index d7d403b50..b94a54787 100644
--- a/compose/service/values/sanitizer.go
+++ b/compose/service/values/sanitizer.go
@@ -33,7 +33,13 @@ func (s sanitizer) Run(m *types.Module, vv types.RecordValueSet) (out types.Reco
 		//
 		// Values are ordered when received so we treat them like it
 		// and assign the appropriate place no.
-		for i, v := range vv.FilterByName(f.Name) {
+		var i = 0
+		for _, v := range vv.FilterByName(f.Name) {
+			if v.IsDeleted() {
+				continue
+			}
+
+			i++
 			out = append(out, &types.RecordValue{
 				Name:  f.Name,
 				Value: v.Value,
@@ -57,6 +63,11 @@ func (s sanitizer) Run(m *types.Module, vv types.RecordValueSet) (out types.Reco
 			continue
 		}
 
+		if v.IsDeleted() || !v.IsUpdated() {
+			// Ignore unchanged and deleted
+			continue
+		}
+
 		kind = strings.ToLower(f.Kind)
 
 		if kind != "string" {
diff --git a/compose/types/record_value.go b/compose/types/record_value.go
index 9b8ea6596..07fc0e068 100644
--- a/compose/types/record_value.go
+++ b/compose/types/record_value.go
@@ -43,6 +43,9 @@ func (set RecordValueSet) FilterByName(name string) (vv RecordValueSet) {
 }
 
 func (set RecordValueSet) FilterByRecordID(recordID uint64) (vv RecordValueSet) {
+	// Make sure we never return nil
+	vv = RecordValueSet{}
+
 	for i := range set {
 		if set[i].RecordID == recordID {
 			vv = append(vv, set[i])
@@ -148,6 +151,7 @@ func (set RecordValueSet) GetClean() (out RecordValueSet) {
 //
 func (set RecordValueSet) Merge(new RecordValueSet) (out RecordValueSet) {
 	if len(set) == 0 {
+		// Empty set, copy all new values and return them
 		for i := range new {
 			new[i].updated = true
 		}
@@ -155,7 +159,7 @@ func (set RecordValueSet) Merge(new RecordValueSet) (out RecordValueSet) {
 		return new
 	}
 
-	out = make([]*RecordValue, 0, len(set)+len(new))
+	out = make([]*RecordValue, 0)
 	for s := range set {
 		// Mark all old as deleted
 		out = append(out, &RecordValue{
@@ -224,8 +228,17 @@ func (set RecordValueSet) Value() (driver.Value, error) {
 // Simple RVS as string output utility fn that
 // can help with debugging
 func (set RecordValueSet) String() (o string) {
-	const tpl = "%-10s %2d %-20s %-20d %-20s %v %v\n"
+	if set == nil {
+		return "<RecordValueSet = nil>"
+	}
+
+	const tpl = "%-10s %2d %-10s %-20d %-10s %v %v\n"
 	for _, v := range set {
+		if v == nil {
+			o += "<RecordValue = nil>\n"
+			continue
+		}
+
 		o += fmt.Sprintf(
 			tpl,
 			v.Name,
diff --git a/compose/types/record_value_test.go b/compose/types/record_value_test.go
index 99071071c..1abcf0958 100644
--- a/compose/types/record_value_test.go
+++ b/compose/types/record_value_test.go
@@ -55,6 +55,12 @@ func TestRecordValueSet_Merge(t *testing.T) {
 			new:  RecordValueSet{{Name: "n", Value: "v"}},
 			want: RecordValueSet{{Name: "n", Value: "v", updated: true}},
 		},
+		{
+			name: "update nil",
+			set:  nil,
+			new:  RecordValueSet{{Name: "n", Value: "v"}},
+			want: RecordValueSet{{Name: "n", Value: "v", oldValue: "", updated: true}},
+		},
 		{
 			name: "update with nil",
 			set:  RecordValueSet{{Name: "n", Value: "v"}},
@@ -73,12 +79,23 @@ func TestRecordValueSet_Merge(t *testing.T) {
 			new:  RecordValueSet{{Name: "n", Value: "2"}},
 			want: RecordValueSet{{Name: "n", Value: "2", oldValue: "1", updated: true}, {Name: "deleted", Value: "d", oldValue: "d", updated: true, DeletedAt: &time.Time{}}},
 		},
+		{
+			name: "update multi value",
+			set:  RecordValueSet{{Name: "c", Value: "1st", Place: 1}, {Name: "c", Value: "2nd", Place: 2}, {Name: "c", Value: "3rd", Place: 3}, {Name: "c", Value: "4th", Place: 4}},
+			new:  RecordValueSet{{Name: "c", Value: "1st", Place: 1}, {Name: "c", Value: "2nd", Place: 2}, {Name: "c", Value: "4th", Place: 3}},
+			want: RecordValueSet{
+				{Name: "c", Value: "1st", Place: 1, oldValue: "1st"},
+				{Name: "c", Value: "2nd", Place: 2, oldValue: "2nd"},
+				{Name: "c", Value: "4th", Place: 3, oldValue: "3rd", updated: true},
+				{Name: "c", Value: "4th", Place: 4, oldValue: "4th", updated: true, DeletedAt: &time.Time{}},
+			},
+		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			if got := tt.set.Merge(tt.new); !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("Update() = %+v, want %+v", got, tt.want)
+				t.Errorf("got:\n%+v\n\nwant\n%+v", got, tt.want)
 			}
 		})
 	}