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

Add Context#BindAndValidate() #1105

Closed
vishr opened this issue Apr 10, 2018 · 12 comments
Closed

Add Context#BindAndValidate() #1105

vishr opened this issue Apr 10, 2018 · 12 comments

Comments

@vishr
Copy link
Member

vishr commented Apr 10, 2018

No description provided.

@dennypenta
Copy link

I recommend you just implement your custom binder that would perform validation.
It's pretty simple and looks like gin does the same.

@p581581
Copy link

p581581 commented May 3, 2018

Any updates?

@Tethik
Copy link

Tethik commented Jun 11, 2018

I implemented this in my own project as follows:

// BindValidate simply combines echo.Context.Bind and echo.Context.Validate for the forms.
func BindValidate(c echo.Context, form interface{}) (err error) {
	if err = c.Bind(form); err != nil {
		return
	}
	return c.Validate(form)
}

Although I think it would make more sense to directly do the validation during the "Bind" function conditionally if the context.Validator is set.

@stale
Copy link

stale bot commented Nov 30, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 30, 2018
@stale stale bot closed this as completed Dec 7, 2018
@lordzsolt
Copy link

lordzsolt commented Nov 25, 2021

@vishr @aldas What do you think about revisiting this issue?

With the current implementation, post-bind validation is not possible. Or does not produce the expected result.

Given an model such as:

type RequestModel struct {
	Include bool `query:"include" validation:"required"`
}

and a request such as: somePath?include=false

Validation will fail, because false is the Zero value of bool type.

There is no way to know if it's false because it's the default value or whether it came in the request.

Expected result

somePath without the query param should fail validation
somePath?include=false should pass validation

@aldas
Copy link
Contributor

aldas commented Nov 25, 2021

@lordzsolt your your case probably changing bool to *bool would help. In your case obstacle is that bool is value type and therefore does not have not existing state. bool as a value type has only true or false (which is default state) states.
So if required query parameter does not exist that value would be default value false and would pass validation. Marking value types as validation:"required" makes no difference.

Validation works on already bound data and if field does not support "not existing state" then "required" can not fail.

See this example:

func main() {
	e := echo.New()

	e.Use(middleware.Logger())
	e.Use(middleware.Recover())

	e.GET("/", func(c echo.Context) error {
		type RequestModel struct {
			Include *bool `query:"include" validation:"required"`
		}

		rm := RequestModel{}
		if err := c.Bind(&rm); err != nil {
			return err
		}

		include := "empty"
		if rm.Include != nil {
			include = fmt.Sprintf("%v", *rm.Include)
		}
		return c.String(http.StatusOK, fmt.Sprintf("rm=%+v, include=%v\n", rm, include))
	})

	log.Fatal(e.Start(":8080"))
}

Output:

x@x:~/code$ curl "http://localhost:8080/?include=true"
rm={Include:0xc000024fd8}, include=true

x@x:~/code$ curl "http://localhost:8080/?"
rm={Include:<nil>}, include=empty

For reference: this is how https://github.com/go-playground/validator handles required tag
https://github.com/go-playground/validator/blob/d4271985b44b735c6f76abc7a06532ee997f9476/baked_in.go#L1456

@aldas
Copy link
Contributor

aldas commented Nov 25, 2021

side note: if default values that differ from "value type default value" are needed then try newer binder that Echo has

		err := echo.QueryParamsBinder(c).
			Bool("include", &rm.Include).
			BindError()

Full example:

func main() {
	e := echo.New()

	e.Use(middleware.Logger())
	e.Use(middleware.Recover())

	e.GET("/", func(c echo.Context) error {
		type RequestModel struct {
			Include bool // note: `echo.QueryParamsBinder` does not use struct tags
		}

		rm := RequestModel{
			Include: true, // set you default value that is not overwritten when query param does not exist
		}
		err := echo.QueryParamsBinder(c).
			Bool("include", &rm.Include).
			BindError()
		if err != nil {
			return err
		}

		return c.String(http.StatusOK, fmt.Sprintf("rm=%+v\n", rm))
	})

	log.Fatal(e.Start(":8080"))
}

Output:

x@x:~/code$ curl "http://localhost:8080/?include=true"
rm={Include:true}
x@x:~/code$ curl "http://localhost:8080/?include=false"
rm={Include:false}
x@x:~/code$ curl "http://localhost:8080/?"
rm={Include:true}

@lordzsolt
Copy link

lordzsolt commented Nov 26, 2021

Validation works on already bound data and if field does not support "not existing state" then "required" can not fail.

With the current implementation, yes. That's the reason I pinged you to ask about your opinion.

What I would imagine happening is that validation would happen within
func (b *DefaultBinder) bindData(destination interface{}, data map[string][]string, tag string) error

Here, you can validate that all the fields marked "required" by destination are present in the data map.

In essence, the contents of data is validated, based on the rules of destination.

@aldas
Copy link
Contributor

aldas commented Nov 26, 2021

This would be applicable only for query params/form fields/headers/cookies and not for json etc. JSON/XML have their own bind implementations that Echo only wraps. So if you are binding to json to struct there is no way to my knowledge to know if field existed or not.

I do not think that validating required is that worthwhile feature to combine binder and validator. If you use pointers instead of value types you are fine as https://github.com/go-playground/validator can handle required in that case. All other validation cases work with current way as they are.

My suggestion is that: in your handler have temporary struct, in to where you are binding your request data. Do validation on that struct which scope is only that handler method. If validation passes then map bound data to DTO or domain object and pass to business logic/service. So then you are not directly passing that struct you can be generous with fields being pointers and you do not have to deal nil check in business logic. If you want to do validation in business code and still check for required on value types - I do not think it makes sense (definitely for booleans, maybe ok for empty strings etc).

It is safer not to pass bound struct directly to business logic - see first example here #1670 (comment)

@aldas
Copy link
Contributor

aldas commented Dec 5, 2021

Note for future: you can create custom binder that wraps default binder and contains validation part. In this thread there are couple of ways to do it #438

@lordzsolt
Copy link

My suggestion is that: in your handler have temporary struct, in to where you are binding your request data. Do validation on that struct which scope is only that handler method. If validation passes then map bound data to DTO or domain object and pass to business logic/service.

@aldas I mean yes, obviously this is possible. But the whole point of using Bind and Validate, for example, is to reduce the amount of boilerplate code you have to write.
If you have 100 routes, you need to have 100 temporary structs, do the mapping 100 times.

I feel like it would be a reasonable expectation for echo to support this.

The CustomBinder is indeed a solution (probably one that I will end up going with), but I would expect that many people would benefit from it being part of echo, rather than everyone implementing it on their own.

@jamesleeht
Copy link

jamesleeht commented Mar 23, 2023

Although I'd love to have this feature baked into the library, I've resorted to this CustomBinder:

package main

import (
	pgValidator "github.com/go-playground/validator"
	"github.com/labstack/echo/v4"
	"net/http"
)

type CustomValidator struct {
	validator *pgValidator.Validate
}

func NewCustomValidator() *CustomValidator {
	return &CustomValidator{validator: pgValidator.New()}
}

func (cv *CustomValidator) Validate(i interface{}) error {
	if err := cv.validator.Struct(i); err != nil {
		// Optionally, you could return the error to give each route more control over the status code
		return echo.NewHTTPError(http.StatusBadRequest, err.Error())
	}
	return nil
}

// ValidatingBinder ensures that you don't have to call both bind and validate in handler
type ValidatingBinder struct {
	*CustomValidator
}

func NewValidatingBinder(cv *CustomValidator) *ValidatingBinder {
	return &ValidatingBinder{CustomValidator: cv}
}

func (cb *ValidatingBinder) Bind(i interface{}, c echo.Context) error {
	db := new(echo.DefaultBinder)

	if err := db.Bind(i, c); err != echo.ErrUnsupportedMediaType {
		return err
	}

	if err := cb.CustomValidator.Validate(i); err != nil {
		return err
	}

	return nil
}

Hope it helps someone.

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

No branches or pull requests

7 participants