From 7f3ea453874655ef0c26cac3d419b5a300f4b174 Mon Sep 17 00:00:00 2001 From: Ben Durrans Date: Fri, 10 Apr 2026 15:06:07 +0100 Subject: [PATCH] refactor: remove no-op path validation This has allowed removing a lot of error paths. Including `BatchUpdateFolderConfigs` which does nothing but log. `enrichFromGit` also did not need to return the `FolderConfig` it took in. --- application/config/config.go | 31 +-- application/server/configuration.go | 31 +-- application/server/configuration_test.go | 10 +- domain/ide/command/folder_handler.go | 13 +- domain/ide/command/ldx_sync_service.go | 12 +- internal/folderconfig/cross_platform_test.go | 5 +- internal/folderconfig/folder_config.go | 33 +-- internal/folderconfig/folder_config_test.go | 277 +------------------ internal/folderconfig/git.go | 8 +- internal/folderconfig/git_test.go | 34 ++- internal/folderconfig/xdg.go | 25 +- internal/folderconfig/xdg_test.go | 14 +- internal/types/config_resolver_test.go | 63 +++++ internal/types/path.go | 15 - internal/vcs/git_utils.go | 7 +- 15 files changed, 150 insertions(+), 428 deletions(-) diff --git a/application/config/config.go b/application/config/config.go index 998b77b34..884f046b8 100644 --- a/application/config/config.go +++ b/application/config/config.go @@ -180,29 +180,22 @@ func ParseOAuthToken(token string, logger *zerolog.Logger) (oauth2.Token, error) return *oauthToken, nil } -// GetFolderConfigFromEngine retrieves or creates a folder config and attaches the engine and resolver. +// GetFolderConfigFromEngine creates a folder config and attaches the engine and resolver. Enriches from Git. func GetFolderConfigFromEngine(engine workflow.Engine, resolver types.ConfigResolverInterface, path types.FilePath, logger *zerolog.Logger) *types.FolderConfig { conf := engine.GetConfiguration() - folderConfig, err := folderconfig.GetOrCreateFolderConfig(conf, path, logger) - if err != nil { - logger.Err(err).Msg("unable to get or create folder config") - folderConfig = &types.FolderConfig{FolderPath: path} - } + folderConfig := folderconfig.GetFolderConfigWithOptions(conf, path, logger, folderconfig.GetFolderConfigOptions{ + EnrichFromGit: true, + }) wireConfigResolver(folderConfig, engine, resolver) return folderConfig } -// GetUnenrichedFolderConfigFromEngine returns a read-only folder config without writing to storage. +// GetUnenrichedFolderConfigFromEngine creates a folder config and attaches the engine and resolver. Does not enrich from Git. func GetUnenrichedFolderConfigFromEngine(engine workflow.Engine, resolver types.ConfigResolverInterface, path types.FilePath, logger *zerolog.Logger) *types.FolderConfig { conf := engine.GetConfiguration() - folderConfig, err := folderconfig.GetFolderConfigWithOptions(conf, path, logger, folderconfig.GetFolderConfigOptions{ - CreateIfNotExist: true, - EnrichFromGit: false, + folderConfig := folderconfig.GetFolderConfigWithOptions(conf, path, logger, folderconfig.GetFolderConfigOptions{ + EnrichFromGit: false, }) - if err != nil { - logger.Err(err).Msg("unable to get or create folder config") - folderConfig = &types.FolderConfig{FolderPath: path} - } wireConfigResolver(folderConfig, engine, resolver) return folderConfig } @@ -730,15 +723,9 @@ func FolderOrganization(conf configuration.Configuration, path types.FilePath, l return globalOrg } - fc, err := folderconfig.GetFolderConfigWithOptions(conf, path, logger, folderconfig.GetFolderConfigOptions{ - CreateIfNotExist: false, - EnrichFromGit: false, + fc := folderconfig.GetFolderConfigWithOptions(conf, path, logger, folderconfig.GetFolderConfigOptions{ + EnrichFromGit: false, }) - if err != nil { - globalOrg := types.GetGlobalOrganization(conf) - ctxLogger.Warn().Err(err).Str("globalOrg", globalOrg).Msg("error getting folder config, falling back to global organization") - return globalOrg - } fcConf := fc.Conf() if fcConf == nil { diff --git a/application/server/configuration.go b/application/server/configuration.go index cff29dd0c..c11585ae2 100644 --- a/application/server/configuration.go +++ b/application/server/configuration.go @@ -30,19 +30,18 @@ import ( "github.com/creachadair/jrpc2" "github.com/creachadair/jrpc2/handler" "github.com/rs/zerolog" + sglsp "github.com/sourcegraph/go-lsp" + "github.com/snyk/go-application-framework/pkg/configuration" "github.com/snyk/go-application-framework/pkg/configuration/configresolver" "github.com/snyk/go-application-framework/pkg/workflow" - sglsp "github.com/sourcegraph/go-lsp" - - "github.com/snyk/snyk-ls/internal/folderconfig" - mcpWorkflow "github.com/snyk/snyk-ls/internal/mcp" "github.com/snyk/snyk-ls/application/config" "github.com/snyk/snyk-ls/application/di" "github.com/snyk/snyk-ls/domain/ide/command" "github.com/snyk/snyk-ls/infrastructure/analytics" ctx2 "github.com/snyk/snyk-ls/internal/context" + mcpWorkflow "github.com/snyk/snyk-ls/internal/mcp" "github.com/snyk/snyk-ls/internal/notification" "github.com/snyk/snyk-ls/internal/product" "github.com/snyk/snyk-ls/internal/types" @@ -290,27 +289,16 @@ func processFolderConfigs(conf configuration.Configuration, engine workflow.Engi Msg("processFolderConfigs - processing folder configs") var processedConfigs []types.FolderConfig - var changedConfigs []*types.FolderConfig // Always notify when the client explicitly sends folder configs — it expects the resolved state back. needsToSendUpdateToClient := len(incomingMap) > 0 for path := range allPaths { - folderConfig, oldSnapshot, newSnapshot, configChanged := processSingleLspFolderConfig(conf, engine, logger, path, incomingMap, notifier) - - if configChanged { - changedConfigs = append(changedConfigs, &folderConfig) - } + folderConfig, oldSnapshot, newSnapshot := processSingleLspFolderConfig(conf, engine, logger, path, incomingMap, notifier) handleFolderCacheClearing(conf, engine, logger, path, oldSnapshot, newSnapshot, triggerSource, configResolver) processedConfigs = append(processedConfigs, folderConfig) } - if len(changedConfigs) > 0 { - if err := folderconfig.BatchUpdateFolderConfigs(conf, changedConfigs, logger); err != nil { - logger.Err(err).Int("count", len(changedConfigs)).Msg("failed to batch update folder configs") - } - } - sendFolderConfigUpdateIfNeeded(conf, engine, logger, notifier, processedConfigs, needsToSendUpdateToClient, triggerSource, configResolver) } @@ -859,8 +847,8 @@ func gatherAllFolderPathsFromLspConfigs(incomingMap map[types.FilePath]types.Lsp // - For *LocalConfigField: nil = don't change, Changed+Value = set, Changed+nil = reset // It loads the existing FolderConfig (unenriched), applies the LspFolderConfig updates, and returns // the processed config without persisting. The caller is responsible for batch-persisting all changes. -// Returns: (processedConfig, oldSnapshot, newSnapshot, configChanged) -func processSingleLspFolderConfig(conf configuration.Configuration, engine workflow.Engine, logger *zerolog.Logger, path types.FilePath, incomingMap map[types.FilePath]types.LspFolderConfig, notifier notification.Notifier) (types.FolderConfig, types.FolderConfigSnapshot, types.FolderConfigSnapshot, bool) { +// Returns: (processedConfig, oldSnapshot, newSnapshot) +func processSingleLspFolderConfig(conf configuration.Configuration, engine workflow.Engine, logger *zerolog.Logger, path types.FilePath, incomingMap map[types.FilePath]types.LspFolderConfig, notifier notification.Notifier) (types.FolderConfig, types.FolderConfigSnapshot, types.FolderConfigSnapshot) { subLogger := logger.With().Str("method", "processSingleLspFolderConfig").Str("path", string(path)).Logger() resolver := di.ConfigResolver() fc := config.GetUnenrichedFolderConfigFromEngine(engine, resolver, path, logger) @@ -870,7 +858,6 @@ func processSingleLspFolderConfig(conf configuration.Configuration, engine workf // Validate that the changes are allowed, then apply the new config. normalizedPath := fc.FolderPath - applyChanged := false if incoming, hasIncoming := incomingMap[normalizedPath]; hasIncoming { hasLockedFieldRejections := validateLockedFields(conf, fc, &incoming, &subLogger) if hasLockedFieldRejections { @@ -879,16 +866,16 @@ func processSingleLspFolderConfig(conf configuration.Configuration, engine workf fmt.Sprintf("Failed to update %s: Some settings are locked by your organization's policy", folderName)) } - applyChanged = fc.ApplyLspUpdate(&incoming) + // Apply the update and don't care if it has changed or not. + _ = fc.ApplyLspUpdate(&incoming) } updateFolderOrgIfNeeded(conf, engine, logger, fc, fc, oldSnapshot, notifier) di.FeatureFlagService().PopulateFolderConfig(fc) newSnapshot := types.ReadFolderConfigSnapshot(conf, normalizedPath) - configChanged := applyChanged - return *fc, oldSnapshot, newSnapshot, configChanged + return *fc, oldSnapshot, newSnapshot } // validateLockedFields checks if any fields in the incoming LspFolderConfig are locked by LDX-Sync. diff --git a/application/server/configuration_test.go b/application/server/configuration_test.go index 4c6947c2b..42da97216 100644 --- a/application/server/configuration_test.go +++ b/application/server/configuration_test.go @@ -722,8 +722,9 @@ func (s *folderConfigTestSetup) createStoredConfig(org string, userSet bool) { } func (s *folderConfigTestSetup) getUpdatedConfig() *types.FolderConfig { - updatedConfig, err := folderconfig.GetOrCreateFolderConfig(s.engineConfig, s.folderPath, s.logger) - require.NoError(s.t, err) + updatedConfig := folderconfig.GetFolderConfigWithOptions(s.engineConfig, s.folderPath, s.logger, folderconfig.GetFolderConfigOptions{ + EnrichFromGit: false, + }) updatedConfig.ConfigResolver = types.NewMinimalConfigResolver(s.engineConfig) return updatedConfig } @@ -1150,8 +1151,9 @@ func Test_updateFolderConfig_Unauthenticated_UserSetsPreferredOrg(t *testing.T) } UpdateSettings(engine.GetConfiguration(), engine, engine.GetLogger(), nil, folderConfigs, analytics.TriggerSourceTest, testutil.DefaultConfigResolver(engine)) - updatedConfig, err := folderconfig.GetOrCreateFolderConfig(engineConfig, folderPath, engine.GetLogger()) - require.NoError(t, err) + updatedConfig := folderconfig.GetFolderConfigWithOptions(engineConfig, folderPath, engine.GetLogger(), folderconfig.GetFolderConfigOptions{ + EnrichFromGit: false, + }) updatedConfig.ConfigResolver = types.NewMinimalConfigResolver(engineConfig) assert.Equal(t, "user-chosen-org", updatedConfig.PreferredOrg(), "PreferredOrg should be set") assert.True(t, updatedConfig.OrgSetByUser(), "OrgSetByUser should be true when user chose org") diff --git a/domain/ide/command/folder_handler.go b/domain/ide/command/folder_handler.go index c8a5fcf7c..2d19cac52 100644 --- a/domain/ide/command/folder_handler.go +++ b/domain/ide/command/folder_handler.go @@ -116,16 +116,9 @@ func buildLspFolderConfigs(conf configuration.Configuration, engine workflow.Eng var lspFolderConfigs []types.LspFolderConfig for _, folder := range ws.Folders() { - fc, err := folderconfig.GetOrCreateFolderConfig(engineConfig, folder.Path(), &log) - if err != nil { - log.Err(err).Msg("unable to load folderConfig") - continue - } - - if fc == nil { - log.Warn().Str("path", string(folder.Path())).Msg("folder config is nil, skipping") - continue - } + fc := folderconfig.GetFolderConfigWithOptions(engineConfig, folder.Path(), &log, folderconfig.GetFolderConfigOptions{ + EnrichFromGit: true, + }) folderConfig := fc.Clone() if featureFlagService != nil { diff --git a/domain/ide/command/ldx_sync_service.go b/domain/ide/command/ldx_sync_service.go index c6d6c0621..348dc85c4 100644 --- a/domain/ide/command/ldx_sync_service.go +++ b/domain/ide/command/ldx_sync_service.go @@ -107,15 +107,11 @@ func (s *DefaultLdxSyncService) RefreshConfigFromLdxSync(ctx context.Context, co } // Get PreferredOrg from folder config (or empty string if missing) - folderConfig, err := folderconfig.GetFolderConfigWithOptions(prefixKeyConfig, f.Path(), &log, folderconfig.GetFolderConfigOptions{ - CreateIfNotExist: false, - EnrichFromGit: false, + folderConfig := folderconfig.GetFolderConfigWithOptions(prefixKeyConfig, f.Path(), &log, folderconfig.GetFolderConfigOptions{ + EnrichFromGit: false, }) - preferredOrg := "" - if err == nil && folderConfig != nil { - snapshot := types.ReadFolderConfigSnapshot(prefixKeyConfig, folderConfig.FolderPath) - preferredOrg = snapshot.PreferredOrg - } + snapshot := types.ReadFolderConfigSnapshot(prefixKeyConfig, folderConfig.FolderPath) + preferredOrg := snapshot.PreferredOrg log.Debug(). Str("projectPath", string(f.Path())). diff --git a/internal/folderconfig/cross_platform_test.go b/internal/folderconfig/cross_platform_test.go index 794d989d4..7ba93dff9 100644 --- a/internal/folderconfig/cross_platform_test.go +++ b/internal/folderconfig/cross_platform_test.go @@ -25,7 +25,7 @@ import ( "github.com/snyk/snyk-ls/internal/types" ) -func Test_GetOrCreateFolderConfig_CrossPlatformPaths(t *testing.T) { +func Test_GetFolderConfigWithOptions_CrossPlatformPaths(t *testing.T) { // Create one temporary directory for testing tempDir := t.TempDir() @@ -76,10 +76,9 @@ func Test_GetOrCreateFolderConfig_CrossPlatformPaths(t *testing.T) { logger := zerolog.New(zerolog.NewTestWriter(t)) // Act - folderConfig, err := GetOrCreateFolderConfig(conf, tt.inputPath, &logger) + folderConfig := GetFolderConfigWithOptions(conf, tt.inputPath, &logger, GetFolderConfigOptions{}) // Assert - require.NoError(t, err) require.NotNil(t, folderConfig) // Calculate the expected normalized path for this specific input diff --git a/internal/folderconfig/folder_config.go b/internal/folderconfig/folder_config.go index 9131af5a5..c5762a97d 100644 --- a/internal/folderconfig/folder_config.go +++ b/internal/folderconfig/folder_config.go @@ -28,44 +28,23 @@ import ( // GetFolderConfigOptions controls the behavior of folder config retrieval type GetFolderConfigOptions struct { - // CreateIfNotExist creates a new folder config if one doesn't exist. - // When ReadOnly=false: creates and saves to storage. - // When ReadOnly=true: creates in-memory but doesn't save. - CreateIfNotExist bool - // EnrichFromGit enriches the folder config with Git branch information. EnrichFromGit bool } // GetFolderConfigWithOptions retrieves folder config from storage with specified behaviors -func GetFolderConfigWithOptions(conf configuration.Configuration, path types.FilePath, logger *zerolog.Logger, opts GetFolderConfigOptions) (*types.FolderConfig, error) { +func GetFolderConfigWithOptions(conf configuration.Configuration, path types.FilePath, logger *zerolog.Logger, opts GetFolderConfigOptions) *types.FolderConfig { l := logger.With().Str("method", "GetFolderConfigWithOptions").Logger() - folderConfig, err := newFolderConfig(path, &l) - if err != nil { - return nil, err - } - - // If folder config doesn't exist and we're not creating, return nil - if folderConfig == nil && !opts.CreateIfNotExist { - return nil, nil - } + normalizedPath := types.PathKey(path) + folderConfig := &types.FolderConfig{FolderPath: normalizedPath} // Enrich from git if requested - if opts.EnrichFromGit && folderConfig != nil { - folderConfig = enrichFromGit(conf, &l, folderConfig) + if opts.EnrichFromGit { + enrichFromGit(conf, &l, folderConfig) } - return folderConfig, nil -} - -// GetOrCreateFolderConfig gets folder config from storage and merges it with Git data. -// Creates the config if it doesn't exist and writes back to storage. -func GetOrCreateFolderConfig(conf configuration.Configuration, path types.FilePath, logger *zerolog.Logger) (*types.FolderConfig, error) { - return GetFolderConfigWithOptions(conf, path, logger, GetFolderConfigOptions{ - CreateIfNotExist: true, - EnrichFromGit: true, - }) + return folderConfig } // SliceContainsParam checks if the parameter name is equal by splitting the given diff --git a/internal/folderconfig/folder_config_test.go b/internal/folderconfig/folder_config_test.go index 8b762590e..86d02c160 100644 --- a/internal/folderconfig/folder_config_test.go +++ b/internal/folderconfig/folder_config_test.go @@ -18,7 +18,6 @@ package folderconfig import ( "os" - "os/exec" "path/filepath" "testing" @@ -27,49 +26,12 @@ import ( "github.com/stretchr/testify/require" "github.com/snyk/go-application-framework/pkg/configuration" - "github.com/snyk/go-application-framework/pkg/configuration/configresolver" - "github.com/snyk/snyk-ls/internal/product" "github.com/snyk/snyk-ls/internal/storage" "github.com/snyk/snyk-ls/internal/types" ) -func Test_GetOrCreateFolderConfig_shouldStoreEverythingInStorageFile(t *testing.T) { - conf, storageFile := SetupConfigurationWithStorage(t) - path := types.FilePath(t.TempDir()) - dir, err := os.UserHomeDir() - require.NoError(t, err) - - nop := zerolog.Nop() - - // act - _, err = GetOrCreateFolderConfig(conf, path, &nop) - require.NoError(t, err) - types.SetFolderUserSetting(conf, path, types.SettingReferenceFolder, dir) - - // verify - expect normalized paths - expectedPath := types.PathKey(path) - expectedReferencePath := types.PathKey(types.FilePath(dir)) - - // Verify config is stored in configuration with normalized path - updatedConfig, err := GetOrCreateFolderConfig(conf, path, &nop) - require.NoError(t, err) - require.Equal(t, expectedPath, updatedConfig.FolderPath) - - // Read reference folder directly from configuration - snap := types.ReadFolderConfigSnapshot(conf, path) - require.Equal(t, expectedReferencePath, snap.ReferenceFolderPath) - - // Verify folder is present in configuration - require.True(t, isFolderPersisted(conf, expectedPath)) - - // Verify storage file has content - bytes, err := os.ReadFile(storageFile) - require.NoError(t, err) - require.Greater(t, len(bytes), 0) -} - -func Test_GetOrCreateFolderConfig_shouldIntegrateGitBranchInformation(t *testing.T) { +func Test_GetFolderConfigWithOptions_shouldIntegrateGitBranchInformation(t *testing.T) { dir := types.FilePath(t.TempDir()) logger := zerolog.New(zerolog.NewTestWriter(t)) repo, err := SetupCustomTestRepo(t, dir, "https://github.com/snyk-labs/nodejs-goof", "", &logger, false) @@ -78,55 +40,14 @@ func Test_GetOrCreateFolderConfig_shouldIntegrateGitBranchInformation(t *testing conf, _ := SetupConfigurationWithStorage(t) // Act - actual, err := GetOrCreateFolderConfig(conf, repo, &logger) - require.NoError(t, err) + actual := GetFolderConfigWithOptions(conf, repo, &logger, GetFolderConfigOptions{EnrichFromGit: true}) // Verify we got branches (from configuration) snap := types.ReadFolderConfigSnapshot(conf, actual.FolderPath) require.Greater(t, len(snap.LocalBranches), 0) } -func Test_GetOrCreateFolderConfig_shouldReturnExistingFolderConfig(t *testing.T) { - conf, _ := SetupConfigurationWithStorage(t) - path := types.FilePath(t.TempDir()) - scanCommandConfig := types.ScanCommandConfig{ - PreScanCommand: "/a", - PreScanOnlyReferenceFolder: false, - PostScanCommand: "/b", - PostScanOnlyReferenceFolder: false, - } - referenceDir := t.TempDir() - - logger := zerolog.New(zerolog.NewTestWriter(t)) - fp := string(types.PathKey(path)) - conf.PersistInStorage(configresolver.UserFolderKey(fp, types.SettingReferenceFolder)) - conf.PersistInStorage(configresolver.UserFolderKey(fp, types.SettingAdditionalParameters)) - conf.PersistInStorage(configresolver.FolderMetadataKey(fp, types.SettingLocalBranches)) - conf.PersistInStorage(configresolver.UserFolderKey(fp, types.SettingBaseBranch)) - conf.PersistInStorage(configresolver.UserFolderKey(fp, types.SettingReferenceBranch)) - conf.PersistInStorage(configresolver.UserFolderKey(fp, types.SettingScanCommandConfig)) - conf.Set(configresolver.UserFolderKey(fp, types.SettingReferenceFolder), &configresolver.LocalConfigField{Value: referenceDir, Changed: true}) - conf.Set(configresolver.UserFolderKey(fp, types.SettingAdditionalParameters), &configresolver.LocalConfigField{Value: []string{"--additional-param=asdf", "--additional-param2=add"}, Changed: true}) - conf.Set(configresolver.FolderMetadataKey(fp, types.SettingLocalBranches), []string{"main", "dev"}) - conf.Set(configresolver.UserFolderKey(fp, types.SettingBaseBranch), &configresolver.LocalConfigField{Value: "main", Changed: true}) - conf.Set(configresolver.UserFolderKey(fp, types.SettingReferenceBranch), &configresolver.LocalConfigField{Value: "main", Changed: true}) - conf.Set(configresolver.UserFolderKey(fp, types.SettingScanCommandConfig), &configresolver.LocalConfigField{Value: map[product.Product]types.ScanCommandConfig{product.ProductOpenSource: scanCommandConfig}, Changed: true}) - - // Act - actual, err := GetOrCreateFolderConfig(conf, path, &logger) - require.NoError(t, err) - - // Verify the folderConfig is what we tried to write. - require.Equal(t, types.PathKey(path), actual.FolderPath) - snap := types.ReadFolderConfigSnapshot(conf, path) - require.Equal(t, types.PathKey(types.FilePath(referenceDir)), snap.ReferenceFolderPath) - assert.Equal(t, []string{"--additional-param=asdf", "--additional-param2=add"}, snap.AdditionalParameters) - assert.ElementsMatch(t, []string{"main", "dev"}, snap.LocalBranches) - assert.Equal(t, "main", snap.BaseBranch) - assert.Equal(t, scanCommandConfig, snap.ScanCommandConfig[product.ProductOpenSource]) -} - -func Test_GetOrCreateFolderConfig_shouldReturnLocalBranchesEvenWithoutBaseBranch(t *testing.T) { +func Test_GetFolderConfigWithOptions_shouldReturnLocalBranchesEvenWithoutBaseBranch(t *testing.T) { // Create a temporary test Git repository with an initial commit and branches branches := []string{"feature-branch", "develop"} tempDir := t.TempDir() @@ -136,11 +57,10 @@ func Test_GetOrCreateFolderConfig_shouldReturnLocalBranchesEvenWithoutBaseBranch logger := zerolog.New(zerolog.NewTestWriter(t)) - // Test GetOrCreateFolderConfig - folderConfig, err := GetOrCreateFolderConfig(conf, types.FilePath(tempDir), &logger) + // Act + folderConfig := GetFolderConfigWithOptions(conf, types.FilePath(tempDir), &logger, GetFolderConfigOptions{EnrichFromGit: true}) // Should not return an error - require.NoError(t, err) require.NotNil(t, folderConfig) // Should have local branches from Git (stored in configuration) @@ -151,193 +71,6 @@ func Test_GetOrCreateFolderConfig_shouldReturnLocalBranchesEvenWithoutBaseBranch assert.Empty(t, snap.BaseBranch) } -func Test_GetOrCreateFolderConfig_GitLocalBranchesTakePriorityOverStoredConfig(t *testing.T) { - // Create a temporary test Git repository with an initial commit and branches - tempDir := t.TempDir() - gitBranches := []string{"main", "git-feature", "git-develop"} - initializeTestGitRepo(t, tempDir, gitBranches) - - // Create a folderConfig with outdated branch info - conf, _ := SetupConfigurationWithStorage(t) - fc := &types.FolderConfig{FolderPath: types.FilePath(tempDir)} - fp := string(types.PathKey(fc.FolderPath)) - conf.Set(configresolver.FolderMetadataKey(fp, types.SettingLocalBranches), []string{"old-main", "old-feature"}) - conf.Set(configresolver.UserFolderKey(fp, types.SettingBaseBranch), &configresolver.LocalConfigField{Value: "old-main", Changed: true}) - logger := zerolog.New(zerolog.NewTestWriter(t)) - - // Act - folderConfig, err := GetOrCreateFolderConfig(conf, types.FilePath(tempDir), &logger) - require.NoError(t, err) - require.NotNil(t, folderConfig) - - // Git local branches should take priority - we should get fresh branches from Git - snap := types.ReadFolderConfigSnapshot(conf, folderConfig.FolderPath) - assert.ElementsMatch(t, gitBranches, snap.LocalBranches) -} - -func Test_GetOrCreateFolderConfig_StoredConfigBaseBranchNotOverwrittenByGit(t *testing.T) { - // Create a temporary test Git repository with an initial commit and main branch - tempDir := t.TempDir() - initializeTestGitRepo(t, tempDir, []string{"main"}) - - // Set a specific default branch in Git config - cmd := exec.Command("git", "config", "init.defaultBranch", "git-default-branch") - cmd.Dir = tempDir - err := cmd.Run() - require.NoError(t, err) - - // Create folderConfig with a different base branch than Git default - conf, _ := SetupConfigurationWithStorage(t) - storedBaseBranch := "some-stored-base-branch" - fc := &types.FolderConfig{FolderPath: types.FilePath(tempDir)} - fp := string(types.PathKey(fc.FolderPath)) - conf.Set(configresolver.UserFolderKey(fp, types.SettingBaseBranch), &configresolver.LocalConfigField{Value: storedBaseBranch, Changed: true}) - conf.Set(configresolver.UserFolderKey(fp, types.SettingReferenceBranch), &configresolver.LocalConfigField{Value: storedBaseBranch, Changed: true}) - logger := zerolog.New(zerolog.NewTestWriter(t)) - - // Act - folderConfig, err := GetOrCreateFolderConfig(conf, types.FilePath(tempDir), &logger) - require.NoError(t, err) - require.NotNil(t, folderConfig) - - // Stored config base branch should be preserved, not overwritten by Git default - bbSnap := types.ReadFolderConfigSnapshot(conf, types.FilePath(tempDir)) - assert.Equal(t, storedBaseBranch, bbSnap.BaseBranch) -} - -func Test_GetOrCreateFolderConfig_NewFolder(t *testing.T) { - conf, _ := SetupConfigurationWithStorage(t) - path := types.FilePath(t.TempDir()) - logger := zerolog.New(zerolog.NewTestWriter(t)) - - // Action - actual, err := GetOrCreateFolderConfig(conf, path, &logger) - - // Assert - require.NoError(t, err) - require.NotNil(t, actual) - newSnap := types.ReadFolderConfigSnapshot(conf, path) - assert.False(t, newSnap.OrgSetByUser, "Auto-org should be enabled") - assert.Empty(t, newSnap.PreferredOrg, "PreferredOrg should be empty for new folders") - assert.Empty(t, newSnap.AutoDeterminedOrg, "AutoDeterminedOrg will be set by LDX-Sync later") -} - -func Test_GetOrCreateFolderConfig_ExistingFolderWithZeroValues(t *testing.T) { - // Setup: existing folder with Go zero-values - conf, _ := SetupConfigurationWithStorage(t) - path := types.FilePath(t.TempDir()) - logger := zerolog.New(zerolog.NewTestWriter(t)) - - // Action - actual, err := GetOrCreateFolderConfig(conf, path, &logger) - - // Assert - require.NoError(t, err) - require.NotNil(t, actual) - zeroSnap := types.ReadFolderConfigSnapshot(conf, path) - assert.False(t, zeroSnap.OrgSetByUser, "OrgSetByUser should be false for zero-value config") -} - -func Test_GetOrCreateFolderConfig_ExistingFolder_PreservesValues(t *testing.T) { - conf, _ := SetupConfigurationWithStorage(t) - path := types.FilePath(t.TempDir()) - logger := zerolog.New(zerolog.NewTestWriter(t)) - - types.SetPreferredOrgAndOrgSetByUser(conf, path, "some-org-id", true) - - // Action - actual, err := GetOrCreateFolderConfig(conf, path, &logger) - - // Assert - require.NoError(t, err) - require.NotNil(t, actual) - snap := types.ReadFolderConfigSnapshot(conf, path) - assert.True(t, snap.OrgSetByUser, "Should remain unchanged") - assert.Equal(t, "some-org-id", snap.PreferredOrg, "PreferredOrg should be preserved") -} - -func Test_BatchUpdateFolderConfigs(t *testing.T) { - t.Run("updates multiple folders in a single load/save cycle", func(t *testing.T) { - conf, _ := SetupConfigurationWithStorage(t) - logger := zerolog.New(zerolog.NewTestWriter(t)) - - path1 := types.FilePath(t.TempDir()) - path2 := types.FilePath(t.TempDir()) - - configs := []*types.FolderConfig{ - {FolderPath: path1}, - {FolderPath: path2}, - } - types.SetFolderUserSetting(conf, path1, types.SettingBaseBranch, "main") - types.SetFolderUserSetting(conf, path1, types.SettingReferenceBranch, "main") - types.SetFolderUserSetting(conf, path2, types.SettingBaseBranch, "develop") - types.SetFolderUserSetting(conf, path2, types.SettingReferenceBranch, "develop") - types.SetPreferredOrgAndOrgSetByUser(conf, path2, "org-1", true) - - err := BatchUpdateFolderConfigs(conf, configs, &logger) - require.NoError(t, err) - - // Verify both were persisted - snap1 := types.ReadFolderConfigSnapshot(conf, path1) - assert.Equal(t, "main", snap1.BaseBranch) - - snap2 := types.ReadFolderConfigSnapshot(conf, path2) - assert.Equal(t, "develop", snap2.BaseBranch) - assert.Equal(t, "org-1", snap2.PreferredOrg) - }) - - t.Run("does nothing for empty slice", func(t *testing.T) { - conf, _ := SetupConfigurationWithStorage(t) - logger := zerolog.New(zerolog.NewTestWriter(t)) - - err := BatchUpdateFolderConfigs(conf, nil, &logger) - require.NoError(t, err) - - err = BatchUpdateFolderConfigs(conf, []*types.FolderConfig{}, &logger) - require.NoError(t, err) - }) - - t.Run("preserves existing folder configs not in the batch", func(t *testing.T) { - conf, _ := SetupConfigurationWithStorage(t) - logger := zerolog.New(zerolog.NewTestWriter(t)) - - // Pre-create a folder config - existingPath := types.FilePath(t.TempDir()) - types.SetFolderUserSetting(conf, existingPath, types.SettingBaseBranch, "existing-branch") - - // Batch update a different folder - newPath := types.FilePath(t.TempDir()) - newFc := &types.FolderConfig{FolderPath: newPath} - types.SetFolderUserSetting(conf, newPath, types.SettingBaseBranch, "new-branch") - err := BatchUpdateFolderConfigs(conf, []*types.FolderConfig{newFc}, &logger) - require.NoError(t, err) - - // Verify existing config is preserved - existingSnap := types.ReadFolderConfigSnapshot(conf, existingPath) - assert.Equal(t, "existing-branch", existingSnap.BaseBranch) - - // Verify new config was added - newSnap := types.ReadFolderConfigSnapshot(conf, newPath) - assert.Equal(t, "new-branch", newSnap.BaseBranch) - }) -} - -// isFolderPersisted checks if any well-known config key exists for the folder (for test verification). -func isFolderPersisted(conf configuration.Configuration, path types.FilePath) bool { - fp := string(types.PathKey(path)) - keys := []string{ - configresolver.UserFolderKey(fp, types.SettingBaseBranch), - configresolver.UserFolderKey(fp, types.SettingReferenceFolder), - configresolver.FolderMetadataKey(fp, types.SettingLocalBranches), - } - for _, k := range keys { - if conf.Get(k) != nil { - return true - } - } - return false -} - func SetupConfigurationWithStorage(t *testing.T) (configuration.Configuration, string) { t.Helper() conf := configuration.NewWithOpts(configuration.WithAutomaticEnv()) diff --git a/internal/folderconfig/git.go b/internal/folderconfig/git.go index 67df42e8a..ef0f7ea11 100644 --- a/internal/folderconfig/git.go +++ b/internal/folderconfig/git.go @@ -75,17 +75,17 @@ func getLocalBranches(repository *git.Repository) ([]string, error) { return localBranches, nil } -func enrichFromGit(conf configuration.Configuration, logger *zerolog.Logger, folderConfig *types.FolderConfig) *types.FolderConfig { +func enrichFromGit(conf configuration.Configuration, logger *zerolog.Logger, folderConfig *types.FolderConfig) { l := logger.With().Str("method", "enrichFromGit").Logger() repository, err := git.PlainOpenWithOptions(string(folderConfig.FolderPath), &git.PlainOpenOptions{DetectDotGit: true}) if err != nil { - return folderConfig // Probably not a git repo and that's okay + return // Probably not a git repo and that's okay } fp := string(types.PathKey(folderConfig.FolderPath)) if fp == "" || conf == nil { - return folderConfig + return } setUser := func(name string, val any) { key := configresolver.UserFolderKey(fp, name) @@ -124,8 +124,6 @@ func enrichFromGit(conf configuration.Configuration, logger *zerolog.Logger, fol setUser(types.SettingReferenceBranch, baseBranch) } } - - return folderConfig } func SetupCustomTestRepo(t *testing.T, rootDir types.FilePath, url string, targetCommit string, logger *zerolog.Logger, useRootDirDirectly bool) (types.FilePath, error) { diff --git a/internal/folderconfig/git_test.go b/internal/folderconfig/git_test.go index 52c9c2523..b9acf33ea 100644 --- a/internal/folderconfig/git_test.go +++ b/internal/folderconfig/git_test.go @@ -24,10 +24,12 @@ import ( "github.com/go-git/go-git/v5" "github.com/rs/zerolog" - "github.com/snyk/go-application-framework/pkg/configuration" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/snyk/go-application-framework/pkg/configuration" + "github.com/snyk/go-application-framework/pkg/configuration/configresolver" + "github.com/snyk/snyk-ls/internal/types" ) @@ -92,7 +94,7 @@ func Test_enrichFromGit_ReturnsLocalBranchesEvenWithoutMainOrMaster(t *testing.T folderConfig.ConfigResolver = types.NewMinimalConfigResolver(conf) // Act - folderConfig = enrichFromGit(conf, &logger, folderConfig) + enrichFromGit(conf, &logger, folderConfig) // Should have local branches (enrichFromGit writes to configuration) require.NotNil(t, folderConfig) @@ -162,3 +164,31 @@ func Test_getBaseBranch_FallsBackToMasterWhenMainNotPresent(t *testing.T) { // Assert we fall back to master (since it exists) and init.defaultBranch & main are not present assert.Equal(t, "master", baseBranch) } + +func Test_enrichFromGit_PreservesUserSetBaseBranch(t *testing.T) { + // Create a temporary test Git repository with an initial commit and main branch + tempDir := t.TempDir() + initializeTestGitRepo(t, tempDir, []string{"main"}) + + // Set a specific default branch in Git config + cmd := exec.Command("git", "config", "init.defaultBranch", "git-default-branch") + cmd.Dir = tempDir + err := cmd.Run() + require.NoError(t, err) + + // Create folderConfig with a different base branch than Git default + conf := configuration.NewWithOpts(configuration.WithAutomaticEnv()) + storedBaseBranch := "some-stored-base-branch" + fc := &types.FolderConfig{FolderPath: types.FilePath(tempDir)} + fp := string(types.PathKey(fc.FolderPath)) + conf.Set(configresolver.UserFolderKey(fp, types.SettingBaseBranch), &configresolver.LocalConfigField{Value: storedBaseBranch, Changed: true}) + conf.Set(configresolver.UserFolderKey(fp, types.SettingReferenceBranch), &configresolver.LocalConfigField{Value: storedBaseBranch, Changed: true}) + logger := zerolog.New(zerolog.NewTestWriter(t)) + + // Act - enrichFromGit should preserve the user-set base branch + enrichFromGit(conf, &logger, fc) + + // Assert - Stored config base branch should be preserved, not overwritten by Git default + bbSnap := types.ReadFolderConfigSnapshot(conf, types.FilePath(tempDir)) + assert.Equal(t, storedBaseBranch, bbSnap.BaseBranch) +} diff --git a/internal/folderconfig/xdg.go b/internal/folderconfig/xdg.go index fdd9f1e0e..80056ed0e 100644 --- a/internal/folderconfig/xdg.go +++ b/internal/folderconfig/xdg.go @@ -21,8 +21,6 @@ import ( "path/filepath" "github.com/adrg/xdg" - "github.com/rs/zerolog" - "github.com/snyk/go-application-framework/pkg/configuration" "github.com/snyk/snyk-ls/internal/types" ) @@ -39,26 +37,7 @@ func ConfigFile(ideName string) (string, error) { return xdg.ConfigFile(path) } -func newFolderConfig(path types.FilePath, logger *zerolog.Logger) (*types.FolderConfig, error) { - if err := types.ValidatePathForStorage(path); err != nil { - logger.Error().Err(err).Str("path", string(path)).Msg("invalid folder path") - return nil, err - } +func newFolderConfig(path types.FilePath) *types.FolderConfig { normalizedPath := types.PathKey(path) - return &types.FolderConfig{FolderPath: normalizedPath}, nil -} - -// BatchUpdateFolderConfigs validates folder configs for batch update. -func BatchUpdateFolderConfigs(conf configuration.Configuration, folderConfigs []*types.FolderConfig, logger *zerolog.Logger) error { - for _, fc := range folderConfigs { - if err := types.ValidatePathForStorage(fc.FolderPath); err != nil { - logger.Error().Err(err).Str("path", string(fc.FolderPath)).Msg("invalid folder path in batch update") - return err - } - } - - logger.Debug(). - Int("folderCount", len(folderConfigs)). - Msg("BatchUpdateFolderConfigs: validated all folders") - return nil + return &types.FolderConfig{FolderPath: normalizedPath} } diff --git a/internal/folderconfig/xdg_test.go b/internal/folderconfig/xdg_test.go index c9dada646..2d2f7881f 100644 --- a/internal/folderconfig/xdg_test.go +++ b/internal/folderconfig/xdg_test.go @@ -22,8 +22,6 @@ import ( "testing" "github.com/adrg/xdg" - "github.com/rs/zerolog" - "github.com/stretchr/testify/require" "github.com/snyk/go-application-framework/pkg/configuration" @@ -43,21 +41,18 @@ func TestConfigFile(t *testing.T) { func Test_folderConfigFromFallbackStorage_NotNilIfCreateIfNotExist(t *testing.T) { conf := configuration.NewWithOpts(configuration.WithAutomaticEnv()) - logger := zerolog.New(zerolog.NewTestWriter(t)) tempDir := t.TempDir() path := types.FilePath(tempDir) // Get the folder config from storage for a folder that doesn't exist yet and verify we get a result back var _ = conf - folderConfig, err := newFolderConfig(path, &logger) - require.NoError(t, err) + folderConfig := newFolderConfig(path) require.NotNil(t, folderConfig) } func Test_folderConfigFromFallbackStorage_NilIfDoNotCreate(t *testing.T) { conf := configuration.NewWithOpts(configuration.WithAutomaticEnv()) - logger := zerolog.New(zerolog.NewTestWriter(t)) tempDir := t.TempDir() path := types.FilePath(tempDir) @@ -65,15 +60,13 @@ func Test_folderConfigFromFallbackStorage_NilIfDoNotCreate(t *testing.T) { // With dynamic persistence, folderConfigFromStorage always returns a minimal config. // createIfNotExist is no longer used; caller handles nil via GetFolderConfigWithOptions. var _ = conf - folderConfig, err := newFolderConfig(path, &logger) - require.NoError(t, err) + folderConfig := newFolderConfig(path) require.NotNil(t, folderConfig) require.Equal(t, types.PathKey(path), folderConfig.FolderPath) } func Test_SetFolderUserSetting_PersistsUserOverrides(t *testing.T) { conf := configuration.NewWithOpts(configuration.WithAutomaticEnv()) - logger := zerolog.New(zerolog.NewTestWriter(t)) tempDir := t.TempDir() path := types.FilePath(tempDir) @@ -84,8 +77,7 @@ func Test_SetFolderUserSetting_PersistsUserOverrides(t *testing.T) { types.SetFolderUserSetting(conf, path, types.SettingRiskScoreThreshold, 800) // Retrieve the config from storage - retrievedConfig, err := newFolderConfig(path, &logger) - require.NoError(t, err) + retrievedConfig := newFolderConfig(path) require.NotNil(t, retrievedConfig) retrievedConfig.ConfigResolver = types.NewMinimalConfigResolver(conf) diff --git a/internal/types/config_resolver_test.go b/internal/types/config_resolver_test.go index dd2792cbf..cbeb4c47f 100644 --- a/internal/types/config_resolver_test.go +++ b/internal/types/config_resolver_test.go @@ -17,6 +17,7 @@ package types_test import ( + "os" "testing" "github.com/golang/mock/gomock" @@ -1933,3 +1934,65 @@ func TestInteg_IsLocked_OrgScope_FolderLevelLockedVsOrgLevel(t *testing.T) { assert.True(t, resolver.IsLocked(types.SettingScanAutomatic, fc)) }) } + +func TestInteg_SetFolderUserSetting_StoresInConfiguration(t *testing.T) { + conf := configuration.NewWithOpts(configuration.WithAutomaticEnv()) + path := types.FilePath(t.TempDir()) + dir, err := os.UserHomeDir() + require.NoError(t, err) + + // Act + types.SetFolderUserSetting(conf, path, types.SettingReferenceFolder, dir) + + // Verify expected normalized paths + expectedPath := types.PathKey(path) + expectedReferencePath := types.PathKey(types.FilePath(dir)) + + // Read reference folder directly from configuration + snap := types.ReadFolderConfigSnapshot(conf, path) + require.Equal(t, expectedReferencePath, snap.ReferenceFolderPath) + + // Verify folder is present in configuration + fp := string(expectedPath) + keys := []string{ + configresolver.UserFolderKey(fp, types.SettingBaseBranch), + configresolver.UserFolderKey(fp, types.SettingReferenceFolder), + configresolver.FolderMetadataKey(fp, types.SettingLocalBranches), + } + found := false + for _, k := range keys { + if conf.Get(k) != nil { + found = true + break + } + } + require.True(t, found, "folder should be persisted in configuration") +} + +func TestInteg_ReadFolderConfigSnapshot_NewFolderDefaults(t *testing.T) { + conf := configuration.NewWithOpts(configuration.WithAutomaticEnv()) + path := types.FilePath(t.TempDir()) + + // Act - Read snapshot for a new folder that hasn't been configured yet + newSnap := types.ReadFolderConfigSnapshot(conf, path) + + // Assert - New folders should have correct default values + assert.False(t, newSnap.OrgSetByUser, "Auto-org should be enabled for new folders") + assert.Empty(t, newSnap.PreferredOrg, "PreferredOrg should be empty for new folders") + assert.Empty(t, newSnap.AutoDeterminedOrg, "AutoDeterminedOrg will be set by LDX-Sync later") +} + +func TestInteg_ReadFolderConfigSnapshot_PreservesUserValues(t *testing.T) { + conf := configuration.NewWithOpts(configuration.WithAutomaticEnv()) + path := types.FilePath(t.TempDir()) + + // Setup - Set user-chosen org values + types.SetPreferredOrgAndOrgSetByUser(conf, path, "some-org-id", true) + + // Act - Read snapshot after setting user values + snap := types.ReadFolderConfigSnapshot(conf, path) + + // Assert - User-set values should be preserved in snapshot + assert.True(t, snap.OrgSetByUser, "OrgSetByUser should be true when user chose org") + assert.Equal(t, "some-org-id", snap.PreferredOrg, "PreferredOrg should be preserved") +} diff --git a/internal/types/path.go b/internal/types/path.go index 39beccb92..698bbed82 100644 --- a/internal/types/path.go +++ b/internal/types/path.go @@ -84,21 +84,6 @@ func ValidatePathStrict(path FilePath) error { return nil } -// ValidatePathForStorage validates a path for storage purposes without requiring the path to exist. -// This function is used when storing paths where the path may not exist yet -// (e.g., user-configured paths for future use, paths during data migration, or storage keys). -// It allows empty paths and doesn't check if the path actually exists on the filesystem. -func ValidatePathForStorage(path FilePath) error { - options := PathValidationOptions{ - AllowEmpty: true, - Existence: NoCheck, // No existence validation needed - } - if err := ValidatePath(path, options); err != nil { - return fmt.Errorf("path validation failed for '%s': %w", string(path), err) - } - return nil -} - // PathKey creates a normalized key for path storage func PathKey(p FilePath) FilePath { // Empty paths can occur during data migration from old storage formats diff --git a/internal/vcs/git_utils.go b/internal/vcs/git_utils.go index dec14fe89..72df33634 100644 --- a/internal/vcs/git_utils.go +++ b/internal/vcs/git_utils.go @@ -104,10 +104,9 @@ func hasUncommitedChanges(repo *git.Repository) bool { } func GetBaseBranchName(conf configuration.Configuration, folderPath types.FilePath, logger *zerolog.Logger) string { - _, err := folderconfig.GetOrCreateFolderConfig(conf, folderPath, logger) - if err != nil { - return "master" - } + folderconfig.GetFolderConfigWithOptions(conf, folderPath, logger, folderconfig.GetFolderConfigOptions{ + EnrichFromGit: true, + }) snapshot := types.ReadFolderConfigSnapshot(conf, folderPath) if snapshot.BaseBranch != "" { return snapshot.BaseBranch