Fix optional binding

This fixes #570. The code was mis-structured, and would occasionally
allocate a destination for an optional query parameter, when that parameter
should have been nil.

Author: mromaszewicz

bindparam.go

@@ -263,7 +263,7 @@ func bindSplitPartsToDestinationStruct(paramName string, parts []string, explode
 	return nil
 }
 
-// This works much like BindStyledParameter, however it takes a query argument
+// BindQueryParameter works much like BindStyledParameter, however it takes a query argument
 // input array from the url package, since query arguments come through a
 // different path than the styled arguments. They're also exceptionally fussy.
 // For example, consider the exploded and unexploded form parameter examples:
@@ -336,10 +336,12 @@ func BindQueryParameter(style string, explode bool, required bool, paramName str
 			case reflect.Slice:
 				// In the slice case, we simply use the arguments provided by
 				// http library.
+
 				if !found {
 					if required {
 						return fmt.Errorf("query parameter '%s' is required", paramName)
 					} else {
+						// If an optional parameter is not found, we do nothing,
 						return nil
 					}
 				}
@@ -349,7 +351,13 @@ func BindQueryParameter(style string, explode bool, required bool, paramName str
 				// form style object binding doesn't tell us which arguments
 				// in the query string correspond to the object's fields. We'll
 				// try to bind field by field.
-				err = bindParamsToExplodedObject(paramName, queryParams, output)
+				var fieldsPresent bool
+				fieldsPresent, err = bindParamsToExplodedObject(paramName, queryParams, output)
+				// If no fields were set, and there is no error, we will not fall
+				// through to assign the destination.
+				if !fieldsPresent {
+					return nil
+				}
 			default:
 				// Primitive object case. We expect to have 1 value to
 				// unmarshal.
@@ -363,6 +371,15 @@ func BindQueryParameter(style string, explode bool, required bool, paramName str
 				if len(values) != 1 {
 					return fmt.Errorf("multiple values for single value parameter '%s'", paramName)
 				}
+
+				if !found {
+					if required {
+						return fmt.Errorf("query parameter '%s' is required", paramName)
+					} else {
+						// If an optional parameter is not found, we do nothing,
+						return nil
+					}
+				}
 				err = BindStringToObject(values[0], output)
 			}
 			if err != nil {
@@ -427,21 +444,28 @@ func BindQueryParameter(style string, explode bool, required bool, paramName str
 	}
 }
 
-// This function reflects the destination structure, and pulls the value for
+// bindParamsToExplodedObject reflects the destination structure, and pulls the value for
 // each settable field from the given parameters map. This is to deal with the
 // exploded form styled object which may occupy any number of parameter names.
 // We don't try to be smart here, if the field exists as a query argument,
-// set its value.
-func bindParamsToExplodedObject(paramName string, values url.Values, dest interface{}) error {
+// set its value. This function returns a boolean, telling us whether there was
+// anything to bind. There will be nothing to bind if a parameter isn't found by name,
+// or none of an exploded object's fields are present.
+func bindParamsToExplodedObject(paramName string, values url.Values, dest interface{}) (bool, error) {
 	// Dereference pointers to their destination values
 	binder, v, t := indirect(dest)
 	if binder != nil {
-		return BindStringToObject(values.Get(paramName), dest)
+		_, found := values[paramName]
+		if !found {
+			return false, nil
+		}
+		return true, BindStringToObject(values.Get(paramName), dest)
 	}
 	if t.Kind() != reflect.Struct {
-		return fmt.Errorf("unmarshaling query arg '%s' into wrong type", paramName)
+		return false, fmt.Errorf("unmarshaling query arg '%s' into wrong type", paramName)
 	}
 
+	fieldsPresent := false
 	for i := 0; i < t.NumField(); i++ {
 		fieldT := t.Field(i)
 
@@ -466,15 +490,16 @@ func bindParamsToExplodedObject(paramName string, values url.Values, dest interf
 		fieldVal, found := values[fieldName]
 		if found {
 			if len(fieldVal) != 1 {
-				return fmt.Errorf("field '%s' specified multiple times for param '%s'", fieldName, paramName)
+				return false, fmt.Errorf("field '%s' specified multiple times for param '%s'", fieldName, paramName)
 			}
 			err := BindStringToObject(fieldVal[0], v.Field(i).Addr().Interface())
 			if err != nil {
-				return fmt.Errorf("could not bind query arg '%s' to request object: %s'", paramName, err)
+				return false, fmt.Errorf("could not bind query arg '%s' to request object: %s'", paramName, err)
 			}
+			fieldsPresent = true
 		}
 	}
-	return nil
+	return fieldsPresent, nil
 }
 
 // indirect

bindparam_test.go

@@ -335,6 +335,30 @@ func TestBindQueryParameter(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, expected, birthday)
 	})
+
+	t.Run("optional", func(t *testing.T) {
+		queryParams := url.Values{
+			"time":   {"2020-12-09T16:09:53+00:00"},
+			"number": {"100"},
+		}
+		// An optional time will be a pointer to a time in a parameter object
+		var optionalTime *time.Time
+		err := BindQueryParameter("form", true, false, "notfound", queryParams, &optionalTime)
+		require.NoError(t, err)
+		assert.Nil(t, optionalTime)
+
+		var optionalNumber *int
+		err = BindQueryParameter("form", true, false, "notfound", queryParams, &optionalNumber)
+		require.NoError(t, err)
+		assert.Nil(t, optionalNumber)
+
+		// If we require values, we require errors when they're not present.
+		err = BindQueryParameter("form", true, true, "notfound", queryParams, &optionalTime)
+		assert.Error(t, err)
+		err = BindQueryParameter("form", true, true, "notfound", queryParams, &optionalNumber)
+		assert.Error(t, err)
+
+	})
 }
 
 func TestBindParameterViaAlias(t *testing.T) {
@@ -423,37 +447,46 @@ func TestBindParamsToExplodedObject(t *testing.T) {
 	}
 
 	var dstTime time.Time
-	err := bindParamsToExplodedObject("time", values, &dstTime)
+	fieldsPresent, err := bindParamsToExplodedObject("time", values, &dstTime)
 	assert.NoError(t, err)
+	assert.True(t, fieldsPresent)
 	assert.EqualValues(t, now, dstTime)
 
 	type AliasedTime time.Time
 	var aDstTime AliasedTime
-	err = bindParamsToExplodedObject("time", values, &aDstTime)
+	fieldsPresent, err = bindParamsToExplodedObject("time", values, &aDstTime)
 	assert.NoError(t, err)
+	assert.True(t, fieldsPresent)
 	assert.EqualValues(t, now, aDstTime)
 
 	expectedDate := MockBinder{Time: time.Date(2020, 11, 6, 0, 0, 0, 0, time.UTC)}
 
 	var dstDate MockBinder
-	err = bindParamsToExplodedObject("date", values, &dstDate)
+	fieldsPresent, err = bindParamsToExplodedObject("date", values, &dstDate)
+	assert.NoError(t, err)
+	assert.True(t, fieldsPresent)
 	assert.EqualValues(t, expectedDate, dstDate)
 
 	var eDstDate EmbeddedMockBinder
-	err = bindParamsToExplodedObject("date", values, &eDstDate)
+	fieldsPresent, err = bindParamsToExplodedObject("date", values, &eDstDate)
+	assert.NoError(t, err)
+	assert.True(t, fieldsPresent)
 	assert.EqualValues(t, expectedDate, dstDate)
 
 	var nTDstDate AnotherMockBinder
-	err = bindParamsToExplodedObject("date", values, &nTDstDate)
+	fieldsPresent, err = bindParamsToExplodedObject("date", values, &nTDstDate)
+	assert.NoError(t, err)
+	assert.True(t, fieldsPresent)
 	assert.EqualValues(t, expectedDate, nTDstDate)
 
 	type ObjectWithOptional struct {
 		Time *time.Time `json:"time,omitempty"`
 	}
 
 	var optDstTime ObjectWithOptional
-	err = bindParamsToExplodedObject("explodedObject", values, &optDstTime)
+	fieldsPresent, err = bindParamsToExplodedObject("explodedObject", values, &optDstTime)
 	assert.NoError(t, err)
+	assert.True(t, fieldsPresent)
 	assert.EqualValues(t, &now, optDstTime.Time)
 }