refactor: clean up form binding & validation - #38873
Open
wxiaoguang wants to merge 3 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors web form binding/validation to remove duplicated per-form Validate(*http.Request, ...) methods, centralizing validation in web.Bind with a shared ValidateContext and a default validator implementation.
Changes:
- Introduces
structs.ValidateContext/structs.FormDefaultValidatorand updates middleware types to use them. - Migrates many forms to embed
middleware.FormDefaultValidatorand updates routes to use the newweb.Bind[*T]()generic binder. - Adjusts validation error display name resolution to use
Locale.HasKey(...)and updates translation interfaces accordingly.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/forms/user_form.go | Embed default validator across user-related forms; removes duplicated Validate methods; keeps a few custom validations. |
| services/forms/user_form_auth_openid.go | Embed default validator for OpenID auth forms; removes duplicated Validate methods. |
| services/forms/runner.go | Embed default validator for runner edit form; removes duplicated Validate. |
| services/forms/repo_tag_form.go | Embed default validator for tag protection form; removes duplicated Validate. |
| services/forms/repo_form.go | Embed default validator across many repo forms; adjusts a couple of custom validations to new signature. |
| services/forms/repo_form_editor.go | Embed default validator for editor-related forms; removes duplicated Validate. |
| services/forms/repo_branch_form.go | Embed default validator for branch forms; removes duplicated Validate. |
| services/forms/package_form.go | Embed default validator for package cleanup rule form; removes duplicated Validate. |
| services/forms/org.go | Embed default validator for org/team forms; removes duplicated Validate. |
| services/forms/auth_form.go | Embed default validator for auth source form; removes duplicated Validate. |
| services/forms/admin.go | Embed default validator for admin forms; removes duplicated Validate. |
| services/context/context.go | Updates fetch-action form validation path (notably removes the old skip mechanism). |
| routers/web/web.go | Migrates many routes to web.Bind[*Form]() for consistent binding/validation. |
| routers/web/repo/actions/view.go | Adds default validator embedding to ViewRequest used with web.Bind. |
| routers/web/auth/oauth2_provider.go | Removes manual validation handling and relies on binder-provided validation state. |
| routers/install/routes.go | Migrates install POST route to web.Bind[*forms.InstallForm](). |
| modules/web/router.go | Reworks Bind into a generic binder that validates, invokes form validation hook, and populates legacy template error fields. |
| modules/web/middleware/binding.go | Moves ValidateContext/default validator to structs; updates display-name logic to use HasKey. |
| modules/web/middleware/binding_test.go | Adapts middleware binding tests to the new default validator embedding. |
| modules/validation/validurl_test.go | Simplifies expected empty-error cases (leans on nil/zero defaults). |
| modules/validation/regex_pattern_test.go | Simplifies expected empty-error cases. |
| modules/validation/refname_test.go | Simplifies expected empty-error cases. |
| modules/validation/glob_pattern_test.go | Simplifies expected empty-error cases. |
| modules/validation/binding_test.go | Removes nil-to-empty normalization and compares directly with expected values. |
| modules/translation/translation.go | Changes translation.Locale to an alias of the new locale-translation interface. |
| modules/translation/mock.go | Adds HasKey implementation for the mock locale. |
| modules/translation/i18n/i18n.go | Introduces LocaleTranslation interface (adds HasKey to translation-capable locales). |
| modules/structs/miscellaneous.go | Adds default validator embedding to structs.MarkupOption for binder compatibility. |
| modules/structs/form.go | New shared home for ValidateContext and FormDefaultValidator. |
Suppressed comments (2)
services/context/context.go:307
- GetFetchActionForm binds and builds an error response, but it no longer invokes the form's Validate(ctx, errs) hook. Since forms have been migrated away from go-chi/binding.Validator, any custom validation (e.g. adding errors via middleware.AddValidationError) will be skipped for fetch-action forms unless this function calls form.Validate explicitly.
func GetFetchActionForm[T interface {
*E
middleware.Form
}, E any](ctx *Context) *E {
if web.IsFormSet(ctx) {
panic("don't mix fetch-action form validation with template-based form validation")
}
form := T(new(E))
errs := binding.Bind(ctx.Req, form)
errorMessage, fieldName, _ := middleware.BuildValidationErrorForUser(form, ctx.Locale, errs)
if errorMessage != "" {
ctx.Resp.Header().Set("Content-Type", "application/json")
ctx.JSONErrorWithField(errorMessage, fieldName)
return nil
}
return form
services/forms/auth_form.go:95
- The AuthenticationForm.Validate method was removed, but its doc comment remains. This leaves an orphaned comment at the end of the file and suggests a missing declaration; it should be deleted.
// Validate validates fields
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wxiaoguang
force-pushed
the
refactor-form
branch
from
August 11, 2026 13:34
24328e0 to
121a33b
Compare
wxiaoguang
force-pushed
the
refactor-form
branch
from
August 11, 2026 14:22
121a33b to
bd57ae8
Compare
bircni
approved these changes
Aug 11, 2026
delvh
approved these changes
Aug 13, 2026
wxiaoguang
enabled auto-merge (squash)
August 14, 2026 01:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clarify the "validation" and "error display" logic.
All the copied&pasted
Validatefunctions are removed.