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.

Sign up for GitHub

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

ISSUE-210 - Add Required property to indicate a config registration c… #211

Merged
merged 6 commits into from
Jun 21, 2024
Merged

ISSUE-210 - Add Required property to indicate a config registration c… #211

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ca08f13
ISSUE-210 - Add Required property to indicate a config registration c…
BowlOfSoup Jun 20, 2024
7f2dde6
ISSUE-210 - Make linter happy
BowlOfSoup Jun 20, 2024
ed62736
ISSUE-210 - Removed assertion on 'zero' value - assert on nil and not…
BowlOfSoup Jun 21, 2024
ad043b2
ISSUE-210 - Removed default loader from 'Required' test.
BowlOfSoup Jun 21, 2024
c3ef145
ISSUE-210 - Added explicit IsNil() check for pointers
BowlOfSoup Jun 21, 2024
05533a4
ISSUE-210 - Add test scenarios for pointers in config
BowlOfSoup Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package config
import (
"fmt"
"reflect"
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -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"])
})
Expand Down Expand Up @@ -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"])
})
Expand All @@ -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"])
})
Expand Down Expand Up @@ -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"])
Expand All @@ -681,6 +686,7 @@ func TestConfig(t *testing.T) {
AuthorizedValues: []any{"a", "b"},
Type: reflect.String,
IsSlice: true,
Required: true,
})
})

Expand All @@ -691,6 +697,7 @@ func TestConfig(t *testing.T) {
AuthorizedValues: []any{},
Type: reflect.Int,
IsSlice: false,
Required: true,
})
})

Expand All @@ -701,7 +708,66 @@ func TestConfig(t *testing.T) {
AuthorizedValues: []any{},
Type: reflect.String,
IsSlice: false,
Required: true,
})
})
})
}

func TestRequiredConfig(t *testing.T) {
defaultLoader.mu.Lock()
loader := loader{
defaults: make(object, len(defaultLoader.defaults)),
}
loadDefaults(defaultLoader.defaults, loader.defaults)
defaultLoader.mu.Unlock()
BowlOfSoup marked this conversation as resolved.
Show resolved Hide resolved

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)
}
}
74 changes: 37 additions & 37 deletions config/default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, 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},
"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, 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},
"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, true},
"host": &Entry{nil, []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},
"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, 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},
"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, 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},
},
},
}
Expand All @@ -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}
}
}
}
21 changes: 13 additions & 8 deletions config/entry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -29,18 +30,23 @@ 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
return nil
}

if err := e.tryEnvVarConversion(key); err != nil {
return err
}

v := reflect.ValueOf(e.Value)
if e.Required && (!v.IsValid() || e.Value == nil) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if e.Required && (!v.IsValid() || e.Value == nil) {
if e.Required && (!v.IsValid() || v.IsNil()) {

Copy link
Contributor Author

@BowlOfSoup BowlOfSoup Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsNil reports whether its argument v is nil. The argument must be a chan, func, interface, map, pointer, or slice value; if it is not, IsNil panics.

I've tested this when you first suggested it, but it will not work if the value is not one of the above. This happens e.g. when an inputted value is actually nil.

So !v.IsValid() || e.Value == nil would cover the functionality we want.

If it's not a value (IsValid) it will return true. If it is, it will check for explicit nil.

Copy link
Member

@System-Glitch System-Glitch Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Here is what I suggest to cover the scenario I showed in the playground link earlier.

Suggested change
if e.Required && (!v.IsValid() || e.Value == nil) {
if e.Required && (!v.IsValid() || e.Value == nil || (v.Kind() == reflect.Pointer && v.IsNil())) {

Updated playground link

return errors.Errorf("%q is required", key)
}

t := reflect.TypeOf(e.Value)
if t == nil {
return nil // Can't determine
}
System-Glitch marked this conversation as resolved.
Show resolved Hide resolved
kind := t.Kind()
if e.IsSlice && kind == reflect.Slice {
kind = t.Elem().Kind()
Expand All @@ -60,10 +66,9 @@ func (e *Entry) validate(key string) error {
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)
}
}
Expand Down