Merge pull request #6 from moov-io/impl-list-files-with-walk

fix: implement ListFiles with Walk to avoid some error cases

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()
@@ -272,44 +272,35 @@ 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(strings.TrimPrefix(dir, string(os.PathSeparator)))
+	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()
+	var filenames []string
+	err := cc.Walk(".", func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
-			return nil, err
+			return err
 		}
-		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
+		if d.IsDir() {
+			return nil
 		}
-	}
-
-	fds, err := conn.List("")
+		matches, err := filepath.Match(pattern, path)
+		if matches && err == nil {
+			filenames = append(filenames, filepath.Base(path))
+		}
+		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
 }
 

client_test.go

@@ -75,7 +75,21 @@ 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"})
+
+		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) {
@@ -85,7 +99,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 +137,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())

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"