Updating UpdateList to update the values on a list (#3899)

* Updating UpdateList to update the values on a list

* Updating PR - Need to verify unit test issues. just looking for feedback.

* Using worker queue to update lists

* Code clean-up

* fix comment spacing

* Adding comment to UpdateList test function

* adding a mutex lock to update list

* move the mutex and change from RLock to Lock

---------

Co-authored-by: Zach Loafman <[email protected]>

pkg/sdkserver/sdkserver.go

@@ -19,10 +19,12 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"slices"
 	"strings"
 	"sync"
 	"time"
 
+	"github.com/mennanov/fmutils"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
@@ -1066,31 +1068,79 @@ func (s *SDKServer) UpdateList(ctx context.Context, in *beta.UpdateListRequest)
 	if in == nil {
 		return nil, errors.Errorf("UpdateListRequest cannot be nil")
 	}
+	if in.List == nil || in.UpdateMask == nil {
+		return nil, errors.Errorf("invalid argument. List: %v and UpdateMask %v cannot be nil", in.List, in.UpdateMask)
+	}
+	if !in.UpdateMask.IsValid(in.List.ProtoReflect().Interface()) {
+		return nil, errors.Errorf("invalid argument. Field Mask Path(s): %v are invalid for List. Use valid field name(s): %v", in.UpdateMask.GetPaths(), in.List.ProtoReflect().Descriptor().Fields())
+	}
 
-	name := in.List.Name
-	s.logger.WithField("name", name).Debug("Update List -- Currently only used for Updating Capacity")
+	if in.List.Capacity < 0 || in.List.Capacity > apiserver.ListMaxCapacity {
+		return nil, errors.Errorf("out of range. Capacity must be within range [0,1000]. Found Capacity: %d", in.List.Capacity)
+	}
 
-	gs, err := s.gameServer()
+	list, err := s.GetList(ctx, &beta.GetListRequest{Name: in.List.Name})
 	if err != nil {
-		return nil, err
+
+		return nil, errors.Errorf("not found. %s List not found", list.Name)
 	}
 
 	s.gsUpdateMutex.Lock()
 	defer s.gsUpdateMutex.Unlock()
 
-	if in.List.Capacity < 0 || in.List.Capacity > apiserver.ListMaxCapacity {
-		return nil, errors.Errorf("out of range. Capacity must be within range [0,1000]. Found Capacity: %d", in.List.Capacity)
-	}
+	// Removes any fields from the request object that are not included in the FieldMask Paths.
+	fmutils.Filter(in.List, in.UpdateMask.Paths)
+
+	// The list will allow the current list to be overwritten
+	batchList := listUpdateRequest{}
 
-	if _, ok := gs.Status.Lists[name]; ok {
-		batchList := s.gsListUpdates[name]
+	// Only set the capacity if its included in the update mask paths
+	if slices.Contains(in.UpdateMask.Paths, "capacity") {
 		batchList.capacitySet = &in.List.Capacity
-		s.gsListUpdates[name] = batchList
-		// Queue up the Update for later batch processing by updateLists.
-		s.workerqueue.Enqueue(cache.ExplicitKey(updateLists))
-		return &beta.List{}, nil
 	}
-	return nil, errors.Errorf("not found. %s List not found", name)
+
+	// Only change the values if its included in the update mask paths
+	if slices.Contains(in.UpdateMask.Paths, "values") {
+		currList := list
+
+		// Find values to remove from the current list
+		valuesToDelete := map[string]bool{}
+		for _, value := range currList.Values {
+			valueFound := false
+			for _, element := range in.List.Values {
+				if value == element {
+					valueFound = true
+				}
+			}
+
+			if !valueFound {
+				valuesToDelete[value] = true
+			}
+		}
+		batchList.valuesToDelete = valuesToDelete
+
+		// Find values that need to be added to the current list from the incomming list
+		valuesToAdd := []string{}
+		for _, value := range in.List.Values {
+			valueFound := false
+			for _, element := range currList.Values {
+				if value == element {
+					valueFound = true
+				}
+			}
+
+			if !valueFound {
+				valuesToAdd = append(valuesToAdd, value)
+			}
+		}
+		batchList.valuesToAppend = valuesToAdd
+	}
+
+	// Queue up the Update for later batch processing by updateLists.
+	s.gsListUpdates[list.Name] = batchList
+	s.workerqueue.Enqueue(cache.ExplicitKey(updateLists))
+	return &beta.List{}, nil
+
 }
 
 // AddListValue collapses all append a value to the end of a List requests into a single UpdateList request.

pkg/sdkserver/sdkserver_test.go

@@ -1674,9 +1674,11 @@ func TestSDKServerUpdateList(t *testing.T) {
 	require.NoError(t, err, "Can not parse FeatureCountsAndLists feature")
 
 	lists := map[string]agonesv1.ListStatus{
-		"foo": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
-		"bar": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
-		"baz": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
+		"foo":  {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
+		"bar":  {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
+		"baz":  {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
+		"qux":  {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
+		"quux": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)},
 	}
 
 	fixtures := map[string]struct {
@@ -1729,6 +1731,37 @@ func TestSDKServerUpdateList(t *testing.T) {
 			updated:                 false,
 			expectedUpdatesQueueLen: 0,
 		},
+		// New test cases to test updating values
+		"update values below capacity": {
+			listName: "qux",
+			request: &beta.UpdateListRequest{
+				List: &beta.List{
+					Name:     "qux",
+					Capacity: int64(100),
+					Values:   []string{"one", "two", "three", "four", "five", "six"},
+				},
+				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"capacity", "values"}},
+			},
+			want:                    agonesv1.ListStatus{Values: []string{"one", "two", "three", "four", "five", "six"}, Capacity: int64(100)},
+			updateErr:               false,
+			updated:                 true,
+			expectedUpdatesQueueLen: 0,
+		},
+		"update values above capacity": {
+			listName: "quux",
+			request: &beta.UpdateListRequest{
+				List: &beta.List{
+					Name:     "quux",
+					Capacity: int64(4),
+					Values:   []string{"one", "two", "three", "four", "five", "six"},
+				},
+				UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"capacity", "values"}},
+			},
+			want:                    agonesv1.ListStatus{Values: []string{"one", "two", "three", "four"}, Capacity: int64(4)},
+			updateErr:               false,
+			updated:                 true,
+			expectedUpdatesQueueLen: 0,
+		},
 	}
 
 	// nolint:dupl  // Linter errors on lines are duplicate of TestSDKServerAddListValue, TestSDKServerRemoveListValue

site/content/en/docs/Guides/Client SDKs/rest.md

@@ -296,35 +296,17 @@ Response:
 ```
 
 ##### Beta: UpdateList
-This function updates the list's properties with the key `players`, such as its capacity and values, and returns the updated list details.
+This function updates the list's properties with the key `players`, such as its capacity and values, and returns the updated list details. Use `AddListValue` or `RemoveListValue` for modifying the `List.Values` field.
 
-**Note:** The behavior of this function differs between the Local SDK Server and the SDK Server.
-
-- **Local SDK Server:** This will overwrite both the capacity and the values with the update request values. Use `AddListValue` or `RemoveListValue` for modifying the `List.Values` field.
-- **SDK Server:** This will only update the capacity of the list and does not modify the values.
 
 ###### Example
-**Local SDK Server Example:**
-
 ```bash
 curl -d '{"capacity": "120", "values": ["player3", "player4"]}' -H "Content-Type: application/json" -X PATCH http://localhost:${AGONES_SDK_HTTP_PORT}/v1beta1/lists/players
 ```
-
 Response:
 ```json
 {"name":"players", "capacity":"120", "values":["player3", "player4"]}
 ```
-**SDK Server Example:**
-
-```bash
-curl -d '{"capacity": "120"}' -H "Content-Type: application/json" -X PATCH http://localhost:${AGONES_SDK_HTTP_PORT}/v1beta1/lists/players
-```
-
-Response:
-```json
-{"name":"players", "capacity":"120", "values":["player0", "player1", "player2"]}
-```
-
 ##### Beta: AddListValue
 This function adds a new value to a list with the key `players` and returns the list with this addition.