go-ozzo / ozzo-validation

An idiomatic Go (golang) validation package. Supports configurable and extensible validation rules (validators) using normal language constructs instead of error-prone struct tags.
MIT License
3.73k stars 224 forks source link

Validate with zero number is not work #183

Closed tranlongan051020 closed 1 year ago

tranlongan051020 commented 1 year ago

I encountered an issue where the console log shows "Validation successful!" for the given Go code, which uses the github.com/go-ozzo/ozzo-validation/v4 library for validation:

package main

import (
    "fmt"

    validation "github.com/go-ozzo/ozzo-validation/v4"
    "github.com/samber/lo"
)

type User struct {
    Number1 int
    Number2 int
    Number3 *int
    Number4 *int
}

func (u User) Validate() error {
    return validation.ValidateStruct(&u,
        validation.Field(&u.Number1, validation.Max(-1)),
        validation.Field(&u.Number2, validation.Min(1)),
        validation.Field(&u.Number3, validation.Max(-1)),
        validation.Field(&u.Number4, validation.Min(1)),
    )
}

func main() {
    user := User{
        Number1: 0,
        Number2: 0,
        Number3: lo.ToPtr(0),
        Number4: lo.ToPtr(0),
    }

    err := user.Validate()
    if err != nil {
        fmt.Println("Validation error:", err)
        return
    }

    fmt.Println("Validation successful!")
}

Upon further investigation, I discovered that the library's validation with zero values has some issues. Can you provide guidance on how to resolve this?

anjolaoluwaakindipe commented 1 year ago

@tranlongan051020 After looking into this, I noticed that the Min and Max methods of the library skips validation checks if the value being validated is zero. This could be a design decision from the maintainers, but I'm not too sure. I cloned the repository and noticed that the fix for your scenario would be to remove the IsEmpty() function from the ThresholdRule Validation method. I also agree that validating zero within a min and max threshold should be treated like any other number

Original Code (minmax.go)

...
func (r ThresholdRule) Validate(value interface{}) error {
    value, isNil := Indirect(value)

    if isNil || IsEmpty(value) { // <---- What's causing the problem
        return nil
    }

    rv := reflect.ValueOf(r.threshold)
    switch rv.Kind() {
    case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
        v, err := ToInt(value)
        if err != nil {
            return err
        }
        if r.compareInt(rv.Int(), v) {
            return nil
        }

    case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
        v, err := ToUint(value)
        if err != nil {
            return err
        }
        if r.compareUint(rv.Uint(), v) {
            return nil
        }

    ...
}
...

Proposed way to solve it

...
func (r ThresholdRule) Validate(value interface{}) error {
    value, isNil := Indirect(value)

    if isNil { // <--- remove check
        return nil
    }

    rv := reflect.ValueOf(r.threshold)
    switch rv.Kind() {
    case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
        v, err := ToInt(value)
        if err != nil {
            return err
        }
        if r.compareInt(rv.Int(), v) {
            return nil
        }

    case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
        v, err := ToUint(value)
        if err != nil {
            return err
        }
        if r.compareUint(rv.Uint(), v) {
            return nil
        }

...
}
...

What isEmpty() is doing

func IsEmpty(value interface{}) bool {
    v := reflect.ValueOf(value)
    switch v.Kind() {
    case reflect.String, reflect.Array, reflect.Map, reflect.Slice:
        return v.Len() == 0
    case reflect.Bool:
        return !v.Bool()
    case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
        return v.Int() == 0
    case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
        return v.Uint() == 0
    case reflect.Float32, reflect.Float64:
        return v.Float() == 0
    case reflect.Invalid:
        return true
    case reflect.Interface, reflect.Ptr:
        if v.IsNil() {
            return true
        }
        return IsEmpty(v.Elem().Interface())
    case reflect.Struct:
        v, ok := value.(time.Time)
        if ok && v.IsZero() {
            return true
        }
    }

    return false
}
Sergei8888 commented 1 year ago

Fix needed

Xyedo commented 1 year ago

i think what the maintainer meant is if its empty (default value or nil), then its optional, so it skip the validation, so we have to use validation.Required for default value or nil, or if you want to use only if the pointer is not nil, you can use validation.NotNil

func (u User) Validate() error {
    return validation.ValidateStruct(&u,
        validation.Field(&u.Number1, validation.Required, validation.Max(-1)),
        validation.Field(&u.Number2, validation.Required, validation.Min(1)),
        validation.Field(&u.Number3, validation.NotNil, validation.Max(-1)),
        validation.Field(&u.Number4, validation.NotNil, validation.Min(1)),
    )
}

and if you run the code, youll get

Validation error: Number1: cannot be blank; Number2: cannot be blank.