panic occurs when invalid validator tag is encountered

[nathanejohnson]

Package version eg. v8, v9:

v9

Issue, Question or Enhancement:

cache.go throws an unhandled panic instead of returning an InvalidValidationError when an undefined validation function is encountered. I was under the impression that with v9, panics should not occur.

Code sample, to showcase or reproduce:

package main

import (
    "fmt"
    "gopkg.in/go-playground/validator.v9"
)

type Foo struct {
    A string `validate:"badvalidator"`
}

func main() {
    f := &Foo{}
    v := validator.New()
    // panics here
    err := v.Struct(f)
    if err != nil {
        fmt.Println("Error was: ", err)
    }
}

[deankarn]

Hey @nathanejohnson

I did this more as a safety precaution than anything, validator is not panicing because of the input, which you are correct about validator not panicing in that regard, but rather it's more that there is a bug in your code.

personally if something like this accidentally, somehow without tests or testing first, got into production I'd rather the application panic and let me know the bug straight up rather than the potential for critical company data being saved that shouldn't if the error isn't handled properly. (and if this got into production, I'd be very suspect if the error is also checked properly)

I'd say it's akin to passing nil to a function expecting a context.Context, they panic in the STD lib.

I could add this functionality, but then everyone would also have to start checking all the error type returned from Validator.

This has given me an idea though, perhaps the validation functions should pass back 2 error variables, the first being the field validation errors, and the second being a processing error. That was as new error are added or removed from validator your logic will never have to change and will be able to remove the panics. perhaps not until the next version though.

[nathanejohnson]

Then what is InvalidValidationError was for, if not a malformed validation:"" tag ? To the caller, I'd argue it's all the same thing, whether it's an invalid "function" or just a specified validation that doesn't make sense. One could also argue that it's never appropriate for a third party package to explicitly throw a panic that is leaked to the caller's context. I can see your argument though, but to that end I have some tests that actually go through all structs where I put validation tags in, and then see if any InvalidValidationError type errors are returned, and also see if any panics are thrown. Seems strange to have to check for both.

[deankarn]

As documented here it's for bad input types, not validation tags.

I just want to say I do understand your position as well and am willing to add this in the next version with a new BadValidationTag or alike. Although the code wouldn't be a breaking change, the logic because nobody is checking for the error would be, and so will just have to wait until the next version.

Thanks for you opinions and views, it's what open source is all about :)

[nathanejohnson]

I guess I am misunderstanding then: What is the InvalidValidationError actually for? I was under the impression that ValidationErrors was returned if the input failed validation, and InvalidValidationError was returned if the specified validator tag was invalid? Sorry if I'm being dense!

EDIT - so I guess it's simply for validator tags on unsupported data types? I guess I misunderstood.

[deankarn]

it's for when bad data aka struct is passed to say validate.Struct(...)

for example if you passed nil, or a string to be validated as a struct.

[nathanejohnson]

https://github.com/myENA/radosgwadmin/blob/master/adminapi_test.go#L67

Here is an example of where I'm testing for InvalidValidationError and recovering from panics. (in pratice I hope neither of these trigger). To my earlier point, I think InvalidValidationError would be a good way to flag I'm mis-using your package. Regardless, I have to handle both cases, panic or InvalidValidationError, in order to have test coverage, and as a caller, I'd argue they're the same.