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

WIP Validation for env variables #13

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Oudwins
Copy link

@Oudwins Oudwins commented Jun 22, 2024

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.....

@Oudwins Oudwins marked this pull request as draft June 22, 2024 05:55
@anthdm
Copy link
Owner

anthdm commented Jun 22, 2024

@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
Copy link
Author

Oudwins commented Jun 23, 2024

@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 Oudwins changed the title Validation for env variables WIP Validation for env variables Jun 23, 2024
@Oudwins
Copy link
Author

Oudwins commented Jun 23, 2024

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("[email protected]") // returns an []string bool

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

@Oudwins
Copy link
Author

Oudwins commented Aug 12, 2024

Hey Anthony,

Not sure when you will see this at it seems like you are AMA at the moment. Something was bugging me about the implementation so I ended up rewriting the entire validation library, I called it Zog....

Here is an example of the code for validating envs:

import (
  z "github.com/Oudwins/zog"
  "github.com/Oudwins/zog/zenv"
)

var envSchema = z.Struct(z.Schema{
	"PORT": z.Int().GT(1000).LT(65535).Default(3000),
	"Db": z.Struct(z.Schema{
		"Host": z.String().Default("localhost"),
		"User": z.String().Default("root"),
		"Pass": z.String().Default("root"),
	}),
})
var Env = struct {
	PORT int // zog will automatically coerce the PORT env to an int
	Db   struct {
		Host string `zog:"DB_HOST"` // we specify the zog tag to tell zog to parse the field from the DB_HOST environment variable
		User string `zog:"DB_USER"`
		Pass string `zog:"DB_PASS"`
	}
}{}

// Init our typesafe env vars, panic if any envs are missing
func Init() {
  errs := envSchema.Parse(zenv.NewDataProvider(), &Env)
  if errs != nil {
    log.Fatal(errs)
  }
}

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

Successfully merging this pull request may close these issues.

2 participants