refactor: clean up form binding & validation (#38873)

Clarify the "validation" and "error display" logic.

All the copied&pasted `Validate` functions are removed.

modules/web/middleware/binding.go

@@ -5,12 +5,11 @@
 package middleware
 
 import (
-	"net/http"
 	"reflect"
 	"strings"
 
-	"gitea.dev/modules/reqctx"
 	"gitea.dev/modules/setting"
+	"gitea.dev/modules/structs"
 	"gitea.dev/modules/translation"
 	"gitea.dev/modules/util"
 	"gitea.dev/modules/validation"
@@ -18,17 +17,13 @@ import (
 	"gitea.com/go-chi/binding"
 )
 
-// ValidateContext is a special context for form validation middleware. It may be different from other contexts.
-type ValidateContext struct {
-	Locale translation.Locale
-	Data   reqctx.ContextData
-	Req    *http.Request
-	Resp   http.ResponseWriter
-}
+type (
+	ValidateContext      = structs.ValidateContext
+	FormDefaultValidator = structs.FormDefaultValidator
+)
 
-// Form form binding interface
 type Form interface {
-	binding.Validator
+	Validate(ctx *ValidateContext, errs binding.Errors) binding.Errors
 }
 
 func init() {
@@ -84,9 +79,17 @@ func getFieldDisplayNameForMessage(f Form, l translation.Locale, fieldNames []st
 		typ = typ.Elem()
 	}
 
-	field, fieldExists := typ.FieldByName(fieldNames[0])
+	fieldName := fieldNames[0]
+	field, fieldExists := typ.FieldByName(fieldName)
 	if !fieldExists {
-		return field, false, ""
+		for tryField := range typ.Fields() {
+			if util.ToSnakeCase(tryField.Name) == fieldName || tryField.Tag.Get("form") == fieldName {
+				field, fieldExists = tryField, true
+			}
+		}
+		if !fieldExists {
+			return field, false, ""
+		}
 	}
 
 	if field.Tag.Get("form") == "-" {
@@ -95,8 +98,9 @@ func getFieldDisplayNameForMessage(f Form, l translation.Locale, fieldNames []st
 
 	trKeyFallback := "form." + field.Name
 	trKey := util.IfZero(field.Tag.Get("locale"), trKeyFallback)
-	displayName = l.TrString(trKey)
-	if displayName == trKeyFallback {
+	if l.HasKey(trKey) {
+		displayName = l.TrString(trKey)
+	} else {
 		displayName = field.Name
 	}
 	return field, true, displayName
@@ -156,7 +160,7 @@ func BuildValidationErrorForUser(f Form, l translation.Locale, bindingErrs bindi
 	case validation.ErrInvalidBadgeSlug:
 		errorMessage = l.TrString("form.invalid_slug_error", fieldDisplayName)
 	default:
-		setting.PanicInDevOrTesting("unknown binding error classification: %v", classification)
+		setting.PanicInDevOrTesting("unknown binding error classification for field %T.%s: %v, err: %s", f, errorFieldName, classification, bindingErrMsg)
 		var msg string
 		if classification != "" && bindingErrMsg != "" {
 			msg = classification + ": " + bindingErrMsg
@@ -171,33 +175,3 @@ func BuildValidationErrorForUser(f Form, l translation.Locale, bindingErrs bindi
 	}
 	return errorMessage, errorFieldName, fieldNames
 }
-
-type contextKeySkipTmplFormValidationErrorType struct{}
-
-var contextKeySkipTmplFormValidationError contextKeySkipTmplFormValidationErrorType
-
-func SkipTmplFormValidationError(ctx reqctx.RequestContext) {
-	ctx.SetContextValue(contextKeySkipTmplFormValidationError, true)
-}
-
-func Validate(ctx *ValidateContext, errs binding.Errors, f Form) binding.Errors {
-	if ctx.Req.Context().Value(contextKeySkipTmplFormValidationError) == true {
-		// if it is not using tmpl-based validation error handling, just return the errors
-		// for example: when using "form-fetch-action", the validation error can be handled by GetFetchActionForm
-		return errs
-	}
-	errorMessage, errorFieldName, _ := BuildValidationErrorForUser(f, ctx.Locale, errs)
-	if errorMessage == "" {
-		return errs
-	}
-
-	// Legacy template error handling: try to restore the form's values as much as possible,
-	// especially for RenderWithErrDeprecated to re-render the form with errors.
-	AssignForm(f, ctx.Data)
-	ctx.Data["HasError"] = true
-	ctx.Data["ErrorMsg"] = errorMessage
-	if errorFieldName != "" {
-		ctx.Data["Err_"+errorFieldName] = true
-	}
-	return errs
-}

modules/web/middleware/binding_test.go

@@ -15,17 +15,16 @@ import (
 )
 
 type testRangeForm struct {
+	FormDefaultValidator
 	Hours int `binding:"Range(0,1000)"`
 }
 
-func (f *testRangeForm) Validate(_ *http.Request, errs binding.Errors) binding.Errors { return errs }
-
 func TestBuildValidationErrorForUser(t *testing.T) {
 	// an out-of-range value must reach its own message instead of the panicking "default" branch
 	form := &testRangeForm{Hours: 2000}
 	errs := binding.Validate(httptest.NewRequest(http.MethodPost, "/", nil), form)
 	errorMessage, errorFieldName, fieldNames := BuildValidationErrorForUser(form, translation.MockLocale{}, errs)
-	assert.Equal(t, "form.range_error:Hours,0,1000", errorMessage)
+	assert.Equal(t, "form.range_error:form.Hours,0,1000", errorMessage)
 	assert.Equal(t, "Hours", errorFieldName)
 	assert.Equal(t, []string{"Hours"}, fieldNames)
 }

modules/web/router.go

@@ -14,6 +14,7 @@ import (
 	"gitea.dev/modules/public"
 	"gitea.dev/modules/reqctx"
 	"gitea.dev/modules/setting"
+	"gitea.dev/modules/translation"
 	"gitea.dev/modules/web/middleware"
 	"gitea.dev/modules/web/types"
 
@@ -21,14 +22,34 @@ import (
 	"github.com/go-chi/chi/v5"
 )
 
-// Bind binding an obj to a handler's context data
-func Bind[T any](_ T) http.HandlerFunc {
+// Bind binding the request form to a form object and assign context data
+func Bind[T interface {
+	*E
+	middleware.Form
+}, E any]() http.HandlerFunc {
 	return func(resp http.ResponseWriter, req *http.Request) {
-		theObj := new(T) // create a new form obj for every request but not use obj directly
-		data := middleware.GetContextData(req.Context())
-		_ = binding.Bind(req, theObj) // no need to handle "errs" here, the errors are handled in our middleware.Validate (binding.go)
-		SetForm(data, theObj)
-		middleware.AssignForm(theObj, data)
+		ctx := reqctx.FromContext(req.Context())
+		data := ctx.GetData()
+		locale := ctx.Value(translation.ContextKey).(translation.Locale) //nolint:forcetypeassert // must exist
+
+		obj := new(E)
+		var form T = obj
+		vctx := &middleware.ValidateContext{Locale: locale, Data: data, Req: req, Resp: resp}
+		errs := binding.Bind(req, obj)
+		errs = form.Validate(vctx, errs)
+		SetForm(data, obj)
+
+		// Legacy template error handling: try to restore the form's values as much as possible,
+		// especially for RenderWithErrDeprecated to re-render the form with errors.
+		middleware.AssignForm(obj, data)
+		errorMessage, errorFieldName, _ := middleware.BuildValidationErrorForUser(form, locale, errs)
+		if errorMessage != "" {
+			data["HasError"] = true
+			data["ErrorMsg"] = errorMessage
+			if errorFieldName != "" {
+				data["Err_"+errorFieldName] = true
+			}
+		}
 	}
 }