Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Map validation, contextual validation, validator tags for custom map/slice types #2877

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kszafran
Copy link
Contributor

@kszafran kszafran commented Sep 22, 2021

This pull request offers a few simple, but very useful improvements:

  1. Adds support for validating maps, including registering tags for map types (as well as slice types).
  2. Adds support for contextual validation. Custom validators (e.g. those registered with (*Validator).RegisterValidationCtx or (*Validator).RegisterStructValidationCtx) will be passed the Gin context (as context.Context). Resolves Access gin request Context from custom validators #2741
  3. Returns slice and map validation errors are regular validator.ValidationErrors, without any custom types. This way callers can handle all validation errors in the same manner. Related to: Export struct sliceValidateError to allow error casting #2777

A further improvement would be to merge this PR: #2825. That will enable getting the actual *gin.Context in custom validators, resolving #1489.

@kszafran kszafran changed the title Validator Map validation, contextual validation, validator tags for custom map/slice types Sep 22, 2021
@kszafran kszafran force-pushed the validator branch 2 times, most recently from 14b4deb to 7971f15 Compare September 22, 2021 09:41
@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #2877 (b626f63) into master (efa3175) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2877      +/-   ##
==========================================
+ Coverage   98.73%   98.76%   +0.02%     
==========================================
  Files          41       41              
  Lines        3080     3150      +70     
==========================================
+ Hits         3041     3111      +70     
  Misses         27       27              
  Partials       12       12              
Flag Coverage Δ
go-1.13 98.76% <100.00%> (+0.02%) ⬆️
go-1.14 98.60% <100.00%> (+0.03%) ⬆️
go-1.15 98.60% <100.00%> (+0.03%) ⬆️
go-1.16 98.60% <100.00%> (+0.03%) ⬆️
go-1.17 98.50% <100.00%> (+0.03%) ⬆️
macos-latest 98.76% <100.00%> (+0.02%) ⬆️
nomsgpack 98.74% <100.00%> (+0.02%) ⬆️
ubuntu-latest 98.76% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
binding/binding.go 100.00% <100.00%> (ø)
binding/binding_nomsgpack.go 100.00% <100.00%> (ø)
binding/default_validator.go 100.00% <100.00%> (ø)
binding/form.go 100.00% <100.00%> (ø)
binding/header.go 100.00% <100.00%> (ø)
binding/json.go 100.00% <100.00%> (ø)
binding/msgpack.go 100.00% <100.00%> (ø)
binding/query.go 100.00% <100.00%> (ø)
binding/uri.go 100.00% <100.00%> (ø)
binding/xml.go 100.00% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efa3175...b626f63. Read the comment docs.

@kszafran kszafran force-pushed the validator branch 3 times, most recently from c790e2a to c52063e Compare September 22, 2021 10:15
@kszafran
Copy link
Contributor Author

kszafran commented Sep 27, 2021

@appleboy @thinkerou Would you have the time to review this PR? I suggest looking at it commit by commit, since each commit is self-contained.

HenryFarias
HenryFarias previously approved these changes Oct 7, 2021
@kszafran
Copy link
Contributor Author

kszafran commented Nov 2, 2021

I did some renaming (Ctx -> Context) and fixed my omission in ShouldBindWith and ShouldBindBodyWith, which should call the contextual binding methods, but didn't. I also rebased onto latest master.

@kszafran
Copy link
Contributor Author

kszafran commented Nov 5, 2021

Turns out that this may break custom validator tags if they use FieldLevel.Top(). I'm working on a version which will avoid that.

This way callers can always expect ValidationErrors, and in case
of slice validation, they can also get indexes of the failing elements.
By passing the Gin context to bindings, custom validators can take
advantage of the information in the context.
@kszafran
Copy link
Contributor Author

kszafran commented Nov 5, 2021

I have reworked my implementation. Now it does not rely on Validate.Var/Validate.VarCtx, except for validating custom slice/array/map types. It will behave similar to the existing implementation, except the returned error is always of type validator.ValidationErrors and can be inspected to get the failing element index (or key in the case of maps).

@SubhramRana
Copy link

how?

@kszafran
Copy link
Contributor Author

kszafran commented Nov 8, 2021

how?

Hey, what exactly are you asking about?

@kszafran
Copy link
Contributor Author

I have found an edge case when validating non-nil pointers with nil value. It can be fixed by changing

	case reflect.Ptr:
		return v.ValidateStructContext(ctx, value.Elem().Interface())

to

	case reflect.Ptr:
		if value.IsNil() {
			return nil
		}
		return v.ValidateStructContext(ctx, value.Elem().Interface())

in ValidateStructContext.

I'm not updating the commits, though, because the maintainers don't seem to be particularly interested in this PR anyway.

}

for _, key := range value.MapKeys() {
if err := v.ValidateStructContext(ctx, value.MapIndex(key).Interface()); err != nil {
Copy link

@cikay cikay Jan 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be more clear to reverse the condition and add the continue statement since the indentation level will be decreased.

@lsdch
Copy link

lsdch commented Feb 18, 2024

Great feature, that will allow me to refactor a bunch of validation code. Hope this gets merged soon! 🙏

@kszafran
Copy link
Contributor Author

Great feature, that will allow me to refactor a bunch of validation code. Hope this gets merged soon! 🙏

This PR has been here for a long time. I kept it open just in case, but I'm not even updating it anymore. The chances of getting it merged seem slim. I could try to rebase it, though, if the maintainers are interested in merging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access gin request Context from custom validators
5 participants