remove unnecessary previous value merging

Author: rmfitzpatrick

internal/configsource/source.go

@@ -92,11 +92,49 @@ type Factory interface {
 // Factories maps the type of a ConfigSource to the respective factory object.
 type Factories map[component.Type]Factory
 
-// BuildConfigSourcesAndResolve inspects the given confmap.Conf and resolves all config sources referenced
-// in the configuration, returning a confmap.Conf in which all env vars and config sources on
+// BuildConfigSourcesFromConf inspects the given confmap.Conf and builds all config sources referenced
+// in the configuration intended to be used with ResolveWithConfigSources().
+func BuildConfigSourcesFromConf(ctx context.Context, confToFurtherResolve *confmap.Conf, logger *zap.Logger, factories Factories, confmapProviders map[string]confmap.Provider) (map[string]ConfigSource, *confmap.Conf, error) {
+	configSourceSettings, confWithoutSettings, err := SettingsFromConf(ctx, confToFurtherResolve, factories, confmapProviders)
+	if err != nil {
+		return nil, nil, fmt.Errorf("failed to parse settings from conf: %w", err)
+	}
+
+	configSources, err := BuildConfigSources(context.Background(), configSourceSettings, logger, factories)
+	if err != nil {
+		return nil, nil, fmt.Errorf("failed to build config sources: %w", err)
+	}
+	return configSources, confWithoutSettings, nil
+}
+
+func BuildConfigSources(ctx context.Context, configSourcesSettings map[string]Settings, logger *zap.Logger, factories Factories) (map[string]ConfigSource, error) {
+	cfgSources := make(map[string]ConfigSource, len(configSourcesSettings))
+	for fullName, cfgSrcSettings := range configSourcesSettings {
+		// If we have the setting we also have the factory.
+		factory, ok := factories[cfgSrcSettings.ID().Type()]
+		if !ok {
+			return nil, fmt.Errorf("unknown %s config source type for %s", cfgSrcSettings.ID().Type(), fullName)
+		}
+
+		cfgSrc, err := factory.CreateConfigSource(ctx, cfgSrcSettings, logger.With(zap.String("config_source", fullName)))
+		if err != nil {
+			return nil, fmt.Errorf("failed to create config source %s: %w", fullName, err)
+		}
+
+		if cfgSrc == nil {
+			return nil, fmt.Errorf("factory for %q produced a nil extension", fullName)
+		}
+
+		cfgSources[fullName] = cfgSrc
+	}
+
+	return cfgSources, nil
+}
+
+// ResolveWithConfigSources returns a confmap.Conf in which all env vars and config sources on
 // the given input config map are resolved to actual literal values of the env vars or config sources.
 //
-// 1. BuildConfigSourcesAndResolve to inject the data from config sources into a configuration;
+// 1. ResolveWithConfigSources to inject the data from config sources into a configuration;
 // 2. Wait for an update on "watcher" func.
 // 3. Close the confmap.Retrieved instance;
 //
@@ -202,45 +240,6 @@ type Factories map[component.Type]Factory
 // results.
 //
 // For an overview about the internals of the Manager refer to the package README.md.
-
-func BuildConfigSourcesAndSettings(ctx context.Context, confToFurtherResolve *confmap.Conf, logger *zap.Logger, factories Factories, confmapProviders map[string]confmap.Provider) (map[string]ConfigSource, *confmap.Conf, error) {
-	configSourceSettings, confWithoutSettings, err := SettingsFromConf(ctx, confToFurtherResolve, factories, confmapProviders)
-	if err != nil {
-		return nil, nil, fmt.Errorf("failed to parse settings from conf: %w", err)
-	}
-
-	configSources, err := BuildConfigSources(context.Background(), configSourceSettings, logger, factories)
-	if err != nil {
-		return nil, nil, fmt.Errorf("failed to build config sources: %w", err)
-	}
-	return configSources, confWithoutSettings, nil
-}
-
-// BuildConfigSources builds the ConfigSource objects according to the given ConfigSettings.
-func BuildConfigSources(ctx context.Context, configSourcesSettings map[string]Settings, logger *zap.Logger, factories Factories) (map[string]ConfigSource, error) {
-	cfgSources := make(map[string]ConfigSource, len(configSourcesSettings))
-	for fullName, cfgSrcSettings := range configSourcesSettings {
-		// If we have the setting we also have the factory.
-		factory, ok := factories[cfgSrcSettings.ID().Type()]
-		if !ok {
-			return nil, fmt.Errorf("unknown %s config source type for %s", cfgSrcSettings.ID().Type(), fullName)
-		}
-
-		cfgSrc, err := factory.CreateConfigSource(ctx, cfgSrcSettings, logger.With(zap.String("config_source", fullName)))
-		if err != nil {
-			return nil, fmt.Errorf("failed to create config source %s: %w", fullName, err)
-		}
-
-		if cfgSrc == nil {
-			return nil, fmt.Errorf("factory for %q produced a nil extension", fullName)
-		}
-
-		cfgSources[fullName] = cfgSrc
-	}
-
-	return cfgSources, nil
-}
-
 func ResolveWithConfigSources(ctx context.Context, configSources map[string]ConfigSource, confmapProviders map[string]confmap.Provider, conf *confmap.Conf, watcher confmap.WatcherFunc) (*confmap.Conf, confmap.CloseFunc, error) {
 	resolved := map[string]any{}
 	var closeFuncs []confmap.CloseFunc

internal/configsource/source_test.go

@@ -33,7 +33,7 @@ import (
 var errValueUpdated = errors.New("configuration must retrieve the updated value")
 
 func BuildConfigSourcesAndResolve(ctx context.Context, confToFurtherResolve *confmap.Conf, logger *zap.Logger, factories Factories, watcher confmap.WatcherFunc) (*confmap.Conf, confmap.CloseFunc, error) {
-	cfgSources, conf, err := BuildConfigSourcesAndSettings(ctx, confToFurtherResolve, logger, factories, nil)
+	cfgSources, conf, err := BuildConfigSourcesFromConf(ctx, confToFurtherResolve, logger, factories, nil)
 	if err != nil {
 		return nil, nil, err
 	}

internal/confmapprovider/configsource/provider.go

@@ -79,9 +79,8 @@ type ProviderWrapper struct {
 var _ confmap.Provider = (*wrappedProvider)(nil)
 
 type wrappedProvider struct {
-	wrapper       *ProviderWrapper
-	provider      confmap.Provider
-	lastRetrieved *confmap.Retrieved
+	wrapper  *ProviderWrapper
+	provider confmap.Provider
 }
 
 func (w *wrappedProvider) Retrieve(ctx context.Context, uri string, watcher confmap.WatcherFunc) (*confmap.Retrieved, error) {
@@ -102,93 +101,65 @@ func (w *wrappedProvider) Shutdown(ctx context.Context) error {
 	return w.provider.Shutdown(ctx)
 }
 
-func (c *ProviderWrapper) Provider(provider confmap.Provider) confmap.Provider {
-	for _, h := range c.hooks {
+func (pw *ProviderWrapper) Provider(provider confmap.Provider) confmap.Provider {
+	for _, h := range pw.hooks {
 		h.OnNew()
 	}
-	c.providersLock.Lock()
-	defer c.providersLock.Unlock()
-	c.providers[provider.Scheme()] = provider
-	return &wrappedProvider{provider: provider, wrapper: c, lastRetrieved: &confmap.Retrieved{}}
+	pw.providersLock.Lock()
+	defer pw.providersLock.Unlock()
+	pw.providers[provider.Scheme()] = provider
+	return &wrappedProvider{provider: provider, wrapper: pw}
 }
 
-// ResolveForWrapped will retrieve from the wrappedProvider provider and merge the result w/ a previous retrieved instance (if any) as latestConf.
-// It will then "configsource.BuildConfigSourcesAndResolve(latestConf)" and return the resolved map as a confmap.Retrieved w/ resolve closer and the wrappedProvider provider closer.
-func (c *ProviderWrapper) ResolveForWrapped(ctx context.Context, uri string, onChange confmap.WatcherFunc, w *wrappedProvider) (*confmap.Retrieved, error) {
-	provider := w.provider
-	retrieved := &confmap.Retrieved{}
-
-	var tmpRetrieved *confmap.Retrieved
-	var err error
-	// Here we are getting the value directly from the provider, which
-	// is what the core's resolver intends (invokes this parent method).
-	if tmpRetrieved, err = provider.Retrieve(ctx, uri, onChange); err != nil {
+// ResolveForWrapped will retrieve from the wrappedProvider and, if possible, resolve all config source directives with their resolved values.
+// If the wrappedProvider's retrieved value is only valid AsRaw() (scalar/array) then that will be returned without further evaluation.
+func (pw *ProviderWrapper) ResolveForWrapped(ctx context.Context, uri string, onChange confmap.WatcherFunc, w *wrappedProvider) (*confmap.Retrieved, error) {
+	retrieved, err := w.provider.Retrieve(ctx, uri, onChange)
+	if err != nil {
 		return nil, fmt.Errorf("configsource provider failed retrieving: %w", err)
-	} else if tmpRetrieved != nil {
-		retrieved = tmpRetrieved
 	}
 
-	var previousConf *confmap.Conf
-	var rawRetrieved *confmap.Retrieved
-	if previousConf, err = w.lastRetrieved.AsConf(); err != nil {
-		return nil, fmt.Errorf("configsource provider failed updated retrieval: %w", err)
-	} else if previousConf != nil {
-		// Need to merge config maps that we've encountered so far
-		if latestConf, e := retrieved.AsConf(); e != nil {
-			// raw fallback
-			if raw, ee := retrieved.AsRaw(); ee == nil {
-				if rawRetrieved, ee = confmap.NewRetrieved(raw); ee != nil {
-					return nil, fmt.Errorf("failed resolving wrappedProvider retrieve: %w", e)
-				}
+	conf, err := retrieved.AsConf()
+	if err != nil {
+		// raw fallback attempt, or return AsConf() error
+		if raw, e := retrieved.AsRaw(); e == nil {
+			if rawRetrieved, ee := confmap.NewRetrieved(raw); ee == nil {
+				// raw confmap.Retrieved values shouldn't be further processed so return here
+				return rawRetrieved, nil
 			}
-		} else if e = latestConf.Merge(previousConf); e != nil {
-			return nil, fmt.Errorf("failed merging previous wrappedProvider retrieve: %w", e)
-		} else if tmpRetrieved, e = confmap.NewRetrieved(latestConf.ToStringMap()); e != nil {
-			return nil, err
-		} else if tmpRetrieved != nil {
-			retrieved = tmpRetrieved
 		}
+		return nil, fmt.Errorf("failed retrieving from wrappedProvider: %w", err)
 	}
 
-	// raw confmap.Retrieved values cannot be coerced AsConf() so we return here to not update lastRetrieved
-	// or attempt subsequent config source value resolution.
-	if rawRetrieved != nil {
-		return rawRetrieved, nil
-	}
-
-	w.lastRetrieved = retrieved
-
-	latestConf, err := w.lastRetrieved.AsConf()
-	if err != nil {
-		return nil, err
-	} else if latestConf == nil {
-		return nil, fmt.Errorf("latest retrieved confmap.Conf is nil")
+	if conf == nil {
+		return nil, fmt.Errorf("retrieved confmap.Conf is unexpectedly nil")
 	}
 
-	scheme, stringMap := provider.Scheme(), latestConf.ToStringMap()
-	for _, h := range c.hooks {
+	scheme, stringMap := w.provider.Scheme(), conf.ToStringMap()
+	for _, h := range pw.hooks {
 		h.OnRetrieve(scheme, stringMap)
 	}
 
-	c.providersLock.Lock()
+	// copy providers map for downstream resolution
+	pw.providersLock.Lock()
 	providers := map[string]confmap.Provider{}
-	for s, p := range c.providers {
+	for s, p := range pw.providers {
 		providers[s] = p
 	}
-	c.providersLock.Unlock()
-	configSources, conf, err := configsource.BuildConfigSourcesAndSettings(ctx, latestConf, c.logger, c.factories, providers)
+	pw.providersLock.Unlock()
+	configSources, confToResolve, err := configsource.BuildConfigSourcesFromConf(ctx, conf, pw.logger, pw.factories, providers)
 	if err != nil {
 		return nil, fmt.Errorf("failed resolving latestConf: %w", err)
 	}
 
-	resolved, closeFunc, err := configsource.ResolveWithConfigSources(ctx, configSources, providers, conf, onChange)
+	resolved, closeFunc, err := configsource.ResolveWithConfigSources(ctx, configSources, providers, confToResolve, onChange)
 	if err != nil {
 		return nil, fmt.Errorf("failed resolving with config sources: %w", err)
 	}
 
 	return confmap.NewRetrieved(
 		resolved.ToStringMap(), confmap.WithRetrievedClose(
-			configsource.MergeCloseFuncs([]confmap.CloseFunc{closeFunc, w.lastRetrieved.Close}),
+			configsource.MergeCloseFuncs([]confmap.CloseFunc{closeFunc, retrieved.Close}),
 		),
 	)
 }