Fix merge of spine data types when messages arrive with unset identifiers

.github/workflows/default.yml

@@ -23,7 +23,7 @@ jobs:
       - name: Set up Go
         uses: actions/setup-go@v5
         with:
-          go-version: ^1.21
+          go-version: ^1.22
 
       - name: Build
         run: go build -v ./...
@@ -32,8 +32,6 @@ jobs:
         uses: golangci/golangci-lint-action@master
         with:
           version: latest
-          skip-pkg-cache: true
-          skip-build-cache: true
           args: --timeout=3m --issues-exit-code=0 ./...
 
       - name: Test

integration_tests/helper_test.go

@@ -154,3 +154,26 @@ func waitForAck(t *testing.T, msgCounterReference *model.MsgCounterType, writeHa
 		}
 	}
 }
+
+// When using waitForNack and waitForAck in the same test ensure that each message sent has a unique message counter
+func waitForNack(t *testing.T, msgCounterReference *model.MsgCounterType, writeHandler *WriteMessageHandler) {
+	var datagram model.Datagram
+
+	msg := writeHandler.ResultWithReference(msgCounterReference)
+	if msg == nil {
+		t.Fatal("acknowledge message was not sent!!")
+	}
+
+	if err := json.Unmarshal(msg, &datagram); err != nil {
+		t.Fatal(err)
+	}
+
+	cmd := datagram.Datagram.Payload.Cmd[0]
+	if cmd.ResultData != nil {
+		if cmd.ResultData.ErrorNumber == nil || uint(*cmd.ResultData.ErrorNumber) == uint(model.ErrorNumberTypeNoError) {
+			t.Fatal("expected error in acknowledgement but received nil/no error")
+		}
+	} else {
+		t.Fatal("expected ResultData with error in acknowledgement but received no ResultData")
+	}
+}

integration_tests/measurement_test.go

@@ -15,6 +15,9 @@ const (
 	m_subscriptionRequestCall_recv_result_file_path = "./testdata/m_subscriptionRequestCall_recv_result.json"
 	m_descriptionListData_recv_reply_file_path      = "./testdata/m_descriptionListData_recv_reply.json"
 	m_measurementListData_recv_notify_file_path     = "./testdata/m_measurementListData_recv_notify.json"
+	m_measurementListData_set_key_path              = "./testdata/m_measurementListData_set_key.json"
+	m_measurementListData_unset_key_path            = "./testdata/m_measurementListData_unset_key.json"
+	m_measurementListData_unset_key_filter_path     = "./testdata/m_measurementListData_unset_key_with_filter.json"
 )
 
 func TestMeasurementSuite(t *testing.T) {
@@ -112,6 +115,38 @@ func (s *MeasurementSuite) TestMeasurementList_Recv() {
 	assert.Equal(s.T(), string(model.MeasurementValueSourceTypeMeasuredValue), string(*item1.ValueSource))
 }
 
+func (s *MeasurementSuite) TestMeasurementUnsetKey() {
+	// Send measurements with everything except MeasurementID set to nil
+	msgCounter, _ := s.remoteDevice.HandleSpineMesssage(loadFileData(s.T(), m_measurementListData_unset_key_path))
+	waitForNack(s.T(), msgCounter, s.writeHandler)
+
+	// Send proper measurements
+	msgCounter, _ = s.remoteDevice.HandleSpineMesssage(loadFileData(s.T(), m_measurementListData_set_key_path))
+	waitForAck(s.T(), msgCounter, s.writeHandler)
+
+	// Try to merge data with unset keys by providing partial filter
+	msgCounter, _ = s.remoteDevice.HandleSpineMesssage(loadFileData(s.T(), m_measurementListData_unset_key_filter_path))
+	waitForNack(s.T(), msgCounter, s.writeHandler)
+
+	remoteDevice := s.sut.RemoteDeviceForSki(s.remoteSki)
+	assert.NotNil(s.T(), remoteDevice)
+
+	mFeature := remoteDevice.FeatureByEntityTypeAndRole(
+		remoteDevice.Entity(spine.NewAddressEntityType([]uint{1, 1})),
+		model.FeatureTypeTypeMeasurement,
+		model.RoleTypeServer)
+	assert.NotNil(s.T(), mFeature)
+
+	fdata := mFeature.DataCopy(model.FunctionTypeMeasurementListData)
+	if !assert.NotNil(s.T(), fdata) {
+		return
+	}
+	data := fdata.(*model.MeasurementListDataType)
+
+	// The 3 unset measurements should have been ignored and not merged => Value should stay unchanged
+	assert.Equal(s.T(), 5.0, data.MeasurementData[0].Value.GetValue())
+}
+
 func (s *MeasurementSuite) TestMeasurementByScope_Recv() {
 	// Act
 	msgCounter, _ := s.remoteDevice.HandleSpineMesssage(loadFileData(s.T(), m_descriptionListData_recv_reply_file_path))

integration_tests/testdata/m_measurementListData_set_key.json

@@ -0,0 +1,68 @@
+{
+  "datagram": {
+    "header": {
+      "specificationVersion": "1.1.1",
+      "addressSource": {
+        "device": "Wallbox",
+        "entity": [1,1],
+        "feature": 11
+      },
+      "addressDestination": {
+        "device": "HEMS",
+        "entity": [1],
+        "feature": 2
+      },
+      "msgCounter": 11,
+      "cmdClassifier": "notify",
+      "ackRequest":true
+    },
+    "payload": {
+      "cmd": [
+        {
+          "function": "measurementListData",
+          "filter": [
+            {
+              "cmdControl":{
+                "partial": {}
+              }
+            }
+          ],
+          "measurementListData": {
+            "measurementData": [
+              {
+                "measurementId": 0,
+                "valueType": "value",
+                "timestamp": "2025-01-09T13:27:50.003Z",
+                "value": {
+                  "number": 5,
+                  "scale": 0
+                },
+                "valueSource": "measuredValue"
+              },
+              {
+                "measurementId": 1,
+                "valueType": "value",
+                "timestamp": "2025-01-09T13:27:50.003Z",
+                "value": {
+                  "number": 1185,
+                  "scale": 0
+                },
+                "valueSource": "measuredValue"
+              },
+              {
+                "measurementId": 2,
+                "valueType": "value",
+                "timestamp": "2025-01-09T13:27:50.003Z",
+                "value": {
+                  "number": 1825,
+                  "scale": 0
+                },
+                "valueSource": "measuredValue"
+              }
+            ]
+          }
+        }
+      ]
+    }
+  }
+}

integration_tests/testdata/m_measurementListData_unset_key.json

@@ -0,0 +1,41 @@
+{
+    "datagram": {
+      "header": {
+        "specificationVersion": "1.1.1",
+        "addressSource": {
+          "device": "Wallbox",
+          "entity": [1,1],
+          "feature": 11
+        },
+        "addressDestination": {
+          "device": "HEMS",
+          "entity": [1],
+          "feature": 2
+        },
+        "msgCounter": 10,
+        "cmdClassifier": "notify",
+        "ackRequest":true
+      },
+      "payload": {
+        "cmd": [
+          {
+            "function": "measurementListData",
+            "measurementListData": {
+              "measurementData": [
+                {
+                  "measurementId": 0
+                },
+                {
+                  "measurementId": 1
+                },
+                {
+                  "measurementId": 3
+                }
+              ]
+            }
+          }
+        ]
+      }
+    }
+  }
+

integration_tests/testdata/m_measurementListData_unset_key_with_filter.json

@@ -0,0 +1,47 @@
+{
+  "datagram": {
+    "header": {
+      "specificationVersion": "1.1.1",
+      "addressSource": {
+        "device": "Wallbox",
+        "entity": [1,1],
+        "feature": 11
+      },
+      "addressDestination": {
+        "device": "HEMS",
+        "entity": [1],
+        "feature": 2
+      },
+      "msgCounter": 12,
+      "cmdClassifier": "notify",
+      "ackRequest":true
+    },
+    "payload": {
+      "cmd": [
+        {
+          "function": "measurementListData",
+          "filter": [
+            {
+              "cmdControl":{
+                "partial": {}
+              }
+            }
+          ],
+          "measurementListData": {
+            "measurementData": [
+              {
+                "measurementId": 0
+              },
+              {
+                "measurementId": 1
+              },
+              {
+                "measurementId": 2
+              }
+            ]
+          }
+        }
+      ]
+    }
+  }
+}

model/update.go

@@ -4,6 +4,7 @@ import (
 	"reflect"
 	"sort"
 
+	"github.com/enbility/ship-go/logging"
 	"github.com/enbility/spine-go/util"
 )
 
@@ -35,6 +36,25 @@ type Updater interface {
 func UpdateList[T any](remoteWrite bool, existingData []T, newData []T, filterPartial, filterDelete *FilterType) ([]T, bool) {
 	success := true
 
+	if !listIsValid(newData) {
+		logging.Log().Debug("incoming list update for type '%s' contains invalid items (some but not all identifiers set), leaving old data unchanged", util.Type[T]().Name())
+		return existingData, false
+	}
+
+	if filterDelete == nil && filterPartial == nil {
+		if len(newData) == 0 {
+			return newData, success
+		}
+		// because no filters are set all items need to have complete identifiers
+		if !listHasAllIdentifiers(newData) {
+			logging.Log().Debug("no filters were set and incoming list update for type '%s' contains items that do not have all identifiers, leaving old data unchanged", util.Type[T]().Name())
+			return existingData, false
+		}
+
+		result := SortData(newData)
+		return result, success
+	}
+
 	// process delete filter (with selectors and elements)
 	if filterDelete != nil {
 		if filterData, err := filterDelete.Data(); err == nil {
@@ -58,29 +78,53 @@ func UpdateList[T any](remoteWrite bool, existingData []T, newData []T, filterPa
 		}
 	}
 
-	// check if items have no identifiers
-	// Currently all fields marked as key are required
-	// TODO: check how to handle if only one identifier is provided
-	if len(newData) > 0 && !HasIdentifiers(newData[0]) {
-		// no identifiers specified --> copy data to all existing items
-		// (see EEBus_SPINE_TS_ProtocolSpecification.pdf, Table 7: Considered cmdOptions combinations for classifier "notify")
-		newData, noErrors := copyToAllData(remoteWrite, existingData, &newData[0])
-		if !noErrors {
-			success = false
-		}
-		return newData, success
+	if len(newData) == 0 {
+		return existingData, success
 	}
 
-	result, noErrors := Merge(remoteWrite, existingData, newData)
-	if !noErrors {
-		success = false
+	updatedData := existingData
+	for _, item := range newData {
+		var noErrors bool
+		if HasNoIdentifiers(item) {
+			// no identifiers specified --> copy data to all existing items
+			// (see EEBus_SPINE_TS_ProtocolSpecification.pdf, Table 7: Considered cmdOptions combinations for classifier "notify")
+			updatedData, noErrors = copyToAllData(remoteWrite, updatedData, &item) // #nosec G601 pointers are dereferenced within each loop iteration via reflection, no aliasing can occur and since go1.22 aliasing doesn't happen in loops regardless
+			if !noErrors {
+				success = false
+			}
+		} else {
+			updatedData, noErrors = Merge(remoteWrite, updatedData, []T{item})
+			if !noErrors {
+				success = false
+			}
+		}
 	}
 
-	result = SortData(result)
+	result := SortData(updatedData)
 
 	return result, success
 }
 
+// Check if every item in list has either all or no identifiers set
+func listIsValid[T any](data []T) bool {
+	for _, item := range data {
+		if !HasAllIdentifiers(item) && !HasNoIdentifiers(item) {
+			return false
+		}
+	}
+	return true
+}
+
+// Check if all items in a list have all of their identifiers
+func listHasAllIdentifiers[T any](data []T) bool {
+	for _, item := range data {
+		if !HasAllIdentifiers(item) {
+			return false
+		}
+	}
+	return true
+}
+
 // return a list of field names that have the eebus tag
 func fieldNamesWithEEBusTag(tag EEBusTag, item any) []string {
 	var result []string
@@ -112,7 +156,29 @@ func fieldNamesWithEEBusTag(tag EEBusTag, item any) []string {
 	return result
 }
 
-func HasIdentifiers(data any) bool {
+// Checks if none of an items identifiers are set if it has any
+func HasNoIdentifiers(data any) bool {
+	keys := fieldNamesWithEEBusTag(EEBusTagKey, data)
+
+	// If item has no fields with tag 'key' then those fields can't be 'missing' or 'unset'
+	if len(keys) == 0 {
+		return false
+	}
+
+	v := reflect.ValueOf(data)
+
+	for _, fieldName := range keys {
+		f := v.FieldByName(fieldName)
+
+		if !f.IsNil() {
+			return false
+		}
+	}
+
+	return true
+}
+
+func HasAllIdentifiers(data any) bool {
 	keys := fieldNamesWithEEBusTag(EEBusTagKey, data)
 
 	v := reflect.ValueOf(data)