From 8f040e9884e8d37364b8f7160ce94e01f38d794c Mon Sep 17 00:00:00 2001 From: Mohamed Eltawel Date: Mon, 1 Jun 2026 09:50:40 +0300 Subject: [PATCH 1/2] Fix remote partial merge write checks --- model/collection_operations.go | 17 +++++--- model/collection_operations_test.go | 26 ++++++++++++ model/deviceconfiguration_additions_test.go | 45 +++++++++++++++++++++ 3 files changed, 82 insertions(+), 6 deletions(-) diff --git a/model/collection_operations.go b/model/collection_operations.go index 5c03f1e..9d01c7b 100644 --- a/model/collection_operations.go +++ b/model/collection_operations.go @@ -153,12 +153,15 @@ func Merge[T any](remoteWrite bool, s1 []T, s2 []T) ([]T, bool) { for _, s1Item := range s1 { s1ItemHash := hashKey(s1Item) s2Item, exist := m2[s1ItemHash] - writeAllowed := writeAllowed(s1Item) - if !writeAllowed && remoteWrite { - success = false - } - // if exists and overwriting is allowed - if exist && (!remoteWrite || writeAllowed) { + + if exist { + if remoteWrite && !writeAllowed(s1Item) { + success = false + result = append(result, s1Item) + m1[s1ItemHash] = s1Item + continue + } + // add values from s1Item that don't exist in s2Item or shouldn't be // set in s2Item updateFields(remoteWrite, s1Item, &s2Item) @@ -179,6 +182,8 @@ func Merge[T any](remoteWrite bool, s1 []T, s2 []T) ([]T, bool) { if !exist && !remoteWrite { // only local updates can append data result = append(result, s2Item) + } else if !exist { + success = false } } diff --git a/model/collection_operations_test.go b/model/collection_operations_test.go index 8db2f4c..9773142 100644 --- a/model/collection_operations_test.go +++ b/model/collection_operations_test.go @@ -116,6 +116,32 @@ func TestUnion_NewAndUpdateDataRemoteWrite(t *testing.T) { } } +func TestUnion_RemoteWriteIgnoresUnchangedItemsWithoutWriteCheckValue(t *testing.T) { + existingData := []testStruct{ + {Id: util.Ptr(uint(0)), Active: util.Ptr(true), Data: util.Ptr(string("data1"))}, + {Id: util.Ptr(uint(1)), Changeable: util.Ptr(true), Active: util.Ptr(false), Data: util.Ptr(string("data2"))}, + } + + newData := []testStruct{ + {Id: util.Ptr(uint(1)), Data: util.Ptr(string("data22"))}, + } + + result, boolV := Merge(true, existingData, newData) + assert.True(t, boolV) + + if assert.Equal(t, 2, len(result)) { + assert.Equal(t, 0, int(*result[0].Id)) + assert.Equal(t, "data1", string(*result[0].Data)) + assert.Equal(t, true, bool(*result[0].Active)) + assert.Nil(t, result[0].Changeable) + + assert.Equal(t, 1, int(*result[1].Id)) + assert.Equal(t, "data22", string(*result[1].Data)) + assert.Equal(t, true, bool(*result[1].Changeable)) + assert.Equal(t, false, bool(*result[1].Active)) + } +} + func TestUnion_InvalidData(t *testing.T) { existingData := []testInvalidStruct{ {Id: util.Ptr(uint(0)), Changeable: util.Ptr("true"), Data: util.Ptr(string("data1"))}, diff --git a/model/deviceconfiguration_additions_test.go b/model/deviceconfiguration_additions_test.go index d07ef59..c5baab1 100644 --- a/model/deviceconfiguration_additions_test.go +++ b/model/deviceconfiguration_additions_test.go @@ -53,6 +53,51 @@ func TestDeviceConfigurationKeyValueListDataType_Update(t *testing.T) { assert.False(t, *item2.Value.Boolean) } +func TestDeviceConfigurationKeyValueListDataType_RemotePartialWriteWithoutSelectorIgnoresUnchangedMissingWriteCheck(t *testing.T) { + sut := DeviceConfigurationKeyValueListDataType{ + DeviceConfigurationKeyValueData: []DeviceConfigurationKeyValueDataType{ + { + KeyId: util.Ptr(DeviceConfigurationKeyIdType(0)), + Value: &DeviceConfigurationKeyValueValueType{ + Boolean: util.Ptr(true), + }, + }, + { + KeyId: util.Ptr(DeviceConfigurationKeyIdType(1)), + IsValueChangeable: util.Ptr(true), + Value: &DeviceConfigurationKeyValueValueType{ + Boolean: util.Ptr(true), + }, + }, + }, + } + + newData := DeviceConfigurationKeyValueListDataType{ + DeviceConfigurationKeyValueData: []DeviceConfigurationKeyValueDataType{ + { + KeyId: util.Ptr(DeviceConfigurationKeyIdType(1)), + Value: &DeviceConfigurationKeyValueValueType{ + Boolean: util.Ptr(false), + }, + }, + }, + } + + _, success := sut.UpdateList(true, true, &newData, NewFilterTypePartial(), nil, util.Ptr(FunctionTypeDeviceConfigurationKeyValueListData)) + assert.True(t, success) + + data := sut.DeviceConfigurationKeyValueData + if assert.Equal(t, 2, len(data)) { + assert.Equal(t, DeviceConfigurationKeyIdType(0), *data[0].KeyId) + assert.Nil(t, data[0].IsValueChangeable) + assert.True(t, *data[0].Value.Boolean) + + assert.Equal(t, DeviceConfigurationKeyIdType(1), *data[1].KeyId) + assert.True(t, *data[1].IsValueChangeable) + assert.False(t, *data[1].Value.Boolean) + } +} + func TestDeviceConfigurationKeyValueDescriptionListDataType_Update(t *testing.T) { sut := DeviceConfigurationKeyValueDescriptionListDataType{ DeviceConfigurationKeyValueDescriptionData: []DeviceConfigurationKeyValueDescriptionDataType{ From 4061b1bd5c95d9ac1e048e11bd98ca07cd7b855b Mon Sep 17 00:00:00 2001 From: Mohamed Eltawel Date: Mon, 1 Jun 2026 10:21:46 +0300 Subject: [PATCH 2/2] Merge duplicate feature function permissions --- spine/device_local_write_permissions_test.go | 78 ++++++++++++++------ spine/feature_local.go | 18 +++-- 2 files changed, 66 insertions(+), 30 deletions(-) diff --git a/spine/device_local_write_permissions_test.go b/spine/device_local_write_permissions_test.go index 0215aca..358cd76 100644 --- a/spine/device_local_write_permissions_test.go +++ b/spine/device_local_write_permissions_test.go @@ -11,7 +11,7 @@ import ( // are returned for different write permission scenarios func TestWritePermissionErrorMessages(t *testing.T) { _, localEntity := createLocalDeviceAndEntity(1) - + // Create a LoadControl feature localFeature := NewFeatureLocal( localEntity.NextFeatureId(), @@ -19,57 +19,57 @@ func TestWritePermissionErrorMessages(t *testing.T) { model.FeatureTypeTypeLoadControl, model.RoleTypeServer, ) - + // Add a function that supports both read and write localFeature.AddFunctionType(model.FunctionTypeLoadControlLimitListData, true, true) - + // Add a function that only supports read (no write) localFeature.AddFunctionType(model.FunctionTypeLoadControlLimitDescriptionListData, true, false) - + localEntity.AddFeature(localFeature) - + // Get operations map operations := localFeature.Operations() - + t.Run("function exists and supports write", func(t *testing.T) { function := model.FunctionTypeLoadControlLimitListData ops, ok := operations[function] - + assert.True(t, ok, "Function should exist in operations") assert.True(t, ops.Write(), "Function should support write") - + // In this case, the check passes and no error is generated }) - + t.Run("function exists but does not support write", func(t *testing.T) { function := model.FunctionTypeLoadControlLimitDescriptionListData ops, ok := operations[function] - + assert.True(t, ok, "Function should exist in operations") assert.False(t, ops.Write(), "Function should NOT support write") - + // Verify the error message that would be generated if ok && !ops.Write() { expectedMsg := "write operation not supported for this function" err := model.NewErrorType(model.ErrorNumberTypeCommandNotSupported, expectedMsg) - + assert.Equal(t, model.ErrorNumberTypeCommandNotSupported, err.ErrorNumber) assert.Equal(t, expectedMsg, string(*err.Description)) } }) - + t.Run("function not found in operations", func(t *testing.T) { // Try a function that doesn't exist in LoadControl feature function := model.FunctionTypeDeviceClassificationManufacturerData _, ok := operations[function] - + assert.False(t, ok, "Function should NOT exist in LoadControl operations") - + // Verify the error message that would be generated if !ok { expectedMsg := "function not found in feature operations" err := model.NewErrorType(model.ErrorNumberTypeCommandNotSupported, expectedMsg) - + assert.Equal(t, model.ErrorNumberTypeCommandNotSupported, err.ErrorNumber) assert.Equal(t, expectedMsg, string(*err.Description)) } @@ -79,33 +79,65 @@ func TestWritePermissionErrorMessages(t *testing.T) { // TestFeatureLocal_AddFunctionType verifies AddFunctionType works correctly func TestFeatureLocal_AddFunctionType(t *testing.T) { _, localEntity := createLocalDeviceAndEntity(1) - + localFeature := NewFeatureLocal( localEntity.NextFeatureId(), localEntity, model.FeatureTypeTypeMeasurement, model.RoleTypeServer, ) - + // Test adding functions with different permissions localFeature.AddFunctionType(model.FunctionTypeMeasurementListData, true, true) localFeature.AddFunctionType(model.FunctionTypeMeasurementDescriptionListData, true, false) localFeature.AddFunctionType(model.FunctionTypeMeasurementConstraintsListData, false, false) - + ops := localFeature.Operations() - + // Verify read+write function assert.NotNil(t, ops[model.FunctionTypeMeasurementListData]) assert.True(t, ops[model.FunctionTypeMeasurementListData].Read()) assert.True(t, ops[model.FunctionTypeMeasurementListData].Write()) - + // Verify read-only function assert.NotNil(t, ops[model.FunctionTypeMeasurementDescriptionListData]) assert.True(t, ops[model.FunctionTypeMeasurementDescriptionListData].Read()) assert.False(t, ops[model.FunctionTypeMeasurementDescriptionListData].Write()) - + // Verify no-access function (unusual but possible) assert.NotNil(t, ops[model.FunctionTypeMeasurementConstraintsListData]) assert.False(t, ops[model.FunctionTypeMeasurementConstraintsListData].Read()) assert.False(t, ops[model.FunctionTypeMeasurementConstraintsListData].Write()) -} \ No newline at end of file +} + +func TestFeatureLocal_AddFunctionType_MergesDuplicatePermissions(t *testing.T) { + _, localEntity := createLocalDeviceAndEntity(1) + + localFeature := NewFeatureLocal( + localEntity.NextFeatureId(), + localEntity, + model.FeatureTypeTypeMeasurement, + model.RoleTypeServer, + ) + + function := model.FunctionTypeMeasurementListData + + localFeature.AddFunctionType(function, false, false) + ops := localFeature.Operations()[function] + assert.False(t, ops.Read()) + assert.False(t, ops.Write()) + + localFeature.AddFunctionType(function, true, true) + ops = localFeature.Operations()[function] + assert.True(t, ops.Read()) + assert.True(t, ops.Write()) + assert.False(t, ops.ReadPartial()) + assert.True(t, ops.WritePartial()) + + localFeature.AddFunctionType(function, false, false) + ops = localFeature.Operations()[function] + assert.True(t, ops.Read()) + assert.True(t, ops.Write()) + assert.False(t, ops.ReadPartial()) + assert.True(t, ops.WritePartial()) +} diff --git a/spine/feature_local.go b/spine/feature_local.go index d3f9d2c..0e6ce56 100644 --- a/spine/feature_local.go +++ b/spine/feature_local.go @@ -71,18 +71,22 @@ func (r *FeatureLocal) AddFunctionType(function model.FunctionType, read, write if r.role != model.RoleTypeServer && r.role != model.RoleTypeSpecial { return } - if r.operations[function] != nil { - return - } + writePartial := false + if existing := r.operations[function]; existing != nil { + read = existing.Read() || read + write = existing.Write() || write + writePartial = existing.WritePartial() + } + if write { // partials are not supported on all features and functions, so check if this function supports it if fctData := r.functionData(function); fctData != nil { - writePartial = fctData.SupportsPartialWrite() + writePartial = writePartial || fctData.SupportsPartialWrite() } } // Partial reads are intentionally not supported (spec-compliant design decision) - // SPINE specification section 5.3.4.5 states: "A server MAY ignore unsupported cmdOption + // SPINE specification section 5.3.4.5 states: "A server MAY ignore unsupported cmdOption // combinations and then replies with more than the requested parts instead." // By setting readPartial to false, we ensure all read requests return full data, // which provides the safest interoperability behavior for multi-vendor scenarios. @@ -761,11 +765,11 @@ func (r *FeatureLocal) processRead(function model.FunctionType, requestHeader *m } // SPEC-COMPLIANT BEHAVIOR: Partial filters are intentionally ignored - // + // // The incoming message may contain FilterPartial with element selectors, // selectors, or other cmdOptions, but we always reply with full data. // This implements SPINE specification section 5.3.4.5: - // "A server MAY ignore unsupported cmdOption combinations and then replies + // "A server MAY ignore unsupported cmdOption combinations and then replies // with more than the requested parts instead." // // Benefits of this approach: