From 4e23bfd413d029ff18b7d545fa493b28980b90ad Mon Sep 17 00:00:00 2001 From: Matthew F Leader Date: Thu, 4 Jan 2024 12:27:18 -0500 Subject: [PATCH] add error tests rewrite test and add comment fix init slice len fix gosec warning maybe report abs file path config use abs filepath use regular file move code clarify test simplify error msg func comment --- cmd/arcaflow/main.go | 16 +++++---- loadfile/loadfile.go | 17 ++++----- loadfile/loadfile_test.go | 73 ++++++++++++++++++++++++++++++++------- 3 files changed, 79 insertions(+), 27 deletions(-) diff --git a/cmd/arcaflow/main.go b/cmd/arcaflow/main.go index 896a0409..a01cc207 100644 --- a/cmd/arcaflow/main.go +++ b/cmd/arcaflow/main.go @@ -130,15 +130,16 @@ Options: RequiredFileKeyWorkflow: workflowFile, } - requiredFilesAbsPaths, err := loadfile.ContextAbsFilepaths(dir, requiredFiles) + requiredFilesAbsPaths, err := loadfile.AbsPathsWithContext(dir, requiredFiles) if err != nil { + flag.Usage() tempLogger.Errorf("Failed to determine absolute path of arcaflow context directory %s (%v)", dir, err) os.Exit(ExitCodeInvalidData) } var configData any = map[any]any{} if configFile != "" { - configData, err = loadYamlFile(configFile) + configData, err = loadYamlFile(requiredFilesAbsPaths[RequiredFileKeyConfig]) if err != nil { tempLogger.Errorf("Failed to load configuration file %s (%v)", configFile, err) flag.Usage() @@ -151,17 +152,20 @@ Options: flag.Usage() os.Exit(ExitCodeInvalidData) } - cfg.Log.Stdout = os.Stderr + // now we are ready to instantiate our main logger + cfg.Log.Stdout = os.Stderr logger := log.New(cfg.Log).WithLabel("source", "main") - var requiredFilesAbsSlice = make([]string, len(requiredFiles)) + var requiredFilesAbsSlice = make([]string, len(requiredFilesAbsPaths)) + var j int for _, f := range requiredFilesAbsPaths { - requiredFilesAbsSlice = append(requiredFilesAbsSlice, f) + requiredFilesAbsSlice[j] = f + j++ } dirContext, err := loadfile.LoadContext(requiredFilesAbsSlice) if err != nil { - logger.Errorf("Failed to load configuration file %s (%v)", configFile, err) + logger.Errorf("Failed to load required files into context (%v)", err) flag.Usage() os.Exit(ExitCodeInvalidData) } diff --git a/loadfile/loadfile.go b/loadfile/loadfile.go index ce4c0912..242b5188 100644 --- a/loadfile/loadfile.go +++ b/loadfile/loadfile.go @@ -7,32 +7,33 @@ import ( "path/filepath" ) -// LoadContext reads the contents at each file into a map where the key -// is the absolute filepath and file contents is the value. +// LoadContext reads the content of each file into a map where the key +// is the absolute filepath and the file content is the value. func LoadContext(neededFilepaths []string) (map[string][]byte, error) { result := map[string][]byte{} var err error for _, filePath := range neededFilepaths { absPath, err := filepath.Abs(filePath) if err != nil { - return nil, fmt.Errorf("failed to obtain absolute path of file %s (%w)", filepath.Base(filePath), err) + return nil, fmt.Errorf("error obtaining absolute path of file %s (%w)", + filePath, err) } - fileData, err := os.ReadFile(absPath) //nolint:gosec + fileData, err := os.ReadFile(filepath.Clean(absPath)) if err != nil { - return nil, fmt.Errorf("failed to read file from context directory: %s (%w)", absPath, err) + return nil, fmt.Errorf("error reading file %s (%w)", absPath, err) } result[absPath] = fileData } return result, err } -// ContextAbsFilepaths creates a map of absolute filepaths. If a required +// AbsPathsWithContext creates a map of absolute filepaths. If a required // file is not provided with an absolute path, then it is joined with the // root directory. -func ContextAbsFilepaths(rootDir string, requiredFiles map[string]string) (map[string]string, error) { +func AbsPathsWithContext(rootDir string, requiredFiles map[string]string) (map[string]string, error) { absDir, err := filepath.Abs(rootDir) if err != nil { - return nil, err + return nil, fmt.Errorf("error determining context directory absolute path %s (%w)", rootDir, err) } requiredFilesAbs := map[string]string{} for key, f := range requiredFiles { diff --git a/loadfile/loadfile_test.go b/loadfile/loadfile_test.go index 7e572457..24954564 100644 --- a/loadfile/loadfile_test.go +++ b/loadfile/loadfile_test.go @@ -22,39 +22,86 @@ func TestLoadContext(t *testing.T) { assert.NoError(t, os.MkdirAll(testdir, os.ModePerm)) - // create a directory and a file + // create a directory dirname := "mydir" dirpath := filepath.Join(testdir, dirname) - filename := "myfile" assert.NoError(t, os.MkdirAll(dirpath, os.ModePerm)) - f, err := os.CreateTemp(testdir, filename) + + // create a file + filename := "myfile" + filePath := filepath.Join(testdir, filename) + f, err := os.Create(filepath.Clean(filePath)) assert.NoError(t, err) - tempfilepath := f.Name() assert.NoError(t, f.Close()) - // create symlinks to the above directory and file + // create symlink to the directory symlinkDirname := dirname + "_sym" - symlinkFilepath := tempfilepath + "_sym" symlinkDirpath := filepath.Join(testdir, symlinkDirname) assert.NoError(t, os.Symlink(dirpath, symlinkDirpath)) - assert.NoError(t, os.Symlink(tempfilepath, symlinkFilepath)) + + // create symlink to the file + symlinkFilepath := filePath + "_sym" + assert.NoError(t, os.Symlink(filePath, symlinkFilepath)) neededFiles := []string{ - tempfilepath, + filePath, symlinkFilepath, } filemap, err := loadfile.LoadContext(neededFiles) - filemapExp := map[string][]byte{ - tempfilepath: {}, - symlinkFilepath: {}, - } // assert no error on attempting to read files // that cannot be read assert.NoError(t, err) - // assert only the regular file will be loaded + // assert only the regular and symlinked file are loaded + filemapExp := map[string][]byte{ + filePath: {}, + symlinkFilepath: {}, + } assert.Equals(t, filemap, filemapExp) + // error on loading a directory + neededFiles = []string{ + dirpath, + } + _, err = loadfile.LoadContext(neededFiles) + assert.Error(t, err) + + // error on loading a symlink directory + neededFiles = []string{ + symlinkDirpath, + } + _, err = loadfile.LoadContext(neededFiles) + assert.Error(t, err) + + t.Cleanup(func() { + assert.NoError(t, os.RemoveAll(testdir)) + }) +} + +// This tests AbsPathsWithContext joins relative paths with the +// context (root) directory, and passes through absolute paths +// unmodified. +func TestContextAbsFilepaths(t *testing.T) { + testdir, err := os.MkdirTemp(os.TempDir(), "") + assert.NoError(t, err) + + testFilepaths := map[string]string{ + "a": "a.yaml", + "b": "/b.toml", + "c": "../rel/subdir/c.txt", + } + + absPathsExp := map[string]string{ + "a": filepath.Join(testdir, testFilepaths["a"]), + // since the 'b' file has an absolute path, it should be unmodified + "b": "/b.toml", + "c": filepath.Join(testdir, testFilepaths["c"]), + } + + absPathsGot, err := loadfile.AbsPathsWithContext(testdir, testFilepaths) + assert.NoError(t, err) + assert.Equals(t, absPathsExp, absPathsGot) + t.Cleanup(func() { assert.NoError(t, os.RemoveAll(testdir)) })