Camel 22510 avoid leaking internal data types#21704
Conversation
# Conflicts: # components/camel-ai/camel-langchain4j-agent/src/main/java/org/apache/camel/component/langchain4j/agent/LangChain4jAgentProducer.java
|
please rebase this branch |
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
use junit6 instead of 5 |
|
Do we expect users will want to convert from |
No we dont use that anywhere else. And indeed its not a type anyone really use. Also this PR has an unit that doesn't really test the newly introduced converter |
|
@chandru9 can you eplain your reason for this PR |
|
@jamesnetherton : Good point. The BufferedImage converter was added to support direct image processing inputs, but since Camel routes typically handle images as byte[], InputStream, or WrappedFile, and we already detect the MIME type to create the appropriate ImageContent, introducing BufferedImage may not be necessary. To avoid introducing the AWT dependency, I can remove the BufferedImage converter and rely on byte[] / InputStream based conversion using the detected content-type. This should cover the intended use cases for image processing in routes. |
|
@davsclaus : PR intended to improve the usability of the langchain4j-agent component by enabling automatic body conversion to AiAgentBody for common input types such as files, streams,text.image etc. |
|
Thanks, removing that AWT BufferedImage is welcome. |
orpiske
left a comment
There was a problem hiding this comment.
Thanks @chandru9! I like the general idea of the PR, but, please I'd like some clarifications regarding the fixed usage of PNG format (maybe I'm not fully understanding it ... but I tried pointing out the parts that are unclear to me).
|
There are uncommitted changes |
|
@Croway , @davsclaus , @jamesnetherton and @orpiske |
gnodet
left a comment
There was a problem hiding this comment.
Thanks for the work on this @chandru9! The direction of using Camel's type converter system instead of Agent.processBody() is architecturally sound. A few items still need to be addressed before this can be merged:
Blocking
-
Uncommitted changes after build — as @davsclaus pointed out, the generated files don't match after a clean build. Please run:
mvn install -pl components/camel-ai/camel-langchain4j-agent -DskipTests mvn formatter:format impsort:sort -pl components/camel-ai/camel-langchain4j-agent
and commit the resulting changes.
-
Star import —
dev.langchain4j.data.message.*needs to be expanded to individual imports. This was already flagged by @davsclaus. Please expand it to the specific classes used:AudioContent,Content,ImageContent,PdfFileContent,TextContent,VideoContent. -
Agent.processBody()is now dead code — the PR removes the call fromLangChain4jAgentProducerbut leaves the method on theAgentinterface. Users who have customAgentimplementations overridingprocessBody()will find their overrides silently ignored. This is a breaking change. Please either:- Add
@Deprecatedto the method with a clear message pointing to the type converter approach, or - Remove the method entirely and document the change in the 4.19 upgrade guide.
- Add
-
Behavioral change for String payloads — previously, when the body was a plain String,
Agent.processBody()set the user message but did not create aContentobject on theAiAgentBody. Now,textToAiAgentBodyalways wraps the String in aTextContent. If any customAgent.chat()implementations check whethercontentis null to differentiate text-only vs multimodal messages, this will break them. Please verify this is intentional and compatible withAgentWithMemoryandAgentWithoutMemory.
Moderate
-
Inconsistent
exchange.getIn()vsexchange.getMessage()— the PR correctly switchesdetectMimeTypeFromHeaders()andbuildAiAgentBody()toexchange.getMessage(), but methods not touched by this PR (toAiAgentBody(),byteArrayToAiAgentBody(),detectMimeType()) still useexchange.getIn(). Since you're already in this file, it would be good to be consistent — but this can also be a follow-up. -
Test class named
*ITbut doesn't need external infrastructure —LangChain4jAgentAutoConversionITuses a mock agent and no testcontainers. Consider renaming to*Testso it runs withmvn testinstead of requiringmvn verify. -
shouldAutoConvertInputStreamtest — this sends aByteArrayInputStreamwithout setting a MIME type header.detectMimeTypeFromHeaders()will throwIllegalArgumentExceptionwhen no MIME type is found. Is this test expected to pass? -
Spurious blank line — there's an unnecessary blank line added at the beginning of
createContent()(around line 193 in the diff).
Nit
- Removed inline comments on cloud header constants — the comments like
// AWS S3,// Azure Blob Storageetc. were helpful for readability. Consider keeping them.
|
Hello @gnodet, thank you for the detailed review, |
|
Description
Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.