gnodet-bot commented on code in PR #29168:
URL: https://github.com/apache/camel/pull/29168#discussion_r4467486089
##########
core/camel-util/src/main/java/org/apache/camel/util/ObjectHelper.java:
##########
@@ -500,6 +500,25 @@ public static Class<?> loadSimpleType(String name) {
return Object[].class;
} else if ("java.lang.String[]".equals(name) ||
"String[]".equals(name)) {
return String[].class;
+ // JVM internal binary names for primitive arrays (e.g. [B for
byte[], [I for int[])
+ // These are produced by Class.getName() on array types and appear
in DataType when
+ // a byte[] body is tagged with new DataType(body.getClass())
+ } else if ("[B".equals(name)) {
+ return byte[].class;
+ } else if ("[S".equals(name)) {
Review Comment:
🐛 **Misplaced comment:** This 3-line comment block sits at 12-space
indentation inside the `String[]` branch body, _after_ the `return
String[].class;` statement — it's syntactically dead. The comment belongs just
before the `} else if ("[B".equals(name))` block so it introduces the new
section rather than dangling inside the previous one.
Move it to:
```suggestion
return String[].class;
// JVM internal binary names for primitive arrays (e.g. [B for
byte[], [I for int[])
// These are produced by Class.getName() on array types and appear
in DataType when
// a byte[] body is tagged with new DataType(body.getClass())
} else if ("[B".equals(name)) {
```
##########
core/camel-core/src/test/java/org/apache/camel/processor/transformer/TransformerContractTest.java:
##########
@@ -44,6 +45,49 @@ public boolean isUseRouteBuilder() {
return false;
Review Comment:
🎨 **Style:** Mixed import style — this non-static import is inconsistent
with the existing `import static
org.junit.jupiter.api.Assertions.assertEquals;` already in the file. Either
switch to static imports for the two `assertArrayEquals` calls:
```suggestion
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
```
Or drop the static import of `assertEquals` and use
`Assertions.assertEquals(...)` throughout — but mixing both styles in the same
file is confusing.
##########
core/camel-util/src/main/java/org/apache/camel/util/ObjectHelper.java:
##########
@@ -561,6 +580,18 @@ private static Class<?> doLoadClass(String name,
ClassLoader loader) {
}
}
+ // ClassLoader.loadClass() cannot resolve JVM internal array binary
names such as
+ // [Ljava.lang.String; or multi-dimensional arrays like [[B — only
Class.forName() can.
+ if (name.startsWith("[")) {
+ try {
+ return Class.forName(name, false, loader);
+ } catch (ClassNotFoundException e) {
+ if (logger().isTraceEnabled()) {
Review Comment:
⚠️ **Misleading comment:** The comment claims `[Ljava.lang.String;` is
handled here, but `normalizeClassName()` (called earlier in the load path)
strips the trailing `;`, so `Class.forName("[Ljava.lang.String", ...)` will
throw `ClassNotFoundException`. The test file itself acknowledges this
limitation with the note _"Note: loadClass() normalises the name, stripping
';'"_. Update the comment to match reality:
```suggestion
// ClassLoader.loadClass() cannot resolve JVM internal array binary
names such as
// [[B (multi-dimensional arrays) — only Class.forName() can.
// Note: [Ljava.lang.String; is NOT handled here because
normalizeClassName() strips
// the trailing ';'; object arrays use the "java.lang.String[]"
alias in loadSimpleType().
```
--
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]
