Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Method parameter conversion error handling logic should preserve JSON-RPC id from the request #290

Open
cyb3r4nt opened this issue Jun 10, 2022 · 2 comments

Comments

@cyb3r4nt
Copy link
Contributor

cyb3r4nt commented Jun 10, 2022

During upgrade to newer 1.6.0 version an error related to invalid handling of JSON-RPC id was discovered.

Suppose you have a server with following interface:

public interface ServiceInterfaceWithParamNameAnnotation {
	String methodWithDifferentTypes(
		@JsonRpcParam("param1") Boolean booleanParam1,
		@JsonRpcParam("param2") Double doubleParam2,
		@JsonRpcParam("param3") UUID doubleParam3
	);
}

When a valid json, but with invalid parameters, is sent to server:

{"method":"methodWithDifferentTypes","id":4,"jsonrpc":"2.0","params":[true,3.14,"iWantToBeAnUUID"]} 

Then server responds with

 {"jsonrpc":"2.0","id":"null","error":{"code":-32700,"message":"JSON parse error"}} 

, where initial id value is lost.

This is also true for the batch requests, where multiple requests are sent at once.
It should be possible to find a failed request by id from the response.

The "JSON parse error" -32700 is not really correct here because:

  • input is a valid JSON-RPC object
  • method exists
  • parameter count is correct

Valid error probably should be the "Invalid params" -32602 , because wrong parameter was sent.

More detailed log (code line numbers may be wrong):

2022-06-11 01:33:06,940 DEBUG c.g.j.JsonRpcBasicServer:109 [Test worker] created server for interface interface com.googlecode.jsonrpc4j.server.JsonRpcServerAnnotatedParamTest$ServiceInterfaceWithParamNameAnnotation with handler class com.sun.proxy.$Proxy13 
2022-06-11 01:33:07,025 DEBUG c.g.j.JsonRpcBasicServer:420 [Test worker] Request: {"method":"methodWithDifferentTypes","id":4,"jsonrpc":"2.0","params":[true,3.14,"iWantToBeAnUUID"]} 
2022-06-11 01:33:07,075 DEBUG c.g.j.JsonRpcBasicServer:585 [Test worker] Invoking method: methodWithDifferentTypes with args [true, 3.14, "iWantToBeAnUUID"] 
2022-06-11 01:33:07,097 DEBUG c.g.j.JsonRpcBasicServer:1011 [Test worker] Response: {"jsonrpc":"2.0","id":"null","error":{"code":-32700,"message":"JSON parse error"}} 

Variant with batching:

2022-06-11 02:03:41,279 DEBUG c.g.j.JsonRpcBasicServer:109 [Test worker] created server for interface interface com.googlecode.jsonrpc4j.server.JsonRpcServerTest$ServiceInterface with handler class com.sun.proxy.$Proxy13 
2022-06-11 02:03:41,294 DEBUG c.g.j.JsonRpcBasicServer:109 [Test worker] created server for interface interface com.googlecode.jsonrpc4j.server.JsonRpcServerAnnotatedParamTest$ServiceInterfaceWithParamNameAnnotation with handler class com.sun.proxy.$Proxy28 
2022-06-11 02:03:41,456 DEBUG c.g.j.JsonRpcServer:115 [Test worker] Handling HttpServletRequest org.springframework.mock.web.MockHttpServletRequest@1578104e 
2022-06-11 02:03:41,500 DEBUG c.g.j.JsonRpcBasicServer:303 [Test worker] Handling 3 requests 
2022-06-11 02:03:41,501 DEBUG c.g.j.JsonRpcBasicServer:420 [Test worker] Request: {"method":"methodWithDifferentTypes","id":1,"jsonrpc":"2.0","params":[true,3.14,"cbddeb26-ea0c-48f9-adeb-5b28158fc71c"]} 
2022-06-11 02:03:41,523 DEBUG c.g.j.JsonRpcBasicServer:585 [Test worker] Invoking method: methodWithDifferentTypes with args [true, 3.14, "cbddeb26-ea0c-48f9-adeb-5b28158fc71c"] 
2022-06-11 02:03:41,550 DEBUG c.g.j.JsonRpcBasicServer:602 [Test worker] Invoked method: methodWithDifferentTypes, result passResult1 
2022-06-11 02:03:41,557 DEBUG c.g.j.JsonRpcBasicServer:420 [Test worker] Request: {"method":"methodWithDifferentTypes","id":2,"jsonrpc":"2.0","params":["truth",3.14,"e43f3ef3-88c4-4ded-9be0-8efaecd3c1dc"]} 
2022-06-11 02:03:41,557 DEBUG c.g.j.JsonRpcBasicServer:585 [Test worker] Invoking method: methodWithDifferentTypes with args ["truth", 3.14, "e43f3ef3-88c4-4ded-9be0-8efaecd3c1dc"] 
2022-06-11 02:03:41,559 DEBUG c.g.j.JsonRpcBasicServer:420 [Test worker] Request: {"method":"methodWithDifferentTypes","id":3,"jsonrpc":"2.0","params":[true,3.14,"cbddeb26-ea0c-48f9-adeb-5b28158fc71c"]} 
2022-06-11 02:03:41,559 DEBUG c.g.j.JsonRpcBasicServer:585 [Test worker] Invoking method: methodWithDifferentTypes with args [true, 3.14, "cbddeb26-ea0c-48f9-adeb-5b28158fc71c"] 
2022-06-11 02:03:41,560 DEBUG c.g.j.JsonRpcBasicServer:602 [Test worker] Invoked method: methodWithDifferentTypes, result passResult3 
2022-06-11 02:03:41,561 DEBUG c.g.j.JsonRpcBasicServer:339 [Test worker] served 3 requests, error 1, result JsonError{code=-32002, message='bulk error', data=null} 
2022-06-11 02:03:41,561 DEBUG c.g.j.JsonRpcBasicServer:1011 [Test worker] Response: [{"jsonrpc":"2.0","id":1,"result":"passResult1"},{"jsonrpc":"2.0","id":"null","error":{"code":-32700,"message":"JSON parse error"}},{"jsonrpc":"2.0","id":3,"result":"passResult3"}] 

The behavior was correct in previous 1.5.3 version, but in 1.6.0 server started to output null values for id in responses.
This is caused by the recent change in the error handling logic:
https://github.com/briandilley/jsonrpc4j/blob/1.6.0/src/main/java/com/googlecode/jsonrpc4j/JsonRpcBasicServer.java#L466

cyb3r4nt pushed a commit to cyb3r4nt/jsonrpc4j that referenced this issue Jun 10, 2022
…sion errors.

Server used to output null as id value if some parameter conversion has been failed.
Request id context was lost due to exceptions and null was sent to client.

Fix contains more grained exception handling and restore of initial request context.

Added also tests, which check for expected id result in the responses.
@shubhamChouksey1
Copy link

What is the best way to add validations for the API input parameters ?
Also how to handle the case when the API input parameter is an interface, and several different implementation class are valid input parameters for the API ?

@cyb3r4nt
Copy link
Contributor Author

cyb3r4nt commented Aug 9, 2022

What is the best way to add validations for the API input parameters ?

In my opinion, this framework should not deal with validation of users' business objects.
It may only provide provide deserialization for required types, but validation should be done somewhere else in the users' code.
Also, in my opinion, deserialization and validation should be two different steps: successful receival of the object and validation with proper logging inside the application.

The best option is to manually check the object just after framework has returned control into users' code.
The second option is to use request interceptors, like this one: https://github.com/briandilley/jsonrpc4j/blob/master/src/main/java/com/googlecode/jsonrpc4j/RequestInterceptor.java
There is also possibility to use javax.validation, but this is not quite supported yet.
There is also one open PR which tries to do this, #285.
It is also not quite clear who and where needs to call the javax.validation: jsonrpc4j or Spring framework or users' code.

Current issue is not quite related to the object validation, an it descibes a bug about the JSON-RPC id field and error handling.

Also how to handle the case when the API input parameter is an interface, and several different implementation class are valid input parameters for the API ?

How the framework will know which concrete implementation to inject into users' method?
Is there any good possibility how to express this using JSON objects?
What are you trying to achieve when interface is declared in the method parameters?

When remote methods are called via RPC, then data is exchanged between remote processes.
And framework gives the ability to map that data into some objects.
This data is only some messages between two processes.
It is better if remote logic does not know anything about your interfaces and various implementations.
API contract usually requires some well known parameters, like primitive types and collections, and their composition.

If you really need this, then there are some interceptors provided by this framework, which may transform objects.
Or even better approach to receive the message, as data object, and transform it to required interface and implementation directly in the users' code.

briandilley added a commit that referenced this issue May 24, 2023
#290: Fix JSON-RPC id output in case of method type conversion errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants