Chore: Imperative request data binding (#39837)

* rename Bind to BindMiddleware

* make things private

* removed unused part of data bindings

* provide json and form binding helpers

* add example of binding migration in login api

* implement validation

* fix tests

* remove debug output

* put new bind api into macaron pacakge

* revert bind api breaking change

pkg/api/api.go

@@ -40,7 +40,7 @@ func (hs *HTTPServer) registerRoutes() {
 
 	// not logged in views
 	r.Get("/logout", hs.Logout)
-	r.Post("/login", quota("session"), bind(dtos.LoginCommand{}), routing.Wrap(hs.LoginPost))
+	r.Post("/login", quota("session"), routing.Wrap(hs.LoginPost))
 	r.Get("/login/:name", quota("session"), hs.OAuthLogin)
 	r.Get("/login", hs.LoginView)
 	r.Get("/invite/:code", hs.Index)

pkg/api/login.go

@@ -19,6 +19,7 @@ import (
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/util/errutil"
+	macaron "gopkg.in/macaron.v1"
 )
 
 const (
@@ -170,7 +171,11 @@ func (hs *HTTPServer) LoginAPIPing(c *models.ReqContext) response.Response {
 	return response.Error(401, "Unauthorized", nil)
 }
 
-func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) response.Response {
+func (hs *HTTPServer) LoginPost(c *models.ReqContext) response.Response {
+	cmd := dtos.LoginCommand{}
+	if err := macaron.Bind(c.Req, &cmd); err != nil {
+		return response.Error(http.StatusBadRequest, "bad login data", err)
+	}
 	authModule := ""
 	var user *models.User
 	var resp *response.NormalResponse

pkg/api/login_test.go

@@ -1,9 +1,11 @@
 package api
 
 import (
+	"bytes"
 	"encoding/hex"
 	"errors"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
@@ -336,11 +338,9 @@ func TestLoginPostRedirect(t *testing.T) {
 	hs.Cfg.CookieSecure = true
 
 	sc.defaultHandler = routing.Wrap(func(w http.ResponseWriter, c *models.ReqContext) response.Response {
-		cmd := dtos.LoginCommand{
+		c.Req.Header.Set("Content-Type", "application/json")
-			User:     "admin",
+		c.Req.Body = io.NopCloser(bytes.NewBufferString(`{"user":"admin","password":"admin"}`))
-			Password: "admin",
+		return hs.LoginPost(c)
-		}
-		return hs.LoginPost(c, cmd)
 	})
 
 	bus.AddHandler("grafana-auth", func(query *models.LoginUserQuery) error {
@@ -614,11 +614,10 @@ func TestLoginPostRunLokingHook(t *testing.T) {
 	}
 
 	sc.defaultHandler = routing.Wrap(func(w http.ResponseWriter, c *models.ReqContext) response.Response {
-		cmd := dtos.LoginCommand{
+		c.Req.Header.Set("Content-Type", "application/json")
-			User:     "admin",
+		c.Req.Body = io.NopCloser(bytes.NewBufferString(`{"user":"admin","password":"admin"}`))
-			Password: "admin",
+		x := hs.LoginPost(c)
-		}
+		return x
-		return hs.LoginPost(c, cmd)
 	})
 
 	testHook := loginHookTest{}

pkg/macaron/binding.go

@@ -0,0 +1,74 @@
+package macaron
+
+import (
+	"encoding/json"
+	"fmt"
+	"io"
+	"net/http"
+	"reflect"
+)
+
+// Bind deserializes JSON payload from the request
+func Bind(req *http.Request, v interface{}) error {
+	if req.Body != nil {
+		defer req.Body.Close()
+		err := json.NewDecoder(req.Body).Decode(v)
+		if err != nil && err != io.EOF {
+			return err
+		}
+	}
+	return validate(v)
+}
+
+type Validator interface {
+	Validate() error
+}
+
+func validate(obj interface{}) error {
+	// If type has a Validate() method - use that
+	if validator, ok := obj.(Validator); ok {
+		return validator.Validate()
+	}
+	// Otherwise, use relfection to match `binding:"Required"` struct field tags.
+	// Resolve all pointers and interfaces, until we get a concrete type.
+	t := reflect.TypeOf(obj)
+	v := reflect.ValueOf(obj)
+	for v.Kind() == reflect.Interface || v.Kind() == reflect.Ptr {
+		t = t.Elem()
+		v = v.Elem()
+	}
+	switch v.Kind() {
+	// For arrays and slices - iterate over each element and validate it recursively
+	case reflect.Slice, reflect.Array:
+		for i := 0; i < v.Len(); i++ {
+			e := v.Index(i).Interface()
+			if err := validate(e); err != nil {
+				return err
+			}
+		}
+	// For structs - iterate over each field, check for the "Required" constraint (Macaron legacy), then validate it recursively
+	case reflect.Struct:
+		for i := 0; i < v.NumField(); i++ {
+			field := t.Field(i)
+			value := v.Field(i)
+			rule := field.Tag.Get("binding")
+			if !value.CanInterface() {
+				continue
+			}
+			if rule == "Required" {
+				zero := reflect.Zero(field.Type).Interface()
+				if value.Kind() == reflect.Slice {
+					if value.Len() == 0 {
+						return fmt.Errorf("required slice %s must not be empty", field.Name)
+					}
+				} else if reflect.DeepEqual(zero, value.Interface()) {
+					return fmt.Errorf("required value %s must not be empty", field.Name)
+				}
+			}
+			if err := validate(value.Interface()); err != nil {
+				return err
+			}
+		}
+	}
+	return nil
+}

pkg/macaron/binding/binding.go

@@ -37,11 +37,11 @@ func bind(ctx *macaron.Context, obj interface{}, ifacePtr ...interface{}) {
 	if ctx.Req.Method == "POST" || ctx.Req.Method == "PUT" || ctx.Req.Method == "PATCH" || ctx.Req.Method == "DELETE" {
 		switch {
 		case strings.Contains(contentType, "form-urlencoded"):
-			_, _ = ctx.Invoke(Form(obj, ifacePtr...))
+			_, _ = ctx.Invoke(bindForm(obj, ifacePtr...))
 		case strings.Contains(contentType, "multipart/form-data"):
-			_, _ = ctx.Invoke(MultipartForm(obj, ifacePtr...))
+			_, _ = ctx.Invoke(bindMultipartForm(obj, ifacePtr...))
 		case strings.Contains(contentType, "json"):
-			_, _ = ctx.Invoke(Json(obj, ifacePtr...))
+			_, _ = ctx.Invoke(bindJson(obj, ifacePtr...))
 		default:
 			var errors Errors
 			if contentType == "" {
@@ -53,7 +53,7 @@ func bind(ctx *macaron.Context, obj interface{}, ifacePtr ...interface{}) {
 			ctx.Map(obj) // Map a fake struct so handler won't panic.
 		}
 	} else {
-		_, _ = ctx.Invoke(Form(obj, ifacePtr...))
+		_, _ = ctx.Invoke(bindForm(obj, ifacePtr...))
 	}
 }
 
@@ -87,9 +87,6 @@ func errorHandler(errs Errors, rw http.ResponseWriter) {
 	}
 }
 
-// CustomErrorHandler will be invoked if errors occured.
-var CustomErrorHandler func(*macaron.Context, Errors)
-
 // Bind wraps up the functionality of the Form and Json middleware
 // according to the Content-Type and verb of the request.
 // A Content-Type is required for POST and PUT requests.
@@ -101,26 +98,11 @@ var CustomErrorHandler func(*macaron.Context, Errors)
 func Bind(obj interface{}, ifacePtr ...interface{}) macaron.Handler {
 	return func(ctx *macaron.Context) {
 		bind(ctx, obj, ifacePtr...)
-		if handler, ok := obj.(ErrorHandler); ok {
+		_, _ = ctx.Invoke(errorHandler)
-			_, _ = ctx.Invoke(handler.Error)
-		} else if CustomErrorHandler != nil {
-			_, _ = ctx.Invoke(CustomErrorHandler)
-		} else {
-			_, _ = ctx.Invoke(errorHandler)
-		}
 	}
 }
 
-// BindIgnErr will do the exactly same thing as Bind but without any
+// bindForm is middleware to deserialize form-urlencoded data from the request.
-// error handling, which user has freedom to deal with them.
-// This allows user take advantages of validation.
-func BindIgnErr(obj interface{}, ifacePtr ...interface{}) macaron.Handler {
-	return func(ctx *macaron.Context) {
-		bind(ctx, obj, ifacePtr...)
-	}
-}
-
-// Form is middleware to deserialize form-urlencoded data from the request.
 // It gets data from the form-urlencoded body, if present, or from the
 // query string. It uses the http.Request.ParseForm() method
 // to perform deserialization, then reflection is used to map each field
@@ -129,7 +111,7 @@ func BindIgnErr(obj interface{}, ifacePtr ...interface{}) macaron.Handler {
 // keys, for example: key=val1&key=val2&key=val3
 // An interface pointer can be added as a second argument in order
 // to map the struct to a specific interface.
-func Form(formStruct interface{}, ifacePtr ...interface{}) macaron.Handler {
+func bindForm(formStruct interface{}, ifacePtr ...interface{}) macaron.Handler {
 	return func(ctx *macaron.Context) {
 		var errors Errors
 
@@ -153,11 +135,11 @@ func Form(formStruct interface{}, ifacePtr ...interface{}) macaron.Handler {
 // Set this to whatever value you prefer; default is 10 MB.
 var MaxMemory = int64(1024 * 1024 * 10)
 
-// MultipartForm works much like Form, except it can parse multipart forms
+// bindMultipartForm works much like Form, except it can parse multipart forms
 // and handle file uploads. Like the other deserialization middleware handlers,
 // you can pass in an interface to make the interface available for injection
 // into other handlers later.
-func MultipartForm(formStruct interface{}, ifacePtr ...interface{}) macaron.Handler {
+func bindMultipartForm(formStruct interface{}, ifacePtr ...interface{}) macaron.Handler {
 	return func(ctx *macaron.Context) {
 		var errors Errors
 		ensureNotPointer(formStruct)
@@ -189,12 +171,12 @@ func MultipartForm(formStruct interface{}, ifacePtr ...interface{}) macaron.Hand
 	}
 }
 
-// Json is middleware to deserialize a JSON payload from the request
+// bindJson is middleware to deserialize a JSON payload from the request
 // into the struct that is passed in. The resulting struct is then
 // validated, but no error handling is actually performed here.
 // An interface pointer can be added as a second argument in order
 // to map the struct to a specific interface.
-func Json(jsonStruct interface{}, ifacePtr ...interface{}) macaron.Handler {
+func bindJson(jsonStruct interface{}, ifacePtr ...interface{}) macaron.Handler {
 	return func(ctx *macaron.Context) {
 		var errors Errors
 		ensureNotPointer(jsonStruct)
@@ -210,52 +192,11 @@ func Json(jsonStruct interface{}, ifacePtr ...interface{}) macaron.Handler {
 	}
 }
 
-// URL is the middleware to parse URL parameters into struct fields.
+// validateMiddleware is middleware to enforce required fields. If the struct
-func URL(obj interface{}, ifacePtr ...interface{}) macaron.Handler {
+// passed in implements Validator, then the user-defined validateMiddleware method
-	return func(ctx *macaron.Context) {
-		var errors Errors
-
-		ensureNotPointer(obj)
-		obj := reflect.New(reflect.TypeOf(obj))
-
-		val := obj.Elem()
-		for k, v := range macaron.Params(ctx.Req) {
-			field := val.FieldByName(k[1:])
-			if field.IsValid() {
-				errors = setWithProperType(field.Kind(), v, field, k, errors)
-			}
-		}
-		validateAndMap(obj, ctx, errors, ifacePtr...)
-	}
-}
-
-// RawValidate is same as Validate but does not require a HTTP context,
-// and can be used independently just for validation.
-// This function does not support Validator interface.
-func RawValidate(obj interface{}) Errors {
-	var errs Errors
-	v := reflect.ValueOf(obj)
-	k := v.Kind()
-	if k == reflect.Interface || k == reflect.Ptr {
-		v = v.Elem()
-		k = v.Kind()
-	}
-	if k == reflect.Slice || k == reflect.Array {
-		for i := 0; i < v.Len(); i++ {
-			e := v.Index(i).Interface()
-			errs = validateStruct(errs, e)
-		}
-	} else {
-		errs = validateStruct(errs, obj)
-	}
-	return errs
-}
-
-// Validate is middleware to enforce required fields. If the struct
-// passed in implements Validator, then the user-defined Validate method
 // is executed, and its errors are mapped to the context. This middleware
 // performs no error handling: it merely detects errors and maps them.
-func Validate(obj interface{}) macaron.Handler {
+func validateMiddleware(obj interface{}) macaron.Handler {
 	return func(ctx *macaron.Context) {
 		var errs Errors
 		v := reflect.ValueOf(obj)
@@ -268,13 +209,13 @@ func Validate(obj interface{}) macaron.Handler {
 			for i := 0; i < v.Len(); i++ {
 				e := v.Index(i).Interface()
 				errs = validateStruct(errs, e)
-				if validator, ok := e.(Validator); ok {
+				if validator, ok := e.(_Validator); ok {
 					errs = validator.Validate(ctx, errs)
 				}
 			}
 		} else {
 			errs = validateStruct(errs, obj)
-			if validator, ok := obj.(Validator); ok {
+			if validator, ok := obj.(_Validator); ok {
 				errs = validator.Validate(ctx, errs)
 			}
 		}
@@ -283,9 +224,9 @@ func Validate(obj interface{}) macaron.Handler {
 }
 
 var (
-	AlphaDashPattern    = regexp.MustCompile(`[^\d\w-_]`)
+	alphaDashPattern    = regexp.MustCompile(`[^\d\w-_]`)
-	AlphaDashDotPattern = regexp.MustCompile(`[^\d\w-_\.]`)
+	alphaDashDotPattern = regexp.MustCompile(`[^\d\w-_\.]`)
-	EmailPattern        = regexp.MustCompile("[\\w!#$%&'*+/=?^_`{|}~-]+(?:\\.[\\w!#$%&'*+/=?^_`{|}~-]+)*@(?:[\\w](?:[\\w-]*[\\w])?\\.)+[a-zA-Z0-9](?:[\\w-]*[\\w])?")
+	emailPattern        = regexp.MustCompile("[\\w!#$%&'*+/=?^_`{|}~-]+(?:\\.[\\w!#$%&'*+/=?^_`{|}~-]+)*@(?:[\\w](?:[\\w-]*[\\w])?\\.)+[a-zA-Z0-9](?:[\\w-]*[\\w])?")
 )
 
 // Copied from github.com/asaskevich/govalidator.
@@ -323,40 +264,30 @@ func isURL(str string) bool {
 }
 
 type (
-	// Rule represents a validation rule.
+	// rule represents a validation rule.
-	Rule struct {
+	rule struct {
 		// IsMatch checks if rule matches.
 		IsMatch func(string) bool
 		// IsValid applies validation rule to condition.
 		IsValid func(Errors, string, interface{}) (bool, Errors)
 	}
 
-	// ParamRule does same thing as Rule but passes rule itself to IsValid method.
+	// paramRule does same thing as Rule but passes rule itself to IsValid method.
-	ParamRule struct {
+	paramRule struct {
 		// IsMatch checks if rule matches.
 		IsMatch func(string) bool
 		// IsValid applies validation rule to condition.
 		IsValid func(Errors, string, string, interface{}) (bool, Errors)
 	}
 
-	// RuleMapper and ParamRuleMapper represent validation rule mappers,
+	// _RuleMapper and ParamRuleMapper represent validation rule mappers,
 	// it allwos users to add custom validation rules.
-	RuleMapper      []*Rule
+	_RuleMapper      []*rule
-	ParamRuleMapper []*ParamRule
+	_ParamRuleMapper []*paramRule
 )
 
-var ruleMapper RuleMapper
+var ruleMapper _RuleMapper
-var paramRuleMapper ParamRuleMapper
+var paramRuleMapper _ParamRuleMapper
-
-// AddRule adds new validation rule.
-func AddRule(r *Rule) {
-	ruleMapper = append(ruleMapper, r)
-}
-
-// AddParamRule adds new validation rule.
-func AddParamRule(r *ParamRule) {
-	paramRuleMapper = append(paramRuleMapper, r)
-}
 
 func in(fieldValue interface{}, arr string) bool {
 	val := fmt.Sprintf("%v", fieldValue)
@@ -464,12 +395,12 @@ VALIDATE_RULES:
 				break VALIDATE_RULES
 			}
 		case rule == "AlphaDash":
-			if AlphaDashPattern.MatchString(fmt.Sprintf("%v", fieldValue)) {
+			if alphaDashPattern.MatchString(fmt.Sprintf("%v", fieldValue)) {
 				errors.Add([]string{field.Name}, ERR_ALPHA_DASH, "AlphaDash")
 				break VALIDATE_RULES
 			}
 		case rule == "AlphaDashDot":
-			if AlphaDashDotPattern.MatchString(fmt.Sprintf("%v", fieldValue)) {
+			if alphaDashDotPattern.MatchString(fmt.Sprintf("%v", fieldValue)) {
 				errors.Add([]string{field.Name}, ERR_ALPHA_DASH_DOT, "AlphaDashDot")
 				break VALIDATE_RULES
 			}
@@ -525,7 +456,7 @@ VALIDATE_RULES:
 				break VALIDATE_RULES
 			}
 		case rule == "Email":
-			if !EmailPattern.MatchString(fmt.Sprintf("%v", fieldValue)) {
+			if !emailPattern.MatchString(fmt.Sprintf("%v", fieldValue)) {
 				errors.Add([]string{field.Name}, ERR_EMAIL, "Email")
 				break VALIDATE_RULES
 			}
@@ -590,9 +521,6 @@ VALIDATE_RULES:
 	return errors
 }
 
-// NameMapper represents a form tag name mapper.
-type NameMapper func(string) string
-
 var (
 	nameMapper = func(field string) string {
 		newstr := make([]rune, 0, len(field))
@@ -609,11 +537,6 @@ var (
 	}
 )
 
-// SetNameMapper sets name mapper.
-func SetNameMapper(nm NameMapper) {
-	nameMapper = nm
-}
-
 // Takes values from the form data and puts them into a struct
 func mapForm(formStruct reflect.Value, form map[string][]string,
 	formfile map[string][]*multipart.FileHeader, errors Errors) Errors {
@@ -755,7 +678,7 @@ func ensureNotPointer(obj interface{}) {
 // with errors from deserialization, then maps both the
 // resulting struct and the errors to the context.
 func validateAndMap(obj reflect.Value, ctx *macaron.Context, errors Errors, ifacePtr ...interface{}) {
-	_, _ = ctx.Invoke(Validate(obj.Interface()))
+	_, _ = ctx.Invoke(validateMiddleware(obj.Interface()))
 	errors = append(errors, getErrors(ctx)...)
 	ctx.Map(errors)
 	ctx.Map(obj.Elem().Interface())
@@ -770,15 +693,9 @@ func getErrors(ctx *macaron.Context) Errors {
 }
 
 type (
-	// ErrorHandler is the interface that has custom error handling process.
+	// _Validator is the interface that handles some rudimentary
-	ErrorHandler interface {
-		// Error handles validation errors with custom process.
-		Error(*macaron.Context, Errors)
-	}
-
-	// Validator is the interface that handles some rudimentary
 	// request validation logic so your application doesn't have to.
-	Validator interface {
+	_Validator interface {
 		// Validate validates that the request is OK. It is recommended
 		// that validation be limited to checking values for syntax and
 		// semantics, enough to know that you can make sense of the request

pkg/macaron/binding_test.go

@@ -0,0 +1,101 @@
+package macaron
+
+import (
+	"errors"
+	"testing"
+)
+
+type StructWithInt struct {
+	A int `binding:"Required"`
+}
+
+type StructWithPrimitives struct {
+	A int     `binding:"Required"`
+	B string  `binding:"Required"`
+	C bool    `binding:"Required"`
+	D float64 `binding:"Required"`
+}
+
+type StructWithPrivateFields struct {
+	A int `binding:"Required"` // must be validated
+	b int `binding:"Required"` // will not be used
+}
+
+type StructWithInterface struct {
+	A interface{} `binding:"Required"`
+}
+type StructWithSliceInts struct {
+	A []int `binding:"Required"`
+}
+type StructWithSliceStructs struct {
+	A []StructWithInt `binding:"Required"`
+}
+type StructWithSliceInterfaces struct {
+	A []interface{} `binding:"Required"`
+}
+type StructWithStruct struct {
+	A StructWithInt `binding:"Required"`
+}
+type StructWithStructPointer struct {
+	A *StructWithInt `binding:"Required"`
+}
+type StructWithValidation struct {
+	A int
+}
+
+func (sv StructWithValidation) Validate() error {
+	if sv.A < 10 {
+		return errors.New("too small")
+	}
+	return nil
+}
+
+func TestValidationSuccess(t *testing.T) {
+	for _, x := range []interface{}{
+		42,
+		"foo",
+		struct{ A int }{},
+		StructWithInt{42},
+		StructWithPrimitives{42, "foo", true, 12.34},
+		StructWithPrivateFields{12, 0},
+		StructWithInterface{"foo"},
+		StructWithSliceInts{[]int{1, 2, 3}},
+		StructWithSliceInterfaces{[]interface{}{1, 2, 3}},
+		StructWithSliceStructs{[]StructWithInt{{1}, {2}}},
+		StructWithStruct{StructWithInt{3}},
+		StructWithStructPointer{&StructWithInt{3}},
+		StructWithValidation{42},
+	} {
+		if err := validate(x); err != nil {
+			t.Error("Validation failed:", x, err)
+		}
+	}
+}
+func TestValidationFailure(t *testing.T) {
+	for i, x := range []interface{}{
+		StructWithInt{0},
+		StructWithPrimitives{0, "foo", true, 12.34},
+		StructWithPrimitives{42, "", true, 12.34},
+		StructWithPrimitives{42, "foo", false, 12.34},
+		StructWithPrimitives{42, "foo", true, 0},
+		StructWithPrivateFields{0, 1},
+		StructWithInterface{},
+		StructWithInterface{nil},
+		StructWithSliceInts{},
+		StructWithSliceInts{[]int{}},
+		StructWithSliceStructs{[]StructWithInt{}},
+		StructWithSliceStructs{[]StructWithInt{{0}, {2}}},
+		StructWithSliceStructs{[]StructWithInt{{2}, {0}}},
+		StructWithSliceInterfaces{[]interface{}{}},
+		StructWithSliceInterfaces{nil},
+		StructWithStruct{StructWithInt{}},
+		StructWithStruct{StructWithInt{0}},
+		StructWithStructPointer{},
+		StructWithStructPointer{&StructWithInt{}},
+		StructWithValidation{2},
+	} {
+		if err := validate(x); err == nil {
+			t.Error("Validation should fail:", i, x)
+		}
+	}
+}