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

required_without does not work with pointers #483

Closed
tbe opened this issue Jun 13, 2019 · 14 comments · Fixed by #484
Closed

required_without does not work with pointers #483

tbe opened this issue Jun 13, 2019 · 14 comments · Fixed by #484

Comments

@tbe
Copy link

tbe commented Jun 13, 2019

Package version eg. v8, v9:

v9

Issue, Question or Enhancement:

required_without does not work with pointers

Code sample, to showcase or reproduce:

Playground: https://play.golang.org/p/5hpXOfDJU4V

package main

import (
	"fmt"
	"gopkg.in/go-playground/validator.v9"
)

type a struct {
	Field1 *string `validate:"required_without=Field2"`
	Field2 *int    `validate:"required_without=Field1"`
}

func main() {
	validate := validator.New()

	testStr := new(string)
	*testStr = "abcd"

	testInt := new(int)
	*testInt = 42

	if err := validate.Struct(a{Field1: testStr}); err != nil {
		fmt.Println(err)
	} else {
		fmt.Println("Test A OK")
	}

	if err := validate.Struct(a{Field2: testInt}); err != nil {
		fmt.Println(err)
	} else {
		fmt.Println("Test B OK")
	}

	if err := validate.Struct(a{Field1: testStr, Field2: testInt}); err != nil {
		fmt.Println(err)
	} else {
		fmt.Println("Test C OK")
	}
}
@AlexanderGrom
Copy link

Hello!
https://play.golang.org/p/lx1Qoi1HLOF

package main

import (
	"fmt"
	"gopkg.in/go-playground/validator.v9"
)

type a struct {
	Field1 string `validate:"required_with=Field2"`
	Field2 int    `validate:"required_with=Field1"`
}

func main() {
	validate := validator.New()

	// req is a pointer!
	req := &a{
		Field1: "string",
	}

	defer func() {
		if r := recover(); r != nil {
			fmt.Println("Recovered:", r)
		}
	}()

	// We get panic! (req is a pointer)
	if err := validate.Struct(req); err != nil {
		fmt.Println(err)
	} else {
		fmt.Println("Test A OK")
	}

	if err := validate.Struct(*req); err != nil {
		fmt.Println(err)
	} else {
		fmt.Println("Test B OK")
	}
}

I get panic: reflect: call of reflect.Value.FieldByName on ptr Value

fl.Parent().Kind() is a pointer if struct is a pointer.

Usually in http frameworks the request is a pointer.

var request = new(myRequest)
if err = ctx.BindJSON(request); err != nil {
	c.BadRequestError(ctx, "bad_request")
	return
}

@crathjen
Copy link

@tbe did #484 fix your issue? Because I still seem to be experiencing the problem that you identified (seeing it on the required_without_all tag rather than required_with).
Another example: https://play.golang.org/p/Gx6hh4ASKQa

package main

import (
	"gopkg.in/go-playground/validator.v9"
	"fmt"
)

func main() {
	
	v:=validator.New()
	
	result := v.Struct(foo{A: &emptystruct{}})
	if result != nil {
		fmt.Println(fmt.Sprintf("Unexpected Error: %s", result))
	}
	
	result = v.Struct(bar{A: "somevalue"})
	if result != nil {
		fmt.Println(fmt.Sprintf("Unexpected Error: %s", result))
	}
	
}

type foo struct {
	A *emptystruct `validate:"required"`
	B *emptystruct `validate:"required_without_all=A C"`
	C *emptystruct
}

type bar struct {
	A string `validate:"required"`
	B string `validate:"required_without_all=A C"`
	C string
}

type emptystruct struct {
}

gives the error

Unexpected Error: Key: 'foo.B' Error:Field validation for 'B' failed on the 'required_without_all' tag

The fact that this does not also output a failure for foo.A being required highlights the discrepancy between the behavior of required and the behavior of required_without_all

@johndww
Copy link

johndww commented Aug 23, 2019

@leafduo @crathjen @tbe I don't think the fixed worked either.

func TestLookup(t *testing.T) {
	type Lookup struct {
		FieldA *string `json:"fieldA,omitempty" validate:"required_without=FieldB"`
		FieldB *string `json:"fieldB,omitempty" validate:"required_without=FieldA"`
	}

	fieldAValue := "1232"
	lookup := Lookup{
		FieldA: &fieldAValue,
		FieldB: nil,
	}

	assert.NoError(t, validator.New().Struct(lookup))
}

fails on 9.29.1

@renxiawang
Copy link

@johndww +1, still failing on 9.29.1.

@deankarn
Copy link
Contributor

sorry that I have not been able to take a look at this, super busy with work right now.

If anyone can help with the debugging and a PR it would be very appreciated :)

@leafduo
Copy link
Contributor

leafduo commented Sep 22, 2019

#484 fixed panicking when the variable to validate is pointer, as in

errs = validate.Struct(&test2)

But it did not fix the problem of required_without working with pointers. Digging into the code, I've found out that the problem is in traverseField:

validator/validator.go

Lines 96 to 159 in 556b9da

func (v *validate) traverseField(ctx context.Context, parent reflect.Value, current reflect.Value, ns []byte, structNs []byte, cf *cField, ct *cTag) {
var typ reflect.Type
var kind reflect.Kind
current, kind, v.fldIsPointer = v.extractTypeInternal(current, false)
switch kind {
case reflect.Ptr, reflect.Interface, reflect.Invalid:
if ct == nil {
return
}
if ct.typeof == typeOmitEmpty || ct.typeof == typeIsDefault {
return
}
if ct.hasTag {
v.str1 = string(append(ns, cf.altName...))
if v.v.hasTagNameFunc {
v.str2 = string(append(structNs, cf.name...))
} else {
v.str2 = v.str1
}
if kind == reflect.Invalid {
v.errs = append(v.errs,
&fieldError{
v: v.v,
tag: ct.aliasTag,
actualTag: ct.tag,
ns: v.str1,
structNs: v.str2,
fieldLen: uint8(len(cf.altName)),
structfieldLen: uint8(len(cf.name)),
param: ct.param,
kind: kind,
},
)
return
}
v.errs = append(v.errs,
&fieldError{
v: v.v,
tag: ct.aliasTag,
actualTag: ct.tag,
ns: v.str1,
structNs: v.str2,
fieldLen: uint8(len(cf.altName)),
structfieldLen: uint8(len(cf.name)),
value: current.Interface(),
param: ct.param,
kind: kind,
typ: current.Type(),
},
)
return
}

// traverseField validates any field, be it a struct or single field, ensures it's validity and passes it along to be validated via it's tag options
func (v *validate) traverseField(ctx context.Context, parent reflect.Value, current reflect.Value, ns []byte, structNs []byte, cf *cField, ct *cTag) {

	var typ reflect.Type
	var kind reflect.Kind

	current, kind, v.fldIsPointer = v.extractTypeInternal(current, false)

	switch kind {
	case reflect.Ptr, reflect.Interface, reflect.Invalid:

		if ct == nil {
			return
		}

		if ct.typeof == typeOmitEmpty || ct.typeof == typeIsDefault {
			return
		}

		if ct.hasTag {
//...
			v.errs = append(v.errs,
				&fieldError{
					v:              v.v,
					tag:            ct.aliasTag,
					actualTag:      ct.tag,
					ns:             v.str1,
					structNs:       v.str2,
					fieldLen:       uint8(len(cf.altName)),
					structfieldLen: uint8(len(cf.name)),
					value:          current.Interface(),
					param:          ct.param,
					kind:           kind,
					typ:            current.Type(),
				},
			)

			return
		}
//...

According to code above, any pointer with validator other than omitempty or isdefault and has tag will throw error. Although I've found the bad code path, I don't know how to fix it, because I don't fully understand the logic. For example, how does other validators work with pointers? What does hasTag mean?

@deankarn can you shed some light?

BTW, in the test code, if errs != nil is misspelled as if errs == nil at

if errs == nil {
and
if errs == nil {
, which causes the bug not to be found.

@leafduo
Copy link
Contributor

leafduo commented Sep 22, 2019

Maybe #510 will fix this issue?

@deankarn
Copy link
Contributor

For context how other validations work with pointers is that they are dereferenced prior to validation, but since the value is nil then there's nothing to dereference.

I think another exception may be necessary like omitempty and isdefault.

I like #510 but it's unfortunately a breaking change.

@deankarn
Copy link
Contributor

I released a bunch of fixes today including all required_with* functionality, please let me know if this works as expected for you now, see v9.30.0

@renxiawang
Copy link

So with the latest version, the issue is not fully resolved.
Working code: https://play.golang.org/p/PPp8OeT-2xG
Not working code: https://play.golang.org/p/scNTDSb_r4c (assigning false to a *bool breaks)

@deankarn
Copy link
Contributor

deankarn commented Nov 17, 2019

I believe I've properly corrected the issues now and released in:

please let me know if this resolves your issues :)

P.S. I highly recomment updating to v10

@melonaerial
Copy link

Hello @deankarn
Do you ave any plans to add one_of functionality for required? For example, if I want to that just one of Field1 and Field2 was set, I have to write code something like this https://play.golang.org/p/tY4x6H5h8io . But this not so good hack I think. Or write required validation by my function.

@deankarn
Copy link
Contributor

@melonaerial can you please open a new issue next time 🙏 it's hard discussing in a closed and unrelated thread.

I'm not 100% sure what you're trying to accomplish? doesn't required_without_all suite your use case?

@MaciejKaras
Copy link

Hi @deankarn

I'm not sure it works when you have mulitple validation tags. Please run:
https://play.golang.org/p/Aq1ebMyzXs8

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

Successfully merging a pull request may close this issue.

9 participants