Skip to content

fix(methodObject): remove result as required#240

Closed
shanejonas wants to merge 1 commit intomasterfrom
fix/methodobject-remove-result-req
Closed

fix(methodObject): remove result as required#240
shanejonas wants to merge 1 commit intomasterfrom
fix/methodobject-remove-result-req

Conversation

@shanejonas
Copy link
Member

fixes #230

@shanejonas shanejonas self-assigned this Oct 22, 2019
@shanejonas shanejonas requested a review from a team October 22, 2019 17:08
@github-actions github-actions bot added the spec label Oct 22, 2019
<a name="method-externalDocs"></a>externalDocs | [External Documentation Object](#external-documentation-object) | Additional external documentation for this method.
<a name="method-params"></a>params | [[Content Descriptor](#content-descriptor-object) \| [Reference Object](#reference-object) \| [OneOf Object](#oneof-object)] | **REQUIRED**. A list of parameters that are applicable for this method. The list MUST NOT include duplicated parameters and therefore require [name](#content-descriptor-name) to be unique. The list can use the [Reference Object](#reference-object) to link to parameters that are defined by the [Content Descriptor Object](#content-descriptor-object). It may also nest the content descriptor or reference object inside of a [OneOf Object](#oneof-object). All optional params (content descriptor objects with "required": false) MUST be positioned after all required params in the list.
<a name="method-result"></a>result | [Content Descriptor](#content-descriptor-object) \| [Reference Object](#reference-object) \| [OneOf Object](#oneof-object) | **REQUIRED**. The description of the result returned by the method. It MUST be a Content Descriptor.
<a name="method-result"></a>result | [Content Descriptor](#content-descriptor-object) \| [Reference Object](#reference-object) \| [OneOf Object](#oneof-object) | The description of the result returned by the method. It MUST be a Content Descriptor. Not providing a result field indicates a notification.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong to me, and I recommend we reject the PR.
Every method has a result, even if that result is void. In JSON-RPC the client may decide it doesn't care to receive the result by sending a notification (omitting the id in the request), but that's a client decision and nothing to do with the full protocol of the schema the client can expect if it did not send a notification.

Now, if Open-RPC allows a server to document that it only supports notifications for some subset of its methods (that it would reject a request with an ID for those methods) then it makes sense to not have to document a result. Is that the case though, that servers or the Open-RPC spec would support only receiving a particular RPC method request without an id such that a result will never be sent?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AArnott I wrote a comment linked to your suggestion(s). I think you're right and I fully agree with you.

I would suggest to keep result required but allow it to be false. This would indicate that

a server (...) only supports notifications

What to you think about it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of false since the question/field isn't asking for a boolean answer. null might make more sense here. If we want to support notification-only schemas, then omitting result may make sense -- more sense IMO than setting it to false. But specifying it with a null value makes it more explicit and therefore noticeable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as i mentioned here: #230 (comment)

We can already define null results.

Bit I do agree with the criticism that omitting it is implicit vs explicit

@BelfordZ
Copy link
Member

since there is no consensus here, let's continue discussion in this issue: #230

@BelfordZ BelfordZ closed this Nov 19, 2019
@shanejonas
Copy link
Member Author

reopening this in discussion of #230

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How to define a notification ?

5 participants