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

Fix Bad Field type panic #1201

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

darklam
Copy link

@darklam darklam commented Dec 2, 2023

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:

  • Tests exist or have been written that cover this particular change.

@go-playground/validator-maintainers

@darklam darklam requested a review from a team as a code owner December 2, 2023 15:02
@coveralls
Copy link

Coverage Status

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

case reflect.Ptr, reflect.Interface:
if !ct.runValidationWhenNil && current.IsNil() {
v.errs = append(v.errs,
&fieldError{
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not very familiar with the errors in this library, so I just went by what was used in the other instance where the runValidationWhenNil is checked and could be entirely wrong 😄

@deankarn
Copy link
Contributor

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
Copy link
Author

darklam commented Dec 14, 2023

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 😄

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

Successfully merging this pull request may close these issues.

Panic Bad field type *int64, when using required_if and omitempty at the same time
3 participants