Croway commented on PR #29848:
URL: https://github.com/apache/camel/pull/29848#issuecomment-6854139570
_Claude Code on behalf of Croway_
Thanks, escaping at the `ChannelSftp` call is the right place. I ran the new
`SftpFileNameWithSpecialCharactersIT`: all 13 cases pass on this PR (`3a10aa0`)
and all 13 fail on main (`9235b3c`). I also ran a reproducer on both, using the
same embedded SFTP server (attached below). The points below come from that
run, except the test-coverage notes in 5, which are from reading the test.
**1. A name with `\`: the existence check and the delete look at different
files, and with this PR the delete can remove an unrelated file**
`lstat()` (the existence check) splits the name at the last `/` or `\` and
joins the parts with `/`, so `inbox/sub\file.txt` is checked as
`inbox/sub/file.txt`. The store (`normalizePath`) and the rename target
(`compactPath`) also treat `\` as a separator. `deleteFile` now removes the
literal `inbox/sub\file.txt`.
Producer with `tempPrefix=inprogress-` and `CamelFileName=sub\file.txt`,
sent twice. Before the second send, an unrelated file literally named
`inbox/sub\file.txt` exists:
| | 2nd send | files after |
|---|---|---|
| main | fails: `Cannot delete file: .../inbox/sub\file.txt`
(SSH_FX_NO_SUCH_FILE) | `inbox/sub/file.txt=first`,
`inbox/sub\file.txt=unrelated` |
| PR | succeeds | `inbox/sub/file.txt=second`; **`inbox/sub\file.txt`
deleted** |
Without the unrelated file, the second send fails with `Cannot delete file`
on both. Since every other step treats `\` as a separator, I think `deleteFile`
(or the producer target) should use the same normalized name before quoting,
rather than the literal one.
**2. The upgrade guide says `\` is now used as given, but it isn't, on
either main or this PR**
- Producer `CamelFileName=a\b.txt` is stored as `inbox/a/b.txt`.
- Consumer: a listed file literally named `inbox/a\b.txt` fails with `Cannot
change directory to: a` and stays where it is.
I'd limit the sentence (and the "as the other file based components do" in
the javadoc of `quotePathForJsch`) to `*` and `?`. Two more points on the
migration advice:
- `include`/`antInclude` are consumer options, so they don't help someone
who put a wildcard in the producer's `CamelFileName` header. For the producer,
the header just has to be the real name.
- A wildcard in the endpoint directory (`cd`/`ls` used to match it) isn't
mentioned.
**3. The 4.22.3 and 4.18.6 upgrade-guide entries belong in this PR**
CAMEL-26925 has fixVersions 4.18.6 and 4.22.3, and main already has the
`Upgrading from 4.22.2 to 4.22.3` and `Upgrading from 4.18.5 to 4.18.6`
sections. The project convention is "For backported changes, add the entry on
`main`", but the description says these entries will follow with the backports.
Separately: a wildcard `fileName` stops selecting files in a patch release. Is
that intended for the LTS branches?
**4. Not introduced here, but this PR makes it the common path: a stepwise
retrieve leaves the channel in the file's directory when `get` fails**
`retrieveFileToStreamInBody` and `retrieveFileToFileInLocalWorkDirectory`
change back to the previous directory only on success (no `finally`). Test: a
relative endpoint directory with
`useList=false&fileName=data-*.txt&ignoreFileNotFoundOrPermissionError=true`
and default stepwise, plus a `data-1.txt` file:
- main: the first poll matches `data-*.txt` and consumes `data-1.txt`.
- PR: the `get` of the literal `data-*.txt` fails, as the upgrade guide
describes, and the working directory stays in `.../inbox`. Every later poll
fails with `Cannot change directory to: target`, so a file literally named
`data-*.txt` created afterwards is never picked up.
Main has the same leak once any `get` fails, so this could be its own JIRA.
But it's exactly the setup the upgrade guide describes, so users who follow the
migration note will hit it.
**5. Tests**
- `chmodOnlyChangesStoredFile` only checks that the other file's mode is
unchanged. It should also check that the stored file got the mode, e.g.
`assertEquals(PosixFilePermissions.fromString("rwxrwxrwx"),
Files.getPosixFilePermissions(inbox.resolve("data-?.txt")))`, and compare the
permission sets instead of their `toString()`.
- Changed calls with no test: `rename` (consumer `move`/`preMove`,
`fileExist=Move`), `localWorkDirectory`, `fileExist=Append`, the checksum file
(`storeFileDirectly`), `chmodDirectory`, and the `cd` directory check
(`existDirCheckUsingLs=false`).
<details>
<summary>Reproducer (<code>Pr29848ReproIT</code>, in
<code>components/camel-ftp</code> next to
<code>SftpExistsFileIT</code>)</summary>
```java
package org.apache.camel.component.file.remote.sftp.integration;
import java.io.IOException;
import java.lang.reflect.Field;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;
import org.apache.camel.CamelExecutionException;
import org.apache.camel.Consumer;
import org.apache.camel.Endpoint;
import org.apache.camel.Exchange;
import org.apache.camel.builder.RouteBuilder;
import org.apache.camel.component.file.remote.RemoteFileOperations;
import org.apache.camel.component.mock.MockEndpoint;
import org.apache.camel.spi.PollingConsumerPollStrategy;
import org.apache.camel.test.junit6.CamelTestSupport;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.EnabledIf;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import static org.awaitility.Awaitility.await;
/**
* Reproducers for the review of PR #29848. Each test prints what it
observed (lines starting with REPRO).
*/
@EnabledIf(value =
"org.apache.camel.test.infra.ftp.services.embedded.SftpUtil#hasRequiredAlgorithms('src/test/resources/hostkey.pem')")
class Pr29848ReproIT extends CamelTestSupport {
@RegisterExtension
SftpExistsFileIT.CountingSftpEmbeddedService service = new
SftpExistsFileIT.CountingSftpEmbeddedService();
private Path base;
private Path inbox;
@BeforeEach
void createTestDirectory() throws IOException {
base =
Files.createTempDirectory(Files.createDirectories(service.getFtpRootDir()),
"repro");
inbox = Files.createDirectories(base.resolve("inbox"));
}
// finding: with tempPrefix the existence check uses the name literally,
the rename target is compacted with \ as separator
@ParameterizedTest
@ValueSource(strings = { "\\.\\./keep.txt", "sub\\file.txt" })
void secondSendWithTempPrefixAndBackslash(String name) throws Exception {
String uri = sftpUri("inbox") + "&tempPrefix=inprogress-";
template.sendBodyAndHeader(uri, "first", Exchange.FILE_NAME, name);
System.out.println("REPRO tempPrefix name=" + name + " after 1st
send: " + tree());
try {
template.sendBodyAndHeader(uri, "second", Exchange.FILE_NAME,
name);
System.out.println("REPRO tempPrefix name=" + name + " 2nd send
OK: " + tree());
} catch (CamelExecutionException e) {
System.out.println("REPRO tempPrefix name=" + name + " 2nd send
FAILED: " + rootMessage(e) + " ; " + tree());
}
}
// finding: the existence check finds sub/file.txt, the delete then
removes the file literally named sub\file.txt
@Test
void secondSendDeletesLiteralBackslashFile() throws Exception {
String uri = sftpUri("inbox") + "&tempPrefix=inprogress-";
template.sendBodyAndHeader(uri, "first", Exchange.FILE_NAME,
"sub\\file.txt");
Files.writeString(inbox.resolve("sub\\file.txt"), "unrelated");
System.out.println("REPRO victim before 2nd send: " + tree());
try {
template.sendBodyAndHeader(uri, "second", Exchange.FILE_NAME,
"sub\\file.txt");
System.out.println("REPRO victim 2nd send OK: " + tree());
} catch (CamelExecutionException e) {
System.out.println("REPRO victim 2nd send FAILED: " +
rootMessage(e) + " ; " + tree());
}
}
// finding: a producer name with \ is not stored literally
@Test
void producerBackslashName() throws Exception {
template.sendBodyAndHeader(sftpUri("inbox"), "stored",
Exchange.FILE_NAME, "a\\b.txt");
System.out.println("REPRO producer a\\b.txt stored as: " + tree());
}
// finding: a listed file whose name contains \ is not retrieved
literally
@Test
void consumerBackslashName() throws Exception {
Files.writeString(inbox.resolve("a\\b.txt"), "wanted");
MockEndpoint result = getMockEndpoint("mock:result");
MockEndpoint error = getMockEndpoint("mock:error");
context.addRoutes(new RouteBuilder() {
@Override
public void configure() {
onException(Exception.class).handled(true).to("mock:error");
from(sftpUri("inbox") +
"&delete=true&initialDelay=0&delay=60000&bridgeErrorHandler=true")
.convertBodyTo(String.class).to("mock:result");
}
});
await().atMost(10, TimeUnit.SECONDS)
.until(() -> result.getReceivedCounter() +
error.getReceivedCounter() > 0);
if (result.getReceivedCounter() > 0) {
Exchange e = result.getReceivedExchanges().get(0);
System.out.println("REPRO consumer a\\b.txt RECEIVED body=" +
e.getMessage().getBody()
+ " CamelFileName=" +
e.getMessage().getHeader(Exchange.FILE_NAME));
} else {
Exception ex =
error.getReceivedExchanges().get(0).getProperty(Exchange.EXCEPTION_CAUGHT,
Exception.class);
System.out.println("REPRO consumer a\\b.txt ERROR: " +
rootMessage(ex));
}
System.out.println("REPRO consumer a\\b.txt files after: " + tree());
}
// finding: stepwise retrieve does not change back to the original
directory when get fails
@Test
void stepwiseRetrieveFailureLeavesDirectory() throws Exception {
Files.writeString(inbox.resolve("data-1.txt"), "other");
// base is relative to the home directory of the server (the module
directory)
String relative = base.resolve("inbox").toString();
List<String> dirs = new CopyOnWriteArrayList<>();
List<String> failures = new CopyOnWriteArrayList<>();
context.getRegistry().bind("ps", new PollingConsumerPollStrategy() {
@Override
public boolean begin(Consumer consumer, Endpoint endpoint) {
dirs.add(currentDirectory(consumer));
return true;
}
@Override
public void commit(Consumer consumer, Endpoint endpoint, int
polledMessages) {
}
@Override
public boolean rollback(Consumer consumer, Endpoint endpoint,
int retryCounter, Exception e) {
failures.add(rootMessage(e));
return false;
}
});
MockEndpoint result = getMockEndpoint("mock:result");
context.addRoutes(new RouteBuilder() {
@Override
public void configure() {
from("sftp://localhost:"; + service.getPort() + "/" + relative
+ "?username=admin&password=admin&knownHostsFile=" +
service.getKnownHostsFile()
+
"&useList=false&fileName=data-*.txt&ignoreFileNotFoundOrPermissionError=true"
+ "&initialDelay=0&delay=200&pollStrategy=#ps")
.convertBodyTo(String.class).to("mock:result");
}
});
await().atMost(10, TimeUnit.SECONDS).until(() -> dirs.size() >= 4 ||
result.getReceivedCounter() > 0);
// the literal file now appears: is it picked up?
Files.writeString(inbox.resolve("data-*.txt"), "wanted");
await().atMost(10, TimeUnit.SECONDS).until(() -> dirs.size() >= 12);
System.out.println("REPRO stepwise relative dir=" + relative);
System.out.println("REPRO stepwise current directory at the start of
each poll: " + dirs.subList(0, 6));
System.out.println("REPRO stepwise poll failures: " +
failures.stream().distinct().toList());
System.out.println("REPRO stepwise received bodies: "
+ result.getReceivedExchanges().stream().map(x ->
x.getMessage().getBody()).distinct().toList());
}
private static String currentDirectory(Consumer consumer) {
try {
Class<?> c = consumer.getClass();
while (c != null) {
try {
Field f = c.getDeclaredField("operations");
f.setAccessible(true);
return ((RemoteFileOperations<?>)
f.get(consumer)).getCurrentDirectory();
} catch (NoSuchFieldException e) {
c = c.getSuperclass();
}
}
return "?";
} catch (Exception e) {
return "not connected (" + e.getMessage() + ")";
}
}
private String tree() throws IOException {
try (Stream<Path> s = Files.walk(base)) {
return s.filter(Files::isRegularFile).map(p -> {
try {
return base.relativize(p) + "=" + Files.readString(p);
} catch (IOException e) {
return base.relativize(p) + "=?";
}
}).sorted().toList().toString();
}
}
private static String rootMessage(Throwable t) {
StringBuilder sb = new StringBuilder();
while (t != null) {
sb.append(t.getClass().getSimpleName()).append(":
").append(t.getMessage()).append(" <- ");
t = t.getCause() == t ? null : t.getCause();
}
return sb.toString();
}
private String sftpUri(String directory) {
return "sftp://localhost:"; + service.getPort() + "/" + base + "/" +
directory
+ "?username=admin&password=admin&knownHostsFile=" +
service.getKnownHostsFile();
}
}
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
