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: support sse protocol #252

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jfallows
Copy link

@jfallows jfallows commented Jun 7, 2024

Description

  • Added sse message binding object
  • Uses http protocol in server definition

Note: we are in the process of testing these proposed changes with zilla open source project to confirm they are suffcient to support sse message validation, etc.

See aklivity/zilla#952

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.

@fmvilas
Copy link
Member

fmvilas commented Jun 8, 2024

Shouldn't this fall under the HTTP binding instead?

sse/README.md Outdated Show resolved Hide resolved
sse/README.md Outdated Show resolved Hide resolved
@jonaslagoni
Copy link
Member

Would users reading the bindings, know that the SSE-specific information is part of the HTTP bindings? Or is it more for convenience in maintaining them? 🤔

I mean leaning more towards keeping them separate I think 🤔

@jfallows
Copy link
Author

jfallows commented Jun 8, 2024

Hey guys, thanks for the feedback.

SSE does have a standard content type for http responses, defining the SSE protocol framing, but the message payload defined by the application is contained in that framing, i.e. spanning one or more data fields per SSE event.

Therefore message schema types for validation need to be described at a higher level than the SSE http response, as SSE has its own logical channel of application defined messages.

@jfallows jfallows requested a review from fmvilas June 12, 2024 22:37
@jfallows jfallows force-pushed the support-sse-protocol branch from 3f91c86 to 75bc30a Compare June 12, 2024 22:39
@fmvilas
Copy link
Member

fmvilas commented Jun 17, 2024

as SSE has its own logical channel of application defined messages

🤔 This wasn't obvious to me at first glance. Can't we map AsyncAPI channels to these logical channel definitions? Cause I have the feeling that's how it should be but speaking from pure gut feeling.

@jfallows
Copy link
Author

as SSE has its own logical channel of application defined messages

🤔 This wasn't obvious to me at first glance. Can't we map AsyncAPI channels to these logical channel definitions? Cause I have the feeling that's how it should be but speaking from pure gut feeling.

Right, the endpoint is identified via URL such as https://example.com/server/sent/events.

Here we propose to identify the logical channel of messages via the request path, such as /server/sent/events above, much like the http and websockets bindings.

@fmvilas
Copy link
Member

fmvilas commented Jun 20, 2024

Then I think http://example.com should be a server definition and /server/sent/events should be the address of the channel. So better to keep it out of the sse binding. What do you think?

@jfallows
Copy link
Author

Then I think http://example.com should be a server definition and /server/sent/events should be the address of the channel. So better to keep it out of the sse binding. What do you think?

https://example.com/server/sent/events is the endpoint URL from the clients point of view, such as via JavaScript in the browser.

var events = new EventSource("https://example.com/server/sent/events");

Are you suggesting that the server definition would use http protocol instead of sse protocol?

If so, then we would have just the Message Binding Object, indicating the sse event type such as message to specify allowed sse message types?

@fmvilas
Copy link
Member

fmvilas commented Jun 20, 2024

Yeah, exactly. That would simplify things IMHO. The server protocol could be http or http+sse. Either way should be fine.

@jfallows
Copy link
Author

Yeah, exactly. That would simplify things IMHO. The server protocol could be http or http+sse. Either way should be fine.

@fmvilas I've gone with http for now, since there are more dimensions to this with secure and http or https seems simpler.

Please let me know if having an sse message binding object without specifying the protocol as sse at server level introduces any downstream issues for validation or parsers for code generation.

@fmvilas
Copy link
Member

fmvilas commented Jun 21, 2024

Yeah now there's nothing in the AsyncAPI document saying that this API MUST use SSE. It only defines that "when using SSE", this message's event type is X. IMHO, there should be something else indicating that an API uses SSE, like server protocol http+sse or https+sse. I'd not allow sse as the server protocol because it lacks information about the HTTP counterpart, for instance, if it's secure (https) or not.

Copy link

This pull request has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Oct 20, 2024
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.

3 participants