diff --git a/.github/workflows/default.yml b/.github/workflows/default.yml index a09e0b3..aeb74c9 100644 --- a/.github/workflows/default.yml +++ b/.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 diff --git a/integration_tests/helper_test.go b/integration_tests/helper_test.go index 095ff6d..98befaa 100644 --- a/integration_tests/helper_test.go +++ b/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") + } +} diff --git a/integration_tests/measurement_test.go b/integration_tests/measurement_test.go index 15401df..5afc4bf 100644 --- a/integration_tests/measurement_test.go +++ b/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)) diff --git a/integration_tests/testdata/m_measurementListData_set_key.json b/integration_tests/testdata/m_measurementListData_set_key.json new file mode 100644 index 0000000..21ad323 --- /dev/null +++ b/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" + } + ] + } + } + ] + } + } +} diff --git a/integration_tests/testdata/m_measurementListData_unset_key.json b/integration_tests/testdata/m_measurementListData_unset_key.json new file mode 100644 index 0000000..eb95412 --- /dev/null +++ b/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 + } + ] + } + } + ] + } + } + } + \ No newline at end of file diff --git a/integration_tests/testdata/m_measurementListData_unset_key_with_filter.json b/integration_tests/testdata/m_measurementListData_unset_key_with_filter.json new file mode 100644 index 0000000..4eab406 --- /dev/null +++ b/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 + } + ] + } + } + ] + } + } +} diff --git a/model/update.go b/model/update.go index b20ab76..dbbe8bd 100644 --- a/model/update.go +++ b/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) diff --git a/model/update_test.go b/model/update_test.go index aea6fe5..c75dd4c 100644 --- a/model/update_test.go +++ b/model/update_test.go @@ -13,19 +13,27 @@ type TestUpdateData struct { DataItem *int } +type TestUpdateDataMultiKey struct { + Id *uint `eebus:"key"` + DataType *string `eebus:"key"` + IsChangeable *bool `eebus:"writecheck"` + DataItem *int +} + type TestUpdater struct { // updateSelectorHashKey *string // deleteSelectorHashKey *string } func TestUpdateList_NewItem(t *testing.T) { + partialFilter := NewFilterTypePartial() existingData := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(1))}} newData := []TestUpdateData{{Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(2))}} expectedResult := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(1))}, {Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(2))}} // Act - result, boolV := UpdateList(false, existingData, newData, nil, nil) + result, boolV := UpdateList(false, existingData, newData, partialFilter, nil) assert.True(t, boolV) assert.Equal(t, expectedResult, result) @@ -33,20 +41,21 @@ func TestUpdateList_NewItem(t *testing.T) { expectedResult = []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(1))}} // Act - result, boolV = UpdateList(true, existingData, newData, nil, nil) + result, boolV = UpdateList(true, existingData, newData, partialFilter, nil) assert.False(t, boolV) assert.Equal(t, expectedResult, result) } func TestUpdateList_ChangedItem(t *testing.T) { + partialFilter := NewFilterTypePartial() existingData := []TestUpdateData{{Id: util.Ptr(uint(1)), IsChangeable: util.Ptr(false), DataItem: util.Ptr(int(1))}} newData := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(2))}} expectedResult := []TestUpdateData{{Id: util.Ptr(uint(1)), IsChangeable: util.Ptr(false), DataItem: util.Ptr(int(2))}} // Act - result, boolV := UpdateList(false, existingData, newData, nil, nil) + result, boolV := UpdateList(false, existingData, newData, partialFilter, nil) assert.True(t, boolV) assert.Equal(t, expectedResult, result) @@ -54,20 +63,21 @@ func TestUpdateList_ChangedItem(t *testing.T) { expectedResult = []TestUpdateData{{Id: util.Ptr(uint(1)), IsChangeable: util.Ptr(false), DataItem: util.Ptr(int(1))}} // Act - result, boolV = UpdateList(true, existingData, newData, nil, nil) + result, boolV = UpdateList(true, existingData, newData, partialFilter, nil) assert.False(t, boolV) assert.Equal(t, expectedResult, result) } func TestUpdateList_NewAndChangedItem(t *testing.T) { + partialFilter := NewFilterTypePartial() existingData := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(1))}} newData := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(2))}, {Id: util.Ptr(uint(3)), DataItem: util.Ptr(int(3))}} expectedResult := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(2))}, {Id: util.Ptr(uint(3)), DataItem: util.Ptr(int(3))}} // Act - result, boolV := UpdateList(false, existingData, newData, nil, nil) + result, boolV := UpdateList(false, existingData, newData, partialFilter, nil) assert.True(t, boolV) assert.Equal(t, expectedResult, result) @@ -75,20 +85,21 @@ func TestUpdateList_NewAndChangedItem(t *testing.T) { expectedResult = []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(1))}} // Act - result, boolV = UpdateList(true, existingData, newData, nil, nil) + result, boolV = UpdateList(true, existingData, newData, partialFilter, nil) assert.False(t, boolV) assert.Equal(t, expectedResult, result) } func TestUpdateList_ItemWithNoIdentifier(t *testing.T) { + partialFilter := NewFilterTypePartial() existingData := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(1))}, {Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(2))}} newData := []TestUpdateData{{DataItem: util.Ptr(int(3))}} expectedResult := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(3))}, {Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(3))}} // Act - result, boolV := UpdateList(false, existingData, newData, nil, nil) + result, boolV := UpdateList(false, existingData, newData, partialFilter, nil) assert.True(t, boolV) assert.Equal(t, expectedResult, result) @@ -96,12 +107,46 @@ func TestUpdateList_ItemWithNoIdentifier(t *testing.T) { expectedResult = []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(3))}, {Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(3))}} // Act - result, boolV = UpdateList(true, existingData, newData, nil, nil) + result, boolV = UpdateList(true, existingData, newData, partialFilter, nil) + + assert.False(t, boolV) + assert.Equal(t, expectedResult, result) +} + +func TestUpdateList_ItemWithSomeIdentifiers(t *testing.T) { + partialFilter := NewFilterTypePartial() + existingData := []TestUpdateDataMultiKey{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(1)), DataType: util.Ptr("testType")}} + newData := []TestUpdateDataMultiKey{{Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(1))}, {Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(2)), DataType: util.Ptr("testType")}} + + expectedResult := existingData + + result, boolV := UpdateList(false, existingData, newData, partialFilter, nil) assert.False(t, boolV) assert.Equal(t, expectedResult, result) } +func TestUpdateList_ItemsWithNoAndAllIdentifiers(t *testing.T) { + partialFilter := NewFilterTypePartial() + existingData := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(1))}} + newData := []TestUpdateData{{DataItem: util.Ptr(int(2))}, {Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(3))}} + + expectedResult := []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(2))}, {Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(3))}} + + result, boolV := UpdateList(false, existingData, newData, partialFilter, nil) + + assert.True(t, boolV) + assert.Equal(t, expectedResult, result) + + newData = []TestUpdateData{{Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(3))}, {DataItem: util.Ptr(int(2))}} + expectedResult = []TestUpdateData{{Id: util.Ptr(uint(1)), DataItem: util.Ptr(int(2))}, {Id: util.Ptr(uint(2)), DataItem: util.Ptr(int(2))}} + + result, boolV = UpdateList(false, existingData, newData, partialFilter, nil) + + assert.True(t, boolV) + assert.Equal(t, expectedResult, result) +} + func TestUpdateList_FilterDelete(t *testing.T) { existingData := []LoadControlLimitDataType{ { diff --git a/spine/function_data.go b/spine/function_data.go index 5b843d4..43edb25 100644 --- a/spine/function_data.go +++ b/spine/function_data.go @@ -56,9 +56,14 @@ func (r *FunctionData[T]) UpdateData(remoteWrite, persist bool, newData *T, filt r.mux.Lock() defer r.mux.Unlock() - if filterPartial == nil && filterDelete == nil && persist { - // just set the data - r.data = newData + // Only non list data is handled here + if filterPartial == nil && filterDelete == nil && persist && !r.SupportsPartialWrite() { + if model.HasAllIdentifiers(newData) { + // just set the data + r.data = newData + return r.data, nil + } + logging.Log().Debug("incoming new data of type '%s' does not have all identifiers set, leaving old data unchanged", util.Type[T]().Name()) return r.data, nil }