Merge pull request #39 from DevLabFoundry/fix/data-race

fix: data race in token parsing

Author: ElvenSpellmaker

internal/store/store.go

@@ -2,7 +2,6 @@ package store
 
 import (
 	"errors"
-	"fmt"
 
 	"github.com/DevLabFoundry/configmanager/v2/internal/config"
 )
@@ -18,22 +17,9 @@ var (
 
 // Strategy iface that all store implementations
 // must conform to, in order to be be used by the retrieval implementation
+//
+// Defined on the package for easier re-use across the program
 type Strategy interface {
 	Token() (s string, e error)
 	SetToken(s *config.ParsedTokenConfig)
 }
-
-type DefaultStrategy struct {
-}
-
-func NewDefatultStrategy() *DefaultStrategy {
-	return &DefaultStrategy{}
-}
-
-// SetToken on default strategy
-func (implmt *DefaultStrategy) SetToken(token *config.ParsedTokenConfig) {}
-
-// Token
-func (implmt *DefaultStrategy) Token() (string, error) {
-	return "", fmt.Errorf("default strategy does not implement token retrieval")
-}

internal/store/store_test.go

@@ -1,24 +1 @@
 package store_test
-
-import (
-	"testing"
-
-	"github.com/DevLabFoundry/configmanager/v2/internal/store"
-)
-
-func Test_StoreDefault(t *testing.T) {
-
-	t.Run("Default Shoudl not errror", func(t *testing.T) {
-		rs := store.NewDefatultStrategy()
-		if rs == nil {
-			t.Fatal("unable to init default strategy")
-		}
-	})
-	t.Run("Token method should error", func(t *testing.T) {
-		rs := store.NewDefatultStrategy()
-		if _, err := rs.Token(); err == nil {
-			t.Fatal("Token should return not implemented error")
-		}
-	})
-
-}

internal/strategy/strategy.go

@@ -1,9 +1,13 @@
+// Package strategy is a strategy pattern wrapper around the store implementations
+//
+// NOTE: this may be refactored out into the store package directly
 package strategy
 
 import (
 	"context"
 	"errors"
 	"fmt"
+	"sync"
 
 	"github.com/DevLabFoundry/configmanager/v2/internal/config"
 	"github.com/DevLabFoundry/configmanager/v2/internal/log"
@@ -44,32 +48,43 @@ func defaultStrategyFuncMap(logger log.ILogger) map[config.ImplementationPrefix]
 	}
 }
 
+type strategyFnMap struct {
+	mu      sync.Mutex
+	funcMap StrategyFuncMap
+}
 type RetrieveStrategy struct {
 	implementation  store.Strategy
 	config          config.GenVarsConfig
-	strategyFuncMap StrategyFuncMap
-	token           string
+	strategyFuncMap strategyFnMap
 }
+type Opts func(*RetrieveStrategy)
 
 // New
-func New(s store.Strategy, config config.GenVarsConfig, logger log.ILogger) *RetrieveStrategy {
+func New(config config.GenVarsConfig, logger log.ILogger, opts ...Opts) *RetrieveStrategy {
 	rs := &RetrieveStrategy{
-		implementation:  s,
 		config:          config,
-		strategyFuncMap: defaultStrategyFuncMap(logger),
+		strategyFuncMap: strategyFnMap{mu: sync.Mutex{}, funcMap: defaultStrategyFuncMap(logger)},
+	}
+	// overwrite or add any options/defaults set above
+	for _, o := range opts {
+		o(rs)
 	}
+
 	return rs
 }
 
 // WithStrategyFuncMap Adds custom implementations for prefix
 //
 // Mainly used for testing
 // NOTE: this may lead to eventual optional configurations by users
-func (rs *RetrieveStrategy) WithStrategyFuncMap(funcMap StrategyFuncMap) *RetrieveStrategy {
-	for prefix, implementation := range funcMap {
-		rs.strategyFuncMap[config.ImplementationPrefix(prefix)] = implementation
+func WithStrategyFuncMap(funcMap StrategyFuncMap) Opts {
+	return func(rs *RetrieveStrategy) {
+		for prefix, implementation := range funcMap {
+			rs.strategyFuncMap.mu.Lock()
+			defer rs.strategyFuncMap.mu.Unlock()
+			rs.strategyFuncMap.funcMap[config.ImplementationPrefix(prefix)] = implementation
+		}
 	}
-	return rs
 }
 
 func (rs *RetrieveStrategy) setImplementation(strategy store.Strategy) {
@@ -120,7 +135,7 @@ func (rs *RetrieveStrategy) SelectImplementation(ctx context.Context, token *con
 		return nil, fmt.Errorf("unable to get prefix, %w", ErrTokenInvalid)
 	}
 
-	if store, found := rs.strategyFuncMap[token.Prefix()]; found {
+	if store, found := rs.strategyFuncMap.funcMap[token.Prefix()]; found {
 		return store(ctx, token)
 	}
 

internal/strategy/strategy_test.go

@@ -68,7 +68,7 @@ func Test_Strategy_Retrieve_succeeds(t *testing.T) {
 	}
 	for name, tt := range ttests {
 		t.Run(name, func(t *testing.T) {
-			rs := strategy.New(store.NewDefatultStrategy(), *tt.config, log.New(io.Discard))
+			rs := strategy.New(*tt.config, log.New(io.Discard))
 			token, _ := config.NewParsedTokenConfig(tt.token, *tt.config)
 			got := rs.RetrieveByToken(context.TODO(), tt.impl(t), token)
 			if got.Err != nil {
@@ -103,8 +103,7 @@ func Test_CustomStrategyFuncMap_add_own(t *testing.T) {
 				return m, nil
 			}
 
-			s := strategy.New(store.NewDefatultStrategy(), *genVarsConf, log.New(io.Discard))
-			s.WithStrategyFuncMap(strategy.StrategyFuncMap{config.AzTableStorePrefix: custFunc})
+			s := strategy.New(*genVarsConf, log.New(io.Discard), strategy.WithStrategyFuncMap(strategy.StrategyFuncMap{config.AzTableStorePrefix: custFunc}))
 
 			store, _ := s.SelectImplementation(context.TODO(), token)
 			_ = s.RetrieveByToken(context.TODO(), store, token)
@@ -273,7 +272,7 @@ func Test_SelectImpl_With(t *testing.T) {
 			tearDown := tt.setUpTearDown()
 			defer tearDown()
 			want := tt.expect()
-			rs := strategy.New(store.NewDefatultStrategy(), *tt.config, log.New(io.Discard))
+			rs := strategy.New(*tt.config, log.New(io.Discard))
 			token, _ := config.NewParsedTokenConfig(tt.token, *tt.config)
 			got, err := rs.SelectImplementation(context.TODO(), token)
 

pkg/generator/generator.go

@@ -29,7 +29,7 @@ type retrieveIface interface {
 // relevant strategy network calls to the config store implementations
 type GenVars struct {
 	Logger   log.ILogger
-	strategy retrieveIface
+	strategy strategy.StrategyFuncMap
 	ctx      context.Context
 	config   config.GenVarsConfig
 	// rawMap is the internal object that holds the values
@@ -61,22 +61,21 @@ func newGenVars(ctx context.Context, opts ...Opts) *GenVars {
 		// return using default config
 		config: *conf,
 	}
+	g.strategy = nil
 
+	// now apply additional opts
 	for _, o := range opts {
 		o(g)
 	}
 
-	// using a default Strategy
-	g.strategy = strategy.New(store.NewDefatultStrategy(), *conf, g.Logger)
-	// now apply
 	return g
 }
 
 // WithStrategyMap
 //
-// Adds addtional funcs for storageRetrieval
+// Adds addtional funcs for storageRetrieval used for testing only
 func (c *GenVars) WithStrategyMap(sm strategy.StrategyFuncMap) *GenVars {
-	c.strategy.WithStrategyFuncMap(sm)
+	c.strategy = sm
 	return c
 }
 
@@ -195,12 +194,13 @@ func (c *GenVars) generate(rawMap *rawTokenMap) error {
 		token := parsedToken // safe closure capture
 		// take value from config allocation on a per iteration basis
 		go func() {
-			storeStrategy, err := c.strategy.SelectImplementation(c.ctx, token)
+			s := strategy.New(c.config, c.Logger, strategy.WithStrategyFuncMap(c.strategy))
+			storeStrategy, err := s.SelectImplementation(c.ctx, token)
 			if err != nil {
 				outCh <- &strategy.TokenResponse{Err: err}
 				return
 			}
-			outCh <- c.strategy.RetrieveByToken(c.ctx, storeStrategy, token)
+			outCh <- s.RetrieveByToken(c.ctx, storeStrategy, token)
 		}()
 	}