robtimus/sftp-fs

Files.createDirectories does not work as expected with SFTP FS

Closed this issue · 5 comments

raphw commented

The implementation of Files.createDirectory for the SFTP file system does not throw a FileAlreadyExistsException if a folder already was created. This breaks the implementation of Files.createDirectories to be thrown. This is visible from the stack:

java.nio.file.FileSystemException: /some/folder: Failure
at com.github.robtimus.filesystems.sftp.DefaultFileSystemExceptionFactory.asFileSystemException(DefaultFileSystemExceptionFactory.java:132)
at com.github.robtimus.filesystems.sftp.DefaultFileSystemExceptionFactory.createCreateDirectoryException(DefaultFileSystemExceptionFactory.java:74)
at com.github.robtimus.filesystems.sftp.SSHChannelPool$Channel.mkdir(SSHChannelPool.java:516)
at com.github.robtimus.filesystems.sftp.SFTPFileSystem.createDirectory(SFTPFileSystem.java:381)
at com.github.robtimus.filesystems.sftp.SFTPPath.createDirectory(SFTPPath.java:175)
at com.github.robtimus.filesystems.sftp.SFTPFileSystemProvider.createDirectory(SFTPFileSystemProvider.java:262)
at java.nio.file.Files.createDirectory(Files.java:674)
at java.nio.file.Files.createAndCheckIsDirectory(Files.java:781)
at java.nio.file.Files.createDirectories(Files.java:767)
...
Caused by: com.jcraft.jsch.SftpException: Failure
at com.jcraft.jsch.ChannelSftp.throwStatusError(ChannelSftp.java:2873)
at com.jcraft.jsch.ChannelSftp.mkdir(ChannelSftp.java:2182)
at com.github.robtimus.filesystems.sftp.SSHChannelPool$Channel.mkdir(SSHChannelPool.java:514)
... 17 common frames omitted

The problem is that this check is never triggered due to the wrong exception (FileSystemException instead of FileAlreadyExistsException) being thrown:

try {
  createDirectory(dir, attrs);
} catch (FileAlreadyExistsException x) {
  if (!isDirectory(dir, LinkOption.NOFOLLOW_LINKS))
    throw x;
}

Can you check what the status code is of the original SftpException? JSch doesn't really have a long list of recognized error codes, but if the status code is generic enough I can update DefaultFileSystemExceptionFactory.createCreateDirectoryException to throw an FileAlreadyExistsException instead. If not you can sub class DefaultFileSystemExceptionFactory and provide that to the file system provider.

raphw commented

It's a generic error, unfortunately, nothing that indicates that a folder exists. I think the only way to handle this would be to catch the error and to call stat on the folder to see if it exists in case of exception where one would throw a FileAlreadyExistsException instead.

Decorating the exception does of course feel like a lot of extra work but assuming that most I/O operations execute successfully, I argue this should not be an overly costly change. At the moment, the createDirectories method is however not fullfiling its contract, this is why I would ideally change the behavior in your library rather then working around it.

Fixed in version 1.3.3

raphw commented

Awesome, I just started working on a pull request yesterday. Thank you for your work, I really appreciate it!

You're welcome.