open-rpc / spec

The OpenRPC specification
https://spec.open-rpc.org
Apache License 2.0
165 stars 49 forks source link

Is there a more elegant way to define "no result" for a method #321

Closed ricktaylor closed 1 year ago

ricktaylor commented 3 years ago

In my API I have some functions that do not return a result. I implement this by responding with a "null" value (but a "true" is just as good). The reason being that anything apart from success is an error, so I don't need to return anything apart from "there is no error".

E.g. (In pure JSON-RPC)

--> {"jsonrpc": "2.0", "method": "reboot", "id": 1}
<-- {"jsonrpc": "2.0", "result": null, "id": 1}

In the OpenRPC 1.2.6 specification however, a "result" property is required on a method, so I have to provide a name and a description etc, and yes I know I can use a $ref to a '#/components/null_result' to save typing, but it would be much easier to not have "result" required, implying "null".

Note: I am not trying to define JSON-RPC Notifications (no "id" field, no response expected, etc.) just allowing reasonably smart tooling to spot that there is no "result" defined, and just expect/respond with "null".

My question is: are the spec authors happy with the status-quo of requiring an API to define a "null_result" component?

github-actions[bot] commented 3 years ago

Welcome to OpenRPC! Thank you for taking the time to create an issue. Please review the guidelines

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

hholst80 commented 3 years ago

I have exactly the same issue. I cannot describe an JSONRPC 2.0 method that does not provide a result. It seems to me to be an oversight in the spec?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ghostbuster91 commented 2 years ago

According to the spec response is a contentDescriptor so I guess that one could utilize required option for marking a response as optional, although the wording is weird.

On the other hand ethereum spec defines optional responses in terms of allOf with either some object or null.

It would be great to know what is the proper way as I couldn't find any mention of that in the specification.

zcstarr commented 2 years ago

It's kind of a question of intent. If the intent is to be a notification. Then there really isn't a great way to represent that yet.

Otherwise if the intent is to say that you make a request where the response is SomeObject | null

You could use required or allOf to determine the response, I don't think there's any preferred method there. I suppose I would lean towards required if it's just SomeObject | null , if it's more types then allOf might be better. i.e. SomeObject | string | null

Is that what you were looking for? @shanejonas feel free to weigh in

ghostbuster91 commented 2 years ago

Thank you for the response.

It's kind of a question of intent. If the intent is to be a notification. Then there really isn't a great way to represent that yet.

I am not sure I follow.

According to the jsonrpc spec notification is just a request without the id field. Every jsonrpc method should handle both regular requests and notifications. Since notifications are part of the jsonrpc protocol I don't think that we should modify return type definition to take them into account. Note that, server should not respond to a notification request, but this is not always possible - for example in http1.x protocol we have to send something back. However I don't think that this artificial response should be a part of openrpc documentation. JsonRpc is agnostic to the transport layer, and so openrpc documentation should be.

On the other hand we might want to signal to our clients that the response is optional. For example get_blockByNumber might not find a block for the given number. In case of rest api we often signal that by an empty response with 404 error code. While jsonrpc can be used over http it doesn't have to and so we don't have the access on the protocol layer to http codes.

On the jsonrpc level we have two options to encode such state:

Otherwise if the intent is to say that you make a request where the response is SomeObject | null

You could use required or allOf to determine the response, I don't think there's any preferred method there.

Yes, that is my intent.

The openrpc playground doesn't handle required field in case of response's contentDescriptor in any meaningful way. Which leans me toward the statement that allOf is the preferred way. But maybe we should just create an issue in openrpc-playground to handle that case properly?

ricktaylor commented 2 years ago

To clarify, my original request was not for support for notifications, or for responses that may (or may not) have a result, but simply for the case when there is no result ever.

zcstarr commented 2 years ago

@ghostbuster91

According to the jsonrpc spec notification is just a request without the id field. Every jsonrpc method should handle both regular requests and notifications.

The suggestion I was trying to describe was a potential change to allow us to distinguish if a method is a notification or not. Currently our spec requires result field and does not have any affordance for saying something is not expecting a result at all and is therefore a notification.

In case of rest api we often signal that by an empty response with 404 error code. While jsonrpc can be used over http it doesn't have to and so we don't have the access on the protocol layer to http codes.

Agreed definitely don't want to encode ay protocol specific things into the specification. Your options which is throw an error in the JsonRPC specified way or return result null are both valid options.

I would probably error on the side of specificity, returning null result, and reserve error for a case that represents an error. But that's in the eyes of the api implementor.

Thanks for the heads up. I'd say file an issue on open-rpc playground and we'll take a look at it over there. It should be supported, and if it's not or erroring out in some way, happy to take a look and patch it up :D.

BelfordZ commented 2 years ago

@ricktaylor No plans as of right now to make result optional. If we do ever change this, it will be to better support describing notifications (maybe thats where the notification talk came from).

Since the json-rpc spec requires response to have result or error, describing it is essential. If you dont care about the type/structure of the result, you can set schema: true on the contentDescriptor (true is a jsonschema that is equivalent to 'any').

As a general guiding principle, we aim to reduce 'feature multiplicity' as much as possible, providing as few ways to do 1 single thing as possible (jsonschema makes this pretty hard). While I fully understand where you are coming from in your suggestion (easier to write, nicer to read), it introduces a shorthand, or second way to describe the same intent.

BelfordZ commented 2 years ago

@ghostbuster91 In your case, I think you are running up against the limitations of JSON-RPC itself, rather than OpenRPC.

from json-rpc 2.0 spec:

When a rpc call is made, the Server MUST reply with a Response, except for in the case of Notifications.

An empty 404 is not a JSONRPC response, and though it will probably work for most things, is not really to-the-spec json-rpc.

BelfordZ commented 1 year ago

methodObject.result is no longer required, but for a different reason than suggested in this issue. As such I will close this issue.

The correct approach to describing a method whose result is always null is to provide a json-schema that describes the value to be as such.