Include the request ID in all error responses when available. This prevents any clients from getting stuck awaiting for a specific response. The ID will not be available if the request is somehow malformed. (#10740)

spericas · web-flow · commit e818f5a64906 · 2025-10-01T13:05:49.000-04:00
diff --git a/webserver/jsonrpc/src/main/java/io/helidon/webserver/jsonrpc/JsonRpcRouting.java b/webserver/jsonrpc/src/main/java/io/helidon/webserver/jsonrpc/JsonRpcRouting.java
@@ -110,7 +110,7 @@ public void routing(HttpRules httpRules) {
                         if (errorHandler != null) {
                             Optional<JsonRpcError> userError = errorHandler.handle(req, jsonObject);
                             if (userError.isPresent()) {
-                                res.status(Status.OK_200).send(jsonRpcError(userError.get(), res, jsonObject));
+                                res.status(Status.OK_200).send(jsonRpcError(userError.get(), res, jsonObject.get("id")));
                             } else {
                                 res.status(Status.OK_200).send();
                             }
@@ -143,14 +143,14 @@ public void routing(HttpRules httpRules) {
 
                             // use error if returned, otherwise internal error
                             if (mappedError.isPresent()) {
-                                JsonObject jsonRpcError = jsonRpcError(mappedError.get(), res, null);
+                                JsonObject jsonRpcError = jsonRpcError(mappedError.get(), res, jsonObject.get("id"));
                                 res.status(Status.OK_200).send(jsonRpcError);
                                 return;
                             }
                         } catch (Throwable throwable2) {
                             // falls through
                         }
-                        sendInternalError(res);
+                        sendInternalError(res, jsonObject.get("id"));
                     }
                 } else if (jsonRequest instanceof JsonArray jsonArray) {
                     // we must receive at least one request
@@ -178,9 +178,10 @@ public void routing(HttpRules httpRules) {
                             // Use error if returned by error handler
                             if (errorHandler != null) {
                                 Optional<JsonRpcError> userError = errorHandler.handle(req, jsonObject);
-                                userError.ifPresent(e -> arrayBuilder.add(jsonRpcError(e, res, jsonObject)));
+                                userError.ifPresent(
+                                        e -> arrayBuilder.add(jsonRpcError(e, res, jsonObject.get("id"))));
                             } else {
-                                JsonObject verifyError = jsonRpcError(error, res, jsonObject);
+                                JsonObject verifyError = jsonRpcError(error, res, jsonObject.get("id"));
                                 arrayBuilder.add(verifyError);
                             }
                             continue;
@@ -202,14 +203,14 @@ public void routing(HttpRules httpRules) {
 
                                 // use error if returned, otherwise internal error
                                 if (mappedError.isPresent()) {
-                                    JsonObject jsonRpcError = jsonRpcError(mappedError.get(), res, null);
+                                    JsonObject jsonRpcError = jsonRpcError(mappedError.get(), res, jsonObject.get("id"));
                                     arrayBuilder.add(jsonRpcError);
                                     continue;
                                 }
                             } catch (Throwable throwable2) {
                                 // falls through
                             }
-                            JsonObject internalError = jsonRpcError(INTERNAL_ERROR_ERROR, res, null);
+                            JsonObject internalError = jsonRpcError(INTERNAL_ERROR_ERROR, res, jsonObject.get("id"));
                             arrayBuilder.add(internalError);
                         }
                     }
@@ -260,10 +261,10 @@ private JsonRpcError verifyJsonRpc(JsonObject object, Map<String, JsonRpcHandler
         }
     }
 
-    private JsonObject jsonRpcError(JsonRpcError error, ServerResponse res, JsonObject jsonObject) {
+    private JsonObject jsonRpcError(JsonRpcError error, ServerResponse res, JsonValue id) {
         JsonRpcResponse rpcRes = new JsonRpcResponseImpl(JsonValue.NULL, res);
-        if (jsonObject != null && jsonObject.containsKey("id")) {
-            rpcRes.rpcId(jsonObject.get("id"));
+        if (id != null) {
+            rpcRes.rpcId(id);
         }
         if (error.data().isEmpty()) {
             rpcRes.error(error.code(), error.message());
@@ -273,8 +274,8 @@ private JsonObject jsonRpcError(JsonRpcError error, ServerResponse res, JsonObje
         return rpcRes.asJsonObject();
     }
 
-    private void sendInternalError(ServerResponse res) {
-        JsonObject internalError = jsonRpcError(INTERNAL_ERROR_ERROR, res, null);
+    private void sendInternalError(ServerResponse res, JsonValue id) {
+        JsonObject internalError = jsonRpcError(INTERNAL_ERROR_ERROR, res, id);
         res.status(Status.OK_200).send(internalError);
     }
 
@@ -369,7 +370,7 @@ public void send() {
                 }
                 sendCalled.set(true);
             } catch (Exception e) {
-                sendInternalError(res);
+                sendInternalError(res, rpcId().orElse(null));
             }
         }
 
@@ -398,7 +399,7 @@ public void send() {
                     arrayBuilder.add(asJsonObject());
                 }
             } catch (Exception e) {
-                sendInternalError(res);
+                sendInternalError(res, rpcId().orElse(null));
             }
         }
     }
diff --git a/webserver/tests/jsonrpc/src/test/java/io/helidon/webserver/tests/jsonrpc/JsonRpcExceptionTest.java b/webserver/tests/jsonrpc/src/test/java/io/helidon/webserver/tests/jsonrpc/JsonRpcExceptionTest.java
@@ -98,8 +98,11 @@ void beforeEach() {
     @Test
     void testPing1() throws InterruptedException {
         try (JsonRpcClientResponse res = client.rpcMethod("ping1")
+                .rpcId(1)
                 .path("/rpc")
                 .submit()) {
+            assertThat(res.rpcId().isPresent(), is(true));
+            assertThat(res.rpcId().get().toString(), is("1"));
             assertThat(res.error().isPresent(), is(true));
             assertThat(res.error().get().code(), is(-10001));
             assertThat(res.error().get().message(), is(MESSAGE1));
@@ -110,8 +113,11 @@ void testPing1() throws InterruptedException {
     @Test
     void testPing2() throws InterruptedException {
         try (JsonRpcClientResponse res = client.rpcMethod("ping2")
+                .rpcId(1)
                 .path("/rpc")
                 .submit()) {
+            assertThat(res.rpcId().isPresent(), is(true));
+            assertThat(res.rpcId().get().toString(), is("1"));
             assertThat(res.error().isPresent(), is(true));
             assertThat(res.error().get().code(), is(-10002));
             assertThat(res.error().get().message(), is(MESSAGE2));
@@ -122,8 +128,11 @@ void testPing2() throws InterruptedException {
     @Test
     void testPing3() throws InterruptedException {
         try (JsonRpcClientResponse res = client.rpcMethod("ping3")
+                .rpcId(1)
                 .path("/rpc")
                 .submit()) {
+            assertThat(res.rpcId().isPresent(), is(true));
+            assertThat(res.rpcId().get().toString(), is("1"));
             assertThat(res.error().isPresent(), is(true));
             assertThat(res.error().get().code(), is(JsonRpcError.INTERNAL_ERROR));
         }

Original file line number	Diff line number	Diff line change
`@@ -110,7 +110,7 @@ public void routing(HttpRules httpRules) {`
`110`	`110`	`if (errorHandler != null) {`
`111`	`111`	`Optional<JsonRpcError> userError = errorHandler.handle(req, jsonObject);`
`112`	`112`	`if (userError.isPresent()) {`
`113`		`- res.status(Status.OK_200).send(jsonRpcError(userError.get(), res, jsonObject));`
	`113`	`+ res.status(Status.OK_200).send(jsonRpcError(userError.get(), res, jsonObject.get("id")));`
`114`	`114`	`} else {`
`115`	`115`	`res.status(Status.OK_200).send();`
`116`	`116`	`}`
`@@ -143,14 +143,14 @@ public void routing(HttpRules httpRules) {`
`143`	`143`
`144`	`144`	`// use error if returned, otherwise internal error`
`145`	`145`	`if (mappedError.isPresent()) {`
`146`		`- JsonObject jsonRpcError = jsonRpcError(mappedError.get(), res, null);`
	`146`	`+ JsonObject jsonRpcError = jsonRpcError(mappedError.get(), res, jsonObject.get("id"));`
`147`	`147`	`res.status(Status.OK_200).send(jsonRpcError);`
`148`	`148`	`return;`
`149`	`149`	`}`
`150`	`150`	`} catch (Throwable throwable2) {`
`151`	`151`	`// falls through`
`152`	`152`	`}`
`153`		`- sendInternalError(res);`
	`153`	`+ sendInternalError(res, jsonObject.get("id"));`
`154`	`154`	`}`
`155`	`155`	`} else if (jsonRequest instanceof JsonArray jsonArray) {`
`156`	`156`	`// we must receive at least one request`
`@@ -178,9 +178,10 @@ public void routing(HttpRules httpRules) {`
`178`	`178`	`// Use error if returned by error handler`
`179`	`179`	`if (errorHandler != null) {`
`180`	`180`	`Optional<JsonRpcError> userError = errorHandler.handle(req, jsonObject);`
`181`		`- userError.ifPresent(e -> arrayBuilder.add(jsonRpcError(e, res, jsonObject)));`
	`181`	`+ userError.ifPresent(`
	`182`	`+ e -> arrayBuilder.add(jsonRpcError(e, res, jsonObject.get("id"))));`
`182`	`183`	`} else {`
`183`		`- JsonObject verifyError = jsonRpcError(error, res, jsonObject);`
	`184`	`+ JsonObject verifyError = jsonRpcError(error, res, jsonObject.get("id"));`
`184`	`185`	`arrayBuilder.add(verifyError);`
`185`	`186`	`}`
`186`	`187`	`continue;`
`@@ -202,14 +203,14 @@ public void routing(HttpRules httpRules) {`
`202`	`203`
`203`	`204`	`// use error if returned, otherwise internal error`
`204`	`205`	`if (mappedError.isPresent()) {`
`205`		`- JsonObject jsonRpcError = jsonRpcError(mappedError.get(), res, null);`
	`206`	`+ JsonObject jsonRpcError = jsonRpcError(mappedError.get(), res, jsonObject.get("id"));`
`206`	`207`	`arrayBuilder.add(jsonRpcError);`
`207`	`208`	`continue;`
`208`	`209`	`}`
`209`	`210`	`} catch (Throwable throwable2) {`
`210`	`211`	`// falls through`
`211`	`212`	`}`
`212`		`- JsonObject internalError = jsonRpcError(INTERNAL_ERROR_ERROR, res, null);`
	`213`	`+ JsonObject internalError = jsonRpcError(INTERNAL_ERROR_ERROR, res, jsonObject.get("id"));`
`213`	`214`	`arrayBuilder.add(internalError);`
`214`	`215`	`}`
`215`	`216`	`}`
`@@ -260,10 +261,10 @@ private JsonRpcError verifyJsonRpc(JsonObject object, Map<String, JsonRpcHandler`
`260`	`261`	`}`
`261`	`262`	`}`
`262`	`263`
`263`		`- private JsonObject jsonRpcError(JsonRpcError error, ServerResponse res, JsonObject jsonObject) {`
	`264`	`+ private JsonObject jsonRpcError(JsonRpcError error, ServerResponse res, JsonValue id) {`
`264`	`265`	`JsonRpcResponse rpcRes = new JsonRpcResponseImpl(JsonValue.NULL, res);`
`265`		`- if (jsonObject != null && jsonObject.containsKey("id")) {`
`266`		`- rpcRes.rpcId(jsonObject.get("id"));`
	`266`	`+ if (id != null) {`
	`267`	`+ rpcRes.rpcId(id);`
`267`	`268`	`}`
`268`	`269`	`if (error.data().isEmpty()) {`
`269`	`270`	`rpcRes.error(error.code(), error.message());`
`@@ -273,8 +274,8 @@ private JsonObject jsonRpcError(JsonRpcError error, ServerResponse res, JsonObje`
`273`	`274`	`return rpcRes.asJsonObject();`
`274`	`275`	`}`
`275`	`276`
`276`		`- private void sendInternalError(ServerResponse res) {`
`277`		`- JsonObject internalError = jsonRpcError(INTERNAL_ERROR_ERROR, res, null);`
	`277`	`+ private void sendInternalError(ServerResponse res, JsonValue id) {`
	`278`	`+ JsonObject internalError = jsonRpcError(INTERNAL_ERROR_ERROR, res, id);`
`278`	`279`	`res.status(Status.OK_200).send(internalError);`
`279`	`280`	`}`
`280`	`281`
`@@ -369,7 +370,7 @@ public void send() {`
`369`	`370`	`}`
`370`	`371`	`sendCalled.set(true);`
`371`	`372`	`} catch (Exception e) {`
`372`		`- sendInternalError(res);`
	`373`	`+ sendInternalError(res, rpcId().orElse(null));`
`373`	`374`	`}`
`374`	`375`	`}`
`375`	`376`
`@@ -398,7 +399,7 @@ public void send() {`
`398`	`399`	`arrayBuilder.add(asJsonObject());`
`399`	`400`	`}`
`400`	`401`	`} catch (Exception e) {`
`401`		`- sendInternalError(res);`
	`402`	`+ sendInternalError(res, rpcId().orElse(null));`
`402`	`403`	`}`
`403`	`404`	`}`
`404`	`405`	`}`