chore: audit and test for nils (#206)

* chore: audit request structs for nils and bad defaults

* audited dictionaries

* audited remaining request types

* fix request property name

* add pubsub nil value check in topic publish

* don't precompute error responses

momento/dictionary_test.go

@@ -96,6 +96,8 @@ var _ = Describe("Dictionary methods", func() {
 		Entry("nonexistent cache name", uuid.NewString(), uuid.NewString(), NotFoundError),
 		Entry("empty cache name", "", sharedContext.CollectionName, InvalidArgumentError),
 		Entry("empty dictionary name", sharedContext.CacheName, "", InvalidArgumentError),
+		Entry("nil dictionary name", sharedContext.CacheName, nil, InvalidArgumentError),
+		Entry("nil cache name", nil, sharedContext.CollectionName, InvalidArgumentError),
 	)
 
 	DescribeTable("add string and bytes value for single field happy path",
@@ -129,16 +131,22 @@ var _ = Describe("Dictionary methods", func() {
 		Entry("using bytes value and field", Bytes("myField"), Bytes("myValue"), "myField", []byte("myField"), "myValue", []byte("myValue")),
 	)
 
-	It("returns an error for set field when field is empty", func() {
-		Expect(
-			sharedContext.Client.DictionarySetField(sharedContext.Ctx, &DictionarySetFieldRequest{
-				CacheName:      sharedContext.CacheName,
-				DictionaryName: sharedContext.CollectionName,
-				Field:          String(""),
-				Value:          String("myValue"),
-			}),
-		).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
-	})
+	DescribeTable("try using empty and nil fields and values for set",
+		func(field Value, value Value) {
+			Expect(
+				sharedContext.Client.DictionarySetField(sharedContext.Ctx, &DictionarySetFieldRequest{
+					CacheName:      sharedContext.CacheName,
+					DictionaryName: sharedContext.CollectionName,
+					Field:          field,
+					Value:          value,
+				}),
+			).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+		},
+		Entry("empty field", String(""), String("value")),
+		Entry("nil field", nil, String("value")),
+		Entry("nil value", String("field"), nil),
+		Entry("both nil", nil, nil),
+	)
 
 	DescribeTable("add string fields and string and bytes values for set fields happy path",
 		func(elements map[string]Value, expectedItemsStringValue map[string]string, expectedItemsByteValue map[string][]byte) {
@@ -193,6 +201,16 @@ var _ = Describe("Dictionary methods", func() {
 		).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
 	})
 
+	It("returns an error if an item value is nil", func() {
+		Expect(
+			sharedContext.Client.DictionarySetFields(sharedContext.Ctx, &DictionarySetFieldsRequest{
+				CacheName:      sharedContext.CacheName,
+				DictionaryName: sharedContext.CollectionName,
+				Elements:       map[string]Value{"myField": String("myValue"), "myOtherField": nil},
+			}),
+		).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+	})
+
 	Describe("dictionary increment", func() {
 
 		It("populates nonexistent field", func() {
@@ -266,6 +284,17 @@ var _ = Describe("Dictionary methods", func() {
 				Fail("expected a hit for get field but got a miss")
 			}
 		})
+
+		It("returns an error when field is nil", func() {
+			Expect(
+				sharedContext.Client.DictionaryIncrement(sharedContext.Ctx, &DictionaryIncrementRequest{
+					CacheName:      sharedContext.CacheName,
+					DictionaryName: sharedContext.CollectionName,
+					Field:          nil,
+					Amount:         1,
+				}),
+			).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+		})
 	})
 
 	Describe("dictionary get", func() {
@@ -325,6 +354,16 @@ var _ = Describe("Dictionary methods", func() {
 				Expect(getResp).To(BeAssignableToTypeOf(&DictionaryGetFieldMiss{}))
 			})
 
+			It("returns an error for a nil field", func() {
+				Expect(
+					sharedContext.Client.DictionaryGetField(sharedContext.Ctx, &DictionaryGetFieldRequest{
+						CacheName:      sharedContext.CacheName,
+						DictionaryName: sharedContext.CollectionName,
+						Field:          nil,
+					}),
+				).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+			})
+
 		})
 
 		When("getting multiple fields", func() {
@@ -393,6 +432,16 @@ var _ = Describe("Dictionary methods", func() {
 				}
 			})
 
+			It("returns an error for a nil field", func() {
+				Expect(
+					sharedContext.Client.DictionaryGetFields(sharedContext.Ctx, &DictionaryGetFieldsRequest{
+						CacheName:      sharedContext.CacheName,
+						DictionaryName: sharedContext.CollectionName,
+						Fields:         []Value{String("myField"), nil},
+					}),
+				).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+			})
+
 		})
 	})
 
@@ -496,6 +545,16 @@ var _ = Describe("Dictionary methods", func() {
 				Expect(removeResp).To(BeAssignableToTypeOf(&DictionaryRemoveFieldSuccess{}))
 			})
 
+			It("returns an error when trying to remove a nil field", func() {
+				Expect(
+					sharedContext.Client.DictionaryRemoveField(sharedContext.Ctx, &DictionaryRemoveFieldRequest{
+						CacheName:      sharedContext.CacheName,
+						DictionaryName: sharedContext.CollectionName,
+						Field:          nil,
+					}),
+				).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+			})
+
 		})
 
 		When("removing multiple fields", func() {
@@ -544,6 +603,26 @@ var _ = Describe("Dictionary methods", func() {
 				Expect(removeResp).To(BeAssignableToTypeOf(&DictionaryRemoveFieldsSuccess{}))
 			})
 
+			It("returns an error when Fields is nil", func() {
+				Expect(
+					sharedContext.Client.DictionaryRemoveFields(sharedContext.Ctx, &DictionaryRemoveFieldsRequest{
+						CacheName:      sharedContext.CacheName,
+						DictionaryName: sharedContext.CollectionName,
+						Fields:         nil,
+					}),
+				).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+			})
+
+			It("returns an error when one field is nil", func() {
+				Expect(
+					sharedContext.Client.DictionaryRemoveFields(sharedContext.Ctx, &DictionaryRemoveFieldsRequest{
+						CacheName:      sharedContext.CacheName,
+						DictionaryName: sharedContext.CollectionName,
+						Fields:         []Value{String("myField"), nil, String("myField2")},
+					}),
+				).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+			})
+
 		})
 	})
 

momento/list_test.go

@@ -312,7 +312,7 @@ var _ = Describe("List methods", func() {
 
 			It("returns an invalid argument for a nil value", func() {
 				populateList(sharedContext, 5)
-				concatValues := []Value{nil, nil}
+				concatValues := []Value{nil, String("aRealValue"), nil}
 				Expect(
 					sharedContext.Client.ListConcatenateFront(sharedContext.Ctx, &ListConcatenateFrontRequest{
 						CacheName:          sharedContext.CacheName,
@@ -382,7 +382,7 @@ var _ = Describe("List methods", func() {
 
 			It("returns an invalid argument for a nil value", func() {
 				populateList(sharedContext, 5)
-				concatValues := []Value{nil, nil}
+				concatValues := []Value{nil, String("aRealValue"), nil}
 				Expect(
 					sharedContext.Client.ListConcatenateBack(sharedContext.Ctx, &ListConcatenateBackRequest{
 						CacheName:           sharedContext.CacheName,
@@ -523,6 +523,16 @@ var _ = Describe("List methods", func() {
 				}
 			})
 
+			It("returns an error for a nil value", func() {
+				Expect(
+					sharedContext.Client.ListRemoveValue(sharedContext.Ctx, &ListRemoveValueRequest{
+						CacheName: sharedContext.CacheName,
+						ListName:  sharedContext.CollectionName,
+						Value:     nil,
+					}),
+				).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+			})
+
 		})
 
 		When("removing a value that appears multiple times", func() {
@@ -560,6 +570,16 @@ var _ = Describe("List methods", func() {
 				}
 			})
 
+			It("returns an error for a nil value", func() {
+				Expect(
+					sharedContext.Client.ListRemoveValue(sharedContext.Ctx, &ListRemoveValueRequest{
+						CacheName: sharedContext.CacheName,
+						ListName:  sharedContext.CollectionName,
+						Value:     nil,
+					}),
+				).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+			})
+
 		})
 
 		When("removing a value that isn't in the list", func() {
@@ -597,6 +617,7 @@ var _ = Describe("List methods", func() {
 				).To(BeAssignableToTypeOf(&ListRemoveValueSuccess{}))
 
 			})
+
 		})
 
 	})

momento/pubsub_test.go

@@ -64,7 +64,7 @@ var _ = Describe("Pubsub", func() {
 		}
 
 		cancelContext, cancelFunction := context.WithCancel(sharedContext.Ctx)
-		receivedValues := []TopicValue{}
+		var receivedValues []TopicValue
 		ready := make(chan int, 1)
 		go func() {
 			ready <- 1
@@ -100,6 +100,16 @@ var _ = Describe("Pubsub", func() {
 		Expect(receivedValues).To(Equal(publishedValues))
 	})
 
+	It("returns an error when trying to publish a nil topic value", func() {
+		Expect(
+			sharedContext.Client.TopicPublish(sharedContext.Ctx, &TopicPublishRequest{
+				CacheName: sharedContext.CacheName,
+				TopicName: sharedContext.CollectionName,
+				Value:     nil,
+			}),
+		).Error().To(HaveMomentoErrorCode(InvalidArgumentError))
+	})
+
 	Describe(`TopicSubscribe`, func() {
 		It(`Does not error on a non-existent topic`, func() {
 			Expect(

momento/requester.go

@@ -70,29 +70,59 @@ type hasTTL interface {
 	ttl() time.Duration
 }
 
+func buildError(errorCode string, errorMessage string, originalError error) MomentoError {
+	return convertMomentoSvcErrorToCustomerError(
+		momentoerrors.NewMomentoSvcErr(errorCode, errorMessage, originalError),
+	)
+}
+
 func prepareName(name string, label string) (string, error) {
 	if len(strings.TrimSpace(name)) < 1 {
 		errStr := fmt.Sprintf("%v cannot be empty", label)
-		return "", momentoerrors.NewMomentoSvcErr(momentoerrors.InvalidArgumentError, errStr, nil)
+		return "", buildError(momentoerrors.InvalidArgumentError, errStr, nil)
 	}
 	return name, nil
 }
 
+func prepareElementName(name Value) ([]byte, error) {
+	if name == nil {
+		return nil, buildError(
+			momentoerrors.InvalidArgumentError, "element name cannot be nil or empty", nil,
+		)
+	}
+
+	// just validate not empty using prepareName
+	nameBytes := name.asBytes()
+	_, err := prepareName(string(nameBytes), "element name")
+	if err != nil {
+		return nil, err
+	}
+
+	return nameBytes, nil
+}
+
 func prepareCacheName(r hasCacheName) (string, error) {
 	return prepareName(r.cacheName(), "Cache name")
 }
 
 func prepareKey(r hasKey) ([]byte, error) {
-	key := r.key().asBytes()
+	if r.key() == nil {
+		return nil, buildError(momentoerrors.InvalidArgumentError, "key cannot be nil or empty", nil)
+	}
 
+	key := r.key().asBytes()
 	if len(key) == 0 {
-		err := momentoerrors.NewMomentoSvcErr(momentoerrors.InvalidArgumentError, "key cannot be empty", nil)
-		return nil, convertMomentoSvcErrorToCustomerError(err)
+		return nil, buildError(momentoerrors.InvalidArgumentError, "key cannot be nil or empty", nil)
 	}
 	return key, nil
 }
 
 func prepareField(r hasField) ([]byte, error) {
+	if r.field() == nil {
+		return nil, buildError(
+			momentoerrors.InvalidArgumentError, "field cannot be nil or empty", nil,
+		)
+	}
 	field := r.field().asBytes()
 	if err := validateNotEmpty(field, "field"); err != nil {
 		return nil, err
@@ -101,11 +131,17 @@ func prepareField(r hasField) ([]byte, error) {
 }
 
 func prepareFields(r hasFields) ([][]byte, error) {
+	if r.fields() == nil {
+		return nil, buildError(momentoerrors.InvalidArgumentError, "fields cannot be nil or empty", nil)
+	}
 	var fields [][]byte
 	for _, valueField := range r.fields() {
+		if valueField == nil {
+			return nil, buildError(momentoerrors.InvalidArgumentError, "fields cannot be nil or empty", nil)
+		}
 		field := valueField.asBytes()
 		if err := validateNotEmpty(field, "field"); err != nil {
-			return nil, err
+			return nil, buildError(momentoerrors.InvalidArgumentError, "fields cannot be nil or empty", nil)
 		}
 		fields = append(fields, field)
 	}
@@ -114,10 +150,8 @@ func prepareFields(r hasFields) ([][]byte, error) {
 
 func prepareValue(r hasValue) ([]byte, momentoerrors.MomentoSvcErr) {
 	if r.value() == nil {
-		return []byte{}, momentoerrors.NewMomentoSvcErr(
-			momentoerrors.InvalidArgumentError,
-			"value may not be nil",
-			nil,
+		return []byte{}, buildError(
+			momentoerrors.InvalidArgumentError, "value may not be nil", nil,
 		)
 	}
 	return r.value().asBytes(), nil
@@ -135,10 +169,8 @@ func prepareElements(r hasElements) (map[string][]byte, error) {
 	retMap := make(map[string][]byte)
 	for k, v := range r.elements() {
 		if v == nil {
-			return map[string][]byte{}, momentoerrors.NewMomentoSvcErr(
-				momentoerrors.InvalidArgumentError,
-				"item values may not be nil",
-				nil,
+			return map[string][]byte{}, buildError(
+				momentoerrors.InvalidArgumentError, "item values may not be nil", nil,
 			)
 		}
 		if err := validateNotEmpty([]byte(k), "item keys"); err != nil {
@@ -155,24 +187,21 @@ func prepareTTL(r hasTTL, defaultTtl time.Duration) (uint64, error) {
 		ttl = defaultTtl
 	}
 	if ttl <= time.Duration(0) {
-		return 0, momentoerrors.NewMomentoSvcErr(
-			momentoerrors.InvalidArgumentError,
-			"ttl must be a non-zero positive value",
-			nil,
+		return 0, buildError(
+			momentoerrors.InvalidArgumentError, "ttl must be a non-zero positive value", nil,
 		)
 	}
 	return uint64(ttl.Milliseconds()), nil
 }
 
 func momentoValuesToPrimitiveByteList(i []Value) ([][]byte, momentoerrors.MomentoSvcErr) {
+	if i == nil {
+		return [][]byte{}, buildError(momentoerrors.InvalidArgumentError, "values may not be nil", nil)
+	}
 	var rList [][]byte
 	for _, mb := range i {
 		if mb == nil {
-			return [][]byte{}, momentoerrors.NewMomentoSvcErr(
-				momentoerrors.InvalidArgumentError,
-				"values may not be nil",
-				nil,
-			)
+			return [][]byte{}, buildError(momentoerrors.InvalidArgumentError, "values may not be nil", nil)
 		}
 		rList = append(rList, mb.asBytes())
 	}
@@ -181,7 +210,7 @@ func momentoValuesToPrimitiveByteList(i []Value) ([][]byte, momentoerrors.Moment
 
 func validateNotEmpty(field []byte, label string) error {
 	if len(field) == 0 {
-		return momentoerrors.NewMomentoSvcErr(
+		return buildError(
 			momentoerrors.InvalidArgumentError, fmt.Sprintf("%s cannot be empty", label), nil,
 		)
 	}