fix: IAW conf key dep on org and fix SAST cache [IDE-1501] (#457)

IAW enabled config value was relying on the org, but did not have it as a key dependency, so was not invalidating the cache on org change.
The fetching of the SAST settings was trying to cache itself instead of relying on the config to do the caching.
Also made the SAST sub-settings use the main config for the SAST settings to prevent unnecessary re-fetching from the API. Note to the reviewer: The PR is split into 2 commits, if you don't like this change we can drop the second commit.

Optimised the tests to use common code to verify that the cache is cleared when the org changes. The previous SAST tests were not actually testing this.
This required moving the test utils to a sub-package, to prevent an import loop.

Author: rrama

pkg/local_workflows/code_workflow.go

@@ -43,30 +43,23 @@ func GetCodeFlagSet() *pflag.FlagSet {
 }
 
 // WORKFLOWID_CODE defines a new workflow identifier
-var WORKFLOWID_CODE workflow.Identifier = workflow.NewWorkflowIdentifier(codeWorkflowName)
+var WORKFLOWID_CODE = workflow.NewWorkflowIdentifier(codeWorkflowName)
 
 func getSastSettings(engine workflow.Engine) (*sast_contract.SastResponse, error) {
 	config := engine.GetConfiguration()
 	org := config.GetString(configuration.ORGANIZATION)
 	client := engine.GetNetworkAccess().GetHttpClient()
 	url := config.GetString(configuration.API_URL)
 	apiClient := api.NewApi(url, client)
-	tmp, err := apiClient.GetSastSettings(org)
-	if err != nil {
-		engine.GetLogger().Err(err).Msg("Failed to access settings.")
-		return nil, err
-	}
-
-	engine.GetConfiguration().Set(code_workflow.ConfigurationSastSettings, tmp)
-	return tmp, nil
+	return apiClient.GetSastSettings(org)
 }
 
 func getSastSettingsConfig(engine workflow.Engine) configuration.DefaultValueFunction {
 	err := engine.GetConfiguration().AddKeyDependency(code_workflow.ConfigurationSastSettings, configuration.ORGANIZATION)
 	if err != nil {
 		engine.GetLogger().Err(err).Msg("Failed to add dependency for SAST settings.")
 	}
-	callback := func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) {
+	callback := func(_ configuration.Configuration, existingValue any) (any, error) {
 		if existingValue != nil {
 			return existingValue, nil
 		}
@@ -82,44 +75,57 @@ func getSastSettingsConfig(engine workflow.Engine) configuration.DefaultValueFun
 	return callback
 }
 
+func getSastResponseFromConfig(config configuration.Configuration) (*sast_contract.SastResponse, error) {
+	sastResponseGeneric, err := config.GetWithError(code_workflow.ConfigurationSastSettings)
+	if err != nil {
+		return nil, err
+	}
+	if sastResponseGeneric == nil {
+		return nil, fmt.Errorf("SAST settings are nil")
+	}
+	sastResponse, ok := sastResponseGeneric.(*sast_contract.SastResponse)
+	if !ok {
+		return nil, fmt.Errorf("SAST settings are not a SastResponse")
+	}
+	return sastResponse, nil
+}
+
 func getSastEnabled(engine workflow.Engine) configuration.DefaultValueFunction {
-	err := engine.GetConfiguration().AddKeyDependency(code_workflow.ConfigurationSastEnabled, configuration.ORGANIZATION)
+	err := engine.GetConfiguration().AddKeyDependency(code_workflow.ConfigurationSastEnabled, code_workflow.ConfigurationSastSettings)
 	if err != nil {
 		engine.GetLogger().Err(err).Msg("Failed to add dependency for SAST settings.")
 	}
-	callback := func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) {
+	callback := func(config configuration.Configuration, existingValue any) (any, error) {
 		if existingValue != nil {
 			return existingValue, nil
 		}
-
-		response, err := getSastSettings(engine)
+		sastResponse, err := getSastResponseFromConfig(config)
 		if err != nil {
 			engine.GetLogger().Err(err).Msg("Failed to access settings.")
 			return false, err
 		}
 
-		return response.SastEnabled, nil
+		return sastResponse.SastEnabled, nil
 	}
 	return callback
 }
 
 func getSlceEnabled(engine workflow.Engine) configuration.DefaultValueFunction {
-	err := engine.GetConfiguration().AddKeyDependency(code_workflow.ConfigurarionSlceEnabled, configuration.ORGANIZATION)
+	err := engine.GetConfiguration().AddKeyDependency(code_workflow.ConfigurarionSlceEnabled, code_workflow.ConfigurationSastSettings)
 	if err != nil {
 		engine.GetLogger().Err(err).Msg("Failed to add dependency for SAST settings.")
 	}
-	callback := func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) {
+	callback := func(config configuration.Configuration, existingValue any) (any, error) {
 		if existingValue != nil {
 			return existingValue, nil
 		}
-
-		response, err := getSastSettings(engine)
+		sastResponse, err := getSastResponseFromConfig(config)
 		if err != nil {
 			engine.GetLogger().Err(err).Msg("Failed to access settings.")
 			return false, err
 		}
 
-		return response.LocalCodeEngine.Enabled, nil
+		return sastResponse.LocalCodeEngine.Enabled, nil
 	}
 	return callback
 }

pkg/local_workflows/code_workflow_test.go

@@ -9,7 +9,6 @@ import (
 	"os"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/golang/mock/gomock"
 	"github.com/rs/zerolog"
@@ -25,6 +24,7 @@ import (
 	"github.com/snyk/go-application-framework/pkg/local_workflows/code_workflow/sast_contract"
 	"github.com/snyk/go-application-framework/pkg/local_workflows/content_type"
 	"github.com/snyk/go-application-framework/pkg/local_workflows/json_schemas"
+	testutils "github.com/snyk/go-application-framework/pkg/local_workflows/test_utils"
 	"github.com/snyk/go-application-framework/pkg/mocks"
 	"github.com/snyk/go-application-framework/pkg/networking"
 	"github.com/snyk/go-application-framework/pkg/ui"
@@ -460,67 +460,6 @@ func Test_Code_UseNativeImplementation(t *testing.T) {
 	})
 }
 
-// Helper function to test key dependencies for configuration keys
-func testOrganizationDependency(
-	t *testing.T,
-	configKey string,
-	dependencyKey string,
-	configIsBoolean bool,
-) {
-	t.Helper()
-
-	// Setup
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-
-	config := configuration.NewWithOpts(configuration.WithCachingEnabled(10 * time.Minute))
-	config.Set(dependencyKey, "value1")
-
-	// Track how many times the callback is invoked
-	callCount := 0
-	testCallback := func(c configuration.Configuration, existingValue interface{}) (interface{}, error) {
-		callCount++
-		// Return a value that changes on each call. This allows us to check whether the returned value comes from
-		// the cache or this callback function.
-		if configIsBoolean {
-			// Alternate between true and false
-			return callCount%2 == 1, nil
-		}
-		return fmt.Sprintf("value-%d", callCount), nil
-	}
-
-	// Register the dependency
-	err := config.AddKeyDependency(configKey, dependencyKey)
-	assert.NoError(t, err)
-	config.AddDefaultValue(configKey, testCallback)
-
-	// First Get - should invoke callback
-	result1 := getValue(config, configKey, configIsBoolean)
-	assert.NotNil(t, result1)
-	assert.Equal(t, 1, callCount, "Callback should be invoked on first read")
-
-	// Second Get - should use cached value
-	result2 := getValue(config, configKey, configIsBoolean)
-	assert.Equal(t, result1, result2, "Cached value should be used on second read")
-	assert.Equal(t, 1, callCount, "Callback should not be called on second read")
-
-	// Change the value of the dependency - this should clear the cached value.
-	config.Set(dependencyKey, "value2")
-
-	// Third Get - should invoke callback again since cache was cleared
-	result3 := getValue(config, configKey, configIsBoolean)
-	assert.NotNil(t, result3)
-	assert.NotEqual(t, result1, result3, "Cached value should not be used after dependency changed")
-	assert.Equal(t, 2, callCount, "Callback should be called again after dependency changed")
-}
-
-func getValue(config configuration.Configuration, key string, isBoolean bool) interface{} {
-	if isBoolean {
-		return config.GetBool(key)
-	}
-	return config.Get(key)
-}
-
 // setupMockEngine creates a mock engine with basic expectations
 func setupMockEngine(t *testing.T) *mocks.MockEngine {
 	t.Helper()
@@ -591,16 +530,7 @@ func setupMockEngineWithServer(t *testing.T, server *httptest.Server) (*mocks.Mo
 	return mockEngine, config
 }
 
-func Test_GetSastSettingsConfig(t *testing.T) {
-	t.Run("adds organization dependency and clears cache on org change", func(t *testing.T) {
-		testOrganizationDependency(
-			t,
-			code_workflow.ConfigurationSastSettings,
-			configuration.ORGANIZATION,
-			false,
-		)
-	})
-
+func Test_getSastSettingsConfig(t *testing.T) {
 	t.Run("callback returns existing value when provided", func(t *testing.T) {
 		existingValue := &sast_contract.SastResponse{SastEnabled: true}
 
@@ -625,6 +555,28 @@ func Test_GetSastSettingsConfig(t *testing.T) {
 		assert.True(t, sastResponse.LocalCodeEngine.Enabled)
 	})
 
+	t.Run("adds organization dependency and clears cache on org change", func(t *testing.T) {
+		testutils.CheckCacheRespectOrgDependency(
+			t,
+			code_workflow.ConfigurationSastSettings,
+			func(isFirstCall bool) any {
+				return &sast_contract.SastResponse{
+					SastEnabled:     isFirstCall,
+					LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: isFirstCall},
+				}
+			},
+			InitCodeWorkflow,
+			&sast_contract.SastResponse{
+				SastEnabled:     true,
+				LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: true},
+			},
+			&sast_contract.SastResponse{
+				SastEnabled:     false,
+				LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: false},
+			},
+		)
+	})
+
 	t.Run("callback returns error when API call fails and existing value is nil", func(t *testing.T) {
 		server := setupMockServerWithError(t)
 		defer server.Close()
@@ -637,16 +589,7 @@ func Test_GetSastSettingsConfig(t *testing.T) {
 	})
 }
 
-func Test_GetSastEnabled(t *testing.T) {
-	t.Run("adds organization dependency and clears cache on org change", func(t *testing.T) {
-		testOrganizationDependency(
-			t,
-			code_workflow.ConfigurationSastEnabled,
-			configuration.ORGANIZATION,
-			true,
-		)
-	})
-
+func Test_getSastEnabled(t *testing.T) {
 	t.Run("callback function returns existing value when provided", func(t *testing.T) {
 		mockEngine := setupMockEngine(t)
 		result, err := getSastEnabled(mockEngine)(mockEngine.GetConfiguration(), true)
@@ -656,17 +599,57 @@ func Test_GetSastEnabled(t *testing.T) {
 		assert.True(t, boolResult, "Should return existing value when provided")
 	})
 
-	t.Run("callback fetches settings when existing value is nil", func(t *testing.T) {
-		server := setupMockServerForSastSettings(t, true, false)
-		defer server.Close()
+	t.Run("callback reads from ConfigurationSastSettings (pre-cached) when existing value is nil", func(t *testing.T) {
+		mockEngine := setupMockEngine(t)
+		config := mockEngine.GetConfiguration()
 
-		mockEngine, config := setupMockEngineWithServer(t, server)
+		// Set ConfigurationSastSettings in config
+		sastSettings := &sast_contract.SastResponse{
+			SastEnabled:     true,
+			LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: false},
+		}
+		config.Set(code_workflow.ConfigurationSastSettings, sastSettings)
 
 		result, err := getSastEnabled(mockEngine)(config, nil)
 		assert.NoError(t, err)
 		boolResult, ok := result.(bool)
 		assert.True(t, ok, "result should be of type bool")
-		assert.True(t, boolResult, "Should return SastEnabled from API response")
+		assert.True(t, boolResult, "Should return SastEnabled from ConfigurationSastSettings")
+	})
+
+	t.Run("depends on ConfigurationSastSettings", func(t *testing.T) {
+		testutils.CheckConfigCachesDependency(
+			t,
+			code_workflow.ConfigurationSastEnabled,
+			code_workflow.ConfigurationSastSettings,
+			getSastEnabled,
+			&sast_contract.SastResponse{
+				SastEnabled:     true,
+				LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: false},
+			},
+			&sast_contract.SastResponse{
+				SastEnabled:     false,
+				LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: false},
+			},
+			true,
+			false,
+		)
+	})
+
+	t.Run("respects organization changes (full chain)", func(t *testing.T) {
+		testutils.CheckCacheRespectOrgDependency(
+			t,
+			code_workflow.ConfigurationSastEnabled,
+			func(isFirstCall bool) any {
+				return &sast_contract.SastResponse{
+					SastEnabled:     isFirstCall,
+					LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: false},
+				}
+			},
+			InitCodeWorkflow,
+			true,
+			false,
+		)
 	})
 
 	t.Run("callback returns false and error when API call fails and existing value is nil", func(t *testing.T) {
@@ -683,16 +666,7 @@ func Test_GetSastEnabled(t *testing.T) {
 	})
 }
 
-func Test_GetSlceEnabled(t *testing.T) {
-	t.Run("adds organization dependency and clears cache on org change", func(t *testing.T) {
-		testOrganizationDependency(
-			t,
-			code_workflow.ConfigurarionSlceEnabled,
-			configuration.ORGANIZATION,
-			true,
-		)
-	})
-
+func Test_getSlceEnabled(t *testing.T) {
 	t.Run("callback function returns existing value when provided", func(t *testing.T) {
 		mockEngine := setupMockEngine(t)
 		result, err := getSlceEnabled(mockEngine)(mockEngine.GetConfiguration(), true)
@@ -702,17 +676,57 @@ func Test_GetSlceEnabled(t *testing.T) {
 		assert.True(t, boolResult, "Should return existing value when provided")
 	})
 
-	t.Run("callback fetches settings when existing value is nil", func(t *testing.T) {
-		server := setupMockServerForSastSettings(t, false, true)
-		defer server.Close()
+	t.Run("callback reads from ConfigurationSastSettings (pre-cached) when existing value is nil", func(t *testing.T) {
+		mockEngine := setupMockEngine(t)
+		config := mockEngine.GetConfiguration()
 
-		mockEngine, config := setupMockEngineWithServer(t, server)
+		// Set ConfigurationSastSettings in config
+		sastSettings := &sast_contract.SastResponse{
+			SastEnabled:     false,
+			LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: true},
+		}
+		config.Set(code_workflow.ConfigurationSastSettings, sastSettings)
 
 		result, err := getSlceEnabled(mockEngine)(config, nil)
 		assert.NoError(t, err)
 		boolResult, ok := result.(bool)
 		assert.True(t, ok, "result should be of type bool")
-		assert.True(t, boolResult, "Should return LocalCodeEngine.Enabled from API response")
+		assert.True(t, boolResult, "Should return LocalCodeEngine.Enabled from ConfigurationSastSettings")
+	})
+
+	t.Run("depends on ConfigurationSastSettings", func(t *testing.T) {
+		testutils.CheckConfigCachesDependency(
+			t,
+			code_workflow.ConfigurarionSlceEnabled,
+			code_workflow.ConfigurationSastSettings,
+			getSlceEnabled,
+			&sast_contract.SastResponse{
+				SastEnabled:     false,
+				LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: true},
+			},
+			&sast_contract.SastResponse{
+				SastEnabled:     false,
+				LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: false},
+			},
+			true,
+			false,
+		)
+	})
+
+	t.Run("respects organization changes (full chain)", func(t *testing.T) {
+		testutils.CheckCacheRespectOrgDependency(
+			t,
+			code_workflow.ConfigurarionSlceEnabled,
+			func(isFirstCall bool) any {
+				return &sast_contract.SastResponse{
+					SastEnabled:     false,
+					LocalCodeEngine: sast_contract.LocalCodeEngine{Enabled: isFirstCall},
+				}
+			},
+			InitCodeWorkflow,
+			true,
+			false,
+		)
 	})
 
 	t.Run("callback returns false and error when API call fails and existing value is nil", func(t *testing.T) {