Config: fix don't create empty dir when resolving path (#575)

* Config: fix don't create empty dir when resolving path

* refactor migration of default config file

* gracefully handle src == target on Move
* create target directory on Move, if necessary

* unify resolution of default config location

* refactor the use of flagSet to be explicit
* remove package variable
* use empty config file path setting if not set in flags
* resolve default file location the same way when showing the target and
when loading default config file

* don't migrate if target file is the same as source

* rename configfile to configFile

* add migrateConfig tests

common/file/file.go

@@ -28,11 +28,29 @@ func Write(file string, data []byte) error {
 // This must be used across the codebase for compatibility with Docker volumes
 // and Golang (fixes Invalid cross-device link when using os.Rename)
 func Move(sourcePath, destPath string) error {
+	sourceAbs, err := filepath.Abs(sourcePath)
+	if err != nil {
+		return err
+	}
+	destAbs, err := filepath.Abs(destPath)
+	if err != nil {
+		return err
+	}
+	if sourceAbs == destAbs {
+		return nil
+	}
 	inputFile, err := os.Open(sourcePath)
 	if err != nil {
 		return err
 	}
 
+	destDir := filepath.Dir(destPath)
+	if !Exists(destDir) {
+		err = os.MkdirAll(destDir, 0770)
+		if err != nil {
+			return err
+		}
+	}
 	outputFile, err := os.Create(destPath)
 	if err != nil {
 		inputFile.Close()

common/file/file_test.go

@@ -91,16 +91,25 @@ func TestMove(t *testing.T) {
 			{InFile: "*", OutFile: "gct.txt", Write: true, ErrExpected: true},
 			{InFile: "*", OutFile: "gct.txt", Write: false, ErrExpected: true},
 			{InFile: "in.txt", OutFile: "*", Write: true, ErrExpected: true},
-			{InFile: "in.txt", OutFile: "gct.txt", Write: true, ErrExpected: false},
 		}
 	default:
 		tests = []testTable{
 			{InFile: "", OutFile: "gct.txt", Write: true, ErrExpected: true},
 			{InFile: "", OutFile: "gct.txt", Write: false, ErrExpected: true},
 			{InFile: "in.txt", OutFile: "", Write: true, ErrExpected: true},
-			{InFile: "in.txt", OutFile: "gct.txt", Write: true, ErrExpected: false},
 		}
 	}
+	tests = append(tests, []testTable{
+		{InFile: "in.txt", OutFile: "gct.txt", Write: true, ErrExpected: false},
+		{InFile: "in.txt", OutFile: "non-existing/gct.txt", Write: true, ErrExpected: false},
+		{InFile: "in.txt", OutFile: "in.txt", Write: true, ErrExpected: false},
+	}...)
+
+	if Exists("non-existing") {
+		t.Error("target 'non-existing' should not exist")
+	}
+	defer os.RemoveAll("non-existing")
+	defer os.Remove("in.txt")
 
 	for x := range tests {
 		err := tester(tests[x].InFile, tests[x].OutFile, tests[x].Write)

config/config.go

@@ -1457,129 +1457,99 @@ func (c *Config) CheckConnectionMonitorConfig() {
 // Windows: %APPDATA%\GoCryptoTrader\config.json or config.dat
 // Helpful for printing application usage
 func DefaultFilePath() string {
-	f := filepath.Join(common.GetDefaultDataDir(runtime.GOOS), File)
-	if !file.Exists(f) {
-		encFile := filepath.Join(common.GetDefaultDataDir(runtime.GOOS), EncryptedFile)
-		if file.Exists(encFile) {
-			return encFile
-		}
+	foundConfig, _, err := GetFilePath("")
+	if err != nil {
+		// If there was no config file, show default location for .json
+		return filepath.Join(common.GetDefaultDataDir(runtime.GOOS), File)
 	}
-	return f
+	return foundConfig
+}
+
+// GetAndMigrateDefaultPath returns the target config file
+// migrating it from the old default location to new one,
+// if it was implicitly loaded from a default location and
+// wasn't already in the correct 'new' default location
+func GetAndMigrateDefaultPath(configFile string) (string, error) {
+	filePath, wasDefault, err := GetFilePath(configFile)
+	if err != nil {
+		return "", err
+	}
+	if wasDefault {
+		return migrateConfig(filePath, common.GetDefaultDataDir(runtime.GOOS))
+	}
+	return filePath, nil
 }
 
 // GetFilePath returns the desired config file or the default config file name
-// based on if the application is being run under test or normal mode. It will
-// also move/rename the config file under the following conditions:
-// 1) If a config file is found in the executable path directory and no explicit
-//    config path is set, plus no config is found in the GCT data dir, it will
-//    move it to the GCT data dir. If a config already exists in the GCT data
-//    dir, it will warn the user and load the config found in the GCT data dir
-// 2) If a config file in the GCT data dir has the file extension .dat but
-//    contains json data, it will rename to the file to config.json
-// 3) If a config file in the GCT data dir has the file extension .json but
-//    contains encrypted data, it will rename the file to config.dat
-func GetFilePath(configfile string) (string, error) {
+// and whether it was loaded from a default location (rather than explicitly specified)
+func GetFilePath(configfile string) (configPath string, isImplicitDefaultPath bool, err error) {
 	if configfile != "" {
-		return configfile, nil
+		return configfile, false, nil
 	}
 
 	exePath, err := common.GetExecutablePath()
 	if err != nil {
-		return "", err
+		return "", false, err
 	}
-
-	oldDirs := []string{
+	newDir := common.GetDefaultDataDir(runtime.GOOS)
+	defaultPaths := []string{
 		filepath.Join(exePath, File),
 		filepath.Join(exePath, EncryptedFile),
-	}
-
-	newDir := common.GetDefaultDataDir(runtime.GOOS)
-	err = common.CreateDir(newDir)
-	if err != nil {
-		return "", err
-	}
-	newDirs := []string{
 		filepath.Join(newDir, File),
 		filepath.Join(newDir, EncryptedFile),
 	}
 
-	// First upgrade the old dir config file if it exists to the corresponding
-	// new one
-	for x := range oldDirs {
-		if !file.Exists(oldDirs[x]) {
-			continue
-		}
-		if file.Exists(newDirs[x]) {
-			log.Warnf(log.ConfigMgr,
-				"config.json file found in root dir and gct dir; cannot overwrite, defaulting to gct dir config.json at %s",
-				newDirs[x])
-			return newDirs[x], nil
-		}
-		if filepath.Ext(oldDirs[x]) == ".json" {
-			err = file.Move(oldDirs[x], newDirs[0])
-			if err != nil {
-				return "", err
-			}
-			log.Debugf(log.ConfigMgr,
-				"Renamed old config file %s to %s\n",
-				oldDirs[x],
-				newDirs[0])
-		} else {
-			err = file.Move(oldDirs[x], newDirs[1])
-			if err != nil {
-				return "", err
-			}
-			log.Debugf(log.ConfigMgr,
-				"Renamed old config file %s to %s\n",
-				oldDirs[x],
-				newDirs[1])
+	for _, p := range defaultPaths {
+		if file.Exists(p) {
+			configfile = p
+			break
 		}
 	}
-
-	// Secondly check to see if the new config file extension is correct or not
-	for x := range newDirs {
-		if !file.Exists(newDirs[x]) {
-			continue
-		}
-
-		data, err := ioutil.ReadFile(newDirs[x])
-		if err != nil {
-			return "", err
-		}
-
-		if ConfirmECS(data) {
-			if filepath.Ext(newDirs[x]) == ".dat" {
-				return newDirs[x], nil
-			}
-
-			err = file.Move(newDirs[x], newDirs[1])
-			if err != nil {
-				return "", err
-			}
-			return newDirs[1], nil
-		}
-
-		if filepath.Ext(newDirs[x]) == ".json" {
-			return newDirs[x], nil
-		}
-
-		err = file.Move(newDirs[x], newDirs[0])
-		if err != nil {
-			return "", err
-		}
-
-		return newDirs[0], nil
+	if configfile == "" {
+		return "", false, fmt.Errorf("config.json file not found in %s, please follow README.md in root dir for config generation",
+			newDir)
 	}
 
-	return "", fmt.Errorf("config.json file not found in %s, please follow README.md in root dir for config generation",
-		newDir)
+	return configfile, true, nil
+}
+
+// migrateConfig will move the config file to the target
+// config directory as `File` or `EncryptedFile` depending on whether the config
+// is encrypted
+func migrateConfig(configFile, targetDir string) (string, error) {
+	data, err := ioutil.ReadFile(configFile)
+	if err != nil {
+		return "", err
+	}
+
+	var target string
+	if ConfirmECS(data) {
+		target = EncryptedFile
+	} else {
+		target = File
+	}
+	target = filepath.Join(targetDir, target)
+	if configFile == target {
+		return configFile, nil
+	}
+	if file.Exists(target) {
+		log.Warnf(log.ConfigMgr, "config file already found in '%s'; not overwriting, defaulting to %s", target, configFile)
+		return configFile, nil
+	}
+
+	err = file.Move(configFile, target)
+	if err != nil {
+		return "", err
+	}
+
+	return target, nil
 }
 
 // ReadConfig verifies and checks for encryption and verifies the unencrypted
 // file contains JSON.
 // Prompts for decryption key, if target file is encrypted
 func (c *Config) ReadConfig(configPath string, dryrun bool) error {
-	defaultPath, err := GetFilePath(configPath)
+	defaultPath, _, err := GetFilePath(configPath)
 	if err != nil {
 		return err
 	}
@@ -1658,7 +1628,7 @@ func (c *Config) SaveConfig(configPath string, dryrun bool) error {
 		return nil
 	}
 
-	defaultPath, err := GetFilePath(configPath)
+	defaultPath, _, err := GetFilePath(configPath)
 	if err != nil {
 		return err
 	}

config/config_test.go

@@ -1,6 +1,8 @@
 package config
 
 import (
+	"io/ioutil"
+	"os"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -28,6 +30,17 @@ const (
 	testString              = "test"
 )
 
+func TestGetNonExistentDefaultFilePathDoesNotCreateDefaultDir(t *testing.T) {
+	dir := common.GetDefaultDataDir(runtime.GOOS)
+	if file.Exists(dir) {
+		t.Skip("The default directory already exists before running the test")
+	}
+	GetFilePath("")
+	if file.Exists(dir) {
+		t.Fatalf("The target directory was created in %s", dir)
+	}
+}
+
 func TestGetCurrencyConfig(t *testing.T) {
 	cfg := GetConfig()
 	err := cfg.LoadConfig(TestFile, true)
@@ -1705,21 +1718,25 @@ func TestDefaultFilePath(t *testing.T) {
 
 func TestGetFilePath(t *testing.T) {
 	expected := "blah.json"
-	result, _ := GetFilePath("blah.json")
+	result, wasDefault, _ := GetFilePath("blah.json")
 	if result != "blah.json" {
 		t.Errorf("TestGetFilePath: expected %s got %s", expected, result)
 	}
+	if wasDefault {
+		t.Errorf("TestGetFilePath: expected non-default")
+	}
 
 	expected = DefaultFilePath()
-	result, err := GetFilePath("")
+	result, wasDefault, err := GetFilePath("")
 	if file.Exists(expected) {
 		if err != nil || result != expected {
 			t.Errorf("TestGetFilePath: expected %s got %s", expected, result)
 		}
-	} else {
-		if err == nil {
-			t.Error("Expected error when default config file does not exist")
+		if !wasDefault {
+			t.Errorf("TestGetFilePath: expected default file")
 		}
+	} else if err == nil {
+		t.Error("Expected error when default config file does not exist")
 	}
 }
 
@@ -2077,3 +2094,110 @@ func TestGetDataPath(t *testing.T) {
 		})
 	}
 }
+
+func TestMigrateConfig(t *testing.T) {
+	type args struct {
+		configFile string
+		targetDir  string
+	}
+
+	dir, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(dir)
+
+	tests := []struct {
+		name    string
+		setup   func(t *testing.T)
+		cleanup func(t *testing.T)
+		args    args
+		want    string
+		wantErr bool
+	}{
+		{
+			name: "nonexisting",
+			args: args{
+				configFile: "not-exists.json",
+			},
+			wantErr: true,
+		},
+		{
+			name: "source present, no target dir",
+			setup: func(t *testing.T) {
+				test, err := os.Create("test.json")
+				if err != nil {
+					t.Fatal(err)
+				}
+				test.Close()
+			},
+			cleanup: func(t *testing.T) {
+				os.Remove("test.json")
+			},
+			args: args{
+				configFile: "test.json",
+				targetDir:  filepath.Join(dir, "new"),
+			},
+			want:    filepath.Join(dir, "new", File),
+			wantErr: false,
+		},
+		{
+			name: "source same as target",
+			setup: func(t *testing.T) {
+				err := file.Write(filepath.Join(dir, File), nil)
+				if err != nil {
+					t.Fatal(err)
+				}
+			},
+			args: args{
+				configFile: filepath.Join(dir, File),
+				targetDir:  dir,
+			},
+			want:    filepath.Join(dir, File),
+			wantErr: false,
+		},
+		{
+			name: "source and target present",
+			setup: func(t *testing.T) {
+				err := file.Write(filepath.Join(dir, File), nil)
+				if err != nil {
+					t.Fatal(err)
+				}
+				err = file.Write(filepath.Join(dir, "src", EncryptedFile), nil)
+				if err != nil {
+					t.Fatal(err)
+				}
+			},
+			args: args{
+				configFile: filepath.Join(dir, "src", EncryptedFile),
+				targetDir:  dir,
+			},
+			want: filepath.Join(dir, "src", EncryptedFile),
+			// We only expect warning
+			wantErr: false,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.setup != nil {
+				tt.setup(t)
+			}
+			if tt.cleanup != nil {
+				defer tt.cleanup(t)
+			}
+			got, err := migrateConfig(tt.args.configFile, tt.args.targetDir)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("migrateConfig() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if got != tt.want {
+				t.Errorf("migrateConfig() = %v, want %v", got, tt.want)
+			}
+			if err == nil && !file.Exists(got) {
+				t.Errorf("migrateConfig: %v should exist", got)
+			}
+		})
+	}
+}

engine/engine.go

@@ -2,7 +2,6 @@ package engine
 
 import (
 	"errors"
-	"flag"
 	"fmt"
 	"log"
 	"path/filepath"
@@ -47,9 +46,6 @@ type Engine struct {
 // Vars for engine
 var (
 	Bot *Engine
-
-	// Stores the set flags
-	flagSet = make(map[string]bool)
 )
 
 // New starts a new engine
@@ -66,17 +62,15 @@ func New() (*Engine, error) {
 }
 
 // NewFromSettings starts a new engine based on supplied settings
-func NewFromSettings(settings *Settings) (*Engine, error) {
+func NewFromSettings(settings *Settings, flagSet map[string]bool) (*Engine, error) {
 	if settings == nil {
 		return nil, errors.New("engine: settings is nil")
 	}
-	// collect flags
-	flag.Visit(func(f *flag.Flag) { flagSet[f.Name] = true })
 
 	var b Engine
 	var err error
 
-	b.Config, err = loadConfigWithSettings(settings)
+	b.Config, err = loadConfigWithSettings(settings, flagSet)
 	if err != nil {
 		return nil, fmt.Errorf("failed to load config. Err: %s", err)
 	}
@@ -96,13 +90,13 @@ func NewFromSettings(settings *Settings) (*Engine, error) {
 		return nil, fmt.Errorf("unable to adjust runtime GOMAXPROCS value. Err: %s", err)
 	}
 
-	validateSettings(&b, settings)
+	validateSettings(&b, settings, flagSet)
 	return &b, nil
 }
 
 // loadConfigWithSettings creates configuration based on the provided settings
-func loadConfigWithSettings(settings *Settings) (*config.Config, error) {
-	filePath, err := config.GetFilePath(settings.ConfigFile)
+func loadConfigWithSettings(settings *Settings, flagSet map[string]bool) (*config.Config, error) {
+	filePath, err := config.GetAndMigrateDefaultPath(settings.ConfigFile)
 	if err != nil {
 		return nil, err
 	}
@@ -127,7 +121,7 @@ func loadConfigWithSettings(settings *Settings) (*config.Config, error) {
 }
 
 // validateSettings validates and sets all bot settings
-func validateSettings(b *Engine, s *Settings) {
+func validateSettings(b *Engine, s *Settings, flagSet map[string]bool) {
 	b.Settings.Verbose = s.Verbose
 	b.Settings.EnableDryRun = s.EnableDryRun
 	b.Settings.EnableAllExchanges = s.EnableAllExchanges

engine/engine_test.go

@@ -51,12 +51,12 @@ func TestLoadConfigWithSettings(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			// prepare the 'flags'
-			flagSet = make(map[string]bool)
+			flagSet := make(map[string]bool)
 			for _, v := range tt.flags {
 				flagSet[v] = true
 			}
 			// Run the test
-			got, err := loadConfigWithSettings(tt.settings)
+			got, err := loadConfigWithSettings(tt.settings, flagSet)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("loadConfigWithSettings() error = %v, wantErr %v", err, tt.wantErr)
 				return
@@ -77,7 +77,7 @@ func TestStartStopDoesNotCausePanic(t *testing.T) {
 	botOne, err := NewFromSettings(&Settings{
 		ConfigFile:   config.TestFile,
 		EnableDryRun: true,
-	})
+	}, make(map[string]bool))
 	if err != nil {
 		t.Error(err)
 	}

engine/exchange.go

@@ -57,7 +57,7 @@ func dryrunParamInteraction(param string) {
 		return
 	}
 
-	if !Bot.Settings.EnableDryRun && !flagSet["dryrun"] {
+	if !Bot.Settings.EnableDryRun {
 		log.Warnf(log.Global,
 			"Command line argument '-%s' induces dry run mode."+
 				" Set -dryrun=false if you wish to override this.",

engine/exchange_test.go

@@ -183,13 +183,12 @@ func TestDryRunParamInteraction(t *testing.T) {
 		t.Error(err)
 	}
 
-	// Now set dryrun mode to false (via flagset and the previously enabled
-	// setting), enable exchange verbose mode and verify that verbose mode
+	// Now set dryrun mode to true,
+	// enable exchange verbose mode and verify that verbose mode
 	// will be set on Bitfinex
-	bot.Settings.EnableDryRun = false
+	bot.Settings.EnableDryRun = true
 	bot.Settings.CheckParamInteraction = true
 	bot.Settings.EnableExchangeVerbose = true
-	flagSet["dryrun"] = true
 	if err = bot.LoadExchange(testExchange, false, nil); err != nil {
 		t.Error(err)
 	}
@@ -199,8 +198,8 @@ func TestDryRunParamInteraction(t *testing.T) {
 		t.Error(err)
 	}
 
-	if bot.Settings.EnableDryRun ||
+	if !bot.Settings.EnableDryRun ||
 		!exchCfg.Verbose {
-		t.Error("dryrun should be false and verbose should be true")
+		t.Error("dryrun should be true and verbose should be true")
 	}
 }

gctscript/wrappers/gct/exchange/exchange_test.go

@@ -181,7 +181,7 @@ func TestExchange_CancelOrder(t *testing.T) {
 }
 
 func setupEngine() (err error) {
-	engine.Bot, err = engine.NewFromSettings(&settings)
+	engine.Bot, err = engine.NewFromSettings(&settings, nil)
 	if err != nil {
 		return err
 	}

main.go

@@ -109,7 +109,17 @@ func main() {
 
 	var err error
 	settings.CheckParamInteraction = true
-	engine.Bot, err = engine.NewFromSettings(&settings)
+
+	// collect flags
+	flagSet := make(map[string]bool)
+	// Stores the set flags
+	flag.Visit(func(f *flag.Flag) { flagSet[f.Name] = true })
+	if !flagSet["config"] {
+		// If config file is not explicitly set, fall back to default path resolution
+		settings.ConfigFile = ""
+	}
+
+	engine.Bot, err = engine.NewFromSettings(&settings, flagSet)
 	if engine.Bot == nil || err != nil {
 		log.Fatalf("Unable to initialise bot engine. Error: %s\n", err)
 	}