Repository: algolia/algoliasearch-client-go
Pull request #622 (2020-08-13 13:15:40)

Batch
### Summary

I initially suspected that `.SaveObjects()` calls with a size higher than `search.DefaultMaxBatchSize` would result in the last batch being sent with an extra `nil` record, I've started to add to test to confirm this. However, after writing the test and investigating the original code, I've noticed that the extra `nil` record was actually never sent because it was not reaching the actual batch sending, so everything was fine in the first place. To avoid confusion in the future, this PR keeps the added test and refactor a bit the logic of the automatic batching to make it clearer that no extra `nil` record is sent.

### Test plan

All original indexing integration tests plus the new one I've added to ensure variable batch sizes. I've also added a new unit test for the new function `shouldSendBatch()`, which helps clarifies the intent in the automatic batching code.


Patch:
diff --git a/.circleci/config.yml b/.circleci/config.yml
index bf3c91b3..38f1e1fa 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -101,6 +101,20 @@ jobs:
           key: go-cache-1-14
           paths:
             - "~/go/pkg"
+  test-1-15:
+    docker:
+      - image: circleci/golang:1.15
+    steps:
+      - checkout
+      - restore_cache:
+          key: go-cache-1-15
+      - credentials
+      - test:
+          go_version: "1.15"
+      - save_cache:
+          key: go-cache-1-15
+          paths:
+            - "~/go/pkg"
 
 workflows:
   version: 2
@@ -111,3 +125,4 @@ workflows:
       - test-1-12
       - test-1-13
       - test-1-14
+      - test-1-15
diff --git a/algolia/search/client.go b/algolia/search/client.go
index 106b4a87..14e08842 100644
--- a/algolia/search/client.go
+++ b/algolia/search/client.go
@@ -54,7 +54,7 @@ func NewClientWithConfig(config Configuration) *Client {
 		}
 	}
 
-	if config.MaxBatchSize == 0 {
+	if config.MaxBatchSize <= 0 {
 		maxBatchSize = DefaultMaxBatchSize
 	} else {
 		maxBatchSize = config.MaxBatchSize
diff --git a/algolia/search/index_objects.go b/algolia/search/index_objects.go
index 02a06edf..fc0303ed 100644
--- a/algolia/search/index_objects.go
+++ b/algolia/search/index_objects.go
@@ -171,7 +171,7 @@ func (i *Index) batch(objects interface{}, action BatchAction, opts ...interface
 			return multipleRes, fmt.Errorf("missing objectID in object %#v", object)
 		}
 
-		if len(batch) >= i.maxBatchSize || object == nil {
+		if shouldSendBatch(i.maxBatchSize, batch, object) {
 			operations, err = newOperationBatch(batch, action)
 			if err != nil {
 				return multipleRes, fmt.Errorf("could not generate intermediate batch: %v", err)
@@ -181,19 +181,26 @@ func (i *Index) batch(objects interface{}, action BatchAction, opts ...interface
 				return multipleRes, fmt.Errorf("could not send intermediate batch: %v", err)
 			}
 			multipleRes.Responses = append(multipleRes.Responses, singleRes)
-			batch = []interface{}{object}
-		} else {
-			batch = append(batch, object)
+			batch = nil
 		}
 
 		if object == nil {
 			break
+		} else {
+			batch = append(batch, object)
 		}
 	}
 
 	return multipleRes, nil
 }
 
+func shouldSendBatch(maxBatchSize int, batch []interface{}, object interface{}) bool {
+	isMaxBatchSizePositive := maxBatchSize > 0
+	isBatchBigEnough := len(batch) >= maxBatchSize
+	isLastBatch := object == nil
+	return isMaxBatchSizePositive && (isBatchBigEnough || isLastBatch)
+}
+
 // Batch sends all the given indexing operations with a single call.
 func (i *Index) Batch(operations []BatchOperation, opts ...interface{}) (res BatchRes, err error) {
 	body := batchReq{Requests: operations}


Test patch:
diff --git a/algolia/search/index_objects_test.go b/algolia/search/index_objects_test.go
new file mode 100644
index 00000000..d73bfb83
--- /dev/null
+++ b/algolia/search/index_objects_test.go
@@ -0,0 +1,62 @@
+package search
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestShouldSendBatch(t *testing.T) {
+	type Object struct{}
+
+	for _, c := range []struct {
+		name         string
+		maxBatchSize int
+		batch        []interface{}
+		object       interface{}
+		expected     bool
+	}{
+		{
+			"zero values",
+			0,
+			[]interface{}{},
+			Object{},
+			false,
+		},
+		{
+			"batch not big enough with remaining objects",
+			1,
+			[]interface{}{},
+			Object{},
+			false,
+		},
+		{
+			"batch big enough with remaining objects",
+			1,
+			[]interface{}{Object{}},
+			Object{},
+			true,
+		},
+		{
+			"batch not big enough without remaining objects",
+			2,
+			[]interface{}{Object{}},
+			nil,
+			true,
+		},
+		{
+			"batch big enough without remaining objects",
+			2,
+			[]interface{}{Object{}, Object{}},
+			nil,
+			true,
+		},
+	} {
+		require.Equal(
+			t,
+			c.expected,
+			shouldSendBatch(c.maxBatchSize, c.batch, c.object),
+			"%q test case failed", c.name,
+		)
+	}
+}
diff --git a/cts/extra/batch_sizes_test.go b/cts/extra/batch_sizes_test.go
new file mode 100644
index 00000000..c8443c2d
--- /dev/null
+++ b/cts/extra/batch_sizes_test.go
@@ -0,0 +1,81 @@
+package extra
+
+import (
+	"fmt"
+	"math"
+	"strconv"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/algolia/algoliasearch-client-go/v3/algolia/search"
+	"github.com/algolia/algoliasearch-client-go/v3/cts"
+)
+
+func TestBatchSizes(t *testing.T) {
+	initIndexWithMaxBatchSize := func(maxBatchSize int) *search.Index {
+		appID, apiKey := cts.GetTestingCredentials(t, "ALGOLIA_APPLICATION_ID_1", "ALGOLIA_ADMIN_KEY_1")
+		indexName := cts.GenerateIndexName(t)
+		client := search.NewClientWithConfig(search.Configuration{
+			AppID:        appID,
+			APIKey:       apiKey,
+			MaxBatchSize: maxBatchSize,
+		})
+		return client.InitIndex(indexName)
+	}
+
+	for _, c := range []struct {
+		batchSize    int
+		maxBatchSize int
+	}{
+		{0, 1000},
+		{1, 1000},
+		{10, 1000},
+		{999, 1000},
+		{1000, 1000},
+		{1001, 1000},
+		{1999, 1000},
+		{2000, 1000},
+		{2001, 1000},
+		{100, 9},
+	} {
+		var (
+			expectedNbBatches        int
+			expectedRecordsPerBatch  int
+			expectedRecordsLastBatch int
+			batch                    []map[string]string
+		)
+
+		if c.batchSize <= c.maxBatchSize {
+			expectedNbBatches = 1
+			expectedRecordsPerBatch = 0
+			expectedRecordsLastBatch = c.batchSize
+		} else {
+			expectedNbBatches = int(math.Ceil(float64(c.batchSize) / float64(c.maxBatchSize)))
+			expectedRecordsPerBatch = c.maxBatchSize
+			remaining := c.batchSize % c.maxBatchSize
+			if remaining == 0 {
+				expectedRecordsLastBatch = c.maxBatchSize
+			} else {
+				expectedRecordsLastBatch = remaining
+			}
+		}
+
+		for i := 0; i < c.batchSize; i++ {
+			batch = append(batch, map[string]string{"objectID": strconv.Itoa(i)})
+		}
+
+		name := fmt.Sprintf("trying to save %d records in %d batches", c.batchSize, expectedNbBatches)
+		index := initIndexWithMaxBatchSize(c.maxBatchSize)
+		res, err := index.SaveObjects(batch)
+
+		require.NoError(t, err, name)
+		require.Len(t, res.ObjectIDs(), c.batchSize, name)
+		require.Len(t, res.Responses, expectedNbBatches, name)
+
+		for i := 0; i < expectedNbBatches-1; i++ {
+			require.Len(t, res.Responses[i].ObjectIDs, expectedRecordsPerBatch, name)
+		}
+		require.Len(t, res.Responses[expectedNbBatches-1].ObjectIDs, expectedRecordsLastBatch, name)
+	}
+}
diff --git a/cts/index/url_encoding_index_name_test.go b/cts/index/url_encoding_index_name_test.go
index 12e9304f..f15ffcfb 100644
--- a/cts/index/url_encoding_index_name_test.go
+++ b/cts/index/url_encoding_index_name_test.go
@@ -27,10 +27,10 @@ func TestEnableURLEncodingIndexName(t *testing.T) {
 		if !unicode.IsPrint(rune(c)) ||
 			unicode.IsNumber(rune(c)) ||
 			unicode.IsLetter(rune(c)) ||
-			strings.Contains(forbiddenCharacters, string(c)) {
+			strings.Contains(forbiddenCharacters, fmt.Sprint(c)) {
 			continue
 		}
-		indexNames = append(indexNames, baseIndexName+string(c))
+		indexNames = append(indexNames, baseIndexName+fmt.Sprint(c))
 	}
 
 	g := wait.NewGroup()