CAMEL-24366: Add embeddingModel auto-embed to langchain4j-embeddingstore component - #25387
Conversation
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Claude Code on behalf of davsclaus
Nice feature addition — clean code, good test coverage, and well-documented. The header-takes-precedence design preserves backward compatibility nicely.
One question about a test dependency (see inline comment). Otherwise this looks good.
Note: this review covers project conventions and code structure. It does not replace specialized review tools such as CodeRabbit, Sourcery, or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
…ore component Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
53e3977 to
004cbbe
Compare
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 13 tested, 25 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean feature with good backward compatibility and solid test coverage. The resolveEmbedding() extraction is a nice factoring. One logic issue found in the null-body guard, plus a minor Javadoc note.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
| TextSegment textSegment = in.getHeader(LangChain4jEmbeddingsHeaders.TEXT_SEGMENT, TextSegment.class); | ||
|
|
||
| if (embedding == null && config.getEmbeddingModel() != null) { | ||
| String text = in.getBody(String.class); |
There was a problem hiding this comment.
The null-body check here is unconditional within the auto-embed block, but it shouldn't be. When a TextSegment header is already present (fetched at line 236), the body text isn't needed — embed(textSegment) at line 249 will use the header-provided segment. Currently, sending a message with a TextSegment header but a null/non-String body will throw IllegalArgumentException even though there's enough information to compute the embedding.
Consider moving the body-fetch and null-check inside the if (textSegment == null) block:
| String text = in.getBody(String.class); | |
| if (textSegment == null) { | |
| String text = in.getBody(String.class); | |
| if (text == null) { | |
| throw new IllegalArgumentException( | |
| "Message body cannot be converted to String for auto-embedding. " | |
| + "Either set the body to a text value or provide a pre-computed embedding via the " | |
| + LangChain4jEmbeddingsHeaders.EMBEDDING + " header."); | |
| } | |
| textSegment = TextSegment.from(text); | |
| } |
Note: the existing test addWithTextSegmentHeaderPreservedDuringAutoEmbed always supplies a non-null body, so it doesn't catch this edge case.
3 participants
fixes https://issues.apache.org/jira/browse/CAMEL-24366
Summary
embeddingModelproperty (autowired) to thelangchain4j-embeddingstorecomponentEmbeddingheaderCamelLangChain4jEmbeddingsEmbeddingheader always takes precedence when present, preserving backward compatibilityChanges
LangChain4jEmbeddingStoreConfiguration— newembeddingModelfield with@UriParamand@Metadata(autowired = true)LangChain4jEmbeddingStoreProducer— extracted sharedresolveEmbedding()helper used by bothadd()andsearch(), with null-body guard and TextSegment header preservationLangChain4jEmbeddingStoreAutoEmbedTest— 6 tests covering auto-embed ADD/SEARCH, header precedence, null body error, TextSegment preservation, and no-model fallbackTest plan