gnodet commented on PR #21994:
URL: https://github.com/apache/camel/pull/21994#issuecomment-4065427557
## Code Review
Thanks for the contribution! The idea of providing RAG convenience beans is
sound and follows the pattern established by `camel-qdrant`. Here's detailed
feedback:
### Critical: Existing tests should not be modified
The existing `MilvusCreateCollectionTest` and `MilvusComponentIT` previously
validated the raw Milvus component behavior by constructing SDK objects
directly. Replacing them with RAG helpers means:
- The tests now test RAG helpers + component together, not the component in
isolation
- If a RAG helper has a bug, the test would pass/fail for the wrong reason
- The original tests used `CollectionSchemaParam` with `withShardsNum(2)`
and `withEnableDynamicField(false)` — the RAG helper doesn't expose these
options, so the test is now covering a **different code path**
**Recommendation:** Revert the existing tests. Add separate unit tests for
each RAG helper class.
### Input validation and error handling
**`RAGInsert.java` / `RAGUpsert.java`** — The `textFieldMappings` parsing is
fragile:
```java
String[] pair = mapping.trim().split("=");
String variableName = pair[1].trim(); // ArrayIndexOutOfBoundsException if
no '='
```
A malformed mapping like `"content"` (missing `=value`) throws
`ArrayIndexOutOfBoundsException` with no useful context. Similarly,
`Integer.parseInt(dimension)` and `Long.parseLong(limit)` throw unhelpful
exceptions.
**`RAGInsert.java` / `RAGUpsert.java` / `RAGSearch.java`** —
`exchange.getIn().getBody(List.class)` returns `null` if the body can't be
converted, but the result is used without a null check, causing a
`NullPointerException` deeper in the SDK.
### Code duplication between RAGInsert and RAGUpsert
These two classes are nearly identical — the only differences are
`InsertParam` vs `UpsertParam` and `MilvusAction.INSERT` vs
`MilvusAction.UPSERT`. Consider extracting common logic into a shared base
class or utility method.
### Hardcoded values that should be configurable
- **`RAGSearch`** hardcodes `ConsistencyLevel.STRONG` — this should be a
configurable property
- **`RAGSearch`** doesn't expose an `offset` property (the original test
used `.withOffset(0L)`)
- **`RAGCreateIndex`** doesn't support setting an index name (the original
IT test used `.withIndexName("userFaceIndex")`)
- **`RAGCreateCollection`** — additional text fields always use `VarChar`
and the same `maxLength` as the primary field
### RAGDelete without filter may be dangerous
If `filter` is null (the default), the `DeleteParam` is built without an
expression. Depending on the SDK behavior, this could be a no-op or could
delete everything. Consider requiring a filter or logging a warning.
### Numeric properties as Strings
`dimension`, `textFieldMaxLength`, and `limit` are stored as `String` but
always parsed to `int`/`long`. Camel's property binding handles type conversion
automatically, so using native types would catch invalid values earlier. If
this is intentional for JBang/YAML compatibility (matching the Qdrant pattern),
a brief comment would help.
### Missing documentation
None of the new classes have Javadoc. For classes intended as public-facing
beans, at minimum class-level Javadoc explaining purpose and usage would be
helpful.
### Summary
The concept is good but the PR needs some work before it's ready:
1. Revert existing test modifications — add dedicated RAG helper tests
instead
2. Add input validation with descriptive error messages
3. Add null checks for exchange body
4. Make hardcoded values (consistency level, index name, offset) configurable
5. Reduce duplication between RAGInsert/RAGUpsert
6. Add Javadoc
--
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]
