Fix refresh() not clearing cached state when file was never

attached #758

- Use longer lines
- Sort members
- Reduce vertical whitespace

Author: garydgregory

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

@@ -147,21 +147,6 @@ protected synchronized void doDetach() throws Exception {
         attrs = null;
     }
 
-    /**
-     * {@inheritDoc}
-     * <p>
-     * Clears the cached {@code attrs} regardless of the {@code attached} state.
-     * The {@code attrs} field can be populated without going through {@code attach()}
-     * (e.g. via {@code setStat()} in {@code doListChildrenResolved()}), so
-     * {@code doDetach()} alone may not clear it if the file was never attached.
-     * </p>
-     */
-    @Override
-    public synchronized void refresh() throws FileSystemException {
-        super.refresh();
-        attrs = null;
-    }
-
     /**
      * Returns the size of the file content (in bytes).
      */
@@ -511,6 +496,19 @@ private void putChannel(final ChannelSftp channel) {
         getAbstractFileSystem().putChannel(channel);
     }
 
+    /**
+     * {@inheritDoc}
+     * <p>
+     * Clears the cached {@code attrs} regardless of the {@code attached} state. The {@code attrs} field can be populated without going through {@code attach()}
+     * (e.g. via {@code setStat()} in {@code doListChildrenResolved()}), so {@code doDetach()} alone may not clear it if the file was never attached.
+     * </p>
+     */
+    @Override
+    public synchronized void refresh() throws FileSystemException {
+        super.refresh();
+        attrs = null;
+    }
+
     /**
      * Sets attrs from listChildrenResolved
      */

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

@@ -14,6 +14,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 package org.apache.commons.vfs2.provider.ftp;
 
 import java.io.File;
@@ -29,18 +30,25 @@
 import org.junit.jupiter.api.Test;
 
 /**
- * Tests that {@link org.apache.commons.vfs2.provider.AbstractFileObject#refresh()} correctly clears cached state
- * even when the file was never attached.
+ * Tests that {@link org.apache.commons.vfs2.provider.AbstractFileObject#refresh()} correctly clears cached state even when the file was never attached.
  * <p>
- * Regression test for the bug in {@code AbstractFileObject.detach()} where the {@code if (attached)} guard
- * prevented clearing cached fields ({@code type}, {@code parent}, {@code children}) on objects that were never
- * attached. Provider-specific cached fields like {@code FtpFileObject.childMap} can be populated without going
- * through {@code attach()} (e.g. via {@code getChildFile()} → {@code doGetChildren()}), so {@code refresh()}
- * must clear cached state regardless of the {@code attached} flag.
+ * Regression test for the bug in {@code AbstractFileObject.detach()} where the {@code if (attached)} guard prevented clearing cached fields ({@code type},
+ * {@code parent}, {@code children}) on objects that were never attached. Provider-specific cached fields like {@code FtpFileObject.childMap} can be populated
+ * without going through {@code attach()} (e.g. via {@code getChildFile()} → {@code doGetChildren()}), so {@code refresh()} must clear cached state regardless
+ * of the {@code attached} flag.
  * </p>
  */
 public class FtpRefreshCachedStateTest {
 
+    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;
+    }
+
     @BeforeEach
     public void setUp() throws Exception {
         FtpProviderTest.setUpClass(VfsTestUtils.getTestDirectory(), null, null);
@@ -51,27 +59,16 @@ public void tearDown() {
         FtpProviderTest.tearDownClass();
     }
 
-    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;
-    }
-
     /**
-     * Tests that {@code exists()} returns {@code false} for a file that was deleted from the FTP server,
-     * even when the parent's cached {@code childMap} was populated without {@code attach()}.
+     * Tests that {@code exists()} returns {@code false} for a file that was deleted from the FTP server, even when the parent's cached {@code childMap} was
+     * populated without {@code attach()}.
      * <p>
-     * The scenario: a file exists, its parent's {@code childMap} is populated (via {@code getChildFile()}
-     * during the first {@code exists()} call), the file is then deleted on the server, and the parent's
-     * {@code refresh()} must clear the stale {@code childMap} so the next {@code exists()} returns
-     * {@code false}.
+     * The scenario: a file exists, its parent's {@code childMap} is populated (via {@code getChildFile()} during the first {@code exists()} call), the file is
+     * then deleted on the server, and the parent's {@code refresh()} must clear the stale {@code childMap} so the next {@code exists()} returns {@code false}.
      * </p>
      * <p>
-     * Before the fix, {@code refresh()} → {@code detach()} skipped cache clearing because the parent was
-     * never explicitly attached ({@code attached == false}), leaving stale data in {@code childMap}.
+     * Before the fix, {@code refresh()} → {@code detach()} skipped cache clearing because the parent was never explicitly attached ({@code attached == false}),
+     * leaving stale data in {@code childMap}.
      * </p>
      */
     @Test
@@ -80,32 +77,24 @@ public void testExistsReturnsFalseAfterFileDeletedAndParentRefreshed() throws Ex
         final File ftpHome = new File(VfsTestUtils.getTestDirectory());
         final File tempFile = new File(ftpHome, "refresh-test-file.txt");
         tempFile.createNewFile();
-
         try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
             manager.addProvider("ftp", new FtpFileProvider());
             manager.init();
-
             final FileSystemOptions options = createOptions();
-            final FileObject file = manager.resolveFile(
-                FtpProviderTest.getConnectionUri() + "/refresh-test-file.txt", options);
-
+            final FileObject file = manager.resolveFile(FtpProviderTest.getConnectionUri() + "/refresh-test-file.txt", options);
             // Verify the file exists. This populates the parent's childMap
             // via setFTPFile() → getParent().getChildFile() → doGetChildren().
             Assertions.assertTrue(file.exists(), "File should exist on the FTP server");
-
             // Delete the file directly on the filesystem.
             Assertions.assertTrue(tempFile.delete(), "Temp file should be deleted");
-
             // Refresh the parent to clear its cached childMap, then refresh the file.
             // Before the fix, the parent's refresh() skipped clearing childMap because
             // the parent was never attached (attached == false).
             final FileObject parent = file.getParent();
             parent.refresh();
             file.refresh();
-
             // exists() must return false now that the file is deleted.
-            Assertions.assertFalse(file.exists(),
-                "exists() must return false after file is deleted and parent is refreshed");
+            Assertions.assertFalse(file.exists(), "exists() must return false after file is deleted and parent is refreshed");
         } finally {
             tempFile.delete();
         }

commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/sftp/SftpRefreshCachedStateTest.java

@@ -14,6 +14,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 package org.apache.commons.vfs2.provider.sftp;
 
 import java.io.File;
@@ -28,13 +29,11 @@
 import org.junit.jupiter.api.Test;
 
 /**
- * Tests that {@link SftpFileObject#refresh()} correctly clears cached {@code attrs}
- * even when the file was never attached.
+ * Tests that {@link SftpFileObject#refresh()} correctly clears cached {@code attrs} even when the file was never attached.
  * <p>
- * Regression test for the bug where {@code attrs} can be populated without going through
- * {@code attach()} (via {@code setStat()} in {@code doListChildrenResolved()}). Without
- * the fix, {@code refresh()} skips clearing {@code attrs} because {@code attached == false},
- * causing {@code exists()} to return stale results.
+ * Regression test for the bug where {@code attrs} can be populated without going through {@code attach()} (via {@code setStat()} in
+ * {@code doListChildrenResolved()}). Without the fix, {@code refresh()} skips clearing {@code attrs} because {@code attached == false}, causing
+ * {@code exists()} to return stale results.
  * </p>
  */
 public class SftpRefreshCachedStateTest {
@@ -50,37 +49,30 @@ public void tearDown() throws InterruptedException {
     }
 
     /**
-     * Tests that {@code exists()} returns {@code false} for a file that was deleted,
-     * when the child's {@code attrs} was populated via the parent's listing without
-     * {@code attach()}.
+     * Tests that {@code exists()} returns {@code false} for a file that was deleted, when the child's {@code attrs} was populated via the parent's listing
+     * without {@code attach()}.
      * <p>
-     * The key is that we do NOT call {@code exists()} or {@code getType()} on the child
-     * before deleting — only the parent's {@code getChildren()} populates the child's
-     * {@code attrs} via {@code setStat()} (without triggering {@code attach()}).
-     * Then after deleting the file, {@code refresh()} must clear the stale {@code attrs}.
+     * The key is that we do NOT call {@code exists()} or {@code getType()} on the child before deleting — only the parent's {@code getChildren()} populates the
+     * child's {@code attrs} via {@code setStat()} (without triggering {@code attach()}). Then after deleting the file, {@code refresh()} must clear the stale
+     * {@code attrs}.
      * </p>
      */
     @Test
     public void testExistsReturnsFalseAfterFileDeletedWithStaleAttrs() throws Exception {
         final File testDir = VfsTestUtils.getTestDirectoryFile();
         final File tempFile = new File(testDir, "sftp-refresh-test-file.txt");
         tempFile.createNewFile();
-
         try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
             manager.addProvider("sftp", new SftpFileProvider());
             manager.init();
-
             final FileSystemOptions options = new FileSystemOptions();
             final SftpFileSystemConfigBuilder builder = SftpFileSystemConfigBuilder.getInstance();
             builder.setStrictHostKeyChecking(options, "no");
-
             final String uri = SftpTestServerHelper.getConnectionUri();
-
             // List the parent's children. doListChildrenResolved() calls setStat()
             // on each child, populating attrs WITHOUT calling attach().
             final FileObject parent = manager.resolveFile(uri, options);
             final FileObject[] children = parent.getChildren();
-
             // Find our file among the children — its attrs is set but attached=false.
             FileObject file = null;
             for (final FileObject child : children) {
@@ -90,18 +82,14 @@ public void testExistsReturnsFalseAfterFileDeletedWithStaleAttrs() throws Except
                 }
             }
             Assertions.assertNotNull(file, "File should be found in parent listing");
-
             // Delete the file directly on the filesystem.
             Assertions.assertTrue(tempFile.delete(), "Temp file should be deleted");
-
             // Refresh the child. Before the fix, refresh() skipped clearing attrs
             // because attached == false (attrs was set via setStat, not attach).
             file.refresh();
-
             // exists() must return false. Before the fix, doGetType() found the stale
             // attrs (non-null), skipped statSelf(), and returned FILE instead of IMAGINARY.
-            Assertions.assertFalse(file.exists(),
-                "exists() must return false after file is deleted and refreshed");
+            Assertions.assertFalse(file.exists(), "exists() must return false after file is deleted and refreshed");
         } finally {
             tempFile.delete();
         }