Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SftpSession.exists() does not throw exception #3247

Closed
gary-x-li opened this issue Apr 12, 2020 · 5 comments · Fixed by #3248
Closed

SftpSession.exists() does not throw exception #3247

gary-x-li opened this issue Apr 12, 2020 · 5 comments · Fixed by #3248

Comments

@gary-x-li
Copy link

SftpSession.exists() method catches SftpException but ignores it. Why not throw a NestedIOException like other methods do?

In my code I use exists to check whether the remote file exists or not before I call remove to delete the file. In case when the logic inside exists throws an IOException (Pipe closed), it returns false, indicating the remote file does not exists.

Thanks,
Gary

@artembilan
Copy link
Member

We need to think why is that exception ignored so far because the session contract is:

	/**
	 * Check if the remote file or directory exists.
	 * @param path the remote path.
	 * @return {@code true} or {@code false} if remote path exists or not.
	 * @throws IOException an IO exception during remote interaction.
	 */
	boolean exists(String path) throws IOException;

Meanwhile you can consider a workaround for your use-case like:

SftpRemoteFileTemplate remoteFileTemplate = new SftpRemoteFileTemplate(sessionFactory());
remoteFileTemplate.<Boolean, ChannelSftp>executeWithClient(client -> {
				client.lstat(path);
				return true;
		});

So, if an exception is thrown from that lstat() call, it won't return false for you, but will be re-thrown to your code.

@artembilan artembilan added the status: waiting-for-triage The issue need to be evaluated and its future decided label Apr 13, 2020
@artembilan
Copy link
Member

I think our solution must be like this: https://stackoverflow.com/questions/13202789/how-to-check-directory-is-existed-before-creating-a-new-directory-in-jsch

Where a logical answer is present in the comment from Ton Bosma:

I think Exception is to eager, it can be more specific:

try{
      channel.stat(folderName); 
   } 
   catch (SftpException e ) { 
       if (e.id == ChannelSftp.SSH_FX_NO_SUCH_FILE ) {
             channel.mkdir(folderName);
       }
       else {
         throw e;
       } 
  }

Does it make sense?

WDYT, @garyrussell ?

Thanks

@artembilan artembilan self-assigned this Apr 13, 2020
@artembilan artembilan added this to the 5.3 RC1 milestone Apr 13, 2020
@artembilan artembilan added backport 5.2.x in: sftp type: enhancement and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Apr 13, 2020
@artembilan
Copy link
Member

Even if I treat this as a enhancement I think we need to back-port it to correct a behavior according contract expectations.

@garyrussell
Copy link
Contributor

I assume you mean...

if (e.id == ChannelSftp.SSH_FX_NO_SUCH_FILE ) {
    return false;
}
else {
    throw e;
}

Yes, I agree it should be back-ported even though it's a behavior change.

@artembilan
Copy link
Member

Yes, sorry, copy/paste artifact.

PR is on its way. Stay tuned...

artembilan added a commit to artembilan/spring-integration that referenced this issue Apr 13, 2020
Fixes spring-projects#3247

When there is no path on the SFTP server, a `ChannelSftp.SSH_FX_NO_SUCH_FILE`
error is returned in the thrown `SftpException`.

* Fix `SftpSession.exists()` to check for the `SSH_FX_NO_SUCH_FILE` to
return `false` and re-throw an exception otherwise
* Add mock test for `SftpSession.exists()`
* Add `org.mockito.AdditionalMatchers` to `checkstyle.xml` exclusions

**Cherry-pick to 5.2.x & 5.1.x**
garyrussell pushed a commit that referenced this issue Apr 14, 2020
* GH-3247: Fix `SftpSession.exists` for error code

Fixes #3247

When there is no path on the SFTP server, a `ChannelSftp.SSH_FX_NO_SUCH_FILE`
error is returned in the thrown `SftpException`.

* Fix `SftpSession.exists()` to check for the `SSH_FX_NO_SUCH_FILE` to
return `false` and re-throw an exception otherwise
* Add mock test for `SftpSession.exists()`
* Add `org.mockito.AdditionalMatchers` to `checkstyle.xml` exclusions

**Cherry-pick to 5.2.x & 5.1.x**

* * Add exists tests against Mina embedded server
garyrussell pushed a commit that referenced this issue Apr 14, 2020
* GH-3247: Fix `SftpSession.exists` for error code

Fixes #3247

When there is no path on the SFTP server, a `ChannelSftp.SSH_FX_NO_SUCH_FILE`
error is returned in the thrown `SftpException`.

* Fix `SftpSession.exists()` to check for the `SSH_FX_NO_SUCH_FILE` to
return `false` and re-throw an exception otherwise
* Add mock test for `SftpSession.exists()`
* Add `org.mockito.AdditionalMatchers` to `checkstyle.xml` exclusions

**Cherry-pick to 5.2.x & 5.1.x**

* * Add exists tests against Mina embedded server
garyrussell pushed a commit that referenced this issue Apr 14, 2020
* GH-3247: Fix `SftpSession.exists` for error code

Fixes #3247

When there is no path on the SFTP server, a `ChannelSftp.SSH_FX_NO_SUCH_FILE`
error is returned in the thrown `SftpException`.

* Fix `SftpSession.exists()` to check for the `SSH_FX_NO_SUCH_FILE` to
return `false` and re-throw an exception otherwise
* Add mock test for `SftpSession.exists()`
* Add `org.mockito.AdditionalMatchers` to `checkstyle.xml` exclusions

**Cherry-pick to 5.2.x & 5.1.x**

* * Add exists tests against Mina embedded server
garyrussell added a commit that referenced this issue Apr 14, 2020
garyrussell added a commit that referenced this issue Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants