Repository: amp-labs/connectors
Pull request #690 (2024-07-13 09:47:11)

tidy: Relocate substitution algorithm to package
This is refactoring. Moving algorithm that performs catalog varialbe substitution into a separate package. Providers package should be more concise.

As an example running Read on DynamicsCRM where workspace is required produces same output prior to change.
![image](https://github.com/user-attachments/assets/666dada4-048b-4f3a-ace3-0c7cde935f03)


Cobalt0s: * **#690** <a href="https://app.graphite.dev/github/pr/amp-labs/connectors/690?utm_source=stack-comment-icon" target="_blank"><img src="https://static.graphite.dev/graphite-32x32-black.png" alt="Graphite" width="10px" height="10px"/></a> 👈
* `main`

This stack of pull requests is managed by Graphite. <a href="https://stacking.dev/?utm_source=stack-comment">Learn more about stacking.</a>
<h2></h2>

 Join @Cobalt0s and the rest of your teammates on <a href="https://graphite.dev?utm-source=stack-comment"><img src="https://static.graphite.dev/graphite-32x32-black.png" alt="Graphite" width="11px" height="11px"/> <b>Graphite</b></a>
  <!-- Current dependencies on/for this PR: -->

Patch:
diff --git a/common/paramsbuilder/variables.go b/common/paramsbuilder/variables.go
index 49ec15ea..f8ac93ab 100644
--- a/common/paramsbuilder/variables.go
+++ b/common/paramsbuilder/variables.go
@@ -3,7 +3,7 @@ package paramsbuilder
 import (
 	"log/slog"
 
-	"github.com/spyzhov/ajson"
+	"github.com/amp-labs/connectors/common/substitutions"
 )
 
 const (
@@ -21,30 +21,24 @@ type SubstitutionPlan struct {
 	To   string
 }
 
-type RegistryValue interface {
-	string | *ajson.Node
-}
-
-type SubstitutionRegistry[V RegistryValue] map[string]V
-
-func NewCatalogSubstitutionRegistry(vars []CatalogVariable) SubstitutionRegistry[string] {
-	substitutions := make(SubstitutionRegistry[string])
+func NewCatalogSubstitutionRegistry(vars []CatalogVariable) substitutions.Registry[string] {
+	subs := make(substitutions.Registry[string])
 
 	for _, variable := range vars {
 		s := variable.GetSubstitutionPlan()
-		substitutions[s.From] = s.To
+		subs[s.From] = s.To
 	}
 
-	return substitutions
+	return subs
 }
 
 // NewCatalogVariables converts JSON into supported list of Catalog Variables.
 // This enforces type checking.
-func NewCatalogVariables[V RegistryValue](registry SubstitutionRegistry[V]) []CatalogVariable {
+func NewCatalogVariables[V substitutions.RegistryValue](registry substitutions.Registry[V]) []CatalogVariable {
 	result := make([]CatalogVariable, 0)
 
 	for key, value := range registry {
-		name := registryValueToString(value)
+		name := substitutions.RegistryValueToString(value)
 
 		switch key {
 		case variableWorkspace:
@@ -56,16 +50,3 @@ func NewCatalogVariables[V RegistryValue](registry SubstitutionRegistry[V]) []Ca
 
 	return result
 }
-
-func registryValueToString[V RegistryValue](value V) string {
-	var name string
-	if v, ok := any(value).(string); ok {
-		name = v
-	}
-
-	if v, ok := any(value).(*ajson.Node); ok {
-		name = v.MustString()
-	}
-
-	return name
-}
diff --git a/common/substitutions/registry.go b/common/substitutions/registry.go
new file mode 100644
index 00000000..5ad80d9c
--- /dev/null
+++ b/common/substitutions/registry.go
@@ -0,0 +1,47 @@
+package substitutions
+
+import (
+	"errors"
+
+	"github.com/spyzhov/ajson"
+)
+
+var ErrSubstitutionFailure = errors.New("failed to resolve substitutions")
+
+type RegistryValue interface {
+	string | *ajson.Node
+}
+
+type Registry[V RegistryValue] map[string]V
+
+func (r Registry[V]) convertStrMap() map[string]string {
+	result := make(map[string]string)
+	for k, v := range r {
+		result[k] = RegistryValueToString(v)
+	}
+
+	return result
+}
+
+// Apply substitutions to the struct. Creates side effects.
+func (r Registry[V]) Apply(input any) error {
+	err := substituteStruct(input, r.convertStrMap())
+	if err != nil {
+		return errors.Join(err, ErrSubstitutionFailure)
+	}
+
+	return nil
+}
+
+func RegistryValueToString[V RegistryValue](value V) string {
+	var name string
+	if v, ok := any(value).(string); ok {
+		name = v
+	}
+
+	if v, ok := any(value).(*ajson.Node); ok {
+		name = v.MustString()
+	}
+
+	return name
+}
diff --git a/common/substitutions/substitutions.go b/common/substitutions/substitutions.go
new file mode 100644
index 00000000..a687c087
--- /dev/null
+++ b/common/substitutions/substitutions.go
@@ -0,0 +1,79 @@
+package substitutions
+
+import (
+	"reflect"
+	"strings"
+	"text/template" // nosemgrep: go.lang.security.audit.xss.import-text-template.import-text-template
+)
+
+// substituteStruct performs string substitution on the fields of the input struct
+// using the substitutions map.
+func substituteStruct(input interface{}, substitutions map[string]string) (err error) { //nolint:gocognit,cyclop,lll
+	configStruct := reflect.ValueOf(input).Elem()
+	for i := 0; i < configStruct.NumField(); i++ {
+		field := configStruct.Field(i)
+
+		// If the field is a string, perform substitution on it.
+		if field.Kind() == reflect.String {
+			substitutedVal, err := substitute(field.String(), substitutions)
+			if err != nil {
+				return err
+			}
+
+			field.SetString(substitutedVal)
+		}
+
+		if field.Kind() == reflect.Pointer {
+			if field.Elem().Kind() == reflect.Struct {
+				err := substituteStruct(field.Elem().Addr().Interface(), substitutions)
+				if err != nil {
+					return err
+				}
+			}
+		}
+
+		// If the field is a struct, perform substitution on its fields.
+		if field.Kind() == reflect.Struct {
+			err := substituteStruct(field.Addr().Interface(), substitutions)
+			if err != nil {
+				return err
+			}
+		}
+
+		// If the field is a map, perform substitution on its values.
+		if field.Kind() == reflect.Map {
+			for _, key := range field.MapKeys() {
+				val := field.MapIndex(key)
+				if val.Kind() == reflect.String {
+					substitutedVal, err := substitute(val.String(), substitutions)
+					if err != nil {
+						return err
+					}
+
+					field.SetMapIndex(key, reflect.ValueOf(substitutedVal))
+				}
+			}
+		}
+	}
+
+	return nil
+}
+
+// substitute performs string substitution on the input string
+// using the substitutions map.
+func substitute(input string, substitutions map[string]string) (string, error) {
+	// missing variables are not allowed, Execute will throw an error.
+	tmpl, err := template.New("-").Option("missingkey=error").Parse(input)
+	if err != nil {
+		return "", err
+	}
+
+	var result strings.Builder
+
+	err = tmpl.Execute(&result, &substitutions)
+	if err != nil {
+		return "", err
+	}
+
+	return result.String(), nil
+}
diff --git a/providers/utils.go b/providers/utils.go
index 8c3afb37..f563dd7d 100644
--- a/providers/utils.go
+++ b/providers/utils.go
@@ -8,9 +8,6 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
-	"reflect"
-	"strings"
-	"text/template" // nosemgrep: go.lang.security.audit.xss.import-text-template.import-text-template
 
 	"github.com/amp-labs/connectors/common"
 	"github.com/amp-labs/connectors/common/paramsbuilder"
@@ -20,11 +17,9 @@ import (
 )
 
 var (
-	ErrCatalogNotFound        = errors.New("catalog not found")
-	ErrProviderNotFound       = errors.New("provider not found")
-	ErrProviderOptionNotFound = errors.New("provider option not found")
-	ErrClient                 = errors.New("client creation failed")
-	ErrSubstitutionFailure    = errors.New("failed to resolve substitutions")
+	ErrCatalogNotFound  = errors.New("catalog not found")
+	ErrProviderNotFound = errors.New("provider not found")
+	ErrClient           = errors.New("client creation failed")
 )
 
 type CatalogOption func(params *catalogParams)
@@ -158,91 +153,7 @@ func (c CustomCatalog) ReadInfo(provider Provider, vars ...paramsbuilder.Catalog
 }
 
 func (i *ProviderInfo) SubstituteWith(vars []paramsbuilder.CatalogVariable) error {
-	// TODO catalog substitution algorithm could live outside of this package
-	return applySubstitutions(paramsbuilder.NewCatalogSubstitutionRegistry(vars), i)
-}
-
-func applySubstitutions(substitutions paramsbuilder.SubstitutionRegistry[string], input any) error {
-	// TODO paramsbuilder.SubstitutionRegistry should have this function as a method
-	// (further refactoring maybe not in paramsbuilder package)
-	err := substituteStruct(input, substitutions)
-	if err != nil {
-		return errors.Join(err, ErrSubstitutionFailure)
-	}
-
-	return nil
-}
-
-// substituteStruct performs string substitution on the fields of the input struct
-// using the substitutions map.
-func substituteStruct(input interface{}, substitutions map[string]string) (err error) { //nolint:gocognit,cyclop,lll
-	configStruct := reflect.ValueOf(input).Elem()
-	for i := 0; i < configStruct.NumField(); i++ {
-		field := configStruct.Field(i)
-
-		// If the field is a string, perform substitution on it.
-		if field.Kind() == reflect.String {
-			substitutedVal, err := substitute(field.String(), substitutions)
-			if err != nil {
-				return err
-			}
-
-			field.SetString(substitutedVal)
-		}
-
-		if field.Kind() == reflect.Pointer {
-			if field.Elem().Kind() == reflect.Struct {
-				err := substituteStruct(field.Elem().Addr().Interface(), substitutions)
-				if err != nil {
-					return err
-				}
-			}
-		}
-
-		// If the field is a struct, perform substitution on its fields.
-		if field.Kind() == reflect.Struct {
-			err := substituteStruct(field.Addr().Interface(), substitutions)
-			if err != nil {
-				return err
-			}
-		}
-
-		// If the field is a map, perform substitution on its values.
-		if field.Kind() == reflect.Map {
-			for _, key := range field.MapKeys() {
-				val := field.MapIndex(key)
-				if val.Kind() == reflect.String {
-					substitutedVal, err := substitute(val.String(), substitutions)
-					if err != nil {
-						return err
-					}
-
-					field.SetMapIndex(key, reflect.ValueOf(substitutedVal))
-				}
-			}
-		}
-	}
-
-	return nil
-}
-
-// substitute performs string substitution on the input string
-// using the substitutions map.
-func substitute(input string, substitutions map[string]string) (string, error) {
-	// missing variables are not allowed, Execute will throw an error.
-	tmpl, err := template.New("-").Option("missingkey=error").Parse(input)
-	if err != nil {
-		return "", err
-	}
-
-	var result strings.Builder
-
-	err = tmpl.Execute(&result, &substitutions)
-	if err != nil {
-		return "", err
-	}
-
-	return result.String(), nil
+	return paramsbuilder.NewCatalogSubstitutionRegistry(vars).Apply(i)
 }
 
 func (i *ProviderInfo) GetOption(key string) (string, bool) {


Test patch:
diff --git a/common/paramsbuilder/variables_test.go b/common/paramsbuilder/variables_test.go
index 30fc9319..5ec5db1b 100644
--- a/common/paramsbuilder/variables_test.go
+++ b/common/paramsbuilder/variables_test.go
@@ -3,6 +3,8 @@ package paramsbuilder
 import (
 	"reflect"
 	"testing"
+
+	"github.com/amp-labs/connectors/common/substitutions"
 )
 
 func TestNewCatalogVariables(t *testing.T) {
@@ -10,12 +12,12 @@ func TestNewCatalogVariables(t *testing.T) {
 
 	tests := []struct {
 		name     string
-		input    SubstitutionRegistry[string]
+		input    substitutions.Registry[string]
 		expected []CatalogVariable
 	}{
 		{
 			name: "Unknown substitutions are not translated to Variables",
-			input: SubstitutionRegistry[string]{
+			input: substitutions.Registry[string]{
 				"insect":  "butterfly",
 				"fish":    "catfish",
 				"nothing": "",
@@ -25,7 +27,7 @@ func TestNewCatalogVariables(t *testing.T) {
 		},
 		{
 			name: "Only workspace Variable is captured",
-			input: SubstitutionRegistry[string]{
+			input: substitutions.Registry[string]{
 				"insect":    "butterfly",
 				"workspace": "office",
 			},
@@ -54,19 +56,19 @@ func TestNewCatalogSubstitutionRegistry(t *testing.T) {
 	tests := []struct {
 		name     string
 		input    []CatalogVariable
-		expected SubstitutionRegistry[string]
+		expected substitutions.Registry[string]
 	}{
 		{
 			name:     "No variables - no substitutions",
 			input:    []CatalogVariable{},
-			expected: SubstitutionRegistry[string]{},
+			expected: substitutions.Registry[string]{},
 		},
 		{
 			name: "Workspace is translated into substitution",
 			input: []CatalogVariable{
 				&Workspace{Name: "cool organization"},
 			},
-			expected: SubstitutionRegistry[string]{
+			expected: substitutions.Registry[string]{
 				variableWorkspace: "cool organization",
 			},
 		},