Fix FtpFileObject.exists() returning true for root-level folders after connection drop#757
Open
ilang wants to merge 1 commit intoapache:masterfrom
Open
Fix FtpFileObject.exists() returning true for root-level folders after connection drop#757ilang wants to merge 1 commit intoapache:masterfrom
ilang wants to merge 1 commit intoapache:masterfrom
Conversation
ilang
force-pushed
the
VFS-root-exists-on-disconnect
branch
from
2c619c8 to
5984094
Compare
garydgregory
requested changes
Mar 31, 2026
Member
garydgregory
left a comment
garydgregory
left a comment
There was a problem hiding this comment.
-1: don't write to the console.
| */ | ||
| package org.apache.commons.vfs2.provider.ftp; | ||
|
|
||
| import static org.apache.commons.vfs2.VfsTestUtils.getTestDirectory; |
Member
There was a problem hiding this comment.
Only use static imports for JUnit assertions.
ilang
force-pushed
the
VFS-root-exists-on-disconnect
branch
from
5984094 to
29f5342
Compare
…s after connection drop setFTPFile() blindly assumed that root-level directories (where getParent() returns null) always exist by creating a synthetic FTPFile with DIRECTORY_TYPE. This caused exists() to return true even after the FTP connection was lost. Fix: add verifyRootDirectory() which uses CWD "." to verify the directory exists on the server. CWD "." checks the current directory (the logical VFS root) which is correct for both configurations: - userDirIsRoot=true: current dir is the user's login directory - userDirIsRoot=false: current dir is "/" (set by the factory CWD) Using "." rather than "/" avoids a semantic mismatch on non-chroot servers with userDirIsRoot=true, where "/" would go to the actual server root instead of the user's login directory. Add FtpClient.changeDirectory() default method and FTPClientWrapper implementation to support CWD from FtpFileObject.
ilang
force-pushed
the
VFS-root-exists-on-disconnect
branch
from
0a9e786 to
18bf29e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
getParent()returnsnull(root-level FTP path withuserDirIsRoot=true),setFTPFile()blindly assumed the directory exists by creating anFTPFilewithDIRECTORY_TYPEwithout contacting the server. This causedexists()to returntrueeven after the FTP connection was lost, while non-root folders correctly reported the failure viaFileSystemException.Fix
Use CWD (Change Working Directory) to verify the directory actually exists on the server. CWD is a lightweight control-channel command with no data transfer, unlike LIST which would transfer the entire directory contents.
Changes
FtpClient: addchangeDirectory()default methodFTPClientWrapper: implementchangeDirectory()viaFTPClient.changeWorkingDirectory()FtpFileObject.setFTPFile(): replace blindDIRECTORY_TYPEassumption withverifyRootDirectory()using CWDFtpRootExistsOnDisconnectTest: regression test — fails without fix, passes with fixTest plan
FtpRootExistsOnDisconnectTestverifies exists() detects connection drops on root-level folders