fix: prepending PATH logic [IDE-1314] (#402)

Previously it would not re-prioritise to the front an entry prepended to the PATH.
Plus a bit of a code refactor for clarity.

Author: rrama

pkg/envvars/environment.go

@@ -10,6 +10,14 @@ import (
 	"time"
 
 	"github.com/subosito/gotenv"
+
+	"github.com/snyk/go-application-framework/pkg/utils"
+)
+
+// Environment variable names
+const (
+	PathEnvVarName  = "PATH"
+	ShellEnvVarName = "SHELL"
 )
 
 // LoadConfiguredEnvironment updates the environment with local configuration. Precedence as follows:
@@ -24,7 +32,7 @@ func LoadConfiguredEnvironment(customConfigFiles []string, workingDirectory stri
 	defer func() { _ = gotenv.Apply(strings.NewReader(bashOutput)) }() //nolint:errcheck // we can't do anything with the error
 
 	env := gotenv.Parse(strings.NewReader(bashOutput))
-	specificShell, ok := env["SHELL"]
+	specificShell, ok := env[ShellEnvVarName]
 	if ok {
 		fromSpecificShell := getEnvFromShell(specificShell)
 		_ = gotenv.Apply(strings.NewReader(fromSpecificShell)) //nolint:errcheck // we can't do anything with the error
@@ -41,7 +49,7 @@ func LoadConfiguredEnvironment(customConfigFiles []string, workingDirectory stri
 
 func loadFile(fileName string) {
 	// preserve path
-	path := os.Getenv("PATH")
+	previousPath := os.Getenv(PathEnvVarName)
 
 	// overwrite existing variables with file config
 	err := gotenv.OverLoad(fileName)
@@ -50,7 +58,7 @@ func loadFile(fileName string) {
 	}
 
 	// add previous path to the end of the new
-	UpdatePath(path, false)
+	UpdatePath(previousPath, false)
 }
 
 // guard against command injection
@@ -92,39 +100,38 @@ func getEnvFromShell(shell string) string {
 	return string(env)
 }
 
-// UpdatePath prepends or appends the extension to the current path. If the entry is already there, it skips it. The
-// result is set into the process environment with os.Setenv.
+// UpdatePath prepends or appends the extension to the current path.
+// For append, if the entry is already there, it will not be re-added / moved.
+// For prepend, if the entry is already there, it will be correctly re-prioritized to the front.
+// The result is set into the process environment with os.Setenv.
 //
 //	pathExtension string the path component to be added.
 //	prepend bool whether to pre- or append
 func UpdatePath(pathExtension string, prepend bool) string {
-	pathVarName := "PATH"
+	currentPath := os.Getenv(PathEnvVarName)
 
 	if pathExtension == "" {
-		return os.Getenv(pathVarName)
+		return currentPath
 	}
 
-	currentPath := os.Getenv(pathVarName)
-	currentPathEntries := strings.Split(currentPath, string(os.PathListSeparator))
-
-	pathEntries := map[string]bool{}
-	for _, entry := range currentPathEntries {
-		pathEntries[entry] = true
+	if currentPath == "" {
+		_ = os.Setenv(PathEnvVarName, pathExtension)
+		return pathExtension
 	}
 
+	currentPathEntries := strings.Split(currentPath, string(os.PathListSeparator))
 	addPathEntries := strings.Split(pathExtension, string(os.PathListSeparator))
-	var newPathSlice []string
-	for _, entry := range addPathEntries {
-		if !pathEntries[entry] {
-			newPathSlice = append(newPathSlice, entry)
-		}
-	}
 
-	resultSlice := append(newPathSlice, currentPathEntries...)
-	if !prepend {
-		resultSlice = append(currentPathEntries, newPathSlice...)
+	var combinedSliceWithDuplicates []string
+	if prepend {
+		combinedSliceWithDuplicates = append(addPathEntries, currentPathEntries...)
+	} else {
+		combinedSliceWithDuplicates = append(currentPathEntries, addPathEntries...)
 	}
-	newPath := strings.Join(resultSlice, string(os.PathListSeparator))
-	_ = os.Setenv(pathVarName, newPath)
+
+	newPathSlice := utils.Dedupe(combinedSliceWithDuplicates)
+
+	newPath := strings.Join(newPathSlice, string(os.PathListSeparator))
+	_ = os.Setenv(PathEnvVarName, newPath)
 	return newPath
 }

pkg/envvars/environment_test.go

@@ -42,6 +42,71 @@ func TestUpdatePathWithDefaults(t *testing.T) {
 		require.Equal(t, "b"+pathListSep+pathFromEnv, os.Getenv("PATH"))
 	})
 
+	t.Run("prepend re-prioritizes existing path segment to front", func(t *testing.T) {
+		pathFromEnv := "a" + pathListSep + "b" + pathListSep + "c"
+		t.Setenv("PATH", pathFromEnv)
+
+		UpdatePath("b", true)
+
+		require.Equal(t, "b"+pathListSep+"a"+pathListSep+"c", os.Getenv("PATH"))
+	})
+
+	t.Run("add multiple entries at once (prepend)", func(t *testing.T) {
+		pathFromEnv := "d" + pathListSep + "e"
+		t.Setenv("PATH", pathFromEnv)
+
+		pathToPrepend := "a" + pathListSep + "b" + pathListSep + "c"
+		UpdatePath(pathToPrepend, true)
+
+		require.Equal(t, pathToPrepend+pathListSep+pathFromEnv, os.Getenv("PATH"))
+	})
+
+	t.Run("add multiple entries at once (append)", func(t *testing.T) {
+		pathFromEnv := "a" + pathListSep + "b"
+		t.Setenv("PATH", pathFromEnv)
+
+		pathToAppend := "c" + pathListSep + "d" + pathListSep + "e"
+		UpdatePath(pathToAppend, false)
+
+		require.Equal(t, pathFromEnv+pathListSep+pathToAppend, os.Getenv("PATH"))
+	})
+
+	t.Run("add multiple entries with duplicates of existing entries (prepend)", func(t *testing.T) {
+		pathFromEnv := "b" + pathListSep + "d"
+		t.Setenv("PATH", pathFromEnv)
+
+		UpdatePath("a"+pathListSep+"b"+pathListSep+"c", true)
+
+		require.Equal(t, "a"+pathListSep+"b"+pathListSep+"c"+pathListSep+"d", os.Getenv("PATH"))
+	})
+
+	t.Run("add multiple entries with duplicates of existing entries (append)", func(t *testing.T) {
+		pathFromEnv := "a" + pathListSep + "b" + pathListSep + "c"
+		t.Setenv("PATH", pathFromEnv)
+
+		UpdatePath("b"+pathListSep+"c"+pathListSep+"d", false)
+
+		require.Equal(t, "a"+pathListSep+"b"+pathListSep+"c"+pathListSep+"d", os.Getenv("PATH"))
+	})
+
+	t.Run("add multiple entries with duplicates of new entries (prepend)", func(t *testing.T) {
+		pathFromEnv := "b" + pathListSep + "d"
+		t.Setenv("PATH", pathFromEnv)
+
+		UpdatePath("a"+pathListSep+"b"+pathListSep+"c"+pathListSep+"b", true)
+
+		require.Equal(t, "a"+pathListSep+"b"+pathListSep+"c"+pathListSep+"d", os.Getenv("PATH"))
+	})
+
+	t.Run("add multiple entries with duplicates of new entries (append)", func(t *testing.T) {
+		pathFromEnv := "a" + pathListSep + "b" + pathListSep + "c"
+		t.Setenv("PATH", pathFromEnv)
+
+		UpdatePath("b"+pathListSep+"c"+pathListSep+"d"+pathListSep+"c", false)
+
+		require.Equal(t, "a"+pathListSep+"b"+pathListSep+"c"+pathListSep+"d", os.Getenv("PATH"))
+	})
+
 	t.Run("add to path from environment only if not blank", func(t *testing.T) {
 		pathFromEnv := "a"
 		t.Setenv("PATH", pathFromEnv)

pkg/utils/array.go

@@ -74,6 +74,26 @@ func Merge(input1 []string, input2 []string) []string {
 	return result
 }
 
+// Dedupe removes duplicate entries from a given slice.
+// Returns a new, deduplicated slice.
+//
+// Example:
+//
+//	mySlice := []string{"apple", "banana", "apple", "cherry", "banana", "date"}
+//	dedupedSlice := dedupe(mySlice)
+//	fmt.Println(dedupedSlice) // Output: [apple banana cherry date]
+func Dedupe(s []string) []string {
+	seen := make(map[string]bool)
+	var result []string
+	for _, str := range s {
+		if _, ok := seen[str]; !ok {
+			seen[str] = true
+			result = append(result, str)
+		}
+	}
+	return result
+}
+
 // ToKeyValueMap converts a list of strings to a map of strings.
 // The input list will be converted based on the delimiter, 'splitBy'.
 // The resulting map will contain the keys and values of the input list.