Skip to content

Commit 0ec1a6e

Browse files
ilangilang
committed
[VFS] Optimize setFTPFile() to try CWD before parent LIST
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, or non-existent), falls back to the original parent LIST behavior. Rename verifyRootDirectory() to verifyDirectory() as it now serves both root and non-root paths.
1 parent 0a9e786 commit 0ec1a6e

2 files changed

Lines changed: 192 additions & 8 deletions

File tree

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

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -629,26 +629,31 @@ private void setFTPFile(final boolean flush) throws IOException {
629629
final FtpFileObject parent = (FtpFileObject) FileObjectUtils.getAbstractFileObject(getParent());
630630
final FTPFile newFileInfo;
631631
if (parent != null) {
632-
newFileInfo = parent.getChildFile(UriParser.decode(getName().getBaseName()), flush);
632+
// Try CWD first: if it succeeds, this is a directory and we avoid
633+
// the expensive parent LIST which opens a data channel and transfers
634+
// the entire directory listing. If CWD fails, fall back to parent
635+
// LIST to handle files, symlinks, and non-existent paths.
636+
final FTPFile cwdResult = verifyDirectory();
637+
if (cwdResult != null) {
638+
newFileInfo = cwdResult;
639+
} else {
640+
newFileInfo = parent.getChildFile(UriParser.decode(getName().getBaseName()), flush);
641+
}
633642
} else {
634643
// Root-level resource: no parent to query via getChildFile().
635-
// Verify the directory exists using CWD, which is a lightweight
636-
// control-channel command (no data transfer). Previously this
637-
// assumed the root always exists, causing exists() to return true
638-
// for non-existent FTP folders.
639-
newFileInfo = verifyRootDirectory();
644+
newFileInfo = verifyDirectory();
640645
}
641646
ftpFile = newFileInfo == null ? UNKNOWN : newFileInfo;
642647
}
643648
}
644649

645650
/**
646-
* Verifies a root-level FTP directory exists by attempting to CWD into it.
651+
* Verifies an FTP directory exists by attempting to CWD into it.
647652
* Returns an FTPFile with DIRECTORY_TYPE if CWD succeeds, or {@code null} if it fails.
648653
* CWD is used instead of LIST because it is a lightweight control-channel command
649654
* that does not transfer directory contents.
650655
*/
651-
private FTPFile verifyRootDirectory() throws IOException {
656+
private FTPFile verifyDirectory() throws IOException {
652657
final FtpClient client = getAbstractFileSystem().getClient();
653658
try {
654659
if (client.isDirectory(relPath != null ? relPath : "/")) {

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

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* https://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package org.apache.commons.vfs2.provider.ftp;
18+
19+
import java.io.File;
20+
import java.io.FileWriter;
21+
import java.time.Duration;
22+
23+
import org.apache.commons.vfs2.FileObject;
24+
import org.apache.commons.vfs2.FileSystemOptions;
25+
import org.apache.commons.vfs2.FileType;
26+
import org.apache.commons.vfs2.VfsTestUtils;
27+
import org.apache.commons.vfs2.impl.DefaultFileSystemManager;
28+
import org.junit.jupiter.api.AfterEach;
29+
import org.junit.jupiter.api.Assertions;
30+
import org.junit.jupiter.api.BeforeEach;
31+
import org.junit.jupiter.api.Test;
32+
33+
/**
34+
* Tests that {@link FtpFileObject#setFTPFile} uses CWD to check directory existence before
35+
* falling back to the expensive parent LIST command.
36+
* <p>
37+
* Verifies that:
38+
* <ul>
39+
* <li>Directories are correctly detected via CWD (no parent LIST needed)</li>
40+
* <li>Files are correctly detected via parent LIST fallback</li>
41+
* <li>Non-existent paths return {@link FileType#IMAGINARY}</li>
42+
* <li>Working directory is preserved after all operations</li>
43+
* </ul>
44+
*/
45+
public class FtpCwdOptimizationTest {
46+
47+
private File testDir;
48+
private File nestedDir;
49+
private File fileInRoot;
50+
private File fileInNested;
51+
52+
@BeforeEach
53+
public void setUp() throws Exception {
54+
FtpProviderTest.setUpClass(VfsTestUtils.getTestDirectory(), null, null);
55+
56+
// Create test structure:
57+
// testDir/
58+
// cwdOptDir/
59+
// nested/
60+
// file.txt
61+
// rootfile.txt
62+
testDir = new File(VfsTestUtils.getTestDirectory());
63+
final File cwdOptDir = new File(testDir, "cwdOptDir");
64+
nestedDir = new File(cwdOptDir, "nested");
65+
nestedDir.mkdirs();
66+
fileInRoot = new File(testDir, "rootfile.txt");
67+
fileInNested = new File(cwdOptDir, "file.txt");
68+
try (FileWriter w = new FileWriter(fileInRoot)) {
69+
w.write("root content");
70+
}
71+
try (FileWriter w = new FileWriter(fileInNested)) {
72+
w.write("nested content");
73+
}
74+
}
75+
76+
@AfterEach
77+
public void tearDown() {
78+
FtpProviderTest.tearDownClass();
79+
if (fileInNested != null) fileInNested.delete();
80+
if (fileInRoot != null) fileInRoot.delete();
81+
if (nestedDir != null) nestedDir.delete();
82+
new File(testDir, "cwdOptDir").delete();
83+
}
84+
85+
private static FileSystemOptions createOptions() {
86+
final FileSystemOptions options = new FileSystemOptions();
87+
final FtpFileSystemConfigBuilder builder = FtpFileSystemConfigBuilder.getInstance();
88+
builder.setPassiveMode(options, true);
89+
builder.setConnectTimeout(options, Duration.ofSeconds(10));
90+
return options;
91+
}
92+
93+
/**
94+
* Tests that a nested directory is detected as existing with type FOLDER.
95+
* After the CWD optimization, this should use CWD instead of parent LIST.
96+
*/
97+
@Test
98+
public void testNestedDirectoryExistsViaOptimization() throws Exception {
99+
try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
100+
manager.addProvider("ftp", new FtpFileProvider());
101+
manager.init();
102+
final FileObject dir = manager.resolveFile(
103+
FtpProviderTest.getConnectionUri() + "/cwdOptDir/nested", createOptions());
104+
Assertions.assertTrue(dir.exists(), "Nested directory should exist");
105+
Assertions.assertEquals(FileType.FOLDER, dir.getType(), "Nested directory should be FOLDER");
106+
}
107+
}
108+
109+
/**
110+
* Tests that a file is detected correctly via parent LIST fallback
111+
* (CWD fails on files, so setFTPFile falls back to parent LIST).
112+
*/
113+
@Test
114+
public void testFileDetectedViaListFallback() throws Exception {
115+
try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
116+
manager.addProvider("ftp", new FtpFileProvider());
117+
manager.init();
118+
final FileObject file = manager.resolveFile(
119+
FtpProviderTest.getConnectionUri() + "/cwdOptDir/file.txt", createOptions());
120+
Assertions.assertTrue(file.exists(), "File should exist");
121+
Assertions.assertEquals(FileType.FILE, file.getType(), "file.txt should be FILE");
122+
}
123+
}
124+
125+
/**
126+
* Tests that a non-existent path is correctly detected as IMAGINARY.
127+
*/
128+
@Test
129+
public void testNonExistentPathIsImaginary() throws Exception {
130+
try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
131+
manager.addProvider("ftp", new FtpFileProvider());
132+
manager.init();
133+
final FileObject ghost = manager.resolveFile(
134+
FtpProviderTest.getConnectionUri() + "/cwdOptDir/doesNotExist", createOptions());
135+
Assertions.assertFalse(ghost.exists(), "Non-existent path should not exist");
136+
Assertions.assertEquals(FileType.IMAGINARY, ghost.getType(), "Non-existent path should be IMAGINARY");
137+
}
138+
}
139+
140+
/**
141+
* Tests that the FTP working directory is preserved after checking existence
142+
* of directories, files, and non-existent paths. Operations on unrelated paths
143+
* must still work correctly after any combination of CWD checks.
144+
*/
145+
@Test
146+
public void testWorkingDirectoryPreservedAfterMixedChecks() throws Exception {
147+
try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
148+
manager.addProvider("ftp", new FtpFileProvider());
149+
manager.init();
150+
final FileSystemOptions options = createOptions();
151+
152+
// Access a file first — establishes baseline.
153+
final FileObject rootFile = manager.resolveFile(
154+
FtpProviderTest.getConnectionUri() + "/rootfile.txt", options);
155+
Assertions.assertTrue(rootFile.exists(), "rootfile.txt should exist initially");
156+
157+
// Check a nested directory (triggers CWD optimization).
158+
final FileObject dir = manager.resolveFile(
159+
FtpProviderTest.getConnectionUri() + "/cwdOptDir/nested", options);
160+
Assertions.assertTrue(dir.exists(), "nested dir should exist");
161+
162+
// Check a file in a different directory (triggers LIST fallback).
163+
final FileObject nestedFile = manager.resolveFile(
164+
FtpProviderTest.getConnectionUri() + "/cwdOptDir/file.txt", options);
165+
Assertions.assertTrue(nestedFile.exists(), "file.txt should exist");
166+
167+
// Check a non-existent path (CWD fails, LIST fallback finds nothing).
168+
final FileObject ghost = manager.resolveFile(
169+
FtpProviderTest.getConnectionUri() + "/cwdOptDir/doesNotExist", options);
170+
Assertions.assertFalse(ghost.exists(), "ghost should not exist");
171+
172+
// Now verify the original file is still accessible after all the above.
173+
rootFile.refresh();
174+
Assertions.assertTrue(rootFile.exists(),
175+
"rootfile.txt must still be accessible after mixed CWD/LIST operations");
176+
Assertions.assertEquals(FileType.FILE, rootFile.getType());
177+
}
178+
}
179+
}

0 commit comments

Comments
 (0)