Add Implement JsonRpcService #6289

patcher454 · 2025-06-23T11:30:36Z

Json RPC implementation must precede to implement the MCP mentioned in Support for MCP (Model Context Protocol) #6179

Modifications:

Add Implement JsonRpcService

Result:

Closes Implement JsonRpcService #6286

patcher454 · 2025-07-03T10:33:37Z

If you have a moment, I would appreciate a review.

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcError.java

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcRequest.java

ikhoon · 2025-10-23T03:00:12Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcRequest.java

+    /**
+     * Returns the JSON-RPC version.
+     */
+    String version();


Should we define the version values as an enum type?

I've added a JsonRpcVersion enum.
Could you please check if this aligns with what you intended?

ikhoon · 2025-10-23T03:10:32Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcRequest.java

+    /**
+     * Returns the parameters for the JSON-RPC method.
+     */
+    List<Object> params();


List<Object> would not express by-name parameters. We may introduce JsonRpcParameter to abstract by-position and by-name.

https://www.jsonrpc.org/specification#parameter_structures
by-name: params MUST be an Object,

I have implemented the suggestion to introduce JsonRpcParameter to abstract both by-position and by-name parameters.
Please review the changes.

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

ikhoon · 2025-10-23T04:59:13Z

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

+        // Notification
+        if (req.id() == null) {
+            if (handler != null) {
+                handler.handle(ctx, req);
+            }
+            return UnmodifiableFuture.completedFuture(null);
+        }


Do we return the future when handler.handle(ctx, req) completes.
In addition, it would be worth returning the status code defined in the MCP specification for compatibility.

Should we return 202 Accepted instead if the handler accepts the notification?

Should we return 400 Bad Request if the handler raises an exception?

If the input is a JSON-RPC response or notification:

If the server accepts the input, the server MUST return HTTP status code 202 Accepted with no body.

If the server cannot accept the input, it MUST return an HTTP error status code (e.g., 400 Bad Request). The HTTP response body MAY comprise a JSON-RPC error response that has no id.

https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#:~:text=if%20the%20server%20accepts%20the%20input%2C%20the%20server%20must%20return%20http%20status%20code%20202%20accepted%20with%20no%20body.

Do we return the future when handler.handle(ctx, req) completes.

I don't think this comment is addressed. My suggestion was:

if (handler != null) { return handler.handle(ctx, req) .thenApply(res -> null); } return UnmodifiableFuture.completedFuture(null);

codecov · 2025-10-24T01:49:10Z

Codecov Report

❌ Patch coverage is 69.91870% with 74 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (json-rpc@bd3e002). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
.../armeria/common/jsonrpc/DefaultJsonRpcRequest.java	63.46%	16 Missing and 3 partials ⚠️
...inecorp/armeria/server/jsonrpc/JsonRpcService.java	81.42%	6 Missing and 7 partials ⚠️
.../linecorp/armeria/common/jsonrpc/JsonRpcError.java	61.29%	7 Missing and 5 partials ⚠️
...armeria/server/jsonrpc/DefaultJsonRpcResponse.java	42.85%	7 Missing and 5 partials ⚠️
.../armeria/common/jsonrpc/SimpleJsonRpcResponse.java	26.66%	11 Missing ⚠️
...ecorp/armeria/common/jsonrpc/JsonRpcParameter.java	57.14%	2 Missing and 4 partials ⚠️
...inecorp/armeria/common/jsonrpc/JsonRpcRequest.java	92.85%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             json-rpc    #6289   +/-   ##
===========================================
  Coverage            ?   74.11%           
  Complexity          ?    23087           
===========================================
  Files               ?     2073           
  Lines               ?    86374           
  Branches            ?    11341           
===========================================
  Hits                ?    64012           
  Misses              ?    16930           
  Partials            ?     5432

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2025-10-24T02:52:46Z

🔍 Build Scan® (commit: `e4c355b`)

Job name	Status	Build Scan®
build-ubicloud-standard-16-jdk-8	✅	https://ge.armeria.dev/s/xalg4t7klt3mi
build-ubicloud-standard-16-jdk-21-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/en2p2neslk2qi
build-ubicloud-standard-16-jdk-17-min-java-17-coverage	❌ (failure)	https://ge.armeria.dev/s/t4gs6dimulw56
build-ubicloud-standard-16-jdk-17-min-java-11	❌ (failure)	https://ge.armeria.dev/s/ul76k2sv2bvae
build-ubicloud-standard-16-jdk-17-leak	❌ (failure)	https://ge.armeria.dev/s/6qhfz5xcgybsu
build-ubicloud-standard-16-jdk-11	❌ (failure)	https://ge.armeria.dev/s/uzdo4uprqlmu4
build-macos-latest-jdk-21	❌ (failure)	https://ge.armeria.dev/s/uldfhgprpaznw

ikhoon · 2025-10-24T02:58:09Z

core/src/main/java/com/linecorp/armeria/server/jsonrpc/DefaultJsonRpcResponse.java

+import com.linecorp.armeria.common.jsonrpc.JsonRpcError;
+import com.linecorp.armeria.common.jsonrpc.JsonRpcVersion;
+
+@UnstableApi


@UnstableApi is used in public and protected API.

Suggested change

@UnstableApi

ikhoon · 2025-10-24T02:59:52Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcRequest.java

+    static JsonRpcRequest of(JsonNode node) throws JsonProcessingException {
+        requireNonNull(node, "node");
+        checkArgument(node.isObject(), "node.isObject(): %s (expected: true)", node.isObject());
+        return JacksonUtil.newDefaultObjectMapper().treeToValue(node, DefaultJsonRpcRequest.class);


Let's declare JacksonUtil.newDefaultObjectMapper() in DefaultJsonRpcRequest as a static value to avoid additional allocations.

ikhoon · 2025-10-24T03:01:09Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/SimpleJsonRpcResponse.java

+import com.linecorp.armeria.common.annotation.Nullable;
+import com.linecorp.armeria.common.annotation.UnstableApi;
+
+@UnstableApi


Ditto.

Suggested change

@UnstableApi

ikhoon · 2025-10-24T03:04:29Z

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

+               .thenApply(JsonRpcService::parseRequestContentAsJson)
+               .thenApply(json -> dispatchRequest(ctx, json))
+               .exceptionally(e -> {


Micro optimization) Should we consolidate the three callbacks into CompletableFuture.handle() method?

I've refactored the code to consolidate the three callbacks into a single CompletableFuture.handle() method as you recommended.

Could you please take another look to see if this implementation aligns with what you had in mind?

ikhoon · 2025-10-24T03:19:43Z

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

+        if (rawRequest.isObject()) {
+            return handleUnaryRequest(ctx, rawRequest);
+        } else {
+            return HttpResponse.ofJson(HttpStatus.BAD_REQUEST, JsonRpcError.PARSE_ERROR);


For now, should we return the following message if the request is an array?

JsonRpcError.INVALID_REQUEST.withData("Batch requests are not supported by this server.");

ikhoon · 2025-10-24T03:21:06Z

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

+
+    private HttpResponse toHttpResponse(DefaultJsonRpcResponse response) {
+        if (response == null) {
+            return HttpResponse.of(HttpStatus.ACCEPTED, MediaType.PLAIN_TEXT_UTF_8, "");


We may not need a content-type and an empty string for accepted status.

Suggested change

return HttpResponse.of(HttpStatus.ACCEPTED, MediaType.PLAIN_TEXT_UTF_8, "");

return HttpResponse.of(HttpStatus.ACCEPTED);

I added the empty string because the MCP documentation specifies that the response for this case should have no body. If we simply return HttpResponse.of(HttpStatus.ACCEPTED), the response body will contain the string "202 Accepted".

Understood. We could make the response body empty by specifying response headers only.

HttpResponse.of(ResponseHeaders.of(HttpStatus.ACCEPTED));

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

ikhoon · 2025-10-24T03:29:35Z

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

+        // Notification
+        if (req.id() == null) {
+            if (handler != null) {
+                handler.handle(ctx, req);
+            }
+            return UnmodifiableFuture.completedFuture(null);
+        }


Do we return the future when handler.handle(ctx, req) completes.

I don't think this comment is addressed. My suggestion was:

if (handler != null) { return handler.handle(ctx, req) .thenApply(res -> null); } return UnmodifiableFuture.completedFuture(null);

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

ikhoon · 2025-10-24T03:33:11Z

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcServiceBuilder.java

+ */
+@UnstableApi
+public class JsonRpcServiceBuilder {
+    private final Map<String, JsonRpcHandler> methodHandlers = new HashMap<>();


We prefer immutable collections. Should we use ImmutableMap.Builder to collect the handlers?

… json_rpc

ikhoon

Mostly looks good.

ikhoon · 2025-10-24T06:57:46Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/AbstractJsonRpcResponse.java

+    /**
+    * Creates a new instance with result.
+    */
+    public AbstractJsonRpcResponse(Object result) {


Suggested change

public AbstractJsonRpcResponse(Object result) {

protected AbstractJsonRpcResponse(Object result) {

ikhoon · 2025-10-24T06:57:59Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/AbstractJsonRpcResponse.java

+    /**
+    * Creates a new instance with error.
+    */
+    public AbstractJsonRpcResponse(JsonRpcError error) {


Suggested change

public AbstractJsonRpcResponse(JsonRpcError error) {

protected AbstractJsonRpcResponse(JsonRpcError error) {

ikhoon · 2025-10-24T06:59:09Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/DefaultJsonRpcRequest.java

+        final JsonRpcRequest that = (JsonRpcRequest) obj;
+        return Objects.equals(id, that.id()) &&
+               Objects.equals(method, that.method()) &&
+               Objects.equals(params, that.params());


Question) Is JsonRpcVersion excluded intentionally from equals and hashCode?

I originally omitted it because the version was a constant,
Now that it's an enum, it definitely belongs in the equals() and hashCode() methods.

I've pushed a fix to include JsonRpcVersion in both equals() and hashCode(). Thanks

ikhoon · 2025-10-24T07:00:54Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcParameter.java

+    * Returns {@code true} if this Parameter is a Positional.
+    */
+    public boolean isPositional() {
+        return value instanceof List;
+    }
+
+    /**
+    * Returns {@code true} if this Parameter is a Named.


Suggested change

* Returns {@code true} if this Parameter is a Positional.

*/

public boolean isPositional() {

return value instanceof List;

}

/**

* Returns {@code true} if this Parameter is a Named.

* Returns {@code true} if this parameter is a positional.

*/

public boolean isPositional() {

return value instanceof List;

}

/**

* Returns {@code true} if this parameter is a pamed.

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcParameter.java

ikhoon · 2025-10-24T07:03:40Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcParameter.java

+    public static JsonRpcParameter of(Object parsedParams) {
+        checkArgument(parsedParams instanceof List || parsedParams instanceof Map,
+                "params type: %s (expected: List or Map)",
+                parsedParams != null ? parsedParams.getClass().getName() : "null");
+        return new JsonRpcParameter(parsedParams);
+    }


Should we add overload methods instead of Object type for type-safety?

public static JsonRpcParameter of(List<Object> ...) { } public static JsonRpcParameter of(Map<String, Object> ...) { }

ikhoon · 2025-10-24T07:07:43Z

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

+
+    @VisibleForTesting
+    CompletableFuture<DefaultJsonRpcResponse> invokeMethod(ServiceRequestContext ctx,
+                                                                   JsonRpcRequest req) {


core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcService.java

… json_rpc

ikhoon

Left only minor comments. 😉

ikhoon · 2025-10-30T12:47:05Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

+    public static boolean jsonRpcServiceContentLogging() {
+        return JSON_RPC_SERVICE_CONTENT_LOGGING;
+    }


I don't see we need this flag at the moment since annotatedServiceContentLogging was added to preserve the original behavior. Let's consider adding this option when there is a request for it.

ikhoon · 2025-10-30T12:52:01Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/DefaultJsonRpcRequest.java

+                "id type: %s (expected: Null or Number or String)",
+                Optional.ofNullable(id).map(Object::getClass).orElse(null));


Using Optional might be a bit too much for this case. The intention will be fully conveyed by the id itself. If you prefer type, let's use a ternary operator.

Suggested change

"id type: %s (expected: Null or Number or String)",

Optional.ofNullable(id).map(Object::getClass).orElse(null));

"id: %s (expected: Null or Number or String)", id);

Using id directly as suggested "id: %s (expected: Null or Number or String)", id) triggers a nullable warning.
As you also suggested, I've updated it to use a ternary operator instead. Thanks!

core/src/main/java/com/linecorp/armeria/common/jsonrpc/DefaultJsonRpcRequest.java

ikhoon · 2025-10-30T13:07:09Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcRequest.java

+    static JsonRpcRequest of(@Nullable Object id, String method, Map<String, Object> parameter) {
+        return new DefaultJsonRpcRequest(id,
+                requireNonNull(method, "method"),
+                requireNonNull(parameter, "param"));


Suggested change

requireNonNull(parameter, "param"));

requireNonNull(parameter, "parameter"));

ikhoon · 2025-10-30T13:08:08Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcRequest.java

+
+    /**
+    * Returns the ID of the JSON-RPC request.
+    * type must be Number or String


Suggested change

* type must be Number or String

* The type must be {@link Number} or {@link String}.

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcServiceBuilder.java

ikhoon · 2025-10-30T13:19:05Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcResponse.java

+import com.linecorp.armeria.common.annotation.UnstableApi;
+
+/**
+ * A Json-RPC.


Suggested change

* A Json-RPC.

* A JSON-RPC response.

ikhoon · 2025-10-30T13:19:26Z

core/src/main/java/com/linecorp/armeria/common/jsonrpc/JsonRpcRequest.java

+import com.linecorp.armeria.common.annotation.UnstableApi;
+
+/**
+ * A Json-RPC request.


Suggested change

* A Json-RPC request.

* A JSON-RPC request.

ikhoon · 2025-10-30T13:22:22Z

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcServiceBuilder.java

+     * Constructs a new {@link JsonRpcService}.
+     */
+    public JsonRpcService build() {
+        return new JsonRpcService(methodHandlers.build());


How about raising an exception if methodHandlers.isEmpty() == true?

ikhoon

Thanks for your hard work, @patcher454! 🚀👍

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcServiceBuilder.java

ikhoon

Still LGTM

patcher454 · 2025-10-31T09:27:37Z

@ikhoon

It looks like the :examples:athenz-example:test task is consistently failing in the build.
Is this a known issue, or is there something specific I need to fix or modify in my changes?
Any guidance would be appreciated. Thanks!

ikhoon · 2025-10-31T09:30:54Z

#6473 Please merge main branch to resolve it.

Draft

bd03700

ikhoon changed the base branch from main to json-rpc June 24, 2025 02:50

ikhoon mentioned this pull request Jun 24, 2025

Implement JsonRpcService #6286

Open

Fix Prevent blocking and handle Notifications correctly

b34715e

patcher454 force-pushed the json_rpc branch from 4103bdb to c37f67c Compare June 28, 2025 08:13

Fix errors

e2433cb

patcher454 force-pushed the json_rpc branch from c37f67c to e2433cb Compare June 28, 2025 08:20

patcher454 added 7 commits June 30, 2025 11:04

Fix correctly parse object parameters in JsonRpcRequest

01e1997

Fix Rename to SimpleJsonRpcResponse

f6e0ea5

Add jsonRpcResponse of parameter type checking

25ad292

Fix jsonRpcServie notification error

7a69c1f

Test jsonRpcRequest

2f65ec1

Test jsonRpcResponse

f1842b0

Test jsonRpcService

529171e

patcher454 marked this pull request as ready for review July 3, 2025 09:13

patcher454 requested review from ikhoon, jrhee17, minwoox and trustin as code owners July 3, 2025 09:13

patcher454 added 2 commits July 3, 2025 09:30

Fix jsonRpcRequest params allow a null element

9087bfb

Add requireNonNull

25b89d0

patcher454 added 8 commits July 6, 2025 07:06

Fix incorrect variable name

04622c7

Fix jsonproperty name

6475fdf

Fix Javadoc

544921a

Fix remove nullable

cb7d97b

Fix requireNonNull

3b41429

Fix Error Message

047a343

Fix Javadoc

eea7afa

Fix Property final

5a4cb2b

ikhoon requested changes Oct 23, 2025

View reviewed changes

ikhoon added the new feature label Oct 23, 2025

fix

ef47d49

patcher454 requested a review from ikhoon October 23, 2025 14:33

Merge branch 'json-rpc' into json_rpc

dd3f83f

ikhoon reviewed Oct 24, 2025

View reviewed changes

patcher454 added 2 commits October 24, 2025 06:19

fix

7317cad

Merge branch 'json_rpc' of https://github.com/patcher454/armeria into…

2b558b3

… json_rpc

patcher454 requested a review from ikhoon October 24, 2025 06:37

ikhoon reviewed Oct 24, 2025

View reviewed changes

patcher454 added 6 commits October 24, 2025 13:26

fix

4b2a6dc

fix: Restore indentation

cd1d60a

fix: missing

1cb9eaf

Merge branch 'json_rpc' of https://github.com/patcher454/armeria into…

1443994

… json_rpc

fix: DefaultJsonRpcResponse hashcode

56aaee4

fix: make parameters immutable and non-null

6606d68

patcher454 requested a review from ikhoon October 27, 2025 05:57

ikhoon reviewed Oct 30, 2025

View reviewed changes

patcher454 added 3 commits October 30, 2025 16:31

fix: remove json rpc flag

a0b743b

fix

52344f3

fix: remove remain flag

176a7ea

patcher454 requested a review from ikhoon October 30, 2025 17:37

ikhoon approved these changes Oct 31, 2025

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/jsonrpc/JsonRpcServiceBuilder.java Show resolved Hide resolved

fix: add javadoc

e4c355b

patcher454 requested a review from ikhoon October 31, 2025 08:29

ikhoon approved these changes Oct 31, 2025

View reviewed changes

	return HttpResponse.of(HttpStatus.ACCEPTED, MediaType.PLAIN_TEXT_UTF_8, "");
	return HttpResponse.of(HttpStatus.ACCEPTED);

	public AbstractJsonRpcResponse(Object result) {
	protected AbstractJsonRpcResponse(Object result) {

	public AbstractJsonRpcResponse(JsonRpcError error) {
	protected AbstractJsonRpcResponse(JsonRpcError error) {

		"id type: %s (expected: Null or Number or String)",
		Optional.ofNullable(id).map(Object::getClass).orElse(null));

	"id type: %s (expected: Null or Number or String)",
	Optional.ofNullable(id).map(Object::getClass).orElse(null));
	"id: %s (expected: Null or Number or String)", id);

	requireNonNull(parameter, "param"));
	requireNonNull(parameter, "parameter"));

	* type must be Number or String
	* The type must be {@link Number} or {@link String}.

Add Implement JsonRpcService #6289

Are you sure you want to change the base?

Add Implement JsonRpcService #6289

Uh oh!

Conversation

patcher454 commented Jun 23, 2025

Uh oh!

patcher454 commented Jul 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Build Scan® (commit: e4c355b)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikhoon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ikhoon left a comment

codecov bot commented Oct 24, 2025 •

edited

Loading

github-actions bot commented Oct 24, 2025 •

edited

Loading

🔍 Build Scan® (commit: `e4c355b`)