From ca08f13725339c1b412535322dcb10b767089520 Mon Sep 17 00:00:00 2001 From: BowlOfSoup Date: Thu, 20 Jun 2024 14:02:18 +0200 Subject: [PATCH 1/6] ISSUE-210 - Add Required property to indicate a config registration can't be empty or nil (+ tests) --- config/config_test.go | 101 ++++++++++++++++++++++++++++++++++++++++++ config/default.go | 74 +++++++++++++++---------------- config/entry.go | 42 +++++++++++++++++- 3 files changed, 178 insertions(+), 39 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 97cd8679..ddbb3120 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -705,3 +705,104 @@ func TestConfig(t *testing.T) { }) }) } + +func TestEntryValidationRequired(t *testing.T) { + tests := []struct { + name string + entry Entry + key string + expectsError bool + errMsg string + }{ + { + name: "Required string empty", + entry: Entry{Value: "", Type: reflect.String, Required: true}, + key: "emptyString", + expectsError: true, + errMsg: `"emptyString" is required and cannot be an empty string`, + }, + { + name: "Required string non-empty", + entry: Entry{Value: "hello", Type: reflect.String, Required: true}, + key: "nonEmptyString", + expectsError: false, + }, + { + name: "Required int zero", + entry: Entry{Value: 0, Type: reflect.Int, Required: true}, + key: "zeroInt", + expectsError: true, + errMsg: `"zeroInt" is required and cannot be zero`, + }, + { + name: "Required int non-zero", + entry: Entry{Value: 123, Type: reflect.Int, Required: true}, + key: "nonZeroInt", + expectsError: false, + }, + { + name: "Required (string) slice empty", + entry: Entry{Value: []string{}, Type: reflect.String, IsSlice: true, Required: true}, + key: "emptySlice", + expectsError: true, + errMsg: `"emptySlice" is required and cannot be an empty string`, + }, + { + name: "Required slice non-empty", + entry: Entry{Value: []string{"hello"}, Type: reflect.String, IsSlice: true, Required: true}, + key: "nonEmptySlice", + expectsError: false, + }, + { + name: "Required float zero", + entry: Entry{Value: 0.0, Type: reflect.Float64, Required: true}, + key: "zeroFloat", + expectsError: true, + errMsg: `"zeroFloat" is required and cannot be zero`, + }, + { + name: "Required float non-zero", + entry: Entry{Value: 1.23, Type: reflect.Float64, Required: true}, + key: "nonZeroFloat", + expectsError: false, + }, + { + name: "Required map empty", + entry: Entry{Value: map[string]string{}, Type: reflect.Map, Required: true}, + key: "emptyMap", + expectsError: true, + errMsg: `"emptyMap" is required and cannot be an empty map`, + }, + { + name: "Required map non-empty", + entry: Entry{Value: map[string]string{"key": "value"}, Type: reflect.Map, Required: true}, + key: "nonEmptyMap", + expectsError: false, + }, + { + name: "Required nil pointer", + entry: Entry{Value: (*string)(nil), Type: reflect.Ptr, Required: true}, + key: "nilPointer", + expectsError: true, + errMsg: `"nilPointer" is required and cannot be nil`, + }, + { + name: "Required non-nil pointer", + entry: Entry{Value: new(string), Type: reflect.Ptr, Required: true}, + key: "nonNilPointer", + expectsError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.entry.validate(tt.key) + if tt.expectsError { + require.Error(t, err) + assert.Equal(t, tt.errMsg, err.Error()) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/config/default.go b/config/default.go index 3fa548e5..b9904153 100644 --- a/config/default.go +++ b/config/default.go @@ -4,49 +4,49 @@ import "reflect" var configDefaults = object{ "app": object{ - "name": &Entry{"goyave", []any{}, reflect.String, false}, - "environment": &Entry{"localhost", []any{}, reflect.String, false}, - "debug": &Entry{true, []any{}, reflect.Bool, false}, - "defaultLanguage": &Entry{"en-US", []any{}, reflect.String, false}, + "name": &Entry{"goyave", []any{}, reflect.String, false, false}, + "environment": &Entry{"localhost", []any{}, reflect.String, false, false}, + "debug": &Entry{true, []any{}, reflect.Bool, false, false}, + "defaultLanguage": &Entry{"en-US", []any{}, reflect.String, false, false}, }, "server": object{ - "host": &Entry{"127.0.0.1", []any{}, reflect.String, false}, - "domain": &Entry{"", []any{}, reflect.String, false}, - "port": &Entry{8080, []any{}, reflect.Int, false}, - "writeTimeout": &Entry{10, []any{}, reflect.Int, false}, - "readTimeout": &Entry{10, []any{}, reflect.Int, false}, - "readHeaderTimeout": &Entry{10, []any{}, reflect.Int, false}, - "idleTimeout": &Entry{20, []any{}, reflect.Int, false}, - "websocketCloseTimeout": &Entry{10, []any{}, reflect.Int, false}, - "maxUploadSize": &Entry{10.0, []any{}, reflect.Float64, false}, + "host": &Entry{"127.0.0.1", []any{}, reflect.String, false, false}, + "domain": &Entry{"", []any{}, reflect.String, false, false}, + "port": &Entry{8080, []any{}, reflect.Int, false, false}, + "writeTimeout": &Entry{10, []any{}, reflect.Int, false, false}, + "readTimeout": &Entry{10, []any{}, reflect.Int, false, false}, + "readHeaderTimeout": &Entry{10, []any{}, reflect.Int, false, false}, + "idleTimeout": &Entry{20, []any{}, reflect.Int, false, false}, + "websocketCloseTimeout": &Entry{10, []any{}, reflect.Int, false, false}, + "maxUploadSize": &Entry{10.0, []any{}, reflect.Float64, false, false}, "proxy": object{ - "protocol": &Entry{"http", []any{"http", "https"}, reflect.String, false}, - "host": &Entry{nil, []any{}, reflect.String, false}, - "port": &Entry{80, []any{}, reflect.Int, false}, - "base": &Entry{"", []any{}, reflect.String, false}, + "protocol": &Entry{"http", []any{"http", "https"}, reflect.String, false, false}, + "host": &Entry{nil, []any{}, reflect.String, false, false}, + "port": &Entry{80, []any{}, reflect.Int, false, false}, + "base": &Entry{"", []any{}, reflect.String, false, false}, }, }, "database": object{ - "connection": &Entry{"none", []any{}, reflect.String, false}, - "host": &Entry{"127.0.0.1", []any{}, reflect.String, false}, - "port": &Entry{0, []any{}, reflect.Int, false}, - "name": &Entry{"", []any{}, reflect.String, false}, - "username": &Entry{"", []any{}, reflect.String, false}, - "password": &Entry{"", []any{}, reflect.String, false}, - "options": &Entry{"", []any{}, reflect.String, false}, - "maxOpenConnections": &Entry{20, []any{}, reflect.Int, false}, - "maxIdleConnections": &Entry{20, []any{}, reflect.Int, false}, - "maxLifetime": &Entry{300, []any{}, reflect.Int, false}, - "defaultReadQueryTimeout": &Entry{20000, []any{}, reflect.Int, false}, - "defaultWriteQueryTimeout": &Entry{40000, []any{}, reflect.Int, false}, + "connection": &Entry{"none", []any{}, reflect.String, false, false}, + "host": &Entry{"127.0.0.1", []any{}, reflect.String, false, false}, + "port": &Entry{0, []any{}, reflect.Int, false, false}, + "name": &Entry{"", []any{}, reflect.String, false, false}, + "username": &Entry{"", []any{}, reflect.String, false, false}, + "password": &Entry{"", []any{}, reflect.String, false, false}, + "options": &Entry{"", []any{}, reflect.String, false, false}, + "maxOpenConnections": &Entry{20, []any{}, reflect.Int, false, false}, + "maxIdleConnections": &Entry{20, []any{}, reflect.Int, false, false}, + "maxLifetime": &Entry{300, []any{}, reflect.Int, false, false}, + "defaultReadQueryTimeout": &Entry{20000, []any{}, reflect.Int, false, false}, + "defaultWriteQueryTimeout": &Entry{40000, []any{}, reflect.Int, false, false}, "config": object{ - "skipDefaultTransaction": &Entry{false, []any{}, reflect.Bool, false}, - "dryRun": &Entry{false, []any{}, reflect.Bool, false}, - "prepareStmt": &Entry{true, []any{}, reflect.Bool, false}, - "disableNestedTransaction": &Entry{false, []any{}, reflect.Bool, false}, - "allowGlobalUpdate": &Entry{false, []any{}, reflect.Bool, false}, - "disableAutomaticPing": &Entry{false, []any{}, reflect.Bool, false}, - "disableForeignKeyConstraintWhenMigrating": &Entry{false, []any{}, reflect.Bool, false}, + "skipDefaultTransaction": &Entry{false, []any{}, reflect.Bool, false, false}, + "dryRun": &Entry{false, []any{}, reflect.Bool, false, false}, + "prepareStmt": &Entry{true, []any{}, reflect.Bool, false, false}, + "disableNestedTransaction": &Entry{false, []any{}, reflect.Bool, false, false}, + "allowGlobalUpdate": &Entry{false, []any{}, reflect.Bool, false, false}, + "disableAutomaticPing": &Entry{false, []any{}, reflect.Bool, false, false}, + "disableForeignKeyConstraintWhenMigrating": &Entry{false, []any{}, reflect.Bool, false, false}, }, }, } @@ -70,7 +70,7 @@ func loadDefaults(src object, dst object) { } value = slice.Interface() } - dst[k] = &Entry{value, entry.AuthorizedValues, entry.Type, entry.IsSlice} + dst[k] = &Entry{value, entry.AuthorizedValues, entry.Type, entry.IsSlice, entry.Required} } } } diff --git a/config/entry.go b/config/entry.go index 844c18dc..eef48edb 100644 --- a/config/entry.go +++ b/config/entry.go @@ -19,6 +19,7 @@ type Entry struct { AuthorizedValues []any // Leave empty for "any" Type reflect.Kind IsSlice bool + Required bool } func makeEntryFromValue(value any) *Entry { @@ -29,17 +30,22 @@ func makeEntryFromValue(value any) *Entry { kind = t.Elem().Kind() isSlice = true } - return &Entry{value, []any{}, kind, isSlice} + return &Entry{value, []any{}, kind, isSlice, false} } func (e *Entry) validate(key string) error { - if e.Value == nil { // nil values means unset + if e.Value == nil { + if e.Required { + return errors.Errorf("%q is required and must contain a value", key) + } + // When just nil, we don't need to validate. return nil } if err := e.tryEnvVarConversion(key); err != nil { return err } + t := reflect.TypeOf(e.Value) kind := t.Kind() if e.IsSlice && kind == reflect.Slice { @@ -56,6 +62,38 @@ func (e *Entry) validate(key string) error { return errors.Errorf(message, key, e.Type) } + v := reflect.ValueOf(e.Value) + if e.Required { + switch kind { + case reflect.String: + if v.Len() == 0 { + return errors.Errorf("%q is required and cannot be an empty string", key) + } + case reflect.Map: + if len(v.MapKeys()) == 0 { + return errors.Errorf("%q is required and cannot be an empty map", key) + } + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + if v.Int() == 0 { + return errors.Errorf("%q is required and cannot be zero", key) + } + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + if v.Uint() == 0 { + return errors.Errorf("%q is required and cannot be zero", key) + } + case reflect.Float32, reflect.Float64: + if v.Float() == 0 { + return errors.Errorf("%q is required and cannot be zero", key) + } + case reflect.Ptr, reflect.Interface: + if v.IsNil() { + return errors.Errorf("%q is required and cannot be nil", key) + } + default: + // Nothing to reflect on. Should pass. + } + } + if len(e.AuthorizedValues) > 0 { if e.IsSlice { // Accepted values for slices define the values that can be used inside the slice From 7f2dde66ac0eb3d0a35381d6b0645045d10e0266 Mon Sep 17 00:00:00 2001 From: BowlOfSoup Date: Thu, 20 Jun 2024 15:26:53 +0200 Subject: [PATCH 2/6] ISSUE-210 - Make linter happy --- config/config_test.go | 4 +-- config/entry.go | 69 ++++++++++++++++++++++++------------------- 2 files changed, 41 insertions(+), 32 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index ddbb3120..cc8454c3 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -709,10 +709,10 @@ func TestConfig(t *testing.T) { func TestEntryValidationRequired(t *testing.T) { tests := []struct { name string - entry Entry key string - expectsError bool errMsg string + entry Entry + expectsError bool }{ { name: "Required string empty", diff --git a/config/entry.go b/config/entry.go index eef48edb..284cdecd 100644 --- a/config/entry.go +++ b/config/entry.go @@ -62,36 +62,8 @@ func (e *Entry) validate(key string) error { return errors.Errorf(message, key, e.Type) } - v := reflect.ValueOf(e.Value) - if e.Required { - switch kind { - case reflect.String: - if v.Len() == 0 { - return errors.Errorf("%q is required and cannot be an empty string", key) - } - case reflect.Map: - if len(v.MapKeys()) == 0 { - return errors.Errorf("%q is required and cannot be an empty map", key) - } - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - if v.Int() == 0 { - return errors.Errorf("%q is required and cannot be zero", key) - } - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - if v.Uint() == 0 { - return errors.Errorf("%q is required and cannot be zero", key) - } - case reflect.Float32, reflect.Float64: - if v.Float() == 0 { - return errors.Errorf("%q is required and cannot be zero", key) - } - case reflect.Ptr, reflect.Interface: - if v.IsNil() { - return errors.Errorf("%q is required and cannot be nil", key) - } - default: - // Nothing to reflect on. Should pass. - } + if err := e.validateRequired(kind, key); err != nil { + return err } if len(e.AuthorizedValues) > 0 { @@ -230,3 +202,40 @@ func (e *Entry) convertEnvVar(str, key string) (any, error) { return nil, nil } + +func (e *Entry) validateRequired(kind reflect.Kind, key string) error { + v := reflect.ValueOf(e.Value) + + if e.Required { + switch kind { + case reflect.String: + if v.Len() == 0 { + return errors.Errorf("%q is required and cannot be an empty string", key) + } + case reflect.Map: + if len(v.MapKeys()) == 0 { + return errors.Errorf("%q is required and cannot be an empty map", key) + } + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + if v.Int() == 0 { + return errors.Errorf("%q is required and cannot be zero", key) + } + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + if v.Uint() == 0 { + return errors.Errorf("%q is required and cannot be zero", key) + } + case reflect.Float32, reflect.Float64: + if v.Float() == 0 { + return errors.Errorf("%q is required and cannot be zero", key) + } + case reflect.Ptr, reflect.Interface: + if v.IsNil() { + return errors.Errorf("%q is required and cannot be nil", key) + } + default: + return nil + } + } + + return nil +} From ed62736ace48ed39fb54dc7797e47f77cbef30e9 Mon Sep 17 00:00:00 2001 From: BowlOfSoup Date: Fri, 21 Jun 2024 07:49:38 +0200 Subject: [PATCH 3/6] ISSUE-210 - Removed assertion on 'zero' value - assert on nil and not defined --- config/config_test.go | 157 ++++++++++++++++-------------------------- config/default.go | 70 +++++++++---------- config/entry.go | 62 +++-------------- 3 files changed, 106 insertions(+), 183 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index cc8454c3..45f8474e 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "fmt" "reflect" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -63,6 +64,7 @@ func TestLoad(t *testing.T) { AuthorizedValues: []any{}, Type: reflect.String, IsSlice: false, + Required: true, } assert.Equal(t, expected, cfg.config["app"].(object)["name"]) }) @@ -106,6 +108,7 @@ func TestLoad(t *testing.T) { AuthorizedValues: []any{}, Type: reflect.String, IsSlice: false, + Required: true, } assert.Equal(t, expected, cfg.config["app"].(object)["name"]) }) @@ -130,6 +133,7 @@ func TestLoad(t *testing.T) { AuthorizedValues: []any{}, Type: reflect.String, IsSlice: false, + Required: true, } assert.Equal(t, expected, cfg.config["app"].(object)["name"]) }) @@ -670,6 +674,7 @@ func TestConfig(t *testing.T) { AuthorizedValues: []any{}, Type: reflect.String, IsSlice: false, + Required: true, } loader.register("app.name", entry) assert.NotSame(t, &entry, loader.defaults["app"].(object)["name"]) @@ -681,6 +686,7 @@ func TestConfig(t *testing.T) { AuthorizedValues: []any{"a", "b"}, Type: reflect.String, IsSlice: true, + Required: true, }) }) @@ -691,6 +697,7 @@ func TestConfig(t *testing.T) { AuthorizedValues: []any{}, Type: reflect.Int, IsSlice: false, + Required: true, }) }) @@ -701,108 +708,66 @@ func TestConfig(t *testing.T) { AuthorizedValues: []any{}, Type: reflect.String, IsSlice: false, + Required: true, }) }) }) } -func TestEntryValidationRequired(t *testing.T) { - tests := []struct { - name string - key string - errMsg string - entry Entry - expectsError bool - }{ - { - name: "Required string empty", - entry: Entry{Value: "", Type: reflect.String, Required: true}, - key: "emptyString", - expectsError: true, - errMsg: `"emptyString" is required and cannot be an empty string`, - }, - { - name: "Required string non-empty", - entry: Entry{Value: "hello", Type: reflect.String, Required: true}, - key: "nonEmptyString", - expectsError: false, - }, - { - name: "Required int zero", - entry: Entry{Value: 0, Type: reflect.Int, Required: true}, - key: "zeroInt", - expectsError: true, - errMsg: `"zeroInt" is required and cannot be zero`, - }, - { - name: "Required int non-zero", - entry: Entry{Value: 123, Type: reflect.Int, Required: true}, - key: "nonZeroInt", - expectsError: false, - }, - { - name: "Required (string) slice empty", - entry: Entry{Value: []string{}, Type: reflect.String, IsSlice: true, Required: true}, - key: "emptySlice", - expectsError: true, - errMsg: `"emptySlice" is required and cannot be an empty string`, - }, - { - name: "Required slice non-empty", - entry: Entry{Value: []string{"hello"}, Type: reflect.String, IsSlice: true, Required: true}, - key: "nonEmptySlice", - expectsError: false, - }, - { - name: "Required float zero", - entry: Entry{Value: 0.0, Type: reflect.Float64, Required: true}, - key: "zeroFloat", - expectsError: true, - errMsg: `"zeroFloat" is required and cannot be zero`, - }, - { - name: "Required float non-zero", - entry: Entry{Value: 1.23, Type: reflect.Float64, Required: true}, - key: "nonZeroFloat", - expectsError: false, - }, - { - name: "Required map empty", - entry: Entry{Value: map[string]string{}, Type: reflect.Map, Required: true}, - key: "emptyMap", - expectsError: true, - errMsg: `"emptyMap" is required and cannot be an empty map`, - }, - { - name: "Required map non-empty", - entry: Entry{Value: map[string]string{"key": "value"}, Type: reflect.Map, Required: true}, - key: "nonEmptyMap", - expectsError: false, - }, - { - name: "Required nil pointer", - entry: Entry{Value: (*string)(nil), Type: reflect.Ptr, Required: true}, - key: "nilPointer", - expectsError: true, - errMsg: `"nilPointer" is required and cannot be nil`, - }, - { - name: "Required non-nil pointer", - entry: Entry{Value: new(string), Type: reflect.Ptr, Required: true}, - key: "nonNilPointer", - expectsError: false, - }, +func TestRequiredConfig(t *testing.T) { + defaultLoader.mu.Lock() + loader := loader{ + defaults: make(object, len(defaultLoader.defaults)), } + loadDefaults(defaultLoader.defaults, loader.defaults) + defaultLoader.mu.Unlock() - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := tt.entry.validate(tt.key) - if tt.expectsError { - require.Error(t, err) - assert.Equal(t, tt.errMsg, err.Error()) - } else { - require.NoError(t, err) - } - }) + loader.register("testCategory.nullValueNotRequired", Entry{ + Value: nil, + AuthorizedValues: []any{}, + Type: reflect.String, + Required: false, + }) + + loader.register("testCategory.nullValueRequired", Entry{ + Value: nil, + AuthorizedValues: []any{}, + Type: reflect.String, + Required: true, + }) + + loader.register("testCategory.valueCompletelyMissing", Entry{ + Type: reflect.String, + Required: true, + }) + + loader.register("testCategory.valueValidAndDefined", Entry{ + Type: reflect.Int, + Required: true, + }) + + cfgJSON := `{ + "testCategory": { + "nullValueNotRequired": null, + "nullValueRequired": null, + "valueValidAndDefined": 123 + } + }` + + _, err := loader.loadJSON(cfgJSON) + require.Error(t, err) + + expectedErrors := []string{ + "- \"testCategory.valueCompletelyMissing\" is required", + "- \"testCategory.nullValueRequired\" is required", + } + + actualErrors := strings.Split(err.Error(), "\n") + for i, line := range actualErrors { + actualErrors[i] = strings.TrimSpace(line) + } + + for _, expectedMessage := range expectedErrors { + assert.Contains(t, actualErrors, expectedMessage) } } diff --git a/config/default.go b/config/default.go index b9904153..ec40b549 100644 --- a/config/default.go +++ b/config/default.go @@ -4,49 +4,49 @@ import "reflect" var configDefaults = object{ "app": object{ - "name": &Entry{"goyave", []any{}, reflect.String, false, false}, - "environment": &Entry{"localhost", []any{}, reflect.String, false, false}, - "debug": &Entry{true, []any{}, reflect.Bool, false, false}, - "defaultLanguage": &Entry{"en-US", []any{}, reflect.String, false, false}, + "name": &Entry{"goyave", []any{}, reflect.String, false, true}, + "environment": &Entry{"localhost", []any{}, reflect.String, false, true}, + "debug": &Entry{true, []any{}, reflect.Bool, false, true}, + "defaultLanguage": &Entry{"en-US", []any{}, reflect.String, false, true}, }, "server": object{ - "host": &Entry{"127.0.0.1", []any{}, reflect.String, false, false}, - "domain": &Entry{"", []any{}, reflect.String, false, false}, - "port": &Entry{8080, []any{}, reflect.Int, false, false}, - "writeTimeout": &Entry{10, []any{}, reflect.Int, false, false}, - "readTimeout": &Entry{10, []any{}, reflect.Int, false, false}, - "readHeaderTimeout": &Entry{10, []any{}, reflect.Int, false, false}, - "idleTimeout": &Entry{20, []any{}, reflect.Int, false, false}, - "websocketCloseTimeout": &Entry{10, []any{}, reflect.Int, false, false}, - "maxUploadSize": &Entry{10.0, []any{}, reflect.Float64, false, false}, + "host": &Entry{"127.0.0.1", []any{}, reflect.String, false, true}, + "domain": &Entry{"", []any{}, reflect.String, false, true}, + "port": &Entry{8080, []any{}, reflect.Int, false, true}, + "writeTimeout": &Entry{10, []any{}, reflect.Int, false, true}, + "readTimeout": &Entry{10, []any{}, reflect.Int, false, true}, + "readHeaderTimeout": &Entry{10, []any{}, reflect.Int, false, true}, + "idleTimeout": &Entry{20, []any{}, reflect.Int, false, true}, + "websocketCloseTimeout": &Entry{10, []any{}, reflect.Int, false, true}, + "maxUploadSize": &Entry{10.0, []any{}, reflect.Float64, false, true}, "proxy": object{ - "protocol": &Entry{"http", []any{"http", "https"}, reflect.String, false, false}, + "protocol": &Entry{"http", []any{"http", "https"}, reflect.String, false, true}, "host": &Entry{nil, []any{}, reflect.String, false, false}, - "port": &Entry{80, []any{}, reflect.Int, false, false}, - "base": &Entry{"", []any{}, reflect.String, false, false}, + "port": &Entry{80, []any{}, reflect.Int, false, true}, + "base": &Entry{"", []any{}, reflect.String, false, true}, }, }, "database": object{ - "connection": &Entry{"none", []any{}, reflect.String, false, false}, - "host": &Entry{"127.0.0.1", []any{}, reflect.String, false, false}, - "port": &Entry{0, []any{}, reflect.Int, false, false}, - "name": &Entry{"", []any{}, reflect.String, false, false}, - "username": &Entry{"", []any{}, reflect.String, false, false}, - "password": &Entry{"", []any{}, reflect.String, false, false}, - "options": &Entry{"", []any{}, reflect.String, false, false}, - "maxOpenConnections": &Entry{20, []any{}, reflect.Int, false, false}, - "maxIdleConnections": &Entry{20, []any{}, reflect.Int, false, false}, - "maxLifetime": &Entry{300, []any{}, reflect.Int, false, false}, - "defaultReadQueryTimeout": &Entry{20000, []any{}, reflect.Int, false, false}, - "defaultWriteQueryTimeout": &Entry{40000, []any{}, reflect.Int, false, false}, + "connection": &Entry{"none", []any{}, reflect.String, false, true}, + "host": &Entry{"127.0.0.1", []any{}, reflect.String, false, true}, + "port": &Entry{0, []any{}, reflect.Int, false, true}, + "name": &Entry{"", []any{}, reflect.String, false, true}, + "username": &Entry{"", []any{}, reflect.String, false, true}, + "password": &Entry{"", []any{}, reflect.String, false, true}, + "options": &Entry{"", []any{}, reflect.String, false, true}, + "maxOpenConnections": &Entry{20, []any{}, reflect.Int, false, true}, + "maxIdleConnections": &Entry{20, []any{}, reflect.Int, false, true}, + "maxLifetime": &Entry{300, []any{}, reflect.Int, false, true}, + "defaultReadQueryTimeout": &Entry{20000, []any{}, reflect.Int, false, true}, + "defaultWriteQueryTimeout": &Entry{40000, []any{}, reflect.Int, false, true}, "config": object{ - "skipDefaultTransaction": &Entry{false, []any{}, reflect.Bool, false, false}, - "dryRun": &Entry{false, []any{}, reflect.Bool, false, false}, - "prepareStmt": &Entry{true, []any{}, reflect.Bool, false, false}, - "disableNestedTransaction": &Entry{false, []any{}, reflect.Bool, false, false}, - "allowGlobalUpdate": &Entry{false, []any{}, reflect.Bool, false, false}, - "disableAutomaticPing": &Entry{false, []any{}, reflect.Bool, false, false}, - "disableForeignKeyConstraintWhenMigrating": &Entry{false, []any{}, reflect.Bool, false, false}, + "skipDefaultTransaction": &Entry{false, []any{}, reflect.Bool, false, true}, + "dryRun": &Entry{false, []any{}, reflect.Bool, false, true}, + "prepareStmt": &Entry{true, []any{}, reflect.Bool, false, true}, + "disableNestedTransaction": &Entry{false, []any{}, reflect.Bool, false, true}, + "allowGlobalUpdate": &Entry{false, []any{}, reflect.Bool, false, true}, + "disableAutomaticPing": &Entry{false, []any{}, reflect.Bool, false, true}, + "disableForeignKeyConstraintWhenMigrating": &Entry{false, []any{}, reflect.Bool, false, true}, }, }, } diff --git a/config/entry.go b/config/entry.go index 284cdecd..86535318 100644 --- a/config/entry.go +++ b/config/entry.go @@ -34,19 +34,19 @@ func makeEntryFromValue(value any) *Entry { } func (e *Entry) validate(key string) error { - if e.Value == nil { - if e.Required { - return errors.Errorf("%q is required and must contain a value", key) - } - // When just nil, we don't need to validate. - return nil - } - if err := e.tryEnvVarConversion(key); err != nil { return err } + v := reflect.ValueOf(e.Value) + if e.Required && (!v.IsValid() || e.Value == nil) { + return errors.Errorf("%q is required", key) + } + t := reflect.TypeOf(e.Value) + if t == nil { + return nil // Can't determine + } kind := t.Kind() if e.IsSlice && kind == reflect.Slice { kind = t.Elem().Kind() @@ -62,18 +62,13 @@ func (e *Entry) validate(key string) error { return errors.Errorf(message, key, e.Type) } - if err := e.validateRequired(kind, key); err != nil { - return err - } - if len(e.AuthorizedValues) > 0 { if e.IsSlice { // Accepted values for slices define the values that can be used inside the slice // It doesn't represent the value of the slice itself (content and order) - list := reflect.ValueOf(e.Value) - length := list.Len() + length := v.Len() for i := 0; i < length; i++ { - if !lo.Contains(e.AuthorizedValues, list.Index(i).Interface()) { + if !lo.Contains(e.AuthorizedValues, v.Index(i).Interface()) { return errors.Errorf("%q elements must have one of the following values: %v", key, e.AuthorizedValues) } } @@ -202,40 +197,3 @@ func (e *Entry) convertEnvVar(str, key string) (any, error) { return nil, nil } - -func (e *Entry) validateRequired(kind reflect.Kind, key string) error { - v := reflect.ValueOf(e.Value) - - if e.Required { - switch kind { - case reflect.String: - if v.Len() == 0 { - return errors.Errorf("%q is required and cannot be an empty string", key) - } - case reflect.Map: - if len(v.MapKeys()) == 0 { - return errors.Errorf("%q is required and cannot be an empty map", key) - } - case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: - if v.Int() == 0 { - return errors.Errorf("%q is required and cannot be zero", key) - } - case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: - if v.Uint() == 0 { - return errors.Errorf("%q is required and cannot be zero", key) - } - case reflect.Float32, reflect.Float64: - if v.Float() == 0 { - return errors.Errorf("%q is required and cannot be zero", key) - } - case reflect.Ptr, reflect.Interface: - if v.IsNil() { - return errors.Errorf("%q is required and cannot be nil", key) - } - default: - return nil - } - } - - return nil -} From ad043b209086e31df6743047845699abd79ae52e Mon Sep 17 00:00:00 2001 From: BowlOfSoup Date: Fri, 21 Jun 2024 09:38:42 +0200 Subject: [PATCH 4/6] ISSUE-210 - Removed default loader from 'Required' test. --- config/config_test.go | 5 +---- config/entry.go | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/config/config_test.go b/config/config_test.go index 45f8474e..4aa23d5c 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -715,12 +715,9 @@ func TestConfig(t *testing.T) { } func TestRequiredConfig(t *testing.T) { - defaultLoader.mu.Lock() loader := loader{ - defaults: make(object, len(defaultLoader.defaults)), + defaults: make(object), } - loadDefaults(defaultLoader.defaults, loader.defaults) - defaultLoader.mu.Unlock() loader.register("testCategory.nullValueNotRequired", Entry{ Value: nil, diff --git a/config/entry.go b/config/entry.go index 86535318..9c2e46a5 100644 --- a/config/entry.go +++ b/config/entry.go @@ -45,7 +45,7 @@ func (e *Entry) validate(key string) error { t := reflect.TypeOf(e.Value) if t == nil { - return nil // Can't determine + return nil // Can't determine type, is 'zero' value. } kind := t.Kind() if e.IsSlice && kind == reflect.Slice { From c3ef145b13affce26209a7dde14776ab8ba05cd6 Mon Sep 17 00:00:00 2001 From: BowlOfSoup Date: Fri, 21 Jun 2024 09:43:56 +0200 Subject: [PATCH 5/6] ISSUE-210 - Added explicit IsNil() check for pointers --- config/entry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/entry.go b/config/entry.go index 9c2e46a5..c4ad1c29 100644 --- a/config/entry.go +++ b/config/entry.go @@ -39,7 +39,7 @@ func (e *Entry) validate(key string) error { } v := reflect.ValueOf(e.Value) - if e.Required && (!v.IsValid() || e.Value == nil) { + if e.Required && (!v.IsValid() || e.Value == nil || (v.Kind() == reflect.Pointer && v.IsNil())) { return errors.Errorf("%q is required", key) } From 05533a486ea85ee291cd77ddadd8c9a64ff96527 Mon Sep 17 00:00:00 2001 From: BowlOfSoup Date: Fri, 21 Jun 2024 10:01:28 +0200 Subject: [PATCH 6/6] ISSUE-210 - Add test scenarios for pointers in config --- config/config_test.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/config/config_test.go b/config/config_test.go index 4aa23d5c..acacf5d0 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -743,11 +743,28 @@ func TestRequiredConfig(t *testing.T) { Required: true, }) + var nilPointer *string + loader.register("testCategory.nilPointerRequired", Entry{ + Value: nilPointer, + Type: reflect.Ptr, + Required: true, + }) + + validPointer := new(string) + *validPointer = "valid" + loader.register("testCategory.validPointer", Entry{ + Value: validPointer, + Type: reflect.Ptr, + Required: true, + }) + cfgJSON := `{ "testCategory": { "nullValueNotRequired": null, "nullValueRequired": null, - "valueValidAndDefined": 123 + "valueValidAndDefined": 123, + "valueValidAndDefined": 123, + "validPointer": "valid" } }` @@ -757,6 +774,7 @@ func TestRequiredConfig(t *testing.T) { expectedErrors := []string{ "- \"testCategory.valueCompletelyMissing\" is required", "- \"testCategory.nullValueRequired\" is required", + "- \"testCategory.nilPointerRequired\" is required", } actualErrors := strings.Split(err.Error(), "\n")