Struct experiment (#1150)

## PR This PR does the following: - Reverts #1122 - Re-implements struct level validations in a different way which also support `or`s etc.. which previous implementation did not. - Adds special case to ignore `required` validation on non-pointer structs to preserve pre-struct level tag validation support. - Added new `WithRequiredStructEnabled` option to opt-in to this new behaviour, that will become the default in the next major version.
go-playground · Aug 29, 2023 · 9b7c4de · 9b7c4de
1 parent 3094d59
commit 9b7c4de
Show file tree

Hide file tree

Showing 12 changed files with 212 additions and 185 deletions.
diff --git a/.github/workflows/workflow.yml b/.github/workflows/workflow.yml
@@ -8,7 +8,7 @@ jobs:
   test:
     strategy:
       matrix:
-        go-version: [1.20.x]
+        go-version: [1.17.x,1.18.x,1.21.x]
         os: [ubuntu-latest, macos-latest, windows-latest]
     runs-on: ${{ matrix.os }}
     steps:
@@ -32,7 +32,7 @@ jobs:
         run: go test -race -covermode=atomic -coverprofile="profile.cov" ./...
 
       - name: Send Coverage
-        if: matrix.os == 'ubuntu-latest' && matrix.go-version == '1.20.x'
+        if: matrix.os == 'ubuntu-latest' && matrix.go-version == '1.21.x'
         uses: shogo82148/actions-goveralls@v1
         with:
           path-to-profile: profile.cov
@@ -43,7 +43,7 @@ jobs:
     steps:
       - uses: actions/setup-go@v3
         with:
-          go-version: 1.19.x
+          go-version: 1.21.x
       - uses: actions/checkout@v3
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v3

diff --git a/Makefile b/Makefile
@@ -13,6 +13,6 @@ test:
 	$(GOCMD) test -cover -race ./...
 
 bench:
-	$(GOCMD) test -bench=. -benchmem ./...
+	$(GOCMD) test -run=NONE -bench=. -benchmem ./...
 
 .PHONY: test lint linters-install
diff --git a/README.md b/README.md
diff --git a/_examples/simple/main.go b/_examples/simple/main.go
@@ -30,7 +30,7 @@ var validate *validator.Validate
 
 func main() {
 
-	validate = validator.New()
+	validate = validator.New(validator.WithRequiredStructEnabled())
 
 	validateStruct()
 	validateVariable()

diff --git a/baked_in.go b/baked_in.go
@@ -23,7 +23,7 @@ import (
 	"golang.org/x/text/language"
 
 	"github.com/gabriel-vasile/mimetype"
-	"github.com/leodido/go-urn"
+	urn "github.com/leodido/go-urn"
 )
 
 // Func accepts a FieldLevel interface for all validation needs. The return

diff --git a/cache.go b/cache.go
@@ -20,7 +20,6 @@ const (
 	typeOr
 	typeKeys
 	typeEndKeys
-	typeNestedStructLevel
 )
 
 const (
@@ -153,7 +152,7 @@ func (v *Validate) extractStructCache(current reflect.Value, sName string) *cStr
 		// and so only struct level caching can be used instead of combined with Field tag caching
 
 		if len(tag) > 0 {
-			ctag, _ = v.parseFieldTagsRecursive(tag, fld, "", false)
+			ctag, _ = v.parseFieldTagsRecursive(tag, fld.Name, "", false)
 		} else {
 			// even if field doesn't have validations need cTag for traversing to potential inner/nested
 			// elements of the field.
@@ -172,7 +171,7 @@ func (v *Validate) extractStructCache(current reflect.Value, sName string) *cStr
 	return cs
 }
 
-func (v *Validate) parseFieldTagsRecursive(tag string, field reflect.StructField, alias string, hasAlias bool) (firstCtag *cTag, current *cTag) {
+func (v *Validate) parseFieldTagsRecursive(tag string, fieldName string, alias string, hasAlias bool) (firstCtag *cTag, current *cTag) {
 	var t string
 	noAlias := len(alias) == 0
 	tags := strings.Split(tag, tagSeparator)
@@ -186,9 +185,9 @@ func (v *Validate) parseFieldTagsRecursive(tag string, field reflect.StructField
 		// check map for alias and process new tags, otherwise process as usual
 		if tagsVal, found := v.aliases[t]; found {
 			if i == 0 {
-				firstCtag, current = v.parseFieldTagsRecursive(tagsVal, field, t, true)
+				firstCtag, current = v.parseFieldTagsRecursive(tagsVal, fieldName, t, true)
 			} else {
-				next, curr := v.parseFieldTagsRecursive(tagsVal, field, t, true)
+				next, curr := v.parseFieldTagsRecursive(tagsVal, fieldName, t, true)
 				current.next, current = next, curr
 
 			}
@@ -236,7 +235,7 @@ func (v *Validate) parseFieldTagsRecursive(tag string, field reflect.StructField
 				}
 			}
 
-			current.keys, _ = v.parseFieldTagsRecursive(string(b[:len(b)-1]), field, "", false)
+			current.keys, _ = v.parseFieldTagsRecursive(string(b[:len(b)-1]), fieldName, "", false)
 			continue
 
 		case endKeysTag:
@@ -285,18 +284,14 @@ func (v *Validate) parseFieldTagsRecursive(tag string, field reflect.StructField
 
 				current.tag = vals[0]
 				if len(current.tag) == 0 {
-					panic(strings.TrimSpace(fmt.Sprintf(invalidValidation, field.Name)))
+					panic(strings.TrimSpace(fmt.Sprintf(invalidValidation, fieldName)))
 				}
 
 				if wrapper, ok := v.validations[current.tag]; ok {
 					current.fn = wrapper.fn
 					current.runValidationWhenNil = wrapper.runValidatinOnNil
 				} else {
-					panic(strings.TrimSpace(fmt.Sprintf(undefinedValidation, current.tag, field.Name)))
-				}
-
-				if current.typeof == typeDefault && isNestedStructOrStructPtr(field) {
-					current.typeof = typeNestedStructLevel
+					panic(strings.TrimSpace(fmt.Sprintf(undefinedValidation, current.tag, fieldName)))
 				}
 
 				if len(orVals) > 1 {
@@ -324,7 +319,7 @@ func (v *Validate) fetchCacheTag(tag string) *cTag {
 		// isn't parsed again.
 		ctag, found = v.tagCache.Get(tag)
 		if !found {
-			ctag, _ = v.parseFieldTagsRecursive(tag, reflect.StructField{}, "", false)
+			ctag, _ = v.parseFieldTagsRecursive(tag, "", "", false)
 			v.tagCache.Set(tag, ctag)
 		}
 	}

diff --git a/doc.go b/doc.go
@@ -247,7 +247,7 @@ Example #2
 This validates that the value is not the data types default zero value.
 For numbers ensures value is not zero. For strings ensures value is
 not "". For slices, maps, pointers, interfaces, channels and functions
-ensures the value is not nil. For structs ensures value is not the zero value.
+ensures the value is not nil. For structs ensures value is not the zero value when using WithRequiredStructEnabled.
 
 	Usage: required
 

diff --git a/options.go b/options.go
@@ -0,0 +1,16 @@
+package validator
+
+// Option represents a configurations option to be applied to validator during initialization.
+type Option func(*Validate)
+
+// WithRequiredStructEnabled enables required tag on non-pointer structs to be applied instead of ignored.
+//
+// This was made opt-in behaviour in order to maintain backward compatibility with the behaviour previous
+// to being able to apply struct level validations on struct fields directly.
+//
+// It is recommended you enabled this as it will be the default behaviour in v11+
+func WithRequiredStructEnabled() Option {
+	return func(v *Validate) {
+		v.requiredStructEnabled = true
+	}
+}
diff --git a/util.go b/util.go
@@ -292,11 +292,3 @@ func panicIf(err error) {
 		panic(err.Error())
 	}
 }
-
-func isNestedStructOrStructPtr(v reflect.StructField) bool {
-	if v.Type == nil {
-		return false
-	}
-	kind := v.Type.Kind()
-	return kind == reflect.Struct || kind == reflect.Ptr && v.Type.Elem().Kind() == reflect.Struct
-}
diff --git a/validator.go b/validator.go
@@ -99,6 +99,8 @@ func (v *validate) traverseField(ctx context.Context, parent reflect.Value, curr
 
 	current, kind, v.fldIsPointer = v.extractTypeInternal(current, false)
 
+	var isNestedStruct bool
+
 	switch kind {
 	case reflect.Ptr, reflect.Interface, reflect.Invalid:
 
@@ -161,85 +163,56 @@ func (v *validate) traverseField(ctx context.Context, parent reflect.Value, curr
 		}
 
 	case reflect.Struct:
-
-		typ = current.Type()
-
-		if !typ.ConvertibleTo(timeType) {
-
-			if ct != nil {
-
-				if ct.typeof == typeStructOnly {
-					goto CONTINUE
-				} else if ct.typeof == typeIsDefault || ct.typeof == typeNestedStructLevel {
-					// set Field Level fields
-					v.slflParent = parent
-					v.flField = current
-					v.cf = cf
-					v.ct = ct
-
-					if !ct.fn(ctx, v) {
-						v.str1 = string(append(ns, cf.altName...))
-
-						if v.v.hasTagNameFunc {
-							v.str2 = string(append(structNs, cf.name...))
-						} else {
-							v.str2 = v.str1
-						}
-
-						v.errs = append(v.errs,
-							&fieldError{
-								v:              v.v,
-								tag:            ct.aliasTag,
-								actualTag:      ct.tag,
-								ns:             v.str1,
-								structNs:       v.str2,
-								fieldLen:       uint8(len(cf.altName)),
-								structfieldLen: uint8(len(cf.name)),
-								value:          current.Interface(),
-								param:          ct.param,
-								kind:           kind,
-								typ:            typ,
-							},
-						)
-						return
-					}
-				}
-
-				ct = ct.next
-			}
-
-			if ct != nil && ct.typeof == typeNoStructLevel {
-				return
-			}
-
-		CONTINUE:
-			// if len == 0 then validating using 'Var' or 'VarWithValue'
-			// Var - doesn't make much sense to do it that way, should call 'Struct', but no harm...
-			// VarWithField - this allows for validating against each field within the struct against a specific value
-			//                pretty handy in certain situations
-			if len(cf.name) > 0 {
-				ns = append(append(ns, cf.altName...), '.')
-				structNs = append(append(structNs, cf.name...), '.')
-			}
-
-			v.validateStruct(ctx, parent, current, typ, ns, structNs, ct)
-			return
+		isNestedStruct = !current.Type().ConvertibleTo(timeType)
+		// For backward compatibility before struct level validation tags were supported
+		// as there were a number of projects relying on `required` not failing on non-pointer
+		// structs. Since it's basically nonsensical to use `required` with a non-pointer struct
+		// are explicitly skipping the required validation for it. This WILL be removed in the
+		// next major version.
+		if !v.v.requiredStructEnabled && ct != nil && ct.tag == requiredTag {
+			ct = ct.next
 		}
 	}
 
-	if ct == nil || !ct.hasTag {
-		return
-	}
-
 	typ = current.Type()
 
 OUTER:
 	for {
-		if ct == nil {
+		if ct == nil || !ct.hasTag || (isNestedStruct && len(cf.name) == 0) {
+			// isNestedStruct check here
+			if isNestedStruct {
+				// if len == 0 then validating using 'Var' or 'VarWithValue'
+				// Var - doesn't make much sense to do it that way, should call 'Struct', but no harm...
+				// VarWithField - this allows for validating against each field within the struct against a specific value
+				//                pretty handy in certain situations
+				if len(cf.name) > 0 {
+					ns = append(append(ns, cf.altName...), '.')
+					structNs = append(append(structNs, cf.name...), '.')
+				}
+
+				v.validateStruct(ctx, parent, current, typ, ns, structNs, ct)
+			}
 			return
 		}
 
 		switch ct.typeof {
+		case typeNoStructLevel:
+			return
+
+		case typeStructOnly:
+			if isNestedStruct {
+				// if len == 0 then validating using 'Var' or 'VarWithValue'
+				// Var - doesn't make much sense to do it that way, should call 'Struct', but no harm...
+				// VarWithField - this allows for validating against each field within the struct against a specific value
+				//                pretty handy in certain situations
+				if len(cf.name) > 0 {
+					ns = append(append(ns, cf.altName...), '.')
+					structNs = append(append(structNs, cf.name...), '.')
+				}
+
+				v.validateStruct(ctx, parent, current, typ, ns, structNs, ct)
+			}
+			return
 
 		case typeOmitEmpty:
 
@@ -366,7 +339,7 @@ OUTER:
 						ct = ct.next
 
 						if ct == nil {
-							return
+							continue OUTER
 						}
 
 						if ct.typeof != typeOr {

diff --git a/validator_instance.go b/validator_instance.go
@@ -79,27 +79,28 @@ type internalValidationFuncWrapper struct {
 
 // Validate contains the validator settings and cache
 type Validate struct {
-	tagName          string
-	pool             *sync.Pool
-	hasCustomFuncs   bool
-	hasTagNameFunc   bool
-	tagNameFunc      TagNameFunc
-	structLevelFuncs map[reflect.Type]StructLevelFuncCtx
-	customFuncs      map[reflect.Type]CustomTypeFunc
-	aliases          map[string]string
-	validations      map[string]internalValidationFuncWrapper
-	transTagFunc     map[ut.Translator]map[string]TranslationFunc // map[<locale>]map[<tag>]TranslationFunc
-	rules            map[reflect.Type]map[string]string
-	tagCache         *tagCache
-	structCache      *structCache
+	tagName               string
+	pool                  *sync.Pool
+	tagNameFunc           TagNameFunc
+	structLevelFuncs      map[reflect.Type]StructLevelFuncCtx
+	customFuncs           map[reflect.Type]CustomTypeFunc
+	aliases               map[string]string
+	validations           map[string]internalValidationFuncWrapper
+	transTagFunc          map[ut.Translator]map[string]TranslationFunc // map[<locale>]map[<tag>]TranslationFunc
+	rules                 map[reflect.Type]map[string]string
+	tagCache              *tagCache
+	structCache           *structCache
+	hasCustomFuncs        bool
+	hasTagNameFunc        bool
+	requiredStructEnabled bool
 }
 
 // New returns a new instance of 'validate' with sane defaults.
 // Validate is designed to be thread-safe and used as a singleton instance.
 // It caches information about your struct and validations,
 // in essence only parsing your validation tags once per struct type.
 // Using multiple instances neglects the benefit of caching.
-func New() *Validate {
+func New(options ...Option) *Validate {
 
 	tc := new(tagCache)
 	tc.m.Store(make(map[string]*cTag))
@@ -146,6 +147,9 @@ func New() *Validate {
 		},
 	}
 
+	for _, o := range options {
+		o(v)
+	}
 	return v
 }