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

Unique tag panics if value is nil #749

Closed
Jake-Convictional opened this issue Apr 7, 2021 · 2 comments · Fixed by #1041
Closed

Unique tag panics if value is nil #749

Jake-Convictional opened this issue Apr 7, 2021 · 2 comments · Fixed by #1041

Comments

@Jake-Convictional
Copy link
Contributor

Package version eg. v9, v10:

v10 (introduced in 10.4.0)

Issue, Question or Enhancement:

If a pointer field involved in a unique tag is nil then the validator panics with

panic: reflect: call of reflect.Value.SetMapIndex on zero Value

This is a bug because nil is a valid state for pointer fields, and this behavior would require the user to validate they don't have a nil value before running the rest of the validation (which is silly). I'm assuming this bug was introduced by #644 (though I didn't take a close look, the bug does not exist in v10.3.0, but does exist in v10.4.0 onward). To resolve, maybe just needs a condition to check if the value is nil.

As for the expected behavior, I think the alternatives are:

  1. Unique or nil: Any occurrence of the "unique" pointer field can have a nil value, and if so, that instance isn't accounted for in the uniqueness check. This is my personal preference. I think it follows the spirit of the "unique" tag the best, plus there is no easy way right now to get that behavior if the user wants it.
  2. No Nils: No occurrences of the "unique" field may have nil values. Any nil value causes the unique tag to fail. I would vote against this, because it can be achieved fairly easily already by adding a required tag to the unique field.
  3. Strict uniqueness: One occurrence of the unique field can have a nil value, but if two values are nil, validation will fail. This is the most rigorous implementation of the unique tag, but I can't imagine anyone truly wanting/expecting that behavior.

Code sample, to showcase or reproduce:

Playground sample

type A struct {
	B []B	`validate:"unique=Value"`
}

type B struct {
	Value *string // I hope this is never nil!
}

func main() {
	a := A{B: []B{{}}}
	v := validator.New()
	if err := v.Struct(&a); err != nil { // -> "panic: reflect: call of reflect.Value.SetMapIndex on zero Value"
		fmt.Println(err)
		return
	}
	fmt.Println("ok")
}
@deankarn
Copy link
Contributor

Thanks @Jake-Convictional I'll have to think about this.

By the way I noticed you've been posting here a bit and I'm looking for others to help maintain this and some other libs as it's hard time wise for me to keep up, would you be interested?

@yansal
Copy link
Contributor

yansal commented Dec 14, 2022

I have created a PR to fix this issue: #1041

I chose to implement this behavior :

Unique or nil: Any occurrence of the "unique" pointer field can have a nil value, and if so, that instance isn't accounted for in the uniqueness check. This is my personal preference. I think it follows the spirit of the "unique" tag the best, plus there is no easy way right now to get that behavior if the user wants it.

deankarn pushed a commit that referenced this issue Mar 19, 2023
Fix #749

Co-authored-by: Yann Salaün <ysalaun@yubik.io>
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 a pull request may close this issue.

3 participants