go-macaron / binding

Package binding is a middleware that provides request data binding and validation for Macaron.
Apache License 2.0
23 stars 17 forks source link

Rule.isValid param Errors as value #9

Closed atroshkov closed 8 years ago

atroshkov commented 8 years ago

Errors not modified in custom rules since the parameter is passed as a value

unknwon commented 8 years ago

Hi, can you give an example?

unknwon commented 8 years ago

Hmm... I see the problem. So to fix this, we need an API break change to return two values bool, Errors.

atroshkov commented 8 years ago
package models

import (
    "regexp"

    "github.com/go-macaron/binding"
    "gopkg.in/macaron.v1"
)

func init() {
    userNameRegexp, err := regexp.Compile("^[a-zA-Z0-9]+$")
    if err != nil {
        panic(err)
    }

    binding.AddRule(&binding.Rule{
        IsMatch: func(rule string) bool {
            return rule == "UserName"
        },
        IsValid: func(errs binding.Errors, name string, v interface{}) bool {
            if !userNameRegexp.MatchString(v.(string)) {
                errs.Add([]string{name}, "UserNameFormat", "format")
                // here errs = [format]
                return false
            }
            return true
        },
    })
}

type UserNameRequest struct {
    UserName string `binding:"UserName"`
}

func (r UserNameRequest) Error(ctx *macaron.Context, errs binding.Errors) {
    // but here errs = [], even if the regexp does not match
}
atroshkov commented 8 years ago

May be:

Rule struct {
    // IsMatch checks if rule matches.
    IsMatch func(string) bool
    // IsValid applies validation rule to condition.
    IsValid func(Errors, string, interface{}) bool
}
...
if ruleMapper[i].IsMatch(rule) && !ruleMapper[i].IsValid(errors, field.Name, fieldValue) {
    break VALIDATE_RULES
}

to:

Rule struct {
    // IsMatch checks if rule matches.
    IsMatch func(string) bool
    // IsValid applies validation rule to condition.
    IsValid func(*Errors, string, interface{}) bool
}
...
if ruleMapper[i].IsMatch(rule) && !ruleMapper[i].IsValid(&errors, field.Name, fieldValue) {
    break VALIDATE_RULES
}
unknwon commented 8 years ago

Thanks, since it's going to have an API break change anyway, I'm going to return two values bool, Errors.

unknwon commented 8 years ago

OK, fix has been pushed, please update the package and test.

atroshkov commented 8 years ago

Thanks. All works fine.

atroshkov commented 8 years ago

IsValid executed before IsMatch and IsValid change errors, even if IsMatch return false. Please replace:

default:
    // Apply custom validation rules.
    var isValid bool
    for i := range ruleMapper {
        isValid, errors = ruleMapper[i].IsValid(errors, field.Name, fieldValue)
        if ruleMapper[i].IsMatch(rule) && !isValid {
            break VALIDATE_RULES
        }
    }

to:

default:
    // Apply custom validation rules.
    var isValid bool
    for i := range ruleMapper {
        if ruleMapper[i].IsMatch(rule) {
            isValid, errors = ruleMapper[i].IsValid(errors, field.Name, fieldValue)
            if !isValid {
                break VALIDATE_RULES
            }
        }
    }
unknwon commented 8 years ago

Changes pushed.

atroshkov commented 8 years ago

Thanks