gnodet-bot commented on code in PR #26757:
URL: https://github.com/apache/camel/pull/26757#discussion_r4079547634
##########
components/camel-bean/src/main/java/org/apache/camel/language/bean/BeanExpression.java:
##########
@@ -535,6 +542,40 @@ exchange, describeBean(holder, beanName, exchange),
return newResult;
}
+ /** Says that the bean is not a Map with that key, as null is a value a
key can hold. */
+ private static final Object NO_SUCH_KEY = new Object();
+
+ /**
+ * The value of the key on a Map bean when the method of that name does
not exist, so that ${body.sku} reads the sku
+ * of a map the way ${body[sku]} does - what a map means in jq, JavaScript
and Groovy too (CAMEL-24916).
+ * <p/>
+ * A method still wins: ${body.size} on a Map calls size() as before. A
name that is not a key still fails, so a
+ * misspelled field is still reported.
+ *
+ * @return the value of the key, or {@link #NO_SUCH_KEY} when this is not
that case
Review Comment:
💡 **Nit: incomplete Javadoc `@param` coverage**
The Javadoc documents `@return` but none of the four parameters.
`mapValue()` is private, so this is low-stakes, but the contract of each
parameter is non-obvious — particularly `cause` (why is it passed instead of
inspecting `resultExchange` internally?) and `holder` (why nullable?). Add four
`@param` tags before the `@return`:
```suggestion
* @param holder the bean holder; may be null (treated as no bean)
* @param exchange the current exchange, forwarded to {@link
BeanHolder#getBean(Exchange)}
* @param methodName the OGNL segment that failed as a method call (i.e.
the candidate key name)
* @param cause the exception thrown by the failed method invocation
* @return the value of the key, or {@link #NO_SUCH_KEY} when this is
not that case
```
##########
core/camel-core/src/test/java/org/apache/camel/language/simple/SimpleSyntaxHintsTest.java:
##########
@@ -144,16 +144,42 @@ public void testOgnlRuntimeMessages() {
}
@Test
- public void testOgnlDotOnAMapSaysToUseAKey() {
+ public void testOgnlDotOnAMapReadsTheKey() {
+ // CAMEL-24916: a map has no method type, so the key is what the dot
can mean
exchange.getIn().setBody(new
java.util.LinkedHashMap<>(java.util.Map.of("type", "order")));
- Exception e = assertThrows(Exception.class,
- () ->
context.resolveLanguage("simple").createExpression("${body.type}").evaluate(exchange,
- String.class));
- assertThat(e.getMessage()).contains("the value is a Map: a key is read
with [type], as in ${body[type]}");
+ assertEquals("order",
context.resolveLanguage("simple").createExpression("${body.type}").evaluate(exchange,
+ String.class));
assertEquals("order",
context.resolveLanguage("simple").createExpression("${body[type]}").evaluate(exchange,
String.class));
}
+ @Test
+ public void testOgnlDotOnAMapWithoutThatKeySaysToUseAKey() {
+ exchange.getIn().setBody(new
java.util.LinkedHashMap<>(java.util.Map.of("type", "order")));
+ Exception e = assertThrows(Exception.class,
+ () ->
context.resolveLanguage("simple").createExpression("${body.typo}").evaluate(exchange,
+ String.class));
+ assertThat(e.getMessage()).contains("the value is a Map: a key is read
with [typo], as in ${body[typo]}");
+ }
+
+ @Test
+ public void testAMethodOfAMapStillWins() {
+ exchange.getIn().setBody(new
java.util.LinkedHashMap<>(java.util.Map.of("size", "not the size")));
+ assertEquals("1",
context.resolveLanguage("simple").createExpression("${body.size}").evaluate(exchange,
+ String.class), "size() is a method of Map, so it still answers
before the key");
+ }
+
+ @Test
+ public void testOgnlDotOnANestedMapReadsTheKey() {
+ java.util.Map<String, Object> item = new java.util.LinkedHashMap<>();
+ item.put("sku", "CAMEL-MUG");
+ java.util.Map<String, Object> body = new java.util.LinkedHashMap<>();
+ body.put("item", item);
+ exchange.getIn().setBody(body);
+ assertEquals("CAMEL-MUG",
context.resolveLanguage("simple").createExpression("${body.item.sku}")
+ .evaluate(exchange, String.class));
+ }
Review Comment:
💡 **Missing test: null-valued map entry**
The new `containsKey`/`get` path in `mapValue()` has one subtle branch not
covered: a map that *has* the key but maps it to `null`. The sentinel
`NO_SUCH_KEY` is needed precisely to distinguish "key absent" from "key present
with value null", and the correctness of that distinction (`value !=
NO_SUCH_KEY` in the caller) deserves a test.
Suggested addition after `testOgnlDotOnANestedMapReadsTheKey`:
```java
@Test
public void testOgnlDotOnAMapWithNullValueReturnsNull() {
java.util.HashMap<String, Object> m = new java.util.HashMap<>();
m.put("sku", null);
exchange.getIn().setBody(m);
assertNull(context.resolveLanguage("simple").createExpression("${body.sku}").evaluate(exchange,
Object.class),
"a null value is a valid map entry; the expression must return
null, not throw");
}
```
--
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]
