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

Breaking change: required no longer validates zero value strings #359

Closed
bmhatfield opened this issue Jan 9, 2020 · 10 comments
Closed

Breaking change: required no longer validates zero value strings #359

bmhatfield opened this issue Jan 9, 2020 · 10 comments
Labels

Comments

@bmhatfield
Copy link

I have a simple struct, like:

type SpannerSlugger struct {
	Slug string `spanner:"slug" valid:"required"`
	Name string `spanner:"name" valid:"required"`
}

and a test like

func TestValidation_Slugs(t *testing.T) {
	ok, err := govalidator.ValidateStruct(&db.SpannerSlugger{})
	assert.OK(t, err)
	assert.True(t, ok, "expected ok")
}

On SHA f61b66f89f4a, this works as expected (the empty string values result in the test failing)

% cat go.mod | grep govalidator
	github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a

On SHA 475eaeb16496, it fails silently (the test passes without the expected error)

cat go.mod | grep govalidator
	github.com/asaskevich/govalidator v0.0.0-20200108200545-475eaeb16496

This seems like a pretty significant regression / change, as validation that I expect to be performed is no longer being performed.

@bmhatfield
Copy link
Author

I just spent some time trying out different commit SHAs, and it appears that this commit is what introduced this breakage: 2abf7b9

The switch from isEmptyValue to isFieldSet coupled with the corresponding test changes seems suspicious.

cc @asaskevich

@asaskevich
Copy link
Owner

@bmhatfield Thank you, investigating for the solution

@kolaente
Copy link

kolaente commented Apr 6, 2020

I am also hitting this, is there any update?

@fabianem
Copy link

fabianem commented May 12, 2020

Any news regarding this issue?
Experiencing the same problem on commit 21a406dcc535 but not only for strings - had the same error for validating int64.
Seems that the required flag now allows for zero values on int64.

@BahmanBinary
Copy link

I have this problem too
giphy

@kolaente
Copy link

kolaente commented Jul 21, 2020

@asaskevich Anything new? This is clearly a breaking change. It prevents me from updating govalidator to a newer version since it breaks my application validation logic.

@bmhatfield
Copy link
Author

bmhatfield commented Dec 8, 2020

Bump? Based upon the responses here, it seems like the breaking commit author's understanding is incomplete and perhaps 329 should be reverted? In particular, assuming that everyone can use pointers or must otherwise switch to the runelength validator seems incorrect to me.

@migueleliasweb
Copy link

I'm facing this problem too! As a temporary workaround I had to change my code to also validate length which is by far not a great solution. Can we get some traction on this?

Also having proper versions tagged and usable with go modules would help heaps in cases like this where we need to find a previous working version =( . See: #384

@bmhatfield
Copy link
Author

It appears this issue may have been resolved: 63eac46

@sergeyglazyrindev
Copy link

Hello guys!
I forked this package cause owner disappeared. Hope, he will be back, but it would be easier to merge these changes back if he is back
Link to my repo: create issue there and we'll discuss it.

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

7 participants