[
https://issues.apache.org/jira/browse/CAMEL-26337?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18139156#comment-18139156
]
Claus Ibsen commented on CAMEL-26337:
-------------------------------------
_Claude Code on behalf of Claus Ibsen (davsclaus)_
[~fmariani] you are working on FTP at the moment, so can you pick this one up?
Below is an analysis of the code, which your agent can use as a starting point.
It was written by a coding agent and needs a human to check it.
h3. What is wrong
* {{FtpProcessStrategyFactory.getExclusiveReadLockStrategy()}} and
{{SftpProcessStrategyFactory.getExclusiveReadLockStrategy()}} (camel-ftp,
package {{org.apache.camel.component.file.remote.strategy}}) only handle
{{none}}/{{false}}, {{rename}} and {{changed}}. Every other value falls through
to {{return null}}, which means no read lock and no log line.
* The gap is wider than the report says:
** {{markerFile}} and {{fileLock}} are dropped silently too.
** {{camel-mina-sftp}} and {{camel-azure-files}} have the same fall-through.
Both extend {{RemoteFileEndpoint}}.
* Existing checks to mirror:
**
{{components/camel-smb/src/main/java/org/apache/camel/component/smb/SmbEndpoint.java}}
{{readLockCheck()}} (around line 187) already does what the reporter asks,
with {{valid = "none,rename,changed"}}.
**
{{components/camel-file/src/main/java/org/apache/camel/component/file/FileEndpoint.java}}
{{readLockCheck()}} (around line 158) throws
{{IllegalArgumentException("ReadLock invalid: ...")}}.
*
{{components/camel-ftp-common/src/main/java/org/apache/camel/component/file/remote/RemoteFileEndpoint.java}}
{{createConsumer()}} has no such check.
h3. Suggested fix
# Add a {{readLockCheck()}} to {{RemoteFileEndpoint.createConsumer()}}, like
the one in smb, with the valid set {{none,rename,changed}}. It throws
{{IllegalArgumentException("ReadLock invalid: <value>, must be one of: ...")}}.
#* One change covers ftp, ftps, sftp, mina-sftp and azure-files.
#* {{GenericFilePollingConsumer}} also goes through {{createConsumer()}}, so
{{pollEnrich}} is covered.
#* scp extends {{RemoteFileEndpoint}} as well, but it is producer-only.
#* *Open decision on {{readLock=false}}:* the remote factories treat it as
{{none}} today, but the smb and file checks reject it. Either keep accepting it
(safer for existing users), or reject it and say so in the upgrade guide.
# Add the missing excludes. {{FtpEndpoint}}, {{FtpsEndpoint}}, {{SmbEndpoint}}
and {{FilesEndpoint}} already exclude the idempotent release options.
{{SftpEndpoint}} (line 39) and {{MinaSftpEndpoint}} (line 45) do not, so
{{sftp.json}} and {{mina-sftp.json}} advertise four options that do nothing.
#* Add
{{readLockIdempotentReleaseAsync,readLockIdempotentReleaseAsyncPoolSize,readLockIdempotentReleaseDelay,readLockIdempotentReleaseExecutorService}}
to their {{@Metadata(excludeProperties=...)}}.
#* Then regenerate the catalog and the endpoint DSL.
# Docs: in the limitations section of the ftp, ftps and sftp component docs,
list the supported read locks ({{none}}, {{rename}}, {{changed}}) and say that
other values now fail. Regenerate the doc copies under
{{catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/docs/}}.
# Tests: there is no test today for the "ReadLock invalid" path in file, smb or
ftp.
#* In camel-ftp, test that creating a consumer on {{ftp:}} and {{sftp:}} with
{{readLock=idempotent-changed}} (and with {{markerFile}}) throws
{{IllegalArgumentException}}.
#* Add a similar test in camel-mina-sftp.
# Upgrade guide: add an entry to {{camel-4x-upgrade-guide-4_24.adoc}} for
camel-ftp, camel-sftp, camel-ftps, camel-mina-sftp and camel-azure-files.
Values that were ignored silently now fail when the consumer is created.
h3. Out of scope
* *Narrowing the catalog enum per scheme:* {{readLock}} and its 8-value enum
are inherited from {{GenericFileEndpoint}} (line 344).
{{EndpointSchemaGeneratorMojo}} only supports {{excludeProperties}} per
subclass, not overriding an inherited enum. This needs generator work (smb has
the same problem), so it is a follow-up JIRA if wanted.
* *Design:* CAMEL-25016 and the users@ thread from 2020 say the idempotent
locks are file-only by design. This fix keeps that design: it only stops
accepting these values silently.
* *Backport:* I suggest main only (4.24). Turning a silent no-op into a startup
failure does not belong in an LTS patch release.
[~pmauney] thanks for the report and the offer of a PR. Federico is already
working in this area, so he will take it.
> camel-ftp - readLock=idempotent, idempotent-changed and idempotent-rename are
> accepted on FTP/FTPS/SFTP endpoints but produce no read lock
> ------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: CAMEL-26337
> URL: https://issues.apache.org/jira/browse/CAMEL-26337
> Project: Camel
> Issue Type: Bug
> Components: camel-ftp, camel-sftp
> Affects Versions: 4.22.2, 4.23.0
> Reporter: Parker Mauney
> Priority: Minor
>
> The {{readLock}} option on the ftp, ftps and sftp endpoints shares its
> metadata with {{{}GenericFileEndpoint{}}}, so the published enum is the file
> component's: {{{}none, markerFile, fileLock, rename, changed, idempotent,
> idempotent-changed, idempotent-rename{}}}. The text marks the three
> {{idempotent*}} values "(only for file component)", but
> {{FtpProcessStrategyFactory}} and {{SftpProcessStrategyFactory}} only build a
> lock for {{{}none{}}}, {{rename}} and {{changed}} and return {{null}} for
> anything else, with no warning or validation error.
> So an SFTP consumer with {{readLock=idempotent-changed}} runs with no read
> lock at all and downloads files as soon as they are listed, including files
> still being uploaded (truncated at a 16 KB boundary) and files that are still
> 0 bytes. Nothing in the logs says the lock is missing.
> Reproducer (camel-4.23.0):
> {code:java}
> Map<String, Object> params = Map.of("readLock", "idempotent-changed");
> GenericFileProcessStrategy<FTPFile> strategy =
> new FtpProcessStrategyFactory().createGenericFileProcessStrategy(new
> DefaultCamelContext(), params);
> GenericFileExclusiveReadLockStrategy<FTPFile> lock =
> ((GenericFileProcessStrategySupport<FTPFile>)
> strategy).getExclusiveReadLockStrategy();
> assertNotNull(lock); // null; "changed" gives
> FtpChangedExclusiveReadLockStrategy
> {code}
> Same for {{{}SftpProcessStrategyFactory{}}}, {{idempotent}} and
> {{{}idempotent-rename{}}}.
> Expected: endpoint creation fails (or at least warns) when {{readLock}} names
> a strategy the remote component does not implement. It would also help to
> limit the enum in the ftp/ftps/sftp metadata to {{none, rename, changed}} so
> the docs, the endpoint DSL and {{camel validate}} stop advertising the
> file-only values.
> I know the idempotent locks are file-only by design (found in a users@ thread
> from Nov 2020, and the CAMEL-25016 PR notes). This is concerned with the
> silent acceptance and the component docs. Happy to send a PR for the
> validation if that is the preferred fix.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
