search

CXuesong / JsonRpc.Standard

An asynchronous .NET Standard library for JSON RPC client & server implementation.
Apache License 2.0
33 stars 16 forks source link

AspnetCore handler doesn't match the 2.0 spec #14

Closed mikedld closed 4 years ago

mikedld commented 4 years ago

The code that determines the HTTP status code seems to only follow the 1.2 spec:

3.6.2 Errors HTTP Status code message
500 -32700 Parse error.
400 -32600 Invalid Request.
404 -32601 Method not found.
500 -32602 Invalid params.
500 -32603 Internal error.
500 -32099..-32000 Server error.

https://github.com/CXuesong/JsonRpc.Standard/blob/2e8cf27fb6b249e6b1f40b8378fc3b26aba44fd6/JsonRpc.AspNetCore/AspNetCoreRpcServerHandler.cs#L97-L111

2.0 spec states that

The status code SHOULD be:

  • 200 OK for responses (both for Response and Error objects)
  • 204 No Response / 202 Accepted for empty responses, i.e. as response to a Notification
  • 307 Temporary Redirect / 308 Permanent Redirect for HTTP-redirections (note that the request may not be automatically retransmitted)
  • 405 Method Not Allowed if HTTP GET is not supported: for all HTTP GET requests if HTTP GET is supported: if the client tries to call a non-safe/non-indempotent method via HTTP GET
  • 415 Unsupported Media Type if the Content-Type is not application/json
  • others for HTTP-errors or HTTP-features outside of the scope of JSON-RPC

And then gives particular examples where both responses containing "result" and "error" return with HTTP status of 200.

Content-Type header value is not up to the spec either, being set to application/json-rpc (per 1.2 spec) and not application/json. This one could be worked around by adjusting the AspNetCoreRpcServerHandler.ResponseContentType property.

CXuesong commented 4 years ago

I think adding a flag in AspNetCoreRpcServerHandler to switch the behavior should solve the problem.

mikedld commented 4 years ago

Sounds good to me, as this would also preserve current behavior for users that don't care that much.

CXuesong commented 4 years ago

After thinking this over, I've decided to go with virtual method instead of using flags, so any user can do whatever customizations on the status code.

And since we are focusing on the implementation of JSON-RPC 2.0, I'll make that as the default behavior. Not sure whether there are many downstream users consuming this ASP.NET Core package, as I've actually spotted a bug in the implementation 😂

https://github.com/CXuesong/JsonRpc.Standard/blob/2e8cf27fb6b249e6b1f40b8378fc3b26aba44fd6/JsonRpc.AspNetCore/AspNetCoreRpcServerHandler.cs#L93

CXuesong commented 4 years ago

Released fix in v0.5.4.

mikedld commented 4 years ago

Thanks for a swift fix!