From 6699d060f097061ab177b1194ba059cc4f6e1daa Mon Sep 17 00:00:00 2001 From: Kuisong Tong Date: Tue, 30 Jan 2024 20:24:23 -0800 Subject: [PATCH] preventive coding (#77) more preventive coding to enhance integrity . --- .github/workflows/test.yml | 2 +- config.go | 98 ++++++++++++++++++++++++++++---------- config_test.go | 10 ---- default.go | 14 +++++- doc.go | 2 +- provider/env/env.go | 7 +-- provider/file/file.go | 12 ++++- provider/file/file_test.go | 2 +- provider/flag/flag.go | 7 +-- provider/fs/fs.go | 19 ++++++-- provider/fs/fs_test.go | 5 +- provider/pflag/pflag.go | 7 +-- 12 files changed, 126 insertions(+), 59 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e266e400..2d52ebdf 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,7 +29,7 @@ jobs: run: go test -v -shuffle=on -count=10 -race ./... working-directory: provider/file - name: Test (file) - run: go test -v -shuffle=on ./... + run: go test -v ./... working-directory: provider/file - name: Race Test (pflag) run: go test -v -shuffle=on -count=10 -race ./... diff --git a/config.go b/config.go index 2f909647..9bb3c533 100644 --- a/config.go +++ b/config.go @@ -10,6 +10,7 @@ import ( "log/slog" "strings" "sync" + "time" "github.com/go-viper/mapstructure/v2" @@ -33,26 +34,32 @@ type Config struct { } type provider struct { - values map[string]any - watcher Watcher + loader Loader + values map[string]any } // New creates a new Config with the given Option(s). func New(opts ...Option) *Config { option := &options{ - delimiter: ".", - tagName: "konf", - decodeHook: mapstructure.ComposeDecodeHookFunc( - mapstructure.StringToTimeDurationHookFunc(), - mapstructure.StringToSliceHookFunc(","), - mapstructure.TextUnmarshallerHookFunc(), - ), values: make(map[string]any), onChanges: make(map[string][]func(*Config)), } for _, opt := range opts { opt(option) } + if option.delimiter == "" { + option.delimiter = "." + } + if option.tagName == "" { + option.tagName = "konf" + } + if option.decodeHook == nil { + option.decodeHook = mapstructure.ComposeDecodeHookFunc( + mapstructure.StringToTimeDurationHookFunc(), + mapstructure.StringToSliceHookFunc(","), + mapstructure.TextUnmarshallerHookFunc(), + ) + } return (*Config)(option) } @@ -61,10 +68,11 @@ func New(opts ...Option) *Config { // Each loader takes precedence over the loaders before it. // // This method can be called multiple times but it is not concurrency-safe. +// It panics if any loader is nil. func (c *Config) Load(loaders ...Loader) error { - for _, loader := range loaders { + for i, loader := range loaders { if loader == nil { - continue + panic(fmt.Sprintf("cannot load config from nil loader at loaders[%d]", i)) } values, err := loader.Load() @@ -73,14 +81,12 @@ func (c *Config) Load(loaders ...Loader) error { } maps.Merge(c.values, values) - // Merged to empty map to convert to lower case. provider := &provider{ + loader: loader, values: make(map[string]any), } + // Merged to empty map to convert to lower case. maps.Merge(provider.values, values) - if w, ok := loader.(Watcher); ok { - provider.watcher = w - } c.providers = append(c.providers, provider) slog.Info( @@ -97,12 +103,19 @@ func (c *Config) Load(loaders ...Loader) error { // WARNING: All loaders passed in Load after calling Watch do not get watched. // // It only can be called once. Call after first has no effects. +// It panics if ctx is nil. func (c *Config) Watch(ctx context.Context) error { //nolint:cyclop,funlen,gocognit - initialized := true + if ctx == nil { + panic("cannot watch change with nil context") + } + + watched := true c.watchOnce.Do(func() { - initialized = false + watched = false }) - if initialized { + if watched { + slog.Warn("Config has been watched, call Watch again has no effects.") + return nil } @@ -124,9 +137,32 @@ func (c *Config) Watch(ctx context.Context) error { //nolint:cyclop,funlen,gocog maps.Merge(values, w.values) } c.values = values + slog.Info("Configuration has been updated with change.") - for _, onChange := range onChanges { - onChange(c) + if len(onChanges) > 0 { + func() { + ctx, cancel = context.WithTimeout(context.Background(), time.Minute) + defer cancel() + + done := make(chan struct{}) + go func() { + defer close(done) + + for _, onChange := range onChanges { + onChange(c) + } + }() + + select { + case <-done: + slog.InfoContext(ctx, "Configuration has been applied to onChanges.") + case <-ctx.Done(): + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + slog.WarnContext(ctx, "Configuration has not been fully applied to onChanges due to timeout."+ + " Please check if the onChanges is blocking or takes too long to complete.") + } + } + }() } case <-ctx.Done(): @@ -137,9 +173,9 @@ func (c *Config) Watch(ctx context.Context) error { //nolint:cyclop,funlen,gocog errChan := make(chan error, len(c.providers)) for _, provider := range c.providers { - if provider.watcher != nil { - provider := provider + provider := provider + if watcher, ok := provider.loader.(Watcher); ok { waitGroup.Add(1) go func() { defer waitGroup.Done() @@ -170,10 +206,12 @@ func (c *Config) Watch(ctx context.Context) error { //nolint:cyclop,funlen,gocog slog.Info( "Configuration has been changed.", - "provider", provider.watcher, + "loader", watcher, ) } - if err := provider.watcher.Watch(ctx, onChange); err != nil { + + slog.Info("Watching configuration change.", "loader", watcher) + if err := watcher.Watch(ctx, onChange); err != nil { errChan <- fmt.Errorf("watch configuration change: %w", err) cancel() } @@ -218,8 +256,16 @@ func sub(values map[string]any, path string, delimiter string) any { // It requires Config.Watch has been called first. // The paths are case-insensitive. // +// The onChange function must be non-blocking and usually completes instantly. +// If it requires a long time to complete, it should be executed in a separate goroutine. +// // This method is concurrency-safe. -func (c *Config) OnChange(onchange func(*Config), paths ...string) { +// It panics if onChange is nil. +func (c *Config) OnChange(onChange func(*Config), paths ...string) { + if onChange == nil { + panic("cannot register nil onChange") + } + c.onChangesMutex.Lock() defer c.onChangesMutex.Unlock() @@ -229,7 +275,7 @@ func (c *Config) OnChange(onchange func(*Config), paths ...string) { for _, path := range paths { path = strings.ToLower(path) - c.onChanges[path] = append(c.onChanges[path], onchange) + c.onChanges[path] = append(c.onChanges[path], onChange) } } diff --git a/config_test.go b/config_test.go index 3354f84f..371cc549 100644 --- a/config_test.go +++ b/config_test.go @@ -31,16 +31,6 @@ func TestConfig_Unmarshal(t *testing.T) { assert.Equal(t, "", value) }, }, - { - description: "nil loader", - opts: []konf.Option{}, - loaders: []konf.Loader{nil}, - assert: func(config *konf.Config) { - var value string - assert.NoError(t, config.Unmarshal("config", &value)) - assert.Equal(t, "", value) - }, - }, { description: "for primary type", loaders: []konf.Loader{mapLoader{"config": "string"}}, diff --git a/default.go b/default.go index 7a77462c..ba1955fa 100644 --- a/default.go +++ b/default.go @@ -39,7 +39,11 @@ func Unmarshal(path string, target any) error { // when the value of any given path in the default Config changes. // The paths are case-insensitive. // +// The onChange function must be non-blocking and usually completes instantly. +// If it requires a long time to complete, it should be executed in a separate goroutine. +// // This method is concurrency-safe. +// It panics if onChange is nil. func OnChange(onChange func(), paths ...string) { defaultConfig.Load().OnChange(func(*Config) { onChange() }, paths...) } @@ -47,8 +51,14 @@ func OnChange(onChange func(), paths ...string) { // SetDefault sets the given Config as the default Config. // After this call, the konf package's top functions (e.g. konf.Get) // will interact with the given Config. -func SetDefault(c *Config) { - defaultConfig.Store(c) +// +// It panics if config is nil. +func SetDefault(config *Config) { + if config == nil { + panic("cannot set default with nil config") + } + + defaultConfig.Store(config) } var defaultConfig atomic.Pointer[Config] //nolint:gochecknoglobals diff --git a/doc.go b/doc.go index 3a684db7..8f1f41a5 100644 --- a/doc.go +++ b/doc.go @@ -18,6 +18,7 @@ Configuration is hierarchical, and the path is a sequence of keys that separated The default delimiter is `.`, which makes configuration path like `parent.child.key`. # Load Configuration + After creating a [Config], you can load configuration from multiple [Loader](s) using [Config.Load]. Each loader takes precedence over the loaders before it. As long as the configuration has been loaded, it can be used in following code to get or unmarshal configuration, even for loading configuration @@ -28,7 +29,6 @@ and then use the file path to load configuration from file system. [Config.Watch] watches and updates configuration when it changes, which leads [Config.Unmarshal] always returns latest configuration. - You may use [Config.OnChange] to register a callback if the value of any path have been changed. It could push the change into application objects instead pulling the configuration periodically. */ diff --git a/provider/env/env.go b/provider/env/env.go index b305a507..a3ac9e93 100644 --- a/provider/env/env.go +++ b/provider/env/env.go @@ -29,12 +29,13 @@ type Env struct { // New creates an Env with the given Option(s). func New(opts ...Option) Env { - option := &options{ - delimiter: "_", - } + option := &options{} for _, opt := range opts { opt(option) } + if option.delimiter == "" { + option.delimiter = "_" + } return Env(*option) } diff --git a/provider/file/file.go b/provider/file/file.go index ca5bd779..046aef22 100644 --- a/provider/file/file.go +++ b/provider/file/file.go @@ -30,14 +30,22 @@ type File struct { } // New creates a File with the given path and Option(s). +// +// It panics if the path is empty. func New(path string, opts ...Option) File { + if path == "" { + panic("cannot create File with empty path") + } + option := &options{ - path: path, - unmarshal: json.Unmarshal, + path: path, } for _, opt := range opts { opt(option) } + if option.unmarshal == nil { + option.unmarshal = json.Unmarshal + } return File(*option) } diff --git a/provider/file/file_test.go b/provider/file/file_test.go index 2e587efa..029867bb 100644 --- a/provider/file/file_test.go +++ b/provider/file/file_test.go @@ -61,7 +61,7 @@ func TestFile_Load(t *testing.T) { t.Parallel() values, err := file.New(testcase.path, testcase.opts...).Load() - if err != nil { + if testcase.err != "" { assert.True(t, strings.HasPrefix(err.Error(), testcase.err)) } else { assert.NoError(t, err) diff --git a/provider/flag/flag.go b/provider/flag/flag.go index 522388f6..6a732fbf 100644 --- a/provider/flag/flag.go +++ b/provider/flag/flag.go @@ -32,12 +32,13 @@ type Flag struct { // New creates a Flag with the given Option(s). func New(opts ...Option) Flag { - option := &options{ - delimiter: ".", - } + option := &options{} for _, opt := range opts { opt(option) } + if option.delimiter == "" { + option.delimiter = "." + } if option.set == nil { flag.Parse() option.set = flag.CommandLine diff --git a/provider/fs/fs.go b/provider/fs/fs.go index ec9399cc..5a8b5379 100644 --- a/provider/fs/fs.go +++ b/provider/fs/fs.go @@ -32,15 +32,26 @@ type FS struct { } // New creates a FS with the given fs.FS, path and Option(s). -func New(fs fs.FS, path string, opts ...Option) FS { +// +// It panics if the fs is nil or the path is empty. +func New(fs fs.FS, path string, opts ...Option) FS { //nolint:varnamelen + if fs == nil { + panic("cannot create FS with nil fs") + } + if path == "" { + panic("cannot create FS with empty path") + } + option := &options{ - fs: fs, - path: path, - unmarshal: json.Unmarshal, + fs: fs, + path: path, } for _, opt := range opts { opt(option) } + if option.unmarshal == nil { + option.unmarshal = json.Unmarshal + } return FS(*option) } diff --git a/provider/fs/fs_test.go b/provider/fs/fs_test.go index eea40aae..441a2753 100644 --- a/provider/fs/fs_test.go +++ b/provider/fs/fs_test.go @@ -8,7 +8,6 @@ package fs_test import ( "errors" "io/fs" - "strings" "testing" "testing/fstest" @@ -80,8 +79,8 @@ func TestFile_Load(t *testing.T) { t.Parallel() values, err := kfs.New(testcase.fs, testcase.path, testcase.opts...).Load() - if err != nil { - assert.True(t, strings.HasPrefix(err.Error(), testcase.err)) + if testcase.err != "" { + assert.EqualError(t, err, testcase.err) } else { assert.NoError(t, err) assert.Equal(t, testcase.expected, values) diff --git a/provider/pflag/pflag.go b/provider/pflag/pflag.go index ebc24cdc..b467c2ea 100644 --- a/provider/pflag/pflag.go +++ b/provider/pflag/pflag.go @@ -34,12 +34,13 @@ type PFlag struct { // New creates a PFlag with the given Option(s). func New(opts ...Option) PFlag { - option := &options{ - delimiter: ".", - } + option := &options{} for _, opt := range opts { opt(option) } + if option.delimiter == "" { + option.delimiter = "." + } if option.set == nil { pflag.CommandLine.AddGoFlagSet(flag.CommandLine) pflag.Parse()