Refactoring and Table tests

kyaml/setters2/settersutil/substitutioncreator.go

@@ -4,7 +4,6 @@
 package settersutil
 
 import (
-	"math"
 	"strings"
 
 	"sigs.k8s.io/kustomize/kyaml/errors"
@@ -138,17 +137,23 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) {
 		if marker, ok := indices[patternInd]; ok {
 			// increment the patternInd to end of marker. This helps us to extract the substring before next marker.
 			patternInd += len(marker)
-			value := c.extractValueForMarker(&fvInd, fv, patternInd, indices)
+			var value string
+			if value, fvInd, err = c.extractValueForMarker(fvInd, fv, patternInd, indices); err != nil {
+				return nil, err
+			}
 			// if marker is repeated in the pattern, make sure that the corresponding values
 			// are same and throw error if not.
 			if prevValue, ok := m[marker]; ok && prevValue != value {
-				return nil, errors.Errorf("same marker is found to have different values in field value")
+				return nil, errors.Errorf(
+					"marker %s is found to have different values %s and %s", marker, prevValue, value)
 			}
 			m[marker] = value
 		} else {
 			// Ex: fv = "samething/ubuntu:0.1.0" pattern = "something/IMAGE:VERSION". Error out at 'a' in fv.
 			if fv[fvInd] != pattern[patternInd] {
-				return nil, errors.Errorf("unable to derive values for markers, create setters for all markers and then try again")
+				return nil, errors.Errorf(
+					"unable to derive values for markers, " +
+						"create setters for all markers and then try again")
 			}
 			fvInd++
 			patternInd++
@@ -156,7 +161,9 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) {
 	}
 	// check if both strings are completely visited or throw error
 	if fvInd < len(fv) || patternInd < len(pattern) {
-		return nil, errors.Errorf("unable to derive values for markers, create setters for all markers and then try again")
+		return nil, errors.Errorf(
+			"unable to derive values for markers, " +
+				"create setters for all markers and then try again")
 	}
 	return m, nil
 }
@@ -164,48 +171,83 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) {
 // GetStartIndices returns the start indices of all the markers in the pattern
 func (c SubstitutionCreator) GetStartIndices() (map[int]string, error) {
 	indices := make(map[int]string)
-	p := c.Pattern
 	for _, value := range c.Values {
-		m := value.Marker
 		found := false
-		for i := range p {
-			if strings.HasPrefix(p[i:], m) {
-				indices[i] = m
+		for i := range c.Pattern {
+			if strings.HasPrefix(c.Pattern[i:], value.Marker) {
+				indices[i] = value.Marker
 				found = true
 			}
 		}
 		if !found {
-			return nil, errors.Errorf("Unable to find marker " + m + " in the pattern")
+			return nil, errors.Errorf("unable to find marker " + value.Marker + " in the pattern")
 		}
 	}
+	if err := validateMarkers(indices); err != nil {
+		return nil, err
+	}
 	return indices, nil
 }
 
-func (c SubstitutionCreator) extractValueForMarker(fvInd *int, fv string, patternInd int, indices map[int]string) string {
-	value := ""
-	lm := lenTillNextMarker(indices, patternInd)
-
-	// keep appending chars to value till we find the substring between 2 markers in fv
-	// In example fv = "something/ubuntu:0.1.0", pattern = "something/IMAGE:VERSION",
-	// proceed till we find substring ":" in fv which makes value = ubuntu for marker IMAGE
-	for *fvInd < len(fv) && (patternInd == len(c.Pattern) ||
-		fv[*fvInd:min(len(fv), *fvInd+lm)] != c.Pattern[patternInd:min(patternInd+lm, len(c.Pattern))]) {
-		value += string(fv[*fvInd])
-		*fvInd++
-	}
-	return value
-}
-
-// lenToNextMarker takes in the indices map, an index and returns the distance to
-// next marker start index in indices map
-func lenTillNextMarker(m map[int]string, j int) int {
-	res := math.MaxInt32
-	for k := range m {
-		if k > j {
-			res = min(k-j, res)
+// validateMarkers takes the indices map, checks if any of 2 markers not have delimiters,
+// checks if any marker is substring of other and returns error
+func validateMarkers(indices map[int]string) error {
+	for k1, v1 := range indices {
+		for k2, v2 := range indices {
+			if k1 != k2 && k1+len(v1) == k2 {
+				return errors.Errorf(
+					"markers %s and %s are found to have no delimiters between them,"+
+						" pre-create setters and try again", v1, v2)
+			}
+			if v1 != v2 && strings.Contains(v1, v2) {
+				return errors.Errorf(
+					"markers %s is substring of %s,"+
+						" no marker should be substring of other", v2, v1)
+			}
 		}
 	}
-	return res
+	return nil
+}
+
+// extractValueForMarker returns the value string for a marker and the incremented index
+func (c SubstitutionCreator) extractValueForMarker(fvInd int, fv string, patternInd int, indices map[int]string) (string, int, error) {
+	nonMarkerStr := strTillNextMarker(indices, patternInd, c.Pattern)
+
+	// return the remaining string of fv till end if patternInd is at end of pattern
+	if patternInd == len(c.Pattern) {
+		return fv[fvInd:], len(fv), nil
+	}
+
+	// split remaining fv starting from fvInd with the non marker substring delimiter and get the first value
+	// In example fv = "something/ubuntu::0.1.0", pattern = "something/IMAGE::VERSION",
+	// split with "::" delimiter in fv which gives markerValue = ubuntu for marker IMAGE
+	// increment fvInd by length of extracted marker value and return fvInd
+	if markerValues := strings.Split(fv[fvInd:], nonMarkerStr); len(markerValues) > 0 {
+		return markerValues[0], fvInd + len(markerValues[0]), nil
+	}
+
+	return "", -1, errors.Errorf(
+		"unable to derive values for markers," +
+			" create setters for all markers and then try again")
+}
+
+// substrOfLen takes a string, start index and length and returns substring of given length
+// or till end of string
+func substrOfLen(str string, startInd int, length int) string {
+	return str[startInd:min(len(str), startInd+length)]
+}
+
+// strTillNextMarker takes in the indices map, a start index and returns the substring till
+// start of next marker
+func strTillNextMarker(indices map[int]string, startInd int, pattern string) string {
+	// initialize with max value which is length of pattern
+	nextMarkerStartInd := len(pattern)
+	for ind := range indices {
+		if ind > startInd {
+			nextMarkerStartInd = min(ind-startInd, nextMarkerStartInd)
+		}
+	}
+	return substrOfLen(pattern, startInd, nextMarkerStartInd)
 }
 
 func min(a int, b int) int {

kyaml/setters2/settersutil/substitutioncreator_test.go

@@ -7,110 +7,105 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"sigs.k8s.io/kustomize/kyaml/errors"
 	"sigs.k8s.io/kustomize/kyaml/setters2"
 )
 
-func TestGetValuesForMarkersPositive(t *testing.T) {
-	c := SubstitutionCreator{
-		Pattern:    "something/IMAGE::VERSION/otherthing/IMAGE::VERSION/",
-		Values:     Values(),
-		FieldValue: "something/nginx::0.1.0/otherthing/nginx::0.1.0/",
+func TestGetValuesForMarkers(t *testing.T) {
+	var tests = []struct {
+		name           string
+		pattern        string
+		fieldValue     string
+		markers        []string
+		expectedError  error
+		expectedOutput map[string]string
+	}{
+		{
+			name:           "positive example",
+			markers:        []string{"IMAGE", "VERSION"},
+			pattern:        "something/IMAGE::VERSION/otherthing/IMAGE::VERSION/",
+			fieldValue:     "something/nginx::0.1.0/otherthing/nginx::0.1.0/",
+			expectedOutput: map[string]string{"IMAGE": "nginx", "VERSION": "0.1.0"},
+		},
+		{
+			name:          "marker with different values",
+			markers:       []string{"IMAGE", "VERSION"},
+			pattern:       "something/IMAGE:VERSION/IMAGE",
+			fieldValue:    "something/nginx:0.1.0/ubuntu",
+			expectedError: errors.Errorf("marker IMAGE is found to have different values nginx and ubuntu"),
+		},
+		{
+			name:          "unmatched pattern",
+			markers:       []string{"IMAGE", "VERSION"},
+			pattern:       "something/IMAGE:VERSION",
+			fieldValue:    "otherthing/nginx:0.1.0",
+			expectedError: errors.Errorf("unable to derive values for markers"),
+		},
+		{
+			name:          "unmatched pattern at the end",
+			markers:       []string{"IMAGE", "VERSION"},
+			pattern:       "something/IMAGE:VERSION/abc",
+			fieldValue:    "something/nginx:0.1.0/abcd",
+			expectedError: errors.Errorf("unable to derive values for markers"),
+		},
+		{
+			name:          "substring markers",
+			markers:       []string{"IMAGE", "VERSION", "MAGE"},
+			pattern:       "something/IMAGE:VERSION/abc/MAGE",
+			fieldValue:    "something/nginx:0.1.0/abc/ubuntu",
+			expectedError: errors.Errorf("no marker should be substring of other"),
+		},
+		{
+			name:          "markers with no delimiters",
+			markers:       []string{"IMAGE", "VERSION"},
+			pattern:       "something/IMAGEVERSION/",
+			fieldValue:    "something/nginx0.1.0/",
+			expectedError: errors.Errorf("no delimiters between them"),
+		},
+		{
+			name:          "unmatched delimiter",
+			markers:       []string{"IMAGE", "VERSION"},
+			pattern:       "something/IMAGE:^VERSION/otherthing/IMAGE::VERSION/",
+			fieldValue:    "something/nginx::0.1.0/otherthing/nginx::0.1.0/",
+			expectedError: errors.Errorf("unable to derive values for markers"),
+		},
 	}
+	for i := range tests {
+		test := tests[i]
+		t.Run(test.name, func(t *testing.T) {
+			values := []setters2.Value{}
+			for _, marker := range test.markers {
+				value := setters2.Value{
+					Marker: marker,
+				}
+				values = append(values, value)
+			}
 
-	m, err := c.GetValuesForMarkers()
+			sc := SubstitutionCreator{
+				Pattern:    test.pattern,
+				Values:     values,
+				FieldValue: test.fieldValue,
+			}
 
-	if !assert.NoError(t, err) {
-		t.FailNow()
-	}
+			m, err := sc.GetValuesForMarkers()
 
-	assert.Equal(t, m["IMAGE"], "nginx")
-	assert.Equal(t, m["VERSION"], "0.1.0")
-}
-
-func TestGetValuesForMarkersDiffMarkerValues(t *testing.T) {
-	c := SubstitutionCreator{
-		Pattern:    "something/IMAGE:VERSION/IMAGE",
-		Values:     Values(),
-		FieldValue: "something/nginx:0.1.0/ubuntu",
-	}
-
-	_, err := c.GetValuesForMarkers()
-
-	if !assert.Error(t, err) {
-		t.FailNow()
-	}
-
-	if !assert.Equal(t, err.Error(), "same marker is found to have different values in field value") {
-		t.FailNow()
+			if test.expectedError == nil {
+				// fail if expectedError is nil but actual error is not
+				if !assert.NoError(t, err) {
+					t.FailNow()
+				}
+				// check if all the expected markers and values are present in actual map
+				for k, v := range test.expectedOutput {
+					if val, ok := m[k]; ok {
+						assert.Equal(t, v, val)
+					} else {
+						t.FailNow()
+					}
+				}
+			} else {
+				//if expectedError is not nil, check for correctness of error message
+				assert.Contains(t, err.Error(), test.expectedError.Error())
+			}
+		})
 	}
 }
-
-func TestGetValuesForMarkersNoMatch(t *testing.T) {
-	c := SubstitutionCreator{
-		Pattern:    "something/IMAGE:VERSION",
-		Values:     Values(),
-		FieldValue: "otherthing/nginx:0.1.0",
-	}
-
-	_, err := c.GetValuesForMarkers()
-
-	if !assert.Error(t, err) {
-		t.FailNow()
-	}
-
-	if !assert.Equal(t, err.Error(), "unable to derive values for markers, create setters for all markers and then try again") {
-		t.FailNow()
-	}
-}
-
-func TestGetValuesForMarkersNoMatch2(t *testing.T) {
-	c := SubstitutionCreator{
-		Pattern:    "something/IMAGE:VERSION/abc",
-		Values:     Values(),
-		FieldValue: "something/nginx:0.1.0/abcd",
-	}
-
-	_, err := c.GetValuesForMarkers()
-
-	if !assert.Error(t, err) {
-		t.FailNow()
-	}
-
-	if !assert.Equal(t, err.Error(), "unable to derive values for markers, create setters for all markers and then try again") {
-		t.FailNow()
-	}
-}
-
-func TestGetValuesForMarkersSubStngMarkers(t *testing.T) {
-	value3 := setters2.Value{
-		Marker: "MAGE",
-	}
-
-	c := SubstitutionCreator{
-		Pattern:    "something/IMAGE:VERSION/abc/MAGE",
-		Values:     append(Values(), value3),
-		FieldValue: "something/nginx:0.1.0/abc/ubuntu",
-	}
-
-	m, err := c.GetValuesForMarkers()
-
-	if !assert.NoError(t, err) {
-		t.FailNow()
-	}
-
-	assert.Equal(t, m["IMAGE"], "nginx")
-	assert.Equal(t, m["VERSION"], "0.1.0")
-	assert.Equal(t, m["MAGE"], "ubuntu")
-}
-
-func Values() []setters2.Value {
-	value1 := setters2.Value{
-		Marker: "IMAGE",
-	}
-
-	value2 := setters2.Value{
-		Marker: "VERSION",
-	}
-
-	return []setters2.Value{value1, value2}
-}