[VFS-862] Fix ON_RESOLVE triggering refresh on internal navigation (#761)

* [VFS-862] Fix ON_RESOLVE triggering refresh on internal navigation

CacheStrategy.ON_RESOLVE is intended to refresh files when the user
resolves them via the public API. However, internal navigation methods
(getParent, getRoot, getChildren child resolution, symlink resolution)
also called fileSystem.resolveFile(), triggering ON_RESOLVE refreshes
on files the user never asked to refresh.

This became a severe regression after refresh() was changed to
unconditionally clear FtpFileObject.childMap: each child's getParent()
refreshed the parent, clearing its childMap, forcing a new FTP LIST
command per child. A directory with N files produced ~N LIST commands
instead of 1.

Fix:
- Add resolveFileInternal() that skips the ON_RESOLVE refresh. All
  internal navigation call sites use it instead of resolveFile().
- After a fresh directory listing, FTP and SFTP providers propagate
  metadata to cached child objects in-place, preserving object identity.

This establishes a clear contract: cached state is used until the user
explicitly calls refresh() or resolves via the public API. Internal
navigation never triggers server operations.

Tests:
- FtpGetChildrenListCommandTest: verifies findFiles() on a directory
  with 50 files issues exactly 1 LIST command with ON_RESOLVE.
- SftpGetChildrenListCommandTest: verifies refresh + findFiles()
  returns fresh children reflecting filesystem changes.

* [VFS-862] Disable HTTP persistent connections in Jackrabbit 1.x tests

Jackrabbit 1.x bundles Jetty 6.x which does not drain unconsumed
request bodies before sending error responses (e.g. 404) on persistent
connections, violating HTTP/1.1 requirements. This leaves stale request
bytes that corrupt the next request on a reused connection.

The resolveFileInternal change in getParent() removed an extra PROPFIND
request to the parent directory that used to happen between the child's
PROPFIND (404) and the child's PUT. That extra request happened to flush
the stale bytes from the connection. Without it, the server reads the
112-byte PROPFIND XML request body as the PUT file content.

Fix: add Connection: close to the test HttpClient configuration so
each request uses a fresh connection.

Jetty 9.4+ handles this correctly (see jetty#651, jetty#4117, jetty#6168).
Jackrabbit 2 tests pass without changes. No production code modified.

* [VFS-862] Remove SFTP test that passes without the fix

The SFTP test cannot distinguish the fix from the original ON_RESOLVE
behavior because SFTP's doListChildrenResolved() pushes metadata to
each child via setStat() immediately after resolution. The ON_RESOLVE
refresh clears attrs, but setStat() repopulates it right after — same
server traffic either way. The optimization is purely client-side.

The FTP test remains and definitively proves the fix (82 LISTs without
the fix, 1 LIST with).

* Add comment.

---------

Co-authored-by: Gary Gregory <garydgregory@users.noreply.github.com>

Author: ilang

commons-vfs2-jackrabbit1/src/test/java/org/apache/commons/vfs2/provider/webdav/test/WebdavProviderTestCase.java

@@ -36,6 +36,9 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import java.util.Collections;
+
+import org.apache.commons.httpclient.Header;
 import org.apache.commons.io.file.PathUtils;
 import org.apache.commons.vfs2.AbstractProviderTestConfig;
 import org.apache.commons.vfs2.FileObject;
@@ -284,7 +287,25 @@ public FileObject getBaseTestFolder(final FileSystemManager manager) throws Exce
                 .getFileSystemConfigBuilder("webdav");
         final FileSystemOptions opts = new FileSystemOptions();
         builder.setRootURI(opts, uri);
-        return manager.resolveFile(uri, opts);
+        final FileObject baseFolder = manager.resolveFile(uri, opts);
+
+        // Disable HTTP persistent connections for the embedded Jackrabbit 1.x
+        // test server. Jackrabbit 1.x bundles Jetty 6.x which does not drain
+        // unconsumed request bodies before sending error responses (e.g. 404),
+        // violating HTTP/1.1 persistent connection requirements. This leaves
+        // stale request bytes on the connection that corrupt subsequent requests.
+        // See https://github.com/jetty/jetty.project/issues/651
+        // See https://github.com/jetty/jetty.project/issues/4117
+        final java.lang.reflect.Method getClient =
+                org.apache.commons.vfs2.provider.http.HttpFileSystem.class
+                        .getDeclaredMethod("getClient");
+        getClient.setAccessible(true);
+        ((org.apache.commons.httpclient.HttpClient) getClient.invoke(
+                baseFolder.getFileSystem())).getParams().setParameter(
+                        "http.default-headers",
+                        Collections.singletonList(new Header("Connection", "close")));
+
+        return baseFolder;
     }
 
     @Override

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileObject.java

@@ -1306,7 +1306,7 @@ public FileObject getParent() throws FileSystemException {
                 if (name == null) {
                     return null;
                 }
-                parent = fileSystem.resolveFile(name);
+                parent = fileSystem.resolveFileInternal(name);
             }
             return parent;
         }
@@ -1805,7 +1805,26 @@ private void removeChildrenCache() {
     }
 
     private FileObject resolveFile(final FileName child) throws FileSystemException {
-        return fileSystem.resolveFile(child);
+        return resolveFileInternal(child);
+    }
+
+    /**
+     * Resolves a file by name for internal navigation, skipping the
+     * {@link org.apache.commons.vfs2.CacheStrategy#ON_RESOLVE} refresh. Cache policy should only
+     * apply to external API calls, not internal VFS plumbing like
+     * {@link #getParent()}, {@link #getChildren()}, or symlink resolution.
+     * <p>
+     * Subclasses should use this instead of {@code getFileSystem().resolveFile()}
+     * when navigating to related files (parent, children, link targets).
+     * </p>
+     *
+     * @param name The FileName to resolve.
+     * @return The resolved FileObject.
+     * @throws FileSystemException if an error occurs.
+     * @since 2.11.0
+     */
+    protected FileObject resolveFileInternal(final FileName name) throws FileSystemException {
+        return fileSystem.resolveFileInternal(name);
     }
 
     /**

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileSystem.java

@@ -377,7 +377,7 @@ public FileObject getParentLayer() throws FileSystemException {
      */
     @Override
     public FileObject getRoot() throws FileSystemException {
-        return resolveFile(rootName);
+        return resolveFileInternal(rootName);
     }
 
     /**
@@ -525,11 +525,39 @@ public File replicateFile(final FileObject file, final FileSelector selector) th
      */
     @Override
     public FileObject resolveFile(final FileName name) throws FileSystemException {
-        return resolveFile(name, true);
+        return resolveFile(name, true, true);
     }
 
-    private synchronized FileObject resolveFile(final FileName name, final boolean useCache)
-            throws FileSystemException {
+    /**
+     * Resolves a file by name for internal navigation (e.g. {@code getParent()},
+     * {@code resolveFiles()} in {@code getChildren()}). Skips the
+     * {@link CacheStrategy#ON_RESOLVE} refresh, since cache policy should only
+     * apply to external API calls, not internal VFS plumbing. Without this,
+     * {@code ON_RESOLVE} triggers a refresh cascade where each child's
+     * {@code getParent()} refreshes the parent, clearing its cached state and
+     * causing O(N) redundant operations.
+     *
+     * @param name The FileName to resolve.
+     * @return The resolved FileObject.
+     * @throws FileSystemException if an error occurs.
+     * @since 2.11.0
+     */
+    synchronized FileObject resolveFileInternal(final FileName name) throws FileSystemException {
+        return resolveFile(name, true, false);
+    }
+
+    /**
+     * Resolves a file by name.
+     *
+     * @param name The FileName to resolve.
+     * @param useCache whether to use the file cache.
+     * @param applyRefreshPolicy whether to apply the {@link CacheStrategy#ON_RESOLVE} refresh.
+     *        Should be {@code true} for external API calls and {@code false} for internal navigation.
+     * @return The resolved FileObject.
+     * @throws FileSystemException if an error occurs.
+     */
+    private synchronized FileObject resolveFile(final FileName name, final boolean useCache,
+            final boolean applyRefreshPolicy) throws FileSystemException {
         if (!rootName.getRootURI().equals(name.getRootURI())) {
             throw new FileSystemException("vfs.provider/mismatched-fs-for-name.error", name, rootName,
                     name.getRootURI());
@@ -561,7 +589,8 @@ private synchronized FileObject resolveFile(final FileName name, final boolean u
         /*
           resync the file information if requested
          */
-        if (getFileSystemManager().getCacheStrategy().equals(CacheStrategy.ON_RESOLVE)) {
+        if (applyRefreshPolicy
+                && getFileSystemManager().getCacheStrategy().equals(CacheStrategy.ON_RESOLVE)) {
             file.refresh();
         }
         return file;

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

@@ -501,8 +501,29 @@ public FileObject[] getChildren() throws FileSystemException {
              * has C children, P parents, there will be (C * P) listings made with (C * (P + 1)) refreshes, when there
              * should really only be 1 listing and C refreshes.
              */
+            final boolean freshList = childMap == null;
             inRefresh.set(true);
-            return super.getChildren();
+            final FileObject[] result = super.getChildren();
+            // [VFS-862] Fix ON_RESOLVE triggering refresh on internal navigation.
+            // If a fresh LIST was done, propagate the new metadata to cached
+            // child objects so they reflect the latest directory listing.
+            if (freshList && childMap != null) {
+                synchronized (getFileSystem()) {
+                    for (final FileObject child : result) {
+                        final FtpFileObject ftpChild = (FtpFileObject) FileObjectUtils
+                                .getAbstractFileObject(child);
+                        final FTPFile entry = childMap.get(
+                                UriParser.decode(child.getName().getBaseName()));
+                        if (entry != null) {
+                            ftpChild.ftpFile = entry;
+                            ftpChild.linkDestination = null;
+                            ftpChild.mdtmSet = false;
+                            ftpChild.injectType(null);
+                        }
+                    }
+                }
+            }
+            return result;
         } finally {
             inRefresh.set(false);
         }
@@ -530,7 +551,7 @@ private FileObject getLinkDestination() throws FileSystemException {
             final FileName parent = getName().getParent();
             final FileName relativeTo = parent == null ? getName() : parent;
             final FileName linkDestinationName = getFileSystem().getFileSystemManager().resolveName(relativeTo, path);
-            linkDestination = getFileSystem().resolveFile(linkDestinationName);
+            linkDestination = resolveFileInternal(linkDestinationName);
         }
         return linkDestination;
     }

commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileObject.java

@@ -346,10 +346,15 @@ protected FileObject[] doListChildrenResolved() throws Exception {
                 continue;
             }
 
-            final FileObject fo = getFileSystem().resolveFile(getFileSystem().getFileSystemManager()
+            final FileObject fo = resolveFileInternal(getFileSystem().getFileSystemManager()
                     .resolveName(getName(), UriParser.encode(name), NameScope.CHILD));
 
-            ((SftpFileObject) FileObjectUtils.getAbstractFileObject(fo)).setStat(stat.getAttrs());
+            final SftpFileObject sftpChild = (SftpFileObject) FileObjectUtils.getAbstractFileObject(fo);
+            sftpChild.setStat(stat.getAttrs());
+            // Clear cached type so it re-evaluates from the fresh attrs.
+            // Without this, a cached type from a prior listing is returned
+            // without consulting the updated attrs.
+            sftpChild.injectType(null);
 
             children.add(fo);
         }

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

@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.vfs2.provider.ftp;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.time.Duration;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.commons.vfs2.CacheStrategy;
+import org.apache.commons.vfs2.FileDepthSelector;
+import org.apache.commons.vfs2.FileObject;
+import org.apache.commons.vfs2.FileSystemOptions;
+import org.apache.commons.vfs2.VfsTestUtils;
+import org.apache.commons.vfs2.impl.DefaultFileSystemManager;
+import org.apache.ftpserver.command.CommandFactoryFactory;
+import org.apache.ftpserver.command.impl.LIST;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests that {@link CacheStrategy#ON_RESOLVE} does not cause redundant FTP LIST
+ * commands due to internal VFS navigation triggering cache refreshes.
+ * <p>
+ * With {@code ON_RESOLVE}, every {@code fileSystem.resolveFile()} call triggers
+ * {@code refresh()} on the returned file. This is correct for external API calls
+ * (the user explicitly resolving a file), but internal navigation methods like
+ * {@code getParent()}, {@code getRoot()}, and child resolution in
+ * {@code getChildren()} also call {@code resolveFile()}, unintentionally refreshing
+ * cached files and clearing their state.
+ * </p>
+ * <p>
+ * This was always wasteful, but became a severe regression after
+ * {@code FtpFileObject.refresh()} was changed to unconditionally clear
+ * {@code childMap} (to fix stale cached state on never-attached files).
+ * That change turned the unnecessary refreshes into O(N) LIST commands:
+ * each child's {@code getParent()} refreshes the parent, clearing its
+ * {@code childMap}, forcing a new LIST for every child.
+ * </p>
+ */
+public class FtpGetChildrenListCommandTest {
+
+    private static final int FILE_COUNT = 50;
+    private static final AtomicInteger listCommandCount = new AtomicInteger();
+    private static final LIST defaultListCommand = new LIST();
+
+    private File[] testFiles;
+
+    @BeforeEach
+    public void setUp() throws Exception {
+        listCommandCount.set(0);
+
+        // Create test files in the FTP home directory.
+        final File testDir = new File(VfsTestUtils.getTestDirectory());
+        testFiles = new File[FILE_COUNT];
+        for (int i = 0; i < FILE_COUNT; i++) {
+            testFiles[i] = new File(testDir, "amplification-test-" + i + ".txt");
+            try (FileWriter w = new FileWriter(testFiles[i])) {
+                w.write("content-" + i);
+            }
+        }
+
+        // Start FTP server with a LIST command wrapper that counts invocations.
+        final CommandFactoryFactory cmdFactory = new CommandFactoryFactory();
+        cmdFactory.addCommand("LIST", (session, context, request) -> {
+            listCommandCount.incrementAndGet();
+            defaultListCommand.execute(session, context, request);
+        });
+        FtpProviderTest.setUpClass(VfsTestUtils.getTestDirectory(), null,
+                cmdFactory.createCommandFactory());
+    }
+
+    @AfterEach
+    public void tearDown() {
+        FtpProviderTest.tearDownClass();
+        if (testFiles != null) {
+            for (final File f : testFiles) {
+                f.delete();
+            }
+        }
+    }
+
+    private static FileSystemOptions createOptions() {
+        final FileSystemOptions options = new FileSystemOptions();
+        final FtpFileSystemConfigBuilder builder = FtpFileSystemConfigBuilder.getInstance();
+        builder.setUserDirIsRoot(options, true);
+        builder.setPassiveMode(options, true);
+        builder.setConnectTimeout(options, Duration.ofSeconds(10));
+        return options;
+    }
+
+    /**
+     * Verifies that {@code findFiles()} on a directory with {@code N} children
+     * issues exactly 1 LIST command. {@code findFiles()} internally calls
+     * {@code getType()} on each child via {@code traverse()}, which is where
+     * the cascade occurs.
+     * <p>
+     * With the bug, each child's {@code getType()} triggers a parent LIST via
+     * the ON_RESOLVE cascade, producing ~N LIST commands.
+     * Without the bug, the parent's {@code childMap} is reused across children,
+     * resulting in exactly 1 LIST.
+     * </p>
+     */
+    @Test
+    public void testGetChildrenIssuesSingleList() throws Exception {
+        try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
+            manager.addProvider("ftp", new FtpFileProvider());
+            manager.setCacheStrategy(CacheStrategy.ON_RESOLVE);
+            manager.init();
+
+            final FileSystemOptions options = createOptions();
+            final FileObject root = manager.resolveFile(
+                    FtpProviderTest.getConnectionUri(), options);
+
+            listCommandCount.set(0);
+            root.refresh();
+            final FileObject[] children = root.findFiles(new FileDepthSelector(1, 1));
+            assertNotNull(children, "findFiles should return results");
+            assertTrue(children.length >= FILE_COUNT,
+                    "Should find at least " + FILE_COUNT + " files, found: " + children.length);
+
+            final int listCount = listCommandCount.get();
+
+            // With the bug: LIST count ≈ FILE_COUNT (one per child due to cascade)
+            // Without the bug: exactly 1 LIST (the getChildren() directory listing)
+            assertEquals(1, listCount,
+                    "Scan should produce exactly 1 LIST command, but produced "
+                            + listCount + ". This indicates the ON_RESOLVE childMap "
+                            + "amplification bug: each child's getParent() re-resolution "
+                            + "refreshes the parent, clearing its childMap and forcing a new LIST.");
+        }
+    }
+}