livebud / bud

The Full-Stack Web Framework for Go
MIT License
5.58k stars 178 forks source link

Required controller input validation #171

Open matthewmueller opened 2 years ago

matthewmueller commented 2 years ago

Given the following controller/controller.go:

package controller

type Controller struct {}

type User struct {
  Name string `json:"name"`
  Email string `json:"email"`
}

func (c *Controller) Create(name, email string) (*User, error) {
  return &User{name, email}, nil
}

If you run the following request: GET /?name=Alice, you'll get back the following response:

{
  "name": "Alice",
  "email": ""
}

It should actually be a validation error, "email cannot be blank", never run the controller action and most likely return a 422.

matthewmueller commented 2 years ago

I'm thinking of doing an easy version of this first where we generate a struct compatible with https://github.com/go-playground/validator.

Long-run I'd like to generate the unmarshaler (maybe using or inspired by easyjson), but that's going to take some time and I'd like to have the interface working earlier.

jfmario commented 2 years ago

Users already have to add json:"post_id" and so forth to their input structs, would it be a valid solution to document to users that validator.Struct will be executed against the input so that they can add validate tags to struct fields as needed? Or is this depending too much on a third party API?

matthewmueller commented 2 years ago

That's even more straightforward solution. I think that'd be a great first step!

My main concern of just allowing an unaltered version of this is around needing to break the API at some point. I'm doubtful all the decisions in validator will be good decisions for Bud.

But maybe that could be a 1.x or 2.x transition. I like the idea of getting all the pieces in place, then refining as we learn more.

matthewmueller commented 2 years ago

(Linking an existing draft: https://github.com/livebud/bud/discussions/15)

If we go the validator route, we'll still want to generate required struct tags. For example:

func (c *Controller) Signup(email, password string, age *int) error { ... }

Would correspond to the following generated struct:

type SignupInput struct {
  Email string `json:"email" validate:"required"`
  Password string `json:"password" validate:"required"`
  Age *int `json:"age"`
}

Bud's API contract is required by default rather than optional by default like Go. If you want optional, you use pointers.

The documentation has more details: https://www.notion.so/Hey-Bud-4d81622cc49942f9917c5033e5205c69#f385e0bd96d349f1b78da9fbfc305eb6

FeldrinH commented 2 years ago

Using validate:"required" has the problem that this makes the default value invalid, so something like {"email": "", "password": ""} would be considered invalid, which would be problematic if the default value is in fact a valid value. This would be especially problematic for numbers, where the default value is 0.

matthewmueller commented 2 years ago

That's a good point @FeldrinH. I think this needs to be thought through a bit further, but the place we'd like to get to is when you have

func Signup(email, password string, age *int)

The following would be allowed:

{ "email": "", "password": "" }

But this would not be:

{ "email": "" }

Because password wasn't defined.