gnodet-bot commented on code in PR #29273:
URL: https://github.com/apache/camel/pull/29273#discussion_r4489972667
##########
components/camel-ftp/src/main/java/org/apache/camel/component/file/remote/SftpOperations.java:
##########
@@ -1413,65 +1414,131 @@ public boolean existsFile(String name) throws
GenericFileOperationFailedExceptio
if (endpoint.isFastExistsCheck()) {
return fastExistsFile(name);
}
- // check whether a file already exists
- String directory = FileUtil.onlyPath(name);
- if (directory == null) {
- // assume current dir if no path could be extracted
- directory = ".";
- }
- String onlyName = FileUtil.stripPath(name);
+ return getFileAttributes(name) != null;
+ } finally {
+ lock.unlock();
+ }
+ }
+ /**
+ * Gets the attributes of a file with a single request, whatever the
number of files in its directory.
+ * <p/>
+ * These are the attributes of the path itself, as in a directory listing:
a symbolic link is not followed, so for a
+ * link they are the size and modification time of the link, and a
dangling link exists. When the server answers
+ * that request with another status than no such file, such as when it
does not permit or support it on the path,
+ * the attributes are taken from a listing of the directory of the file
instead.
+ *
+ * @param name the name of the file, with
its directory if any
+ * @return the attributes, or {@code
null} if there is no such file
+ * @throws GenericFileOperationFailedException if the attributes cannot be
read
+ */
+ public SftpATTRS getFileAttributes(String name) throws
GenericFileOperationFailedException {
Review Comment:
⚠️ **Medium:** `getFileAttributes` returns `SftpATTRS` (from
`com.jcraft.jsch`) as part of a `public` method signature. This couples the
public API of `SftpOperations` to JSch's type. If Camel ever switches SFTP
libraries again (as it did from JSch to mwiede-jsch), this signature becomes a
breaking change.
The only external caller is `SftpExclusiveReadLockCheck` in a sibling
package — it only needs `size()` and `lastModified()`. Consider either making
the method package-private, or returning a minimal Camel-owned record (e.g.
`record FileAttributes(long size, long lastModified) {}`) to decouple the API
from JSch's type hierarchy.
##########
components/camel-ftp/src/main/java/org/apache/camel/component/file/remote/SftpOperations.java:
##########
@@ -1413,65 +1414,131 @@ public boolean existsFile(String name) throws
GenericFileOperationFailedExceptio
if (endpoint.isFastExistsCheck()) {
return fastExistsFile(name);
}
- // check whether a file already exists
- String directory = FileUtil.onlyPath(name);
- if (directory == null) {
- // assume current dir if no path could be extracted
- directory = ".";
- }
- String onlyName = FileUtil.stripPath(name);
+ return getFileAttributes(name) != null;
+ } finally {
+ lock.unlock();
+ }
+ }
+ /**
+ * Gets the attributes of a file with a single request, whatever the
number of files in its directory.
+ * <p/>
+ * These are the attributes of the path itself, as in a directory listing:
a symbolic link is not followed, so for a
+ * link they are the size and modification time of the link, and a
dangling link exists. When the server answers
+ * that request with another status than no such file, such as when it
does not permit or support it on the path,
+ * the attributes are taken from a listing of the directory of the file
instead.
+ *
+ * @param name the name of the file, with
its directory if any
+ * @return the attributes, or {@code
null} if there is no such file
+ * @throws GenericFileOperationFailedException if the attributes cannot be
read
+ */
+ public SftpATTRS getFileAttributes(String name) throws
GenericFileOperationFailedException {
+ lock.lock();
+ try {
try {
- @SuppressWarnings("rawtypes")
- List files = channel.ls(directory);
- // can return either null or an empty list depending on FTP
servers
- if (files == null) {
- return false;
- }
- for (Object file : files) {
- ChannelSftp.LsEntry entry = (ChannelSftp.LsEntry) file;
- String existing = entry.getFilename();
- LOG.trace("Existing file: {}, target file: {}", existing,
name);
- existing = FileUtil.stripPath(existing);
- if (existing != null && existing.equals(onlyName)) {
- return true;
- }
- }
- return false;
+ return lstat(name);
} catch (SftpException e) {
- // or an exception can be thrown with id 2 which means file
does not
- // exists
- if (ChannelSftp.SSH_FX_NO_SUCH_FILE == e.id) {
- return false;
+ if (!isLstatUnanswered(e)) {
+ throw new
GenericFileOperationFailedException(e.getMessage(), e);
}
- // otherwise its a more serious error so rethrow
- throw new GenericFileOperationFailedException(e.getMessage(),
e);
+ LOG.debug("Cannot get the attributes of: {} due to: {}.
Listing its directory instead.", name,
+ e.getMessage());
}
+ return listFileAttributes(name);
} finally {
lock.unlock();
}
}
- protected boolean fastExistsFile(String name) throws
GenericFileOperationFailedException {
- LOG.trace("fastExistsFile({})", name);
+ /**
+ * Gets the attributes of a file with a single LSTAT request.
+ *
+ * @param name the name of the file, with its directory if any
+ * @return the attributes, or {@code null} if there is no
such file
+ * @throws SftpException if the server answers with another status, or the
request fails
+ */
+ private SftpATTRS lstat(String name) throws SftpException {
+ // the name starts after the last separator, either '/' or '\\', as in
the store and in the listing of the
+ // directory, and is joined to the directory with '/'. The directory
itself is sent as it is given
+ String directory = FileUtil.onlyPath(name);
+ String onlyName = FileUtil.stripPath(name);
+ String path;
+ if (directory == null) {
+ path = onlyName;
+ } else if (directory.endsWith("/")) {
+ path = directory + onlyName;
+ } else {
+ path = directory + "/" + onlyName;
+ }
+ try {
+ return channel.lstat(quotePathForJsch(path));
+ } catch (SftpException e) {
+ if (ChannelSftp.SSH_FX_NO_SUCH_FILE == e.id) {
+ return null;
+ }
+ throw e;
+ }
+ }
+
+ /**
+ * Whether the server answered an LSTAT request with a status that does
not tell whether the path exists, so that
+ * the directory is listed instead, as before: any status other than no
such file, which {@link #lstat(String)}
+ * already answers. A server may not permit or support the request on a
path that it lists, answer with the generic
+ * failure for a path that does not exist, or send a status of a later
version of the protocol.
+ */
+ private static boolean isLstatUnanswered(SftpException e) {
Review Comment:
💡 **Medium:** The `isLstatUnanswered` heuristic relies on an undocumented
JSch convention: `throwStatusError()` uses `SftpException(int, String)` (no
cause) for server-sent SSH_FXP_STATUS responses, while the outer catch wraps
I/O errors with `SftpException(int, String, Throwable)` (with cause). I
verified this holds in JSch 2.28.8 bytecode, but a future JSch upgrade could
silently break the assumption.
Consider adding a targeted unit test to pin this:
```java
// SftpOperationsLstatHeuristicTest
assertTrue(isLstatUnanswered(new SftpException(SSH_FX_PERMISSION_DENIED,
"denied")));
assertFalse(isLstatUnanswered(new SftpException(SSH_FX_FAILURE, "io", new
IOException())));
```
This way a JSch upgrade that changes the exception constructor convention
will be caught early by CI rather than silently falling back to listing.
--
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]
