From 131c377371e4891c1a96927e7aeab6547915d662 Mon Sep 17 00:00:00 2001 From: Kyriacos <58787538+kkyr@users.noreply.github.com> Date: Sun, 10 Dec 2023 02:13:57 +0100 Subject: [PATCH] Fix issue where map fields would not be validated (#27) * Maps are now properly populated * Update doc.go * Update linter rules * Update to go 1.21 * Add test case * Update golangci-lint to latest version * Update godoc --- .golangci.yml | 2 + build/lint.sh | 2 +- doc.go | 307 ++++++++++++++++++++------------------ field.go | 40 ++++- field_test.go | 21 +++ fig_test.go | 25 +++- go.mod | 2 +- testdata/invalid/pod.json | 9 +- testdata/invalid/pod.toml | 4 + testdata/invalid/pod.yaml | 5 + testdata/valid/pod.json | 10 +- testdata/valid/pod.toml | 4 + testdata/valid/pod.yaml | 5 + 13 files changed, 275 insertions(+), 161 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 051ef44..2076b62 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -19,6 +19,8 @@ linters: - nlreturn - errname - nonamedreturns + - tagalign + - depguard issues: exclude-rules: diff --git a/build/lint.sh b/build/lint.sh index 8c7fd43..114403e 100755 --- a/build/lint.sh +++ b/build/lint.sh @@ -6,7 +6,7 @@ export CGO_ENABLED=1 if ! command -v golangci-lint &> /dev/null; then echo "Installing golangci-lint" - curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.51.0 + curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.55.2 fi echo -n "Running golangci-lint: " diff --git a/doc.go b/doc.go index afd9f67..0d402e3 100644 --- a/doc.go +++ b/doc.go @@ -5,69 +5,69 @@ Config files may be defined in yaml, json or toml format. When you call `Load()`, fig takes the following steps: - 1. Fills config struct from the config file (if enabled) - 2. Fills config struct from the environment (if enabled) - 3. Sets defaults (where applicable) - 4. Validates required fields (where applicable) + 1. Fills config struct from the config file (if enabled) + 2. Fills config struct from the environment (if enabled) + 3. Sets defaults (where applicable) + 4. Validates required fields (where applicable) -Example +# Example Define your configuration file in the root of your project: - # config.yaml + # config.yaml - build: "2020-01-09T12:30:00Z" + build: "2020-01-09T12:30:00Z" - server: - ports: - - 8080 - cleanup: 1h + server: + ports: + - 8080 + cleanup: 1h - logger: - level: "warn" - trace: true + logger: + level: "warn" + trace: true Define your struct and load it: - package main + package main - import ( - "fmt" + import ( + "fmt" - "github.com/kkyr/fig" - ) + "github.com/kkyr/fig" + ) - type Config struct { - Build time.Time `fig:"build" validate:"required"` - Server struct { - Host string `fig:"host" default:"127.0.0.1"` - Ports []int `fig:"ports" default:"[80,443]"` - Cleanup time.Duration `fig:"cleanup" default:"30m"` - } - Logger struct { - Level string `fig:"level" default:"info"` - Trace bool `fig:"trace"` - } - } + type Config struct { + Build time.Time `fig:"build" validate:"required"` + Server struct { + Host string `fig:"host" default:"127.0.0.1"` + Ports []int `fig:"ports" default:"[80,443]"` + Cleanup time.Duration `fig:"cleanup" default:"30m"` + } + Logger struct { + Level string `fig:"level" default:"info"` + Trace bool `fig:"trace"` + } + } - func main() { - var cfg Config - _ = fig.Load(&cfg) + func main() { + var cfg Config + _ = fig.Load(&cfg) - fmt.Printf("%+v\n", cfg) - // Output: {Build:2019-12-25 00:00:00 +0000 UTC Server:{Host:127.0.0.1 Ports:[8080] Cleanup:1h0m0s} Logger:{Level:warn Trace:true}} - } + fmt.Printf("%+v\n", cfg) + // Output: {Build:2019-12-25 00:00:00 +0000 UTC Server:{Host:127.0.0.1 Ports:[8080] Cleanup:1h0m0s} Logger:{Level:warn Trace:true}} + } -Configuration +# Configuration Pass options as additional parameters to `Load()` to configure fig's behaviour. -IgnoreFile +# IgnoreFile Do not look for any configuration file with `IgnoreFile()`. - fig.Load(&cfg, fig.IgnoreFile()) + fig.Load(&cfg, fig.IgnoreFile()) If IgnoreFile is given then any other configuration file related options like `File` and `Dirs` are simply ignored. @@ -76,30 +76,30 @@ File & Dirs By default fig searches for a file named `config.yaml` in the directory it is run from. Change the file and directories fig searches in with `File()` and `Dirs()`. - fig.Load(&cfg, - fig.File("settings.json"), - fig.Dirs(".", "home/user/myapp", "/opt/myapp"), - ) + fig.Load(&cfg, + fig.File("settings.json"), + fig.Dirs(".", "home/user/myapp", "/opt/myapp"), + ) Fig searches for the file in dirs sequentially and uses the first matching file. The decoder (yaml/json/toml) used is picked based on the file's extension. -Tag +# Tag The struct tag key tag fig looks for to find the field's alt name can be changed using `Tag()`. - type Config struct { - Host string `yaml:"host" validate:"required"` - Level string `yaml:"level" default:"info"` - } + type Config struct { + Host string `yaml:"host" validate:"required"` + Level string `yaml:"level" default:"info"` + } - var cfg Config - fig.Load(&cfg, fig.Tag("yaml")) + var cfg Config + fig.Load(&cfg, fig.Tag("yaml")) By default fig uses the tag key `fig`. -Environment +# Environment Fig can be configured to additionally set fields using the environment. This behaviour can be enabled using the option `UseEnv(prefix)`. If loading from file is also enabled then first the struct is loaded @@ -113,50 +113,54 @@ A field's path is formed by prepending its name with the names of all the surrou If a field has an alt name defined in its struct tag then that name is preferred over its struct name. - type Config struct { - Build time.Time - LogLevel string `fig:"log_level"` - Server struct { - Host string - } - } + type Config struct { + Build time.Time + LogLevel string `fig:"log_level"` + Server struct { + Host string + } + } With the struct above and `UseEnv("myapp")` fig would search for the following environment variables: - MYAPP_BUILD - MYAPP_LOG_LEVEL - MYAPP_SERVER_HOST + MYAPP_BUILD + MYAPP_LOG_LEVEL + MYAPP_SERVER_HOST Fields contained in struct slices whose elements already exists can be also be set via the environment in the form PARENT_IDX_FIELD, where idx is the index of the field in the slice. - type Config struct { - Server []struct { - Host string - } - } + type Config struct { + Server []struct { + Host string + } + } With the config above individual servers may be configured with the following environment variable: - MYAPP_SERVER_0_HOST - MYAPP_SERVER_1_HOST - ... + MYAPP_SERVER_0_HOST + MYAPP_SERVER_1_HOST + ... Note: the Server slice must already have members inside it (i.e. from loading of the configuration file) for the containing fields to be altered via the environment. Fig will not instantiate and insert elements into the slice. -Time +## Limitations + +Maps and map values cannot be populated from the environment. + +# Time Change the layout fig uses to parse times using `TimeLayout()`. - type Config struct { - Date time.Time `fig:"date" default:"12-25-2019"` - } + type Config struct { + Date time.Time `fig:"date" default:"12-25-2019"` + } - var cfg Config - fig.Load(&cfg, fig.TimeLayout("01-02-2006")) + var cfg Config + fig.Load(&cfg, fig.TimeLayout("01-02-2006")) - fmt.Printf("%+v", cfg) - // Output: {Date:2019-12-25 00:00:00 +0000 UTC} + fmt.Printf("%+v", cfg) + // Output: {Date:2019-12-25 00:00:00 +0000 UTC} By default fig parses time using the `RFC.3339` layout (`2006-01-02T15:04:05Z07:00`). @@ -165,105 +169,116 @@ By default fig parses time using the `RFC.3339` layout (`2006-01-02T15:04:05Z07: By default fig ignores any fields in the config file that are not present in the struct. This behaviour can be changed using `UseStrict()` to achieve strict parsing. When strict parsing is enabled, extra fields in the config file will cause an error. -Required +# Required A validate key with a required value in the field's struct tag makes fig check if the field has been set after it's been loaded. Required fields that are not set are returned as an error. - type Config struct { - Host string `fig:"host" validate:"required"` // or simply `validate:"required"` - } + type Config struct { + Host string `fig:"host" validate:"required"` // or simply `validate:"required"` + } Fig uses the following properties to check if a field is set: - basic types: != to its zero value ("" for str, 0 for int, etc.) - slices, arrays: len() > 0 - pointers*, interfaces: != nil - structs: always true (use a struct pointer to check for struct presence) - time.Time: !time.IsZero() - time.Duration: != 0 + basic types: != to its zero value ("" for str, 0 for int, etc.) + slices, arrays: len() > 0 + pointers*, interfaces: != nil + structs: always true (use a struct pointer to check for struct presence) + time.Time: !time.IsZero() + time.Duration: != 0 - *pointers to non-struct types (with the exception of time.Time) are de-referenced if they are non-nil and then checked + *pointers to non-struct types (with the exception of time.Time) are de-referenced if they are non-nil and then checked See example below to help understand: - type Config struct { - A string `validate:"required"` - B *string `validate:"required"` - C int `validate:"required"` - D *int `validate:"required"` - E []float32 `validate:"required"` - F struct{} `validate:"required"` - G *struct{} `validate:"required"` - H struct { - I interface{} `validate:"required"` - J interface{} `validate:"required"` - } `validate:"required"` - K *[]bool `validate:"required"` - L []uint `validate:"required"` - M *time.Time `validate:"required"` - N *regexp.Regexp `validate:"required"` - } - - var cfg Config - - // simulate loading of config file - b := "" - cfg.B = &b - cfg.H.I = 5.5 - cfg.K = &[]bool{} - cfg.L = []uint{5} - m := time.Time{} - cfg.M = &m - - err := fig.Load(&cfg) - fmt.Print(err) - // A: required validation failed, B: required validation failed, C: required validation failed, D: required validation failed, E: required validation failed, G: required validation failed, H.J: required validation failed, K: required validation failed, M: required validation failed, N: required validation failed - -Default + type Config struct { + A string `validate:"required"` + B *string `validate:"required"` + C int `validate:"required"` + D *int `validate:"required"` + E []float32 `validate:"required"` + F struct{} `validate:"required"` + G *struct{} `validate:"required"` + H struct { + I interface{} `validate:"required"` + J interface{} `validate:"required"` + } `validate:"required"` + K *[]bool `validate:"required"` + L []uint `validate:"required"` + M *time.Time `validate:"required"` + N *regexp.Regexp `validate:"required"` + } + + var cfg Config + + // simulate loading of config file + b := "" + cfg.B = &b + cfg.H.I = 5.5 + cfg.K = &[]bool{} + cfg.L = []uint{5} + m := time.Time{} + cfg.M = &m + + err := fig.Load(&cfg) + fmt.Print(err) + // A: required validation failed, B: required validation failed, C: required validation failed, D: required validation failed, E: required validation failed, G: required validation failed, H.J: required validation failed, K: required validation failed, M: required validation failed, N: required validation failed + +# Default A default key in the field tag makes fig fill the field with the value specified when the field is not otherwise set. Fig attempts to parse the value based on the field's type. If parsing fails then an error is returned. - type Config struct { - Port int `fig:"port" default:"8000"` // or simply `default:"8000"` - } - + type Config struct { + Port int `fig:"port" default:"8000"` // or simply `default:"8000"` + } A default value can be set for the following types: - all basic types except bool and complex - time.Time - time.Duration - *regexp.Regexp - slices (of above types) + all basic types except bool and complex + time.Time + time.Duration + *regexp.Regexp + slices (of above types) Successive elements of slice defaults should be separated by a comma. The entire slice can optionally be enclosed in square brackets: - type Config struct { - Durations []time.Duration `default:"[30m,1h,90m,2h]"` // or `default:"30m,1h,90m,2h"` - } + type Config struct { + Durations []time.Duration `default:"[30m,1h,90m,2h]"` // or `default:"30m,1h,90m,2h"` + } + +## Limitations: + + 1. Boolean values: + Fig cannot distinguish between false and an unset value for boolean types. + As a result, default values for booleans are not currently supported. + + 2. Maps: + Maps are not supported because providing a map in a string form would be complex and error-prone. + Users are encouraged to use structs instead for more reliable and structured data handling. -Note: the default setter knows if it should fill a field or not by comparing if the current value of the field is equal to the corresponding zero value for that field's type. This happens after the configuration is loaded and has the implication that the zero value set explicitly by the user will get overwritten by any default value registered for that field. It's for this reason that defaults on booleans are not permitted, as a boolean field with a default value of `true` would always be true (since if it were set to false it'd be overwritten). + 3. Map values: + Values retrieved from a map through reflection are not addressable. + Therefore, setting default values for map values is not currently supported. -Mutual exclusion +# Mutual exclusion The required validation and the default field tags are mutually exclusive as they are contradictory. This is not allowed: - type Config struct { - Level string `validate:"required" default:"warn"` // will result in an error - } + type Config struct { + Level string `validate:"required" default:"warn"` // will result in an error + } -Errors +# Errors A wrapped error `ErrFileNotFound` is returned when fig is not able to find a config file to load. This can be useful for instance to fallback to a different configuration loading mechanism. - var cfg Config - err := fig.Load(&cfg) - if errors.Is(err, fig.ErrFileNotFound) { - // load config from elsewhere - } + var cfg Config + err := fig.Load(&cfg) + if errors.Is(err, fig.ErrFileNotFound) { + // load config from elsewhere + } */ package fig diff --git a/field.go b/field.go index 3910fe1..8756761 100644 --- a/field.go +++ b/field.go @@ -48,6 +48,13 @@ func flattenField(f *field, fs *[]*field, tagKey string) { flattenField(child, fs, tagKey) } } + + case reflect.Map: + for _, key := range f.v.MapKeys() { + child := newMapField(f, key, tagKey) + *fs = append(*fs, child) + flattenField(child, fs, tagKey) + } } } @@ -60,7 +67,7 @@ func newStructField(parent *field, idx int, tagKey string) *field { v: parent.v.Field(idx), t: parent.v.Field(idx).Type(), st: parent.t.Field(idx), - sliceIdx: -1, + sliceIdx: -1, // not applicable for struct fields } f.structTag = parseTag(f.st.Tag, tagKey) return f @@ -81,14 +88,32 @@ func newSliceField(parent *field, idx int, tagKey string) *field { return f } +// newMapField is a constructor for a field that is a map entry. +// key is the key of the map entry, and tagKey is the key of the +// tag that contains the field alt name (if any). +func newMapField(parent *field, key reflect.Value, tagKey string) *field { + f := &field{ + parent: parent, + v: parent.v.MapIndex(key), + t: parent.v.MapIndex(key).Type(), + st: parent.st, + sliceIdx: -1, // not applicable for map entries + mapKey: &key, + } + f.structTag = parseTag(f.st.Tag, tagKey) + return f +} + // field is a settable field of a config object. type field struct { parent *field - v reflect.Value - t reflect.Type - st reflect.StructField - sliceIdx int // >=0 if this field is a member of a slice. + v reflect.Value + t reflect.Type + st reflect.StructField + + sliceIdx int // >=0 if this field is a member of a slice. + mapKey *reflect.Value // key of the map entry if this field is a map entry, nil otherwise. structTag } @@ -102,6 +127,9 @@ func (f *field) name() string { if f.sliceIdx >= 0 { return fmt.Sprintf("[%d]", f.sliceIdx) } + if f.mapKey != nil { + return fmt.Sprintf("[%s]", f.mapKey.String()) + } if f.altName != "" { return f.altName } @@ -120,7 +148,7 @@ func (f *field) path() (path string) { path += f.name() // if it's a slice/array we don't want a dot before the slice indexer // e.g. we want A[0].B instead of A.[0].B - if f.t.Kind() != reflect.Slice && f.t.Kind() != reflect.Array { + if f.t.Kind() != reflect.Slice && f.t.Kind() != reflect.Array && f.t.Kind() != reflect.Map { path += "." } } diff --git a/field_test.go b/field_test.go index 9deba47..923c92b 100644 --- a/field_test.go +++ b/field_test.go @@ -44,6 +44,27 @@ func Test_flattenCfg(t *testing.T) { checkField(t, fields[9], "k", "J.k") } +func Test_flattenCfg_Map(t *testing.T) { + type A struct { + B string `fig:"b"` + } + + cfg := struct { + D map[string]A + }{} + cfg.D = map[string]A{ + "key1": {B: "b"}, + } + + fields := flattenCfg(&cfg, "fig") + if len(fields) != 3 { + t.Fatalf("len(fields) == %d, expected %d", len(fields), 3) + } + checkField(t, fields[0], "D", "D") + checkField(t, fields[1], "[key1]", "D[key1]") + checkField(t, fields[2], "b", "D[key1].b") +} + func Test_newStructField(t *testing.T) { cfg := struct { A int `fig:"a" default:"5" validate:"required"` diff --git a/fig_test.go b/fig_test.go index fec130e..737ab64 100644 --- a/fig_test.go +++ b/fig_test.go @@ -32,12 +32,17 @@ type Spec struct { Volumes []*Volume `fig:"volumes"` } +type Arg struct { + Value string `fig:"value" validate:"required"` +} + type Container struct { - Name string `fig:"name" validate:"required"` - Image string `fig:"image" validate:"required"` - Command []string `fig:"command"` - Env []Env `fig:"env"` - Ports []Port `fig:"ports"` + Name string `fig:"name" validate:"required"` + Image string `fig:"image" validate:"required"` + Command []string `fig:"command"` + Env []Env `fig:"env"` + Ports []Port `fig:"ports"` + Args map[string]Arg `fig:"args" validate:"required"` Resources struct { Limits struct { CPU string `fig:"cpu"` @@ -117,6 +122,14 @@ func validPodConfig() Pod { Name: "config", }, }, + Args: map[string]Arg{ + "-w": { + Value: "true", + }, + "--mem": { + Value: "low", + }, + }, }, } pod.Spec.Containers[0].Resources.Limits.CPU = "0.1" @@ -197,6 +210,8 @@ func Test_fig_Load_Required(t *testing.T) { "kind", "metadata.master", "spec.containers[0].image", + "spec.containers[0].args[-w].value", + "spec.containers[0].args[-o].value", "spec.volumes[0].configMap.items", "spec.volumes[1].name", } diff --git a/go.mod b/go.mod index 7114303..82619fb 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/kkyr/fig -go 1.20 +go 1.21 require ( github.com/mitchellh/mapstructure v1.5.0 diff --git a/testdata/invalid/pod.json b/testdata/invalid/pod.json index 5b32ff7..d4cc98c 100644 --- a/testdata/invalid/pod.json +++ b/testdata/invalid/pod.json @@ -36,7 +36,14 @@ "mountPath": "/redis-master", "name": "config" } - ] + ], + "args": { + "-w": { + "value": "" + }, + "-o": { + } + } } ], "volumes": [ diff --git a/testdata/invalid/pod.toml b/testdata/invalid/pod.toml index 0a6d627..dc392d6 100644 --- a/testdata/invalid/pod.toml +++ b/testdata/invalid/pod.toml @@ -30,6 +30,10 @@ name = "redis" mountPath = "/redis-master" name = "config" + [spec.containers.args] + "-w" = { value = "" } + "-o" = { } + [[spec.volumes]] name = "data" diff --git a/testdata/invalid/pod.yaml b/testdata/invalid/pod.yaml index 8293481..5958688 100644 --- a/testdata/invalid/pod.yaml +++ b/testdata/invalid/pod.yaml @@ -20,6 +20,11 @@ spec: name: data - mountPath: /redis-master name: config + args: + "-w": + value: "" + "-o": + value: volumes: - name: data configMap: diff --git a/testdata/valid/pod.json b/testdata/valid/pod.json index eeafa74..d9b8d3f 100644 --- a/testdata/valid/pod.json +++ b/testdata/valid/pod.json @@ -39,7 +39,15 @@ "mountPath": "/redis-master", "name": "config" } - ] + ], + "args": { + "-w": { + "value": "true" + }, + "--mem": { + "value": "low" + } + } } ], "volumes": [ diff --git a/testdata/valid/pod.toml b/testdata/valid/pod.toml index dd7b9a2..d88e175 100644 --- a/testdata/valid/pod.toml +++ b/testdata/valid/pod.toml @@ -34,6 +34,10 @@ master = true mountPath = "/redis-master" name = "config" + [spec.containers.args] + "-w" = { value = "true" } + "--mem" = { value = "low" } + [[spec.volumes]] name = "data" diff --git a/testdata/valid/pod.yaml b/testdata/valid/pod.yaml index 3919442..609150d 100644 --- a/testdata/valid/pod.yaml +++ b/testdata/valid/pod.yaml @@ -23,6 +23,11 @@ spec: name: data - mountPath: /redis-master name: config + args: + "-w": + value: "true" + "--mem": + value: "low" volumes: - name: data - name: config