labstack / echo

High performance, minimalist Go web framework
https://echo.labstack.com
MIT License
29.48k stars 2.21k forks source link

Add Context#BindAndValidate() #1105

Closed vishr closed 5 years ago

dennypenta commented 6 years ago

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

p581581 commented 6 years ago

Any updates?

Tethik commented 6 years ago

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[bot] commented 5 years ago

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.

lordzsolt commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/labstack/echo/issues/1670#issue-737638643

aldas commented 2 years ago

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 https://github.com/labstack/echo/issues/438

lordzsolt commented 2 years ago

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 commented 1 year ago

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.