Skip to content

#941 Added SseDelegatingHandler #2300

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

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

vijay-karavadra
Copy link
Contributor

@vijay-karavadra vijay-karavadra commented Jul 13, 2025

Closes #941

Proposed Changes

  • Added SseDelegatingHandler to handle SSE event stream requests

Discussions

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 87.032% (-0.2%) from 87.279%
when pulling caed517 on vijay-karavadra:develop
into 7c583bf on ThreeMammals:develop.

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 87.032% (-0.2%) from 87.279%
when pulling caed517 on vijay-karavadra:develop
into 7c583bf on ThreeMammals:develop.

@vijay-karavadra
Copy link
Contributor Author

@raman-m Please review.

@raman-m raman-m changed the title Added SseDelegatingHandler #941 Added SseDelegatingHandler Jul 14, 2025
@raman-m raman-m requested review from RaynaldM, raman-m and ggnaegi July 14, 2025 12:32
@raman-m raman-m added feature A new feature Core Ocelot Core related or system upgrade (not a public feature) labels Jul 14, 2025
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

To be fair, this is a draft version of the feature. This handler has not yet been integrated into Ocelot's pipeline. We need to ensure the feature is functional by writing acceptance tests that make real HTTP requests to an Ocelot instance.
Could you continue working on this pull request?
Rest assured, we will guide and support you throughout the development process.


namespace Ocelot.Requester
{
public class SseDelegatingHandler : DelegatingHandler
Copy link
Member

Choose a reason for hiding this comment

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

This handler has been designed, but it hasn't been integrated into Ocelot's pipeline yet.

public class SseDelegatingHandlerTests
{
[Fact]
public async Task SendAsync_ForNonSseRequest_CallsBaseHandler()
Copy link
Member

Choose a reason for hiding this comment

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

Having a single unit test is just the first step in comprehensive testing.

@raman-m
Copy link
Member

raman-m commented Jul 14, 2025

@vijay-karavadra commented on July 14, 2025

@raman-m Please review.

No problem! The code review has been completed!

@vijay-karavadra
Copy link
Contributor Author

To be fair, this is a draft version of the feature. This handler has not yet been integrated into Ocelot's pipeline. We need to ensure the feature is functional by writing acceptance tests that make real HTTP requests to an Ocelot instance.
Could you continue working on this pull request?
Rest assured, we will guide and support you throughout the development process.

Sure @raman-m .
I would love to work on it.
Could you share some of the sample acceptance tests for reference?

@raman-m
Copy link
Member

raman-m commented Jul 15, 2025

@vijay-karavadra commented on July 14, 2025

Could you share some of the sample acceptance tests for reference?

Dear Vijay,
Our acceptance testing project can be found here → Ocelot.AcceptanceTests.
As I mentioned earlier, there's nothing to test at the moment since the DelegatingHandler hasn't been integrated into Ocelot's pipeline.
Also, why the DelegatingHandler? Perhaps other solutions are available, and have you considered alternative designs?

  1. The starting point should be configuration, as we are proposing support for a new protocol. However, I don't see any updates in the configuration yet.
  2. I now notice a design challenge with the new SSE protocol, which problem was partially discussed in the Server-sent events protocol #941 (comment).
  3. Finally, will Ocelot's SSE route rely on existing http and https schemes to support SSE traffic, or will a dedicated pipeline be introduced to handle routes with a new sse protocol/scheme?

We need to prioritize discussing this with the team and community before developing any delegating handlers.

I’m marking this PR as a draft since it’s too early to implement the feature, as the idea and design haven’t been thoroughly discussed yet.

@raman-m raman-m marked this pull request as draft July 15, 2025 12:46
@vijay-karavadra
Copy link
Contributor Author

@vijay-karavadra commented on July 14, 2025

Could you share some of the sample acceptance tests for reference?

Dear Vijay, Our acceptance testing project can be found here → Ocelot.AcceptanceTests. As I mentioned earlier, there's nothing to test at the moment since the DelegatingHandler hasn't been integrated into Ocelot's pipeline. Also, why the DelegatingHandler? Perhaps other solutions are available, and have you considered alternative designs?

  1. The starting point should be configuration, as we are proposing support for a new protocol. However, I don't see any updates in the configuration yet.
  2. I now notice a design challenge with the new SSE protocol, which problem was partially discussed in the Server-sent events protocol #941 (comment).
  3. Finally, will Ocelot's SSE route rely on existing http and https schemes to support SSE traffic, or will a dedicated pipeline be introduced to handle routes with a new sse protocol/scheme?

We need to prioritize discussing this with the team and community before developing any delegating handlers.

I’m marking this PR as a draft since it’s too early to implement the feature, as the idea and design haven’t been thoroughly discussed yet.

Ok @raman-m . FYI I tested in the below manner and it worked for me. It works for the https/http. We just need to persist the connection during the communication, so I didn't need to introduce a new DownstreamScheme/protocol.
However I understand if you decide to go with a new scheme then will do it accordingly.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Ocelot Core related or system upgrade (not a public feature) feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server-sent events protocol
3 participants