Skip to content
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 #847

Merged
merged 43 commits into from
Mar 10, 2023

Conversation

GreenRover
Copy link
Collaborator

@GreenRover GreenRover commented Oct 4, 2022

Title: "Request/Response paradigm support in operation"
Related issue(s): #94 #558
Champion: @GreenRover


Explanation of Problem & Solution:
Very common type of operation is currently missing in specification per discussion in referenced (related) issues. Operation that results in direct or indirect response to the caller.
The most basic example is publishing a message, where the result of publish operation should be known as soon as possible to the caller. Eg. invoking operation on given channel will return the specific message (or oneOf messages) right away to the caller.
The complicated example is correlation of the request and response, where publish/subscribe operation with given message will result in the specific message to be delivered back to the caller via different channel.

Solution has two parts:

  1. Allowing referencing single or multiple messages as a result to operation (indifferent to whether the operation is server or client initiated)
  2. Allowing hinting single or multiple messages that can be expected by other party to be received as a result to using specific message in any kind of operation

Please consume before take part to discussion:

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

examples/kraken-websocket.yml Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
examples/kraken-websocket.yml Outdated Show resolved Hide resolved
@GreenRover GreenRover requested review from fmvilas and removed request for dalelane, derberg and asyncapi-bot-eve October 4, 2022 16:01
@GreenRover GreenRover changed the title #94 request reply pattern feat: request/response support Oct 5, 2022
@GreenRover
Copy link
Collaborator Author

@smarek Do you like to review this pr?

@smarek
Copy link
Contributor

smarek commented Oct 5, 2022

@GreenRover thank you for tagging me, but i've lost interest long time ago, because the whole process (discussion, calls, revisions...) took too long and in the end my proposal was discarded in favor of writing the whole thing over in next major version of AsyncAPI. I wish you all the luck in progressing with this feature.

@GreenRover
Copy link
Collaborator Author

@smarek I know, this is the attempt for the next major. Only wanted to ask you this pr would match all your requirements.

irony: And its is not take this long, we only started discussions 5 month ago.

spec/asyncapi.md Outdated Show resolved Hide resolved
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

left some comments, I'm basically full of doubts 😄

spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
@derberg
Copy link
Member

derberg commented Oct 24, 2022

@GreenRover

Hey, I have few requests:

spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
@GreenRover
Copy link
Collaborator Author

@derberg

  • remove message.type from kraken example
    done.

so for websocket use case replyChannel do not help much, as there is one channel anyway
ok i will remove this but in case of websocket is a correlationId also not überfüll (not added by me)

migrate streetlight-kafak to async api v3
I can not do that i have no idea how to migrate the channel traits. Some one else need to migrate the examples first.

adeo
sorry i dont see there a proper sample. Dont like to add this by my own, this will end in having this pr open for ever.
Just add the samples to the next-major-spect branch and i will add async api related parts.

@GreenRover
Copy link
Collaborator Author

Branch is merge ready again

@derberg
Copy link
Member

derberg commented Nov 9, 2022

@GreenRover

ok i will remove this but in case of websocket is a correlationId also not überfüll (not added by me)

sorry I didn't get it. CorrelationId example was not added originally to Kraken example -> https://www.asyncapi.com/blog/websocket-part2#lets-have-a-look-at-the-final-document

I can not do that i have no idea how to migrate the channel traits. Some one else need to migrate the examples first.

fair point. You contribute a feature to 3.0, but there are other features already and examples are not adjusted accordingly. I forgot about it.

sorry i dont see there a proper sample. Dont like to add this by my own, this will end in having this pr open for ever.
Just add the samples to the next-major-spect branch and i will add async api related parts.

the point is that I would like us to make sure that we merge req/rep when we have it presented in real life examples, to make sure they actually make point. I'm happy to help with Adeo. I know how they workaround req/rep there, and also have direct contact with owners. So I definitely would love to take up a challenge to take Adeo's file and migrate to current 3.0 with reg/repl

@fmvilas
Copy link
Member

fmvilas commented Nov 10, 2022

We had a great conversation in the Spec 3.0 meeting yesterday. If you're interested, here's the recording: https://www.youtube.com/watch?v=koLWWoBnIMo.

Summary of the meeting

We tried many different approaches to define request/reply interactions but we weren't happy with any of them. @GreenRover kindly added a few more examples to this PR so we can improve the discussion here.

Copy link
Collaborator

@char0n char0n left a comment

Choose a reason for hiding this comment

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

Great significant work here!

@char0n
Copy link
Collaborator

char0n commented Mar 8, 2023

I have NOTE regarding description wording:

This PR introduces following sentence (among others)

A list of $ref pointers pointing to the supported

Although this is consistent with other descriptions within the spec (search for "$ref point" string), wouldn't better wording be?:

A list of Reference Objects referencing supported

Reasoning:

We should refer to things as they are defined within the spec. Reference Object is Reference Object, and we should probably not introduce new synonymous nomenclature like $ref pointer. Reference Object references other components and not points to them.

This is something to be discussed in separated issue, if it is agreed that this is something that we want to address.

@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@GreenRover
Copy link
Collaborator Author

@fmvilas Please approve again. Thant i have 3 contributor and can merge.

@fmvilas
Copy link
Member

fmvilas commented Mar 10, 2023

I completely agree, @char0n. Let's do it in a separate PR.

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

Great work, @GreenRover! Feel free to merge now! 🚀

@GreenRover
Copy link
Collaborator Author

/ready-to-merge

@asyncapi-bot asyncapi-bot merged commit d367c23 into asyncapi:next-major-spec Mar 10, 2023
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.0-next-major-spec.10 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@jonaslagoni
Copy link
Member

jonaslagoni commented Mar 10, 2023

😱 It's merged!? 😆

Finally 🔥! Well done @GreenRover and what a patience 😄

@smoya
Copy link
Member

smoya commented Mar 14, 2023

Correct me if I'm wrong @GreenRover.

I noticed we still have this PR pending with changes in JSON Schema files #847
Also, we need to update the parser in https://github.com/asyncapi/parser-js/tree/next-major-spec

Are you going to work on it?

@GreenRover
Copy link
Collaborator Author

@smoya Currently i focus on next spec 3.0 feature, #622.
And waiting for spec.json 3.0. (No idea how i can contribute to spec.json)
Than i plan to contribute to parser-js.
Maybe later to modelina but have a need to get asyncapi-react 3.0 ready soon.
So if you like to boostrap me some how. Please do so.

@jonaslagoni
Copy link
Member

Added an issue for the parser here: asyncapi/parser-js#732

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

Successfully merging this pull request may close these issues.

10 participants