Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validators boolean logic #329

Closed
skryja opened this issue Dec 12, 2017 · 7 comments
Closed

Validators boolean logic #329

skryja opened this issue Dec 12, 2017 · 7 comments
Assignees
Labels

Comments

@skryja
Copy link

skryja commented Dec 12, 2017

Package version eg. v8, v9:

v9

Issue, Question or Enhancement:

I'm trying to test a struct with validation tags but i can't understand how the boolean logic works.
In this example it's expected that the validator on the Path attribute returns false value, but i don't know how it returns true, because the require.Error in TestPatchOpValid fails.
I'm using two custom tags:

  • isfield: it checks if a struct's attribute has a certain value (eg. isfield=Op:remove)

  • attrpath: it checks if the value has a correct composition (it's not so important in this case)

Have you an explanation for the validators' logic?

Code sample, to showcase or reproduce:

type Operation struct {
	Op    string   `json:"op" validate:"required,eq=add|eq=remove|eq=replace"`
	Path  string   `json:"path,omitempty" validate:"attrpath|eq='',isfield=Op:replace|isfield=Op:add|attrpath"`
	Value []*Value `json:"value,omitempty" validate:"gt=0|isfield=Op:remove"`
}

func TestPatchOpValid(t *testing.T) {
	var err error
        op := Operation{}
	op.Op = "replace"
	op.Path = "#######" // wrong path
	op.Value = []*Value{
		&Value{
			Display: "Babs Jensen",
			Ref:     "https://example.com/v2/Users/2819c223",
			Value:   "2819c223-7f76",
			Type:    "home",
		},
	}

	err = validation.Validator.Struct(op)
	fmt.Println(err)
	require.Error(t, err)
@deankarn
Copy link
Contributor

Hello @skryja it's a little hard not knowing what the custom function do, but will try to layout the operations that should occur for the Path field:

Block1 & Block2 below are AND's eg. if Block1 and Block2 = validation success

Block1

  • first it will check attrpath|eq=''
    • if attrpath returns true, move to the next validation block
    • if attrpath returns false, move to eq='' if returns true move to next validation block otherwise return error and stop other validation for the field

Block2

  • next it will check isfield=Op:replace|isfield=Op:add|attrpath
    • if isfield=Op:replace returns true, move to the next validation block(which there is none validation of field complete)
    • if isfield=Op:replace returns false, move to isfield=Op:add if returns true, move to next validation block
    • if isfield=Op:add returns false then move to attrpath if it returns true move to next validation block
    • if attrpath returns false return error.

it seems odd to me to have 2 validations here, and that attrpath is part of both, I think it could be combined to validate:"eq='',isfield=Op:replace|isfield=Op:add|attrpath" or even validate:"omitempty,isfield=Op:replace|isfield=Op:add|attrpath"

I hope this helps, please let me know :)

@deankarn deankarn self-assigned this Dec 13, 2017
@skryja
Copy link
Author

skryja commented Dec 13, 2017

Thanks @joeybloggs for your answer,
but in the case of the test, attrpath and eq both return false so, as you said, it should stop the validation on the field and should return an error on the validation tag eq as I would like it to be in the test. Instead, the validation seems to continue and goes on Block2 and returns true because of isfield=Op:replace and the test will fail.

@deankarn
Copy link
Contributor

Hey @skryja I may have solved this in a dream ;)

one thing I realized was the eq='' is probably not checking for what you want it’s equivalent to Path == “''” and not Path == “” and should be changed to eq=

Maybe I’m way off?

@deankarn deankarn added bug and removed question labels Dec 13, 2017
@deankarn
Copy link
Contributor

Hey @skryja I have confirmed that this is a bug and is due to the way I was checking when to terminate each or instance; will be fixed soon, nice catch 😄

@deankarn
Copy link
Contributor

Thanks @skryja for reporting, it should now be fixed in v9.9.1 release, please update and try again

if you have any more problems with it please let me know 😄

@skryja
Copy link
Author

skryja commented Dec 14, 2017

Thanks @joeybloggs ...now it works! :)
Meanwhile my colleague @leogr found this alternative solution:

isfield=Op:add|isfield=Op:replace|required,omitempty,attrpath

If you want to take a look at what isfield and attrpath validators do; also you can check our repo here.

Thanks again for your time 😃

@deankarn
Copy link
Contributor

deankarn commented Dec 14, 2017

@skryja Thanks for that ! always interested in seeing how others are using it :) I may even add a few after seeing your like prefix and suffix validations.

oh and I noticed your issue 37 about the required tag failing, there are a few issue, think one still open tagged question, that explains required’s behavior, but to paraphrase:

required checks that the field is not the field types default value, for bool false, int 0, string “” and so forth.

It doesn’t make much sense on a bool unless you need it tri-state and for that you would use a pointer to a bool. In fact if you have optional variables that’s how you would handle if they were actually set or not as the default value of a pointer is nil(yes nil is a type)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants