davsclaus commented on code in PR #25881:
URL: https://github.com/apache/camel/pull/25881#discussion_r3898202437
##########
components/camel-file/src/main/java/org/apache/camel/component/file/GenericFileConverter.java:
##########
@@ -54,6 +54,11 @@ public static Object convertTo(Class<?> type, Exchange
exchange, Object value, T
GenericFile<?> file = (GenericFile<?>) value;
Object body = file.getBody();
if (body == null) {
+ LOG.warn(
+ "Cannot convert GenericFile '{}' to {} because the
file body has not been loaded."
+ + " The remote file content was not retrieved before
stream caching."
+ + " Check your SFTP/FTP consumer configuration
(localWorkDirectory, streamDownload).",
+ file.getFileName(), type.getName());
Review Comment:
This block is still present on `b950df73`, but the commit message and your
reply above state it was removed ("Removed the WARN from
`GenericFileConverter`... risked log flooding"). Since `convertTo()` here is a
`@Converter(fallback = true)` method, it's invoked speculatively for *any*
`GenericFile` conversion — `.convertBodyTo()`, bean parameter binding,
`getBody(X.class)`, etc. — not only `RemoteFile -> StreamCache`. The message
text ("...was not retrieved before stream caching") will be actively misleading
on those other paths. Please delete this block as described, and rely solely on
the `StreamCachingHelper` diagnostic below (once its own duplication is fixed).
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/StreamCachingHelper.java:
##########
@@ -67,6 +72,17 @@ private static StreamCache tryStreamCache(
StreamCache sc = strategy.cache(exchange);
if (sc != null) {
inMessage.setBody(sc);
+ } else {
+ // warn if a WrappedFile (e.g. RemoteFile) had a null
embedded body —
+ // this means the remote content was never retrieved
before stream caching ran
+ Object body = inMessage.getBody();
+ if (body instanceof WrappedFile<?> wf && wf.getBody() ==
null) {
+ LOG.warn(
+ "Stream caching skipped: the body is a
WrappedFile ({}) whose content has not been loaded."
+ + " The remote file was not retrieved before
stream caching."
+ + " Check your consumer configuration (e.g.
localWorkDirectory or streamDownload for FTP/SFTP).",
+ body.getClass().getSimpleName());
+ }
Review Comment:
Verified by running: I built this branch and ran
`GenericFileStreamCachingNullBodyTest` with the build cache disabled to force
real execution. `target/camel-core-test.log` shows this WARN firing twice for
one exchange through a 2-node route:
```
WARN StreamCachingHelper - Stream caching skipped: the body is a
WrappedFile...
WARN StreamCachingHelper - Stream caching skipped: the body is a
WrappedFile...
```
`StreamCachingAdvice.before()` (`CamelInternalProcessor.java:1318`) calls
`StreamCachingHelper.convertToStreamCache()` once per wrapped processor node
per exchange, and this branch isn't memoized, so the WARN repeats once per
route step for the same root cause. In a longer route this would flood the log
— the exact concern from the original review, just relocated rather than
resolved. Consider gating this to log once per exchange (e.g. check/set an
exchange property before logging) instead of once per node.
--
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]
