From 8c86aac21d638bef381123a0f1c5fce165c09577 Mon Sep 17 00:00:00 2001 From: Rauno Ots Date: Thu, 15 Oct 2020 04:24:43 +0200 Subject: [PATCH] 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 | 18 ++ common/file/file_test.go | 13 +- config/config.go | 158 +++++++----------- config/config_test.go | 134 ++++++++++++++- engine/engine.go | 18 +- engine/engine_test.go | 6 +- engine/exchange.go | 2 +- engine/exchange_test.go | 11 +- .../wrappers/gct/exchange/exchange_test.go | 2 +- main.go | 12 +- 10 files changed, 249 insertions(+), 125 deletions(-) diff --git a/common/file/file.go b/common/file/file.go index 9567e06721f..0932a934d4f 100644 --- a/common/file/file.go +++ b/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() diff --git a/common/file/file_test.go b/common/file/file_test.go index 7fd1fc1ea9f..32911472d9f 100644 --- a/common/file/file_test.go +++ b/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) diff --git a/config/config.go b/config/config.go index 97c358f3a4a..ebe5a971f36 100644 --- a/config/config.go +++ b/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 } } + if configfile == "" { + return "", false, fmt.Errorf("config.json file not found in %s, please follow README.md in root dir for config generation", + newDir) + } - // 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 - } + return configfile, true, nil +} - if filepath.Ext(newDirs[x]) == ".json" { - return newDirs[x], 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 + } - err = file.Move(newDirs[x], newDirs[0]) - 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 + } - return newDirs[0], nil + err = file.Move(configFile, target) + if err != nil { + return "", err } - return "", fmt.Errorf("config.json file not found in %s, please follow README.md in root dir for config generation", - newDir) + 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 } diff --git a/config/config_test.go b/config/config_test.go index 7b2c6f7802e..00527c36cf5 100644 --- a/config/config_test.go +++ b/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) + } + }) + } +} diff --git a/engine/engine.go b/engine/engine.go index ec8bd18adcf..794980016c7 100644 --- a/engine/engine.go +++ b/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 diff --git a/engine/engine_test.go b/engine/engine_test.go index 8abaf44d04a..888b619b617 100644 --- a/engine/engine_test.go +++ b/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) } diff --git a/engine/exchange.go b/engine/exchange.go index c960e823efc..6be8ec14a67 100644 --- a/engine/exchange.go +++ b/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.", diff --git a/engine/exchange_test.go b/engine/exchange_test.go index 5b96799dbb9..b4cc1bb219a 100644 --- a/engine/exchange_test.go +++ b/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") } } diff --git a/gctscript/wrappers/gct/exchange/exchange_test.go b/gctscript/wrappers/gct/exchange/exchange_test.go index 21d79755074..21040a896bd 100644 --- a/gctscript/wrappers/gct/exchange/exchange_test.go +++ b/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 } diff --git a/main.go b/main.go index 158fb5dd868..8d3148dfeb9 100644 --- a/main.go +++ b/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) }