fix: prevent panic in unique validation with nil pointer elements

[veeceey]

Summary

• Fixes a panic (reflect: call of reflect.Value.FieldByName on zero Value) when the unique tag validates slices or maps containing nil pointer elements.
• Three code paths in isUnique are affected: (1) slices without a field param (unique), (2) slices with a field param (unique=Name), and (3) maps with pointer values. All three now safely handle nil elements.
• Nil elements are treated as equal to each other for uniqueness purposes: a single nil is valid, but multiple nils in the same collection fail the unique check.

Fixes #1319

Reproduction

type A struct {
    Name string
}
e := struct {
    F1 []*A `validate:"unique=Name"`
}{F1: []*A{nil}}
err := validate.Struct(e) // panics before this fix

Changes

• baked_in.go: In all three branches of isUnique (slice no-param, slice with-param, map), check e.IsValid() after reflect.Indirect and skip nil elements with a counter. Return false when nilCount > 1 (duplicate nils are not unique).
• validator_test.go: Added TestUniqueValidationNilPtrSlice with 9 sub-tests covering single nil, duplicate nils, and mixed nil/non-nil for all three code paths.

Test plan

• New test TestUniqueValidationNilPtrSlice passes (9 sub-tests)
• Full test suite passes (go test ./... -- all 24 packages OK, zero failures)
• No regressions in existing TestUniqueValidation, TestUniqueValidationStructSlice, and TestUniqueValidationStructPtrSlice

[coveralls]

coverage: 73.849% (+0.02%) from 73.832%
when pulling e0f3812 on veeceey:fix-unique-nil-panic
into dede341 on go-playground:master.

[nodivbyzero]

Thanks for the contribution!

I took a look at your changes - what do you think about treating nil as just another comparable key and dropping nilCount altogether?
Something like:

if !e.IsValid() {
			m.SetMapIndex(zero, reflect.ValueOf(struct{}{}))
			continue
		}

Then we could keep the original check:

return field.Len() == m.Len()

[veeceey]

Thanks for the suggestion! I considered that approach but there's a subtle correctness issue — if we use a zero-value key for nil pointers, it would collide with an actual element whose fields are all zero-valued. For example, []*Inner{nil, {Name: ""}} would incorrectly fail the unique check because both nil and the zero-value struct would map to the same key.

The nilCount approach avoids that by keeping nil tracking completely separate from the map, so nil elements only compare against other nil elements.

[nodivbyzero]

@veeceey please take a look at my PR: #1532

[veeceey]

Hey @nodivbyzero, thanks for putting that together! I took a look at #1532. Your approach is definitely cleaner in terms of not needing a separate counter.

One thing I noticed though - using reflect.Zero(elem) as the map key for nil values means nil pointers and actual zero-value structs would collide. For example, []*Inner{nil, {Name: ""}} would fail the unique check since both would map to the same zero-value key, even though they're arguably different values (nil vs an actual empty struct).

That said, I can see the argument that treating nil as equivalent to zero-value might be reasonable depending on the use case. Happy to defer to the maintainers on which behavior they prefer - just wanted to flag the semantic difference.

[veeceey]

Hi maintainers, just checking in on this one. I've had a good discussion with @nodivbyzero about the approach (nilCount vs zero-value key). My approach keeps nil tracking separate to avoid collisions between nil pointers and zero-value structs. @nodivbyzero opened an alternative in #1532. Would love a maintainer's take on which semantic approach you prefer — happy to adjust either way!

[nodivbyzero]

@veeceey Is there a test case that succeeds on your branch but fails on mine?

[veeceey]

@nodivbyzero Yes — consider this test case:

type Inner struct {
    Name string
}
// A slice with nil and a zero-value struct
s := []*Inner{nil, {Name: ""}}

On my branch, this correctly passes the unique validation (nil and &Inner{Name: ""}) are treated as distinct values). On your branch (#1532), both would map to reflect.Zero(elem), so they'd collide and the validation would incorrectly fail, reporting a duplicate when there isn't one.

Happy to add this as an explicit test case if that would help illustrate the difference!

[nodivbyzero]

@veeceey something like this?

validator/validator_test.go

Lines 11033 to 11040 in 26d790b

	t.Run("unique_slice_with_nil_and_zero_value_struct", func(t *testing.T) {
	s := []*Inner{nil, {Name: ""}}
	errs := validate.Var(s, "unique")
	if errs != nil {
	t.Fatalf("nil and non-nil map values should pass unique validation, got: %v", errs)
	}
	})
	}

[veeceey]

@nodivbyzero Yes, exactly like that! That test case would pass on my branch (nil and &Inner{Name: ""} are treated as distinct) but would fail on yours since both would map to the same zero-value key. Happy to add it as an explicit test case here if the maintainers would find that helpful for comparing the two approaches.

[nodivbyzero]

Yes, exactly like that! That test case would pass on my branch (nil and &Inner{Name: ""} are treated as distinct) but would fail on yours

@veeceey this test case has been added in my branch :)

[veeceey]

Nice, glad that test case made it in! Sounds like #1532 has converged to handle the nil vs zero-value distinction properly now. Happy to close this one if maintainers prefer that approach — just let me know.

[veeceey]

Closing in favor of #1532 which now covers the nil vs zero-value distinction as well. Thanks @nodivbyzero for picking it up!