search

sourcegraph / jsonrpc2

Package jsonrpc2 provides a client and server implementation of JSON-RPC 2.0 (http://www.jsonrpc.org/specification)
MIT License
195 stars 62 forks source link

Omit data field from error when empty #66

Closed samherrmann closed 1 year ago

samherrmann commented 1 year ago

The JSON-RPC 2.0 specification allows the data member of an error object to be omitted. Before this commit, if Error.Data was nil then the JSON encoding was "null". That means that the data member was included in every JSON-RPC error object, even when the data member was not explicitly set.

This commit adds the omitempty struct tag to the Error.Data field, meaning that the data member is omitted from the JSON encoding unless explicitly set with the Error.SetError() method. Beware that this is a breaking change for clients that may have strict null checks on the data member.

samherrmann commented 1 year ago

I don't know how I feel about the inconsistency between this PR and #62. #62 was implemented in a way where params = nil is encoded as null by default, and is only omitted if the OmitNilParams call option is used (for backwards compatibility). Meanwhile, this PR makes the error data member omitted by default. I think if we wanted the data member to continue being encoded as null by default, then we'd have to create a connection option. I feel like such an option would solely exist for backwards compatibility reasons, but I can't think of a reason why someone would deliberately want "data": null to be included in all their error responses that don't actually use the data member. Of course the same argument can be made about the params member in the request.

If we ignored backwards compatibility for a second for the sake of this discussion, then I personally wished that both the params member in the request and the data member in errors were both omitted by default. Why carry that extra weight in the JSON-RPC payload? If that were the default behavior it would still be trivial for someone to explicitly set either of these members to null, without the use of a call and/or connection option. Given that this library has not reached v1.0.0 yet, I wonder if it might be worth dropping the recently added OmitNilParams call option and make the params member also be omitted by default. Yes that would be a breaking change, but might that be the better approach in the long run due to the smaller API footprint, smaller JSON-RPC payload, and consistency? Thoughts?

By the way, I scanned through the JSON-RPC 2.0 spec and it looks like request params and error data are the only "omittable" members. I should have looked into this before I delivered the OmitNilParams call option.

samherrmann commented 1 year ago

I agree with your analysis. So for request params the caller could just not call SetParams, then we would omit the field. This feels very nice. Happy to make that breaking change since I think it fundementally improves the API / makes it less clunky.

Did a quick read of the spec again to refresh my memory, and it all aligns with what you said.

Also LGTM.

Awesome! 🙂 Did you want to merge this PR first then and I'll submit a separate PR for the request params change after? I can also add the commit to this PR, whichever you prefer.

keegancsmith commented 1 year ago

merged, so will merge the next one :)