Repository: vmware/govmomi
Pull request #2700 (2022-01-11 18:36:18)

vcsim: Allow updating custom fields
## Description

The govmomi simulator currently has unexpected behavior when trying to update the custom fields of a VM. Instead of replacing the old value with the new one, it creates a duplicate key with the new value. More details in https://github.com/vmware/govmomi/issues/2687

Closes: https://github.com/vmware/govmomi/issues/2687

I suggest ignoring whitespace when reviewing.

## Type of change

Please mark options that are relevant:

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
- [ ] This change requires a documentation update
- [ ] Build related change

## How Has This Been Tested?

Updated relevant unit test to account for updating custom values.

## Checklist:

- [x] My code follows the CONTRIBUTION [guidelines](./../CONTRIBUTING.md) of
  this project
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation (not needed)
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] Any dependent changes have been merged

vmwclabot: @nilsanderselde, you must sign our contributor license agreement before your changes are merged. Click [here](https://cla.vmware.com/cla/0832d408c7354a70a2a987d20344f678) to sign the agreement. If you are a VMware employee, read [this](https://cla.vmware.com/employee) for further instruction.
vmwclabot: @nilsanderselde, VMware has approved your signed contributor license agreement.
embano1: @nilsanderselde THX! Can you please follow the [`CONTRIBUTION`](https://github.com/vmware/govmomi/blob/master/CONTRIBUTING.md#example-1---fix-a-bug-in-govmomi) guidelines around naming and contents for the `commit`?
nilsanderselde: > @nilsanderselde THX! Can you please follow the [`CONTRIBUTION`](https://github.com/vmware/govmomi/blob/master/CONTRIBUTING.md#example-1---fix-a-bug-in-govmomi) guidelines around naming and contents for the `commit`?

@embano1 Sorry about that, I think it should be good now.

Patch:
diff --git a/simulator/custom_fields_manager.go b/simulator/custom_fields_manager.go
index 049a3ed3..4aac583f 100644
--- a/simulator/custom_fields_manager.go
+++ b/simulator/custom_fields_manager.go
@@ -188,8 +188,30 @@ func (c *CustomFieldsManager) SetField(ctx *Context, req *types.SetField) soap.H
 		Value:            req.Value,
 	}
 
+	removeIndex := func(s []types.BaseCustomFieldValue, i int) []types.BaseCustomFieldValue {
+		new := make([]types.BaseCustomFieldValue, 0)
+		new = append(new, s[:i]...)
+		return append(new, s[i+1:]...)
+	}
+
+	removeExistingValues := func(s []types.BaseCustomFieldValue) []types.BaseCustomFieldValue {
+		for i := 0; i < len(s); {
+			if s[i].GetCustomFieldValue().Key == newValue.GetCustomFieldValue().Key {
+				s = removeIndex(s, i)
+			}
+			i++
+		}
+		return s
+	}
+
 	entity := ctx.Map.Get(req.Entity).(mo.Entity).Entity()
+
 	ctx.WithLock(entity, func() {
+		// Check if custom value and value are already set. If so, remove them.
+		entity.CustomValue = removeExistingValues(entity.CustomValue)
+		entity.Value = removeExistingValues(entity.Value)
+
+		// Add the new value
 		entity.CustomValue = append(entity.CustomValue, newValue)
 		entity.Value = append(entity.Value, newValue)
 	})


Test patch:
diff --git a/simulator/object_test.go b/simulator/object_test.go
index 5ae4bafd..a7758fb8 100644
--- a/simulator/object_test.go
+++ b/simulator/object_test.go
@@ -54,6 +54,7 @@ func TestObjectCustomFields(t *testing.T) {
 
 	fieldName := "testField"
 	fieldValue := "12345"
+	updatedFieldValue := "67890"
 
 	// Test that field is not created
 	err = vmm.SetCustomValue(ctx, fieldName, fieldValue)
@@ -113,37 +114,47 @@ func TestObjectCustomFields(t *testing.T) {
 		t.Fatalf("len(vm.AvailableField) expected 2, got %d", len(vm.AvailableField))
 	}
 
-	// Set field
-	err = vmm.SetCustomValue(ctx, fieldName, fieldValue)
-	if err != nil {
-		t.Fatal(err)
-	}
+	testFieldValues := func(want string) {
+		if len(vm.CustomValue) != 1 {
+			t.Fatalf("len(vm.CustomValue) expected 1, got %d", len(vm.CustomValue))
+		}
 
-	if len(vm.CustomValue) != 1 {
-		t.Fatalf("len(vm.CustomValue) expected 1, got %d", len(vm.CustomValue))
-	}
+		if len(vm.Value) != 1 {
+			t.Fatalf("len(vm.Value) expected 1, got %d", len(vm.Value))
+		}
 
-	if len(vm.Value) != 1 {
-		t.Fatalf("len(vm.Value) expected 1, got %d", len(vm.Value))
-	}
+		if vm.CustomValue[0].(*types.CustomFieldStringValue).Key != field.Key {
+			t.Fatalf("vm.CustomValue[0].Key expected %d, got %d",
+				field.Key, vm.CustomValue[0].(*types.CustomFieldStringValue).Key)
+		}
+		if vm.CustomValue[0].(*types.CustomFieldStringValue).Value != want {
+			t.Fatalf("vm.CustomValue[0].Value expected %s, got %s",
+				want, vm.CustomValue[0].(*types.CustomFieldStringValue).Value)
+		}
 
-	if vm.CustomValue[0].(*types.CustomFieldStringValue).Key != field.Key {
-		t.Fatalf("vm.CustomValue[0].Key expected %d, got %d",
-			field.Key, vm.CustomValue[0].(*types.CustomFieldStringValue).Key)
-	}
-	if vm.CustomValue[0].(*types.CustomFieldStringValue).Value != fieldValue {
-		t.Fatalf("vm.CustomValue[0].Value expected %s, got %s",
-			fieldValue, vm.CustomValue[0].(*types.CustomFieldStringValue).Value)
+		if vm.Value[0].(*types.CustomFieldStringValue).Key != field.Key {
+			t.Fatalf("vm.Value[0].Key expected %d, got %d",
+				field.Key, vm.Value[0].(*types.CustomFieldStringValue).Key)
+		}
+		if vm.Value[0].(*types.CustomFieldStringValue).Value != want {
+			t.Fatalf("vm.Value[0].Value expected %s, got %s",
+				want, vm.Value[0].(*types.CustomFieldStringValue).Value)
+		}
 	}
 
-	if vm.Value[0].(*types.CustomFieldStringValue).Key != field.Key {
-		t.Fatalf("vm.Value[0].Key expected %d, got %d",
-			field.Key, vm.Value[0].(*types.CustomFieldStringValue).Key)
+	// Set field
+	err = vmm.SetCustomValue(ctx, fieldName, fieldValue)
+	if err != nil {
+		t.Fatal(err)
 	}
-	if vm.Value[0].(*types.CustomFieldStringValue).Value != fieldValue {
-		t.Fatalf("vm.Value[0].Value expected %s, got %s",
-			fieldValue, vm.Value[0].(*types.CustomFieldStringValue).Value)
+	testFieldValues(fieldValue)
+
+	// Update field
+	err = vmm.SetCustomValue(ctx, fieldName, updatedFieldValue)
+	if err != nil {
+		t.Fatal(err)
 	}
+	testFieldValues(updatedFieldValue)
 
 	// Rename field
 	newName := field.Name + "_renamed"