gnodet-bot commented on code in PR #26496:
URL: https://github.com/apache/camel/pull/26496#discussion_r4023982399
##########
components/camel-huawei/camel-huaweicloud-functiongraph/src/main/java/org/apache/camel/FunctionGraphUtils.java:
##########
@@ -34,7 +35,17 @@ private FunctionGraphUtils() {
*/
public static String extractJsonFieldAsString(String jsonString, String
fieldName) {
Gson gson = new Gson();
- return gson.fromJson(jsonString,
JsonObject.class).getAsJsonObject(fieldName).toString();
+ JsonObject root = gson.fromJson(jsonString, JsonObject.class);
+ if (root == null) {
+ return null;
+ }
+ JsonElement field = root.get(fieldName);
+ if (field == null || field.isJsonNull()) {
+ return null;
+ }
+ // a FunctionGraph 'body' is commonly a JSON-encoded string/primitive
(HTTP-triggered functions),
+ // not always an object; return the raw value for primitives and the
JSON text for objects/arrays
+ return field.isJsonPrimitive() ? field.getAsString() :
field.toString();
Review Comment:
⚠️ **Silent null body silently dropped into the exchange** —
`extractJsonFieldAsString` now returns `null` when `response.getResult()` is
`null` or has no `body` field. The caller at `FunctionGraphProducer.java:109`
does `exchange.getMessage().setBody(responseBody)` without any null check, so a
function that returns no body silently produces a `null` exchange body. Pre-PR
this crashed with NPE (visible); post-PR it silently clears the body, and any
downstream processor that expects a non-null body fails with a cryptic error
far from this call site.
At minimum, log a warning when the result is null:
```java
JsonObject root = gson.fromJson(jsonString, JsonObject.class);
if (root == null) {
LOG.warn("extractJsonFieldAsString: input JSON is null or
unparseable; returning null");
return null;
}
JsonElement field = root.get(fieldName);
if (field == null || field.isJsonNull()) {
LOG.warn("extractJsonFieldAsString: field '{}' absent or null in
response; returning null", fieldName);
return null;
}
// a FunctionGraph 'body' is commonly a JSON-encoded
string/primitive (HTTP-triggered functions),
// not always an object; return the raw value for primitives and the
JSON text for objects/arrays
return field.isJsonPrimitive() ? field.getAsString() :
field.toString();
```
Or — better — check for null in `FunctionGraphProducer` and throw a
meaningful exception rather than silently passing null downstream.
##########
components/camel-huawei/camel-huaweicloud-functiongraph/src/test/java/org/apache/camel/FunctionGraphUtilsTest.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel;
+
+import org.apache.camel.constants.FunctionGraphConstants;
+import org.apache.camel.models.ClientConfigurations;
+import org.apache.camel.test.junit6.CamelTestSupport;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class FunctionGraphUtilsTest extends CamelTestSupport {
+
+ // --- extractJsonFieldAsString: must not assume 'body' is a JSON object
(HTTP-triggered functions
+ // return it as a JSON-encoded string/primitive), and must tolerate an
absent/null field ---
+
+ @Test
+ public void extractObjectBodyReturnsItsJson() {
+ String result =
FunctionGraphUtils.extractJsonFieldAsString("{\"body\":{\"orderId\":1,\"ok\":true}}",
"body");
+ assertEquals("{\"orderId\":1,\"ok\":true}", result);
+ }
+
+ @Test
+ public void extractStringBodyReturnsTheRawString() {
+ // previously threw ClassCastException because getAsJsonObject was
forced on a string member
+ String result =
FunctionGraphUtils.extractJsonFieldAsString("{\"body\":\"hello world\"}",
"body");
+ assertEquals("hello world", result);
+ }
+
+ @Test
+ public void extractNumericBodyReturnsItsValue() {
+ String result =
FunctionGraphUtils.extractJsonFieldAsString("{\"body\":42}", "body");
+ assertEquals("42", result);
+ }
+
+ @Test
+ public void extractAbsentFieldReturnsNull() {
Review Comment:
**Missing test: `extractJsonFieldAsString(null, "body")` should return
null** — the PR adds a `root == null` guard, but there's no test that exercises
it. A function that returns no result (`response.getResult()` == null) would
hit this path in production. Add:
```java
@Test
public void extractNullJsonReturnsNull() {
assertNull(FunctionGraphUtils.extractJsonFieldAsString(null,
"body"));
}
```
##########
components/camel-huawei/camel-huaweicloud-functiongraph/src/test/java/org/apache/camel/FunctionGraphUtilsTest.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel;
+
+import org.apache.camel.constants.FunctionGraphConstants;
+import org.apache.camel.models.ClientConfigurations;
+import org.apache.camel.test.junit6.CamelTestSupport;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class FunctionGraphUtilsTest extends CamelTestSupport {
+
+ // --- extractJsonFieldAsString: must not assume 'body' is a JSON object
(HTTP-triggered functions
Review Comment:
**`CamelTestSupport` not needed for 4 of 5 tests** —
`extractObjectBodyReturnsItsJson`, `extractStringBodyReturnsTheRawString`,
`extractNumericBodyReturnsItsValue`, and `extractAbsentFieldReturnsNull` only
call a static utility method; they don't touch the Camel context at all.
Extending `CamelTestSupport` starts and stops a full Camel context for every
test, adding significant overhead for nothing. Split into two classes: a plain
JUnit 5 class (no superclass) for the static-method tests, and keep
`CamelTestSupport` only for `urnKeepsRegionWhenEndpointIsAlsoConfigured`.
##########
components/camel-huawei/camel-huaweicloud-functiongraph/src/test/java/org/apache/camel/FunctionGraphUtilsTest.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel;
+
+import org.apache.camel.constants.FunctionGraphConstants;
+import org.apache.camel.models.ClientConfigurations;
+import org.apache.camel.test.junit6.CamelTestSupport;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class FunctionGraphUtilsTest extends CamelTestSupport {
+
+ // --- extractJsonFieldAsString: must not assume 'body' is a JSON object
(HTTP-triggered functions
+ // return it as a JSON-encoded string/primitive), and must tolerate an
absent/null field ---
+
+ @Test
+ public void extractObjectBodyReturnsItsJson() {
+ String result =
FunctionGraphUtils.extractJsonFieldAsString("{\"body\":{\"orderId\":1,\"ok\":true}}",
"body");
+ assertEquals("{\"orderId\":1,\"ok\":true}", result);
+ }
+
+ @Test
+ public void extractStringBodyReturnsTheRawString() {
+ // previously threw ClassCastException because getAsJsonObject was
forced on a string member
+ String result =
FunctionGraphUtils.extractJsonFieldAsString("{\"body\":\"hello world\"}",
"body");
+ assertEquals("hello world", result);
+ }
+
+ @Test
+ public void extractNumericBodyReturnsItsValue() {
+ String result =
FunctionGraphUtils.extractJsonFieldAsString("{\"body\":42}", "body");
+ assertEquals("42", result);
+ }
+
+ @Test
+ public void extractAbsentFieldReturnsNull() {
+ // previously threw NullPointerException
+
assertNull(FunctionGraphUtils.extractJsonFieldAsString("{\"statusCode\":200}",
"body"));
+ }
+
+ // --- ClientConfigurations must keep the region for the invoke URN even
when 'endpoint' is also set ---
+
+ @Test
+ public void urnKeepsRegionWhenEndpointIsAlsoConfigured() {
+ FunctionGraphEndpoint endpoint = context.getEndpoint(
+
"hwcloud-functiongraph:invokeFunction?region=eu-west-101&endpoint=https://function.example.com";
+ +
"&projectId=proj-1&functionName=fn&functionPackage=pkg"
+ +
"&accessKey=ak&secretKey=sk&ignoreSslVerification=true",
+ FunctionGraphEndpoint.class);
+
+ ClientConfigurations clientConfigurations = new
ClientConfigurations(endpoint);
+
+ assertEquals("eu-west-101", clientConfigurations.getRegion(),
+ "region must be populated even when the client is initialized
from the endpoint");
+ // functionName/functionPackage are filled in by the producer at
invoke time; here we only assert the
+ // region segment is present (previously it was 'urn:fss:null:...'
whenever endpoint was configured)
+ String urn =
FunctionGraphUtils.composeUrn(FunctionGraphConstants.URN_FORMAT,
clientConfigurations);
+ assertTrue(urn.startsWith("urn:fss:eu-west-101:proj-1:function:"),
+ "the invoke URN must carry the region, was: " + urn);
+ }
+}
Review Comment:
**Missing negative test: endpoint-only (no region) should fail fast** — the
constructor refactor introduces a gap where providing `endpoint` but no
`region` silently succeeds (no throw), yet `composeUrn` will produce
`urn:fss:null:…`. Add a test that asserts this configuration throws
`IllegalArgumentException`:
```java
@Test
public void endpointWithoutRegionThrows() {
FunctionGraphEndpoint ep = context.getEndpoint(
"hwcloud-functiongraph:invokeFunction?endpoint=https://function.example.com";
+ "&projectId=proj-1&functionName=fn&functionPackage=pkg"
+ "&accessKey=ak&secretKey=sk&ignoreSslVerification=true",
FunctionGraphEndpoint.class);
assertThrows(IllegalArgumentException.class, () -> new
ClientConfigurations(ep),
"region is required even when endpoint is set");
}
```
(This test will currently **fail**, exposing the real remaining bug in the
constructor logic.)
--
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]
