revert #28 and add tests (#29)

* Revert "Skipping empty string in update expressions as it used to convert to … (#28)"

This reverts commit 7fec125.

* add tests

Author: pm-nilesh-chate

.gitignore

@@ -1,4 +1,5 @@
 .idea
 vendor
 Gopkg.*
-.coverage
+.coverage*
+coverage*

dynamo_repository.go

@@ -109,9 +109,6 @@ func (repository Repository) UpdateWithContext(ctx context.Context, expression U
 	}
 
 	for expr, value := range values {
-		if isEmptyString(value) {
-			continue
-		}
 		if expression == Add {
 			update.Add(expr, value)
 		}
@@ -162,9 +159,6 @@ func (repository Repository) prepareUpdateWithUpdateExpressions(
 		expression := UpdateExpression(updateExpression)
 
 		for expr, value := range v {
-			if isEmptyString(value) {
-				continue
-			}
 			if expression == Add {
 				update.Add(expr, value)
 			}

helper.go

@@ -13,11 +13,6 @@ func valueFromPtr[T any](ptr *T) T {
 	return *ptr
 }
 
-func isEmptyString(value any) bool {
-	str, ok := value.(string)
-	return ok && str == ""
-}
-
 func buildTableKeyCondition(table dynamo.Table, key KeyInterface) *dynamo.Query {
 	q := table.Get(*key.HashKeyName(), key.HashKey())
 

key.go

@@ -100,8 +100,5 @@ func isValidHashKey(key KeyInterface) error {
 	if key.HashKey() == nil {
 		return ErrInvalidHashKeyValue
 	}
-	if str, ok := key.HashKey().(string); ok && str == "" {
-		return ErrInvalidHashKeyValue
-	}
 	return nil
 }

repository_batch_get_test.go

@@ -0,0 +1,209 @@
+package djoemo_test
+
+import (
+	"context"
+	"errors"
+
+	"github.com/adjoeio/djoemo"
+	"github.com/adjoeio/djoemo/mock"
+	"github.com/aws/aws-sdk-go/service/dynamodb"
+	"github.com/aws/aws-sdk-go/service/dynamodb/dynamodbattribute"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"go.uber.org/mock/gomock"
+)
+
+var _ = Describe("Repository BatchGetItemsWithContext", func() {
+	const UserTableName = "UserTable"
+
+	var (
+		dAPIMock    *mock.MockDynamoDBAPI
+		repository  djoemo.RepositoryInterface
+		logMock     *mock.MockLogInterface
+		metricsMock *mock.MockMetricsInterface
+	)
+
+	BeforeEach(func() {
+		mockCtrl := gomock.NewController(GinkgoT())
+		dAPIMock = mock.NewMockDynamoDBAPI(mockCtrl)
+		logMock = mock.NewMockLogInterface(mockCtrl)
+		metricsMock = mock.NewMockMetricsInterface(mockCtrl)
+		repository = djoemo.NewRepository(dAPIMock)
+		repository.WithLog(logMock)
+		repository.WithMetrics(metricsMock)
+	})
+
+	It("should return false and nil when keys slice is empty", func() {
+		users := &[]User{}
+		found, err := repository.BatchGetItemsWithContext(context.Background(), []djoemo.KeyInterface{}, users)
+		Expect(err).To(BeNil())
+		Expect(found).To(BeFalse())
+	})
+
+	It("should return error when key has no table name", func() {
+		invalidKey := djoemo.Key().WithHashKeyName("UUID").WithHashKey("uuid")
+		metricsMock.EXPECT().Record(gomock.Any(), djoemo.OpRead, invalidKey, gomock.Any(), false)
+
+		users := &[]User{}
+		found, err := repository.BatchGetItemsWithContext(
+			context.Background(),
+			[]djoemo.KeyInterface{invalidKey},
+			users,
+		)
+		Expect(err).To(Equal(djoemo.ErrInvalidTableName))
+		Expect(found).To(BeFalse())
+	})
+
+	It("should return error when keys refer to different tables", func() {
+		key1 := djoemo.Key().WithTableName(UserTableName).
+			WithHashKeyName("UUID").WithHashKey("uuid1")
+		key2 := djoemo.Key().WithTableName("OtherTable").
+			WithHashKeyName("UUID").WithHashKey("uuid2")
+
+		// Note: success flag captured from outer err var which isn't
+		// updated for ErrInvalidBatchRequest return path.
+		metricsMock.EXPECT().Record(gomock.Any(), djoemo.OpRead, gomock.Any(), gomock.Any(), gomock.Any()).Times(2)
+
+		users := &[]User{}
+		found, err := repository.BatchGetItemsWithContext(
+			context.Background(),
+			[]djoemo.KeyInterface{key1, key2},
+			users,
+		)
+		Expect(err).To(Equal(djoemo.ErrInvalidBatchRequest))
+		Expect(found).To(BeFalse())
+	})
+
+	It("should return items when batch get succeeds by hash key", func() {
+		key1 := djoemo.Key().WithTableName(UserTableName).
+			WithHashKeyName("UUID").WithHashKey("uuid1")
+		key2 := djoemo.Key().WithTableName(UserTableName).
+			WithHashKeyName("UUID").WithHashKey("uuid2")
+
+		items := []map[string]*dynamodb.AttributeValue{}
+		for _, u := range []map[string]interface{}{
+			{"UUID": "uuid1", "UserName": "name1"},
+			{"UUID": "uuid2", "UserName": "name2"},
+		} {
+			av, _ := dynamodbattribute.MarshalMap(u)
+			items = append(items, av)
+		}
+		output := &dynamodb.BatchGetItemOutput{
+			Responses: map[string][]map[string]*dynamodb.AttributeValue{
+				UserTableName: items,
+			},
+		}
+
+		dAPIMock.EXPECT().
+			BatchGetItemWithContext(gomock.Any(), gomock.Any()).
+			Return(output, nil).
+			AnyTimes()
+
+		metricsMock.EXPECT().Record(gomock.Any(), djoemo.OpRead, key1, gomock.Any(), true)
+		metricsMock.EXPECT().Record(gomock.Any(), djoemo.OpRead, key2, gomock.Any(), true)
+
+		users := &[]User{}
+		found, err := repository.BatchGetItemsWithContext(
+			context.Background(),
+			[]djoemo.KeyInterface{key1, key2},
+			users,
+		)
+		Expect(err).To(BeNil())
+		Expect(found).To(BeTrue())
+		Expect(len(*users)).To(Equal(2))
+	})
+
+	It("should handle hash and range keys", func() {
+		key1 := djoemo.Key().WithTableName(UserTableName).
+			WithHashKeyName("UUID").WithHashKey("uuid1").
+			WithRangeKeyName("Email").WithRangeKey("a@adjoe.io")
+		key2 := djoemo.Key().WithTableName(UserTableName).
+			WithHashKeyName("UUID").WithHashKey("uuid2").
+			WithRangeKeyName("Email").WithRangeKey("b@adjoe.io")
+
+		items := []map[string]*dynamodb.AttributeValue{}
+		for _, u := range []map[string]interface{}{
+			{"UUID": "uuid1", "Email": "a@adjoe.io"},
+			{"UUID": "uuid2", "Email": "b@adjoe.io"},
+		} {
+			av, _ := dynamodbattribute.MarshalMap(u)
+			items = append(items, av)
+		}
+		output := &dynamodb.BatchGetItemOutput{
+			Responses: map[string][]map[string]*dynamodb.AttributeValue{
+				UserTableName: items,
+			},
+		}
+
+		dAPIMock.EXPECT().
+			BatchGetItemWithContext(gomock.Any(), gomock.Any()).
+			Return(output, nil).
+			AnyTimes()
+
+		metricsMock.EXPECT().Record(gomock.Any(), djoemo.OpRead, key1, gomock.Any(), true)
+		metricsMock.EXPECT().Record(gomock.Any(), djoemo.OpRead, key2, gomock.Any(), true)
+
+		profiles := &[]Profile{}
+		found, err := repository.BatchGetItemsWithContext(
+			context.Background(),
+			[]djoemo.KeyInterface{key1, key2},
+			profiles,
+		)
+		Expect(err).To(BeNil())
+		Expect(found).To(BeTrue())
+		Expect(len(*profiles)).To(Equal(2))
+	})
+
+	It("should return false and nil when dynamo returns ErrNotFound", func() {
+		key := djoemo.Key().WithTableName(UserTableName).
+			WithHashKeyName("UUID").WithHashKey("uuid")
+
+		output := &dynamodb.BatchGetItemOutput{
+			Responses: map[string][]map[string]*dynamodb.AttributeValue{
+				UserTableName: {},
+			},
+		}
+
+		dAPIMock.EXPECT().
+			BatchGetItemWithContext(gomock.Any(), gomock.Any()).
+			Return(output, nil).
+			AnyTimes()
+
+		metricsMock.EXPECT().Record(gomock.Any(), djoemo.OpRead, key, gomock.Any(), true)
+		logMock.EXPECT().WithContext(gomock.Any()).Return(logMock)
+		logMock.EXPECT().WithField(djoemo.TableName, UserTableName).Return(logMock)
+		logMock.EXPECT().Info(djoemo.ErrNoItemFound.Error())
+
+		users := &[]User{}
+		found, err := repository.BatchGetItemsWithContext(
+			context.Background(),
+			[]djoemo.KeyInterface{key},
+			users,
+		)
+		Expect(err).To(BeNil())
+		Expect(found).To(BeFalse())
+	})
+
+	It("should return false and error when dynamo returns an error", func() {
+		key := djoemo.Key().WithTableName(UserTableName).
+			WithHashKeyName("UUID").WithHashKey("uuid")
+
+		dbErr := errors.New("some dynamo error")
+
+		dAPIMock.EXPECT().
+			BatchGetItemWithContext(gomock.Any(), gomock.Any()).
+			Return(nil, dbErr).
+			AnyTimes()
+
+		metricsMock.EXPECT().Record(gomock.Any(), djoemo.OpRead, key, gomock.Any(), false)
+
+		users := &[]User{}
+		found, err := repository.BatchGetItemsWithContext(
+			context.Background(),
+			[]djoemo.KeyInterface{key},
+			users,
+		)
+		Expect(err).To(HaveOccurred())
+		Expect(found).To(BeFalse())
+	})
+})