[VFS] Optimize FTP directory existence to use CWD instead of parent LIST by ilang · Pull Request #759 · apache/commons-vfs

ilang · 2026-03-31T22:22:49Z

Summary

setFTPFile() now tries CWD before falling back to parent LIST for directory existence checks
CWD is a lightweight control-channel command (no data transfer) vs LIST which opens a data channel and transfers the entire directory listing
When the path is a directory, the expensive parent LIST is skipped entirely
When the path is a file or non-existent, falls back to the original parent LIST behavior
Timestamps on CWD-resolved directories are lazily fetched via parent LIST when first requested
Adds FtpClient.isDirectory() with PWD save/restore to avoid changing the working directory

Symlink note

CWD follows symlinks transparently, so a symlink-to-directory is classified as a plain directory via the CWD fast path. This is acceptable because FTP symlink behavior in VFS is provider-internal — doGetType() resolves symlinks to their target type, and isSymbolicLink() always returns false for FTP (the FTP provider does not override doIsSymbolicLink()). The embedded MINA FTP server also cannot report symlinks in LIST output (it uses the old java.io.File API which follows symlinks transparently).

Depends on

This PR depends on #757 (root exists on disconnect fix). Please merge #757 first.

Test plan

New FtpCwdOptimizationTest with 5 tests:
- Nested directory detected via CWD optimization
- File detected via LIST fallback
- Non-existent path returns IMAGINARY
- Working directory preserved after mixed CWD/LIST operations
- Directory timestamp lazily fetched via parent LIST (no NPE)
All existing FTP tests pass

garydgregory

Hello @ilang

Please rebase on git master and review my comments.

Isn't it possible that LIST would work when CDW could fail due to access restrictions?

TY!

garydgregory · 2026-04-05T14:15:56Z

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ftp/FtpClient.java

+     * @param relPath The pathname of the directory to change to.
+     * @return true if successfully completed, false if not.
+     * @throws IOException If an I/O error occurs.
+     * @since 2.10.1


The next feature version will be 2.11.0 as you can see in the POM. How did you get to 2.10.1? Is this AI generated?

Sorry for the wrong version, I do work with AI, but I closely review it, and make sure there are tests in place. I guess I missed the version here.

garydgregory · 2026-04-05T14:16:03Z

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ftp/FtpClient.java

+     * @param relPath The pathname to test.
+     * @return true if the path is an existing directory, false otherwise.
+     * @throws IOException If an I/O error occurs.
+     * @since 2.10.1


The next feature version will be 2.11.0 as you can see in the POM. How did you get to 2.10.1? Is this AI generated?

Sorry for the wrong version, I do work with AI, but I closely review it, and make sure there are tests in place. I guess I missed the version here.

garydgregory · 2026-04-05T14:16:47Z

commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/ftp/FtpCwdOptimizationTest.java

+            manager.init();
+            final FileObject dir = manager.resolveFile(
+                FtpProviderTest.getConnectionUri() + "/cwdOptDir/nested", createOptions());
+            Assertions.assertTrue(dir.exists(), "Nested directory should exist");


Please use static imports but only for JUnit.

ilang · 2026-04-05T19:07:11Z

Hello @ilang

Please rebase on git master and review my comments.

Isn't it possible that LIST would work when CDW could fail due to access restrictions?

TY!

Thanks, @garydgregory

Good point, had too look it up myself. and yes, CWD could fail due to access restrictions while LIST on the parent would still work.
However, this is already handled correctly in the implementation.
If CWD fails for any reason (access denied, not a directory, or non-existent), we fall back to the original parent LIST behavior:

in FTPFileObject :

private void setFTPFile(final boolean flush) throws IOException {
...
final FTPFile cwdResult = verifyDirectory();
if (cwdResult != null) {
newFileInfo = cwdResult;
} else {
newFileInfo = parent.getChildFile(UriParser.decode(getName().getBaseName()), flush);
}
...

The only time we don't fall back is when this is a root directory, however the root case is also covered: the factory already CWDs to the root directory during connection setup.
if CWD fails, the connection itself fails with FileSystemException. So if a user is connected, CWD "." on the root is guaranteed to work since they're already in that directory.

ilang · 2026-04-05T19:07:26Z

Hi @garydgregory, I am going to rebase and commit, I also added a very small optimization, to save one cwd command when it is the "." folder.

@SInCE

For directory existence checks, CWD is a lightweight control-channel command with no data transfer, while LIST opens a data channel and transfers the entire directory contents. When CWD succeeds (path is a directory), the expensive parent LIST is skipped entirely. When CWD fails (path is a file, symlink, non-existent, or access-restricted), falls back to the original parent LIST behavior. Add FtpClient.isDirectory() with PWD save/restore in FTPClientWrapper to avoid changing the working directory when probing non-root paths. Skip save/restore for CWD "." (root check) since it does not change the working directory. Rename verifyRootDirectory() to verifyDirectory() as it now serves both root and non-root paths. Fix NPE in getTimestampMillis() for CWD-resolved directories: the synthetic FTPFile has no timestamp, so lazily fetch the real FTPFile via parent LIST when timestamp is first requested. Review fixes: - Fix @SInCE 2.10.1 to @SInCE 2.11.0 to match POM version. - Use static imports for JUnit assertions.

sebbASF · 2026-04-06T08:05:34Z

I may be missing something here, but why not use the FTP command MLST [1] to return just the details for a single file/directory?

[1] https://commons.apache.org/proper/commons-net/apidocs/org/apache/commons/net/ftp/FTPClient.html#mlistFile(java.lang.String)

ilang · 2026-04-06T22:17:34Z

I may be missing something here, but why not use the FTP command MLST [1] to return just the details for a single file/directory?

[1] https://commons.apache.org/proper/commons-net/apidocs/org/apache/commons/net/ftp/FTPClient.html#mlistFile(java.lang.String)

Thanks @sebbASF I will check it, I moved this pull request to be a draft, to check more stuff, and while I am dealing with my pervious PR that needs fixing.

ilang marked this pull request as draft March 31, 2026 22:35

ilang force-pushed the VFS-cwd-optimization branch from 0ec1a6e to c0cec45 Compare April 1, 2026 08:26

ilang marked this pull request as ready for review April 1, 2026 09:54

garydgregory requested changes Apr 5, 2026

View reviewed changes

ilang force-pushed the VFS-cwd-optimization branch from c0cec45 to 2ed521d Compare April 5, 2026 19:10

ilang marked this pull request as draft April 5, 2026 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VFS] Optimize FTP directory existence to use CWD instead of parent LIST#759

[VFS] Optimize FTP directory existence to use CWD instead of parent LIST#759
ilang wants to merge 1 commit intoapache:masterfrom
new-proimage:VFS-cwd-optimization

ilang commented Mar 31, 2026 •

edited

Loading

Uh oh!

garydgregory left a comment

Uh oh!

garydgregory Apr 5, 2026

Uh oh!

ilang Apr 5, 2026 •

edited

Loading

Uh oh!

garydgregory Apr 5, 2026

Uh oh!

ilang Apr 5, 2026

Uh oh!

garydgregory Apr 5, 2026

Uh oh!

ilang commented Apr 5, 2026

Uh oh!

ilang commented Apr 5, 2026 •

edited

Loading

Uh oh!

sebbASF commented Apr 6, 2026

Uh oh!

ilang commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ilang commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Symlink note

Depends on

Test plan

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

garydgregory Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

ilang Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garydgregory Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

ilang Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

garydgregory Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

ilang commented Apr 5, 2026

Uh oh!

ilang commented Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebbASF commented Apr 6, 2026

Uh oh!

ilang commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ilang commented Mar 31, 2026 •

edited

Loading

ilang Apr 5, 2026 •

edited

Loading

ilang commented Apr 5, 2026 •

edited

Loading