"Failed to delete" warning reported for each non-replaced file by SftpOutboundGateway

[mauromol]

Using Spring Integration 5.4.11

Describe the bug

When you setup a SftpOutboundGateway with a FileExistsMode set to REPLACE you'll get a series of warnings in the logs even if no replace is performed (because the local file does not exist).

To Reproduce

Create a SFTP outbound gateway like this and suppose you have the remote directory path in the message payload in the request channel:

<int-sftp:outbound-gateway
  session-factory="mySftpSessionFactory"
  request-channel="requestChannel"
  reply-channel="replyChannel"
  command="mget"
  command-options="-P -R"
  expression="payload.concat('/*')"
  local-directory-expression="'file:${java.io.tmpdir}/myappdir/'.concat(#remoteDirectory)"
  filter="myfilter"
  mode="REPLACE" />

<bean
  id="myFilter"
  class="org.springframework.integration.sftp.filters.SftpSimplePatternFileListFilter">
    <constructor-arg value="*.txt" />
    <property name="alwaysAcceptDirectories" value="true" />
</bean>

If a remote file does not exist locally, a warning gets printed in the logs:

Failed to delete /tmp/myappdir/remote/path/myFile.txt

Expected behavior

If the remote file does not currently exist locally, it does not need to be replaced, so it should not be deleted at all.

I think the problem lies in org.springframework.integration.file.remote.gateway.AbstractRemoteFileOutboundGateway.get(Message<?>, Session<F>, String, String, String, F):

FileExistsMode existsMode = this.fileExistsMode;
boolean appending = FileExistsMode.APPEND.equals(existsMode);
boolean exists = localFile.exists();
boolean replacing = FileExistsMode.REPLACE.equals(existsMode)
		|| (exists && FileExistsMode.REPLACE_IF_MODIFIED.equals(existsMode)
		&& localFile.lastModified() != getModified(fileInfo));
if (!exists || appending || replacing) {
  // ...
  if (replacing && !localFile.delete()) {
    this.logger.warn(() -> "Failed to delete " + localFile);
  }
// ...

I believe that the replacing boolean value should be turned to something like this:

boolean replacing = exists && (FileExistsMode.REPLACE.equals(existsMode)
		|| (FileExistsMode.REPLACE_IF_MODIFIED.equals(existsMode)
		&& localFile.lastModified() != getModified(fileInfo)));

That is: we don't need to replace if the local file does not exist... The localFile.delete() call will always fail if the local file does not exist.

[artembilan]

Thank you for the report, @mauromol !

Your feedback is always great.

I confirm your concern, however I think the proposed change is bit wrong.
See the next sentence in that method: if (!exists || appending || replacing) {
So, if replacing == false according your proposal, we never going to write into a local file.
I think that if (replacing && !localFile.delete()) { should be improved with a && exists instead.

WDYT?

Feel free to contribute a fix: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc and we will continue discussion on your PR.

[mauromol]

Hi Artem,
shouldn't we go into that block of code all the same since !exists is true (even if replacing is false)?

[artembilan]

Yeah... You are right. Sorry.
Your solution looks good.

[mauromol]

I may try to see if I can provide a PR myself... Do you have any deadline for this?

[artembilan]

Thanks. Not immediate.
Releases are somewhere next month, like November 16th.

[artembilan]

Hi @mauromol !

Any chances to hear from you back if you can make it into the PR for this issue?

Thanks