search

go-playground / validator

:100:Go Struct and Field validation, including Cross Field, Cross Struct, Map, Slice and Array diving
MIT License
16.07k stars 1.29k forks source link

Fix Bad Field type panic #1201

Open darklam opened 7 months ago

darklam commented 7 months ago

Fixes Or Enhances

Fixes #907 (and possibly others?)

At first I found it weird that the required_if affected the behavior of the gte tag, but after some debugging it seems that the condition runValidationWhenNil is not checked for each field tag, but only for the first one. In this case it was set to true for the required_if tag, but set to false for gte, so when the required_if preceded the gte tag it meant that the field was incorrectly validated through gte even when it was nil.

Make sure that you've checked the boxes below before you submit PR:

@go-playground/validator-maintainers

coveralls commented 7 months ago

Coverage Status

coverage: 73.933% (+0.03%) from 73.903% when pulling d015ea40f59bae1e210eba68b3db03e4b8e31111 on darklam:fix-bad-field-type-panic into 84254aeb5a59e615ec0b66ab53b988bc0677f55e on go-playground:master.

deankarn commented 7 months ago

Thanks fro the PR @darklam

I will have to think hard on if this could cause any breaking side-effects, see here for details.

darklam commented 6 months ago

Thanks fro the PR @darklam

I will have to think hard on if this could cause any breaking side-effects, see here for details.

Yes, agreed that this should be scrutinized a lot regarding breaking changes before merging. I'm happy to add more test cases if there are any specific ones coming to mind. Also, my first thought was indeed to change some baked ins like gte to handle nil cases, but it didn't seem like the way to go because it became quite repetitive. I think that the check for that should happen before getting to these validators (as the original intention seems to be with the runValidationWhenNil flag), but of course this is not an informed opinion as I'm quite new with the codebase 😄