gnodet commented on PR #21916:
URL: https://github.com/apache/camel/pull/21916#issuecomment-4034232703
## Code Review
I reviewed this PR and found several issues:
### Bugs
**Bug 1: `?` prefix not stripped for single-segment paths**
In `pathString()`, `pathInteger()`, `pathBoolean()`, and `path()`, when the
path has no dot (single segment like `"?cheese"`), the `?` prefix is never
stripped from `sub`. The code does `jo.getString("?cheese")` instead of
`jo.getString("cheese")`. This means `path("?cheese")` returns `null` even when
key `"cheese"` exists in the JSON object.
**Bug 2: `exchangeProperty:` prefix not handled by `singleInputExpression`**
In `SimpleFunctionExpression.java`, the input parsing checks for
`exp.startsWith("exchangeProperty:")`, but
`ExpressionBuilder.singleInputExpression()` only handles `"property:"` (not
`"exchangeProperty:"`). If a user writes
`${simpleJsonpath(exchangeProperty:myProp, some.path)}`, it would silently fall
through to `variableExpression("exchangeProperty:myProp")`, looking for a
variable named `"exchangeProperty:myProp"` rather than the exchange property
`"myProp"`.
**Bug 3: Unsafe cast to `Jsonable` in `doPath()` for arrays of primitives**
In the `doPath()` method, array elements are cast to `(Jsonable)`:
```java
answer = (Jsonable) arr.getLast();
answer = (Jsonable) arr.get(pos);
```
If the JSON array contains primitive values (strings, numbers, booleans)
rather than nested objects, this will throw a `ClassCastException`. For
example, `{"tags": ["a", "b"]}` with path `tags[0]` would fail.
**Bug 4: Documentation example uses out-of-bounds index**
The AsciiDoc example references `library.book[2].year` but the array only
has indices 0 and 1.
### Minor Issues
- Trailing semicolon after class closing brace in `JsonObject.java` (`};`
instead of `}`)
- Misplaced Javadoc — the `splitWithDelimiters` / array bracket
documentation fragment appears inside the `JsonObject(Map)` constructor Javadoc
instead of on the `path()` method
- Significant code duplication across `pathString()`, `pathInteger()`,
`pathBoolean()`, `path()` — nearly identical boilerplate for optional marker
handling, dot splitting, and `doPath()` delegation. Could be refactored into a
single private helper.
### Test Gaps
- No test for single-segment optional path with an existing key (would
expose Bug 1)
- No test for arrays of primitives (would expose Bug 3)
- No test with `exchangeProperty:` input source
--
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]
