Skip to content

Commit 7d73e52

Browse files
artembilangaryrussell
artembilan
authored and
garyrussell
committed
GH-8745: Add RFT.shouldMarkSessionAsDirty() (#8759)
* GH-8745: Add RFT.shouldMarkSessionAsDirty() Fixes #8745 Not all errors caught in the `RemoteFileTemplate.execute()` are fatal to mark session as dirty and physically close the target session in the cache * Introduce a `RemoteFileTemplate.shouldMarkSessionAsDirty()` to consult with an exception if it is really a fatal error to close the session in the end. * Override `shouldMarkSessionAsDirty()` in the `RemoteFileTemplate` implementations to check statuses of respective protocol errors **Cherry-pick to `6.1.x` & `6.0.x`** * * Fix tests for pool interaction * * Fix language in Javadocs * Add more `not dirty` statuses to `SftpRemoteFileTemplate` & `SmbRemoteFileTemplate`
1 parent 31de789 commit 7d73e52

File tree

7 files changed

+240
-43
lines changed

7 files changed

+240
-43
lines changed

Diff for: ‎spring-integration-file/src/main/java/org/springframework/integration/file/remote/RemoteFileTemplate.java

+25-21
Original file line numberDiff line numberDiff line change
@@ -451,27 +451,42 @@ public <T> T execute(SessionCallback<F, T> callback) {
451451
}
452452
return callback.doInSession(session);
453453
}
454-
catch (Exception e) {
455-
if (session != null) {
454+
catch (Exception ex) {
455+
if (session != null && shouldMarkSessionAsDirty(ex)) {
456456
session.dirty();
457457
}
458-
if (e instanceof MessagingException) { // NOSONAR
459-
throw (MessagingException) e;
458+
if (ex instanceof MessagingException messagingException) { // NOSONAR
459+
throw messagingException;
460460
}
461-
throw new MessagingException("Failed to execute on session", e);
461+
throw new MessagingException("Failed to execute on session", ex);
462462
}
463463
finally {
464464
if (!invokeScope && session != null) {
465465
try {
466466
session.close();
467467
}
468-
catch (Exception ignored) {
469-
this.logger.debug("failed to close Session", ignored);
468+
catch (Exception ex) {
469+
this.logger.debug("failed to close Session", ex);
470470
}
471471
}
472472
}
473473
}
474474

475+
/**
476+
* Determine whether {@link Session#dirty()} should be called
477+
* in the {@link #execute(SessionCallback)} when an exception is thrown from the callback.
478+
* By default, this method returns {@code true}.
479+
* Remote file protocol extensions can override this method to provide
480+
* a specific strategy against the thrown exception, e.g. {@code file not found} error
481+
* is not a signal that session is broken.
482+
* @param ex the exception to check if {@link Session} must be marked as dirty.
483+
* @return true if {@link Session#dirty()} should be called.
484+
* @since 6.0.8
485+
*/
486+
protected boolean shouldMarkSessionAsDirty(Exception ex) {
487+
return true;
488+
}
489+
475490
@Override
476491
public <T> T invoke(OperationsCallback<F, T> action) {
477492
Session<F> contextSession = this.contextSessions.get();
@@ -503,8 +518,7 @@ public <T, C> T executeWithClient(ClientCallback<C, T> callback) {
503518
private StreamHolder payloadToInputStream(Message<?> message) throws MessageDeliveryException {
504519
Object payload = message.getPayload();
505520
try {
506-
if (payload instanceof File) {
507-
File inputFile = (File) payload;
521+
if (payload instanceof File inputFile) {
508522
if (inputFile.exists()) {
509523
return new StreamHolder(
510524
new BufferedInputStream(new FileInputStream(inputFile)), inputFile.getAbsolutePath());
@@ -526,8 +540,7 @@ else if (payload instanceof byte[] || payload instanceof String) {
526540
else if (payload instanceof InputStream) {
527541
return new StreamHolder((InputStream) payload, "InputStream payload");
528542
}
529-
else if (payload instanceof Resource) {
530-
Resource resource = (Resource) payload;
543+
else if (payload instanceof Resource resource) {
531544
String filename = resource.getFilename();
532545
return new StreamHolder(resource.getInputStream(), filename != null ? filename : "Resource payload");
533546
}
@@ -619,16 +632,7 @@ else if (!directoryPath.endsWith(this.remoteFileSeparator)) {
619632
}
620633
}
621634

622-
private static final class StreamHolder {
623-
624-
private final InputStream stream;
625-
626-
private final String name;
627-
628-
StreamHolder(InputStream stream, String name) {
629-
this.stream = stream;
630-
this.name = name;
631-
}
635+
private record StreamHolder(InputStream stream, String name) {
632636

633637
}
634638

Diff for: ‎spring-integration-ftp/src/main/java/org/springframework/integration/ftp/session/FtpRemoteFileTemplate.java

+40-16
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,12 @@
2020

2121
import org.apache.commons.net.ftp.FTPClient;
2222
import org.apache.commons.net.ftp.FTPFile;
23+
import org.apache.commons.net.ftp.FTPReply;
2324

2425
import org.springframework.integration.file.remote.ClientCallback;
2526
import org.springframework.integration.file.remote.RemoteFileTemplate;
2627
import org.springframework.integration.file.remote.session.SessionFactory;
28+
import org.springframework.lang.Nullable;
2729
import org.springframework.messaging.MessagingException;
2830
import org.springframework.util.Assert;
2931
import org.springframework.util.ObjectUtils;
@@ -34,6 +36,7 @@
3436
*
3537
* @author Gary Russell
3638
* @author Artem Bilan
39+
*
3740
* @since 4.1
3841
*
3942
*/
@@ -82,29 +85,50 @@ protected <T> T doExecuteWithClient(final ClientCallback<FTPClient, T> callback)
8285
public boolean exists(final String path) {
8386
return doExecuteWithClient(client -> {
8487
try {
85-
switch (FtpRemoteFileTemplate.this.existsMode) {
86-
87-
case STAT:
88-
return client.getStatus(path) != null;
89-
90-
case NLST:
91-
String[] names = client.listNames(path);
92-
return !ObjectUtils.isEmpty(names);
93-
94-
case NLST_AND_DIRS:
95-
return FtpRemoteFileTemplate.super.exists(path);
96-
97-
default:
98-
throw new IllegalStateException("Unsupported 'existsMode': " +
99-
FtpRemoteFileTemplate.this.existsMode);
100-
}
88+
return switch (FtpRemoteFileTemplate.this.existsMode) {
89+
case STAT -> client.getStatus(path) != null;
90+
case NLST -> !ObjectUtils.isEmpty(client.listNames(path));
91+
case NLST_AND_DIRS -> FtpRemoteFileTemplate.super.exists(path);
92+
};
10193
}
10294
catch (IOException e) {
10395
throw new MessagingException("Failed to check the remote path for " + path, e);
10496
}
10597
});
10698
}
10799

100+
@Override
101+
protected boolean shouldMarkSessionAsDirty(Exception ex) {
102+
IOException ftpException = findIoException(ex);
103+
if (ftpException != null) {
104+
return isStatusDirty(ftpException.getMessage());
105+
}
106+
else {
107+
return super.shouldMarkSessionAsDirty(ex);
108+
}
109+
}
110+
111+
/**
112+
* Check if {@link IOException#getMessage()} is treated as fatal.
113+
* @param ftpErrorMessage the value from {@link IOException#getMessage()}.
114+
* @return true if {@link IOException#getMessage()} is treated as fatal.
115+
* @since 6.0.8
116+
*/
117+
protected boolean isStatusDirty(String ftpErrorMessage) {
118+
return !ftpErrorMessage.contains("" + FTPReply.FILE_UNAVAILABLE)
119+
&& !ftpErrorMessage.contains("" + FTPReply.FILE_NAME_NOT_ALLOWED);
120+
}
121+
122+
@Nullable
123+
private static IOException findIoException(Throwable ex) {
124+
if (ex == null || ex instanceof IOException) {
125+
return (IOException) ex;
126+
}
127+
else {
128+
return findIoException(ex.getCause());
129+
}
130+
}
131+
108132
/**
109133
* The {@link #exists(String)} operation mode.
110134
* @since 4.1.9

Diff for: ‎spring-integration-ftp/src/test/java/org/springframework/integration/ftp/session/FtpRemoteFileTemplateTests.java

+21-3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2014-2022 the original author or authors.
2+
* Copyright 2014-2023 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -34,6 +34,7 @@
3434
import org.springframework.integration.file.DefaultFileNameGenerator;
3535
import org.springframework.integration.file.remote.ClientCallbackWithoutResult;
3636
import org.springframework.integration.file.remote.SessionCallbackWithoutResult;
37+
import org.springframework.integration.file.remote.session.Session;
3738
import org.springframework.integration.file.remote.session.SessionFactory;
3839
import org.springframework.integration.file.support.FileExistsMode;
3940
import org.springframework.integration.ftp.FtpTestSupport;
@@ -53,9 +54,7 @@
5354
/**
5455
* @author Gary Russell
5556
* @author Artem Bilan
56-
*
5757
* @since 4.1
58-
*
5958
*/
6059
@SpringJUnitConfig
6160
@DirtiesContext
@@ -142,6 +141,25 @@ public void testConnectionClosedAfterExists() throws Exception {
142141
assertThat(pool.getActiveCount()).isEqualTo(0);
143142
}
144143

144+
@Test
145+
public void sessionIsNotDirtyOnNoSuchFileError() {
146+
Session<FTPFile> session = this.sessionFactory.getSession();
147+
session.close();
148+
149+
FtpRemoteFileTemplate template = new FtpRemoteFileTemplate(this.sessionFactory);
150+
151+
assertThatExceptionOfType(MessagingException.class)
152+
.isThrownBy(() -> template.rename("No_such_file1", "No_such_file2"))
153+
.withRootCauseInstanceOf(IOException.class)
154+
.withStackTraceContaining("553 : No such file or directory");
155+
156+
Session<FTPFile> newSession = this.sessionFactory.getSession();
157+
assertThat(TestUtils.getPropertyValue(newSession, "targetSession"))
158+
.isSameAs(TestUtils.getPropertyValue(session, "targetSession"));
159+
160+
newSession.close();
161+
}
162+
145163
@Configuration
146164
public static class Config {
147165

Diff for: ‎spring-integration-sftp/src/main/java/org/springframework/integration/sftp/session/SftpRemoteFileTemplate.java

+51
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,16 @@
1616

1717
package org.springframework.integration.sftp.session;
1818

19+
import java.util.List;
20+
1921
import org.apache.sshd.sftp.client.SftpClient;
22+
import org.apache.sshd.sftp.common.SftpConstants;
23+
import org.apache.sshd.sftp.common.SftpException;
2024

2125
import org.springframework.integration.file.remote.ClientCallback;
2226
import org.springframework.integration.file.remote.RemoteFileTemplate;
2327
import org.springframework.integration.file.remote.session.SessionFactory;
28+
import org.springframework.lang.Nullable;
2429

2530
/**
2631
* SFTP version of {@code RemoteFileTemplate} providing type-safe access to
@@ -34,6 +39,21 @@
3439
*/
3540
public class SftpRemoteFileTemplate extends RemoteFileTemplate<SftpClient.DirEntry> {
3641

42+
protected static final List<Integer> NOT_DIRTY_STATUSES = // NOSONAR
43+
List.of(
44+
SftpConstants.SSH_FX_NO_SUCH_FILE,
45+
SftpConstants.SSH_FX_NO_SUCH_PATH,
46+
SftpConstants.SSH_FX_INVALID_FILENAME,
47+
SftpConstants.SSH_FX_INVALID_HANDLE,
48+
SftpConstants.SSH_FX_FILE_ALREADY_EXISTS,
49+
SftpConstants.SSH_FX_DIR_NOT_EMPTY,
50+
SftpConstants.SSH_FX_NOT_A_DIRECTORY,
51+
SftpConstants.SSH_FX_EOF,
52+
SftpConstants.SSH_FX_CANNOT_DELETE,
53+
SftpConstants.SSH_FX_FILE_IS_A_DIRECTORY,
54+
SftpConstants.SSH_FX_FILE_CORRUPT
55+
);
56+
3757
public SftpRemoteFileTemplate(SessionFactory<SftpClient.DirEntry> sessionFactory) {
3858
super(sessionFactory);
3959
}
@@ -48,4 +68,35 @@ protected <T> T doExecuteWithClient(final ClientCallback<SftpClient, T> callback
4868
return execute(session -> callback.doWithClient((SftpClient) session.getClientInstance()));
4969
}
5070

71+
@Override
72+
protected boolean shouldMarkSessionAsDirty(Exception ex) {
73+
SftpException sftpException = findSftpException(ex);
74+
if (sftpException != null) {
75+
return isStatusDirty(sftpException.getStatus());
76+
}
77+
else {
78+
return super.shouldMarkSessionAsDirty(ex);
79+
}
80+
}
81+
82+
/**
83+
* Check if {@link SftpException#getStatus()} is treated as fatal.
84+
* @param status the value from {@link SftpException#getStatus()}.
85+
* @return true if {@link SftpException#getStatus()} is treated as fatal.
86+
* @since 6.0.8
87+
*/
88+
protected boolean isStatusDirty(int status) {
89+
return !NOT_DIRTY_STATUSES.contains(status);
90+
}
91+
92+
@Nullable
93+
private static SftpException findSftpException(Throwable ex) {
94+
if (ex == null || ex instanceof SftpException) {
95+
return (SftpException) ex;
96+
}
97+
else {
98+
return findSftpException(ex.getCause());
99+
}
100+
}
101+
51102
}

Diff for: ‎spring-integration-sftp/src/test/java/org/springframework/integration/sftp/session/SftpRemoteFileTemplateTests.java

+23-2
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222

2323
import org.apache.sshd.sftp.client.SftpClient;
2424
import org.apache.sshd.sftp.client.SftpVersionSelector;
25+
import org.apache.sshd.sftp.common.SftpException;
2526
import org.junit.jupiter.api.Test;
2627

2728
import org.springframework.beans.factory.BeanFactory;
@@ -37,6 +38,8 @@
3738
import org.springframework.integration.file.remote.session.SessionFactory;
3839
import org.springframework.integration.file.support.FileExistsMode;
3940
import org.springframework.integration.sftp.SftpTestSupport;
41+
import org.springframework.integration.test.condition.LogLevels;
42+
import org.springframework.integration.test.util.TestUtils;
4043
import org.springframework.messaging.MessageDeliveryException;
4144
import org.springframework.messaging.MessagingException;
4245
import org.springframework.messaging.support.GenericMessage;
@@ -51,9 +54,7 @@
5154
/**
5255
* @author Gary Russell
5356
* @author Artem Bilan
54-
*
5557
* @since 4.1
56-
*
5758
*/
5859
@SpringJUnitConfig
5960
@DirtiesContext
@@ -62,6 +63,7 @@ public class SftpRemoteFileTemplateTests extends SftpTestSupport {
6263
@Autowired
6364
private CachingSessionFactory<SftpClient.DirEntry> sessionFactory;
6465

66+
@LogLevels(level = "trace", categories = {"org.apache.sshd", "org.springframework.integration.sftp"})
6567
@Test
6668
public void testINT3412AppendStatRmdir() {
6769
SftpRemoteFileTemplate template = new SftpRemoteFileTemplate(sessionFactory);
@@ -162,6 +164,25 @@ public void renameWithOldSftpVersion() {
162164
oldVersionSession.close();
163165
}
164166

167+
@Test
168+
public void sessionIsNotDirtyOnNoSuchFileError() {
169+
Session<SftpClient.DirEntry> session = this.sessionFactory.getSession();
170+
session.close();
171+
172+
SftpRemoteFileTemplate template = new SftpRemoteFileTemplate(this.sessionFactory);
173+
174+
assertThatExceptionOfType(MessagingException.class)
175+
.isThrownBy(() -> template.list("No_such_dir"))
176+
.withRootCauseInstanceOf(SftpException.class)
177+
.withStackTraceContaining("(SSH_FX_NO_SUCH_FILE): No such file or directory");
178+
179+
Session<SftpClient.DirEntry> newSession = this.sessionFactory.getSession();
180+
assertThat(TestUtils.getPropertyValue(newSession, "targetSession"))
181+
.isSameAs(TestUtils.getPropertyValue(session, "targetSession"));
182+
183+
newSession.close();
184+
}
185+
165186
@Configuration
166187
public static class Config {
167188

0 commit comments

Comments
 (0)