Fix remote write merging and duplicate function permissions

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
 		}
 	}
 

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"))},

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{

spine/device_local_write_permissions_test.go

@@ -11,65 +11,65 @@ import (
 // are returned for different write permission scenarios
 func TestWritePermissionErrorMessages(t *testing.T) {
 	_, localEntity := createLocalDeviceAndEntity(1)
-	
+
 	// Create a LoadControl feature
 	localFeature := NewFeatureLocal(
 		localEntity.NextFeatureId(),
 		localEntity,
 		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())
-}
+}
+
+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())
+}

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: