Error not handled for invalid JSON-RPC 2.0 response Object

[mevir]

If the server response is not a valid json object the following error is thrown:
TypeError: invalid 'in' operand response (line 132 for a simple JSON-RPC response, line 599 for a batch response).
It may also affect the web-sockets JSON-RPC responses (not tested).

If the server response is a valid json but both the "error" and "result" members are missing from response it will be passed as successful with undefined as result (same lines).

[fiddur]

Hmm, if it's not JSON at all, $.ajax should have thrown an error I think, but if it's JSON but not object (array or string) using in will not be correct.

You're right, there could be some more sanity checks of the response.

[mevir]

Thank you for reply Fredrik!
Yes, if it's not JSON at all the error is catch through the error callback of $.ajax.
Here I would call the errorCb with a JSON-RPC Error like object (meaning {code: "code", message: "message"}) instead of {error: "message"} - what do you think?
If it's OK I can take care of fixing the issue ...

[fiddur]

The error handling should really be better structured, but that is for another version as it is a breaking change (and we have another version in working…).

If you want to implement a non-breaking error handling, I would suggest NOT imitating a jsonrpc-error, since you need to know that it isn't from the server but rather is a transport problem, like the timeout error. On the other hand, we need more info…

So, an error object without code could indicate that it's not from the server. To indicate the problem programatically there could be a type instead. So the two problems discussed could be:

{
  type: 'invalid_json',
  text: <original non-json string>
}

and

{
  type: 'invalid_jsonrpc',
  response: <original json decoded response>
}

What do you think?

[mevir]

Hmm ... agree and not agree.
Basically I know I have a JSON-RPC error from the error "code" member value (-32000 to -32099, -32600 to -32603 and -32700, according with specifications).
Another "code" value may be used for http request error handling (non 200 status in response):

error = {
    code: -32800,
    message: jqXHR.statusText
}

and another for invalid response format:

error = {
    code: -32801,
    message:'Invalid JSON-RPC response format'
}

I think it depends on what you would prefer to check in the error object: the code member for value or the existence of a specific member (for instance type / code).
For me it would make sense to write the errorCb like this:

    errorCb: function(error) {
        switch(error.code) {
            case -32800:
                // do the error handling for non-200 http
            break;
            case -32801:
                // do the error handling for invalid JSON-RPC response format
            break;
            ...
        }
    }

rather than:

    errorCb: function(error) {
        if ("type" in error) {
            // do the error handling for non-200 http and invalid JSON-RPC response format
        } else if("code" in error) {
            // do the error handling for valid JSON-RPC error object
        } else {
            // do the error handling for other errors
        }
    }

But again, I think it's a matter of taste ....

[fiddur]

No. All integers are available in jsonrpc-errors. We made that reading-error ourselves at first, but if you read again: "The remainder of the space is available for application defined errors." The application in this case is the one sending the jsonrpc-error, and thereby, the jsonrpc-client can't use that to identify other types of error.

[mevir]

Ha, I missed that ... but the solution is inside:

The error codes are nearly the same as those suggested for XML-RPC at the following url: http://xmlrpc-epi.sourceforge.net/specs/rfc.fault_codes.php

And here we have:

-32300 ---> transport error

which can be used for non-200 status (at least it is already used for XML-RPC non-200 status) and

-32500 ---> application error

which can be used for not valid JSON-RPC responses (it makes sense, if the response is not valid JSON-RPC then the application is in trouble).
just my thoughts ... What do you think?

[fiddur]

It's a nice idea, keeping the same format. But can you be sure that a jsonrpc-server wont use these errors for it's own problems? Like a backend having a transport error from it's database or other external source?

[mevir]

You never can be sure, of course :D. It depends on how the coder reads and understand the specifications (it may be one like me, which missed that part in specifications ;) ).
Let's see:

The error codes from and including -32768 to -32000 are reserved for pre-defined errors. Any code within this range, but not defined explicitly below is reserved for future use.

and

-32000 to -32099 Server error. Reserved for implementation-defined server-errors.

and

The remainder of the space is available for application defined errors.

From my point of view, if I am coding a server side application which is using a JSON-RPC server I MUST never set the error code in the range -32768 to -32000, because these are server reserved.
If I am coding the JSON-RPC server I may use the -32300 error code but this must not be related with a database transport error (for instance), because the server implementation should not be related with any external data; if there is an external source then that is an application and should not set the code to -32300.

I can't see any other reason for the server to set the code to -32300 and the fact that XML-RPC put the code to this value in case of non-200 http response status is supporting this assumption.

The same with the -32500 code: if my application throws an error it must not be in the range -32768 to -32000.

If my application is in trouble and sends garbage to the JSON-RPC server then the server may decide to set the code to -32500; if not (like in my case when the server [I am using https://www.npmjs.com/package/jrpc2] sends an invalid JSON_RPC response) then the client side is free to set the code to -32500 because it's obviously an application error as long I am expecting to get a valid JSON-RPC response object; but either the code is set by the server or by the client the error code meaning is kept.

Does it make sense?

[fiddur]

You do make a compelling argument :) Picking up error codes from xmlrpc is not entirely correct, even if it was an inspiration for jsonrpc. Technically according to the specification, the code is still "reserved for future use".

-32500 seems a bit wrong to pick up. "application error" could be any error within the application, and that's what all non-reserved numbers are.

Rather, if we would accept using -32300, it could be used for all transport errors, which is all errors the client constructs for itself. Otherwise we would need this discussion for every specific error thrown by the client. I can see:

• Invalid JSON in response.
• Invalid JSON-RPC (e.g. no result).
• Timout.
• Batch response that misses some of the ID:s.
• Wrong/unknown ID in response.
• Websocket error.
• Websocket closed.
• Ajax error.

I think I still would suggest coding the above errors with specific strings, but it could be coerced into a transport json-rpc error perhaps:

{
  code: -32300,
  message: 'invalid_json',
  data: {
    text: <original text>,
    …perhaps other debug data…
  }
}

[mevir]

Placing the detailed error description into the data member of the json-rpc error is a great idea!

I kept digging for more information ...
There is an explanation for the missing transport error code from the JSON-RPC 2.0 specifications:

JSON-RPC 2.0 doesn't define any transport-specific issues, since transport and RPC are independent. V1.0 defined that exceptions must be raised if the connection is closed, and that invalid requests/responses must close the connection (and raise exceptions). (from here)

So throwing exceptions may be another approach which will fit better with the specifications but it will force the programmer to try ... catch the rpc call.

[fiddur]

Using the errorCb is the async version of throwing :)

So, I think we agree on using -32300. Good spec for our next version as well. You could make a PR for the cases we had at the top if you like.

[mevir]

Yes, the errorCb is indeed the async version of throwing :)), so what kind of exception to raise??. The author of the specifications is right when he said the transport and the RPC are independent but it seems he miss the fact that there is no RPC without transport.
Good. I've forked the repo and I wil try to implement the conclusions there - we will see if it's OK or not in PR.
I'll also like to know more about the new version - maybe I can help you with both implementing the code or finding specifications ...
All the best :)

Edit: Somebody raised recently these issues in the JSON-RPC discussion group.

[fiddur]

We'll post something about the new version in the beginning of next week. It'll be a new repo, since it doesn't depend on jQuery anymore.