Add Valuer interface support

[johnniedoe]

The Valuer interface in the sql/driver package seems like one of the primary ways for SQL drivers and ORMs to interact with databases, particularly when NULL values are a possibility.

It would be great if the validator could apply validations directly to Value() values in the case of a type that supports the interface. This would make validations on values of type sql.NullXXX much more straightforward.

BTW - Thanks for making this package. It's been very handy.

[deankarn]

I think I understand what you saying,

Would you be able to provide a quick code sample and I'll take a look.

P.S. are you using v5 or v6 of the library?

update: I think your talking about this Value of type interface https://golang.org/src/database/sql/driver/driver.go?s=566:588#L12
update oh I see this Valuer https://golang.org/src/database/sql/driver/types.go?s=1210:1293#L29

[deankarn]

Summary

OK so I did an implementation test for validating a type of Valuer and it all works except, that there is a performance hit and I'm not sure that this type of functionality should be within the library. The library validates data values, not data values from methods, I'm worried about opening a door to allow other interface types, and checking each is expensive, but usually less so in the calling program.

Benchmarks

ns/op and B/op are ok but struct allocations are up from 30 and 72 to 33 and 75 and that is for all struct tests, regardless if type Valuer is even being used

$ go test -cpu=4 -bench=. -benchmem=true
PASS
BenchmarkFieldSuccess-4                  5000000           333 ns/op          16 B/op          1 allocs/op
BenchmarkFieldFailure-4                  5000000           334 ns/op          16 B/op          1 allocs/op
BenchmarkFieldOrTagSuccess-4              500000          2716 ns/op          20 B/op          2 allocs/op
BenchmarkFieldOrTagFailure-4             1000000          1357 ns/op         384 B/op          6 allocs/op
BenchmarkStructSimpleSuccess-4           1000000          1286 ns/op          24 B/op          3 allocs/op
BenchmarkStructSimpleFailure-4           1000000          1847 ns/op         529 B/op         11 allocs/op
BenchmarkStructSimpleSuccessParallel-4   5000000           385 ns/op          24 B/op          3 allocs/op
BenchmarkStructSimpleFailureParallel-4   2000000           816 ns/op         529 B/op         11 allocs/op
BenchmarkStructComplexSuccess-4           200000          7312 ns/op         416 B/op         33 allocs/op
BenchmarkStructComplexFailure-4           100000         12253 ns/op        2909 B/op         75 allocs/op
BenchmarkStructComplexSuccessParallel-4  1000000          2270 ns/op         417 B/op         33 allocs/op
BenchmarkStructComplexFailureParallel-4   300000          5268 ns/op        2911 B/op         75 allocs/op

Possible Alternative

Why not have a helper function within your code that would check if the value is of type Valuer and have that function call validator?

// Example
func ValidateSQLField(field interface{}, tag string) ValidationErrors {
    if valuer, ok := field.(sql.Valuer); ok {
        val, err := valuer.Value()
        if err != nil {
            // handle the error how you want
        }

        field = val
    }

    return validate.Field(field, tag)
}

what do you think @johnniedoe

[johnniedoe]

I'm currently using v5 w/gin, but I put together a test implementation for v6. It's under: https://github.com/johnniedoe/validator

I was able to avoid the extra allocations, and it seemed to run the benchmarks w/o much noticeable performance impact.

A lot of structs that interface w/SQL may have Valuer fields, so I thought it'd be handy to have.
If you want to keep the core validator free of cases like these though, I totally understand.

Thanks for the suggestion on the custom validator.

[deankarn]

@johnniedoe

Thanks, I will take a look before work this morning, I am still on the fence about adding but will let you know today regardless.

Thanks so much and I'm glad you like the library!

[johnniedoe]

I'm starting to wonder if it would make more sense as a baked-in validator.

Having it automatically applied is handy in most cases, but may violate the
principle of least surprise. If someone happens to implement Valuer in a sub-struct,
they'd lose the ability do per-field validations on that struct. Having it need to get
explicitly called or enabled during config might be the safer option.

[deankarn]

Summary

ok this was a tough one, I was able to implement without affecting allocations nor other performance impacts by tweaking your example a bit, see https://github.com/joeybloggs/validator/tree/add-sql-valuer and it's accompanying benchmarks below.

however I had to take a step back and reevaluate what this library should be doing, what decisions it should be making and more importantly not making. The library validates fields and struct fields and really shouldn't be extracting or calling functions to get the values for validation; that should be up to the calling program to determine.

a few things prompted this decision:

• what if more cases like this were to be added? more checks, more overhead when the calling program could do it with little or no overhead.
• what if there were multiple cases and was checking if implements an interface and two interfaces had the same exact function declarations, but needed to be handled differently?
• what if there were multiple cases and was checking if implements an interface and the struct implements multiple interfaces? should I check one or both? would have to add case specific options, more checks, more overhead

Recommendation

I would recommend creating a/some helper functions as suggested above as it would the easiest and most potable and will avoid any of the issues/questions/points noted above.

Alternatively you could fork the project and implement the same changes I did here: https://github.com/joeybloggs/validator/tree/add-sql-valuer, but then you have to maintain and merge in changes...

Notes

@johnniedoe I just want you to know that It was a hard decision not to implement this and thank you for suggesting it. If you have any other other recommendations or questions please don't hesitate to create an issue or email me.

It would also be nice to know what you ended up, or will end up, doing. If you could comment or add an example function here, I'm sure it would help others that may be reading this. Thanks!

Benchmarks

$ go test -cpu=4 -bench=. -benchmem=true
PASS
BenchmarkFieldSuccess-4                  5000000           319 ns/op          16 B/op          1 allocs/op
BenchmarkFieldFailure-4                  5000000           320 ns/op          16 B/op          1 allocs/op
BenchmarkFieldOrTagSuccess-4             1000000          2462 ns/op          20 B/op          2 allocs/op
BenchmarkFieldOrTagFailure-4             1000000          1326 ns/op         384 B/op          6 allocs/op
BenchmarkStructSimpleSuccess-4           1000000          1209 ns/op          24 B/op          3 allocs/op
BenchmarkStructSimpleFailure-4           1000000          1829 ns/op         529 B/op         11 allocs/op
BenchmarkStructSimpleSuccessParallel-4   5000000           341 ns/op          24 B/op          3 allocs/op
BenchmarkStructSimpleFailureParallel-4   2000000           866 ns/op         529 B/op         11 allocs/op
BenchmarkStructComplexSuccess-4           200000          7283 ns/op         368 B/op         30 allocs/op
BenchmarkStructComplexFailure-4           100000         12043 ns/op        2861 B/op         72 allocs/op
BenchmarkStructComplexSuccessParallel-4  1000000          2433 ns/op         368 B/op         30 allocs/op
BenchmarkStructComplexFailureParallel-4   300000          5978 ns/op        2863 B/op         72 allocs/op

[deankarn]

Hey @johnniedoe just saw your comment above my last, looks like we commented at about the same time so I almost missed it.

I don't know if it would work as a baked in validator function because a few variables are out of scope at that point, such as field name, tags, ValidationErrors etc.. and after extracting the Value from Valuer you would have to call the traverseField function to do all the checks and apply all tags and errors if any, but you need the aforementioned fields that are out of scope.

[johnniedoe]

I see your point that it is essentially a one-off. It may be a fairly common use-case, but the solution is a one-off nonetheless.

Taking a step back, it seems that the broader questions are:

• Should the validator have the ability to directly validate types that the end-user cannot add tags to?
• If so, what is the best mechanism for enabling such functionality?

The first is a question of scope, and is really up to you.
If you're interested in handling these use cases in general, I may try to come with a more generic mechanism. At this point, I'm imagining an array of type-specific pre-filters that could be specified when configuring the validator. That way, it's there if folks need it but incurs little/no overhead otherwise.

Your supplied helper function looks like a nice way to be able to handle the type while still using the existing validators. If I'm reading it correctly though, it seems like using it will require running the validator separately on each field of a struct though, which is a bit unfortunate.

Thanks again for the package, and for taking the time to look into this.

[deankarn]

@johnniedoe you gave me an idea maybe in the config I could have a map[reflect.Type]Func
where Func is a function type that you have to comply with, like the baked in validator function, and the end user can fill this map with all of the types they wish to run a special function on and I can check if that type is within the map and if it is run the function which will return a value and I can validate.

ok I know run-on sentence but sudo coding it:

// this function within callers code
func ValidateValuerType(field reflect.Value) interface {
    if valuer, ok := field.Interface().(sql.Valuer); ok {
        val, err := valuer.Value()
        if err != nil {
            // handle the error how you want
        }
       return val
    }

    return nil
}

// this within validator package
type TypeOverrideFunc func(field reflect.Value) interface

// this within calling program
specialTypes := map[reflect.Type]TypeOverrideFunc{}
specialTypes[reflect.TypeOf((*sql.Valuer)(nil))] = ValidateValuerType
// NullTime comes from here https://github.com/go-sql-driver/mysql/blob/6d2067fccbf9cd2e000d1d2f5d9a89e78ffd0a7d/utils.go#L480
specialTypes[reflect.TypeOf((NullTime{}] = ValidateValuerType
// .. and so on

config := Config {
   TypeOverrides: specialTypes,
}

// check in traverseField method to see if type is within map
// if no continue on
// if yes then run function to get value, and pass it to traverseField like I did up in my example

Good

• O of 1 lookup time for map
• caller maintains full control about the value passed back
• can override any type, even standard datatype such as int
• completely optional

Bad

• have to add each type that implements Valuer to the map

What do you think?

[johnniedoe]

That seems like a pretty good solution.
I'll need to list all of the concrete types of the Valuers when using it, but it's just done once at Config time.

If the typeOverride code in the validator checks that len(TypeOverrides) > 0 before doing the map lookup, it should incur essentially no cost unless it is actually used.

I should probably mention that since reflect.Types are interface types, map lookups keyed on them won't be as quite as fast as a map[int]func lookup would be. I'm seeing ~30ns/lookup on my machine, vs ~5ns/lookup for an int-based map lookup. This seems good enough to me. If this became an issue though, a per-validated structType cache would probably help.

[deankarn]

@johnniedoe perfect, yep the boolean check is exactly what I had in mind but check the len during the validator initialization within the New() function and set it as a value on the config object.

I think map type lookups are ok, even if a little slow in comparison to int, doing it this way would be optional and if they don't want the extra overhead they can always extract the value themselves and validate just the value.

I'll whip something up by next week, can't promise I will work on it during the long weekend ;)
I'll let you know when I complete it so you can have a look and see if it fits you needs.

P.S. I will probably only implement this functionality in v6 as v5 it would be too cumbersome, I'm sure @manucorporat will update to v6 soon enough, he's really good about that sort of thing.

[deankarn]

Hey @johnniedoe I added this in https://github.com/bluesuncorp/validator/releases/tag/v6.2

thanks for your ideas!
I didn't want to have to think about it over the long weekend, so now I don't have to 😄