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

Export struct sliceValidateError to allow error casting #2777

Merged
merged 1 commit into from Nov 21, 2021
Merged

Export struct sliceValidateError to allow error casting #2777

merged 1 commit into from Nov 21, 2021

Conversation

edebernis
Copy link
Contributor

This PR allows to cast error returned by ShouldBindWith functions when input parameter is a slice.

daheige
daheige previously approved these changes Jul 12, 2021
@kszafran
Copy link
Contributor

kszafran commented Aug 6, 2021

@appleboy @thinkerou Can we get this merged? I also found myself needing this. Without it, I have to iterate over slices manually, so that I can access the individual validation errors.

@daheige
Copy link
Contributor

daheige commented Aug 6, 2021 via email

@edebernis
Copy link
Contributor Author

@appleboy @thinkerou Any chance to get this merged ? Thanks !

@appleboy appleboy added this to the v1.8 milestone Sep 21, 2021
@appleboy
Copy link
Member

need @thinkerou's approval.

@kszafran
Copy link
Contributor

kszafran commented Sep 21, 2021

I would actually advice against merging this. I have an alternative idea and could contribute a PR. I'm playing with it right now, validator has a method called Var ((*validator.Validate).Var(...)), which can take a slice or an array. If we use it instead of exposing a custom error type, then all errors returned by bindings will be consistent.

I'm also adding a feature allowing configuring validation tags for map/slice types.

@kszafran
Copy link
Contributor

kszafran commented Sep 21, 2021

Here is a proof of concept. Registering type tags is similar to registering struct-level validators:

var typeTags = make(map[reflect.Type]string)

// RegisterTypeTag registers a validator tag for a map or slice type. For example:
//     infra.RegisterMapTag(api.MyMap{}, "gt=3,dive")
// where api.MyMap is a map type, e.g.
//     type MyMap map[string]int
// If you register a tag, you have to include "dive" to validate map/slice elements.
func RegisterTypeTag(typ interface{}, tag string) {
	// TODO panic if the type is not one of: Slice, Array, Map
	typeTags[reflect.TypeOf(typ)] = tag
}

type defaultValidator struct {
	once     sync.Once
	validate *validator.Validate
}

var _ binding.StructValidator = &defaultValidator{}

func (v *defaultValidator) ValidateStruct(obj interface{}) error {
	if obj == nil {
		return nil
	}

	value := reflect.ValueOf(obj)
	switch value.Kind() {
	case reflect.Ptr:
		return v.ValidateStruct(value.Elem().Interface())
	case reflect.Struct:
		return v.validateStruct(obj)
	case reflect.Slice, reflect.Array, reflect.Map:
		if tag, ok := typeTags[value.Type()]; ok {
			return v.validateVar(obj, tag)
		} else {
			return v.validateVar(obj, "dive")
		}
	default:
		return nil
	}
}

func (v *defaultValidator) validateStruct(obj interface{}) error {
	v.lazyinit()
	return v.validate.Struct(obj)
}

func (v *defaultValidator) validateVar(obj interface{}, tag string) error {
	v.lazyinit()
	return v.validate.Var(obj, tag)
}

func (v *defaultValidator) Engine() interface{} {
	v.lazyinit()
	return v.validate
}

func (v *defaultValidator) lazyinit() {
	v.once.Do(func() {
		v.validate = validator.New()
		v.validate.SetTagName("binding")
	})
}

The benefits are:

  1. you always get the same kind of error, whether you validate struct, slice, or map
  2. you get map validation (not possible currently)
  3. you can define custom tags for top-level slices and maps, for example:
binding.RegisterTypeTag(MyMap{}, "dive,keys,gt=5,endkeys,dive")

This allows you to validate top-level map keys (not possible currently), as well as top-level slices (also not possible):

binding.RegisterTypeTag(MySlice{}, "gt=4,dive")

If we merge this PR and export SliceValidationError, then clients will obviously start to depend on it, and we won't be able to implement this solution, which is more consistent and powerful in my opinion.

@kszafran
Copy link
Contributor

I can create a PR tomorrow. I can also see if I could implement contextual validation: #2741

@kszafran
Copy link
Contributor

Here you go: #2877 @appleboy @thinkerou

@kszafran
Copy link
Contributor

kszafran commented Nov 5, 2021

I have reworked my implementation. It's now more similar to the existing sliceValidateError: #2877

Copy link

@SubhramRana SubhramRana left a comment

Choose a reason for hiding this comment

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

hbvjhbjhbh

Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

lgtm

@thinkerou thinkerou merged commit 57ede9c into gin-gonic:master Nov 21, 2021
daheige pushed a commit to daheige/gin that referenced this pull request Apr 18, 2022
… as (gin-gonic#2777)

SliceValidationError

Co-authored-by: Emeric de Bernis <emeric.debernis@adevinta.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants