anthdm / superkit

MIT License
703 stars 68 forks source link

WIP Validation for env variables #13

Open Oudwins opened 1 week ago

Oudwins commented 1 week ago

Here is the draft as per the proposal in #9.

Some challenges/concerns I have identified:

  1. The Env struct should be inside the user's code but it should be used by Kit (for fetching things like the current environment). Not sure what the best way to approach this
  2. Like I mentioned in the issue not sure about the current implementation of default since you can provide Default & required at the same time which seem unintuitive.

Possible improvements I am not sure if this is something you are considering for the validation library but specifically for env validation it might be nice to have some version of type conversion:

var Env = struct {
    ROTATING_JWT_SECRETS     []string
       //....
}{
    ROTATING_JWT_SECRETS: v.Env("ROTATING_JWT_SECRETS", v.ToList(","), v.ListMin(3)).Validate(),
}

However, this can bring with it lots of footguns since either we make Rules be able to handle every data type (for example required needs to handle strings but also numbers, slices...), we make specific rules for each data type or change the entire API. The first I imagine will be slow, the second quite easy to mix Min for numbers with Min for strings and break everything and the latter a lot of work.....

anthdm commented 1 week ago

@Oudwins This is getting in the right direction. I went over it very briefly. I will dig deeper tomorrow and share some of my thoughts. Thanks a lot for the effort. Appreciate this a lot.

Oudwins commented 1 week ago

@anthdm No worries. My pleasure. Actually I got sick and started working on a API redesign. Once I have a working version I'll tag you on here. Might be you like it more.

Oudwins commented 1 week ago

Alright I am back! Here are the updates @anthdm

1. Changed the API slightly

// before
v.Env("SUPERKIT_SECRET", v.Min(5), v.ContainsSpecial()).Default(".").Validate()
// now
v.Env[string]("SUPER_KIT_SECRET", v.Rules(v.Min(5), V.ContainsSpecial()), "optional_default")

Why new api? Mostly because I couldn't find another way to tell the function to convert the env variable to another type. With the new implementation you can do things like:

v.Env[int]("MAX_CONNECTIONS", v.Rules(v.GT(0)), 10) // will fetch the env variable, coerce it to an int and return it, if env variable is 0 will assume the default value of 10

2. Default now only overrides if the env value is a zero value. Otherwise Env will still panic

os.SetEnv("TEST", "10")
v.Env[int]("TEST", v.Rules(v.GTE(100)), 100) // this will panic because TEST is not zero value

I think this makes more sense, if you set an env variable to an invalid value its more explicit to panic rather than silently give you the default.

3. I was looking at how zod does type coercion and it looks like a very nice API, not sure if it would work in go

const schema = z.coerce.number().GTE(10) 
const number = schema.parse("10") // will convert "10" to a number and then run the validation returning the number if valid

I will have to think a little about if this can/should be implemented

4. I got a little carried away and rewrote the entire validation library.... I didn't put it in this PR because its quite a lot of code and it might not be a direction you want for the library but I'll share just in case.

I implemented it a little closer to what zod does. With method chaining. The main benefit is that it will never allow you to pick incompatible rules (i.e Email rule for an int). The down side, and why I understan you didn't go this route, is that its a little bit of a pain to implement, but I think it provides a better DX.

schema := v.Schema{
   "fieldname": v.String().Min(5).Email().Contains("@gmail.com")
}

It also has the semi-added benefit that I placed the validation function as a method of the actual rules array. So you can do things like this:

errs, ok := v.String().Email().Validate("superkit@example.com") // returns an []string bool

PS: I'll be busy with work from tomorrow so probably won't make much progress until next weekend