asaskevich / govalidator

[Go] Package of validators and sanitizers for strings, numerics, slices and structs
MIT License
6.04k stars 555 forks source link

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

Closed bmhatfield closed 1 year ago

bmhatfield commented 4 years ago

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 commented 4 years ago

I just spent some time trying out different commit SHAs, and it appears that this commit is what introduced this breakage: https://github.com/asaskevich/govalidator/commit/2abf7b9d5da33ab5e25960c301ca26f779a88db9

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

cc @asaskevich

asaskevich commented 4 years ago

@bmhatfield Thank you, investigating for the solution

kolaente commented 4 years ago

I am also hitting this, is there any update?

fabianem commented 4 years ago

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 commented 4 years ago

I have this problem too giphy

kolaente commented 4 years ago

@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 commented 3 years ago

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 commented 3 years ago

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: https://github.com/asaskevich/govalidator/issues/384

bmhatfield commented 3 years ago

It appears this issue may have been resolved: https://github.com/asaskevich/govalidator/commit/63eac4636d06ec91ef7f8290e2e021912e610051

sergeyglazyrindev commented 3 years ago

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.