diff --git a/README.md b/README.md index ec16c58..1a84f8f 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ type MyConfig struct { } var c MyConfig -config.FromEnv().To(&c) +err := config.FromEnv().To(&c) ``` ## How It Works @@ -39,6 +39,10 @@ Its just simple, pure stdlib. * e.g. `PARENT__CHILD` * Env vars map to struct fields case insensitively * NOTE: Also true when using struct tags. +* Any errors encountered are aggregated into a single error value + * the entirety of the struct is always attempted + * failed conversions (i.e. converting "x" to an int) and file i/o are the only sources of errors + * missing values are not errors ## Why you should use this diff --git a/config.go b/config.go index ada05f8..1d85bd4 100644 --- a/config.go +++ b/config.go @@ -43,6 +43,7 @@ const ( type Builder struct { structDelim, sliceDelim string configMap map[string]string + failedFields []string } func newBuilder() *Builder { @@ -58,25 +59,32 @@ func newBuilder() *Builder { // * all int, uint, float variants // * bool, struct, string // * slice of any of the above, except for []struct{} -// It panics under the following circumstances: -// * target is not a struct pointer +// It returns an error if: // * struct contains unsupported fields (pointers, maps, slice of structs, channels, arrays, funcs, interfaces, complex) -func (c *Builder) To(target interface{}) { +// * there were errors doing file i/o +// It panics if: +// * target is not a struct pointer +func (c *Builder) To(target interface{}) error { + if reflect.ValueOf(target).Kind() != reflect.Ptr || reflect.ValueOf(target).Elem().Kind() != reflect.Struct { + panic("config: To(target) must be a *struct") + } c.populateStructRecursively(target, "") + if c.failedFields != nil { + return fmt.Errorf("config: the following fields had errors: %v", c.failedFields) + } + return nil } // From returns a new Builder, populated with the values from file. -// It panics if unable to open the file. func From(file string) *Builder { return newBuilder().From(file) } // From merges new values from file into the current config state, returning the Builder. -// It panics if unable to open the file. func (c *Builder) From(file string) *Builder { content, err := ioutil.ReadFile(file) if err != nil { - panic(fmt.Sprintf("config: failed to read file %v: %v", file, err)) + c.failedFields = append(c.failedFields, fmt.Sprintf("file[%v]", file)) } scanner := bufio.NewScanner(bytes.NewReader(content)) var ss []string @@ -84,7 +92,7 @@ func (c *Builder) From(file string) *Builder { ss = append(ss, scanner.Text()) } if scanner.Err() != nil { - panic(fmt.Sprintf("config: failed to scan file %v: %v", file, scanner.Err())) + c.failedFields = append(c.failedFields, fmt.Sprintf("file[%v]", file)) } c.mergeConfig(stringsToMap(ss)) return c @@ -129,6 +137,8 @@ func stringsToMap(ss []string) map[string]string { // slices and values are set directly. // nested structs recurse through this function. // values are derived from the field name, prefixed with the field names of any parents. +// +// failed fields are added to the builder for error reporting func (c *Builder) populateStructRecursively(structPtr interface{}, prefix string) { structValue := reflect.ValueOf(structPtr).Elem() for i := 0; i < structValue.NumField(); i++ { @@ -142,9 +152,13 @@ func (c *Builder) populateStructRecursively(structPtr interface{}, prefix string case reflect.Struct: c.populateStructRecursively(fieldPtr, key+c.structDelim) case reflect.Slice: - convertAndSetSlice(fieldPtr, stringToSlice(value, c.sliceDelim)) + for _, index := range convertAndSetSlice(fieldPtr, stringToSlice(value, c.sliceDelim)) { + c.failedFields = append(c.failedFields, fmt.Sprintf("%v[%v]", key, index)) + } default: - convertAndSetValue(fieldPtr, value) + if !convertAndSetValue(fieldPtr, value) { + c.failedFields = append(c.failedFields, key) + } } } } @@ -187,68 +201,85 @@ func stringToSlice(s, delim string) []string { // convertAndSetSlice builds a slice of a dynamic type. // It converts each entry in "values" to the elemType of the passed in slice. // The slice remains nil if "values" is empty. -func convertAndSetSlice(slicePtr interface{}, values []string) { +// All values are attempted. +// Returns the indices of failed values +func convertAndSetSlice(slicePtr interface{}, values []string) []int { sliceVal := reflect.ValueOf(slicePtr).Elem() elemType := sliceVal.Type().Elem() - for _, s := range values { + var failedIndices []int + for i, s := range values { valuePtr := reflect.New(elemType) - convertAndSetValue(valuePtr.Interface(), s) - sliceVal.Set(reflect.Append(sliceVal, valuePtr.Elem())) + if !convertAndSetValue(valuePtr.Interface(), s) { + failedIndices = append(failedIndices, i) + } else { + sliceVal.Set(reflect.Append(sliceVal, valuePtr.Elem())) + } } + return failedIndices } -// convertAndSetValue receives a settable of an arbitrary kind, and sets its value to s". +// convertAndSetValue receives a settable of an arbitrary kind, and sets its value to s, returning true. // It calls the matching strconv function on s, based on the settable's kind. // All basic types (bool, int, float, string) are handled by this function. // Slice and struct are handled elsewhere. -// Unhandled kinds panic. -// Errors in string conversion are ignored, and the settable remains a zero value. -func convertAndSetValue(settable interface{}, s string) { +// +// An unhandled kind or a failed parse returns false. +// False is used to prevent accidental logging of secrets as +// as the strconv include s in their error message. +func convertAndSetValue(settable interface{}, s string) bool { settableValue := reflect.ValueOf(settable).Elem() + var ( + err error + i int64 + u uint64 + b bool + f float64 + ) switch settableValue.Kind() { case reflect.String: settableValue.SetString(s) case reflect.Int: - val, _ := strconv.ParseInt(s, 10, 0) - settableValue.SetInt(val) + i, err = strconv.ParseInt(s, 10, 0) + settableValue.SetInt(i) case reflect.Int8: - val, _ := strconv.ParseInt(s, 10, 8) - settableValue.SetInt(val) + i, err = strconv.ParseInt(s, 10, 8) + settableValue.SetInt(i) case reflect.Int16: - val, _ := strconv.ParseInt(s, 10, 16) - settableValue.SetInt(val) + i, err = strconv.ParseInt(s, 10, 16) + settableValue.SetInt(i) case reflect.Int32: - val, _ := strconv.ParseInt(s, 10, 32) - settableValue.SetInt(val) + i, err = strconv.ParseInt(s, 10, 32) + settableValue.SetInt(i) case reflect.Int64: - val, _ := strconv.ParseInt(s, 10, 64) - settableValue.SetInt(val) + i, err = strconv.ParseInt(s, 10, 64) + settableValue.SetInt(i) case reflect.Uint: - val, _ := strconv.ParseUint(s, 10, 0) - settableValue.SetUint(val) + u, err = strconv.ParseUint(s, 10, 0) + settableValue.SetUint(u) case reflect.Uint8: - val, _ := strconv.ParseUint(s, 10, 8) - settableValue.SetUint(val) + u, err = strconv.ParseUint(s, 10, 8) + settableValue.SetUint(u) case reflect.Uint16: - val, _ := strconv.ParseUint(s, 10, 16) - settableValue.SetUint(val) + u, err = strconv.ParseUint(s, 10, 16) + settableValue.SetUint(u) case reflect.Uint32: - val, _ := strconv.ParseUint(s, 10, 32) - settableValue.SetUint(val) + u, err = strconv.ParseUint(s, 10, 32) + settableValue.SetUint(u) case reflect.Uint64: - val, _ := strconv.ParseUint(s, 10, 64) - settableValue.SetUint(val) + u, err = strconv.ParseUint(s, 10, 64) + settableValue.SetUint(u) case reflect.Bool: - val, _ := strconv.ParseBool(s) - settableValue.SetBool(val) + b, err = strconv.ParseBool(s) + settableValue.SetBool(b) case reflect.Float32: - val, _ := strconv.ParseFloat(s, 32) - settableValue.SetFloat(val) + f, err = strconv.ParseFloat(s, 32) + settableValue.SetFloat(f) case reflect.Float64: - val, _ := strconv.ParseFloat(s, 64) - settableValue.SetFloat(val) + f, err = strconv.ParseFloat(s, 64) + settableValue.SetFloat(f) default: - panic(fmt.Sprintf("config: cannot handle kind %v", settableValue.Type().Kind())) + err = fmt.Errorf("config: cannot handle kind %v", settableValue.Type().Kind()) } + return err == nil } diff --git a/config_test.go b/config_test.go index e5f8bf5..f453043 100644 --- a/config_test.go +++ b/config_test.go @@ -14,12 +14,16 @@ func Test_Integration(t *testing.T) { type D struct { E bool F bool `config:"feRdiNaND"` // case insensitive + J *int } type testConfig struct { A int `config:" "` // no-op/ignored B string `config:"B"` // no effect C []int D D `config:"dOg"` // case insensitive for sub-configs + G []int + H uint8 + I string } file, err := ioutil.TempFile("", "testenv") @@ -28,12 +32,21 @@ func Test_Integration(t *testing.T) { } defer os.Remove(file.Name()) + nonExistFile, err := ioutil.TempFile("", "nonexistfile") + if err != nil { + t.Fatalf("failed to create temporary file: %v", err) + } + os.Remove(nonExistFile.Name()) + testData := strings.Join([]string{ "A=1", "B=abc", "C=4 5 6", "DoG__E=true", "DoG__FErDINANd=true", + "G=1 y 2", // should log G[1] as it is an incorrect type, but still work with 0 and 2 + "H=-84", // should log H as it is an incorrect type + "I=", // should NOT log I as there is no way to tell if it is missing or deliberately empty }, "\n") _, err = file.Write([]byte(testData)) if err != nil { @@ -52,16 +65,79 @@ func Test_Integration(t *testing.T) { D: D{ E: true, F: true, + J: nil, }, + G: []int{1, 2}, + H: 0, + I: "", } + wantFailedFields := []string{"file[" + nonExistFile.Name() + "]", "dog__j", "g[1]", "h"} - From(file.Name()).FromEnv().To(&got) + builder := From(file.Name()).From(nonExistFile.Name()).FromEnv() + gotErr := builder.To(&got) if !reflect.DeepEqual(got, want) { - t.Errorf("Integration: got %+v, want %+v", got, want) + t.Errorf("Integration: got %+v, wantPanic %+v", got, want) + } + if gotErr == nil { + t.Errorf("Integration: should have had an error") + } + if !reflect.DeepEqual(builder.failedFields, wantFailedFields) { + t.Errorf("Integration: gotFailedFields %+v, wantFailedFields %+v", builder.failedFields, wantFailedFields) } os.Clearenv() } +func Test_shouldPanic(t *testing.T) { + t.Parallel() + + var s struct{} + var i int + + tests := []struct { + name string + target interface{} + wantPanic bool + }{ + { + name: "*struct", + target: &s, + wantPanic: false, + }, + { + name: "struct", + target: s, + wantPanic: true, + }, + { + name: "*int", + target: &i, + wantPanic: true, + }, + { + name: "int", + target: &i, + wantPanic: true, + }, + { + name: "nil", + target: nil, + wantPanic: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + defer func() { + if r := recover(); (r == nil) == tt.wantPanic { + t.Errorf("should have caused a panic") + } + }() + _ = FromEnv().To(tt.target) + }) + } +} + func Test_stringToSlice(t *testing.T) { t.Parallel() type args struct { @@ -111,9 +187,10 @@ func Test_convertAndSetValue(t *testing.T) { s string } tests := []struct { - name string - args args - want interface{} + name string + args args + want interface{} + wantErr bool }{ { name: "int", @@ -227,15 +304,27 @@ func Test_convertAndSetValue(t *testing.T) { }, want: func() interface{} { v := "abc"; return &v }(), }, + { + name: "bad convert", + args: args{ + settable: new(int), + s: "abc", + }, + want: func() interface{} { v := 0; return &v }(), + wantErr: true, + }, } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - convertAndSetValue(tt.args.settable, tt.args.s) + gotErr := convertAndSetValue(tt.args.settable, tt.args.s) if !reflect.DeepEqual(tt.args.settable, tt.want) { t.Errorf("convertAndSetValue = %v, want %v", tt.args.settable, tt.want) } + if gotErr == tt.wantErr { + t.Errorf("convertAndSetValue err = %v, want %v", gotErr, tt.wantErr) + } }) } } @@ -247,9 +336,10 @@ func Test_convertAndSetSlice(t *testing.T) { values []string } tests := []struct { - name string - args args - want interface{} + name string + args args + want interface{} + wantErr []int }{ { name: "string slice", @@ -257,7 +347,7 @@ func Test_convertAndSetSlice(t *testing.T) { slicePtr: new([]string), values: []string{"a", "b", "c", "def"}, }, - want: func() interface{} { v := []string{"a", "b", "c", "def"}; return &v }, + want: func() interface{} { v := []string{"a", "b", "c", "def"}; return &v }(), }, { name: "int slice", @@ -265,14 +355,30 @@ func Test_convertAndSetSlice(t *testing.T) { slicePtr: new([]int), values: []string{"1", "2", "3", "4"}, }, - want: func() interface{} { v := []int{1, 2, 3, 4}; return &v }, + want: func() interface{} { v := []int{1, 2, 3, 4}; return &v }(), + }, + { + name: "int slice - bad values", + args: args{ + slicePtr: new([]int), + values: []string{"1", "x", "3", "4"}, + }, + want: func() interface{} { v := []int{1, 3, 4}; return &v }(), + wantErr: []int{1}, }, } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - convertAndSetSlice(tt.args.slicePtr, tt.args.values) + gotErr := convertAndSetSlice(tt.args.slicePtr, tt.args.values) + if !reflect.DeepEqual(tt.args.slicePtr, tt.want) { + t.Errorf("convertAndSetSlice = %v, want: %v", tt.args.slicePtr, tt.want) + } + if !reflect.DeepEqual(gotErr, tt.wantErr) { + t.Errorf("convertAndSetSlice err = %v, want: %v", gotErr, tt.wantErr) + } + }) } } diff --git a/example_test.go b/example_test.go index 01ce2cb..650d63f 100644 --- a/example_test.go +++ b/example_test.go @@ -44,6 +44,18 @@ func Example() { // [0.0.0.0 1.1.1.1 2.2.2.2] 3 } +func Example_errorHandling() { + os.Clearenv() + os.Setenv("PORT", "X") + + var c MyConfig + err := config.FromEnv().To(&c) + fmt.Println(err) + + // Output: + // config: the following fields had errors: [port feature_flag] +} + func Example_fromFileWithOverride() { tempFile, _ := ioutil.TempFile("", "temp") tempFile.Write([]byte(strings.Join([]string{"PORT=1234", "FEATURE_FLAG=true"}, "\n")))