diff --git a/pkg/sdkserver/sdkserver.go b/pkg/sdkserver/sdkserver.go index 40f1bea133..a8cdd87c0a 100644 --- a/pkg/sdkserver/sdkserver.go +++ b/pkg/sdkserver/sdkserver.go @@ -1028,6 +1028,7 @@ func (s *SDKServer) GetList(ctx context.Context, in *alpha.GetListRequest) (*alp if len(listUpdate.valuesToAppend) != 0 { protoList.Values = agonesv1.MergeRemoveDuplicates(protoList.Values, listUpdate.valuesToAppend) } + // Truncates Values to less than or equal to Capacity if len(protoList.Values) > int(protoList.Capacity) { protoList.Values = append([]string{}, protoList.Values[:protoList.Capacity]...) } @@ -1047,9 +1048,8 @@ func (s *SDKServer) UpdateList(ctx context.Context, in *alpha.UpdateListRequest) if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) { return nil, errors.Errorf("%s not enabled", runtime.FeatureCountsAndLists) } - - 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 == nil { + return nil, errors.Errorf("UpdateListRequest cannot be nil") } name := in.List.Name @@ -1089,9 +1089,12 @@ func (s *SDKServer) AddListValue(ctx context.Context, in *alpha.AddListValueRequ if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) { return nil, errors.Errorf("%s not enabled", runtime.FeatureCountsAndLists) } + if in == nil { + return nil, errors.Errorf("AddListValueRequest cannot be nil") + } s.logger.WithField("name", in.Name).Debug("Add List Value") - gs, err := s.gameServer() + list, err := s.GetList(ctx, &alpha.GetListRequest{Name: in.Name}) if err != nil { return nil, err } @@ -1099,40 +1102,23 @@ func (s *SDKServer) AddListValue(ctx context.Context, in *alpha.AddListValueRequ s.gsUpdateMutex.Lock() defer s.gsUpdateMutex.Unlock() - if list, ok := gs.Status.Lists[in.Name]; ok { - batchList := s.gsListUpdates[in.Name] - // Verify room to add another value - var capacity int64 - if batchList.capacitySet != nil { - capacity = *batchList.capacitySet - } else { - capacity = int64(len(list.Values) + len(batchList.valuesToAppend) - len(batchList.valuesToDelete)) - } - if list.Capacity <= capacity { - return nil, errors.Errorf("out of range. No available capacity. Current Capacity: %d, List Size: %d", list.Capacity, len(list.Values)) - } - // Verify value does not already exist in the list - // TODO: This does not check batched but not yet applied append / remove values. Should we do this? - // (Easy to check not yet applied values, hard to check removed and re-added values.) I'm - // thinking this would be better / easier to do as part of the batch apply update to list, and - // not verify here. - for _, val := range list.Values { - if in.Value == val { - return nil, errors.Errorf("already exists. Value: %s already in List: %s", in.Value, in.Name) - } - } - for _, val := range batchList.valuesToAppend { - if in.Value == val { - return nil, errors.Errorf("already exists. Already received request to remove Value: %s from the List: %s", in.Value, in.Name) - } + // Verify room to add another value + if int(list.Capacity) <= len(list.Values) { + return nil, errors.Errorf("out of range. No available capacity. Current Capacity: %d, List Size: %d", list.Capacity, len(list.Values)) + } + // Verify value does not already exist in the list + for _, val := range list.Values { + if in.Value == val { + return nil, errors.Errorf("already exists. Value: %s already in List: %s", in.Value, in.Name) } - batchList.valuesToAppend = append(batchList.valuesToAppend, in.Value) - s.gsListUpdates[in.Name] = batchList - // Queue up the Update for later batch processing by updateLists. - s.workerqueue.Enqueue(cache.ExplicitKey(updateLists)) - return &alpha.List{}, nil } - return nil, errors.Errorf("not found. %s List not found", in.Name) + list.Values = append(list.Values, in.Value) + batchList := s.gsListUpdates[in.Name] + batchList.valuesToAppend = list.Values + s.gsListUpdates[in.Name] = batchList + // Queue up the Update for later batch processing by updateLists. + s.workerqueue.Enqueue(cache.ExplicitKey(updateLists)) + return &alpha.List{}, nil } // RemoveListValue collapses all remove a value from a List requests into a single UpdateList request. @@ -1144,10 +1130,12 @@ func (s *SDKServer) RemoveListValue(ctx context.Context, in *alpha.RemoveListVal if !runtime.FeatureEnabled(runtime.FeatureCountsAndLists) { return nil, errors.Errorf("%s not enabled", runtime.FeatureCountsAndLists) } - + if in == nil { + return nil, errors.Errorf("RemoveListValueRequest cannot be nil") + } s.logger.WithField("name", in.Name).Debug("Remove List Value") - gs, err := s.gameServer() + list, err := s.GetList(ctx, &alpha.GetListRequest{Name: in.Name}) if err != nil { return nil, err } @@ -1155,26 +1143,23 @@ func (s *SDKServer) RemoveListValue(ctx context.Context, in *alpha.RemoveListVal s.gsUpdateMutex.Lock() defer s.gsUpdateMutex.Unlock() - if list, ok := gs.Status.Lists[in.Name]; ok { - // Verify value exists in the list - for _, val := range list.Values { - if in.Value != val { - continue - } - // Add value to remove to gsListUpdates map. - batchList := s.gsListUpdates[in.Name] - if batchList.valuesToDelete == nil { - batchList.valuesToDelete = map[string]bool{} - } - batchList.valuesToDelete[in.Value] = true - s.gsListUpdates[in.Name] = batchList - // Queue up the Update for later batch processing by updateLists. - s.workerqueue.Enqueue(cache.ExplicitKey(updateLists)) - return &alpha.List{}, nil + // Verify value exists in the list + for _, val := range list.Values { + if in.Value != val { + continue } - return nil, errors.Errorf("not found. Value: %s not found in List: %s", in.Value, in.Name) + // Add value to remove to gsListUpdates map. + batchList := s.gsListUpdates[in.Name] + if batchList.valuesToDelete == nil { + batchList.valuesToDelete = map[string]bool{} + } + batchList.valuesToDelete[in.Value] = true + s.gsListUpdates[in.Name] = batchList + // Queue up the Update for later batch processing by updateLists. + s.workerqueue.Enqueue(cache.ExplicitKey(updateLists)) + return &alpha.List{}, nil } - return nil, errors.Errorf("not found. %s List not found", in.Name) + return nil, errors.Errorf("not found. Value: %s not found in List: %s", in.Value, in.Name) } // updateList updates the Lists in the GameServer's Status with the batched update list requests. diff --git a/pkg/sdkserver/sdkserver_test.go b/pkg/sdkserver/sdkserver_test.go index 3f7fcd8673..31953aa1e8 100644 --- a/pkg/sdkserver/sdkserver_test.go +++ b/pkg/sdkserver/sdkserver_test.go @@ -1331,22 +1331,54 @@ func TestSDKServerAddListValue(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)}, + "foo": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(10)}, + "bar": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(10)}, + "baz": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(10)}, } fixtures := map[string]struct { - listName string - request *alpha.AddListValueRequest - want agonesv1.ListStatus - updateErr bool - updated bool + listName string + requests []*alpha.AddListValueRequest + want agonesv1.ListStatus + updateErrs []bool + updated bool }{ "Add value": { - listName: "foo", - request: &alpha.AddListValueRequest{Name: "foo", Value: "five"}, - want: agonesv1.ListStatus{Values: []string{"one", "two", "three", "four", "five"}, Capacity: int64(100)}, - updateErr: false, - updated: true, + listName: "foo", + requests: []*alpha.AddListValueRequest{{Name: "foo", Value: "five"}}, + want: agonesv1.ListStatus{Values: []string{"one", "two", "three", "four", "five"}, Capacity: int64(10)}, + updateErrs: []bool{false}, + updated: true, + }, + "Add multiple values including duplicates": { + listName: "bar", + requests: []*alpha.AddListValueRequest{ + {Name: "bar", Value: "five"}, + {Name: "bar", Value: "one"}, + {Name: "bar", Value: "five"}, + {Name: "bar", Value: "zero"}, + }, + want: agonesv1.ListStatus{Values: []string{"one", "two", "three", "four", "five", "zero"}, Capacity: int64(10)}, + updateErrs: []bool{false, true, true, false}, + updated: true, + }, + "Add multiple values past capacity": { + listName: "baz", + requests: []*alpha.AddListValueRequest{ + {Name: "baz", Value: "five"}, + {Name: "baz", Value: "six"}, + {Name: "baz", Value: "seven"}, + {Name: "baz", Value: "eight"}, + {Name: "baz", Value: "nine"}, + {Name: "baz", Value: "ten"}, + {Name: "baz", Value: "eleven"}, + }, + want: agonesv1.ListStatus{ + Values: []string{"one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"}, + Capacity: int64(10), + }, + updateErrs: []bool{false, false, false, false, false, false, true}, + updated: true, }, } @@ -1401,15 +1433,17 @@ func TestSDKServerAddListValue(t *testing.T) { // check initial value comes through require.Eventually(t, func() bool { list, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName}) - return cmp.Equal(list.Values, []string{"one", "two", "three", "four"}) && list.Capacity == 100 && err == nil + return cmp.Equal(list.Values, []string{"one", "two", "three", "four"}) && list.Capacity == 10 && err == nil }, 10*time.Second, time.Second) // Update the List - _, err = sc.AddListValue(context.Background(), testCase.request) - if testCase.updateErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) + for i, req := range testCase.requests { + _, err = sc.AddListValue(context.Background(), req) + if testCase.updateErrs[i] { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } } got, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName}) @@ -1446,21 +1480,46 @@ func TestSDKServerRemoveListValue(t *testing.T) { 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)}, } fixtures := map[string]struct { - listName string - request *alpha.RemoveListValueRequest - want agonesv1.ListStatus - updateErr bool - updated bool + listName string + requests []*alpha.RemoveListValueRequest + want agonesv1.ListStatus + updateErrs []bool + updated bool }{ "Remove value": { - listName: "foo", - request: &alpha.RemoveListValueRequest{Name: "foo", Value: "two"}, - want: agonesv1.ListStatus{Values: []string{"one", "three", "four"}, Capacity: int64(100)}, - updateErr: false, - updated: true, + listName: "foo", + requests: []*alpha.RemoveListValueRequest{{Name: "foo", Value: "two"}}, + want: agonesv1.ListStatus{Values: []string{"one", "three", "four"}, Capacity: int64(100)}, + updateErrs: []bool{false}, + updated: true, + }, + "Remove multiple values including duplicates": { + listName: "bar", + requests: []*alpha.RemoveListValueRequest{ + {Name: "bar", Value: "two"}, + {Name: "bar", Value: "three"}, + {Name: "bar", Value: "two"}, + }, + want: agonesv1.ListStatus{Values: []string{"one", "four"}, Capacity: int64(100)}, + updateErrs: []bool{false, false, true}, + updated: true, + }, + "Remove all values": { + listName: "baz", + requests: []*alpha.RemoveListValueRequest{ + {Name: "baz", Value: "three"}, + {Name: "baz", Value: "two"}, + {Name: "baz", Value: "four"}, + {Name: "baz", Value: "one"}, + }, + want: agonesv1.ListStatus{Values: []string{}, Capacity: int64(100)}, + updateErrs: []bool{false, false, false, false}, + updated: true, }, } @@ -1519,11 +1578,13 @@ func TestSDKServerRemoveListValue(t *testing.T) { }, 10*time.Second, time.Second) // Update the List - _, err = sc.RemoveListValue(context.Background(), testCase.request) - if testCase.updateErr { - assert.Error(t, err) - } else { - assert.NoError(t, err) + for i, req := range testCase.requests { + _, err = sc.RemoveListValue(context.Background(), req) + if testCase.updateErrs[i] { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } } got, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName})