Avoid NullReferenceException if "params" property is null. #25
Conversation
Avoid NullReferenceException if "params" property is null.
david-driscoll
left a comment
david-driscoll
left a comment
There was a problem hiding this comment.
Few changes that are needed. The thing that is causing this is probably a missing attribute in the client request object
We should add [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] to Params to avoid it from being serialized.
src/JsonRpc/Reciever.cs
Outdated
| object requestId = null; | ||
| bool hasRequestId; | ||
| if (hasRequestId = request.TryGetValue("id", out var id)) | ||
| if (hasRequestId = request.TryGetValue("id", out var id) && requestId != null) |
There was a problem hiding this comment.
This shouldn't change, requestId is allowed to be provided as null. This indicates a request vs notification.
There was a problem hiding this comment.
In that case, shouldn't hasRequestId be false if id == null?
src/JsonRpc/Reciever.cs
Outdated
| } | ||
|
|
||
| var hasParams = request.TryGetValue("params", out var @params); | ||
| var hasParams = request.TryGetValue("params", out var @params) && @params != null; |
There was a problem hiding this comment.
But isn't this the source of the NRE? If hasParams is true but @params is null, then the line below will attempt to check @params.Type and crash, won't it?
// @params can be null even though hasParams is true
if (hasParams && @params.Type != JTokenType.ArrayThere was a problem hiding this comment.
nvm - saw your other comment :)
src/JsonRpc/Reciever.cs
Outdated
|
|
||
| var hasParams = request.TryGetValue("params", out var @params); | ||
| var hasParams = request.TryGetValue("params", out var @params) && @params != null; | ||
| if (hasParams && @params.Type != JTokenType.Array && @params.Type != JTokenType.Object) |
There was a problem hiding this comment.
We should change this line to better align to the spec. params if specified must be an array or object.
Swap with:
if (hasParams && @params?.Type != JTokenType.Array && @params?.Type != JTokenType.Object)
There was a problem hiding this comment.
Ah, ok, you want to use the ?. operator. Sorry, fair enough - I missed that when viewing on my phone, I get you now. Will fix it up shortly :)
The first place I ran into this was when troubleshooting an LPS4j-based client, but yeah - I could easily see various clients making a similar mistake. |
|
I wrote to follow the spec first (as per http://www.jsonrpc.org/specification) but if this comes up again with |
|
Thanks! |
2 participants
Specific fix for #24 (a more general fix will be offered as a separate PR).