Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: request/response support in both operation and message objects #594
feat: request/response support in both operation and message objects #594
Changes from 1 commit
a8c808c
81f6fd5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning behind having
response
inside Message Object? Isn't it enough to have it at the operation level?Also, how does it play with the
response
field on the operation? E.g.:What does it mean? Does it mean the one at the operation level prevails over the one at the message level? Maybe both could be expected? If so, how does it work when you have two
oneOf
s on eachresponse
field?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in initial "Identification of potential concerns, challenges, and drawbacks:" number (2).
The reasons are 2
x-response
extension as documented by @derberg in blog websocket-part2About the implementation, if both operation and message level
response
are filled in (even withoneOf
) the parser/interpretation shall evaluate/validate/list all possible messages (merge of bothresponse
oneOf lists). So this design is intentional.This also allows for
operation
level error responses andmessage
level typed responses, eg. as DRY as possibleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't worry about
x-response
, it is just concept, not even official extension.I think having it on operation level is anyway not clear. In the end we talk about reply to a message and not the operation, so intuition automatically tells me to look for it inside the message. It also makes sense for
oneOf
use case, where you have multiple different messages and depending on the message, you can have different reply. This is why I did it this way in my blog post. In the world of WebSocket in cryptocurrency trading APIs, you have one channel, and multiple different messages, when you send messageping
you getpong
in reply, and when you send messagesubscriptionStatus
, you setstatus
message in response. So depending on the message, you get different reply.Keep in mind that my blog post is based on real API, Kraken API, and Gemini in v2 follows the same pattern.
@jonaslagoni how does it work with NATS? what would be better there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derberg each response could in theory be a response to a specific message. There are no limitations there (guess it comes down to implementation) 🙂 So the
response
operation keyword does not make much sense there either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derberg understood, however what i had in mind, was allowing to put the general responses in channel (channel can report authentication, authorization, availability or different status itself, not corresponding to sent message) and message-specific responses linked with message, not channel.
@jonaslagoni i don't really have preference over the naming, so i'm open to suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarek I think what @jonaslagoni meant was that from his perspective and how it worked in NATS is that
response
is enough onmessage
level and not onoperation
level. At least this is my assumption 😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be, but that would require linking all the operation-level responses to all the possible messages that can be processes in that operation/channel, which seemed not-DRY to me, which is also why i proposed
operation response
as deduplication approach.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but when I specify that
pong
is a response toping
and thatsubscriptionStatus
is response tosubscription
I do not validate any DRY principles, right?message can be one or
oneOf
this response on operation level only confuses. If I have just one message, I'll specify it in this one message, but if I haveoneOf
then I will specify it there in every message. When would I do it on operation level?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well for your consideration, which of these seems better to you? (please ignore invalid schema, this is just to illustrate the RFC intent)
just-message
channel and message
In my opinion the channel and message configuration is more clear about response messages belonging to channel operation status and also linking the specific request to one or multiple valid responses. It is not mandatory to define responses on channel or message, but many types/implementations of API are layered (and for good reasons) and individual layers can/may produce responses not relevant to request message but relevant to service/user/operational status/level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I do see your use-case @smarek, I would suggest focusing on what matters here, enabling request/reply, not improving a feature we don't have. Request/reply discussion is complex enough, that adding more into the discussion, won't benefit or focus on what matters 🙂
I find
message
levelresponse
makes the most sense to introduce in this PR, asoperation
level response is more of a nice-to-have feature. I see this because we need to be able to describe for a specific message what the desired response is, yes it is a bit cumbersome to define layer responses, but I think that feature belongs in its own issue, once the core issue has been resolved.What do you think about this? 🙂