From 0b74d1c9cd862106546fe34bc673726512758c21 Mon Sep 17 00:00:00 2001 From: Adam Shannon Date: Mon, 14 Aug 2023 11:27:45 -0500 Subject: [PATCH 1/4] fix: implement ListFiles with Walk to avoid some error cases --- client.go | 50 ++++++++++---------------- client_test.go | 15 ++++++-- docker-compose.yml | 2 +- testdata/ftp-server/archive/empty2.txt | 0 testdata/ftp-server/empty.txt | 0 5 files changed, 32 insertions(+), 35 deletions(-) create mode 100644 testdata/ftp-server/archive/empty2.txt create mode 100644 testdata/ftp-server/empty.txt diff --git a/client.go b/client.go index 7cef8bd..15adcac 100644 --- a/client.go +++ b/client.go @@ -272,44 +272,32 @@ func (cc *client) UploadFile(path string, contents io.ReadCloser) (err error) { } func (cc *client) ListFiles(dir string) ([]string, error) { - cc.mu.Lock() - defer cc.mu.Unlock() - - conn, err := cc.connection() - if err != nil { - return nil, err + pattern := filepath.Clean(dir) + switch { + case pattern == ".": + if dir == "" { + pattern = "*" + } else { + pattern = filepath.Join(dir, "*") + } + case pattern != "": + pattern += "/*" } - if dir != "" && dir != "." { - // Jump to previous directory after command is done - wd, err := conn.CurrentDir() - if err != nil { - return nil, err + var filenames []string + err := cc.Walk(".", func(path string, d fs.DirEntry, err error) error { + if d.IsDir() { + return nil } - defer func(previous string) { - // Return to our previous directory when initially called - if cleanupErr := conn.ChangeDir(previous); cleanupErr != nil { - err = fmt.Errorf("FTP: problem listing %s: %w", dir, cleanupErr) - } - }(wd) - - // Move into directory to run the command - if err := conn.ChangeDir(dir); err != nil { - return nil, err + matches, err := filepath.Match(pattern, path) + if matches && err == nil { + filenames = append(filenames, filepath.Base(path)) } - } - - fds, err := conn.List("") + return err + }) if err != nil { return nil, fmt.Errorf("listing %s failed: %w", dir, err) } - - var filenames []string - for i := range fds { - if fds[i].Type == ftp.EntryTypeFile { - filenames = append(filenames, filepath.Base(fds[i].Name)) - } - } return filenames, nil } diff --git a/client_test.go b/client_test.go index cbabb96..e65e086 100644 --- a/client_test.go +++ b/client_test.go @@ -75,7 +75,13 @@ func TestClient(t *testing.T) { t.Run("list", func(t *testing.T) { filenames, err := client.ListFiles(".") require.NoError(t, err) - require.ElementsMatch(t, filenames, []string{"first.txt", "second.txt"}) + require.ElementsMatch(t, filenames, []string{"first.txt", "second.txt", "empty.txt"}) + }) + + t.Run("list subdir", func(t *testing.T) { + filenames, err := client.ListFiles("archive") + require.NoError(t, err) + require.ElementsMatch(t, filenames, []string{"old.txt", "empty2.txt"}) }) t.Run("walk", func(t *testing.T) { @@ -85,7 +91,7 @@ func TestClient(t *testing.T) { return nil }) require.NoError(t, err) - require.ElementsMatch(t, found, []string{"first.txt", "second.txt", "archive/old.txt"}) + require.ElementsMatch(t, found, []string{"first.txt", "second.txt", "empty.txt", "archive/old.txt", "archive/empty2.txt"}) }) require.NoError(t, client.Close()) @@ -123,15 +129,18 @@ func TestClientErrors(t *testing.T) { t.Run("list", func(t *testing.T) { filenames, err := client.ListFiles("does/not/exist") - require.ErrorContains(t, err, "550 Directory change to /does/not/exist failed: lstat /data/does/not/exist: no such file or directory") + require.NoError(t, err) require.Len(t, filenames, 0) }) t.Run("walk", func(t *testing.T) { + var found []string err := client.Walk("does/not/exist", func(path string, d fs.DirEntry, err error) error { + found = append(found, path) return nil }) require.ErrorContains(t, err, "550 Directory change to /does/not/exist failed: lstat /data/does/not/exist: no such file or directory") + require.Len(t, found, 0) }) require.NoError(t, client.Close()) diff --git a/docker-compose.yml b/docker-compose.yml index 294a766..d98b06e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -4,7 +4,7 @@ version: '3.7' services: ftp: - image: moov/fsftp:v0.2.1 + image: moov/fsftp:v0.2.2 ports: - "2121:2121" - "30000-30009:30000-30009" diff --git a/testdata/ftp-server/archive/empty2.txt b/testdata/ftp-server/archive/empty2.txt new file mode 100644 index 0000000..e69de29 diff --git a/testdata/ftp-server/empty.txt b/testdata/ftp-server/empty.txt new file mode 100644 index 0000000..e69de29 From 28bd8e951db357d9bd7bc27140872a58702b2736 Mon Sep 17 00:00:00 2001 From: Adam Shannon Date: Mon, 14 Aug 2023 12:13:17 -0500 Subject: [PATCH 2/4] fix: treat forced-root paths the same as relative in ListFiles --- client.go | 2 +- client_test.go | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/client.go b/client.go index 15adcac..3c7ef8f 100644 --- a/client.go +++ b/client.go @@ -272,7 +272,7 @@ func (cc *client) UploadFile(path string, contents io.ReadCloser) (err error) { } func (cc *client) ListFiles(dir string) ([]string, error) { - pattern := filepath.Clean(dir) + pattern := filepath.Clean(strings.TrimPrefix(dir, string(os.PathSeparator))) switch { case pattern == ".": if dir == "" { diff --git a/client_test.go b/client_test.go index e65e086..1ba03cf 100644 --- a/client_test.go +++ b/client_test.go @@ -82,6 +82,14 @@ func TestClient(t *testing.T) { filenames, err := client.ListFiles("archive") require.NoError(t, err) require.ElementsMatch(t, filenames, []string{"old.txt", "empty2.txt"}) + + filenames, err = client.ListFiles("/archive") + require.NoError(t, err) + require.ElementsMatch(t, filenames, []string{"old.txt", "empty2.txt"}) + + filenames, err = client.ListFiles("/archive/") + require.NoError(t, err) + require.ElementsMatch(t, filenames, []string{"old.txt", "empty2.txt"}) }) t.Run("walk", func(t *testing.T) { From e2ed682539c334521146b7b3216b3eb353d4ae4e Mon Sep 17 00:00:00 2001 From: Adam Shannon Date: Mon, 14 Aug 2023 13:07:23 -0500 Subject: [PATCH 3/4] docs: mention that we're having issues with multiple open readers --- client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client.go b/client.go index 3c7ef8f..ccb0ab2 100644 --- a/client.go +++ b/client.go @@ -197,7 +197,7 @@ func (cc *client) Open(path string) (*File, error) { // Callers need to close the returned Contents // // Callers should be aware that network errors while reading can occur since contents -// are streamed from the FTP server. +// are streamed from the FTP server. Having multiple open readers may not be supported. func (cc *client) Reader(path string) (*File, error) { cc.mu.Lock() defer cc.mu.Unlock() From b0a3b6d0d44905d873bf827c922157f78d3a5f6f Mon Sep 17 00:00:00 2001 From: Adam Shannon Date: Mon, 14 Aug 2023 13:12:59 -0500 Subject: [PATCH 4/4] fix: linter cleanup --- client.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/client.go b/client.go index ccb0ab2..c0aeebd 100644 --- a/client.go +++ b/client.go @@ -286,6 +286,9 @@ func (cc *client) ListFiles(dir string) ([]string, error) { var filenames []string err := cc.Walk(".", func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } if d.IsDir() { return nil }